maria-developers team mailing list archive

[Vicențiu Ciorbaru <vicentiu@xxxxxxxxxxx>]

On Mon, 9 Oct 2017 at 20:45 Sergei Golubchik <serg@xxxxxxxxxxx> wrote:

> 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
those of priv_user@priv_host.

I've added an extra test case for this exact behaviour now too.

Regards,
> Sergei
> Chief Architect MariaDB
> and security@xxxxxxxxxxx
>