On Wed, Jan 19, 2022 at 07:45:22AM -0500, Stefan Berger wrote:

On 1/19/22 04:46, Christian Brauner wrote:

quoted

On Tue, Jan 18, 2022 at 01:09:12PM -0500, Stefan Berger wrote:

quoted

On 1/14/22 09:45, Christian Brauner wrote:

quoted

On Tue, Jan 04, 2022 at 12:04:16PM -0500, Stefan Berger wrote:

quoted

From: Stefan Berger <stefanb@linux.ibm.com>

Introduce the IMA_NS in Kconfig for IMA namespace enablement.

Enable the lazy initialization of an IMA namespace when a user mounts
SecurityFS. Now a user_namespace will get a pointer to an ima_namespace
and therefore add an implementation of get_current_ns() that returns this
pointer.

get_current_ns() may now return a NULL pointer for as long as the IMA
namespace hasn't been created, yet. Therefore, return early from those
functions that may now get a NULL pointer from this call. The NULL
pointer can typically be treated similar to not having an IMA policy set
and simply return early from a function.

Implement ima_ns_from_file() for SecurityFS-related files where we can
now get the IMA namespace via the user namespace pointer associated
with the superblock of the SecurityFS filesystem instance. Since
the functions using ima_ns_from_file() will only be called after an
ima_namesapce has been allocated they will never get a NULL pointer
for the ima_namespace.

Switch access to userns->ima_ns to use acquire/release semantics to ensure
that a newly created ima_namespace structure is fully visible upon access.

Replace usage of current_user_ns() with ima_ns_from_user_ns() that
implements a method to derive the user_namespace from the given
ima_namespace. It leads to the same result.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---

[...]

quoted

quoted

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index b7dbc687b6ff..5a9b511ebbae 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c

@@ -1333,6 +1333,7 @@ static unsigned int ima_parse_appraise_algos(char *arg)
   static int ima_parse_rule(struct ima_namespace *ns,
   			  char *rule, struct ima_rule_entry *entry)
   {
+	struct user_namespace *user_ns = ima_ns_to_user_ns(ns);

So I think ima_policy_write() and therefore ima_parse_rule() can
legitimately be reached at least from an ancestor userns but also from a
completely unrelated userns via securityfs. Sorry, I didn't see this
earlier. Think of the following two scenarios:

* userns1: unshare -U --map-root --mount
-----------------------------------------
     mount -t securityfs securityfs /userns1_securityfs
     fd_in_userns1 = open("/userns1_securityfs/ima_file, O_RDWR);

     /* I _think_ that sending of fds here should work but I haven't
      * bothered to recheck the scm code as I need to do some driving in a
      * little bit so I'm running out of time...
      */
     send_fd_scm_rights(fd_in_userns1, task_in_userns2);

* userns2: unshare -U --map-root --mount
-----------------------------------------
     fd_from_userns1 = receive_fd_scm_rights();
     write_policy(fd_from_userns1, "my fancy policy");

Passing an fd around like this presumably indicates that you intend to let
the recipient read/write to it.

Yes.

quoted

quoted

It also means that if you inherit an fd from a more privileged imans
instance you can write to it:

Now in this example we have to assume that root is making a mistake passing
the file descriptor around?

# ls -l /sys/kernel/security/ima/
total 0
-r--r-----. 1 root root 0 Jan 18 12:48 ascii_runtime_measurements
-r--r-----. 1 root root 0 Jan 18 12:48 binary_runtime_measurements
-rw-------. 1 root root 0 Jan 18 12:48 policy
-r--r-----. 1 root root 0 Jan 18 12:48 runtime_measurements_count
-r--r-----. 1 root root 0 Jan 18 12:48 violations

quoted

* initial_userns:

So that's the host, right? And this is a 2nd independent example from the
first.

Yes, these are just two examples to give a more complete idea of the
semantics by specifying two cases and how ima would behave.

quoted

quoted

------------------
     mount -t securityfs securityfs /initial_securityfs

     fd_in_initial_securityfs = open("/initial_securityfs/ima_file, O_RDWR);

     pid = fork():
     if (pid == 0) {
	unshare(CLONE_NEWUSER);
	/* write idmapping for yourself */

	write_policy(fd_in_initial_securityfs, "my fancy policy");
     }

would allow an unprivileged caller to alter the host's ima policy (as
you can see the example requires cooperation).

Sorry, not currently following. Root is the only one being able to open that
IMA files on the host, right? Is this a mistake here where root passed the

Yes.

quoted

fd onto the child and that child is not trusted to mess with the fd
including passing it on further?

This is just an example what the current semantics mean in practice.
The above code snippet is neither good nor bad by itself as that depends
on context:

1) Let's say for whatever reason you would like to let unprivileged
    containers add policy rules (sorry in case I'm using the wrong ima
    vernacular) for themselves to the initial ima namespace during
    startup. That can be a rather valid and important use-case. Then this
    code snipped above where root opens a policy fd in the host
    securityfs instance and then let's the container access it across
    fork() + post namespace creation is "good" as it will allow the
    container to write the rules during setup while e.g. letting the
    container manager process (the code prior to fork) continue doing
    other stuff.

I would agree to supporting passing the fd to other containers to have them
add rules to the policy, if that's what is intended.

quoted

2) If you only want to ever allow container manager on the host write
    rules for the container in the initial ima ns but never the container
    setup process itself then the above code is "bad". The policy fd
    should've been closed before the fork() and definitely be opened
    o-cloexec.

I would treat the IMA files' file descriptors like those of fd =
open("/top/secret/file", O_RDWR) assuming the programmer knows the
implications of passing the fd around, including knowing that open fds are
inherited by child processes... I don't see that there's anything wrong with
that.

Agreed. And we do already have these semantics for a range of fds
including splitting open and write between callers and namespaces.