← Back to team overview

maria-developers team mailing list archive

Re: PLEASE REVIEW: (MDEV-7574) Security definer views don't work with CONNECT ODBC tables


Hi Sergei,

On 07/24/2015 10:55 PM, Sergei Golubchik wrote:
Hi, Alexander!

On Jul 24, Alexander Barkov wrote:
Hi Sergei,

Sorry for delay, I was busy with 10.1 issues.

That's perfectly fine. And even good - there's little sense to spend
time on 10.0 bugs when the next release in line is 10.1.

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,

Let me try to explain. The idea is very simple, really.

Calling check_access() inside external_lock() is a bad idea:

- it's simply not correct in case of VIEW or SP,
  because it checks privileges for the current INVOKER
  and ignores the "SQL SECURITY DEFINER" clause.

- and by the way, it's slow, and if it's already done in sql_oarse.cc,
  why call it for the  second time? It's nice to reuse the value
  returned in the first call of check_access().

The correct privilege value that takes into account "SQL SECURITY DEFINER" becomes known in sql_parse.cc, in mysql_execute_command()
and its callees, like insert_precheck() etc.

So I'm trying to make this correct privilege value be consistently available in external_lock() for all SQLCOM_XXXX commands by
assigning it to table->grant.privilege.
Before the patch, table->grant.privilege is just set to 0 in many cases
and therefore is useless in external_lock().

In order to initialize table->grant.privilege properly in all cases,
I added a new parameter "ulong privilege" to these functions:


The idea is to make the correct privilege value available at the point
where a TABLE instance is initialized and assign this value to table->grant.privilege instead of 0.

Does this explanation help?

The patch looks quite safe for 10.0 for me:

It does not seem to affect the other code, except ConnectSE.
It should not be harmful to have the real privilege value instead of 0
in table->grant.privilege. Or, can it be harmful somehow?



Follow ups