Linux-audit Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/2] IMA: pass error code in result parameter to integrity_audit_msg()
@ 2020-06-17 20:44 Lakshmi Ramasubramanian
  2020-06-17 20:44 ` [PATCH 2/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian
  0 siblings, 1 reply; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-17 20:44 UTC (permalink / raw)
  To: zohar, bauerman, nayna, sgrubb, paul
  Cc: rgb, linux-integrity, linux-audit, linux-kernel

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


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] integrity: Add errno field in audit message
  2020-06-17 20:44 [PATCH 1/2] IMA: pass error code in result parameter to integrity_audit_msg() Lakshmi Ramasubramanian
@ 2020-06-17 20:44 ` Lakshmi Ramasubramanian
  2020-06-17 21:15   ` Paul Moore
                     ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-17 20:44 UTC (permalink / raw)
  To: zohar, bauerman, nayna, sgrubb, paul
  Cc: rgb, linux-integrity, linux-audit, linux-kernel

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


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] integrity: Add errno field in audit message
  2020-06-17 20:44 ` [PATCH 2/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian
@ 2020-06-17 21:15   ` Paul Moore
  2020-06-17 22:36   ` Steve Grubb
  2020-06-18 17:41   ` Mimi Zohar
  2 siblings, 0 replies; 7+ messages in thread
From: Paul Moore @ 2020-06-17 21:15 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: rgb, nayna, linux-kernel, zohar, linux-audit, linux-integrity, bauerman

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

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] integrity: Add errno field in audit message
  2020-06-17 20:44 ` [PATCH 2/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian
  2020-06-17 21:15   ` Paul Moore
@ 2020-06-17 22:36   ` Steve Grubb
  2020-06-18 17:41   ` Mimi Zohar
  2 siblings, 0 replies; 7+ messages in thread
From: Steve Grubb @ 2020-06-17 22:36 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: rgb, nayna, linux-kernel, zohar, linux-audit, linux-integrity, bauerman

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);
>  }




--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] integrity: Add errno field in audit message
  2020-06-17 20:44 ` [PATCH 2/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian
  2020-06-17 21:15   ` Paul Moore
  2020-06-17 22:36   ` Steve Grubb
@ 2020-06-18 17:41   ` Mimi Zohar
  2020-06-18 18:05     ` Lakshmi Ramasubramanian
  2 siblings, 1 reply; 7+ messages in thread
From: Mimi Zohar @ 2020-06-18 17:41 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, bauerman, nayna, sgrubb, paul
  Cc: rgb, linux-integrity, linux-audit, linux-kernel

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


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] integrity: Add errno field in audit message
  2020-06-18 17:41   ` Mimi Zohar
@ 2020-06-18 18:05     ` Lakshmi Ramasubramanian
  2020-06-18 18:10       ` Mimi Zohar
  0 siblings, 1 reply; 7+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-06-18 18:05 UTC (permalink / raw)
  To: Mimi Zohar, bauerman, nayna, sgrubb, paul
  Cc: rgb, linux-integrity, linux-audit, linux-kernel

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





--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] integrity: Add errno field in audit message
  2020-06-18 18:05     ` Lakshmi Ramasubramanian
@ 2020-06-18 18:10       ` Mimi Zohar
  0 siblings, 0 replies; 7+ messages in thread
From: Mimi Zohar @ 2020-06-18 18:10 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian, bauerman, nayna, sgrubb, paul
  Cc: rgb, linux-integrity, linux-audit, linux-kernel

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


--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-17 20:44 [PATCH 1/2] IMA: pass error code in result parameter to integrity_audit_msg() Lakshmi Ramasubramanian
2020-06-17 20:44 ` [PATCH 2/2] integrity: Add errno field in audit message Lakshmi Ramasubramanian
2020-06-17 21:15   ` Paul Moore
2020-06-17 22:36   ` Steve Grubb
2020-06-18 17:41   ` Mimi Zohar
2020-06-18 18:05     ` Lakshmi Ramasubramanian
2020-06-18 18:10       ` Mimi Zohar

Linux-audit Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-audit/0 linux-audit/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-audit linux-audit/ https://lore.kernel.org/linux-audit \
		linux-audit@redhat.com
	public-inbox-index linux-audit

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.linux-audit


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git