| Message ID | 20211207202127.1508689-11-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
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. But that's just how I'd conceptualize it. This you should do however you prefer.
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. 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.
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 ?
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.
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@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. > 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? thanks, Mimi
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@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. >> > 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 >
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, + 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); + + return ret; +} + /** * ima_file_mmap - based on policy, collect/store measurement. * @file: pointer to the file to be measured (May be NULL)
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(-)