[PATCH 0/2] Introduce security_create_user_ns()

[PATCH 0/2] Introduce security_create_user_ns()

[Frederick Lawler]

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.

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

[PATCH 1/2] security, lsm: Introduce security_create_user_ns()

[Frederick Lawler]

Preventing user namespace (privileged or otherwise) creation comes in a
few of forms in order of granularity:

        1. /proc/sys/user/max_user_namespaces sysctl
        2. OS specific patch(es)
        3. CONFIG_USER_NS

To block a task based on its attributes, the LSM hook cred_prepare is a
good candidate for use because it provides more granular control, and
it is called before create_user_ns():

        cred = prepare_creds()
                security_prepare_creds()
                        call_int_hook(cred_prepare, ...
        if (cred)
                create_user_ns(cred)

Since security_prepare_creds() is meant for LSMs to copy and prepare
credentials, access control is an unintended use of the hook. Therefore
introduce a new function security_create_user_ns() with an accompanying
create_user_ns LSM hook.

This hook takes the prepared creds and the newly created user namespace
for LSM authors to write policy against. On success, the new namespace
is applied to credentials, otherwise an error is returned.

Signed-off-by: Frederick Lawler <fred@cloudflare.com>
---
 include/linux/lsm_hook_defs.h | 2 ++
 include/linux/lsm_hooks.h     | 5 +++++
 include/linux/security.h      | 8 ++++++++
 kernel/user_namespace.c       | 5 +++++
 security/security.c           | 6 ++++++
 5 files changed, 26 insertions(+)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index eafa1d2489fd..bd9b38db4d03 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -223,6 +223,8 @@ LSM_HOOK(int, -ENOSYS, task_prctl, int option, unsigned long arg2,
 	 unsigned long arg3, unsigned long arg4, unsigned long arg5)
 LSM_HOOK(void, LSM_RET_VOID, task_to_inode, struct task_struct *p,
 	 struct inode *inode)
+LSM_HOOK(int, 0, create_user_ns, const struct cred *new,
+	 const struct user_namespace *new_userns)
 LSM_HOOK(int, 0, ipc_permission, struct kern_ipc_perm *ipcp, short flag)
 LSM_HOOK(void, LSM_RET_VOID, ipc_getsecid, struct kern_ipc_perm *ipcp,
 	 u32 *secid)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 91c8146649f5..1356a792a6bd 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -799,6 +799,11 @@
  *	security attributes, e.g. for /proc/pid inodes.
  *	@p contains the task_struct for the task.
  *	@inode contains the inode structure for the inode.
+ * @create_user_ns:
+ *	Check permission prior to assigning the new namespace to @cred->user_ns.
+ *	@cred points to prepared creds.
+ *	@new_userns points to the newly created user namespace.
+ *	Return 0 if successful, otherwise < 0 error code.
  *
  * Security hooks for Netlink messaging.
  *
diff --git a/include/linux/security.h b/include/linux/security.h
index 7fc4e9f49f54..a656dbe7b65a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -435,6 +435,8 @@ int security_task_kill(struct task_struct *p, struct kernel_siginfo *info,
 int security_task_prctl(int option, unsigned long arg2, unsigned long arg3,
 			unsigned long arg4, unsigned long arg5);
 void security_task_to_inode(struct task_struct *p, struct inode *inode);
+int security_create_user_ns(const struct cred *cred,
+			    const struct user_namespace *new_userns);
 int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag);
 void security_ipc_getsecid(struct kern_ipc_perm *ipcp, u32 *secid);
 int security_msg_msg_alloc(struct msg_msg *msg);
@@ -1185,6 +1187,12 @@ static inline int security_task_prctl(int option, unsigned long arg2,
 static inline void security_task_to_inode(struct task_struct *p, struct inode *inode)
 { }
 
+static inline int security_create_user_ns(const struct cred *cred,
+					  const struct user_namespace *new_userns)
+{
+	return 0;
+}
+
 static inline int security_ipc_permission(struct kern_ipc_perm *ipcp,
 					  short flag)
 {
diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
index 5481ba44a8d6..8c5e5592a503 100644
--- a/kernel/user_namespace.c
+++ b/kernel/user_namespace.c
@@ -9,6 +9,7 @@
 #include <linux/highuid.h>
 #include <linux/cred.h>
 #include <linux/securebits.h>
+#include <linux/security.h>
 #include <linux/keyctl.h>
 #include <linux/key-type.h>
 #include <keys/user-type.h>
@@ -153,6 +154,10 @@ int create_user_ns(struct cred *new)
 	if (!setup_userns_sysctls(ns))
 		goto fail_keyring;
 
+	ret = security_create_user_ns(new, ns);
+	if (ret < 0)
+		goto fail_keyring;
+
 	set_cred_user_ns(new, ns);
 	return 0;
 fail_keyring:
diff --git a/security/security.c b/security/security.c
index 188b8f782220..d6b1751805ca 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1903,6 +1903,12 @@ void security_task_to_inode(struct task_struct *p, struct inode *inode)
 	call_void_hook(task_to_inode, p, inode);
 }
 
+int security_create_user_ns(const struct cred *cred,
+			    const struct user_namespace *new_userns)
+{
+	return call_int_hook(create_user_ns, 0, cred, new_userns);
+}
+
 int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
 {
 	return call_int_hook(ipc_permission, 0, ipcp, flag);
-- 
2.30.2

[PATCH 2/2] bpf-lsm: Make bpf_lsm_create_user_ns() sleepable

[Frederick Lawler]

Users may want to audit calls to security_create_user_ns() and access
user space memory. Also create_user_ns() runs without
pagefault_disabled(). Therefore, make bpf_lsm_create_user_ns() sleepable
for mandatory access control policies.

Signed-off-by: Frederick Lawler <fred@cloudflare.com>
---
 kernel/bpf/bpf_lsm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
index c1351df9f7ee..75853965e7b0 100644
--- a/kernel/bpf/bpf_lsm.c
+++ b/kernel/bpf/bpf_lsm.c
@@ -250,6 +250,7 @@ BTF_ID(func, bpf_lsm_task_getsecid_obj)
 BTF_ID(func, bpf_lsm_task_prctl)
 BTF_ID(func, bpf_lsm_task_setscheduler)
 BTF_ID(func, bpf_lsm_task_to_inode)
+BTF_ID(func, bpf_lsm_create_user_ns)
 BTF_SET_END(sleepable_lsm_hooks)
 
 bool bpf_lsm_is_sleepable_hook(u32 btf_id)
-- 
2.30.2

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

[Casey Schaufler]

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

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

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

[Frederick Lawler]

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


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

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

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

[Casey Schaufler]

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

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

[Casey Schaufler]

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

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

[Paul Moore]

On Wed, Jun 22, 2022 at 10:24 AM Frederick Lawler <fred@cloudflare.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
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, ...).

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.

> > 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 :)

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

-- 
paul-moore.com

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

[Christian Brauner]

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

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

[Frederick Lawler]

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@cloudflare.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.]
>>

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.

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.


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

I don't specifically have a use case to pass the new user namespace for 
this hook at this time. I'll move the hook in v2.

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

Agreed.

> Christian

Links:
1. 
https://lore.kernel.org/all/4ae12ee6-959c-51cb-9d7a-54adb3a0ea53@schaufler-ca.com/
2. 
https://lore.kernel.org/all/4b62f0c5-9f3c-e0bc-d836-1b7cdea429da@schaufler-ca.com/

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

[Christian Brauner]

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@cloudflare.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.]
> > > 
> 
> 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.

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

[Casey Schaufler]

On 6/27/2022 8:56 AM, Christian Brauner wrote:
> 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@cloudflare.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.

Yes, although it would be really helpful if there were a recognized upstream
for eBPF programs so that we could see not only that the hook is used but how
it is being used. It is possible (even likely) that someone will want to change
either the interface or the caller some day. Without having the eBPF that
depends on it, it's hard to determine if a change would be a regression.

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

Agreed. It was always a look deeply into the future sort of thing.
At this point I don't see anything blocking the proposed hook moving
forward.

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

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

[Paul Moore]

On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote:
> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:

...

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

Yes, for the most part I don't really worry about the "is a BPF LSM a
LSM?" question, it's generally not important for most discussions.
However, there is an issue unique to the BPF LSMs which I think is
relevant here: there is no hook implementation code living under
security/.  While I talked about a hook implementation being helpful
to verify the hook prototype, it is also helpful in providing an
in-tree example for other LSMs; unfortunately we don't get that same
example value when the initial hook implementation is a BPF LSM.

-- 
paul-moore.com

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

[Paul Moore]

On Mon, Jun 27, 2022 at 11:51 AM Frederick Lawler <fred@cloudflare.com> 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@cloudflare.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.]
> >>
>
> 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

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

[Daniel Borkmann]

On 6/27/22 11:56 PM, Paul Moore wrote:
> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote:
>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
> 
> ...
> 
>>> 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.
> 
> Yes, for the most part I don't really worry about the "is a BPF LSM a
> LSM?" question, it's generally not important for most discussions.
> However, there is an issue unique to the BPF LSMs which I think is
> relevant here: there is no hook implementation code living under
> security/.  While I talked about a hook implementation being helpful
> to verify the hook prototype, it is also helpful in providing an
> in-tree example for other LSMs; unfortunately we don't get that same
> example value when the initial hook implementation is a BPF LSM.

I would argue that such a patch series must come together with a BPF
selftest which then i) contains an in-tree usage example, ii) adds BPF
CI test coverage. Shipping with a BPF selftest at least would be the
usual expectation.

Thanks,
Daniel

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

[KP Singh]

On Tue, Jun 28, 2022 at 12:15 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 6/27/22 11:56 PM, Paul Moore wrote:
> > On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote:
> >> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
> >
> > ...
> >
> >>> 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.
> >
> > Yes, for the most part I don't really worry about the "is a BPF LSM a
> > LSM?" question, it's generally not important for most discussions.
> > However, there is an issue unique to the BPF LSMs which I think is
> > relevant here: there is no hook implementation code living under
> > security/.  While I talked about a hook implementation being helpful
> > to verify the hook prototype, it is also helpful in providing an
> > in-tree example for other LSMs; unfortunately we don't get that same
> > example value when the initial hook implementation is a BPF LSM.
>
> I would argue that such a patch series must come together with a BPF
> selftest which then i) contains an in-tree usage example, ii) adds BPF
> CI test coverage. Shipping with a BPF selftest at least would be the
> usual expectation.

+1 I would also recommend that this comes with a BPF selftest as
suggested by Daniel.

>
> Thanks,
> Daniel

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

[Paul Moore]

On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 6/27/22 11:56 PM, Paul Moore wrote:
> > On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote:
> >> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
> >
> > ...
> >
> >>> 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.
> >
> > Yes, for the most part I don't really worry about the "is a BPF LSM a
> > LSM?" question, it's generally not important for most discussions.
> > However, there is an issue unique to the BPF LSMs which I think is
> > relevant here: there is no hook implementation code living under
> > security/.  While I talked about a hook implementation being helpful
> > to verify the hook prototype, it is also helpful in providing an
> > in-tree example for other LSMs; unfortunately we don't get that same
> > example value when the initial hook implementation is a BPF LSM.
>
> I would argue that such a patch series must come together with a BPF
> selftest which then i) contains an in-tree usage example, ii) adds BPF
> CI test coverage. Shipping with a BPF selftest at least would be the
> usual expectation.

I'm not going to disagree with that, I generally require matching
tests for new SELinux kernel code, but I was careful to mention code
under 'security/' and not necessarily just a test implementation :)  I
don't want to get into a big discussion about it, but I think having a
working implementation somewhere under 'security/' is more
discoverable for most LSM folks.

-- 
paul-moore.com

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

[Casey Schaufler]

On 6/27/2022 3:27 PM, Paul Moore wrote:
> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 6/27/22 11:56 PM, Paul Moore wrote:
>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote:
>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
>>> ...
>>>
>>>>> 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.
>>> Yes, for the most part I don't really worry about the "is a BPF LSM a
>>> LSM?" question, it's generally not important for most discussions.
>>> However, there is an issue unique to the BPF LSMs which I think is
>>> relevant here: there is no hook implementation code living under
>>> security/.  While I talked about a hook implementation being helpful
>>> to verify the hook prototype, it is also helpful in providing an
>>> in-tree example for other LSMs; unfortunately we don't get that same
>>> example value when the initial hook implementation is a BPF LSM.
>> I would argue that such a patch series must come together with a BPF
>> selftest which then i) contains an in-tree usage example, ii) adds BPF
>> CI test coverage. Shipping with a BPF selftest at least would be the
>> usual expectation.
> I'm not going to disagree with that, I generally require matching
> tests for new SELinux kernel code, but I was careful to mention code
> under 'security/' and not necessarily just a test implementation :)  I
> don't want to get into a big discussion about it, but I think having a
> working implementation somewhere under 'security/' is more
> discoverable for most LSM folks.

I agree. It would be unfortunate if we added a hook explicitly for eBPF
only to discover that the proposed user needs something different. The
LSM community should have a chance to review the code before committing
to all the maintenance required in supporting it.

Is there a reference on how to write an eBPF security module?
There should be something out there warning the eBPF programmer of the
implications of providing a secid_to_secctx hook for starters.

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

[Frederick Lawler]

On 6/27/22 5:15 PM, Daniel Borkmann wrote:
> On 6/27/22 11:56 PM, Paul Moore wrote:
>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> 
>> wrote:
>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
>>
>> ...
>>
>>>> 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.
>>
>> Yes, for the most part I don't really worry about the "is a BPF LSM a
>> LSM?" question, it's generally not important for most discussions.
>> However, there is an issue unique to the BPF LSMs which I think is
>> relevant here: there is no hook implementation code living under
>> security/.  While I talked about a hook implementation being helpful
>> to verify the hook prototype, it is also helpful in providing an
>> in-tree example for other LSMs; unfortunately we don't get that same
>> example value when the initial hook implementation is a BPF LSM.
> 
> I would argue that such a patch series must come together with a BPF
> selftest which then i) contains an in-tree usage example, ii) adds BPF
> CI test coverage. Shipping with a BPF selftest at least would be the
> usual expectation.

Sounds good. I'll add both a eBPF selftest and SELinux implementation 
for v2.

> 
> Thanks,
> Daniel

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

[Paul Moore]

On Tue, Jun 28, 2022 at 11:11 AM Frederick Lawler <fred@cloudflare.com> wrote:
> On 6/27/22 5:15 PM, Daniel Borkmann wrote:
> > On 6/27/22 11:56 PM, Paul Moore wrote:
> >> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org>
> >> wrote:
> >>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
> >>
> >> ...
> >>
> >>>> 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.
> >>
> >> Yes, for the most part I don't really worry about the "is a BPF LSM a
> >> LSM?" question, it's generally not important for most discussions.
> >> However, there is an issue unique to the BPF LSMs which I think is
> >> relevant here: there is no hook implementation code living under
> >> security/.  While I talked about a hook implementation being helpful
> >> to verify the hook prototype, it is also helpful in providing an
> >> in-tree example for other LSMs; unfortunately we don't get that same
> >> example value when the initial hook implementation is a BPF LSM.
> >
> > I would argue that such a patch series must come together with a BPF
> > selftest which then i) contains an in-tree usage example, ii) adds BPF
> > CI test coverage. Shipping with a BPF selftest at least would be the
> > usual expectation.
>
> Sounds good. I'll add both a eBPF selftest and SELinux implementation
> for v2.

Thanks Daniel!

-- 
paul-moore.com

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

[Frederick Lawler]

On 6/27/22 6:18 PM, Casey Schaufler wrote:
> On 6/27/2022 3:27 PM, Paul Moore wrote:
>> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> 
>> wrote:
>>> On 6/27/22 11:56 PM, Paul Moore wrote:
>>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner 
>>>> <brauner@kernel.org> wrote:
>>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
>>>> ...
>>>>
>>>>>> 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.
>>>> Yes, for the most part I don't really worry about the "is a BPF LSM a
>>>> LSM?" question, it's generally not important for most discussions.
>>>> However, there is an issue unique to the BPF LSMs which I think is
>>>> relevant here: there is no hook implementation code living under
>>>> security/.  While I talked about a hook implementation being helpful
>>>> to verify the hook prototype, it is also helpful in providing an
>>>> in-tree example for other LSMs; unfortunately we don't get that same
>>>> example value when the initial hook implementation is a BPF LSM.
>>> I would argue that such a patch series must come together with a BPF
>>> selftest which then i) contains an in-tree usage example, ii) adds BPF
>>> CI test coverage. Shipping with a BPF selftest at least would be the
>>> usual expectation.
>> I'm not going to disagree with that, I generally require matching
>> tests for new SELinux kernel code, but I was careful to mention code
>> under 'security/' and not necessarily just a test implementation :)  I
>> don't want to get into a big discussion about it, but I think having a
>> working implementation somewhere under 'security/' is more
>> discoverable for most LSM folks.
> 
> I agree. It would be unfortunate if we added a hook explicitly for eBPF
> only to discover that the proposed user needs something different. The
> LSM community should have a chance to review the code before committing
> to all the maintenance required in supporting it.
> 
> Is there a reference on how to write an eBPF security module?

There's a documentation page that briefly touches on a BPF LSM 
implementation [1].

> There should be something out there warning the eBPF programmer of the
> implications of providing a secid_to_secctx hook for starters.
> 

Links:
1. https://docs.kernel.org/bpf/prog_lsm.html?highlight=bpf+lsm#

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

[Casey Schaufler]

On 6/28/2022 8:14 AM, Frederick Lawler wrote:
> On 6/27/22 6:18 PM, Casey Schaufler wrote:
>> On 6/27/2022 3:27 PM, Paul Moore wrote:
>>> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>> On 6/27/22 11:56 PM, Paul Moore wrote:
>>>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote:
>>>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
>>>>> ...
>>>>>
>>>>>>> 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.
>>>>> Yes, for the most part I don't really worry about the "is a BPF LSM a
>>>>> LSM?" question, it's generally not important for most discussions.
>>>>> However, there is an issue unique to the BPF LSMs which I think is
>>>>> relevant here: there is no hook implementation code living under
>>>>> security/.  While I talked about a hook implementation being helpful
>>>>> to verify the hook prototype, it is also helpful in providing an
>>>>> in-tree example for other LSMs; unfortunately we don't get that same
>>>>> example value when the initial hook implementation is a BPF LSM.
>>>> I would argue that such a patch series must come together with a BPF
>>>> selftest which then i) contains an in-tree usage example, ii) adds BPF
>>>> CI test coverage. Shipping with a BPF selftest at least would be the
>>>> usual expectation.
>>> I'm not going to disagree with that, I generally require matching
>>> tests for new SELinux kernel code, but I was careful to mention code
>>> under 'security/' and not necessarily just a test implementation :)  I
>>> don't want to get into a big discussion about it, but I think having a
>>> working implementation somewhere under 'security/' is more
>>> discoverable for most LSM folks.
>>
>> I agree. It would be unfortunate if we added a hook explicitly for eBPF
>> only to discover that the proposed user needs something different. The
>> LSM community should have a chance to review the code before committing
>> to all the maintenance required in supporting it.
>>
>> Is there a reference on how to write an eBPF security module?
>
> There's a documentation page that briefly touches on a BPF LSM implementation [1].

That's a brief touch, alright. I'll grant that the LSM interface isn't
especially well documented for C developers, but we have done tutorials
and have multiple examples. I worry that without an in-tree example for
eBPF we might well be setting developers up for spectacular failure.

>
>> There should be something out there warning the eBPF programmer of the
>> implications of providing a secid_to_secctx hook for starters.
>>
>
> Links:
> 1. https://docs.kernel.org/bpf/prog_lsm.html?highlight=bpf+lsm#
>

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

[KP Singh]

On Tue, Jun 28, 2022 at 6:02 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 6/28/2022 8:14 AM, Frederick Lawler wrote:
> > On 6/27/22 6:18 PM, Casey Schaufler wrote:
> >> On 6/27/2022 3:27 PM, Paul Moore wrote:
> >>> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>> On 6/27/22 11:56 PM, Paul Moore wrote:
> >>>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote:
> >>>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
> >>>>> ...
> >>>>>
> >>>>>>> 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.
> >>>>> Yes, for the most part I don't really worry about the "is a BPF LSM a
> >>>>> LSM?" question, it's generally not important for most discussions.
> >>>>> However, there is an issue unique to the BPF LSMs which I think is
> >>>>> relevant here: there is no hook implementation code living under
> >>>>> security/.  While I talked about a hook implementation being helpful
> >>>>> to verify the hook prototype, it is also helpful in providing an
> >>>>> in-tree example for other LSMs; unfortunately we don't get that same
> >>>>> example value when the initial hook implementation is a BPF LSM.
> >>>> I would argue that such a patch series must come together with a BPF
> >>>> selftest which then i) contains an in-tree usage example, ii) adds BPF
> >>>> CI test coverage. Shipping with a BPF selftest at least would be the
> >>>> usual expectation.
> >>> I'm not going to disagree with that, I generally require matching
> >>> tests for new SELinux kernel code, but I was careful to mention code
> >>> under 'security/' and not necessarily just a test implementation :)  I
> >>> don't want to get into a big discussion about it, but I think having a
> >>> working implementation somewhere under 'security/' is more
> >>> discoverable for most LSM folks.
> >>
> >> I agree. It would be unfortunate if we added a hook explicitly for eBPF
> >> only to discover that the proposed user needs something different. The
> >> LSM community should have a chance to review the code before committing
> >> to all the maintenance required in supporting it.
> >>
> >> Is there a reference on how to write an eBPF security module?
> >
> > There's a documentation page that briefly touches on a BPF LSM implementation [1].
>
> That's a brief touch, alright. I'll grant that the LSM interface isn't
> especially well documented for C developers, but we have done tutorials
> and have multiple examples. I worry that without an in-tree example for
> eBPF we might well be setting developers up for spectacular failure.
>

Casey, Daniel and I are recommending an in-tree example, it will be
in BPF selftests and we will CC you on the reviews.

Frederick, is that okay with you?

> >
> >> There should be something out there warning the eBPF programmer of the
> >> implications of providing a secid_to_secctx hook for starters.
> >>
> >
> > Links:
> > 1. https://docs.kernel.org/bpf/prog_lsm.html?highlight=bpf+lsm#
> >

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

[Frederick Lawler]

On 6/28/22 11:12 AM, KP Singh wrote:
> On Tue, Jun 28, 2022 at 6:02 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>>
>> On 6/28/2022 8:14 AM, Frederick Lawler wrote:
>>> On 6/27/22 6:18 PM, Casey Schaufler wrote:
>>>> On 6/27/2022 3:27 PM, Paul Moore wrote:
>>>>> On Mon, Jun 27, 2022 at 6:15 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>>>> On 6/27/22 11:56 PM, Paul Moore wrote:
>>>>>>> On Mon, Jun 27, 2022 at 8:11 AM Christian Brauner <brauner@kernel.org> wrote:
>>>>>>>> On Thu, Jun 23, 2022 at 11:21:37PM -0400, Paul Moore wrote:
>>>>>>> ...
>>>>>>>
>>>>>>>>> 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.
>>>>>>> Yes, for the most part I don't really worry about the "is a BPF LSM a
>>>>>>> LSM?" question, it's generally not important for most discussions.
>>>>>>> However, there is an issue unique to the BPF LSMs which I think is
>>>>>>> relevant here: there is no hook implementation code living under
>>>>>>> security/.  While I talked about a hook implementation being helpful
>>>>>>> to verify the hook prototype, it is also helpful in providing an
>>>>>>> in-tree example for other LSMs; unfortunately we don't get that same
>>>>>>> example value when the initial hook implementation is a BPF LSM.
>>>>>> I would argue that such a patch series must come together with a BPF
>>>>>> selftest which then i) contains an in-tree usage example, ii) adds BPF
>>>>>> CI test coverage. Shipping with a BPF selftest at least would be the
>>>>>> usual expectation.
>>>>> I'm not going to disagree with that, I generally require matching
>>>>> tests for new SELinux kernel code, but I was careful to mention code
>>>>> under 'security/' and not necessarily just a test implementation :)  I
>>>>> don't want to get into a big discussion about it, but I think having a
>>>>> working implementation somewhere under 'security/' is more
>>>>> discoverable for most LSM folks.
>>>>
>>>> I agree. It would be unfortunate if we added a hook explicitly for eBPF
>>>> only to discover that the proposed user needs something different. The
>>>> LSM community should have a chance to review the code before committing
>>>> to all the maintenance required in supporting it.
>>>>
>>>> Is there a reference on how to write an eBPF security module?
>>>
>>> There's a documentation page that briefly touches on a BPF LSM implementation [1].
>>
>> That's a brief touch, alright. I'll grant that the LSM interface isn't
>> especially well documented for C developers, but we have done tutorials
>> and have multiple examples. I worry that without an in-tree example for
>> eBPF we might well be setting developers up for spectacular failure.
>>
> 
> Casey, Daniel and I are recommending an in-tree example, it will be
> in BPF selftests and we will CC you on the reviews.
> 
> Frederick, is that okay with you?

Yep.

> 
>>>
>>>> There should be something out there warning the eBPF programmer of the
>>>> implications of providing a secid_to_secctx hook for starters.
>>>>
>>>
>>> Links:
>>> 1. https://docs.kernel.org/bpf/prog_lsm.html?highlight=bpf+lsm#
>>>

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

[Eric W. Biederman]

Frederick Lawler <fred@cloudflare.com> writes:

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

There is also a very strong security use case for not putting security
checks in the creation of the user namespace.

In particular there are all kinds of kernel features that are useful for
building sandboxes namespaces, chroot, etc, that previous to user
namespaces were not possible to make available to unprivileged users
because they could confuse suid-root executables.  With user namespaces
the concern about confusing suid-root executable goes away.

The only justification I have ever heard for restricting the user
namespace is because it indirectly allows for greater kernel attack
surface.

Do you have a case for restricting the user namespace other than the
kernel is buggy and the user namespace makes the kernel bugs easier
to access?

How does increasing the attack surface of the kernel make the situation
that the kernel is buggy and the attack surface is too big better?

Perhaps it is explained somewhere down-thread and I just have not caught
up yet, but so far I have not see a description of why it makes sense
for a security module to restrict the kernel here.

Eric

p.s.  I am little disappointed that I was not copied on this thread
given that it is my code you are messing with, and I was in an earlier
version of this thread.

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

[Frederick Lawler]

On 6/30/22 1:28 PM, Eric W. Biederman wrote:
> Frederick Lawler <fred@cloudflare.com> writes:
> 
>> 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. 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.
> 
> There is also a very strong security use case for not putting security
> checks in the creation of the user namespace.
> 
> In particular there are all kinds of kernel features that are useful for
> building sandboxes namespaces, chroot, etc, that previous to user
> namespaces were not possible to make available to unprivileged users
> because they could confuse suid-root executables.  With user namespaces
> the concern about confusing suid-root executable goes away.
> 
> The only justification I have ever heard for restricting the user
> namespace is because it indirectly allows for greater kernel attack
> surface.
> 
> Do you have a case for restricting the user namespace other than the
> kernel is buggy and the user namespace makes the kernel bugs easier
> to access?

No.

> 
> How does increasing the attack surface of the kernel make the situation
> that the kernel is buggy and the attack surface is too big better?
> 

We agree that completely blocking this feature may disable effective 
sandboxing techniques, but uncontrolled user namespace creation 
increases the attack surface as well. There are known examples where 
creating a new namespace is the first step in a priv escalation attack.

> Perhaps it is explained somewhere down-thread and I just have not caught
> up yet, but so far I have not see a description of why it makes sense
> for a security module to restrict the kernel here.
> 

Having a new security hook allows us to decide when unpriv user 
namespace creation is acceptable and unacceptable. Patching 
vulnerabilities takes time and effort amongst the community. We want to 
protect our systems so that we are not as impacted by the next 0-day 
disclosure. This in turn decreases our overall attack surface.

Security hooks are a good fit because they offer us flexibility to 
protect our systems how we see fit.

> Eric
> 
> p.s.  I am little disappointed that I was not copied on this thread
> given that it is my code you are messing with, and I was in an earlier
> version of this thread.