Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Frederick Lawler <fred@cloudflare.com>
Cc:     kpsingh@kernel.org, revest@chromium.org, jackmanb@chromium.org,
        ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        kafai@fb.com, songliubraving@fb.com, yhs@fb.com,
        john.fastabend@gmail.com, jmorris@namei.org, serge@hallyn.com,
        paul@paul-moore.com, stephen.smalley.work@gmail.com,
        eparis@parisplace.org, shuah@kernel.org, brauner@kernel.org,
        casey@schaufler-ca.com, bpf@vger.kernel.org,
        linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
        linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, kernel-team@cloudflare.com,
        cgzones@googlemail.com, karl@bigbadwolfsecurity.com
References: <20220721172808.585539-1-fred@cloudflare.com>
Date:   Fri, 22 Jul 2022 12:05:07 -0500
In-Reply-To: <20220721172808.585539-1-fred@cloudflare.com> (Frederick Lawler's
        message of "Thu, 21 Jul 2022 12:28:04 -0500")
Message-ID: <877d45kri4.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH v3 0/4] Introduce security_create_user_ns()
Precedence: bulk

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

That description is wrong.  Your goal his is not to limit access to
the user namespace.  Your goal is to reduce the attack surface of the
kernel by not allowing some processes access to a user namespace.

You have already said that you don't have concerns about the
fundamentals of the user namespace, and what it enables only that
it allows access to exploitable code.

Achieving the protection you seek requires talking and thinking clearly
about the goal.


I have a couple of deep and fundamental problems with this approach,
to limiting access to potentially exploitable code.

1) The first is that unless there is a high probability (say 90%) that at
   any time the only exploitable code in the kernel can only be accessed
   by an unprivileged user with the help of user namespaces, attackers
   will just route around this restriction and so it will achieve
   nothing in practice, while at the same time incur an extra
   maintenance burden.

2) The second is that there is a long standing problem with code that
   gets added to the kernel.  Many times new kernel code because it has
   the potential to confuse suid root executables that code has been
   made root only.  Over time that results in more and more code running
   as root to be able to make use of the useful features of the linux
   kernel.

   One of the goals of the user namespace is to avoid more and more code
   migrating to running as root.  To achieve that goal ordinary
   application developers need to be able to assume that typically user
   namespaces will be available on linux.

   An assumption that ordinary applications like chromium make today.

   Your intentions seem to be to place a capability check so that only
   root can use user namespaces or something of the sort.  Thus breaking
   the general availability of user namespaces for ordinary applications
   on your systems.
   

My apologies if this has been addressed somewhere in the conversation
already.  I don't see these issues addressed in the descriptions of your
patches.

Until these issues are firmly addressed and you are not proposing a
patch that can only cause regressions in userspace applications.

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

>
> 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:
> 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 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 | 88 +++++++++++++++++++
>  .../selftests/bpf/progs/test_deny_namespace.c | 39 ++++++++
>  10 files changed, 160 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

Eric