Re: [PATCH v8 19/19] ima: Enable IMA namespaces

[Christian Brauner <brauner@kernel.org>]

On Tue, Jan 18, 2022 at 01:09:12PM -0500, Stefan Berger wrote:
> 
> On 1/14/22 09:45, Christian Brauner wrote:
> > On Tue, Jan 04, 2022 at 12:04:16PM -0500, Stefan Berger wrote:
> > > 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>
> > > ---
> [...]
> > > 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.

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

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

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

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.

The examples really were just trying to make obvious what the semantics
are that you're buying.