Messages in this thread | | | Date | Wed, 19 Jan 2022 07:45:22 -0500 | Subject | Re: [PATCH v8 19/19] ima: Enable IMA namespaces | From | Stefan Berger <> |
| |
On 1/19/22 04:46, Christian Brauner wrote: > 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.
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.
> > 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.
> > The examples really were just trying to make obvious what the semantics > are that you're buying.
| |