Re: [PATCH v8 10/19] ima: Implement hierarchical processing of file accesses

From: Stefan Berger
Date: Tue Jan 18 2022 - 13:26:06 EST



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@xxxxxxxxxxxxx>

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@xxxxxxxxxxxxx>
---
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.