On Mon, Jun 27, 2022 at 10:51:48AM -0500, Frederick Lawler 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. > > > > 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? > > > > > In the other thread [1], there was mention of xattr mapping support. As I > understand Caseys response to this thread [2], that feature is no longer > requested for this hook. I think that is an orthogonal problem at least wrt to this hook. > > Users can still access the older parent ns from the passed in cred, but I > was thinking of handling the transition point here. There's probably more > suitable hooks for that case. Yes.
