Re: [PATCH v3 0/4] Introduce security_create_user_ns()

From: Frederick Lawler
Date: Wed Aug 03 2022 - 11:20:46 EST

On 8/2/22 4:33 PM, Eric W. Biederman wrote:
Paul Moore <paul@xxxxxxxxxxxxxx> writes:

On July 22, 2022 2:12:03 AM Martin KaFai Lau <kafai@xxxxxx> wrote:

On Thu, Jul 21, 2022 at 12:28:04PM -0500, 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 userns_create LSM hook, then marks the hook as sleepable in BPF.
Patch 1 and 4 still need review from the lsm/security side.


This patchset is in my review queue and assuming everything checks
out, I expect to merge it after the upcoming merge window closes.

It doesn't even address my issues with the last patchset.

Are you referring to [1], and with regards to [2], is the issue that the wording could be improved for both the cover letter and patch 1/4?

Ultimately, the goal of CF is to leverage and use user namespaces and block tasks whose meta information do not align with our allow list criteria. Yes, there is a higher goal of restricting our attack surface. Yes, people will find ways around security. The point is to have multiple levels of security, and this patch series allows people to add another level.

Calling this hook a regression is not true since there's no actual regression in the code. What would constitute a perceived regression is an admin imposing such a SELinux or BPF restriction within their company, but developers in that company ideally would try to work with the admin to enable user namespaces for certain use cases, or alternatively do what you don't want given current tooling: always run code as root. That's where this hook comes in: let people observe and enforce how they see fit. The average enthusiasts would see no impact.

I was requested to add _some_ test to BPF and to add a SELinux implementation. The low hanging fruit for a test to prove that the hook is capable of doing _something_ was to simply just block outright, and provide _some example_ of use. It doesn't make sense for us to write a test that outlines specifically what CF or others are doing because that would put too much emphasis on an implementation detail that doesn't matter to prove that the hook works.

Without Djalal's comment, I can't defend an observability use case that we're not currently leveraging. We have it now, so therefore I'll defend it per KP's suggestion[3] in v5.

By not responding to the email discussions, we can't accurately gauge what should or should not be in the descriptions. No one here necessarily disagrees with some of the points you made, and others have appropriately responded. As others have also wrote, you're not proposing alternatives. How do you expect us to work with that?

Please, let us know which bits and pieces ought to be included in the descriptions, and let us know what things we should call out caveats to that would satisfy your concerns.

Links:
1. https://lore.kernel.org/all/01368386-521f-230b-1d49-de19377c27d1@xxxxxxxxxxxxxx/
2. https://lore.kernel.org/all/877d45kri4.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/#t
3. https://lore.kernel.org/all/CACYkzJ4x90DamdN4dRCn1gZuAHLqJNy4MoP=qTX+44Bqx1uxSQ@xxxxxxxxxxxxxx/
4. https://lore.kernel.org/all/CAEiveUdPhEPAk7Y0ZXjPsD=Vb5hn453CHzS9aG-tkyRa8bf_eg@xxxxxxxxxxxxxx/#t


So it has my NACK.

Eric