diff mbox series

[v7,3/5] ima: permit fsverity's file digests in the IMA measurement list

Message ID 20220325223824.310119-4-zohar@linux.ibm.com (mailing list archive)
State Superseded
Headers show
Series ima: support fs-verity digests and signatures | expand

Commit Message

Mimi Zohar March 25, 2022, 10:38 p.m. UTC
Permit fsverity's file digest (a hash of struct fsverity_digest) to be
included in the IMA measurement list, based on the new measurement
policy rule 'digest_type=verity' option.

To differentiate between a regular IMA file hash from an fsverity's
file digest, use the new d-ngv2 format field included in the ima-ngv2
template.

The following policy rule requires fsverity file digests and specifies
the new 'ima-ngv2' template, which contains the new 'd-ngv2' field.  The
policy rule may be constrained, for example based on a fsuuid or LSM
label.

measure func=FILE_CHECK digest_type=verity template=ima-ngv2

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 Documentation/ABI/testing/ima_policy      |  9 +++++
 Documentation/security/IMA-templates.rst  |  8 +++++
 security/integrity/ima/ima_api.c          | 44 +++++++++++++++++++++--
 security/integrity/ima/ima_policy.c       | 38 +++++++++++++++++++-
 security/integrity/ima/ima_template_lib.c |  8 +++--
 security/integrity/integrity.h            |  3 +-
 6 files changed, 104 insertions(+), 6 deletions(-)

Comments

Eric Biggers April 5, 2022, 7:28 p.m. UTC | #1
On Fri, Mar 25, 2022 at 06:38:22PM -0400, Mimi Zohar wrote:
> Permit fsverity's file digest (a hash of struct fsverity_digest) to be
> included in the IMA measurement list, based on the new measurement
> policy rule 'digest_type=verity' option.

"fsverity's file digest" *is* 'struct fsverity_digest', not a hash of it.
Did you mean to write 'struct fsverity_descriptor'?

> diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
> index 1a91d92950a7..2d4789dc7750 100644
> --- a/Documentation/security/IMA-templates.rst
> +++ b/Documentation/security/IMA-templates.rst
> @@ -68,6 +68,9 @@ descriptors by adding their identifier to the format string
>   - 'd-ng': the digest of the event, calculated with an arbitrary hash
>     algorithm (field format: [<hash algo>:]digest, where the digest
>     prefix is shown only if the hash algorithm is not SHA1 or MD5);
> + - 'd-ngv2': same as d-ng, but prefixed with the digest type.
> +    field format: [<digest type>:<hash algo>:]digest,
> +        where the digest type is either "ima" or "verity".

As in patch 2, it is not clear what the square brackets mean here.  Maybe they
mean that "<digest type>:<hash algo>:" is optional, but it is not explained when
they will be present and when they will not be present.

>   - 'd-modsig': the digest of the event without the appended modsig;
>   - 'n-ng': the name of the event, without size limitations;
>   - 'sig': the file signature, or the EVM portable signature if the file
> @@ -106,3 +109,8 @@ currently the following methods are supported:
>     the ``ima_template=`` parameter;
>   - register a new template descriptor with custom format through the kernel
>     command line parameter ``ima_template_fmt=``.
> +
> +
> +References
> +==========
> +[1] Documentation/filesystems/fsverity.rst

Is this meant to be a footnote?  There are no references to it above.

> @@ -242,14 +267,29 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>  	 */
>  	i_version = inode_query_iversion(inode);
>  	hash.hdr.algo = algo;
> +	hash.hdr.length = hash_digest_size[algo];
>  
>  	/* Initialize hash digest to 0's in case of failure */
>  	memset(&hash.digest, 0, sizeof(hash.digest));
>  
> -	if (buf)
> +	if (buf) {
>  		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> -	else
> +	} else if (iint->flags & IMA_VERITY_REQUIRED) {
> +		result = ima_get_verity_digest(iint, &hash);
> +		switch (result) {
> +		case 0:
> +			break;
> +		case -ENODATA:
> +			audit_cause = "no-verity-digest";
> +			result = -EINVAL;
> +			break;
> +		default:
> +			audit_cause = "invalid-verity-digest";
> +			break;
> +		}
> +	} else {
>  		result = ima_calc_file_hash(file, &hash.hdr);
> +	}
>  
>  	if (result && result != -EBADF && result != -EINVAL)
>  		goto out;

The above code only calls ima_get_verity_digest() if 'buf' is non-NULL,
otherwise it calls ima_calc_buffer_hash().  Under what circumstances is 'buf'
non-NULL?  Does this imply that 'digest_type=verity' does not always use verity
digests, and if not, when are they used and when are they not used?

> +/*
> + * Make sure the policy rule and template format are in sync.
> + */
> +static void check_template_field(const struct ima_template_desc *template,
> +				 const char *field, const char *msg)
> +{
> +	int i;
> +
> +	for (i = 0; i < template->num_fields; i++)
> +		if (!strcmp(template->fields[i]->field_id, field))
> +			return;
> +
> +	pr_notice_once("%s", msg);
> +}

A better description for this function would be something like "Warn if the
template does not contain the given field."

> index daf49894fd7d..d42a01903f08 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -32,7 +32,7 @@
>  #define IMA_HASHED		0x00000200
>  
>  /* iint policy rule cache flags */
> -#define IMA_NONACTION_FLAGS	0xff000000
> +#define IMA_NONACTION_FLAGS	0xff800000
>  #define IMA_DIGSIG_REQUIRED	0x01000000
>  #define IMA_PERMIT_DIRECTIO	0x02000000
>  #define IMA_NEW_FILE		0x04000000
> @@ -40,6 +40,7 @@
>  #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
>  #define IMA_MODSIG_ALLOWED	0x20000000
>  #define IMA_CHECK_BLACKLIST	0x40000000
> +#define IMA_VERITY_REQUIRED	0x80000000

It is intentional that the new bit added to IMA_NONACTION_FLAGS is not the same
as IMA_VERITY_REQUIRED?

- Eric
Mimi Zohar April 28, 2022, 2:03 a.m. UTC | #2
On Tue, 2022-04-05 at 19:28 +0000, Eric Biggers wrote:
> On Fri, Mar 25, 2022 at 06:38:22PM -0400, Mimi Zohar wrote:
> > Permit fsverity's file digest (a hash of struct fsverity_digest) to be
> > included in the IMA measurement list, based on the new measurement
> > policy rule 'digest_type=verity' option.
> 
> "fsverity's file digest" *is* 'struct fsverity_digest', not a hash of it.
> Did you mean to write 'struct fsverity_descriptor'?

Fixed.

> 
> > diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
> > index 1a91d92950a7..2d4789dc7750 100644
> > --- a/Documentation/security/IMA-templates.rst
> > +++ b/Documentation/security/IMA-templates.rst
> > @@ -68,6 +68,9 @@ descriptors by adding their identifier to the format string
> >   - 'd-ng': the digest of the event, calculated with an arbitrary hash
> >     algorithm (field format: [<hash algo>:]digest, where the digest
> >     prefix is shown only if the hash algorithm is not SHA1 or MD5);
> > + - 'd-ngv2': same as d-ng, but prefixed with the digest type.
> > +    field format: [<digest type>:<hash algo>:]digest,
> > +        where the digest type is either "ima" or "verity".
> 
> As in patch 2, it is not clear what the square brackets mean here.  Maybe they
> mean that "<digest type>:<hash algo>:" is optional, but it is not explained when
> they will be present and when they will not be present.

Agreed, removed.

> 
> >   - 'd-modsig': the digest of the event without the appended modsig;
> >   - 'n-ng': the name of the event, without size limitations;
> >   - 'sig': the file signature, or the EVM portable signature if the file
> > @@ -106,3 +109,8 @@ currently the following methods are supported:
> >     the ``ima_template=`` parameter;
> >   - register a new template descriptor with custom format through the kernel
> >     command line parameter ``ima_template_fmt=``.
> > +
> > +
> > +References
> > +==========
> > +[1] Documentation/filesystems/fsverity.rst
> 
> Is this meant to be a footnote?  There are no references to it above.
> 
> > @@ -242,14 +267,29 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
> >  	 */
> >  	i_version = inode_query_iversion(inode);
> >  	hash.hdr.algo = algo;
> > +	hash.hdr.length = hash_digest_size[algo];
> >  
> >  	/* Initialize hash digest to 0's in case of failure */
> >  	memset(&hash.digest, 0, sizeof(hash.digest));
> >  
> > -	if (buf)
> > +	if (buf) {
> >  		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
> > -	else
> > +	} else if (iint->flags & IMA_VERITY_REQUIRED) {
> > +		result = ima_get_verity_digest(iint, &hash);
> > +		switch (result) {
> > +		case 0:
> > +			break;
> > +		case -ENODATA:
> > +			audit_cause = "no-verity-digest";
> > +			result = -EINVAL;
> > +			break;
> > +		default:
> > +			audit_cause = "invalid-verity-digest";
> > +			break;
> > +		}
> > +	} else {
> >  		result = ima_calc_file_hash(file, &hash.hdr);
> > +	}
> >  
> >  	if (result && result != -EBADF && result != -EINVAL)
> >  		goto out;
> 
> The above code only calls ima_get_verity_digest() if 'buf' is non-NULL,
> otherwise it calls ima_calc_buffer_hash().  Under what circumstances is 'buf'
> non-NULL?  Does this imply that 'digest_type=verity' does not always use verity
> digests, and if not, when are they used and when are they not used?

Agreed, it should always be based on policy.

FYI, instead of IMA pre-reading and calculating the file hash, there
are instances where the kernel reads the entire file into memory.   For
example, kernel_read_file() calls security_kernel_post_read_file(),
which calls ima_post_read_file().

> 
> > +/*
> > + * Make sure the policy rule and template format are in sync.
> > + */
> > +static void check_template_field(const struct ima_template_desc *template,
> > +				 const char *field, const char *msg)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < template->num_fields; i++)
> > +		if (!strcmp(template->fields[i]->field_id, field))
> > +			return;
> > +
> > +	pr_notice_once("%s", msg);
> > +}
> 
> A better description for this function would be something like "Warn if the
> template does not contain the given field."

Ok
> 
> > index daf49894fd7d..d42a01903f08 100644
> > --- a/security/integrity/integrity.h
> > +++ b/security/integrity/integrity.h
> > @@ -32,7 +32,7 @@
> >  #define IMA_HASHED		0x00000200
> >  
> >  /* iint policy rule cache flags */
> > -#define IMA_NONACTION_FLAGS	0xff000000
> > +#define IMA_NONACTION_FLAGS	0xff800000
> >  #define IMA_DIGSIG_REQUIRED	0x01000000
> >  #define IMA_PERMIT_DIRECTIO	0x02000000
> >  #define IMA_NEW_FILE		0x04000000
> > @@ -40,6 +40,7 @@
> >  #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
> >  #define IMA_MODSIG_ALLOWED	0x20000000
> >  #define IMA_CHECK_BLACKLIST	0x40000000
> > +#define IMA_VERITY_REQUIRED	0x80000000
> 
> It is intentional that the new bit added to IMA_NONACTION_FLAGS is not the same
> as IMA_VERITY_REQUIRED?

Thanks for catching this.  Previous versions required an additional
bit, but that isn't the case now.

thanks,

Mimi
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 839fab811b18..2e0c501ce9c8 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -51,6 +51,9 @@  Description:
 			appraise_flag:= [check_blacklist]
 			Currently, blacklist check is only for files signed with appended
 			signature.
+			digest_type:= verity
+			    Require fs-verity's file digest instead of the
+			    regular IMA file hash.
 			keyrings:= list of keyrings
 			(eg, .builtin_trusted_keys|.ima). Only valid
 			when action is "measure" and func is KEY_CHECK.
@@ -149,3 +152,9 @@  Description:
 		security.ima xattr of a file:
 
 			appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512
+
+		Example of a 'measure' rule requiring fs-verity's digests
+		with indication of type of digest in the measurement list.
+
+			measure func=FILE_CHECK digest_type=verity \
+				template=ima-ngv2
diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index 1a91d92950a7..2d4789dc7750 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -68,6 +68,9 @@  descriptors by adding their identifier to the format string
  - 'd-ng': the digest of the event, calculated with an arbitrary hash
    algorithm (field format: [<hash algo>:]digest, where the digest
    prefix is shown only if the hash algorithm is not SHA1 or MD5);
+ - 'd-ngv2': same as d-ng, but prefixed with the digest type.
+    field format: [<digest type>:<hash algo>:]digest,
+        where the digest type is either "ima" or "verity".
  - 'd-modsig': the digest of the event without the appended modsig;
  - 'n-ng': the name of the event, without size limitations;
  - 'sig': the file signature, or the EVM portable signature if the file
@@ -106,3 +109,8 @@  currently the following methods are supported:
    the ``ima_template=`` parameter;
  - register a new template descriptor with custom format through the kernel
    command line parameter ``ima_template_fmt=``.
+
+
+References
+==========
+[1] Documentation/filesystems/fsverity.rst
diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index c6805af46211..6bcf42168376 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -14,6 +14,7 @@ 
 #include <linux/xattr.h>
 #include <linux/evm.h>
 #include <linux/iversion.h>
+#include <linux/fsverity.h>
 
 #include "ima.h"
 
@@ -200,6 +201,30 @@  int ima_get_action(struct user_namespace *mnt_userns, struct inode *inode,
 				allowed_algos);
 }
 
+static int ima_get_verity_digest(struct integrity_iint_cache *iint,
+				 struct ima_max_digest_data *hash)
+{
+	enum hash_algo verity_alg;
+	int ret;
+
+	ret = fsverity_get_digest(iint->inode, hash->digest, &verity_alg);
+	if (ret) {
+		memset(hash->digest, 0, sizeof(hash->digest));
+		return ret;
+	}
+
+	/*
+	 * Unlike in the case of actually calculating the file hash, in
+	 * the fsverity case regardless of the hash algorithm, return
+	 * the verity digest to be included in the measurement list. A
+	 * mismatch between the verity algorithm and the xattr signature
+	 * algorithm, if one exists, will be detected later.
+	 */
+	hash->hdr.algo = verity_alg;
+	hash->hdr.length = hash_digest_size[verity_alg];
+	return 0;
+}
+
 /*
  * ima_collect_measurement - collect file measurement
  *
@@ -242,14 +267,29 @@  int ima_collect_measurement(struct integrity_iint_cache *iint,
 	 */
 	i_version = inode_query_iversion(inode);
 	hash.hdr.algo = algo;
+	hash.hdr.length = hash_digest_size[algo];
 
 	/* Initialize hash digest to 0's in case of failure */
 	memset(&hash.digest, 0, sizeof(hash.digest));
 
-	if (buf)
+	if (buf) {
 		result = ima_calc_buffer_hash(buf, size, &hash.hdr);
-	else
+	} else if (iint->flags & IMA_VERITY_REQUIRED) {
+		result = ima_get_verity_digest(iint, &hash);
+		switch (result) {
+		case 0:
+			break;
+		case -ENODATA:
+			audit_cause = "no-verity-digest";
+			result = -EINVAL;
+			break;
+		default:
+			audit_cause = "invalid-verity-digest";
+			break;
+		}
+	} else {
 		result = ima_calc_file_hash(file, &hash.hdr);
+	}
 
 	if (result && result != -EBADF && result != -EINVAL)
 		goto out;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index a0f3775cbd82..c6b0454b2e25 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1024,6 +1024,7 @@  enum policy_opt {
 	Opt_fowner_gt, Opt_fgroup_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_gid_lt, Opt_egid_lt,
 	Opt_fowner_lt, Opt_fgroup_lt,
+	Opt_digest_type,
 	Opt_appraise_type, Opt_appraise_flag, Opt_appraise_algos,
 	Opt_permit_directio, Opt_pcr, Opt_template, Opt_keyrings,
 	Opt_label, Opt_err
@@ -1066,6 +1067,7 @@  static const match_table_t policy_tokens = {
 	{Opt_egid_lt, "egid<%s"},
 	{Opt_fowner_lt, "fowner<%s"},
 	{Opt_fgroup_lt, "fgroup<%s"},
+	{Opt_digest_type, "digest_type=%s"},
 	{Opt_appraise_type, "appraise_type=%s"},
 	{Opt_appraise_flag, "appraise_flag=%s"},
 	{Opt_appraise_algos, "appraise_algos=%s"},
@@ -1173,6 +1175,21 @@  static void check_template_modsig(const struct ima_template_desc *template)
 #undef MSG
 }
 
+/*
+ * Make sure the policy rule and template format are in sync.
+ */
+static void check_template_field(const struct ima_template_desc *template,
+				 const char *field, const char *msg)
+{
+	int i;
+
+	for (i = 0; i < template->num_fields; i++)
+		if (!strcmp(template->fields[i]->field_id, field))
+			return;
+
+	pr_notice_once("%s", msg);
+}
+
 static bool ima_validate_rule(struct ima_rule_entry *entry)
 {
 	/* Ensure that the action is set and is compatible with the flags */
@@ -1215,7 +1232,8 @@  static bool ima_validate_rule(struct ima_rule_entry *entry)
 				     IMA_INMASK | IMA_EUID | IMA_PCR |
 				     IMA_FSNAME | IMA_GID | IMA_EGID |
 				     IMA_FGROUP | IMA_DIGSIG_REQUIRED |
-				     IMA_PERMIT_DIRECTIO | IMA_VALIDATE_ALGOS))
+				     IMA_PERMIT_DIRECTIO | IMA_VALIDATE_ALGOS |
+				     IMA_VERITY_REQUIRED))
 			return false;
 
 		break;
@@ -1708,6 +1726,13 @@  static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 						   LSM_SUBJ_TYPE,
 						   AUDIT_SUBJ_TYPE);
 			break;
+		case Opt_digest_type:
+			ima_log_string(ab, "digest_type", args[0].from);
+			if ((strcmp(args[0].from, "verity")) == 0)
+				entry->flags |= IMA_VERITY_REQUIRED;
+			else
+				result = -EINVAL;
+			break;
 		case Opt_appraise_type:
 			ima_log_string(ab, "appraise_type", args[0].from);
 			if ((strcmp(args[0].from, "imasig")) == 0)
@@ -1798,6 +1823,15 @@  static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 		check_template_modsig(template_desc);
 	}
 
+	/* d-ngv2 template field recommended for unsigned fs-verity digests */
+	if (!result && entry->action == MEASURE &&
+	    entry->flags & IMA_VERITY_REQUIRED) {
+		template_desc = entry->template ? entry->template :
+						  ima_template_desc_current();
+		check_template_field(template_desc, "d-ngv2",
+				     "verity rules should include d-ngv2");
+	}
+
 	audit_log_format(ab, "res=%d", !result);
 	audit_log_end(ab);
 	return result;
@@ -2155,6 +2189,8 @@  int ima_policy_show(struct seq_file *m, void *v)
 		else
 			seq_puts(m, "appraise_type=imasig ");
 	}
+	if (entry->flags & IMA_VERITY_REQUIRED)
+		seq_puts(m, "digest_type=verity ");
 	if (entry->flags & IMA_CHECK_BLACKLIST)
 		seq_puts(m, "appraise_flag=check_blacklist ");
 	if (entry->flags & IMA_PERMIT_DIRECTIO)
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index af269b94b369..590b32b4eedd 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -32,12 +32,14 @@  enum data_formats {
 
 enum digest_type {
 	DIGEST_TYPE_IMA,
+	DIGEST_TYPE_VERITY,
 	DIGEST_TYPE__LAST
 };
 
-#define DIGEST_TYPE_NAME_LEN_MAX 4	/* including NULL */
+#define DIGEST_TYPE_NAME_LEN_MAX 7	/* including NULL */
 static const char * const digest_type_name[DIGEST_TYPE__LAST] = {
-	[DIGEST_TYPE_IMA] = "ima"
+	[DIGEST_TYPE_IMA] = "ima",
+	[DIGEST_TYPE_VERITY] = "verity"
 };
 
 static int ima_write_template_field_data(const void *data, const u32 datalen,
@@ -436,6 +438,8 @@  int ima_eventdigest_ngv2_init(struct ima_event_data *event_data,
 	cur_digestsize = event_data->iint->ima_hash->length;
 
 	hash_algo = event_data->iint->ima_hash->algo;
+	if (event_data->iint->flags & IMA_VERITY_REQUIRED)
+		digest_type = DIGEST_TYPE_VERITY;
 out:
 	return ima_eventdigest_init_common(cur_digest, cur_digestsize,
 					   digest_type, hash_algo,
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index daf49894fd7d..d42a01903f08 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -32,7 +32,7 @@ 
 #define IMA_HASHED		0x00000200
 
 /* iint policy rule cache flags */
-#define IMA_NONACTION_FLAGS	0xff000000
+#define IMA_NONACTION_FLAGS	0xff800000
 #define IMA_DIGSIG_REQUIRED	0x01000000
 #define IMA_PERMIT_DIRECTIO	0x02000000
 #define IMA_NEW_FILE		0x04000000
@@ -40,6 +40,7 @@ 
 #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
 #define IMA_MODSIG_ALLOWED	0x20000000
 #define IMA_CHECK_BLACKLIST	0x40000000
+#define IMA_VERITY_REQUIRED	0x80000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_HASH | IMA_APPRAISE_SUBMASK)