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

[Stefan Berger]

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@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > ---
[...]
> > 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.


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

> ------------------

>     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 fd onto the child and that child is not trusted to mess with 
the fd including passing it on further?


> In both cases the write can legitimately reach ima_policy_write() and
> trigger ima_parse_rule() from another user namespace.
>
> There are multiple ways to go here, I think.
>
> It's important to figure out whether - coming back to an earlier review
> of mine - you're ok with everyone with access to an opened policy fd
> being able to write an ima policy for the namespace in questions as long
> as _the opener of the policy file_ was privileged enough.
>
> If that's the case then you can just remove the WARN_ON()/add a
> non-WARN_ON() helper in there.
>
>  From my ima-naive perspective this seems fine and preferable as this
> means clean permission checking once at open time.
>
> A good question to answer in order to solve this is whether or not a
> given operation is allowed is dependent on what is written, i.e. on the
> content of the rule, I guess. I don't think there is.