Re: [PATCH v6 11/12] ima: Implement support for module-style appended signatures

[Mimi Zohar]

On Fri, 2018-03-16 at 17:38 -0300, Thiago Jung Bauermann wrote:
> This patch actually implements the appraise_type=imasig|modsig option,
> allowing IMA to read and verify modsig signatures.
> 
> In case both are present in the same file, IMA will first check whether the
> key used by the xattr signature is present in the kernel keyring. If not,
> it will try the appended signature.

Yes, this sounds right.

> 
> Signed-off-by: Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxxxxxxx>
> ---
>  security/integrity/ima/ima.h          | 11 +++++++-
>  security/integrity/ima/ima_appraise.c | 53 +++++++++++++++++++++++++++++++----
>  security/integrity/ima/ima_main.c     | 21 +++++++++++---
>  3 files changed, 74 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 49aef56dc96d..c11ccb7c5bfb 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -157,7 +157,8 @@ void ima_init_template_list(void);
> 
>  static inline bool is_ima_sig(const struct evm_ima_xattr_data *xattr_value)
>  {
> -	return xattr_value && xattr_value->type == EVM_IMA_XATTR_DIGSIG;
> +	return xattr_value && (xattr_value->type == EVM_IMA_XATTR_DIGSIG ||
> +			       xattr_value->type == IMA_MODSIG);
>  }
> 
>  /*
> @@ -253,6 +254,8 @@ enum integrity_status ima_get_cache_status(struct integrity_iint_cache *iint,
>  					   enum ima_hooks func);
>  enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
>  				 int xattr_len);
> +bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data *xattr_value,
> +			     int xattr_len);
>  int ima_read_xattr(struct dentry *dentry,
>  		   struct evm_ima_xattr_data **xattr_value);
> 
> @@ -291,6 +294,12 @@ ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value, int xattr_len)
>  	return ima_hash_algo;
>  }
> 
> +static inline bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data
> +					   *xattr_value, int xattr_len)
> +{
> +	return false;
> +}
> +
>  static inline int ima_read_xattr(struct dentry *dentry,
>  				 struct evm_ima_xattr_data **xattr_value)
>  {
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 01172eab297b..84e0fd5a19c8 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -189,6 +189,22 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
>  	return ima_hash_algo;
>  }
> 
> +bool ima_xattr_sig_known_key(const struct evm_ima_xattr_data *xattr_value,
> +			     int xattr_len)
> +{
> +	struct key *keyring;
> +
> +	if (xattr_value->type != EVM_IMA_XATTR_DIGSIG)
> +		return false;
> +
> +	keyring = integrity_keyring_from_id(INTEGRITY_KEYRING_IMA);
> +	if (IS_ERR(keyring))
> +		return false;
> +
> +	return asymmetric_sig_has_known_key(keyring, (const char *) xattr_value,
> +					    xattr_len);
> +}
> +
>  int ima_read_xattr(struct dentry *dentry,
>  		   struct evm_ima_xattr_data **xattr_value)
>  {
> @@ -221,8 +237,12 @@ int ima_appraise_measurement(enum ima_hooks func,
>  	struct inode *inode = d_backing_inode(dentry);
>  	enum integrity_status status = INTEGRITY_UNKNOWN;
>  	int rc = xattr_len, hash_start = 0;
> +	size_t xattr_contents_len;
> +	void *xattr_contents;
> 
> -	if (!(inode->i_opflags & IOP_XATTR))
> +	/* If not appraising a modsig, we need an xattr. */
> +	if ((xattr_value == NULL || xattr_value->type != IMA_MODSIG) &&
> +	    !(inode->i_opflags & IOP_XATTR))
>  		return INTEGRITY_UNKNOWN;
> 
>  	if (rc <= 0) {
> @@ -241,13 +261,29 @@ int ima_appraise_measurement(enum ima_hooks func,
>  		goto out;
>  	}
> 
> -	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
> +	/*
> +	 * If it's a modsig, we don't have the xattr contents to pass to
> +	 * evm_verifyxattr().
> +	 */
> +	if (xattr_value->type == IMA_MODSIG) {
> +		xattr_contents = NULL;
> +		xattr_contents_len = 0;
> +	} else {
> +		xattr_contents = xattr_value;
> +		xattr_contents_len = xattr_len;
> +	}
> +
> +	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_contents,
> +				 xattr_contents_len, iint);
>  	switch (status) {
>  	case INTEGRITY_PASS:
>  	case INTEGRITY_PASS_IMMUTABLE:
>  	case INTEGRITY_UNKNOWN:
>  		break;
>  	case INTEGRITY_NOXATTRS:	/* No EVM protected xattrs. */
> +		/* It's fine not to have xattrs when using a modsig. */
> +		if (xattr_value->type == IMA_MODSIG)
> +			break;
>  	case INTEGRITY_NOLABEL:		/* No security.evm xattr. */
>  		cause = "missing-HMAC";
>  		goto out;
> @@ -288,11 +324,16 @@ int ima_appraise_measurement(enum ima_hooks func,
>  		status = INTEGRITY_PASS;
>  		break;
>  	case EVM_IMA_XATTR_DIGSIG:
> +	case IMA_MODSIG:
>  		set_bit(IMA_DIGSIG, &iint->atomic_flags);
> -		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> -					     (const char *)xattr_value, rc,
> -					     iint->ima_hash->digest,
> -					     iint->ima_hash->length);
> +		if (xattr_value->type == EVM_IMA_XATTR_DIGSIG)
> +			rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
> +						     (const char *)xattr_value,
> +						     rc, iint->ima_hash->digest,
> +						     iint->ima_hash->length);
> +		else
> +			rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA,
> +					       xattr_value);
>  		if (rc == -EOPNOTSUPP) {
>  			status = INTEGRITY_UNKNOWN;
>  		} else if (rc) {
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 5d122daf5c8a..1b11c10f09df 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -183,7 +183,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  	struct evm_ima_xattr_data *xattr_value = NULL;
>  	int xattr_len = 0;
>  	bool violation_check;
> -	enum hash_algo hash_algo;
> +	enum hash_algo hash_algo = HASH_ALGO__LAST;
> 
>  	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
>  		return 0;
> @@ -277,11 +277,24 @@ static int process_measurement(struct file *file, const struct cred *cred,
> 
>  	template_desc = ima_template_desc_current();
>  	if ((action & IMA_APPRAISE_SUBMASK) ||
> -		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
> +	    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
>  		/* read 'security.ima' */
>  		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
> +		if (iint->flags & IMA_MODSIG_ALLOWED &&
> +		    (xattr_len <= 0 || !ima_xattr_sig_known_key(xattr_value,
> +								xattr_len))) {
> +			/*
> +			 * Even if we end up using a modsig, hash_algo should
> +			 * come from the xattr (or even the default hash algo).
> +			 */
> +			hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
> +			ima_read_modsig(func, buf, size, &xattr_value,
> +					&xattr_len);
> +		}
> +	}
> 
> -	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
> +	if (hash_algo == HASH_ALGO__LAST)
> +		hash_algo = ima_get_hash_algo(xattr_value, xattr_len);

Previous versions needed to calculate the file hash based on the
modsig hash algorithm.ÂÂWith the introduction of the digest signature
template field ('d-sig'), the file digest field ('d-ng') is always
calculated based on either the xattr hash algorithm, if one exists, or
the IMA default hash algorithm.

Mimi

> 
>  	rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
>  	if (rc != 0 && rc != -EBADF && rc != -EINVAL)
> @@ -309,7 +322,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  	     !(iint->flags & IMA_NEW_FILE))
>  		rc = -EACCES;
>  	mutex_unlock(&iint->mutex);
> -	kfree(xattr_value);
> +	ima_free_xattr_data(xattr_value);
>  out:
>  	if (pathbuf)
>  		__putname(pathbuf);
>