Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <c8d3c70e74e607a4b73239bef1e9db0d304200fc.camel@linux.ibm.com>
Subject: Re: [PATCH v2 11/12] ima: Introduce template field evmsig and write
 to field sig as fallback
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Roberto Sassu <roberto.sassu@huawei.com>, mjg59@google.com
Cc:     linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, silviu.vlasceanu@huawei.com
Date:   Thu, 17 Sep 2020 10:25:08 -0400
In-Reply-To: <20200904092643.20013-7-roberto.sassu@huawei.com>
References: <20200904092339.19598-1-roberto.sassu@huawei.com>
         <20200904092643.20013-7-roberto.sassu@huawei.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Roberto,

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> With the patch to accept EVM portable signatures when the
> appraise_type=imasig requirement is specified in the policy, appraisal can
> be successfully done even if the file does not have an IMA signature.
> 
> However, remote attestation would not see that a different signature type
> was used, as only IMA signatures can be included in the measurement list.
> This patch solves the issue by introducing the new template field 'evmsig'
> to show EVM portable signatures and by including its value in the existing
> field 'sig' if the IMA signature is not found.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Suggested-by: Mimi Zohar <zohar@linux.ibm.com>

Thank you!   Just a minor comment below.

<snip>

> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index c022ee9e2a4e..2c596c2a89cc 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> 
> @@ -438,7 +439,7 @@ int ima_eventsig_init(struct ima_event_data *event_data,
>  	struct evm_ima_xattr_data *xattr_value = event_data->xattr_value;
>  
>  	if ((!xattr_value) || (xattr_value->type != EVM_IMA_XATTR_DIGSIG))
> -		return 0;
> +		return ima_eventevmsig_init(event_data, field_data);
>  
>  	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
>  					     DATA_FMT_HEX, field_data);
> @@ -484,3 +485,39 @@ int ima_eventmodsig_init(struct ima_event_data *event_data,
>  	return ima_write_template_field_data(data, data_len, DATA_FMT_HEX,
>  					     field_data);
>  }
> +
> +/*
> + *  ima_eventevmsig_init - include the EVM portable signature as part of the
> + *  template data
> + */
> +int ima_eventevmsig_init(struct ima_event_data *event_data,
> +			 struct ima_field_data *field_data)
> +{
> +	struct evm_ima_xattr_data *xattr_data = NULL;
> +	int rc = 0;
> +
> +	if (!event_data->file)
> +		return 0;
> +
> +	if (!(file_inode(event_data->file)->i_opflags & IOP_XATTR))
> +		return 0;
> +
> +	rc = vfs_getxattr_alloc(file_dentry(event_data->file), XATTR_NAME_EVM,
> +				(char **)&xattr_data, 0, GFP_NOFS);
> +	if (rc <= 0) {
> +		if (!rc || rc == -ENODATA)
> +			return 0;
> +
> +		return rc;

We're including the EVM signature on a best effort basis to help with
attestation.  Do we really care why it failed?   Are we going to act on
it?

Mimi

> +	}
> +
> +	if (xattr_data->type != EVM_XATTR_PORTABLE_DIGSIG) {
> +		kfree(xattr_data);
> +		return 0;
> +	}
> +
> +	rc = ima_write_template_field_data((char *)xattr_data, rc, DATA_FMT_HEX,
> +					   field_data);
> +	kfree(xattr_data);
> +	return rc;
> +}