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:Which brings me to another point.
Implement hierarchical processing of file accesses in IMA namespaces byHm, it's much more common to use double underscores then single
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,
underscores to
__process_measurement()
reads a lot more natural to people perusing kernel code quite often.
+ struct file *file, const struct cred *cred,I'd rather write this as:
+ 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);
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.
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.
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.