Re: [PATCH v4 10/16] ima: Implement hierarchical processing of file accesses

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 12/15/21 18:04, Mimi Zohar wrote:
On Wed, 2021-12-08 at 13:22 -0500, Stefan Berger wrote:
On 12/8/21 11:50, Stefan Berger wrote:
On 12/8/21 07:23, Christian Brauner wrote:
On Wed, Dec 08, 2021 at 01:09:54PM +0100, Christian Brauner wrote:
On Tue, Dec 07, 2021 at 03:21:21PM -0500, Stefan Berger wrote:
Implement hierarchical processing of file accesses in IMA
namespaces by
walking the list of IMA namespaces towards the init_ima_ns. This way
file accesses can be audited in an IMA namespace and also be evaluated
against the IMA policies of parent IMA namespaces.

Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
---
   security/integrity/ima/ima_main.c | 29 +++++++++++++++++++++++++----
   1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_main.c
b/security/integrity/ima/ima_main.c
index 2121a831f38a..e9fa46eedd27 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -200,10 +200,10 @@ void ima_file_free(struct file *file)
       ima_check_last_writer(iint, inode, file);
   }
   -static int process_measurement(struct ima_namespace *ns,
-                   struct file *file, const struct cred *cred,
-                   u32 secid, char *buf, loff_t size, int mask,
-                   enum ima_hooks func)
+static int _process_measurement(struct ima_namespace *ns,
Hm, it's much more common to use double underscores then single
underscores to

__process_measurement()

reads a lot more natural to people perusing kernel code quite often.

+                struct file *file, const struct cred *cred,
+                u32 secid, char *buf, loff_t size, int mask,
+                enum ima_hooks func)
   {
       struct inode *inode = file_inode(file);
       struct integrity_iint_cache *iint = NULL;
@@ -405,6 +405,27 @@ static int process_measurement(struct
ima_namespace *ns,
       return 0;
   }
   +static int process_measurement(struct ima_namespace *ns,
+                   struct file *file, const struct cred *cred,
+                   u32 secid, char *buf, loff_t size, int mask,
+                   enum ima_hooks func)
+{
+    int ret = 0;
+    struct user_namespace *user_ns;
+
+    do {
+        ret = _process_measurement(ns, file, cred, secid, buf,
size, mask, func);
+        if (ret)
+            break;
+        user_ns = ns->user_ns->parent;
+        if (!user_ns)
+            break;
+        ns = user_ns->ima_ns;
+    } while (1);
I'd rather write this as:

     struct user_namespace *user_ns = ns->user_ns;

     while (user_ns) {
         ns = user_ns->ima_ns;

             ret = __process_measurement(ns, file, cred, secid, buf,
size, mask, func);
             if (ret)
                 break;
         user_ns = user_ns->parent;

     }

because the hierarchy is only an implicit property inherited by ima
namespaces from the implementation of user namespaces. In other words,
we're only indirectly walking a hierarchy of ima namespaces because
we're walking a hierarchy of user namespaces. So the ima ns actually
just gives us the entrypoint into the userns hierarchy which the double
deref writing it with a while() makes obvious.
Which brings me to another point.

Technically nothing seems to prevent an ima_ns to survive the
destruction of its associated userns in ima_ns->user_ns?

One thread does get_ima_ns() and mucks around with it while another one
does put_user_ns().

Assume it's the last reference to the userns which is now -
asynchronously - cleaned up from ->work. So at some point you're ending
with a dangling pointer in ima_ns->user_ns eventually causing a UAF.

If I'm thinking correct than you need to fix this. I can think of two
ways right now where one of them I'm not sure how well that would work:
1. ima_ns takes a reference count to userns at creation. Here you need
     to make very sure that you're not ending up with reference counting
     cycles where the two structs keep each other alive.
Right. I am not sure what the trigger would be for ima_ns to release
that one reference.


2. rcu trickery. That's the one I'm not sure how well that would work
     where you'd need rcu_read_lock()/rcu_read_unlock() with a
     get_user_ns() in the middle whenever you're trying to get a ref to
     the userns from an ima_ns and handle the case where the userns is
     gone.

Or maybe I'me missing something in the patch series that makes this all
a non-issue.
I suppose one can always call current_user_ns() to get a pointer to
the current user namespace that the process is accessing the file in
that IMA now reacts to. With the hierarchical processing we are
walking backwards towards init_user_ns. The problem should only exist
if something else frees the current user namespace (or its parents) so
that the hierarchy collapses. Assuming we are always in a process
context then 'current' should protect us, no ?

All existing callers to process_measurements call it at least once with
current_cred().

The only problem that I see where we are accessing the IMA namespace
outside a process context is in 4/16 'ima: Move delayed work queue and
variables into ima_namespace' where a delayed work queue is used. I
fixed this now by getting an additional reference to the user namesapce
before scheduling the delayed work and release it when it ran or when it
is canceled (cancel_delayed_work_sync()) but it didn't run.

 From  the "ima: Move delayed work queue and variables into
ima_namespace" patch description:
    Since keys queued up for measurement currently are only relevant in
    the init_ima_ns, call ima_init_key_queue() only when the init_ima_ns
    is initialized.

When IMA_QUEUE_EARLY_BOOT_KEYS is not enabled, ima_should_queue_key()
simply returns false.  Why do the keys workqueue need to be namespaced?
Is this preparatory for some future IMA namespacing?


06 ima: Move policy related variables into ima_namespace

05 ima: Move IMA's keys queue related variables into ima_namespace

04 ima: Move delayed work queue and variables into ima_namespace


06 requires the ima_namespace parameter to be passed into process_buffer_measurement(). The problem was ima_process_queued_keys() that needs to pass the namespace but it's probably sufficient to use &init_ima_ns there as the ima_namespace parameter, which would allow to drop 05 and 04.

  Stefan


thanks,

Mimi




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux