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

[Casey Schaufler <casey@schaufler-ca.com>]

On 6/22/2022 7:24 AM, Frederick Lawler wrote:
> Hi Casey,
>
> 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.

LSM hooks are (or should be) orthogonal to capabilities, except for
where they are required to implement capabilities.

> There is a security use case to prevent this namespace from being created within an unprivileged environment.

Yes, which is why some people argued against allowing unprivileged creation of
user namespaces.

> 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.

If the only reason to have the hook is to disallow unprivileged user namespaces
it's probably time to revise the decision to always allow them. Make it a build
or runtime option. That would address the issue more directly than creating a
security module.

>
> I'm curious what others think about this too.
>
>
>> 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. I believe you brought up in the previous discussions that you'd like to use this hook for xattr purposes. Doesn't that require a namespace?

I'm not saying the information isn't required. But if you create a new namespace
and then decide the user isn't allowed to create a namespace you have to tear it
down. That's ugly. Better to pass the creation parameters to the hook before
creating the namespace.

The relationship between xattrs and namespaces is it's own can of worms.

>
>>>
>>> Links:
>>> 1. https://lore.kernel.org/all/20220608150942.776446-1-fred@cloudflare.com/
>>> 2. https://lore.kernel.org/all/87y1xzyhub.fsf@email.froward.int.ebiederm.org/
>>> 3. https://lore.kernel.org/all/9fe9cd9f-1ded-a179-8ded-5fde8960a586@cloudflare.com/
>>>
>>> Frederick Lawler (2):
>>>    security, lsm: Introduce security_create_user_ns()
>>>    bpf-lsm: Make bpf_lsm_create_user_ns() sleepable
>>>
>>>   include/linux/lsm_hook_defs.h | 2 ++
>>>   include/linux/lsm_hooks.h     | 5 +++++
>>>   include/linux/security.h      | 8 ++++++++
>>>   kernel/bpf/bpf_lsm.c          | 1 +
>>>   kernel/user_namespace.c       | 5 +++++
>>>   security/security.c           | 6 ++++++
>>>   6 files changed, 27 insertions(+)
>>>
>>> -- 
>>> 2.30.2
>>>
>