Re: [PATCH v3 01/11] user_ns: 3 new LSM hooks for user namespace operations

[Paul Moore]

On Mon, Aug 3, 2015 at 9:38 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Mon, Aug 3, 2015 at 4:34 AM, Lukasz Pawelczyk
> <l.pawelczyk@xxxxxxxxxxx> wrote:
>> On piÄ, 2015-07-31 at 22:48 -0500, Serge E. Hallyn wrote:
>>> On Fri, Jul 31, 2015 at 11:28:56AM +0200, Lukasz Pawelczyk wrote:
>>> > On czw, 2015-07-30 at 16:30 -0500, Serge E. Hallyn wrote:
>>> > > On Fri, Jul 24, 2015 at 12:04:35PM +0200, Lukasz Pawelczyk wrote:
>>> > > > @@ -969,6 +982,7 @@ static int userns_install(struct nsproxy
>>> > > > *nsproxy, struct ns_common *ns)
>>> > > >  {
>>> > > >         struct user_namespace *user_ns = to_user_ns(ns);
>>> > > >         struct cred *cred;
>>> > > > +       int err;
>>> > > >
>>> > > >         /* Don't allow gaining capabilities by reentering
>>> > > >          * the same user namespace.
>>> > > > @@ -986,6 +1000,10 @@ static int userns_install(struct nsproxy
>>> > > > *nsproxy, struct ns_common *ns)
>>> > > >         if (!ns_capable(user_ns, CAP_SYS_ADMIN))
>>> > > >                 return -EPERM;
>>> > > >
>>> > > > +       err = security_userns_setns(nsproxy, user_ns);
>>> > > > +       if (err)
>>> > > > +               return err;
>>> > >
>>> > > So at this point the LSM thinks current is in the new ns.  If
>>> > > prepare_creds() fails below, should it be informed of that?
>>> > > (Or am I over-thinking this?)
>>> > >
>>> > > > +
>>> > > >         cred = prepare_creds();
>>> > > >         if (!cred)
>>> > > >                 return -ENOMEM;
>>> >
>>> > Hmm, the use case for this hook I had in mind was just to allow or
>>> > disallow the operation based on the information passed in
>>> > arguments.
>>> > Not to register the current in any way so LSM can think it is or
>>> > isn't
>>> > in the new namespace.
>>> >
>>> > I think that any other LSM check that would like to know in what
>>> > namespace the current is, would just check that from current's
>>> > creds.
>>> > Not use some stale and duplicated information the above hook could
>>> > have
>>> > registered.
>>> >
>>> > I see no reason for this hook to change the LSM state, only to
>>> > answer
>>> > the question: allowed/disallowed (eventually return an error cause
>>> > it
>>> > is unable to give an answer which falls into the disallow
>>> > category).
>>>
>>> How about renaming it "security_userns_may_setns()" for clarity?
>>
>> I personally have nothing against it. However looking at already
>> existing hooks only one of them has "may" in the name (unix_may_send)
>> while a lot clearly have exactly this purpose (e.g. most of inode_*
>> family, some from file_* and task_*). So it seems the trend is against
>> it.
>>
>> What do you think? Anyone else has an opinion?
>
> Personally, I prefer that hooks be named as closely to their caller,
> or calling context, as possible. In this case, it seems like "may" is
> implied. It's an LSM like all the others, so it can fail, which would
> cause the caller to fail too, so "may" tends to be implicit. I would
> leave it as-is, but I could be convinced otherwise.

I agree with Kees, sticking as close as possible to the general format
of "security_<caller>" for LSM hooks makes it easier when
reading/reviewing code.

-- 
paul moore
www.paul-moore.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/