Messages in this thread | | | Date | Wed, 8 Dec 2021 11:50:26 -0500 | Subject | Re: [PATCH v4 10/16] ima: Implement hierarchical processing of file accesses | From | Stefan Berger <> |
| |
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@linux.ibm.com> >>> --- >>> 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 ?
| |