[Maria-developers] PLEASE REVIEW: (MDEV-7574) Security definer views don't work with CONNECT ODBC tables
Hello Sergei, Please review a patch that fixes the problem described in MDEV-7574. The idea is that a SELECT from a view over a CONNECT table now checks FILE privileges of the view definer (unless CREATE VIEW states SQL SECURITY INVOKER). It looks like a very good idea and gives more possible security options. The administrator can create a VIEW and give access to it to some user, without giving FILE privilege to this user. I'm not really sure about the patch, but it seems to do the trick :) From what I understood, FILE_ACL is written (among the other privileges) into thd->security_ctx.privilege in TABLE_LIST::prepare_security(). In case of a DEFINER view, thd->security_ctx.privilege is filled exactly with the definer privileges, and to the invoker privileges otherwise. So inside ha_connect::check_privileges() the fact that there is FILE_ACL in thd->security_ctx.privilege means that TABLE_LIST::prepare_security() was previously called and FILE_ACL is set to DEFINER or INVOKER, according to the view definition. This is exactly what we need. I'm not sure about the opposite: if there is no FILE_ACL in thd->security_ctx.privilege, what does it mean? Does it mean that there is no FILE_ACL for the effective user? Or can it also mean that TABLE_LIST::prepare_security() was not called? Thanks. On 02/12/2015 04:04 AM, Olivier Bertrand wrote:
Hi Alexander,
Can you take care of this issue? I know you wrote the ha_connect::check_privileges function and I personnally have not the faintest idea about how this must be checked.
Thanks, Olivier
-------- Message transféré -------- Sujet : [JIRA] (MDEV-7574) Security definer views don't work with CONNECT ODBC tables Date : Wed, 11 Feb 2015 23:21:00 +0200 (EET) De : Elena Stepanova (JIRA) <jira@mariadb.atlassian.net> Pour : bertrandop@gmail.com
[https://mariadb.atlassian.net/browse/MDEV-7574?page=com.atlassian.jira.plugi... ]
Elena Stepanova reassigned MDEV-7574: -------------------------------------
Assignee: Olivier Bertrand Fix Version/s: 10.0
Security definer views don't work with CONNECT ODBC tables ----------------------------------------------------------
Key: MDEV-7574 URL:https://mariadb.atlassian.net/browse/MDEV-7574 Project: MariaDB Server Issue Type: Bug Components: Storage Engine - Connect Affects Versions: 10.0.16 Reporter: Geoff Montee Assignee: Olivier Bertrand Labels: connect-engine Fix For: 10.0
One possible way to get around the requirement for having the FILE privilege to access ODBC tables with CONNECT would be to have them called indirectly via a security definer view. However, it does not currently work. Create a security definer view to access the ODBC table, then create a new user: {code} [gmontee@localhost ~]$ mysql -u root tmp Reading table information for completion of table and column names You can turn off this feature to get a quicker startup with -A Welcome to the MariaDB monitor. Commands end with ; or \g. Your MariaDB connection id is 16 Server version: 10.0.15-MariaDB-log MariaDB Server Copyright (c) 2000, 2014, Oracle, SkySQL Ab and others. Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. MariaDB [tmp]> SHOW CREATE TABLE datetime_table; +----------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | Table | Create Table | +----------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | datetime_table | CREATE TABLE `datetime_table` ( `id` int(10) NOT NULL, `modifiedon` datetime DEFAULT NULL ) ENGINE=CONNECT DEFAULT CHARSET=latin1 CONNECTION='DSN=connect_test_azure;UID=connect_test;PWD=Password1' `TABLE_TYPE`='ODBC' `TABNAME`='dbo.datetime_table' | +----------------+-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ 1 row in set (0.00 sec) MariaDB [tmp]> DROP USER 'connecttest'@'localhost'; Query OK, 0 rows affected (0.00 sec) MariaDB [tmp]> CREATE OR REPLACE -> DEFINER = CURRENT_USER -> SQL SECURITY DEFINER -> VIEW datetime_view -> AS SELECT * FROM datetime_table; Query OK, 0 rows affected (0.00 sec) MariaDB [tmp]> CREATE USER 'connecttest'@'localhost'; Query OK, 0 rows affected (0.00 sec) MariaDB [tmp]> GRANT SELECT ON datetime_view TO 'connecttest'@'localhost'; Query OK, 0 rows affected (0.00 sec) MariaDB [tmp]> \q Bye {code} Now connect with the new user, and try to use the view: {code} [gmontee@localhost ~]$ mysql -u connecttest tmp Reading table information for completion of table and column names You can turn off this feature to get a quicker startup with -A Welcome to the MariaDB monitor. Commands end with ; or \g. Your MariaDB connection id is 17 Server version: 10.0.15-MariaDB-log MariaDB Server Copyright (c) 2000, 2014, Oracle, SkySQL Ab and others. Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. MariaDB [tmp]> SELECT * FROM datetime_view; ERROR 1045 (28000): Access denied for user 'connecttest'@'localhost' (using password: NO) MariaDB [tmp]> \q Bye {code} It didn't work, so give the user privileges on the underlying ODBC table: {code} [gmontee@localhost ~]$ mysql -u root tmp Reading table information for completion of table and column names You can turn off this feature to get a quicker startup with -A Welcome to the MariaDB monitor. Commands end with ; or \g. Your MariaDB connection id is 18 Server version: 10.0.15-MariaDB-log MariaDB Server Copyright (c) 2000, 2014, Oracle, SkySQL Ab and others. Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. MariaDB [tmp]> GRANT FILE ON *.* TO 'connecttest'@'localhost'; Query OK, 0 rows affected (0.00 sec) MariaDB [tmp]> GRANT SELECT ON datetime_table TO 'connecttest'@'localhost'; Query OK, 0 rows affected (0.00 sec) MariaDB [tmp]> \q Bye {code} Now try using the view again: {code} [gmontee@localhost ~]$ mysql -u connecttest tmp Reading table information for completion of table and column names You can turn off this feature to get a quicker startup with -A Welcome to the MariaDB monitor. Commands end with ; or \g. Your MariaDB connection id is 19 Server version: 10.0.15-MariaDB-log MariaDB Server Copyright (c) 2000, 2014, Oracle, SkySQL Ab and others. Type 'help;' or '\h' for help. Type '\c' to clear the current input statement. MariaDB [tmp]> SELECT * FROM datetime_view; +----+---------------------+ | id | modifiedon | +----+---------------------+ | 1 | 2014-01-01 00:00:00 | | 2 | 2016-01-01 00:00:00 | +----+---------------------+ 2 rows in set (0.24 sec) {code}
-- This message was sent by Atlassian JIRA (v6.4-OD-14-082#64012)
Hi, Alexander! On Feb 17, Alexander Barkov wrote:
From what I understood, FILE_ACL is written (among the other privileges) into thd->security_ctx.privilege in TABLE_LIST::prepare_security(). In case of a DEFINER view, thd->security_ctx.privilege is filled exactly with the definer privileges, and to the invoker privileges otherwise.
So inside ha_connect::check_privileges() the fact that there is FILE_ACL in thd->security_ctx.privilege means that TABLE_LIST::prepare_security() was previously called and FILE_ACL is set to DEFINER or INVOKER, according to the view definition. This is exactly what we need.
Agree, looks good so far :)
I'm not sure about the opposite: if there is no FILE_ACL in thd->security_ctx.privilege, what does it mean? Does it mean that there is no FILE_ACL for the effective user? Or can it also mean that TABLE_LIST::prepare_security() was not called?
As far as I can see, TABLE_LIST::prepare_security() is always called for a view that was successfully opened. That is, no FILE_ACL bit means that there is no FILE privilege.
=== modified file 'storage/connect/ha_connect.cc' --- storage/connect/ha_connect.cc 2015-01-20 00:21:56 +0000 +++ storage/connect/ha_connect.cc 2015-02-17 13:46:34 +0000 @@ -3865,6 +3865,8 @@ bool ha_connect::check_privileges(THD *t case TAB_MAC: case TAB_WMI: case TAB_OEM: + if (table && (table->grant.privilege & FILE_ACL)) + return false; return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0);
I don't like that you use both approaches to check the privileges. This just looks wrong. It should rather be * if it's called from ::external_lock() - use table->grant.privilege. * otherwise (::create() or ::delete_or_rename_table()) - don't use table->grant.privilege, only use check_access(). Something like if (called_from_external_lock) return table->grant.privilege & FILE_ACL; // respect view's definer else return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0); Regards, Sergei
Hi Sergei, On 02/19/2015 07:12 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Feb 17, Alexander Barkov wrote:
From what I understood, FILE_ACL is written (among the other privileges) into thd->security_ctx.privilege in TABLE_LIST::prepare_security(). In case of a DEFINER view, thd->security_ctx.privilege is filled exactly with the definer privileges, and to the invoker privileges otherwise.
So inside ha_connect::check_privileges() the fact that there is FILE_ACL in thd->security_ctx.privilege means that TABLE_LIST::prepare_security() was previously called and FILE_ACL is set to DEFINER or INVOKER, according to the view definition. This is exactly what we need.
Agree, looks good so far :)
I'm not sure about the opposite: if there is no FILE_ACL in thd->security_ctx.privilege, what does it mean? Does it mean that there is no FILE_ACL for the effective user? Or can it also mean that TABLE_LIST::prepare_security() was not called?
As far as I can see, TABLE_LIST::prepare_security() is always called for a view that was successfully opened. That is, no FILE_ACL bit means that there is no FILE privilege.
=== modified file 'storage/connect/ha_connect.cc' --- storage/connect/ha_connect.cc 2015-01-20 00:21:56 +0000 +++ storage/connect/ha_connect.cc 2015-02-17 13:46:34 +0000 @@ -3865,6 +3865,8 @@ bool ha_connect::check_privileges(THD *t case TAB_MAC: case TAB_WMI: case TAB_OEM: + if (table && (table->grant.privilege & FILE_ACL)) + return false; return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0);
I don't like that you use both approaches to check the privileges. This just looks wrong. It should rather be
* if it's called from ::external_lock() - use table->grant.privilege. * otherwise (::create() or ::delete_or_rename_table()) - don't use table->grant.privilege, only use check_access().
Something like
if (called_from_external_lock) return table->grant.privilege & FILE_ACL; // respect view's definer else return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0);
Thanks for the suggestion! Please find a new version attached. It now uses thd_sql_command() and lock_type to decide whether to use table->grant.privilege or check_access(). There is only one problem with that. In case of embedded server table->grant.privilege is always 0, because the embedded version of check_table_access() is just an empty function. This change in sql/handler.cc, in handler::ha_external_lock() helps: +#ifdef NO_EMBEDDED_ACCESS_CHECKS + table->grant.privilege= ~NO_ACCESS; +#endif With this change "mtr --embedded" passes all tests, including those I added in this patch. Possibly, a there is a better way to do this. Please suggest. Thanks.
Regards, Sergei
Hi, Alexander! On Feb 24, Alexander Barkov wrote:
There is only one problem with that. In case of embedded server table->grant.privilege is always 0, because the embedded version of check_table_access() is just an empty function.
This change in sql/handler.cc, in handler::ha_external_lock() helps:
+#ifdef NO_EMBEDDED_ACCESS_CHECKS + table->grant.privilege= ~NO_ACCESS; +#endif
May be, it'd be better to do it in check_table_access() ? With a comment "plugins (e.g. CONNECT engine) should not depend on whether embedded is built with NO_EMBEDDED_ACCESS_CHECKS or without".
=== modified file 'sql/handler.cc' --- sql/handler.cc 2015-01-21 11:03:02 +0000 +++ sql/handler.cc 2015-02-24 11:47:05 +0000 @@ -5873,6 +5873,9 @@ int handler::ha_external_lock(THD *thd,
ha_statistic_increment(&SSV::ha_external_lock_count);
+#ifdef NO_EMBEDDED_ACCESS_CHECKS + table->grant.privilege= ~NO_ACCESS; +#endif
See above. If you could't put this in check_table_access(), then, at least, add this comment here.
/* We cache the table flags if the locking succeeded. Otherwise, we keep them as they were when they were fetched in ha_open().
=== modified file 'storage/connect/ha_connect.cc' --- storage/connect/ha_connect.cc 2015-02-11 20:39:41 +0000 +++ storage/connect/ha_connect.cc 2015-02-24 12:03:25 +0000 @@ -3922,7 +3922,21 @@ int ha_connect::delete_all_rows() } // end of delete_all_rows
-bool ha_connect::check_privileges(THD *thd, PTOS options, char *dbn) +/** + Check privileges. + @param THD - Current thread + @param options - Connect table options + @param dbn - database name + @param using_table_privilege - whether check table->grant.privilege, + or execute check_access(FILE_ACL). + + Using table->grant.privilege is important in cases when we need to take into + account privileges of the VIEW definer when accessing to a view created with + "CREATE VIEW v1 SQL SECURITY DEFINER". + See ha_connect::check_privileges_external_lock() for details. +*/ +bool ha_connect::check_privileges(THD *thd, PTOS options, + char *dbn, bool using_table_privilege) { const char *db= (dbn && *dbn) ? dbn : NULL; TABTYPE type=GetRealType(options); @@ -4143,6 +4180,67 @@ MODE ha_connect::CheckMode(PGLOBAL g, TH return newmode; } // end of check_mode
+ +/** + A check_privilege() wrapper for external_lock(). + Decides if check_privilege(): + - should test table->grant.privilege for FILE_ACL + - or should call check_access(FILE_ACL) + depending on the current SQL command and lock type. +*/ +bool ha_connect::check_privileges_external_lock(PGLOBAL g, THD *thd, + PTOS options, int lock_type) +{ + bool use_table_priv; + switch (thd_sql_command(thd)) + { + case SQLCOM_SELECT: + case SQLCOM_UPDATE: + case SQLCOM_INSERT: + case SQLCOM_DELETE: + case SQLCOM_REPLACE: + case SQLCOM_LOAD: + use_table_priv= true; // use table->grant.privilege + break; + + case SQLCOM_CREATE_TABLE: + case SQLCOM_INSERT_SELECT: + case SQLCOM_REPLACE_SELECT: + case SQLCOM_UPDATE_MULTI: + case SQLCOM_DELETE_MULTI: + /* + CREATE TABLE target_table AS SELECT * FROM source_table; + INSERT INTO target_table SELECT * FROM source_table; + REPLACE INTO target_table SELECT * FROM source_table; + UPDATE target_table,source_table SET target_table.column=xxx WHERE ...; + DELETE target_table FROM target_table,source_table WHERE ...; + + If we're working with "source_table", use table->grant.privilege. + If we're working with "target_table", use check_access(). + */ + use_table_priv= lock_type != F_WRLCK;
I don't quite understand that. Why use_table_priv is FALSE for these commands? Like, why it's true for SQLCOM_INSERT, but false for SQLCOM_INSERT_SELECT? True for SQLCOM_UPDATE, false for SQLCOM_UPDATE_MULTI? Other cases below aren't clear either. Could you explain the rule - when one should use table->grant.privilege and when check_access()? I mean, not a list of cases, but a general underlying rule.
+ break; + + case SQLCOM_TRUNCATE: + case SQLCOM_LOCK_TABLES: + case SQLCOM_DROP_TABLE: + case SQLCOM_RENAME_TABLE: + case SQLCOM_CREATE_VIEW: + case SQLCOM_DROP_VIEW: + case SQLCOM_ALTER_TABLE: + case SQLCOM_DROP_INDEX: + case SQLCOM_CREATE_INDEX: + case SQLCOM_OPTIMIZE: + use_table_priv= false; // use check_access() + break; + default: + report_unsupported_sql_command(g, thd); + return true; // Something went wrong, deny access. + } + return check_privileges(thd, options, table->s->db.str, use_table_priv); +} + + int ha_connect::start_stmt(THD *thd, thr_lock_type lock_type) { int rc= 0; @@ -4614,7 +4712,7 @@ int ha_connect::delete_or_rename_table(c if (!open_table_def(thd, share)) { // Now we can work if ((pos= share->option_struct)) { - if (check_privileges(thd, pos, db)) + if (check_privileges(thd, pos, db, false)) rc= HA_ERR_INTERNAL_ERROR; // ??? else if (IsFileType(GetRealType(pos)) && !pos->filename) @@ -5592,7 +5690,7 @@ int ha_connect::create(const char *name, DBUG_RETURN(HA_ERR_INTERNAL_ERROR); } // endif ttp
- if (check_privileges(thd, options, GetDBfromName(name))) + if (check_privileges(thd, options, GetDBfromName(name), false)) DBUG_RETURN(HA_ERR_INTERNAL_ERROR);
inward= IsFileType(type) && !options->filename;
Regards, Sergei
Hi Sergei, Sorry for delay, I was busy with 10.1 issues. 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. Please see comments below. On 04/29/2015 06:44 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Feb 24, Alexander Barkov wrote:
There is only one problem with that. In case of embedded server table->grant.privilege is always 0, because the embedded version of check_table_access() is just an empty function.
This change in sql/handler.cc, in handler::ha_external_lock() helps:
+#ifdef NO_EMBEDDED_ACCESS_CHECKS + table->grant.privilege= ~NO_ACCESS; +#endif
May be, it'd be better to do it in check_table_access() ?
With a comment "plugins (e.g. CONNECT engine) should not depend on whether embedded is built with NO_EMBEDDED_ACCESS_CHECKS or without".
Thanks for the idea. Note, check_table_access() is not called in case of some SQLCOM_XXXX. So I slightly extended your idea and added this code into st_select_lex::add_table_to_list() instead. This is the place where TABLE_LIST is first initialized.
=== modified file 'sql/handler.cc' --- sql/handler.cc 2015-01-21 11:03:02 +0000 +++ sql/handler.cc 2015-02-24 11:47:05 +0000 @@ -5873,6 +5873,9 @@ int handler::ha_external_lock(THD *thd,
ha_statistic_increment(&SSV::ha_external_lock_count);
+#ifdef NO_EMBEDDED_ACCESS_CHECKS + table->grant.privilege= ~NO_ACCESS; +#endif
See above. If you could't put this in check_table_access(), then, at least, add this comment here.
/* We cache the table flags if the locking succeeded. Otherwise, we keep them as they were when they were fetched in ha_open().
=== modified file 'storage/connect/ha_connect.cc' --- storage/connect/ha_connect.cc 2015-02-11 20:39:41 +0000 +++ storage/connect/ha_connect.cc 2015-02-24 12:03:25 +0000 @@ -3922,7 +3922,21 @@ int ha_connect::delete_all_rows() } // end of delete_all_rows
-bool ha_connect::check_privileges(THD *thd, PTOS options, char *dbn) +/** + Check privileges. + @param THD - Current thread + @param options - Connect table options + @param dbn - database name + @param using_table_privilege - whether check table->grant.privilege, + or execute check_access(FILE_ACL). + + Using table->grant.privilege is important in cases when we need to take into + account privileges of the VIEW definer when accessing to a view created with + "CREATE VIEW v1 SQL SECURITY DEFINER". + See ha_connect::check_privileges_external_lock() for details. +*/ +bool ha_connect::check_privileges(THD *thd, PTOS options, + char *dbn, bool using_table_privilege) { const char *db= (dbn && *dbn) ? dbn : NULL; TABTYPE type=GetRealType(options); @@ -4143,6 +4180,67 @@ MODE ha_connect::CheckMode(PGLOBAL g, TH return newmode; } // end of check_mode
+ +/** + A check_privilege() wrapper for external_lock(). + Decides if check_privilege(): + - should test table->grant.privilege for FILE_ACL + - or should call check_access(FILE_ACL) + depending on the current SQL command and lock type. +*/ +bool ha_connect::check_privileges_external_lock(PGLOBAL g, THD *thd, + PTOS options, int lock_type) +{ + bool use_table_priv; + switch (thd_sql_command(thd)) + { + case SQLCOM_SELECT: + case SQLCOM_UPDATE: + case SQLCOM_INSERT: + case SQLCOM_DELETE: + case SQLCOM_REPLACE: + case SQLCOM_LOAD: + use_table_priv= true; // use table->grant.privilege + break; + + case SQLCOM_CREATE_TABLE: + case SQLCOM_INSERT_SELECT: + case SQLCOM_REPLACE_SELECT: + case SQLCOM_UPDATE_MULTI: + case SQLCOM_DELETE_MULTI: + /* + CREATE TABLE target_table AS SELECT * FROM source_table; + INSERT INTO target_table SELECT * FROM source_table; + REPLACE INTO target_table SELECT * FROM source_table; + UPDATE target_table,source_table SET target_table.column=xxx WHERE ...; + DELETE target_table FROM target_table,source_table WHERE ...; + + If we're working with "source_table", use table->grant.privilege. + If we're working with "target_table", use check_access(). + */ + use_table_priv= lock_type != F_WRLCK;
I don't quite understand that. Why use_table_priv is FALSE for these commands? Like, why it's true for SQLCOM_INSERT, but false for SQLCOM_INSERT_SELECT? True for SQLCOM_UPDATE, false for SQLCOM_UPDATE_MULTI?
Other cases below aren't clear either. Could you explain the rule - when one should use table->grant.privilege and when check_access()? I mean, not a list of cases, but a general underlying rule.
The main problem was that the result of check_access(), which is done in sql_parse.cc through a number of various SQLCOM_XXX dependent functions (e.g. insert_precheck), and which extracts the privileges for the current effective user (which can be invoker or definer, depending on SQL SECURITY clause, e.g. in case of VIEW), was not always available in external_lock() in table.grant->privileges (which was just 0 in some case) So I composed this code heuristically, just testing what works. The goal was: - to reuse table.grant->privileges when it has a valid value that originates from check_access() made in sql_parse.cc - to call its own check_access() when table.grant->privileges is just set to 0. Of course, using check_access() could be be wrong, because it could check privileges for invoker instead of definer in some case. This problem would be important for calling the affected statements from stored procedures. There were no general rule, because availability of the check_access() result heavily depended on the exact SQLCOM_XXXX command. So that was just a switch() that worked somehow. Now I rewrote the code to make the original check_access() result be available in much more cases and extended the test a lot. So external_lock() now uses table.grant->privilege for all SQLCOM_XXXX. Note, check_access() is still called in ha_connect::create() and ha_connect::delete_or_rename_table(). I'd like to get rid of this eventually, in a separate change. You'll find related comments in the patch. Thanks.
+ break; + + case SQLCOM_TRUNCATE: + case SQLCOM_LOCK_TABLES: + case SQLCOM_DROP_TABLE: + case SQLCOM_RENAME_TABLE: + case SQLCOM_CREATE_VIEW: + case SQLCOM_DROP_VIEW: + case SQLCOM_ALTER_TABLE: + case SQLCOM_DROP_INDEX: + case SQLCOM_CREATE_INDEX: + case SQLCOM_OPTIMIZE: + use_table_priv= false; // use check_access() + break; + default: + report_unsupported_sql_command(g, thd); + return true; // Something went wrong, deny access. + } + return check_privileges(thd, options, table->s->db.str, use_table_priv); +} + + int ha_connect::start_stmt(THD *thd, thr_lock_type lock_type) { int rc= 0; @@ -4614,7 +4712,7 @@ int ha_connect::delete_or_rename_table(c if (!open_table_def(thd, share)) { // Now we can work if ((pos= share->option_struct)) { - if (check_privileges(thd, pos, db)) + if (check_privileges(thd, pos, db, false)) rc= HA_ERR_INTERNAL_ERROR; // ??? else if (IsFileType(GetRealType(pos)) && !pos->filename) @@ -5592,7 +5690,7 @@ int ha_connect::create(const char *name, DBUG_RETURN(HA_ERR_INTERNAL_ERROR); } // endif ttp
- if (check_privileges(thd, options, GetDBfromName(name))) + if (check_privileges(thd, options, GetDBfromName(name), false)) DBUG_RETURN(HA_ERR_INTERNAL_ERROR);
inward= IsFileType(type) && !options->filename;
Regards, Sergei
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, please? Regards, Sergei
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, please?
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: ha_create_table() open_table_uncached() open_table_from_share(). 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? Thanks.
Regards, Sergei
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 :) 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; See the commit mail with the complete patch. Regards, Sergei
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
Hi, Alexander! On Jul 26, Alexander Barkov wrote:
+ 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;
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.
Your first patch was if (table && table->grant.privilege & FILE_ACL) return false; return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0); that is, it tried both table->grant.privilege and check_access(). We agreed that it's wrong, but I wanted a test case for it.
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.
The patch is Ok. Thanks for help with this. Can you please push this?
Sure. Thanks! But I'll add your SP test case too. Regards, Sergei
participants (2)
-
Alexander Barkov -
Sergei Golubchik