[PATCH 1/2] IMA: pass error code in result parameter to integrity_audit_msg()

[PATCH 1/2] IMA: pass error code in result parameter to integrity_audit_msg()

[Lakshmi Ramasubramanian]

The value passed in "result" parameter to integrity_audit_msg() is
not an error code in some instances. Update these instances so that
"result" parameter always contains an error code.

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
---
 security/integrity/ima/ima_appraise.c | 20 ++++++++++++--------
 security/integrity/ima/ima_fs.c       |  8 +++++---
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index a9649b04b9f1..253dcb331249 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -226,7 +226,7 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 		}
 		clear_bit(IMA_DIGSIG, &iint->atomic_flags);
 		if (xattr_len - sizeof(xattr_value->type) - hash_start >=
-				iint->ima_hash->length)
+				iint->ima_hash->length) {
 			/*
 			 * xattr length may be longer. md5 hash in previous
 			 * version occupied 20 bytes in xattr, instead of 16
@@ -234,6 +234,9 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 			rc = memcmp(&xattr_value->data[hash_start],
 				    iint->ima_hash->digest,
 				    iint->ima_hash->length);
+			if (rc)
+				rc = -EINVAL;
+		}
 		else
 			rc = -EINVAL;
 		if (rc) {
@@ -355,7 +358,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_backing_inode(dentry);
 	enum integrity_status status = INTEGRITY_UNKNOWN;
-	int rc = xattr_len;
+	int rc = -EACCES;
 	bool try_modsig = iint->flags & IMA_MODSIG_ALLOWED && modsig;
 
 	/* If not appraising a modsig, we need an xattr. */
@@ -363,10 +366,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 		return INTEGRITY_UNKNOWN;
 
 	/* If reading the xattr failed and there's no modsig, error out. */
-	if (rc <= 0 && !try_modsig) {
-		if (rc && rc != -ENODATA)
-			goto out;
-
+	if (xattr_len <= 0 && !try_modsig) {
 		cause = iint->flags & IMA_DIGSIG_REQUIRED ?
 				"IMA-signature-required" : "missing-hash";
 		status = INTEGRITY_NOLABEL;
@@ -379,7 +379,8 @@ int ima_appraise_measurement(enum ima_hooks func,
 		goto out;
 	}
 
-	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
+	status = evm_verifyxattr(dentry, XATTR_NAME_IMA,
+				 xattr_value, xattr_len, iint);
 	switch (status) {
 	case INTEGRITY_PASS:
 	case INTEGRITY_PASS_IMMUTABLE:
@@ -432,14 +433,17 @@ int ima_appraise_measurement(enum ima_hooks func,
 		if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
 		    (!xattr_value ||
 		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
-			if (!ima_fix_xattr(dentry, iint))
+			if (!ima_fix_xattr(dentry, iint)) {
 				status = INTEGRITY_PASS;
+				rc = 0;
+			}
 		}
 
 		/* Permit new files with file signatures, but without data. */
 		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
 		    xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
 			status = INTEGRITY_PASS;
+			rc = 0;
 		}
 
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e3fcad871861..a3a270cff94f 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -335,10 +335,10 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 		result = ima_read_policy(data);
 	} else if (ima_appraise & IMA_APPRAISE_POLICY) {
 		pr_err("signed policy file (specified as an absolute pathname) required\n");
+		result = -EACCES;
 		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
 				    "policy_update", "signed policy required",
-				    1, 0);
-		result = -EACCES;
+				    result, 0);
 	} else {
 		result = ima_parse_add_rule(data);
 	}
@@ -406,6 +406,7 @@ static int ima_open_policy(struct inode *inode, struct file *filp)
 static int ima_release_policy(struct inode *inode, struct file *file)
 {
 	const char *cause = valid_policy ? "completed" : "failed";
+	int result = 0;
 
 	if ((file->f_flags & O_ACCMODE) == O_RDONLY)
 		return seq_release(inode, file);
@@ -413,11 +414,12 @@ static int ima_release_policy(struct inode *inode, struct file *file)
 	if (valid_policy && ima_check_policy() < 0) {
 		cause = "failed";
 		valid_policy = 0;
+		result = -EINVAL;
 	}
 
 	pr_info("policy update %s\n", cause);
 	integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
-			    "policy_update", cause, !valid_policy, 0);
+			    "policy_update", cause, result, 0);
 
 	if (!valid_policy) {
 		ima_delete_rules();
-- 
2.27.0

[PATCH 2/2] integrity: Add errno field in audit message

[Lakshmi Ramasubramanian]

Error code is not included in the audit messages logged by
the integrity subsystem. Add "errno" field in the audit messages
logged by the integrity subsystem and set the value to the error code
passed to integrity_audit_msg() in the "result" parameter.

Sample audit messages:

[    6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate cause=alloc_entry comm="swapper/0" name="boot_aggregate" res=0 errno=-12

[    8.085456] audit: type=1802 audit(1592005947.297:9): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 op=policy_update cause=completed comm="systemd" res=1 errno=0

Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
Suggested-by: Steve Grubb <sgrubb@redhat.com>
---
 security/integrity/integrity_audit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
index 5109173839cc..a265024f82f3 100644
--- a/security/integrity/integrity_audit.c
+++ b/security/integrity/integrity_audit.c
@@ -53,6 +53,6 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
 		audit_log_untrustedstring(ab, inode->i_sb->s_id);
 		audit_log_format(ab, " ino=%lu", inode->i_ino);
 	}
-	audit_log_format(ab, " res=%d", !result);
+	audit_log_format(ab, " res=%d errno=%d", !result, result);
 	audit_log_end(ab);
 }
-- 
2.27.0

Re: [PATCH 2/2] integrity: Add errno field in audit message

[Paul Moore]

On Wed, Jun 17, 2020 at 4:44 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> Error code is not included in the audit messages logged by
> the integrity subsystem. Add "errno" field in the audit messages
> logged by the integrity subsystem and set the value to the error code
> passed to integrity_audit_msg() in the "result" parameter.
>
> Sample audit messages:
>
> [    6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate cause=alloc_entry comm="swapper/0" name="boot_aggregate" res=0 errno=-12
>
> [    8.085456] audit: type=1802 audit(1592005947.297:9): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 op=policy_update cause=completed comm="systemd" res=1 errno=0
>
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> ---
>  security/integrity/integrity_audit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
> index 5109173839cc..a265024f82f3 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -53,6 +53,6 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
>                 audit_log_untrustedstring(ab, inode->i_sb->s_id);
>                 audit_log_format(ab, " ino=%lu", inode->i_ino);
>         }
> -       audit_log_format(ab, " res=%d", !result);
> +       audit_log_format(ab, " res=%d errno=%d", !result, result);
>         audit_log_end(ab);
>  }
> --
> 2.27.0

-- 
paul moore
www.paul-moore.com

Re: [PATCH 2/2] integrity: Add errno field in audit message

[Steve Grubb]

On Wednesday, June 17, 2020 4:44:36 PM EDT Lakshmi Ramasubramanian wrote:
> Error code is not included in the audit messages logged by
> the integrity subsystem. Add "errno" field in the audit messages
> logged by the integrity subsystem and set the value to the error code
> passed to integrity_audit_msg() in the "result" parameter.
> 
> Sample audit messages:
> 
> [    6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0
> auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate
> cause=alloc_entry comm="swapper/0" name="boot_aggregate" res=0 errno=-12
> 
> [    8.085456] audit: type=1802 audit(1592005947.297:9): pid=1 uid=0
> auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0
> op=policy_update cause=completed comm="systemd" res=1 errno=0
> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>

Acked-by: Steve Grubb <sgrubb@redhat.com>

> ---
>  security/integrity/integrity_audit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/integrity/integrity_audit.c
> b/security/integrity/integrity_audit.c index 5109173839cc..a265024f82f3
> 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -53,6 +53,6 @@ void integrity_audit_msg(int audit_msgno, struct inode
> *inode, audit_log_untrustedstring(ab, inode->i_sb->s_id);
>  		audit_log_format(ab, " ino=%lu", inode->i_ino);
>  	}
> -	audit_log_format(ab, " res=%d", !result);
> +	audit_log_format(ab, " res=%d errno=%d", !result, result);
>  	audit_log_end(ab);
>  }

Re: [PATCH 2/2] integrity: Add errno field in audit message

[Mimi Zohar]

On Wed, 2020-06-17 at 13:44 -0700, Lakshmi Ramasubramanian wrote:
> Error code is not included in the audit messages logged by
> the integrity subsystem. Add "errno" field in the audit messages
> logged by the integrity subsystem and set the value to the error code
> passed to integrity_audit_msg() in the "result" parameter.
> 
> Sample audit messages:
> 
> [    6.284329] audit: type=1804 audit(1591756723.627:2): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=kernel op=add_boot_aggregate cause=alloc_entry comm="swapper/0" name="boot_aggregate" res=0 errno=-12
> 
> [    8.085456] audit: type=1802 audit(1592005947.297:9): pid=1 uid=0 auid=4294967295 ses=4294967295 subj=system_u:system_r:init_t:s0 op=policy_update cause=completed comm="systemd" res=1 errno=0
> 
> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> Suggested-by: Steve Grubb <sgrubb@redhat.com>
> ---
>  security/integrity/integrity_audit.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c
> index 5109173839cc..a265024f82f3 100644
> --- a/security/integrity/integrity_audit.c
> +++ b/security/integrity/integrity_audit.c
> @@ -53,6 +53,6 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode,
>  		audit_log_untrustedstring(ab, inode->i_sb->s_id);
>  		audit_log_format(ab, " ino=%lu", inode->i_ino);
>  	}
> -	audit_log_format(ab, " res=%d", !result);
> +	audit_log_format(ab, " res=%d errno=%d", !result, result);
>  	audit_log_end(ab);
>  }

For the reasons that I mentioned previously, unless others are willing
to add their Reviewed-by tag not for the audit aspect in particular,
but IMA itself, I'm not comfortable making this change all at once.

Previously I suggested making the existing integrity_audit_msg() a
wrapper for a new function with errno.  Steve said, "We normally do
not like to have fields that swing in and out ...", but said setting
errno to 0 is fine.  The original integrity_audit_msg() function would
call the new function with errno set to 0.

Mimi

Re: [PATCH 2/2] integrity: Add errno field in audit message

[Lakshmi Ramasubramanian]

On 6/18/20 10:41 AM, Mimi Zohar wrote:

> 
> For the reasons that I mentioned previously, unless others are willing
> to add their Reviewed-by tag not for the audit aspect in particular,
> but IMA itself, I'm not comfortable making this change all at once.
> 
> Previously I suggested making the existing integrity_audit_msg() a
> wrapper for a new function with errno.  Steve said, "We normally do
> not like to have fields that swing in and out ...", but said setting
> errno to 0 is fine.  The original integrity_audit_msg() function would
> call the new function with errno set to 0.

If the original integrity_audit_msg() always calls the new function with 
errno set to 0, there would be audit messages where "res" field is set 
to "0" (fail) because "result" was non-zero, but errno set to "0" 
(success). Wouldn't this be confusing?

In PATCH 1/2 I've made changes to make the "result" parameter to 
integrity_audit_msg() consistent - i.e., it is always an error code (0 
for success and a negative value for error). Would that address your 
concerns?

thanks,
  -lakshmi

Re: [PATCH 2/2] integrity: Add errno field in audit message

[Mimi Zohar]

On Thu, 2020-06-18 at 11:05 -0700, Lakshmi Ramasubramanian wrote:
> On 6/18/20 10:41 AM, Mimi Zohar wrote:
> 
> > 
> > For the reasons that I mentioned previously, unless others are willing
> > to add their Reviewed-by tag not for the audit aspect in particular,
> > but IMA itself, I'm not comfortable making this change all at once.
> > 
> > Previously I suggested making the existing integrity_audit_msg() a
> > wrapper for a new function with errno.  Steve said, "We normally do
> > not like to have fields that swing in and out ...", but said setting
> > errno to 0 is fine.  The original integrity_audit_msg() function would
> > call the new function with errno set to 0.
> 
> If the original integrity_audit_msg() always calls the new function with 
> errno set to 0, there would be audit messages where "res" field is set 
> to "0" (fail) because "result" was non-zero, but errno set to "0" 
> (success). Wouldn't this be confusing?
> 
> In PATCH 1/2 I've made changes to make the "result" parameter to 
> integrity_audit_msg() consistent - i.e., it is always an error code (0 
> for success and a negative value for error). Would that address your 
> concerns?

You're overloading "res" to imply errno.  Define a new parameter
specifically for errno.

Mimi