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