On 1/14/22 09:45, Christian Brauner wrote:

On Tue, Jan 04, 2022 at 12:04:16PM -0500, Stefan Berger wrote:

quoted

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

[...]

quoted

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.