| Message ID | 20220104170416.1923685-11-stefanb@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
On Tue, Jan 04, 2022 at 12:04:07PM -0500, Stefan Berger wrote: > From: Stefan Berger <stefanb@linux.ibm.com> > > Implement hierarchical processing of file accesses in IMA namespaces by > walking the list of user namespaces towards the root. This way file > accesses can be audited in an IMA namespace and also be evaluated against > the IMA policies of parent IMA namespaces. > > __process_measurement() returns either 0 or -EACCES. For hierarchical > processing remember the -EACCES returned by this function but continue > to the parent user namespace. At the end either return 0 or -EACCES > if an error occurred in one of the IMA namespaces. > > Currently the ima_ns pointer of the user_namespace is always NULL except > at the init_user_ns, so test ima_ns for NULL pointer and skip the call to > __process_measurement() if it is NULL. Once IMA namespacing is fully > enabled, the pointer may also be NULL due to late initialization of the > IMA namespace. > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> > --- > include/linux/ima.h | 6 +++++ > security/integrity/ima/ima_main.c | 37 +++++++++++++++++++++++++++---- > 2 files changed, 39 insertions(+), 4 deletions(-) > > diff --git a/include/linux/ima.h b/include/linux/ima.h > index b6ab66a546ae..fcee2a51bb87 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -65,6 +65,12 @@ static inline const char * const *arch_get_ima_policy(void) > } > #endif > > +static inline struct user_namespace > +*ima_ns_to_user_ns(struct ima_namespace *ns) > +{ > + return current_user_ns(); > +} > + > #else > static inline enum hash_algo ima_get_current_hash_algo(void) > { > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c > index 621685d4eb95..51b0ef1cebbe 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; > @@ -395,6 +395,35 @@ 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) > +{ > + struct user_namespace *user_ns = ima_ns_to_user_ns(ns); > + int ret = 0; > + > + while (user_ns) { > + ns = ima_ns_from_user_ns(user_ns); > + if (ns) { > + int rc; > + > + rc = __process_measurement(ns, file, cred, secid, buf, > + size, mask, func); > + switch (rc) { > + case -EACCES: > + /* return this error at the end but continue */ > + ret = -EACCES; > + break; This seems risky. Every error not -EACCES will be counted as a success. It doesn't look like __process_measurement() will return anything else but I would still place a WARN_ON() or WARN_ON_ONCE() in there to make that assumption explicit. Right now it looks like your only error condition is -EACCES and non-ima cracks like me need to read through __process_measurement() to figure out that that's ok. With a WARN_ON* in there I'd not have needed to bother. switch (rc) { case -EACCES: /* return this error at the end but continue */ ret = -EACCES; break default: WARN_ON_ONCE(true); } or sm similar.
On 1/14/22 06:21, Christian Brauner wrote: > On Tue, Jan 04, 2022 at 12:04:07PM -0500, Stefan Berger wrote: >> From: Stefan Berger <stefanb@linux.ibm.com> >> >> Implement hierarchical processing of file accesses in IMA namespaces by >> walking the list of user namespaces towards the root. This way file >> accesses can be audited in an IMA namespace and also be evaluated against >> the IMA policies of parent IMA namespaces. >> >> __process_measurement() returns either 0 or -EACCES. For hierarchical >> processing remember the -EACCES returned by this function but continue >> to the parent user namespace. At the end either return 0 or -EACCES >> if an error occurred in one of the IMA namespaces. >> >> Currently the ima_ns pointer of the user_namespace is always NULL except >> at the init_user_ns, so test ima_ns for NULL pointer and skip the call to >> __process_measurement() if it is NULL. Once IMA namespacing is fully >> enabled, the pointer may also be NULL due to late initialization of the >> IMA namespace. >> >> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> >> --- >> include/linux/ima.h | 6 +++++ >> security/integrity/ima/ima_main.c | 37 +++++++++++++++++++++++++++---- >> 2 files changed, 39 insertions(+), 4 deletions(-) >> >> diff --git a/include/linux/ima.h b/include/linux/ima.h >> index b6ab66a546ae..fcee2a51bb87 100644 >> --- a/include/linux/ima.h >> +++ b/include/linux/ima.h >> @@ -65,6 +65,12 @@ static inline const char * const *arch_get_ima_policy(void) >> } >> #endif >> >> +static inline struct user_namespace >> +*ima_ns_to_user_ns(struct ima_namespace *ns) >> +{ >> + return current_user_ns(); >> +} >> + >> #else >> static inline enum hash_algo ima_get_current_hash_algo(void) >> { >> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c >> index 621685d4eb95..51b0ef1cebbe 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; >> @@ -395,6 +395,35 @@ 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) >> +{ >> + struct user_namespace *user_ns = ima_ns_to_user_ns(ns); >> + int ret = 0; >> + >> + while (user_ns) { >> + ns = ima_ns_from_user_ns(user_ns); >> + if (ns) { >> + int rc; >> + >> + rc = __process_measurement(ns, file, cred, secid, buf, >> + size, mask, func); >> + switch (rc) { >> + case -EACCES: >> + /* return this error at the end but continue */ >> + ret = -EACCES; >> + break; > This seems risky. Every error not -EACCES will be counted as a success. > It doesn't look like __process_measurement() will return anything else > but I would still place a WARN_ON() or WARN_ON_ONCE() in there to make > that assumption explicit. > > Right now it looks like your only error condition is -EACCES and non-ima > cracks like me need to read through __process_measurement() to figure > out that that's ok. With a WARN_ON* in there I'd not have needed to bother. > > switch (rc) { > case -EACCES: > /* return this error at the end but continue */ > ret = -EACCES; > break > default: > WARN_ON_ONCE(true); ret = -EINVAL; > } > > or sm similar. Agreed. To be on the safe side I would add a ret = -EINVAL to it for the unhandled case as shown above.
diff --git a/include/linux/ima.h b/include/linux/ima.h index b6ab66a546ae..fcee2a51bb87 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -65,6 +65,12 @@ static inline const char * const *arch_get_ima_policy(void) } #endif +static inline struct user_namespace +*ima_ns_to_user_ns(struct ima_namespace *ns) +{ + return current_user_ns(); +} + #else static inline enum hash_algo ima_get_current_hash_algo(void) { diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 621685d4eb95..51b0ef1cebbe 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; @@ -395,6 +395,35 @@ 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) +{ + struct user_namespace *user_ns = ima_ns_to_user_ns(ns); + int ret = 0; + + while (user_ns) { + ns = ima_ns_from_user_ns(user_ns); + if (ns) { + int rc; + + rc = __process_measurement(ns, file, cred, secid, buf, + size, mask, func); + switch (rc) { + case -EACCES: + /* return this error at the end but continue */ + ret = -EACCES; + break; + } + } + + user_ns = user_ns->parent; + }; + + return ret; +} + /** * ima_file_mmap - based on policy, collect/store measurement. * @file: pointer to the file to be measured (May be NULL)