maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

Hi Sergei,


On 07/26/2015 02:13 AM, Sergei Golubchik wrote:
> Hi, Alexander!
>
> On Jul 24, Alexander Barkov wrote:
> > > > Thanks for review. A new patch is attached.
> > > > This is a major rewrite since last time.
> > > > I think the code now looks much easier to understand.
> > >
> > > Not really, I couldn't understand what you're trying to do this time :(
> > > Besides it's 32K of changes in 10.0-GA, and for what? Corner case
> > > CONNECT issue. Could you try to come up with a less intrusive solution,
> > > please?
> >
> > Let me try to explain. The idea is very simple, really.
> ...
> > Does this explanation help?
>
> Yes, thanks. But it still affects the rather hot execution path -
> opening of a table - for every user and every table. While it is only
> needed when a user opens a CONNECT table via a view with SQL SECURITY
> DEFINER. See? In the absolute majority of cases, practically all of
> them, your patch only adds an overhead.
>
> Instead, I suggest another patch - this is based on your first fix for
> this bug:
>
> diff --git a/storage/connect/ha_connect.cc b/storage/connect/ha_connect.cc
> index c2fb648..0a024b0 100644
> --- a/storage/connect/ha_connect.cc
> +++ b/storage/connect/ha_connect.cc
> @@ -4020,7 +4020,24 @@ bool ha_connect::check_privileges(THD *thd, PTOS options,
>       case TAB_MAC:
>       case TAB_WMI:
>       case TAB_OEM:
> -      return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0);
> +      /*
> +        If table or table->mdl_ticket is NULL - it's a DLL, e.g. CREATE TABLE.
> +        if the table has an MDL_EXCLUSIVE lock - it's a DDL too, e.g. the
> +        insert step of CREATE ... SELECT.
> +
> +        Otherwise it's a DML, the table was normally opened, locked,
> +        privilege were already checked, and table->grant.privilege is set.
> +        With SQL SECURITY DEFINER, table->grant.privilege has definer's privileges.
> +      */
> +      if (!table || !table->mdl_ticket || table->mdl_ticket->get_type() == MDL_EXCLUSIVE)
> +        return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0);
> +      if (table->grant.privilege & FILE_ACL)
> +        return false;
> +      return true;
>
>       // This is temporary until a solution is found
>       case TAB_TBL:
>
> It passes your test case. In fact, your first fix passes it too :)

Yeah, I guess my condition that switches between checking
grant.privilege and doing check_access() was effectively the same.
But your version looks simpler.


> This one also passes the additional test I've added - where a user can
> access the table, but view's definer cannot:
>
>    --connection default
>    CREATE DEFINER=user@localhost SQL SECURITY DEFINER VIEW v1_baddefiner AS SELECT * FROM t1;
>    --error ER_ACCESS_DENIED_ERROR
>    SELECT * FROM v1_baddefiner;

This is a nice idea. Thanks.

I just tried this:

# Run this as root:
DROP TABLE IF EXISTS t1;
DROP PROCEDURE IF EXISTS p1;
CREATE PROCEDURE p1() SQL SECURITY DEFINER
  CREATE TABLE t1 (a INT) ENGINE=CONNECT TABLE_TYPE=fix FILE_NAME='t1.fix';

# Run this as a user with no FILE_ACL
CALL p1();

and it also worked as expected, CALL p1() succeeded.

> See the commit mail with the complete patch.

The patch is Ok. Thanks for help with this.

Can you please push this?

I'm on vacation starting from tomorrow.

Thanks.

> Regards,
> Sergei