Re: [PATCH 0/2] Introduce security_create_user_ns()

[Paul Moore]

On Mon, Jun 27, 2022 at 11:51 AM Frederick Lawler <fred@xxxxxxxxxxxxxx> wrote:
> On 6/27/22 7:11 AM, Christian Brauner wrote:
> > On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
> >> On Wed, Jun 22, 2022 at 10:24 AM Frederick Lawler <fred@xxxxxxxxxxxxxx> wrote:
> >>> On 6/21/22 7:19 PM, Casey Schaufler wrote:
> >>>> On 6/21/2022 4:39 PM, Frederick Lawler wrote:

...

> >>>>> While creating a LSM BPF MAC policy to block user namespace creation, we
> >>>>> used the LSM cred_prepare hook because that is the closest hook to
> >>>>> prevent
> >>>>> a call to create_user_ns().
> >>>>>
> >>>>> The calls look something like this:
> >>>>>
> >>>>>       cred = prepare_creds()
> >>>>>           security_prepare_creds()
> >>>>>               call_int_hook(cred_prepare, ...
> >>>>>       if (cred)
> >>>>>           create_user_ns(cred)
> >>>>>
> >>>>> We noticed that error codes were not propagated from this hook and
> >>>>> introduced a patch [1] to propagate those errors.
> >>>>>
> >>>>> The discussion notes that security_prepare_creds()
> >>>>> is not appropriate for MAC policies, and instead the hook is
> >>>>> meant for LSM authors to prepare credentials for mutation. [2]
> >>>>>
> >>>>> Ultimately, we concluded that a better course of action is to introduce
> >>>>> a new security hook for LSM authors. [3]
> >>>>>
> >>>>> This patch set first introduces a new security_create_user_ns() function
> >>>>> and create_user_ns LSM hook, then marks the hook as sleepable in BPF.
> >>>>
> >>>> Why restrict this hook to user namespaces? It seems that an LSM that
> >>>> chooses to preform controls on user namespaces may want to do so for
> >>>> network namespaces as well.
> >>>
> >>> IIRC, CLONE_NEWUSER is the only namespace flag that does not require
> >>> CAP_SYS_ADMIN. There is a security use case to prevent this namespace
> >>> from being created within an unprivileged environment. I'm not opposed
> >>> to a more generic hook, but I don't currently have a use case to block
> >>> any others. We can also say the same is true for the other namespaces:
> >>> add this generic security function to these too.
> >>>
> >>> I'm curious what others think about this too.
> >>
> >> While user namespaces are obviously one of the more significant
> >> namespaces from a security perspective, I do think it seems reasonable
> >> that the LSMs could benefit from additional namespace creation hooks.
> >> However, I don't think we need to do all of them at once, starting
> >> with a userns hook seems okay to me.
> >>
> >> I also think that using the same LSM hook as an access control point
> >> for all of the different namespaces would be a mistake.  At the very
> >
> > Agreed. >
> >> least we would need to pass a flag or some form of context to the hook
> >> to indicate which new namespace(s) are being requested and I fear that
> >> is a problem waiting to happen.  That isn't to say someone couldn't
> >> mistakenly call the security_create_user_ns(...) from the mount
> >> namespace code, but I suspect that is much easier to identify as wrong
> >> than the equivalent security_create_ns(USER, ...).
> >
> > Yeah, I think that's a pretty unlikely scenario.
> >
> >>
> >> We also should acknowledge that while in most cases the current task's
> >> credentials are probably sufficient to make any LSM access control
> >> decisions around namespace creation, it's possible that for some
> >> namespaces we would need to pass additional, namespace specific info
> >> to the LSM.  With a shared LSM hook this could become rather awkward.
> >
> > Agreed.
> >
> >>
> >>>> Also, the hook seems backwards. You should
> >>>> decide if the creation of the namespace is allowed before you create it.
> >>>> Passing the new namespace to a function that checks to see creating a
> >>>> namespace is allowed doesn't make a lot off sense.
> >>>
> >>> I think having more context to a security hook is a good thing.
> >>
> >> This is one of the reasons why I usually like to see at least one LSM
> >> implementation to go along with every new/modified hook.  The
> >> implementation forces you to think about what information is necessary
> >> to perform a basic access control decision; sometimes it isn't always
> >> obvious until you have to write the access control :)
> >
> > I spoke to Frederick at length during LSS and as I've been given to
> > understand there's a eBPF program that would immediately use this new
> > hook. Now I don't want to get into the whole "Is the eBPF LSM hook
> > infrastructure an LSM" but I think we can let this count as a legitimate
> > first user of this hook/code.
> >
> >>
> >> [aside: If you would like to explore the SELinux implementation let me
> >> know, I'm happy to work with you on this.  I suspect Casey and the
> >> other LSM maintainers would also be willing to do the same for their
> >> LSMs.]
> >>
>
> I can take a shot at making a SELinux implementation, but the question
> becomes: is that for v2 or a later patch? I don't think the
> implementation for SELinux would be too complicated (i.e. make a call to
> avc_has_perm()?) but, testing and revisions might take a bit longer.

Isn't that the truth, writing code is easy(ish); testing it well is
the hard part ;)  Joking aside, I would suggest starting with v2.

As an example, the code below might be a good place to start - we would need to
discuss this on the SELinux list as there are some design decisions
I'm glossing over[1].

  int selinux_userns_create(struct cred *new)
  {
    u32 sid = current_sid();

    return avc_has_perm(&selinux_state, sid, sid,
                        SECCLASS_NAMESPACE,
                        NAMESPACE__USERNS_CREATE);
  }

You would also need to add the "namespace" class and the
"userns_create" permission in security/selinux/include/classmap.h.

  const struct security_class_mapping secclass_map[] = {
    ...
    { "namespace",
      { "userns_create", NULL } },
  }

As I mentioned earlier, if you find yourself getting stuck, or needing
some help, please feel free to send mail.

[1] This code snippet uses a new object class and permission for this
(namespace:userns_create).  I made that choice as object classes are
limited to 32 unique permissions and I expect the number of namespaces
to continue to grow.

-- 
paul-moore.com