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

Stefan Berger
2022-01-19T12:45:36Z

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.