maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

On May 04, Oleksandr Byelkin wrote:
> revision-id: d08860b28f3 (mariadb-5.5.67-15-gd08860b28f3)
> parent(s): ac2604f923f
> author: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> committer: Oleksandr Byelkin <sanja@xxxxxxxxxxx>
> timestamp: 2020-04-28 09:16:33 +0200
> message:
> 
> MDEV-22374: VIEW with security definer require FILE privilege from definer not invoker in case of INTO OUTFILE
> 
> Check INTO OUTFILE clause always from invoker.
> 
> ---
>  mysql-test/r/view_grant.result | 21 +++++++++++++++++++++
>  mysql-test/t/view_grant.test   | 33 +++++++++++++++++++++++++++++++++
>  sql/sql_parse.cc               | 18 +++++++++---------
>  3 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/mysql-test/r/view_grant.result b/mysql-test/r/view_grant.result
> index b2d3a0b8ca4..37dc6adc978 100644
> --- a/mysql-test/r/view_grant.result
> +++ b/mysql-test/r/view_grant.result
> @@ -1587,3 +1587,24 @@ USE test;
>  DROP DATABASE mysqltest1;
>  DROP USER 'mysqluser1'@'%';
>  DROP USER 'mysqluser2'@'%';
> +#
> +# MDEV-22374: VIEW with security definer require FILE privilege from definer
> +# not invoker in case of INTO OUTFILE
> +#
> +create user test@localhost;
> +grant select on test.* to test@localhost;
> +create table t1 (a int);
> +create definer=test@localhost sql security definer view v1 as select * from t1;
> +# check that ot works without view

typo "it"

> +select * INTO OUTFILE '../..//tmp/test_out_txt' from t1;
> +# check that ot works without file

same
why "//" ?

> +select * from v1;
> +a
> +# rights for file should be taken from current user not view
> +select * INTO OUTFILE '../../tmp/test_out_txt' from (select count(*) from v1) as dv1;
> +select * INTO OUTFILE '../..//tmp/test_out_txt' from (select * from v1) as dv1;
> +select * INTO OUTFILE '../..//tmp/test_out_txt' from v1;

same

> +drop view v1;

please add reverse tests too:

+create sql security definer view v1 as select * from t1;
+connect(con1, localhost, test);
+error 1045;
+select * INTO OUTFILE '../..//tmp/test_out_txt' from t1;
+select * from v1;
+error 1045;
+select * INTO OUTFILE '../../tmp/test_out_txt' from (select count(*) from v1) as dv1;
+error 1045;
+select * INTO OUTFILE '../..//tmp/test_out_txt' from (select * from v1) as dv1;
+error 1045;
+select * INTO OUTFILE '../..//tmp/test_out_txt' from v1;
+connection default;

> +drop table t1;
> +drop user test@localhost;
> +# End of 5.5 tests
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index ae5a6b4cd35..515990c879f 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -2206,15 +2206,15 @@ mysql_execute_command(THD *thd)
>        lex->exchange != NULL implies SELECT .. INTO OUTFILE and this
>        requires FILE_ACL access.
>      */
> -    ulong privileges_requested= lex->exchange ? SELECT_ACL | FILE_ACL :
> -      SELECT_ACL;
> -
> -    if (all_tables)
> -      res= check_table_access(thd,
> -                              privileges_requested,
> -                              all_tables, FALSE, UINT_MAX, FALSE);
> -    else
> -      res= check_access(thd, privileges_requested, any_db, NULL, NULL, 0, 0);
> +    if (lex->exchange)
> +      res= check_access(thd, FILE_ACL, any_db, NULL, NULL, 0, 0);
> +    if (!res)
> +    {
> +      if (all_tables)
> +        res= check_table_access(thd, SELECT_ACL, all_tables, FALSE, UINT_MAX, FALSE);
> +      else
> +        res= check_access(thd, SELECT_ACL, any_db, NULL, NULL, 0, 0);
> +    }

No, I think this special check for a special SELECT ... INTO OUTFILE
case is wrong. Why does the server even checks global privileges via
views? It should not do it, I think, not for FILE not for any other
global or db level privilege. I'd try something like

diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
--- a/sql/sql_acl.cc
+++ b/sql/sql_acl.cc
@@ -4699,7 +4699,7 @@ bool check_grant(THD *thd, ulong want_access, TABLE_LIST >
       Save a copy of the privileges without the SHOW_VIEW_ACL attribute.
       It will be checked during making view.
     */
-    tl->grant.orig_want_privilege= (want_access & ~SHOW_VIEW_ACL);
+    tl->grant.orig_want_privilege= want_access & ~SHOW_VIEW_ACL & TABLE_ACLS;
   }
 
   mysql_rwlock_rdlock(&LOCK_grant);
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
--- a/sql/sql_parse.cc
+++ b/sql/sql_parse.cc
@@ -5204,7 +5204,8 @@ check_table_access(THD *thd, ulong requirements,TABLE_LIS>
        Register access for view underlying table.
        Remove SHOW_VIEW_ACL, because it will be checked during making view
      */
-    table_ref->grant.orig_want_privilege= (want_access & ~SHOW_VIEW_ACL);
+    table_ref->grant.orig_want_privilege=
+      want_access & ~SHOW_VIEW_ACL & TABLE_ACLS;
 
     if (table_ref->schema_table_reformed)
     {

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx