Date:   Tue, 18 Jan 2022 13:09:12 -0500
From:   Stefan Berger <stefanb@...ux.ibm.com>
To:     Christian Brauner <brauner@...nel.org>,
        Stefan Berger <stefanb@...ux.vnet.ibm.com>
Cc:     linux-integrity@...r.kernel.org, zohar@...ux.ibm.com,
        serge@...lyn.com, christian.brauner@...ntu.com,
        containers@...ts.linux.dev, dmitry.kasatkin@...il.com,
        ebiederm@...ssion.com, krzysztof.struczynski@...wei.com,
        roberto.sassu@...wei.com, mpeters@...hat.com, lhinds@...hat.com,
        lsturman@...hat.com, puiterwi@...hat.com, jejb@...ux.ibm.com,
        jamjoom@...ibm.com, linux-kernel@...r.kernel.org,
        paul@...l-moore.com, rgb@...hat.com,
        linux-security-module@...r.kernel.org, jmorris@...ei.org
Subject: Re: [PATCH v8 19/19] ima: Enable IMA namespaces


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

Powered by blists - more mailing lists