lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 18 Jan 2022 13:25:38 -0500 From: Stefan Berger <stefanb@...ux.ibm.com> To: Christian Brauner <brauner@...nel.org>, Stefan Berger <stefanb@...ux.vnet.ibm.com> Cc: linux-integrity@...r.kernel.org, zohar@...ux.ibm.com, serge@...lyn.com, christian.brauner@...ntu.com, containers@...ts.linux.dev, dmitry.kasatkin@...il.com, ebiederm@...ssion.com, krzysztof.struczynski@...wei.com, roberto.sassu@...wei.com, mpeters@...hat.com, lhinds@...hat.com, lsturman@...hat.com, puiterwi@...hat.com, jejb@...ux.ibm.com, jamjoom@...ibm.com, linux-kernel@...r.kernel.org, paul@...l-moore.com, rgb@...hat.com, linux-security-module@...r.kernel.org, jmorris@...ei.org Subject: Re: [PATCH v8 10/19] ima: Implement hierarchical processing of file accesses 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@...ux.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@...ux.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.
Powered by blists - more mailing lists