Re: [Maria-developers] 46b93fc: MDEV-13676: Field "create Procedure" is NULL, even if the the user has role which is the definer. (SHOW CREATE PROCEDURE)
Hi, Vicentiu! Looks good! A couple of comments, see below. Ok to push after fixing, the second review is not needed. On Oct 09, Vicentiu Ciorbaru wrote:
revision-id: 46b93fc2a2fa198d721813d9b93bbe09d2e36eb3 (mariadb-10.0.32-50-g46b93fc) parent(s): dbeffabc83ed01112e09d7e782d44f044cfcb691 author: Vicențiu Ciorbaru committer: Vicențiu Ciorbaru timestamp: 2017-10-09 13:32:40 +0300 message:
MDEV-13676: Field "create Procedure" is NULL, even if the the user has role which is the definer. (SHOW CREATE PROCEDURE)
During show create procedure we ommited to check the current role, if it is the actual definer of the procedure. In addition, we should support indirectly granted roles to the current role. Implemented a recursive lookup to search the tree of grants if the rolename is present.
We both had to check the SQL standard to see what the behavior should be. Please add here (in the commit comment) a reference to the exact part of the standard that specifies this behavior. Like SQL Standard 2016, Part ... Section ... View I_S.ROUTINES selects ROUTINE_BODY and its WHERE clause says that the GRANTEE must be either PUBLIC, or CURRENT_USER or in the ENABLED_ROLES.
diff --git a/sql/sp_head.cc b/sql/sp_head.cc index ea9e1c1..3dd1a65 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -2588,10 +2588,18 @@ bool check_show_routine_access(THD *thd, sp_head *sp, bool *full_access) *full_access= ((!check_table_access(thd, SELECT_ACL, &tables, FALSE, 1, TRUE) && (tables.grant.privilege & SELECT_ACL) != 0) || + /* Check if user owns the routine. */ (!strcmp(sp->m_definer_user.str, thd->security_ctx->priv_user) && !strcmp(sp->m_definer_host.str, - thd->security_ctx->priv_host))); + thd->security_ctx->priv_host)) || + /* Check if current role or any of the sub-granted roles + own the routine. */ + (sp->m_definer_host.length == 0 && + (!strcmp(sp->m_definer_user.str, + thd->security_ctx->priv_role) || + check_role_is_granted(thd->security_ctx->priv_role, NULL, + sp->m_definer_user.str))));
Perhaps you could simplify the condition above a little bit, by replacing it with /* Check if current role or any of the sub-granted roles own the routine. */ (sp->m_definer_host.length == 0 && check_role_is_granted(thd->security_ctx->priv_user, thd->security_ctx->priv_host, sp->m_definer_user.str))); Regards, Sergei Chief Architect MariaDB and security@mariadb.org
Hi, Vicentiu!
Looks good! A couple of comments, see below. Ok to push after fixing, the second review is not needed.
On Oct 09, Vicentiu Ciorbaru wrote:
revision-id: 46b93fc2a2fa198d721813d9b93bbe09d2e36eb3 (mariadb-10.0.32-50-g46b93fc) parent(s): dbeffabc83ed01112e09d7e782d44f044cfcb691 author: Vicențiu Ciorbaru committer: Vicențiu Ciorbaru timestamp: 2017-10-09 13:32:40 +0300 message:
MDEV-13676: Field "create Procedure" is NULL, even if the the user has role which is the definer. (SHOW CREATE PROCEDURE)
During show create procedure we ommited to check the current role, if it is the actual definer of the procedure. In addition, we should support indirectly granted roles to the current role. Implemented a recursive lookup to search the tree of grants if the rolename is present.
We both had to check the SQL standard to see what the behavior should be. Please add here (in the commit comment) a reference to the exact part of the standard that specifies this behavior. Like
SQL Standard 2016, Part ... Section ... View I_S.ROUTINES selects ROUTINE_BODY and its WHERE clause says that the GRANTEE must be either PUBLIC, or CURRENT_USER or in the ENABLED_ROLES.
diff --git a/sql/sp_head.cc b/sql/sp_head.cc index ea9e1c1..3dd1a65 100644 --- a/sql/sp_head.cc +++ b/sql/sp_head.cc @@ -2588,10 +2588,18 @@ bool check_show_routine_access(THD *thd, sp_head *sp, bool *full_access) *full_access= ((!check_table_access(thd, SELECT_ACL, &tables, FALSE, 1, TRUE) && (tables.grant.privilege & SELECT_ACL) != 0) || + /* Check if user owns the routine. */ (!strcmp(sp->m_definer_user.str, thd->security_ctx->priv_user) && !strcmp(sp->m_definer_host.str, - thd->security_ctx->priv_host))); + thd->security_ctx->priv_host)) || + /* Check if current role or any of the sub-granted roles + own the routine. */ + (sp->m_definer_host.length == 0 && + (!strcmp(sp->m_definer_user.str, + thd->security_ctx->priv_role) || + check_role_is_granted(thd->security_ctx->priv_role, NULL, + sp->m_definer_user.str))));
Perhaps you could simplify the condition above a little bit, by replacing it with
/* Check if current role or any of the sub-granted roles own the routine. */ (sp->m_definer_host.length == 0 && check_role_is_granted(thd->security_ctx->priv_user, thd->security_ctx->priv_host, sp->m_definer_user.str)));
I fixed the other change, but the logic you propose here is slightly different. Your version will check all APPLICABLE_ROLES, not only ENABLED_ROLES. We need to look at the subgraph of thd->priv_role only, not
On Mon, 9 Oct 2017 at 20:45 Sergei Golubchik
Sergei Chief Architect MariaDB and security@mariadb.org
participants (2)
-
Sergei Golubchik -
Vicențiu Ciorbaru