Re: [PATCH v3 1/7] integrity: Introduce struct evm_hmac_xattr

[Thiago Jung Bauermann]

Hello Mimi,

Thanks for your review!

The patch at the end of the email implements your suggestions, what do
you think?

Mimi Zohar  writes:
> On Thu, 2017-07-06 at 19:17 -0300, Thiago Jung Bauermann wrote:
>> A separate struct evm_hmac_xattr is introduced, with the original
>> definition of evm_ima_xattr_data to be used in the places that actually
>> expect that definition.
>
> The new structure name implies that the xattr can only be an HMAC. By
> moving the new structure to evm.h we also loose the connection that it
> has to theevm_ima_xattr_type enumeration.

Ok, I renamed it to struct evm_xattr.

> Instead, how about defining the new struct in terms of the modified
> evm_ima_xattr_data struct?

IMHO IMA and EVM's practice of mixing and matching structs to compose
its data structures makes the code somewhat confusing and harder to see
where the actual storage used by a given field is actually allocated.

But I don't feel strongly about it, so the patch at the end of this
emails defines it as:

struct evm_xattr {
struct evm_ima_xattr_data data;
u8 digest[SHA1_DIGEST_SIZE];
} __packed;

Another disadvantage of the above is that you have two fields pointing
to the same place: evm_xattr.data.data and evm_xattr.digest.

> Please leave the new structure in integrity.h.

I think that moving the structure in evm.h is helpful. It immediately
conveys that nothing outside of security/integrity/evm/ interprets the
xattr data in the way defined by struct evm_xattr.

But I also don't feel strongly about it, so I moved it to integrity.h.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center


>From df2e29f29c99f5c986d8005d8af8409b3c4d115c Mon Sep 17 00:00:00 2001
From: Thiago Jung Bauermann 
Date: Tue, 16 May 2017 17:14:49 -0300
Subject: [PATCH v4 1/7] integrity: Introduce struct evm_xattr

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 
---
 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 d7f282d75cc1..d902a18ef66f 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -252,13 +252,13 @@ 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;
 
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,
   _data,
   sizeof(xattr_data), 0);
diff --git a/security/integrity/evm/evm_main.c 
b/security/integrity/evm/evm_main.c
index 063d38aef64e..536694499515 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -116,7 +116,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;
 
@@ -147,7 +147,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;

Re: [PATCH v3 1/7] integrity: Introduce struct evm_hmac_xattr

[Mimi Zohar]

Hi Thiago,

On Thu, 2017-07-06 at 19:17 -0300, Thiago Jung Bauermann wrote:
> 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.

Right, the current EVM code converts the EVM signature to an HMAC the
first time the file is accessed.  So most of the code is based on the
HMAC.

> 
> 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_hmac_xattr is introduced, with the original
> definition of evm_ima_xattr_data to be used in the places that actually
> expect that definition.

The new structure name implies that the xattr can only be an HMAC.  By
moving the new structure to evm.h we also loose the connection that it
has to the evm_ima_xattr_type enumeration.

Instead, how about defining the new struct in terms of the modified
evm_ima_xattr_data struct?  Please leave the new structure in
integrity.h.

Mimi

> 
> Signed-off-by: Thiago Jung Bauermann 

> ---
>  security/integrity/evm/evm.h  | 5 +
>  security/integrity/evm/evm_crypto.c   | 2 +-
>  security/integrity/evm/evm_main.c | 8 
>  security/integrity/ima/ima_appraise.c | 7 ---
>  security/integrity/integrity.h| 2 +-
>  5 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h
> index f5f12727771a..e1081cf2f9c5 100644
> --- a/security/integrity/evm/evm.h
> +++ b/security/integrity/evm/evm.h
> @@ -24,6 +24,11 @@
>  #define EVM_INIT_HMAC0x0001
>  #define EVM_INIT_X5090x0002
> 
> +struct evm_hmac_xattr {
> + u8 type;/* Should be EVM_XATTR_HMAC. */
> + u8 digest[SHA1_DIGEST_SIZE];
> +} __packed;
> +
>  extern int evm_initialized;
>  extern char *evm_hmac;
>  extern char *evm_hash;
> diff --git a/security/integrity/evm/evm_crypto.c 
> b/security/integrity/evm/evm_crypto.c
> index d7f282d75cc1..08dde59f3128 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -252,7 +252,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_hmac_xattr xattr_data;
>   int rc = 0;
> 
>   rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> diff --git a/security/integrity/evm/evm_main.c 
> b/security/integrity/evm/evm_main.c
> index 063d38aef64e..b7c1e11a915e 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -116,7 +116,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_hmac_xattr calc;
>   enum integrity_status evm_status = INTEGRITY_PASS;
>   int rc, xattr_len;
> 
> @@ -147,7 +147,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_hmac_xattr)) {
>   evm_status = INTEGRITY_FAIL;
>   goto out;
>   }
> @@ -155,7 +155,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;
> @@ -467,7 +467,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_hmac_xattr *xattr_data;
>   int rc;
> 
>   if (!evm_initialized || !evm_protected_xattr(lsm_xattr->name))
> diff --git a/security/integrity/ima/ima_appraise.c 
> b/security/integrity/ima/ima_appraise.c
> index 809ba70fbbbf..87d2b601cf8e 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++