[PATCH v4 0/4] Introduce security_create_user_ns()

[PATCH v4 0/4] 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 userns_create 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/

Past discussions:
V3: https://lore.kernel.org/all/20220721172808.585539-1-fred@cloudflare.com/
V2: https://lore.kernel.org/all/20220707223228.1940249-1-fred@cloudflare.com/
V1: https://lore.kernel.org/all/20220621233939.993579-1-fred@cloudflare.com/

Changes since v3:
- Explicitly set CAP_SYS_ADMIN to test namespace is created given
  permission
- Simplify BPF test to use sleepable hook only
- Prefer unshare() over clone() for tests
Changes since v2:
- Rename create_user_ns hook to userns_create
- Use user_namespace as an object opposed to a generic namespace object
- s/domB_t/domA_t in commit message
Changes since v1:
- Add selftests/bpf: Add tests verifying bpf lsm create_user_ns hook patch
- Add selinux: Implement create_user_ns hook patch
- Change function signature of security_create_user_ns() to only take
  struct cred
- Move security_create_user_ns() call after id mapping check in
  create_user_ns()
- Update documentation to reflect changes

Frederick Lawler (4):
  security, lsm: Introduce security_create_user_ns()
  bpf-lsm: Make bpf_lsm_userns_create() sleepable
  selftests/bpf: Add tests verifying bpf lsm userns_create hook
  selinux: Implement userns_create hook

 include/linux/lsm_hook_defs.h                 |   1 +
 include/linux/lsm_hooks.h                     |   4 +
 include/linux/security.h                      |   6 ++
 kernel/bpf/bpf_lsm.c                          |   1 +
 kernel/user_namespace.c                       |   5 +
 security/security.c                           |   5 +
 security/selinux/hooks.c                      |   9 ++
 security/selinux/include/classmap.h           |   2 +
 .../selftests/bpf/prog_tests/deny_namespace.c | 102 ++++++++++++++++++
 .../selftests/bpf/progs/test_deny_namespace.c |  33 ++++++
 10 files changed, 168 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/deny_namespace.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_deny_namespace.c

-- 
2.30.2

[PATCH v4 1/4] 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
userns_create LSM hook.

This hook takes the prepared creds 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>
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

---
Changes since v3:
- No changes
Changes since v2:
- Rename create_user_ns hook to userns_create
Changes since v1:
- Changed commit wording
- Moved execution to be after id mapping check
- Changed signature to only accept a const struct cred *
---
 include/linux/lsm_hook_defs.h | 1 +
 include/linux/lsm_hooks.h     | 4 ++++
 include/linux/security.h      | 6 ++++++
 kernel/user_namespace.c       | 5 +++++
 security/security.c           | 5 +++++
 5 files changed, 21 insertions(+)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index eafa1d2489fd..7ff93cb8ca8d 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -223,6 +223,7 @@ 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, userns_create, const struct cred *cred)
 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..54fe534d0e01 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -799,6 +799,10 @@
  *	security attributes, e.g. for /proc/pid inodes.
  *	@p contains the task_struct for the task.
  *	@inode contains the inode structure for the inode.
+ * @userns_create:
+ *	Check permission prior to creating a new user namespace.
+ *	@cred points to prepared creds.
+ *	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..a195bf33246a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -435,6 +435,7 @@ 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);
 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 +1186,11 @@ 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)
+{
+	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..3f464bbda0e9 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>
@@ -113,6 +114,10 @@ int create_user_ns(struct cred *new)
 	    !kgid_has_mapping(parent_ns, group))
 		goto fail_dec;
 
+	ret = security_create_user_ns(new);
+	if (ret < 0)
+		goto fail_dec;
+
 	ret = -ENOMEM;
 	ns = kmem_cache_zalloc(user_ns_cachep, GFP_KERNEL);
 	if (!ns)
diff --git a/security/security.c b/security/security.c
index 188b8f782220..ec9b4696e86c 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1903,6 +1903,11 @@ 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)
+{
+	return call_int_hook(userns_create, 0, cred);
+}
+
 int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
 {
 	return call_int_hook(ipc_permission, 0, ipcp, flag);
-- 
2.30.2

Re: [PATCH v4 1/4] security, lsm: Introduce security_create_user_ns()

[KP Singh]

On Mon, Aug 1, 2022 at 8:02 PM Frederick Lawler <fred@cloudflare.com> wrote:
>
> 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
> userns_create LSM hook.
>
> This hook takes the prepared creds 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>
> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Reviewed-by: KP Singh <kpsingh@kernel.org>

This looks useful, and I would also like folks to consider the
observability aspects of BPF LSM as
brought up here:

https://lore.kernel.org/all/CAEiveUdPhEPAk7Y0ZXjPsD=Vb5hn453CHzS9aG-tkyRa8bf_eg@mail.gmail.com/

Frederick, what about adding the observability aspects to the commit
description as well.

- KP

>
> ---
> Changes since v3:
> - No changes
> Changes since v2:
> - Rename create_user_ns hook to userns_create
> Changes since v1:
> - Changed commit wording
> - Moved execution to be after id mapping check
> - Changed signature to only accept a const struct cred *
> ---
>  include/linux/lsm_hook_defs.h | 1 +
>  include/linux/lsm_hooks.h     | 4 ++++
>  include/linux/security.h      | 6 ++++++
>  kernel/user_namespace.c       | 5 +++++
>  security/security.c           | 5 +++++
>  5 files changed, 21 insertions(+)
>
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index eafa1d2489fd..7ff93cb8ca8d 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -223,6 +223,7 @@ 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, userns_create, const struct cred *cred)
>  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..54fe534d0e01 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -799,6 +799,10 @@
>   *     security attributes, e.g. for /proc/pid inodes.
>   *     @p contains the task_struct for the task.
>   *     @inode contains the inode structure for the inode.
> + * @userns_create:
> + *     Check permission prior to creating a new user namespace.
> + *     @cred points to prepared creds.
> + *     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..a195bf33246a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -435,6 +435,7 @@ 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);
>  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 +1186,11 @@ 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)
> +{
> +       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..3f464bbda0e9 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>
> @@ -113,6 +114,10 @@ int create_user_ns(struct cred *new)
>             !kgid_has_mapping(parent_ns, group))
>                 goto fail_dec;
>
> +       ret = security_create_user_ns(new);
> +       if (ret < 0)
> +               goto fail_dec;
> +
>         ret = -ENOMEM;
>         ns = kmem_cache_zalloc(user_ns_cachep, GFP_KERNEL);
>         if (!ns)
> diff --git a/security/security.c b/security/security.c
> index 188b8f782220..ec9b4696e86c 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1903,6 +1903,11 @@ 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)
> +{
> +       return call_int_hook(userns_create, 0, cred);
> +}
> +
>  int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
>  {
>         return call_int_hook(ipc_permission, 0, ipcp, flag);
> --
> 2.30.2
>

Re: [PATCH v4 1/4] security, lsm: Introduce security_create_user_ns()

[Frederick Lawler]

On 8/2/22 4:47 PM, KP Singh wrote:
> On Mon, Aug 1, 2022 at 8:02 PM Frederick Lawler <fred@cloudflare.com> wrote:
>>
>> 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
>> userns_create LSM hook.
>>
>> This hook takes the prepared creds 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>
>> Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> 
> Reviewed-by: KP Singh <kpsingh@kernel.org>
> 
> This looks useful, and I would also like folks to consider the
> observability aspects of BPF LSM as
> brought up here:
> 
> https://lore.kernel.org/all/CAEiveUdPhEPAk7Y0ZXjPsD=Vb5hn453CHzS9aG-tkyRa8bf_eg@mail.gmail.com/
> 
> Frederick, what about adding the observability aspects to the commit
> description as well.

Agreed. I'll include that in v5.

> 
> - KP
> 
>>
>> ---
>> Changes since v3:
>> - No changes
>> Changes since v2:
>> - Rename create_user_ns hook to userns_create
>> Changes since v1:
>> - Changed commit wording
>> - Moved execution to be after id mapping check
>> - Changed signature to only accept a const struct cred *
>> ---
>>   include/linux/lsm_hook_defs.h | 1 +
>>   include/linux/lsm_hooks.h     | 4 ++++
>>   include/linux/security.h      | 6 ++++++
>>   kernel/user_namespace.c       | 5 +++++
>>   security/security.c           | 5 +++++
>>   5 files changed, 21 insertions(+)
>>
>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
>> index eafa1d2489fd..7ff93cb8ca8d 100644
>> --- a/include/linux/lsm_hook_defs.h
>> +++ b/include/linux/lsm_hook_defs.h
>> @@ -223,6 +223,7 @@ 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, userns_create, const struct cred *cred)
>>   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..54fe534d0e01 100644
>> --- a/include/linux/lsm_hooks.h
>> +++ b/include/linux/lsm_hooks.h
>> @@ -799,6 +799,10 @@
>>    *     security attributes, e.g. for /proc/pid inodes.
>>    *     @p contains the task_struct for the task.
>>    *     @inode contains the inode structure for the inode.
>> + * @userns_create:
>> + *     Check permission prior to creating a new user namespace.
>> + *     @cred points to prepared creds.
>> + *     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..a195bf33246a 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -435,6 +435,7 @@ 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);
>>   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 +1186,11 @@ 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)
>> +{
>> +       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..3f464bbda0e9 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>
>> @@ -113,6 +114,10 @@ int create_user_ns(struct cred *new)
>>              !kgid_has_mapping(parent_ns, group))
>>                  goto fail_dec;
>>
>> +       ret = security_create_user_ns(new);
>> +       if (ret < 0)
>> +               goto fail_dec;
>> +
>>          ret = -ENOMEM;
>>          ns = kmem_cache_zalloc(user_ns_cachep, GFP_KERNEL);
>>          if (!ns)
>> diff --git a/security/security.c b/security/security.c
>> index 188b8f782220..ec9b4696e86c 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -1903,6 +1903,11 @@ 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)
>> +{
>> +       return call_int_hook(userns_create, 0, cred);
>> +}
>> +
>>   int security_ipc_permission(struct kern_ipc_perm *ipcp, short flag)
>>   {
>>          return call_int_hook(ipc_permission, 0, ipcp, flag);
>> --
>> 2.30.2
>>

[PATCH v4 2/4] bpf-lsm: Make bpf_lsm_userns_create() 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_userns_create() sleepable
for mandatory access control policies.

Signed-off-by: Frederick Lawler <fred@cloudflare.com>
Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>

---
Changes since v3:
- None
Changes since v2:
- Rename create_user_ns hook to userns_create
Changes since v1:
- None
---
 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..4593437809cc 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_userns_create)
 BTF_SET_END(sleepable_lsm_hooks)
 
 bool bpf_lsm_is_sleepable_hook(u32 btf_id)
-- 
2.30.2

Re: [PATCH v4 2/4] bpf-lsm: Make bpf_lsm_userns_create() sleepable

[Alexei Starovoitov]

On Mon, Aug 01, 2022 at 01:01:44PM -0500, Frederick Lawler wrote:
> 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_userns_create() sleepable
> for mandatory access control policies.
> 
> Signed-off-by: Frederick Lawler <fred@cloudflare.com>
> Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>

We can take this set through bpf-next tree if it's easier.

Or if it goes through other trees:
Acked-by: Alexei Starovoitov <ast@kernel.org>

Re: [PATCH v4 2/4] bpf-lsm: Make bpf_lsm_userns_create() sleepable

[Paul Moore]

On Mon, Aug 1, 2022 at 7:00 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, Aug 01, 2022 at 01:01:44PM -0500, Frederick Lawler wrote:
> > 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_userns_create() sleepable
> > for mandatory access control policies.
> >
> > Signed-off-by: Frederick Lawler <fred@cloudflare.com>
> > Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>
>
> We can take this set through bpf-next tree if it's easier.

Thanks Alexei, but I'm currently planning to merge it into the LSM
next branch once the merge window closes.

> Or if it goes through other trees:
> Acked-by: Alexei Starovoitov <ast@kernel.org>

I appreciate the review/ACK, would you mind reviewing the tests too (patch 3/4)?

-- 
paul-moore.com

Re: [PATCH v4 2/4] bpf-lsm: Make bpf_lsm_userns_create() sleepable

[KP Singh]

On Mon, Aug 1, 2022 at 8:02 PM Frederick Lawler <fred@cloudflare.com> wrote:
>
> 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_userns_create() sleepable
> for mandatory access control policies.
>
> Signed-off-by: Frederick Lawler <fred@cloudflare.com>
> Acked-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Acked-by: KP Singh <kpsingh@kernel.org>

[PATCH v4 3/4] selftests/bpf: Add tests verifying bpf lsm userns_create hook

[Frederick Lawler]

The LSM hook userns_create was introduced to provide LSM's an
opportunity to block or allow unprivileged user namespace creation. This
test serves two purposes: it provides a test eBPF implementation, and
tests the hook successfully blocks or allows user namespace creation.

This tests 3 cases:

        1. Unattached bpf program does not block unpriv user namespace
           creation.
        2. Attached bpf program allows user namespace creation given
           CAP_SYS_ADMIN privileges.
        3. Attached bpf program denies user namespace creation for a
           user without CAP_SYS_ADMIN.

Signed-off-by: Frederick Lawler <fred@cloudflare.com>

---
The generic deny_namespace file name is used for future namespace
expansion. I didn't want to limit these files to just the create_user_ns
hook.
Changes since v3:
- Explicitly set CAP_SYS_ADMIN to test namespace is created given
  permission
- Simplify BPF test to use sleepable hook only
- Prefer unshare() over clone() for tests
Changes since v2:
- Rename create_user_ns hook to userns_create
Changes since v1:
- Introduce this patch
---
 .../selftests/bpf/prog_tests/deny_namespace.c | 102 ++++++++++++++++++
 .../selftests/bpf/progs/test_deny_namespace.c |  33 ++++++
 2 files changed, 135 insertions(+)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/deny_namespace.c
 create mode 100644 tools/testing/selftests/bpf/progs/test_deny_namespace.c

diff --git a/tools/testing/selftests/bpf/prog_tests/deny_namespace.c b/tools/testing/selftests/bpf/prog_tests/deny_namespace.c
new file mode 100644
index 000000000000..1bc6241b755b
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/deny_namespace.c
@@ -0,0 +1,102 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <test_progs.h>
+#include "test_deny_namespace.skel.h"
+#include <sched.h>
+#include "cap_helpers.h"
+#include <stdio.h>
+
+static int wait_for_pid(pid_t pid)
+{
+	int status, ret;
+
+again:
+	ret = waitpid(pid, &status, 0);
+	if (ret == -1) {
+		if (errno == EINTR)
+			goto again;
+
+		return -1;
+	}
+
+	if (!WIFEXITED(status))
+		return -1;
+
+	return WEXITSTATUS(status);
+}
+
+/* negative return value -> some internal error
+ * positive return value -> userns creation failed
+ * 0                     -> userns creation succeeded
+ */
+static int create_user_ns(void)
+{
+	pid_t pid;
+
+	pid = fork();
+	if (pid < 0)
+		return -1;
+
+	if (pid == 0) {
+		if (unshare(CLONE_NEWUSER))
+			_exit(EXIT_FAILURE);
+		_exit(EXIT_SUCCESS);
+	}
+
+	return wait_for_pid(pid);
+}
+
+static void test_userns_create_bpf(void)
+{
+	__u32 cap_mask = 1ULL << CAP_SYS_ADMIN;
+	__u64 old_caps = 0;
+
+	cap_enable_effective(cap_mask, &old_caps);
+
+	ASSERT_OK(create_user_ns(), "priv new user ns");
+
+	cap_disable_effective(cap_mask, &old_caps);
+
+	ASSERT_EQ(create_user_ns(), EPERM, "unpriv new user ns");
+
+	if (cap_mask & old_caps)
+		cap_enable_effective(cap_mask, NULL);
+}
+
+static void test_unpriv_userns_create_no_bpf(void)
+{
+	__u32 cap_mask = 1ULL << CAP_SYS_ADMIN;
+	__u64 old_caps = 0;
+
+	cap_disable_effective(cap_mask, &old_caps);
+
+	ASSERT_OK(create_user_ns(), "no-bpf unpriv new user ns");
+
+	if (cap_mask & old_caps)
+		cap_enable_effective(cap_mask, NULL);
+}
+
+void test_deny_namespace(void)
+{
+	struct test_deny_namespace *skel = NULL;
+	int err;
+
+	if (test__start_subtest("unpriv_userns_create_no_bpf"))
+		test_unpriv_userns_create_no_bpf();
+
+	skel = test_deny_namespace__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel load"))
+		goto close_prog;
+
+	err = test_deny_namespace__attach(skel);
+	if (!ASSERT_OK(err, "attach"))
+		goto close_prog;
+
+	if (test__start_subtest("userns_create_bpf"))
+		test_userns_create_bpf();
+
+	test_deny_namespace__detach(skel);
+
+close_prog:
+	test_deny_namespace__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/progs/test_deny_namespace.c b/tools/testing/selftests/bpf/progs/test_deny_namespace.c
new file mode 100644
index 000000000000..09ad5a4ebd1f
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/test_deny_namespace.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <errno.h>
+#include <linux/capability.h>
+
+struct kernel_cap_struct {
+	__u32 cap[_LINUX_CAPABILITY_U32S_3];
+} __attribute__((preserve_access_index));
+
+struct cred {
+	struct kernel_cap_struct cap_effective;
+} __attribute__((preserve_access_index));
+
+char _license[] SEC("license") = "GPL";
+
+SEC("lsm.s/userns_create")
+int BPF_PROG(test_userns_create, const struct cred *cred, int ret)
+{
+	struct kernel_cap_struct caps = cred->cap_effective;
+	int cap_index = CAP_TO_INDEX(CAP_SYS_ADMIN);
+	__u32 cap_mask = CAP_TO_MASK(CAP_SYS_ADMIN);
+
+	if (ret)
+		return 0;
+
+	ret = -EPERM;
+	if (caps.cap[cap_index] & cap_mask)
+		return 0;
+
+	return -EPERM;
+}
-- 
2.30.2

Re: [PATCH v4 3/4] selftests/bpf: Add tests verifying bpf lsm userns_create hook

[KP Singh]

On Mon, Aug 1, 2022 at 8:02 PM Frederick Lawler <fred@cloudflare.com> wrote:
>
> The LSM hook userns_create was introduced to provide LSM's an
> opportunity to block or allow unprivileged user namespace creation. This
> test serves two purposes: it provides a test eBPF implementation, and
> tests the hook successfully blocks or allows user namespace creation.
>
> This tests 3 cases:
>
>         1. Unattached bpf program does not block unpriv user namespace
>            creation.
>         2. Attached bpf program allows user namespace creation given
>            CAP_SYS_ADMIN privileges.
>         3. Attached bpf program denies user namespace creation for a
>            user without CAP_SYS_ADMIN.
>
> Signed-off-by: Frederick Lawler <fred@cloudflare.com>

Looks good to me (Also checked it on vmtest.sh)

Acked-by: KP Singh <kpsingh@kernel.org>

[PATCH v4 4/4] selinux: Implement userns_create hook

[Frederick Lawler]

Unprivileged user namespace creation is an intended feature to enable
sandboxing, however this feature is often used to as an initial step to
perform a privilege escalation attack.

This patch implements a new user_namespace { create } access control
permission to restrict which domains allow or deny user namespace
creation. This is necessary for system administrators to quickly protect
their systems while waiting for vulnerability patches to be applied.

This permission can be used in the following way:

        allow domA_t domA_t : user_namespace { create };

Signed-off-by: Frederick Lawler <fred@cloudflare.com>

---
Changes since v3:
- None
Changes since v2:
- Rename create_user_ns hook to userns_create
- Use user_namespace as an object opposed to a generic namespace object
- s/domB_t/domA_t in commit message
Changes since v1:
- Introduce this patch
---
 security/selinux/hooks.c            | 9 +++++++++
 security/selinux/include/classmap.h | 2 ++
 2 files changed, 11 insertions(+)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index beceb89f68d9..afc9da0249e7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4227,6 +4227,14 @@ static void selinux_task_to_inode(struct task_struct *p,
 	spin_unlock(&isec->lock);
 }
 
+static int selinux_userns_create(const struct cred *cred)
+{
+	u32 sid = current_sid();
+
+	return avc_has_perm(&selinux_state, sid, sid, SECCLASS_USER_NAMESPACE,
+						USER_NAMESPACE__CREATE, NULL);
+}
+
 /* Returns error only if unable to parse addresses */
 static int selinux_parse_skb_ipv4(struct sk_buff *skb,
 			struct common_audit_data *ad, u8 *proto)
@@ -7117,6 +7125,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(task_movememory, selinux_task_movememory),
 	LSM_HOOK_INIT(task_kill, selinux_task_kill),
 	LSM_HOOK_INIT(task_to_inode, selinux_task_to_inode),
+	LSM_HOOK_INIT(userns_create, selinux_userns_create),
 
 	LSM_HOOK_INIT(ipc_permission, selinux_ipc_permission),
 	LSM_HOOK_INIT(ipc_getsecid, selinux_ipc_getsecid),
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index ff757ae5f253..0bff55bb9cde 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -254,6 +254,8 @@ const struct security_class_mapping secclass_map[] = {
 	  { COMMON_FILE_PERMS, NULL } },
 	{ "io_uring",
 	  { "override_creds", "sqpoll", NULL } },
+	{ "user_namespace",
+	  { "create", NULL } },
 	{ NULL }
   };
 
-- 
2.30.2

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

[Eric W. Biederman]

Frederick Lawler <fred@cloudflare.com> writes:

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

Re-nack for all of the same reasons.
AKA This can only break the users of the user namespace.

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

You aren't fixing what your problem you are papering over it by denying
access to the user namespace.

Nack Nack Nack.

Stop.

Go back to the drawing board.

Do not pass go.

Do not collect $200.


> The calls look something like this:
>
>     cred = prepare_creds()
>         security_prepare_creds()
>             call_int_hook(cred_prepare, ...
>     if (cred)
>         create_user_ns(cred)
>
> We noticed that error codes were not propagated from this hook and
> introduced a patch [1] to propagate those errors.
>
> The discussion notes that security_prepare_creds()
> is not appropriate for MAC policies, and instead the hook is
> meant for LSM authors to prepare credentials for mutation. [2]
>
> Ultimately, we concluded that a better course of action is to introduce
> a new security hook for LSM authors. [3]
>
> This patch set first introduces a new security_create_user_ns() function
> and userns_create LSM hook, then marks the hook as sleepable in BPF.
>
> 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/
>
> Past discussions:
> V3: https://lore.kernel.org/all/20220721172808.585539-1-fred@cloudflare.com/
> V2: https://lore.kernel.org/all/20220707223228.1940249-1-fred@cloudflare.com/
> V1: https://lore.kernel.org/all/20220621233939.993579-1-fred@cloudflare.com/
>
> Changes since v3:
> - Explicitly set CAP_SYS_ADMIN to test namespace is created given
>   permission
> - Simplify BPF test to use sleepable hook only
> - Prefer unshare() over clone() for tests
> Changes since v2:
> - Rename create_user_ns hook to userns_create
> - Use user_namespace as an object opposed to a generic namespace object
> - s/domB_t/domA_t in commit message
> Changes since v1:
> - Add selftests/bpf: Add tests verifying bpf lsm create_user_ns hook patch
> - Add selinux: Implement create_user_ns hook patch
> - Change function signature of security_create_user_ns() to only take
>   struct cred
> - Move security_create_user_ns() call after id mapping check in
>   create_user_ns()
> - Update documentation to reflect changes
>
> Frederick Lawler (4):
>   security, lsm: Introduce security_create_user_ns()
>   bpf-lsm: Make bpf_lsm_userns_create() sleepable
>   selftests/bpf: Add tests verifying bpf lsm userns_create hook
>   selinux: Implement userns_create hook
>
>  include/linux/lsm_hook_defs.h                 |   1 +
>  include/linux/lsm_hooks.h                     |   4 +
>  include/linux/security.h                      |   6 ++
>  kernel/bpf/bpf_lsm.c                          |   1 +
>  kernel/user_namespace.c                       |   5 +
>  security/security.c                           |   5 +
>  security/selinux/hooks.c                      |   9 ++
>  security/selinux/include/classmap.h           |   2 +
>  .../selftests/bpf/prog_tests/deny_namespace.c | 102 ++++++++++++++++++
>  .../selftests/bpf/progs/test_deny_namespace.c |  33 ++++++
>  10 files changed, 168 insertions(+)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/deny_namespace.c
>  create mode 100644 tools/testing/selftests/bpf/progs/test_deny_namespace.c

Eric

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

[Paul Moore]

On Mon, Aug 1, 2022 at 10:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Frederick Lawler <fred@cloudflare.com> writes:
>
> > 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().
>
> Re-nack for all of the same reasons.
> AKA This can only break the users of the user namespace.
>
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> You aren't fixing what your problem you are papering over it by denying
> access to the user namespace.
>
> Nack Nack Nack.
>
> Stop.
>
> Go back to the drawing board.
>
> Do not pass go.
>
> Do not collect $200.

If you want us to take your comments seriously Eric, you need to
provide the list with some constructive feedback that would allow
Frederick to move forward with a solution to the use case that has
been proposed.  You response above may be many things, but it is
certainly not that.

We've heard from different users now that there are very real use
cases for this LSM hook.  I understand you are concerned about adding
additional controls to user namespaces, but these are controls
requested by real users, and the controls being requested (LSM hooks,
with BPF and SELinux implementations) are configurable by the *users*
at *runtime*.  This patchset does not force additional restrictions on
user namespaces, it provides a mechanism that *users* can leverage to
add additional granularity to the access controls surrounding user
namespaces.

Eric, if you have a different approach in mind to adding a LSM hook to
user namespace creation I think we would all very much like to hear
about it.  However, if you do not have any suggestions along those
lines, and simply want to NACK any effort to add a LSM hook to user
namespace creation, I think we all understand your point of view and
respectfully disagree.  Barring any new approaches or suggestions, I
think Frederick's patches look reasonable and I still plan on merging
them into the LSM next branch when the merge window closes.

-- 
paul-moore.com

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

[Eric W. Biederman]

Paul Moore <paul@paul-moore.com> writes:

> On Mon, Aug 1, 2022 at 10:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Frederick Lawler <fred@cloudflare.com> writes:
>>
>> > 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().
>>
>> Re-nack for all of the same reasons.
>> AKA This can only break the users of the user namespace.
>>
>> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>
>> You aren't fixing what your problem you are papering over it by denying
>> access to the user namespace.
>>
>> Nack Nack Nack.
>>
>> Stop.
>>
>> Go back to the drawing board.
>>
>> Do not pass go.
>>
>> Do not collect $200.
>
> If you want us to take your comments seriously Eric, you need to
> provide the list with some constructive feedback that would allow
> Frederick to move forward with a solution to the use case that has
> been proposed.  You response above may be many things, but it is
> certainly not that.

I did provide constructive feedback.  My feedback to his problem
was to address the real problem of bugs in the kernel.

It is not a constructive approach to shoot the messenger
and is not a constructive approach to blow me off every time you
reply.

I have proposed that is there is a subsystem that is unduly buggy we
stop it from being enabled with a user-namespaces.

Further this is a hook really should have extra-ordinary requirements,
as all it can do is add additional failure modes to something that
does not really fail.  AKA all it can do is break-userspace.

As such I need to see a justification on why it makes sense to
break-userspace.

> We've heard from different users now that there are very real use
> cases for this LSM hook.  I understand you are concerned about adding
 > additional controls to user namespaces, but these are controls
> requested by real users, and the controls being requested (LSM hooks,
> with BPF and SELinux implementations) are configurable by the *users*
> at *runtime*.  This patchset does not force additional restrictions on
> user namespaces, it provides a mechanism that *users* can leverage to
> add additional granularity to the access controls surrounding user
> namespaces.

But that is not the problem that cloudfare encountered and are trying to
solve.

At least that is not what I was told when I asked early in the review
cycle.

All saying that is user-configurable does is shift the blame from the
kernel maintainers to the users.  Shift the responsibility from people
who should have enough expertise to know what is going on to people
who are by definition have other concerns, so are less likely to be as
well informed, and less likely to come up with good solutions.

> Eric, if you have a different approach in mind to adding a LSM hook to
> user namespace creation I think we would all very much like to hear
> about it.  However, if you do not have any suggestions along those
> lines, and simply want to NACK any effort to add a LSM hook to user
> namespace creation, I think we all understand your point of view and
> respectfully disagree.  Barring any new approaches or suggestions, I
> think Frederick's patches look reasonable and I still plan on merging
> them into the LSM next branch when the merge window closes.


But it is my code you are planning to merge this into, and your are
asking me to support something.

I admit I have not had time to read everything.  I am sick and tired
and quite frankly very tired that people are busy wanting to shoot
the messenger to the fact that there are bugs in the kernel.

I am speaking up and engaging as best as I can with objections that
are not hot-air.

You are very much proposing to merge code that can only cause
regressions and cause me grief.  At least that is all I see.  I don't
see anything in the change descriptions of the change that refutes that.

I don't see any interaction in fact with my concerns.

In fact your last reply was to completely blow off my request on how to
address the concerns that inspired this patch and to say other people
have a use too.

At this point I am happy to turn your request around and ask that you
address my concerns and not blow them off.  As I have seen no
constructive engagement with my concerns.   I think that is reasonable
as by definition I will get the support issues when some LSM has some
ill-thought out idea of how things should work and I get the bug report.

Eric

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

[Paul Moore]

On Mon, Aug 8, 2022 at 2:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Paul Moore <paul@paul-moore.com> writes:
> > On Mon, Aug 1, 2022 at 10:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >> Frederick Lawler <fred@cloudflare.com> writes:
> >>
> >> > 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().
> >>
> >> Re-nack for all of the same reasons.
> >> AKA This can only break the users of the user namespace.
> >>
> >> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >>
> >> You aren't fixing what your problem you are papering over it by denying
> >> access to the user namespace.
> >>
> >> Nack Nack Nack.
> >>
> >> Stop.
> >>
> >> Go back to the drawing board.
> >>
> >> Do not pass go.
> >>
> >> Do not collect $200.
> >
> > If you want us to take your comments seriously Eric, you need to
> > provide the list with some constructive feedback that would allow
> > Frederick to move forward with a solution to the use case that has
> > been proposed.  You response above may be many things, but it is
> > certainly not that.
>
> I did provide constructive feedback.  My feedback to his problem
> was to address the real problem of bugs in the kernel.

We've heard from several people who have use cases which require
adding LSM-level access controls and observability to user namespace
creation.  This is the problem we are trying to solve here; if you do
not like the approach proposed in this patchset please suggest another
implementation that allows LSMs visibility into user namespace
creation.

-- 
paul-moore.com

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

[Eric W. Biederman]

Paul Moore <paul@paul-moore.com> writes:

>> I did provide constructive feedback.  My feedback to his problem
>> was to address the real problem of bugs in the kernel.
>
> We've heard from several people who have use cases which require
> adding LSM-level access controls and observability to user namespace
> creation.  This is the problem we are trying to solve here; if you do
> not like the approach proposed in this patchset please suggest another
> implementation that allows LSMs visibility into user namespace
> creation.

Please stop, ignoring my feedback, not detailing what problem or
problems you are actually trying to be solved, and threatening to merge
code into files that I maintain that has the express purpose of breaking
my users.

You just artificially constrained the problems, so that no other
solution is acceptable.  On that basis alone I am object to this whole
approach to steam roll over me and my code.

Eric

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

[Eric W. Biederman]

"Eric W. Biederman" <ebiederm@xmission.com> writes:

> Paul Moore <paul@paul-moore.com> writes:
>
>>> I did provide constructive feedback.  My feedback to his problem
>>> was to address the real problem of bugs in the kernel.
>>
>> We've heard from several people who have use cases which require
>> adding LSM-level access controls and observability to user namespace
>> creation.  This is the problem we are trying to solve here; if you do
>> not like the approach proposed in this patchset please suggest another
>> implementation that allows LSMs visibility into user namespace
>> creation.
>
> Please stop, ignoring my feedback, not detailing what problem or
> problems you are actually trying to be solved, and threatening to merge
> code into files that I maintain that has the express purpose of breaking
> my users.
>
> You just artificially constrained the problems, so that no other
> solution is acceptable.  On that basis alone I am object to this whole
> approach to steam roll over me and my code.

If you want an example of what kind of harm it can cause to introduce a
failure where no failure was before I invite you to look at what
happened with sendmail when setuid was modified to fail, when changing
the user of a process would cause RLIMIT_NPROC to be exceeded.

I am not arguing that what you are proposing is that bad but unexpected
failures cause real problems, and at a minimum that needs a better
response than: "There is at least one user that wants a failure here".

Frankly I would love to see an argument that semantically it ever makes
sense for creating a user namespace to fail.  If that argument has
already been made, my apologies to the person who made as I missed it,
in being sick and tired, and frustrated at being blown off, when
I asked for a proper discuss of the problem at hand.

Eric

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

[Paul Moore]

On Mon, Aug 8, 2022 at 3:43 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> "Eric W. Biederman" <ebiederm@xmission.com> writes:
> > Paul Moore <paul@paul-moore.com> writes:
> >
> >>> I did provide constructive feedback.  My feedback to his problem
> >>> was to address the real problem of bugs in the kernel.
> >>
> >> We've heard from several people who have use cases which require
> >> adding LSM-level access controls and observability to user namespace
> >> creation.  This is the problem we are trying to solve here; if you do
> >> not like the approach proposed in this patchset please suggest another
> >> implementation that allows LSMs visibility into user namespace
> >> creation.
> >
> > Please stop, ignoring my feedback, not detailing what problem or
> > problems you are actually trying to be solved, and threatening to merge
> > code into files that I maintain that has the express purpose of breaking
> > my users.
> >
> > You just artificially constrained the problems, so that no other
> > solution is acceptable.  On that basis alone I am object to this whole
> > approach to steam roll over me and my code.
>
> If you want an example of what kind of harm it can cause to introduce a
> failure where no failure was before I invite you to look at what
> happened with sendmail when setuid was modified to fail, when changing
> the user of a process would cause RLIMIT_NPROC to be exceeded.

I think we are all familiar with the sendmail capabilities bug and the
others like it, but using that as an excuse to block additional access
controls seems very weak.  The Linux Kernel is very different from
when the sendmail bug hit (what was that, ~20 years ago?), with
advancements in capabilities and other discretionary controls, as well
as mandatory access controls which have enabled Linux to be certified
through a number of third party security evaluations.

> I am not arguing that what you are proposing is that bad but unexpected
> failures cause real problems, and at a minimum that needs a better
> response than: "There is at least one user that wants a failure here".

Let me fix that for you: "There are multiple users who want to have
better visibility and access control for user namespace creation."

--
paul-moore.com

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

[Eric W. Biederman]

Paul Moore <paul@paul-moore.com> writes:

> On Mon, Aug 8, 2022 at 3:43 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>> > Paul Moore <paul@paul-moore.com> writes:
>> >
>> >>> I did provide constructive feedback.  My feedback to his problem
>> >>> was to address the real problem of bugs in the kernel.
>> >>
>> >> We've heard from several people who have use cases which require
>> >> adding LSM-level access controls and observability to user namespace
>> >> creation.  This is the problem we are trying to solve here; if you do
>> >> not like the approach proposed in this patchset please suggest another
>> >> implementation that allows LSMs visibility into user namespace
>> >> creation.
>> >
>> > Please stop, ignoring my feedback, not detailing what problem or
>> > problems you are actually trying to be solved, and threatening to merge
>> > code into files that I maintain that has the express purpose of breaking
>> > my users.
>> >
>> > You just artificially constrained the problems, so that no other
>> > solution is acceptable.  On that basis alone I am object to this whole
>> > approach to steam roll over me and my code.
>>
>> If you want an example of what kind of harm it can cause to introduce a
>> failure where no failure was before I invite you to look at what
>> happened with sendmail when setuid was modified to fail, when changing
>> the user of a process would cause RLIMIT_NPROC to be exceeded.
>
> I think we are all familiar with the sendmail capabilities bug and the
> others like it, but using that as an excuse to block additional access
> controls seems very weak.  The Linux Kernel is very different from
> when the sendmail bug hit (what was that, ~20 years ago?), with
> advancements in capabilities and other discretionary controls, as well
> as mandatory access controls which have enabled Linux to be certified
> through a number of third party security evaluations.

If you are familiar with scenarios like that then why is there not
being due diligence performed to ensure that userspace won't break?

Certainly none of the paperwork you are talking about does that kind
of checking and it most definitely is not happening before the code
gets merged. 

I am saying that performing that due diligence should be a requirement
before anyone even thinks about merging a patch that adds permission
checks where no existed before.

Sometimes changes to fix security bugs can get away with adding new
restrictions because we know with a very very high degree of probability
that the only thing that will break will be exploit code.  In the rare
case when real world applications are broken such changes need to be
reverted or adapted.  No one has even made the argument that only
exploit code will be affected.

So I am sorry I am the one who has to be the one to get in the way of a
broken process with semantic review,  but due diligence has not been
done.  So I am say no way this code should be merged.


In addition to actually breaking existing userspace, I think there is a
very real danger of breaking userspace, I think there is a very real
danger of breaking network effects by making such a large change to the
design of user namespaces.


>> I am not arguing that what you are proposing is that bad but unexpected
>> failures cause real problems, and at a minimum that needs a better
>> response than: "There is at least one user that wants a failure here".
>
> Let me fix that for you: "There are multiple users who want to have
> better visibility and access control for user namespace creation."

Visibility sure.  Design a proper hook for that.  All the proposed hook
can do is print an audit message.  It can't allocate or manage any state
as there is not the corresponding hook when a user namespace is freed.
So the proposed hook is not appropriate for increasing visibility.


Access control.  Not a chance unless it is carefully designed and
reviewed.  There is a very large cost to adding access control where
it has not previously existed.

I talk about that cost as people breaking my users as that is how I see
it.  I don't see any discussion on why I am wrong.

If we are going to add an access controls I want to see someone point
out something that is actually semantically a problem.  What motivates
an access control?

So far the only answer I have received is people want to reduce the
attack surface of the kernel.  I don't possibly see how reducing the
attack surface by removing user namespaces makes the probability of
having an exploitable kernel bug, anything approaching zero.

So I look at the calculus.  Chance of actually breaking userspace, or
preventing people with a legitimate use from using user namespaces > 0%.
Chance of actually preventing a determined attacker from exploiting the
kernel < 1%.  Amount of work to maintain, non-zero, and I really don't
like it.

Lots of work to achieve nothing but breaking some of my users.

So please stop trying to redesign my subsystem and cause me headaches,
unless you are going to do the due diligence necessary to do so
responsibly.

Eric

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

[Paul Moore]

On Tue, Aug 9, 2022 at 12:08 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Paul Moore <paul@paul-moore.com> writes:
> > On Mon, Aug 8, 2022 at 3:43 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >> "Eric W. Biederman" <ebiederm@xmission.com> writes:
> >> > Paul Moore <paul@paul-moore.com> writes:
> >> >
> >> >>> I did provide constructive feedback.  My feedback to his problem
> >> >>> was to address the real problem of bugs in the kernel.
> >> >>
> >> >> We've heard from several people who have use cases which require
> >> >> adding LSM-level access controls and observability to user namespace
> >> >> creation.  This is the problem we are trying to solve here; if you do
> >> >> not like the approach proposed in this patchset please suggest another
> >> >> implementation that allows LSMs visibility into user namespace
> >> >> creation.
> >> >
> >> > Please stop, ignoring my feedback, not detailing what problem or
> >> > problems you are actually trying to be solved, and threatening to merge
> >> > code into files that I maintain that has the express purpose of breaking
> >> > my users.
> >> >
> >> > You just artificially constrained the problems, so that no other
> >> > solution is acceptable.  On that basis alone I am object to this whole
> >> > approach to steam roll over me and my code.
> >>
> >> If you want an example of what kind of harm it can cause to introduce a
> >> failure where no failure was before I invite you to look at what
> >> happened with sendmail when setuid was modified to fail, when changing
> >> the user of a process would cause RLIMIT_NPROC to be exceeded.
> >
> > I think we are all familiar with the sendmail capabilities bug and the
> > others like it, but using that as an excuse to block additional access
> > controls seems very weak.  The Linux Kernel is very different from
> > when the sendmail bug hit (what was that, ~20 years ago?), with
> > advancements in capabilities and other discretionary controls, as well
> > as mandatory access controls which have enabled Linux to be certified
> > through a number of third party security evaluations.
>
> If you are familiar with scenarios like that then why is there not
> being due diligence performed to ensure that userspace won't break?

What level of due diligence would satisfy you Eric?  This feels very
much like an unbounded ask that can be used to permanently block any
effort to add any form of additional access control around user
namespace creation.  If that isn't the case, and this request is being
made in good faith, please elaborate on what you believe would be
sufficient analysis of this patch?

Personally, the fact that the fork()/clone() variants and the
unshare() syscall all can fail and return error codes to userspace
seems like a reasonable level of safety.  If userspace is currently
ignoring the return values of fork/clone/unshare that is a problem
independent of this patchset.  Even looking at the existing
create_user_ns() function shows several cases where the user namespace
code can forcibly error out due to access controls, memory pressure,
etc.  I also see that additional restrictions were put on user
namespace creation after it was introduced, e.g. the chroot
restriction; I'm assuming that didn't break "your" users?  If you can
describe the analysis you did on that change, perhaps we can do the
same for this patchset and call it sufficient, yes?

> >> I am not arguing that what you are proposing is that bad but unexpected
> >> failures cause real problems, and at a minimum that needs a better
> >> response than: "There is at least one user that wants a failure here".
> >
> > Let me fix that for you: "There are multiple users who want to have
> > better visibility and access control for user namespace creation."
>
> Visibility sure.  Design a proper hook for that.  All the proposed hook
> can do is print an audit message.  It can't allocate or manage any state
> as there is not the corresponding hook when a user namespace is freed.
> So the proposed hook is not appropriate for increasing visibility.

Auditing very much increases visibility.  Look at what the BPF LSM can
do, observability is one of its primary goals.

> Access control.  Not a chance unless it is carefully designed and
> reviewed.

See the above.  The relevant syscalls already have the risk of
failure, if userspace is assuming fork/clone/unshare/etc. will never
fail then that application is broken independent of this discussion.

-- 
paul-moore.com

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

[Eric W. Biederman]

Paul Moore <paul@paul-moore.com> writes:
>
> What level of due diligence would satisfy you Eric?

Having a real conversation about what a change is doing and to talk
about it's merits and it's pro's and cons.  I can't promise I would be
convinced but that is the kind of conversation it would take.

I was not trying to place an insurmountable barrier I was simply looking
to see if people had been being careful and doing what is generally
accepted for submitting a kernel patch.  From all I can see that has
completely not happened here.

> If that isn't the case, and this request is being made in good faith

Again you are calling me a liar. I really don't appreciate that.

As for something already returning an error.  The setuid system call
also has error returns, and enforcing RLIMIT_NPROC caused sendmail to
misbehave.

I bring up the past in this way only to illustrate that things can
happen.  That simply examining the kernel and not thinking about
userspace really isn't enough.

I am also concerned about the ecosystem effects of adding random access
control checks to a system call that does not perform access control
checks.

As I said this patch is changing a rather fundamental design decision by
adding an access control.  A design decision that for the most part has
worked out quite well, and has allowed applications to add sandboxing
support to themselves without asking permission to anyone.

Adding an access control all of a sudden means application developers
are having to ask for permission to things that are perfectly safe,
and it means many parts of the kernel gets less love both in use
and in maintenance.

It might be possible to convince me that design decision needs to
change, or that what is being proposed is small enough it does not
practically change that design decision.

Calling me a liar is not the way to change my mind.  Ignoring me
and pushing this through without addressing my concerns is not
the way to change my mind.

I honestly I want what I asked for at the start.  I want discussion of
what problems are being solved so we can talk about the problem or
problems and if this is even the appropriate solution to them.

Eric

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

[Paul Moore]

On Tue, Aug 9, 2022 at 5:41 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Paul Moore <paul@paul-moore.com> writes:
> >
> > What level of due diligence would satisfy you Eric?
>
> Having a real conversation about what a change is doing and to talk
> about it's merits and it's pro's and cons.  I can't promise I would be
> convinced but that is the kind of conversation it would take.

Earlier today you talked about due diligence to ensure that userspace
won't break and I provided my reasoning on why userspace would not
break (at least not because of this change).  Userspace might be
blocked from creating a new user namespace due to a security policy,
but that would be the expected and desired outcome, not breakage.  As
far as your most recent comment regarding merit and pros/cons, I
believe we have had that discussion (quite a few times already); it
just seems you are not satisfied with the majority's conclusion.

Personally, I'm not sure there is anything more I can do to convince
you that this patchset is reasonable; I'm going to leave it to others
at this point, or we can all simply agree to disagree for the moment.
Just as you haven't heard a compelling argument for this patchset, I
haven't heard a compelling argument against it.  Barring some
significant new discussion point, or opinion, I still plan on merging
this into the LSM next branch when the merge window closes next week
so it has time to go through a full round of linux-next testing.
Assuming no unresolvable problems are found during the additional
testing I plan to send it to Linus during the v6.1 merge window and
I'm guessing we will get to go through this all again.  It's less than
ideal, but I think this is where we are at right now.

-- 
paul-moore.com

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

[Alexei Starovoitov]

On Tue, Aug 9, 2022 at 3:40 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Tue, Aug 9, 2022 at 5:41 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > Paul Moore <paul@paul-moore.com> writes:
> > >
> > > What level of due diligence would satisfy you Eric?
> >
> > Having a real conversation about what a change is doing and to talk
> > about it's merits and it's pro's and cons.  I can't promise I would be
> > convinced but that is the kind of conversation it would take.
>
> Earlier today you talked about due diligence to ensure that userspace
> won't break and I provided my reasoning on why userspace would not
> break (at least not because of this change).  Userspace might be
> blocked from creating a new user namespace due to a security policy,
> but that would be the expected and desired outcome, not breakage.  As
> far as your most recent comment regarding merit and pros/cons, I
> believe we have had that discussion (quite a few times already); it
> just seems you are not satisfied with the majority's conclusion.
>
> Personally, I'm not sure there is anything more I can do to convince
> you that this patchset is reasonable; I'm going to leave it to others
> at this point, or we can all simply agree to disagree for the moment.
> Just as you haven't heard a compelling argument for this patchset, I
> haven't heard a compelling argument against it.  Barring some
> significant new discussion point, or opinion, I still plan on merging
> this into the LSM next branch when the merge window closes next week
> so it has time to go through a full round of linux-next testing.
> Assuming no unresolvable problems are found during the additional
> testing I plan to send it to Linus during the v6.1 merge window and
> I'm guessing we will get to go through this all again.  It's less than
> ideal, but I think this is where we are at right now.

+1

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

[Casey Schaufler]

On 8/9/2022 9:07 AM, Eric W. Biederman wrote:
> Paul Moore <paul@paul-moore.com> writes:
>
>> On Mon, Aug 8, 2022 at 3:43 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>> "Eric W. Biederman" <ebiederm@xmission.com> writes:
>>>> Paul Moore <paul@paul-moore.com> writes:
>>>>
>>>>>> I did provide constructive feedback.  My feedback to his problem
>>>>>> was to address the real problem of bugs in the kernel.
>>>>> We've heard from several people who have use cases which require
>>>>> adding LSM-level access controls and observability to user namespace
>>>>> creation.  This is the problem we are trying to solve here; if you do
>>>>> not like the approach proposed in this patchset please suggest another
>>>>> implementation that allows LSMs visibility into user namespace
>>>>> creation.
>>>> Please stop, ignoring my feedback, not detailing what problem or
>>>> problems you are actually trying to be solved, and threatening to merge
>>>> code into files that I maintain that has the express purpose of breaking
>>>> my users.
>>>>
>>>> You just artificially constrained the problems, so that no other
>>>> solution is acceptable.  On that basis alone I am object to this whole
>>>> approach to steam roll over me and my code.
>>> If you want an example of what kind of harm it can cause to introduce a
>>> failure where no failure was before I invite you to look at what
>>> happened with sendmail when setuid was modified to fail, when changing
>>> the user of a process would cause RLIMIT_NPROC to be exceeded.
>> I think we are all familiar with the sendmail capabilities bug and the
>> others like it, but using that as an excuse to block additional access
>> controls seems very weak.  The Linux Kernel is very different from
>> when the sendmail bug hit (what was that, ~20 years ago?), with
>> advancements in capabilities and other discretionary controls, as well
>> as mandatory access controls which have enabled Linux to be certified
>> through a number of third party security evaluations.
> If you are familiar with scenarios like that then why is there not
> being due diligence performed to ensure that userspace won't break?
>
> Certainly none of the paperwork you are talking about does that kind
> of checking and it most definitely is not happening before the code
> gets merged. 
>
> I am saying that performing that due diligence should be a requirement
> before anyone even thinks about merging a patch that adds permission
> checks where no existed before.
>
> Sometimes changes to fix security bugs can get away with adding new
> restrictions because we know with a very very high degree of probability
> that the only thing that will break will be exploit code.  In the rare
> case when real world applications are broken such changes need to be
> reverted or adapted.  No one has even made the argument that only
> exploit code will be affected.
>
> So I am sorry I am the one who has to be the one to get in the way of a
> broken process with semantic review,  but due diligence has not been
> done.  So I am say no way this code should be merged.
>
>
> In addition to actually breaking existing userspace, I think there is a
> very real danger of breaking userspace, I think there is a very real
> danger of breaking network effects by making such a large change to the
> design of user namespaces.
>
>
>>> I am not arguing that what you are proposing is that bad but unexpected
>>> failures cause real problems, and at a minimum that needs a better
>>> response than: "There is at least one user that wants a failure here".
>> Let me fix that for you: "There are multiple users who want to have
>> better visibility and access control for user namespace creation."
> Visibility sure.  Design a proper hook for that.  All the proposed hook
> can do is print an audit message.  It can't allocate or manage any state
> as there is not the corresponding hook when a user namespace is freed.
> So the proposed hook is not appropriate for increasing visibility.
>
>
> Access control.  Not a chance unless it is carefully designed and
> reviewed.  There is a very large cost to adding access control where
> it has not previously existed.
>
> I talk about that cost as people breaking my users as that is how I see
> it.  I don't see any discussion on why I am wrong.
>
> If we are going to add an access controls I want to see someone point
> out something that is actually semantically a problem.  What motivates
> an access control?

Smack has no interest in using the proposed hook because user namespaces
are neither subjects nor objects. They are collections of DAC and/or
privilege configuration alternatives. Or something like that. From the
viewpoint of a security module that only implements old fashioned MAC
there is no value in constraining user namespaces.

SELinux, on the other hand, seeks to be comprehensive well beyond
controlling accesses between subjects and objects. Asking the question
"should there be a control on this operation?" seems sufficient to justify
adding the control to SELinux policy. This is characteristic of
"Fine Grain" control.

So I'm of two minds on this. I don't need the hook, but I also understand
why SELinux and BPF want it. I don't necessarily agree with their logic,
but it is consistent with existing behavior. Any system that uses either
of those security modules needs to be ready for unexpected denials based
on any potential security concern. Keeping namespaces completely orthogonal
to LSM seems doomed to failure eventually.

>
> So far the only answer I have received is people want to reduce the
> attack surface of the kernel.  I don't possibly see how reducing the
> attack surface by removing user namespaces makes the probability of
> having an exploitable kernel bug, anything approaching zero.
>
> So I look at the calculus.  Chance of actually breaking userspace, or
> preventing people with a legitimate use from using user namespaces > 0%.
> Chance of actually preventing a determined attacker from exploiting the
> kernel < 1%.  Amount of work to maintain, non-zero, and I really don't
> like it.
>
> Lots of work to achieve nothing but breaking some of my users.
>
> So please stop trying to redesign my subsystem and cause me headaches,
> unless you are going to do the due diligence necessary to do so
> responsibly.
>
> Eric

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

[Eric W. Biederman]

Casey Schaufler <casey@schaufler-ca.com> writes:

> Smack has no interest in using the proposed hook because user namespaces
> are neither subjects nor objects. They are collections of DAC and/or
> privilege configuration alternatives. Or something like that. From the
> viewpoint of a security module that only implements old fashioned MAC
> there is no value in constraining user namespaces.

The goal is to simply enable things that become safe when you don't have
to worry about confusing setuid executables.

> SELinux, on the other hand, seeks to be comprehensive well beyond
> controlling accesses between subjects and objects. Asking the question
> "should there be a control on this operation?" seems sufficient to justify
> adding the control to SELinux policy. This is characteristic of
> "Fine Grain" control.

I see that from a theoretical standpoint.  On the flip side I prefer
arguments grounded in something more than what an abstract framework
could appreciate.  We are so far from any theoretical purity in the
linux kernel that I can't see theoretical purity being enough to justify
a decision like this.

> So I'm of two minds on this. I don't need the hook, but I also understand
> why SELinux and BPF want it. I don't necessarily agree with their logic,
> but it is consistent with existing behavior. Any system that uses either
> of those security modules needs to be ready for unexpected denials based
> on any potential security concern. Keeping namespaces completely orthogonal
> to LSM seems doomed to failure eventually.

I can cross that bridge when there is a healthy conversation about such
a change.

Too often I get "ouch! Creating a user namespace was used as the easiest
way to exploit a security bug. Let's solve the issue by denying user
namespaces."  Which leads to half thought out policies made out of fear.

Which is where I think this conversation started.  I haven't seen it
make it's way to any healthy reasons to deny user namespaces yet.

Eric

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

[Paul Moore]

On Mon, Aug 8, 2022 at 3:26 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Paul Moore <paul@paul-moore.com> writes:
> >> I did provide constructive feedback.  My feedback to his problem
> >> was to address the real problem of bugs in the kernel.
> >
> > We've heard from several people who have use cases which require
> > adding LSM-level access controls and observability to user namespace
> > creation.  This is the problem we are trying to solve here; if you do
> > not like the approach proposed in this patchset please suggest another
> > implementation that allows LSMs visibility into user namespace
> > creation.
>
> Please stop, ignoring my feedback, not detailing what problem or
> problems you are actually trying to be solved, and threatening to merge
> code into files that I maintain that has the express purpose of breaking
> my users.

I've heard you talk about bugs being the only reason why people would
want to ever block user namespaces, but I think we've all seen use
cases now where it goes beyond that.  However, even if it didn't, the
need to build high confidence/assurance systems where big chunks of
functionality can be disabled based on a security policy is a very
real use case, and this patchset would help enable that.  I've noticed
you like to talk about these hooks being a source of "regressions",
but access controls are not regressions Eric, they are tools that
system builders, administrators, and users use to secure their
systems.

From my perspective, I believe that addresses your feedback around
"fix the bugs" and "this is a regression", which is the only thing
I've noted from your responses in this thread and others, but if I'm
missing something more technical please let me/us know.

> You just artificially constrained the problems, so that no other
> solution is acceptable.

There is a real need to be able to gain both additional visibility and
access control over user namespace creation, please suggest the
approach(es) you would find acceptable.

> On that basis alone I am object to this whole
> approach to steam roll over me and my code.

I saw that choice of wording in your last email and thought it a bit
curious, so I did a quick git log dump on kernel/user_namespace.c and
I see approximately 31 contributors to that one file.  I've always
thought of the open source maintainer role as more of a "steward" and
less of an "owner", but that's just my opinion.

-- 
paul-moore.com

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

[Eric W. Biederman]

Paul Moore <paul@paul-moore.com> writes:

> On Mon, Aug 8, 2022 at 3:26 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>> Paul Moore <paul@paul-moore.com> writes:
>> >> I did provide constructive feedback.  My feedback to his problem
>> >> was to address the real problem of bugs in the kernel.
>> >
>> > We've heard from several people who have use cases which require
>> > adding LSM-level access controls and observability to user namespace
>> > creation.  This is the problem we are trying to solve here; if you do
>> > not like the approach proposed in this patchset please suggest another
>> > implementation that allows LSMs visibility into user namespace
>> > creation.
>>
>> Please stop, ignoring my feedback, not detailing what problem or
>> problems you are actually trying to be solved, and threatening to merge
>> code into files that I maintain that has the express purpose of breaking
>> my users.
>
> I've heard you talk about bugs being the only reason why people would
> want to ever block user namespaces, but I think we've all seen use
> cases now where it goes beyond that.

I really have not, and I don't appreciate being called a liar.

> However, even if it didn't, the
> need to build high confidence/assurance systems where big chunks of
> functionality can be disabled based on a security policy is a very
> real use case, and this patchset would help enable that.

Details please.  What does this look like.  What is the overall plan for
attack surface reduction in one of these systems.  How does it differ
from seccomp?

How does this differ from setting /proc/sys/user/max_usernamespaces to 0?

Why is it only the user namespace that needs to be modified to implement
such a system?

Why is there no discussion of that in the change description.

> I've noticed
> you like to talk about these hooks being a source of "regressions",
> but access controls are not regressions Eric, they are tools that
> system builders, administrators, and users use to secure their
> systems.
>
> From my perspective, I believe that addresses your feedback around
> "fix the bugs" and "this is a regression", which is the only thing
> I've noted from your responses in this thread and others, but if I'm
> missing something more technical please let me/us know.

Which is a short way of saying that the using this hook for attack
surface reduction without a larger plan will be ineffective.  If the
attack surface is not sufficiently reduced it will not achieve a
prevention of exploits and the attacks will still happen and be
successful.

With a change that is designed to prevent exploits not actually doing so
all that is left is breaking userspace and causing maintenance problems.

Earlier I asked to confirm that was the only reason cloudfare was
interested in this change.  I have asked that we have an actual
conversation about what is trying to be achieved.

Instead the conversation has simply been about implementation issues
and not about if the code will be worth having.  So far in my book the
code very much does not look worth having.  That is my technical
judgment and I don't see anyone taking about my arguments or even
really engaging in them.

Since I keep getting blown off, instead of having my concerns addressed
I say this code should not go.


>> You just artificially constrained the problems, so that no other
>> solution is acceptable.
>
> There is a real need to be able to gain both additional visibility and
> access control over user namespace creation, please suggest the
> approach(es) you would find acceptable.

The suggested hook is not at all appropriate for visibility.  Either the
user namespace needs to have some state that can be set, or there needs
to be something that is notified when the user namespace goes away.  At
best the hook can print an audit message.  So the proposed hook is
really not appropriate to add visibility to the user namespace.

For the record I don't object to adding visibility, I am just pointing
out the proposed hook is not appropriate to that task.


What is the need to have an access control?

Why do you need to fundamentally change the design of user namespaces?

Those are questions I have not seen any answers to.  Without actual
answers of what the actual problems are I can't have a reasonable
technical conversation.

>> On that basis alone I am object to this whole
>> approach to steam roll over me and my code.
>
> I saw that choice of wording in your last email and thought it a bit
> curious, so I did a quick git log dump on kernel/user_namespace.c and
> I see approximately 31 contributors to that one file.  I've always
> thought of the open source maintainer role as more of a "steward" and
> less of an "owner", but that's just my opinion.

As such it is unfortunately my responsibility to say no to badly thought
out proposals.  Proposals that will negatively affect the people using
the code I maintain.


My apologies if I have not been more elegant right now when I have been
constantly sick, and tired.  People getting scared of user namespaces
for no real reason has been an on-going trend for a decade or so.  This
isn't a new issue, and it irritates me that it is still going on.  I
have addressed real concerns and fixed code, for many many years.

This round of the people being afraid of user namespaces, I have yet to
find any real concerns.

So when I express my concerns that this is a pointless exercise and
people don't address my concern.  I say no.

Eric

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

[Serge E. Hallyn]

On Mon, Aug 08, 2022 at 03:16:16PM -0400, Paul Moore wrote:
> On Mon, Aug 8, 2022 at 2:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > Paul Moore <paul@paul-moore.com> writes:
> > > On Mon, Aug 1, 2022 at 10:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > >> Frederick Lawler <fred@cloudflare.com> writes:
> > >>
> > >> > 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().
> > >>
> > >> Re-nack for all of the same reasons.
> > >> AKA This can only break the users of the user namespace.
> > >>
> > >> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > >>
> > >> You aren't fixing what your problem you are papering over it by denying
> > >> access to the user namespace.
> > >>
> > >> Nack Nack Nack.
> > >>
> > >> Stop.
> > >>
> > >> Go back to the drawing board.
> > >>
> > >> Do not pass go.
> > >>
> > >> Do not collect $200.
> > >
> > > If you want us to take your comments seriously Eric, you need to
> > > provide the list with some constructive feedback that would allow
> > > Frederick to move forward with a solution to the use case that has
> > > been proposed.  You response above may be many things, but it is
> > > certainly not that.
> >
> > I did provide constructive feedback.  My feedback to his problem
> > was to address the real problem of bugs in the kernel.
> 
> We've heard from several people who have use cases which require
> adding LSM-level access controls and observability to user namespace
> creation.  This is the problem we are trying to solve here; if you do
> not like the approach proposed in this patchset please suggest another
> implementation that allows LSMs visibility into user namespace
> creation.

Regarding the observability - can someone concisely lay out why just
auditing userns creation would not suffice?  Userspace could decide
what to report based on whether the creating user_ns == /proc/1/ns/user...

Regarding limiting the tweaking of otherwise-privileged code by
unprivileged users, i wonder whether we could instead add smarts to
ns_capable().  Point being, uid mapping would still work, but we'd
break the "privileged against resources you own" part of user
namespaces.  I would want it to default to allow, but then when a
0-day is found which requires reaching ns_capable() code, admins 
could easily prevent exploitation until reboot from a fixed kernel.

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

[Paul Moore]

On Sun, Aug 14, 2022 at 11:55 AM Serge E. Hallyn <serge@hallyn.com> wrote:
> On Mon, Aug 08, 2022 at 03:16:16PM -0400, Paul Moore wrote:
> > On Mon, Aug 8, 2022 at 2:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > > Paul Moore <paul@paul-moore.com> writes:
> > > > On Mon, Aug 1, 2022 at 10:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > > >> Frederick Lawler <fred@cloudflare.com> writes:
> > > >>
> > > >> > 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().
> > > >>
> > > >> Re-nack for all of the same reasons.
> > > >> AKA This can only break the users of the user namespace.
> > > >>
> > > >> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > > >>
> > > >> You aren't fixing what your problem you are papering over it by denying
> > > >> access to the user namespace.
> > > >>
> > > >> Nack Nack Nack.
> > > >>
> > > >> Stop.
> > > >>
> > > >> Go back to the drawing board.
> > > >>
> > > >> Do not pass go.
> > > >>
> > > >> Do not collect $200.
> > > >
> > > > If you want us to take your comments seriously Eric, you need to
> > > > provide the list with some constructive feedback that would allow
> > > > Frederick to move forward with a solution to the use case that has
> > > > been proposed.  You response above may be many things, but it is
> > > > certainly not that.
> > >
> > > I did provide constructive feedback.  My feedback to his problem
> > > was to address the real problem of bugs in the kernel.
> >
> > We've heard from several people who have use cases which require
> > adding LSM-level access controls and observability to user namespace
> > creation.  This is the problem we are trying to solve here; if you do
> > not like the approach proposed in this patchset please suggest another
> > implementation that allows LSMs visibility into user namespace
> > creation.
>
> Regarding the observability - can someone concisely lay out why just
> auditing userns creation would not suffice?  Userspace could decide
> what to report based on whether the creating user_ns == /proc/1/ns/user...

One of the selling points of the BPF LSM is that it allows for various
different ways of reporting and logging beyond audit.  However, even
if it was limited to just audit I believe that provides some useful
justification as auditing fork()/clone() isn't quite the same and
could be difficult to do at scale in some configurations.  I haven't
personally added a BPF LSM program to the kernel so I can't speak to
the details on what is possible, but I'm sure others on the To/CC line
could help provide more information if that is important to you.

> Regarding limiting the tweaking of otherwise-privileged code by
> unprivileged users, i wonder whether we could instead add smarts to
> ns_capable().

The existing security_capable() hook is eventually called by ns_capable():

  ns_capable()
    ns_capable_common()
      security_capable(const struct cred *cred,
                       struct user_namespace *ns,
                       int cap,
                       unsigned int opts);

... I'm not sure what additional smarts would be useful here?

[side note: SELinux does actually distinguish between capability
checks in the initial user namespace vs child namespaces]

> Point being, uid mapping would still work, but we'd
> break the "privileged against resources you own" part of user
> namespaces.  I would want it to default to allow, but then when a
> 0-day is found which requires reaching ns_capable() code, admins
> could easily prevent exploitation until reboot from a fixed kernel.

That assumes that everything you care about is behind a capability
check, which is probably going to be correct in a lot of the cases,
but I think it would be a mistake to assume that is always going to be
true.

--
paul-moore.com

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

[Serge E. Hallyn]

On Sun, Aug 14, 2022 at 10:32:51PM -0400, Paul Moore wrote:
> On Sun, Aug 14, 2022 at 11:55 AM Serge E. Hallyn <serge@hallyn.com> wrote:
> > On Mon, Aug 08, 2022 at 03:16:16PM -0400, Paul Moore wrote:
> > > On Mon, Aug 8, 2022 at 2:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > > > Paul Moore <paul@paul-moore.com> writes:
> > > > > On Mon, Aug 1, 2022 at 10:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > > > >> Frederick Lawler <fred@cloudflare.com> writes:
> > > > >>
> > > > >> > 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().
> > > > >>
> > > > >> Re-nack for all of the same reasons.
> > > > >> AKA This can only break the users of the user namespace.
> > > > >>
> > > > >> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > > > >>
> > > > >> You aren't fixing what your problem you are papering over it by denying
> > > > >> access to the user namespace.
> > > > >>
> > > > >> Nack Nack Nack.
> > > > >>
> > > > >> Stop.
> > > > >>
> > > > >> Go back to the drawing board.
> > > > >>
> > > > >> Do not pass go.
> > > > >>
> > > > >> Do not collect $200.
> > > > >
> > > > > If you want us to take your comments seriously Eric, you need to
> > > > > provide the list with some constructive feedback that would allow
> > > > > Frederick to move forward with a solution to the use case that has
> > > > > been proposed.  You response above may be many things, but it is
> > > > > certainly not that.
> > > >
> > > > I did provide constructive feedback.  My feedback to his problem
> > > > was to address the real problem of bugs in the kernel.
> > >
> > > We've heard from several people who have use cases which require
> > > adding LSM-level access controls and observability to user namespace
> > > creation.  This is the problem we are trying to solve here; if you do
> > > not like the approach proposed in this patchset please suggest another
> > > implementation that allows LSMs visibility into user namespace
> > > creation.
> >
> > Regarding the observability - can someone concisely lay out why just
> > auditing userns creation would not suffice?  Userspace could decide
> > what to report based on whether the creating user_ns == /proc/1/ns/user...
> 
> One of the selling points of the BPF LSM is that it allows for various
> different ways of reporting and logging beyond audit.  However, even
> if it was limited to just audit I believe that provides some useful
> justification as auditing fork()/clone() isn't quite the same and
> could be difficult to do at scale in some configurations.  I haven't
> personally added a BPF LSM program to the kernel so I can't speak to
> the details on what is possible, but I'm sure others on the To/CC line
> could help provide more information if that is important to you.
> 
> > Regarding limiting the tweaking of otherwise-privileged code by
> > unprivileged users, i wonder whether we could instead add smarts to
> > ns_capable().
> 
> The existing security_capable() hook is eventually called by ns_capable():
> 
>   ns_capable()
>     ns_capable_common()
>       security_capable(const struct cred *cred,
>                        struct user_namespace *ns,
>                        int cap,
>                        unsigned int opts);
> 
> ... I'm not sure what additional smarts would be useful here?

Oh - i wasn't necessarily thinking of an LSM.  I was picturing a
sysctl next to unprivileged_userns_clone.  But you're right, looks
like an LSM could already do this.  Of course, there's an issue early
on in that the root user in the new namespace couldn't setuid, so
the uid mapping is still limited.  So this idea probably isn't worth
the characters we've typed about it so far, sorry.

> [side note: SELinux does actually distinguish between capability
> checks in the initial user namespace vs child namespaces]
> 
> > Point being, uid mapping would still work, but we'd
> > break the "privileged against resources you own" part of user
> > namespaces.  I would want it to default to allow, but then when a
> > 0-day is found which requires reaching ns_capable() code, admins
> > could easily prevent exploitation until reboot from a fixed kernel.
> 
> That assumes that everything you care about is behind a capability
> check, which is probably going to be correct in a lot of the cases,
> but I think it would be a mistake to assume that is always going to be
> true.

I might be thinking wrongly, but if it's not behind a capability check,
then it seems to me it's not an exploit that can only be reached by
becoming root in a user namespace, which means disabling user namespace
creation by unprivileged users will not stop the attack.

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

[Paul Moore]

On Mon, Aug 15, 2022 at 11:41 AM Serge E. Hallyn <serge@hallyn.com> wrote:
> On Sun, Aug 14, 2022 at 10:32:51PM -0400, Paul Moore wrote:
> > On Sun, Aug 14, 2022 at 11:55 AM Serge E. Hallyn <serge@hallyn.com> wrote:
> > > On Mon, Aug 08, 2022 at 03:16:16PM -0400, Paul Moore wrote:
> > > > On Mon, Aug 8, 2022 at 2:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > > > > Paul Moore <paul@paul-moore.com> writes:
> > > > > > On Mon, Aug 1, 2022 at 10:56 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> > > > > >> Frederick Lawler <fred@cloudflare.com> writes:
> > > > > >>
> > > > > >> > 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().
> > > > > >>
> > > > > >> Re-nack for all of the same reasons.
> > > > > >> AKA This can only break the users of the user namespace.
> > > > > >>
> > > > > >> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > > > > >>
> > > > > >> You aren't fixing what your problem you are papering over it by denying
> > > > > >> access to the user namespace.
> > > > > >>
> > > > > >> Nack Nack Nack.
> > > > > >>
> > > > > >> Stop.
> > > > > >>
> > > > > >> Go back to the drawing board.
> > > > > >>
> > > > > >> Do not pass go.
> > > > > >>
> > > > > >> Do not collect $200.
> > > > > >
> > > > > > If you want us to take your comments seriously Eric, you need to
> > > > > > provide the list with some constructive feedback that would allow
> > > > > > Frederick to move forward with a solution to the use case that has
> > > > > > been proposed.  You response above may be many things, but it is
> > > > > > certainly not that.
> > > > >
> > > > > I did provide constructive feedback.  My feedback to his problem
> > > > > was to address the real problem of bugs in the kernel.
> > > >
> > > > We've heard from several people who have use cases which require
> > > > adding LSM-level access controls and observability to user namespace
> > > > creation.  This is the problem we are trying to solve here; if you do
> > > > not like the approach proposed in this patchset please suggest another
> > > > implementation that allows LSMs visibility into user namespace
> > > > creation.
> > >
> > > Regarding the observability - can someone concisely lay out why just
> > > auditing userns creation would not suffice?  Userspace could decide
> > > what to report based on whether the creating user_ns == /proc/1/ns/user...
> >
> > One of the selling points of the BPF LSM is that it allows for various
> > different ways of reporting and logging beyond audit.  However, even
> > if it was limited to just audit I believe that provides some useful
> > justification as auditing fork()/clone() isn't quite the same and
> > could be difficult to do at scale in some configurations.  I haven't
> > personally added a BPF LSM program to the kernel so I can't speak to
> > the details on what is possible, but I'm sure others on the To/CC line
> > could help provide more information if that is important to you.
> >
> > > Regarding limiting the tweaking of otherwise-privileged code by
> > > unprivileged users, i wonder whether we could instead add smarts to
> > > ns_capable().
> >
> > The existing security_capable() hook is eventually called by ns_capable():
> >
> >   ns_capable()
> >     ns_capable_common()
> >       security_capable(const struct cred *cred,
> >                        struct user_namespace *ns,
> >                        int cap,
> >                        unsigned int opts);
> >
> > ... I'm not sure what additional smarts would be useful here?
>
> Oh - i wasn't necessarily thinking of an LSM.  I was picturing a
> sysctl next to unprivileged_userns_clone.  But you're right, looks
> like an LSM could already do this.  Of course, there's an issue early
> on in that the root user in the new namespace couldn't setuid, so
> the uid mapping is still limited.  So this idea probably isn't worth
> the characters we've typed about it so far, sorry.

No harm, no foul.  This thread has already reached record lows with
respect to usefulness-vs-characters ratio, a few more isn't going to
hurt anything ;)

> > [side note: SELinux does actually distinguish between capability
> > checks in the initial user namespace vs child namespaces]
> >
> > > Point being, uid mapping would still work, but we'd
> > > break the "privileged against resources you own" part of user
> > > namespaces.  I would want it to default to allow, but then when a
> > > 0-day is found which requires reaching ns_capable() code, admins
> > > could easily prevent exploitation until reboot from a fixed kernel.
> >
> > That assumes that everything you care about is behind a capability
> > check, which is probably going to be correct in a lot of the cases,
> > but I think it would be a mistake to assume that is always going to be
> > true.
>
> I might be thinking wrongly, but if it's not behind a capability check,
> then it seems to me it's not an exploit that can only be reached by
> becoming root in a user namespace, which means disabling user namespace
> creation by unprivileged users will not stop the attack.

I was primarily thinking about two things: subj/obj relationships
which are really not addressed with capability checks, and unrelated
problems which aren't the fault of the user namespace but could be
somehow made easier through some of the unique situations offered by
user namespaces.  There are exploits that often require chaining
together multiple "things" to trigger the necessary flaw, and
sometimes the most immediate way to stop such an attack is to apply
additional controls to one of these intermediate steps.  Frekerick's
work puts the necessary infrastructure in place so we can do that with
user namespaces.

-- 
paul-moore.com