Messages in this thread | | | Date | Tue, 18 Jan 2022 13:09:12 -0500 | Subject | Re: [PATCH v8 19/19] ima: Enable IMA namespaces | From | 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@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.
> 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.
| |