[PATCH 0/4] Code improvements in integrity and IMA

[PATCH 0/4] Code improvements in integrity and IMA

[Thiago Jung Bauermann]

Hello,

These patches come from the "appended signatures support for IMA appraisal"
series. They are code improvements and cleanups and I decided to submit
them separately for a couple of reasons:

1. they stand on their own and could be included in v4.17;

2. this will simplify the original patch series a bit, by having it contain
   only the patches actually necessary to implement the feature.

These are the changes made to them since v5 of the modsig series:

- Patch "integrity: Remove unused macro IMA_ACTION_RULE_FLAGS"
  - New patch.

- Patch "ima: Improvements in ima_appraise_measurement()"
  - Moved is_ima_sig() to its own patch (not in this series).

Mimi Zohar (1):
  ima: Improvements in ima_appraise_measurement()

Thiago Jung Bauermann (3):
  integrity: Remove unused macro IMA_ACTION_RULE_FLAGS
  ima: Simplify ima_eventsig_init()
  integrity: Introduce struct evm_xattr

 security/integrity/evm/evm_crypto.c       |  4 ++--
 security/integrity/evm/evm_main.c         | 10 ++++----
 security/integrity/ima/ima_appraise.c     | 40 ++++++++++++++++++-------------
 security/integrity/ima/ima_template_lib.c | 11 +++------
 security/integrity/integrity.h            |  6 ++++-
 5 files changed, 39 insertions(+), 32 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] integrity: Remove unused macro IMA_ACTION_RULE_FLAGS

[Thiago Jung Bauermann]

This macro isn't used anymore since commit 0d73a55208e9 ("ima: re-introduce
own integrity cache lock"), so remove it.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/integrity.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 8224880935e0..5e58e02ba8dc 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -30,7 +30,6 @@
 
 /* iint cache flags */
 #define IMA_ACTION_FLAGS	0xff000000
-#define IMA_ACTION_RULE_FLAGS	0x06000000
 #define IMA_DIGSIG_REQUIRED	0x01000000
 #define IMA_PERMIT_DIRECTIO	0x02000000
 #define IMA_NEW_FILE		0x04000000

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] integrity: Remove unused macro IMA_ACTION_RULE_FLAGS

[Serge E. Hallyn]

Quoting Thiago Jung Bauermann (bauerman at linux.vnet.ibm.com):
> This macro isn't used anymore since commit 0d73a55208e9 ("ima: re-introduce
> own integrity cache lock"), so remove it.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>

Acked-by: Serge Hallyn <serge@hallyn.com>

> ---
>  security/integrity/integrity.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 8224880935e0..5e58e02ba8dc 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -30,7 +30,6 @@
>  
>  /* iint cache flags */
>  #define IMA_ACTION_FLAGS	0xff000000
> -#define IMA_ACTION_RULE_FLAGS	0x06000000
>  #define IMA_DIGSIG_REQUIRED	0x01000000
>  #define IMA_PERMIT_DIRECTIO	0x02000000
>  #define IMA_NEW_FILE		0x04000000
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] ima: Simplify ima_eventsig_init()

[Thiago Jung Bauermann]

The "goto out" statement doesn't have any purpose since there's no cleanup
to be done when returning early, so remove it. This also makes the rc
variable unnecessary so remove it as well.

Also, the xattr_len and fmt variables are redundant so remove them as well.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_template_lib.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 28af43f63572..5afaa53decc5 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -378,16 +378,11 @@ int ima_eventname_ng_init(struct ima_event_data *event_data,
 int ima_eventsig_init(struct ima_event_data *event_data,
 		      struct ima_field_data *field_data)
 {
-	enum data_formats fmt = DATA_FMT_HEX;
 	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
-	int xattr_len = event_data->xattr_len;
-	int rc = 0;
 
 	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
-		goto out;
+		return 0;
 
-	rc = ima_write_template_field_data(xattr_value, xattr_len, fmt,
-					   field_data);
-out:
-	return rc;
+	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
+					     DATA_FMT_HEX, field_data);
 }

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/4] ima: Simplify ima_eventsig_init()

[Serge E. Hallyn]

Quoting Thiago Jung Bauermann (bauerman at linux.vnet.ibm.com):
> The "goto out" statement doesn't have any purpose since there's no cleanup
> to be done when returning early, so remove it. This also makes the rc
> variable unnecessary so remove it as well.
> 
> Also, the xattr_len and fmt variables are redundant so remove them as well.
> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>

Acked-by: Serge Hallyn <serge@hallyn.com>

> ---
>  security/integrity/ima/ima_template_lib.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 28af43f63572..5afaa53decc5 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -378,16 +378,11 @@ int ima_eventname_ng_init(struct ima_event_data *event_data,
>  int ima_eventsig_init(struct ima_event_data *event_data,
>  		      struct ima_field_data *field_data)
>  {
> -	enum data_formats fmt = DATA_FMT_HEX;
>  	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
> -	int xattr_len = event_data->xattr_len;
> -	int rc = 0;
>  
>  	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
> -		goto out;
> +		return 0;
>  
> -	rc = ima_write_template_field_data(xattr_value, xattr_len, fmt,
> -					   field_data);
> -out:
> -	return rc;
> +	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
> +					     DATA_FMT_HEX, field_data);
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ima: Improvements in ima_appraise_measurement()

[Thiago Jung Bauermann]

From: Mimi Zohar <zohar@linux.vnet.ibm.com>

Replace nested ifs in the EVM xattr verification logic with a switch
statement, making the code easier to understand.

Also, add comments to the if statements in the out section and constify the
cause variable.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_appraise.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 0c5f94b7b9c3..dd10ecbdce45 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -215,7 +215,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 			     int xattr_len, int opened)
 {
 	static const char op[] = "appraise_data";
-	char *cause = "unknown";
+	const char *cause = "unknown";
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_backing_inode(dentry);
 	enum integrity_status status = INTEGRITY_UNKNOWN;
@@ -241,16 +241,20 @@ int ima_appraise_measurement(enum ima_hooks func,
 	}
 
 	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-	if ((status != INTEGRITY_PASS) &&
-	    (status != INTEGRITY_PASS_IMMUTABLE) &&
-	    (status != INTEGRITY_UNKNOWN)) {
-		if ((status == INTEGRITY_NOLABEL)
-		    || (status == INTEGRITY_NOXATTRS))
-			cause = "missing-HMAC";
-		else if (status == INTEGRITY_FAIL)
-			cause = "invalid-HMAC";
+	switch (status) {
+	case INTEGRITY_PASS:
+	case INTEGRITY_PASS_IMMUTABLE:
+	case INTEGRITY_UNKNOWN:
+		break;
+	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
+	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
+		cause = "missing-HMAC";
+		goto out;
+	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
+		cause = "invalid-HMAC";
 		goto out;
 	}
+
 	switch (xattr_value->type) {
 	case IMA_XATTR_DIGEST_NG:
 		/* first byte contains algorithm id */
@@ -316,17 +320,20 @@ int ima_appraise_measurement(enum ima_hooks func,
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
 				    op, cause, rc, 0);
 	} else if (status != INTEGRITY_PASS) {
+		/* Fix mode, but don't replace file signatures. */
 		if ((ima_appraise & IMA_APPRAISE_FIX) &&
 		    (!xattr_value ||
 		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
-		} else if ((inode->i_size == 0) &&
-			   (iint->flags & IMA_NEW_FILE) &&
-			   (xattr_value &&
-			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
+		}
+
+		/* 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;
 		}
+
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
 				    op, cause, rc, 0);
 	} else {

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ima: Improvements in ima_appraise_measurement()

[Serge E. Hallyn]

Quoting Thiago Jung Bauermann (bauerman at linux.vnet.ibm.com):
> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> 
> Replace nested ifs in the EVM xattr verification logic with a switch
> statement, making the code easier to understand.
> 
> Also, add comments to the if statements in the out section and constify the
> cause variable.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
> ---
>  security/integrity/ima/ima_appraise.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 0c5f94b7b9c3..dd10ecbdce45 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -215,7 +215,7 @@ int ima_appraise_measurement(enum ima_hooks func,
>  			     int xattr_len, int opened)
>  {
>  	static const char op[] = "appraise_data";
> -	char *cause = "unknown";
> +	const char *cause = "unknown";
>  	struct dentry *dentry = file_dentry(file);
>  	struct inode *inode = d_backing_inode(dentry);
>  	enum integrity_status status = INTEGRITY_UNKNOWN;
> @@ -241,16 +241,20 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	}
>  
>  	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> -	if ((status != INTEGRITY_PASS) &&
> -	    (status != INTEGRITY_PASS_IMMUTABLE) &&
> -	    (status != INTEGRITY_UNKNOWN)) {
> -		if ((status == INTEGRITY_NOLABEL)
> -		    || (status == INTEGRITY_NOXATTRS))
> -			cause = "missing-HMAC";
> -		else if (status == INTEGRITY_FAIL)
> -			cause = "invalid-HMAC";
> +	switch (status) {
> +	case INTEGRITY_PASS:
> +	case INTEGRITY_PASS_IMMUTABLE:
> +	case INTEGRITY_UNKNOWN:

Wouldn't it be more future-proof to replace this with a 'default', or
to at least add a "default: BUG()" to catch new status values?

> +		break;
> +	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
> +	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
> +		cause = "missing-HMAC";
> +		goto out;
> +	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
> +		cause = "invalid-HMAC";
>  		goto out;
>  	}
> +
>  	switch (xattr_value->type) {
>  	case IMA_XATTR_DIGEST_NG:
>  		/* first byte contains algorithm id */
> @@ -316,17 +320,20 @@ int ima_appraise_measurement(enum ima_hooks func,
>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
>  				    op, cause, rc, 0);
>  	} else if (status != INTEGRITY_PASS) {
> +		/* Fix mode, but don't replace file signatures. */
>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
>  		    (!xattr_value ||
>  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
>  			if (!ima_fix_xattr(dentry, iint))
>  				status = INTEGRITY_PASS;
> -		} else if ((inode->i_size == 0) &&
> -			   (iint->flags & IMA_NEW_FILE) &&
> -			   (xattr_value &&
> -			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
> +		}
> +
> +		/* Permit new files with file signatures, but without data. */
> +		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&

This may be correct, but it's not identical to what you're replacing.
Since in either case you're setting status to INTEGRITY_PASS the final
result is the same, but with a few extra possible steps.  Not sure
whether that matters.

> +		    xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
>  			status = INTEGRITY_PASS;
>  		}
> +
>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
>  				    op, cause, rc, 0);
>  	} else {
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ima: Improvements in ima_appraise_measurement()

[Thiago Jung Bauermann]

Hello Serge,

Thanks for quickly reviewing these patches!

Serge E. Hallyn <serge@hallyn.com> writes:

> Quoting Thiago Jung Bauermann (bauerman at linux.vnet.ibm.com):
>> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
>> @@ -241,16 +241,20 @@ int ima_appraise_measurement(enum ima_hooks func,
>>  	}
>>  
>>  	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
>> -	if ((status != INTEGRITY_PASS) &&
>> -	    (status != INTEGRITY_PASS_IMMUTABLE) &&
>> -	    (status != INTEGRITY_UNKNOWN)) {
>> -		if ((status == INTEGRITY_NOLABEL)
>> -		    || (status == INTEGRITY_NOXATTRS))
>> -			cause = "missing-HMAC";
>> -		else if (status == INTEGRITY_FAIL)
>> -			cause = "invalid-HMAC";
>> +	switch (status) {
>> +	case INTEGRITY_PASS:
>> +	case INTEGRITY_PASS_IMMUTABLE:
>> +	case INTEGRITY_UNKNOWN:
>
> Wouldn't it be more future-proof to replace this with a 'default', or
> to at least add a "default: BUG()" to catch new status values?

I agree. I like the "default: BUG()" option.

>> +		break;
>> +	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
>> +	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
>> +		cause = "missing-HMAC";
>> +		goto out;
>> +	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
>> +		cause = "invalid-HMAC";
>>  		goto out;
>>  	}
>> +
>>  	switch (xattr_value->type) {
>>  	case IMA_XATTR_DIGEST_NG:
>>  		/* first byte contains algorithm id */
>> @@ -316,17 +320,20 @@ int ima_appraise_measurement(enum ima_hooks func,
>>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
>>  				    op, cause, rc, 0);
>>  	} else if (status != INTEGRITY_PASS) {
>> +		/* Fix mode, but don't replace file signatures. */
>>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
>>  		    (!xattr_value ||
>>  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
>>  			if (!ima_fix_xattr(dentry, iint))
>>  				status = INTEGRITY_PASS;
>> -		} else if ((inode->i_size == 0) &&
>> -			   (iint->flags & IMA_NEW_FILE) &&
>> -			   (xattr_value &&
>> -			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
>> +		}
>> +
>> +		/* Permit new files with file signatures, but without data. */
>> +		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
>
> This may be correct, but it's not identical to what you're replacing.
> Since in either case you're setting status to INTEGRITY_PASS the final
> result is the same, but with a few extra possible steps.  Not sure
> whether that matters.

Good point. I'll have to defer this one to Mimi though.

>
>> +		    xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
>>  			status = INTEGRITY_PASS;
>>  		}
>> +
>>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
>>  				    op, cause, rc, 0);
>>  	} else {


-- 
Thiago Jung Bauermann
IBM Linux Technology Center

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ima: Improvements in ima_appraise_measurement()

[Mimi Zohar]

On Wed, 2018-03-14 at 21:03 -0300, Thiago Jung Bauermann wrote:
> Hello Serge,
> 
> Thanks for quickly reviewing these patches!
> 
> Serge E. Hallyn <serge@hallyn.com> writes:
> 
> > Quoting Thiago Jung Bauermann (bauerman at linux.vnet.ibm.com):
> >> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> >> @@ -241,16 +241,20 @@ int ima_appraise_measurement(enum ima_hooks func,
> >>  	}
> >>  
> >>  	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> >> -	if ((status != INTEGRITY_PASS) &&
> >> -	    (status != INTEGRITY_PASS_IMMUTABLE) &&
> >> -	    (status != INTEGRITY_UNKNOWN)) {
> >> -		if ((status == INTEGRITY_NOLABEL)
> >> -		    || (status == INTEGRITY_NOXATTRS))
> >> -			cause = "missing-HMAC";
> >> -		else if (status == INTEGRITY_FAIL)
> >> -			cause = "invalid-HMAC";
> >> +	switch (status) {
> >> +	case INTEGRITY_PASS:
> >> +	case INTEGRITY_PASS_IMMUTABLE:
> >> +	case INTEGRITY_UNKNOWN:
> >
> > Wouldn't it be more future-proof to replace this with a 'default', or
> > to at least add a "default: BUG()" to catch new status values?
> 
> I agree. I like the "default: BUG()" option.

Agreed. ?I would put it at the end after INTEGRITY_FAIL.

> 
> >> +		break;
> >> +	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
> >> +	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
> >> +		cause = "missing-HMAC";
> >> +		goto out;
> >> +	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
> >> +		cause = "invalid-HMAC";
> >>  		goto out;
> >>  	}
> >> +
> >>  	switch (xattr_value->type) {
> >>  	case IMA_XATTR_DIGEST_NG:
> >>  		/* first byte contains algorithm id */
> >> @@ -316,17 +320,20 @@ int ima_appraise_measurement(enum ima_hooks func,
> >>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> >>  				    op, cause, rc, 0);
> >>  	} else if (status != INTEGRITY_PASS) {
> >> +		/* Fix mode, but don't replace file signatures. */
> >>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
> >>  		    (!xattr_value ||
> >>  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> >>  			if (!ima_fix_xattr(dentry, iint))
> >>  				status = INTEGRITY_PASS;
> >> -		} else if ((inode->i_size == 0) &&
> >> -			   (iint->flags & IMA_NEW_FILE) &&
> >> -			   (xattr_value &&
> >> -			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
> >> +		}
> >> +
> >> +		/* Permit new files with file signatures, but without data. */
> >> +		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
> >
> > This may be correct, but it's not identical to what you're replacing.
> > Since in either case you're setting status to INTEGRITY_PASS the final
> > result is the same, but with a few extra possible steps.  Not sure
> > whether that matters.
> 
> Good point. I'll have to defer this one to Mimi though.

The end result is the same, but add some needed comments.

Mimi

> 
> >
> >> +		    xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG) {
> >>  			status = INTEGRITY_PASS;
> >>  		}
> >> +
> >>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> >>  				    op, cause, rc, 0);
> >>  	} else {
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ima: Improvements in ima_appraise_measurement()

[Thiago Jung Bauermann]

Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> On Wed, 2018-03-14 at 21:03 -0300, Thiago Jung Bauermann wrote:
>> Hello Serge,
>> 
>> Thanks for quickly reviewing these patches!
>> 
>> Serge E. Hallyn <serge@hallyn.com> writes:
>> 
>> > Quoting Thiago Jung Bauermann (bauerman at linux.vnet.ibm.com):
>> >> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
>> >> @@ -241,16 +241,20 @@ int ima_appraise_measurement(enum ima_hooks func,
>> >>  	}
>> >>  
>> >>  	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
>> >> -	if ((status != INTEGRITY_PASS) &&
>> >> -	    (status != INTEGRITY_PASS_IMMUTABLE) &&
>> >> -	    (status != INTEGRITY_UNKNOWN)) {
>> >> -		if ((status == INTEGRITY_NOLABEL)
>> >> -		    || (status == INTEGRITY_NOXATTRS))
>> >> -			cause = "missing-HMAC";
>> >> -		else if (status == INTEGRITY_FAIL)
>> >> -			cause = "invalid-HMAC";
>> >> +	switch (status) {
>> >> +	case INTEGRITY_PASS:
>> >> +	case INTEGRITY_PASS_IMMUTABLE:
>> >> +	case INTEGRITY_UNKNOWN:
>> >
>> > Wouldn't it be more future-proof to replace this with a 'default', or
>> > to at least add a "default: BUG()" to catch new status values?
>> 
>> I agree. I like the "default: BUG()" option.
>
> Agreed. I would put it at the end after INTEGRITY_FAIL.

Ok, what about the version below?

>> 
>> >> +		break;
>> >> +	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
>> >> +	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
>> >> +		cause = "missing-HMAC";
>> >> +		goto out;
>> >> +	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
>> >> +		cause = "invalid-HMAC";
>> >>  		goto out;
>> >>  	}
>> >> +
>> >>  	switch (xattr_value->type) {
>> >>  	case IMA_XATTR_DIGEST_NG:
>> >>  		/* first byte contains algorithm id */
>> >> @@ -316,17 +320,20 @@ int ima_appraise_measurement(enum ima_hooks func,
>> >>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
>> >>  				    op, cause, rc, 0);
>> >>  	} else if (status != INTEGRITY_PASS) {
>> >> +		/* Fix mode, but don't replace file signatures. */
>> >>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
>> >>  		    (!xattr_value ||
>> >>  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
>> >>  			if (!ima_fix_xattr(dentry, iint))
>> >>  				status = INTEGRITY_PASS;
>> >> -		} else if ((inode->i_size == 0) &&
>> >> -			   (iint->flags & IMA_NEW_FILE) &&
>> >> -			   (xattr_value &&
>> >> -			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
>> >> +		}
>> >> +
>> >> +		/* Permit new files with file signatures, but without data. */
>> >> +		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
>> >
>> > This may be correct, but it's not identical to what you're replacing.
>> > Since in either case you're setting status to INTEGRITY_PASS the final
>> > result is the same, but with a few extra possible steps.  Not sure
>> > whether that matters.
>> 
>> Good point. I'll have to defer this one to Mimi though.
>
> The end result is the same, but add some needed comments.

The patch is unchanged here, then.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center


>From 343bf4ed2974421e254fb4d5cd79aed79c66f016 Mon Sep 17 00:00:00 2001
From: Mimi Zohar <zohar@linux.vnet.ibm.com>
Date: Mon, 21 Aug 2017 19:28:51 -0300
Subject: [PATCH] ima: Improvements in ima_appraise_measurement()

Replace nested ifs in the EVM xattr verification logic with a switch
statement, making the code easier to understand.

Also, add comments to the if statements in the out section and constify the
cause variable.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_appraise.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 0c5f94b7b9c3..8bd7a0733e51 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -215,7 +215,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 			     int xattr_len, int opened)
 {
 	static const char op[] = "appraise_data";
-	char *cause = "unknown";
+	const char *cause = "unknown";
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_backing_inode(dentry);
 	enum integrity_status status = INTEGRITY_UNKNOWN;
@@ -241,16 +241,22 @@ int ima_appraise_measurement(enum ima_hooks func,
 	}
 
 	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-	if ((status != INTEGRITY_PASS) &&
-	    (status != INTEGRITY_PASS_IMMUTABLE) &&
-	    (status != INTEGRITY_UNKNOWN)) {
-		if ((status == INTEGRITY_NOLABEL)
-		    || (status == INTEGRITY_NOXATTRS))
-			cause = "missing-HMAC";
-		else if (status == INTEGRITY_FAIL)
-			cause = "invalid-HMAC";
+	switch (status) {
+	case INTEGRITY_PASS:
+	case INTEGRITY_PASS_IMMUTABLE:
+	case INTEGRITY_UNKNOWN:
+		break;
+	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
+	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
+		cause = "missing-HMAC";
+		goto out;
+	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
+		cause = "invalid-HMAC";
 		goto out;
+	default:
+		WARN_ONCE(true, "Unexpected integrity status %d\n", status);
 	}
+
 	switch (xattr_value->type) {
 	case IMA_XATTR_DIGEST_NG:
 		/* first byte contains algorithm id */
@@ -316,17 +322,20 @@ int ima_appraise_measurement(enum ima_hooks func,
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
 				    op, cause, rc, 0);
 	} else if (status != INTEGRITY_PASS) {
+		/* Fix mode, but don't replace file signatures. */
 		if ((ima_appraise & IMA_APPRAISE_FIX) &&
 		    (!xattr_value ||
 		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
 			if (!ima_fix_xattr(dentry, iint))
 				status = INTEGRITY_PASS;
-		} else if ((inode->i_size == 0) &&
-			   (iint->flags & IMA_NEW_FILE) &&
-			   (xattr_value &&
-			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
+		}
+
+		/* 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;
 		}
+
 		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
 				    op, cause, rc, 0);
 	} else {

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] ima: Improvements in ima_appraise_measurement()

[Serge E. Hallyn]

Quoting Thiago Jung Bauermann (bauerman at linux.vnet.ibm.com):
> 
> Mimi Zohar <zohar@linux.vnet.ibm.com> writes:
> > On Wed, 2018-03-14 at 21:03 -0300, Thiago Jung Bauermann wrote:
> >> Hello Serge,
> >> 
> >> Thanks for quickly reviewing these patches!
> >> 
> >> Serge E. Hallyn <serge@hallyn.com> writes:
> >> 
> >> > Quoting Thiago Jung Bauermann (bauerman at linux.vnet.ibm.com):
> >> >> From: Mimi Zohar <zohar@linux.vnet.ibm.com>
> >> >> @@ -241,16 +241,20 @@ int ima_appraise_measurement(enum ima_hooks func,
> >> >>  	}
> >> >>  
> >> >>  	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> >> >> -	if ((status != INTEGRITY_PASS) &&
> >> >> -	    (status != INTEGRITY_PASS_IMMUTABLE) &&
> >> >> -	    (status != INTEGRITY_UNKNOWN)) {
> >> >> -		if ((status == INTEGRITY_NOLABEL)
> >> >> -		    || (status == INTEGRITY_NOXATTRS))
> >> >> -			cause = "missing-HMAC";
> >> >> -		else if (status == INTEGRITY_FAIL)
> >> >> -			cause = "invalid-HMAC";
> >> >> +	switch (status) {
> >> >> +	case INTEGRITY_PASS:
> >> >> +	case INTEGRITY_PASS_IMMUTABLE:
> >> >> +	case INTEGRITY_UNKNOWN:
> >> >
> >> > Wouldn't it be more future-proof to replace this with a 'default', or
> >> > to at least add a "default: BUG()" to catch new status values?
> >> 
> >> I agree. I like the "default: BUG()" option.
> >
> > Agreed. I would put it at the end after INTEGRITY_FAIL.
> 
> Ok, what about the version below?

Since the status is returned by evm, it seems like an actual BUG() is
appropriate, but ok.

Acked-by: Serge Hallyn <serge@hallyn.com>

> 
> >> 
> >> >> +		break;
> >> >> +	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
> >> >> +	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
> >> >> +		cause = "missing-HMAC";
> >> >> +		goto out;
> >> >> +	case INTEGRITY_FAIL:		/* Invalid HMAC/signature. */
> >> >> +		cause = "invalid-HMAC";
> >> >>  		goto out;
> >> >>  	}
> >> >> +
> >> >>  	switch (xattr_value->type) {
> >> >>  	case IMA_XATTR_DIGEST_NG:
> >> >>  		/* first byte contains algorithm id */
> >> >> @@ -316,17 +320,20 @@ int ima_appraise_measurement(enum ima_hooks func,
> >> >>  		integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> >> >>  				    op, cause, rc, 0);
> >> >>  	} else if (status != INTEGRITY_PASS) {
> >> >> +		/* Fix mode, but don't replace file signatures. */
> >> >>  		if ((ima_appraise & IMA_APPRAISE_FIX) &&
> >> >>  		    (!xattr_value ||
> >> >>  		     xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> >> >>  			if (!ima_fix_xattr(dentry, iint))
> >> >>  				status = INTEGRITY_PASS;
> >> >> -		} else if ((inode->i_size == 0) &&
> >> >> -			   (iint->flags & IMA_NEW_FILE) &&
> >> >> -			   (xattr_value &&
> >> >> -			    xattr_value->type == EVM_IMA_XATTR_DIGSIG)) {
> >> >> +		}
> >> >> +
> >> >> +		/* Permit new files with file signatures, but without data. */
> >> >> +		if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
> >> >
> >> > This may be correct, but it's not identical to what you're replacing.
> >> > Since in either case you're setting status to INTEGRITY_PASS the final
> >> > result is the same, but with a few extra possible steps.  Not sure
> >> > whether that matters.
> >> 
> >> Good point. I'll have to defer this one to Mimi though.
> >
> > The end result is the same, but add some needed comments.

Yes, the same, but with a few extra possible steps, impacting performance,
so I just wanted to call that out.

-serge
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] integrity: Introduce struct evm_xattr

[Thiago Jung Bauermann]

Even though struct evm_ima_xattr_data includes a fixed-size array to hold a
SHA1 digest, most of the code ignores the array and uses the struct to mean
"type indicator followed by data of unspecified size" and tracks the real
size of what the struct represents in a separate length variable.

The only exception to that is the EVM code, which correctly uses the
definition of struct evm_ima_xattr_data.

This patch makes this explicit in the code by removing the length
specification from the array in struct evm_ima_xattr_data. It also changes
the name of the element from digest to data, since in most places the array
doesn't hold a digest.

A separate struct evm_xattr is introduced, with the original definition of
evm_ima_xattr_data to be used in the places that actually expect that
definition.

Signed-off-by: Thiago Jung Bauermann <bauerman@linux.vnet.ibm.com>
---
 security/integrity/evm/evm_crypto.c   |  4 ++--
 security/integrity/evm/evm_main.c     | 10 +++++-----
 security/integrity/ima/ima_appraise.c |  7 ++++---
 security/integrity/integrity.h        |  5 +++++
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a46fba322340..86511cf171c1 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -302,7 +302,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 			const char *xattr_value, size_t xattr_value_len)
 {
 	struct inode *inode = d_backing_inode(dentry);
-	struct evm_ima_xattr_data xattr_data;
+	struct evm_xattr xattr_data;
 	int rc = 0;
 
 	/*
@@ -318,7 +318,7 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
 	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
 			   xattr_value_len, xattr_data.digest);
 	if (rc == 0) {
-		xattr_data.type = EVM_XATTR_HMAC;
+		xattr_data.data.type = EVM_XATTR_HMAC;
 		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
 					   &xattr_data,
 					   sizeof(xattr_data), 0);
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 7a968faca739..cd17744d4749 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -122,7 +122,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 					     struct integrity_iint_cache *iint)
 {
 	struct evm_ima_xattr_data *xattr_data = NULL;
-	struct evm_ima_xattr_data calc;
+	struct evm_xattr calc;
 	enum integrity_status evm_status = INTEGRITY_PASS;
 	int rc, xattr_len;
 
@@ -154,7 +154,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 	/* check value type */
 	switch (xattr_data->type) {
 	case EVM_XATTR_HMAC:
-		if (xattr_len != sizeof(struct evm_ima_xattr_data)) {
+		if (xattr_len != sizeof(struct evm_xattr)) {
 			evm_status = INTEGRITY_FAIL;
 			goto out;
 		}
@@ -162,7 +162,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 				   xattr_value_len, calc.digest);
 		if (rc)
 			break;
-		rc = crypto_memneq(xattr_data->digest, calc.digest,
+		rc = crypto_memneq(xattr_data->data, calc.digest,
 			    sizeof(calc.digest));
 		if (rc)
 			rc = -EINVAL;
@@ -501,7 +501,7 @@ int evm_inode_init_security(struct inode *inode,
 				 const struct xattr *lsm_xattr,
 				 struct xattr *evm_xattr)
 {
-	struct evm_ima_xattr_data *xattr_data;
+	struct evm_xattr *xattr_data;
 	int rc;
 
 	if (!evm_key_loaded() || !evm_protected_xattr(lsm_xattr->name))
@@ -511,7 +511,7 @@ int evm_inode_init_security(struct inode *inode,
 	if (!xattr_data)
 		return -ENOMEM;
 
-	xattr_data->type = EVM_XATTR_HMAC;
+	xattr_data->data.type = EVM_XATTR_HMAC;
 	rc = evm_init_hmac(inode, lsm_xattr, xattr_data->digest);
 	if (rc < 0)
 		goto out;
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index dd10ecbdce45..96e0f95c294b 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -167,7 +167,8 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		return sig->hash_algo;
 		break;
 	case IMA_XATTR_DIGEST_NG:
-		ret = xattr_value->digest[0];
+		/* first byte contains algorithm id */
+		ret = xattr_value->data[0];
 		if (ret < HASH_ALGO__LAST)
 			return ret;
 		break;
@@ -175,7 +176,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 		/* this is for backward compatibility */
 		if (xattr_len == 21) {
 			unsigned int zero = 0;
-			if (!memcmp(&xattr_value->digest[16], &zero, 4))
+			if (!memcmp(&xattr_value->data[16], &zero, 4))
 				return HASH_ALGO_MD5;
 			else
 				return HASH_ALGO_SHA1;
@@ -272,7 +273,7 @@ int ima_appraise_measurement(enum ima_hooks func,
 			/* xattr length may be longer. md5 hash in previous
 			   version occupied 20 bytes in xattr, instead of 16
 			 */
-			rc = memcmp(&xattr_value->digest[hash_start],
+			rc = memcmp(&xattr_value->data[hash_start],
 				    iint->ima_hash->digest,
 				    iint->ima_hash->length);
 		else
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 5e58e02ba8dc..79799a0d9195 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -78,6 +78,11 @@ enum evm_ima_xattr_type {
 
 struct evm_ima_xattr_data {
 	u8 type;
+	u8 data[];
+} __packed;
+
+struct evm_xattr {
+	struct evm_ima_xattr_data data;
 	u8 digest[SHA1_DIGEST_SIZE];
 } __packed;
 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/4] Code improvements in integrity and IMA

[Mimi Zohar]

Hi Thiago,

On Wed, 2018-03-14 at 17:20 -0300, Thiago Jung Bauermann wrote:
> Hello,
> 
> These patches come from the "appended signatures support for IMA appraisal"
> series. They are code improvements and cleanups and I decided to submit
> them separately for a couple of reasons:
> 
> 1. they stand on their own and could be included in v4.17;
> 
> 2. this will simplify the original patch series a bit, by having it contain
>    only the patches actually necessary to implement the feature.

Agreed. ?The first 3 patches are applied. ?The last one, we'll see
about.

Mimi


> 
> These are the changes made to them since v5 of the modsig series:
> 
> - Patch "integrity: Remove unused macro IMA_ACTION_RULE_FLAGS"
>   - New patch.
> 
> - Patch "ima: Improvements in ima_appraise_measurement()"
>   - Moved is_ima_sig() to its own patch (not in this series).
> 
> Mimi Zohar (1):
>   ima: Improvements in ima_appraise_measurement()
> 
> Thiago Jung Bauermann (3):
>   integrity: Remove unused macro IMA_ACTION_RULE_FLAGS
>   ima: Simplify ima_eventsig_init()
>   integrity: Introduce struct evm_xattr
> 
>  security/integrity/evm/evm_crypto.c       |  4 ++--
>  security/integrity/evm/evm_main.c         | 10 ++++----
>  security/integrity/ima/ima_appraise.c     | 40 ++++++++++++++++++-------------
>  security/integrity/ima/ima_template_lib.c | 11 +++------
>  security/integrity/integrity.h            |  6 ++++-
>  5 files changed, 39 insertions(+), 32 deletions(-)
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html