From: Stefan Berger <[email protected]>
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 <[email protected]>
---
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;
+ }
+ }
+
+ 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)
--
2.31.1
On Tue, Jan 04, 2022 at 12:04:07PM -0500, Stefan Berger wrote:
> From: Stefan Berger <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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.