Message-ID: <20220627121137.cnmctlxxtcgzwrws@wittgenstein>
Date:   Mon, 27 Jun 2022 14:11:37 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Frederick Lawler <fred@...udflare.com>,
        Casey Schaufler <casey@...aufler-ca.com>, kpsingh@...nel.org,
        revest@...omium.org, jackmanb@...omium.org, ast@...nel.org,
        daniel@...earbox.net, andrii@...nel.org, kafai@...com,
        songliubraving@...com, yhs@...com, john.fastabend@...il.com,
        jmorris@...ei.org, serge@...lyn.com, bpf@...r.kernel.org,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel-team@...udflare.com
Subject: Re: [PATCH 0/2] Introduce security_create_user_ns()

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@...udflare.com> 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.]
> 
> In this particular case I think the calling task's credentials are
> generally all that is needed.  You mention that the newly created

Agreed.

> namespace would be helpful, so I'll ask: what info in the new ns do
> you believe would be helpful in making an access decision about its
> creation?
> 
> Once we've sorted that we can make a better decision about the hook
> placement, but right now my gut feeling is that we only need to pass
> the task's creds, and I think placing the hook right after the UID/GID
> mapping check (before the new ns allocation) would be the best spot.

When I toyed with this I placed it directly into create_user_ns() and
only relied on the calling task's cred. I just created an eBPF program
that verifies the caller is capable(CAP_SYS_ADMIN). Since both the
chrooted and mapping check return EPERM it doesn't really matter that
much where exactly. Conceptually it makes more sense to me to place it
after the mapping check because then all the preliminaries are done.

Christian

Powered by blists - more mailing lists