Re: [PATCH v6 04/20] evm: add support for different security.evmdata types

[Serge E. Hallyn]

Quoting Mimi Zohar (zohar@xxxxxxxxxxxxxxxxxx):
> From: Dmitry Kasatkin <dmitry.kasatkin@xxxxxxxxx>
> 
> EVM protects a file's security extended attributes(xattrs) against integrity
> attacks. The current patchset maintains an HMAC-sha1 value across the security
> xattrs, storing the value as the extended attribute 'security.evm'. We
> anticipate other methods for protecting the security extended attributes.
> This patch reserves the first byte of 'security.evm' as a place holder for
> the type of method.
> 
> Changelog v6:
> - move evm_ima_xattr_type definition to security/integrity/integrity.h
> - defined a structure for the EVM xattr called evm_ima_xattr_data
>   (based on Serge Hallyn's suggestion)
> 
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@xxxxxxxxx>
> Signed-off-by: Mimi Zohar <zohar@xxxxxxxxxx>
> ---
>  include/linux/integrity.h           |    1 +
>  security/integrity/evm/evm_crypto.c |   11 +++++++----
>  security/integrity/evm/evm_main.c   |   10 +++++-----
>  security/integrity/integrity.h      |   11 +++++++++++
>  4 files changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/integrity.h b/include/linux/integrity.h
> index e715a2a..9684433 100644
> --- a/include/linux/integrity.h
> +++ b/include/linux/integrity.h
> @@ -19,6 +19,7 @@ enum integrity_status {
>  	INTEGRITY_UNKNOWN,
>  };
>  
> +/* List of EVM protected security xattrs */
>  #ifdef CONFIG_INTEGRITY
>  extern int integrity_inode_alloc(struct inode *inode);
>  extern void integrity_inode_free(struct inode *inode);
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index d49bb00..c631b99 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -141,14 +141,17 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>  			const char *xattr_value, size_t xattr_value_len)
>  {
>  	struct inode *inode = dentry->d_inode;
> -	u8 hmac[SHA1_DIGEST_SIZE];
> +	struct evm_ima_xattr_data xattr_data;
>  	int rc = 0;
>  
>  	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> -			   xattr_value_len, hmac);
> -	if (rc == 0)
> +			   xattr_value_len, xattr_data.digest);
> +	if (rc == 0) {
> +		xattr_data.type = EVM_XATTR_HMAC;
>  		rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM,
> -					   hmac, SHA1_DIGEST_SIZE, 0);
> +					   &xattr_data,
> +					   sizeof(xattr_data), 0);
> +	}
>  	else if (rc == -ENODATA)
>  		rc = inode->i_op->removexattr(dentry, XATTR_NAME_EVM);
>  	return rc;
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index a8fa45f..c0580dd1 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -51,20 +51,20 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  					     size_t xattr_value_len,
>  					     struct integrity_iint_cache *iint)
>  {
> -	char hmac_val[SHA1_DIGEST_SIZE];
> +	struct evm_ima_xattr_data xattr_data;
>  	int rc;
>  
>  	if (iint->hmac_status != INTEGRITY_UNKNOWN)
>  		return iint->hmac_status;
>  
> -	memset(hmac_val, 0, sizeof hmac_val);

Why did you drop the memset here?

(You didn't in the previous version of this patch)

Otherwise, looks good.

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

>  	rc = evm_calc_hmac(dentry, xattr_name, xattr_value,
> -			   xattr_value_len, hmac_val);
> +			   xattr_value_len, xattr_data.digest);
>  	if (rc < 0)
>  		return INTEGRITY_UNKNOWN;
>  
> -	rc = vfs_xattr_cmp(dentry, XATTR_NAME_EVM, hmac_val, sizeof hmac_val,
> -			   GFP_NOFS);
> +	xattr_data.type = EVM_XATTR_HMAC;
> +	rc = vfs_xattr_cmp(dentry, XATTR_NAME_EVM, (u8 *)&xattr_data,
> +			   sizeof xattr_data, GFP_NOFS);
>  	if (rc < 0)
>  		goto err_out;
>  	iint->hmac_status = INTEGRITY_PASS;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index 397a46b..7efbf56 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -18,6 +18,17 @@
>  /* iint cache flags */
>  #define IMA_MEASURED		0x01
>  
> +enum evm_ima_xattr_type {
> +	IMA_XATTR_DIGEST = 0x01,
> +	EVM_XATTR_HMAC,
> +	EVM_IMA_XATTR_DIGSIG,
> +};
> +
> +struct evm_ima_xattr_data {
> +	u8 type;
> +	u8 digest[SHA1_DIGEST_SIZE];
> +}  __attribute__((packed));
> +
>  /* integrity data associated with an inode */
>  struct integrity_iint_cache {
>  	struct rb_node rb_node; /* rooted in integrity_iint_tree */
> -- 
> 1.7.3.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/