[PATCH v5 0/8] ima: support fs-verity digests and signatures

[PATCH v5 0/8] ima: support fs-verity digests and signatures

[Mimi Zohar]

Support for including fs-verity file digests and signatures in the IMA
measurement list as well as verifying the fs-verity file digest based
signatures, both based on IMA policy rules, was discussed prior to
fs-verity being upstreamed[1,2].

Support for including fs-verity file digests in the 'd-ng' template
field is based on a new policy rule option named 'digest_type=verity'.
A new template field named 'd-type' as well as a new template named
'ima-ngv2' are defined to differentiate between the regular IMA file
hashes from the fs-verity file digests (tree-hash based file hashes)
stored in the 'd-ng' template field of the measurement list.

A new signature version (v3) is defined as a hash of the 'ima_file_id'
struct, to disambiguate the signatures stored as 'security.ima' xattr.
The policy rule 'appraise_type=' option is extended to support 'sigv3',
which is initially limited to fs-verity.

The fs-verity 'appraise' rules are identified by the 'digest-type=verity'
option and require the 'appraise_type=sigv3' option.

Lastly, for IMA to differentiate between the original IMA signature
from an fs-verity signature a new 'xattr_type' named IMA_VERITY_DIGSIG
is defined.


[1] https://events19.linuxfoundation.org/wp-content/uploads/2017/11/fs-verify_Mike-Halcrow_Eric-Biggers.pdf
[2] Documentation/filesystems/fsverity.rst

Changelog v5:
- Define ima_max_digest_size struct, removing the locally defined versions.
- Don't overload the 'digest_type=verity' to imply a verity signature,
  but extend the 'appraise_type' policy rule option to define 'sigv3'.

Changelog v4:
- Based on Eric Bigger's signature verification concerns of replacing the
  contents of a file with the ima_file_id struct hash, require per policy
  rule signature versions.
- Addressed Eric Bigger's other comments.
- Added new audit messages "causes".
- Updated patch descriptions.

Changelog v3:
- Addressed Eric Bigger's comments: included Ack, incremented the
  signature format version, the crypto issues are generic and will be
  addressed by him separately.
- Addressed Vitaly Chikunov's comments: hard coded maximum digest size
  rather than using a flexible array, removed unnecessary assignment, and
  fixed comment to match variable name.
- Defined new "ima_max_digest_size" struct to avoid wrapping the
  "ima_digest_data" struct inside a function local structure or
  having to dynamically allocate it with enough memory for the specific
  hash algo size.

Changelog v2:
- Addressed Eric Bigger's comments: sign the hash of fsverity's digest
  and the digest's metadata, use match_string, use preferred function
  name fsverity_get_digest(), support including unsigned fs-verity's
  digests in the IMA measurement list.
- Remove signatures requirement for including fs-verity's file digests in
  the 'd-ng' field of the measurement list.

Changelog v1:
- Updated both fsverity and IMA documentation.
- Addressed both Eric Bigger's and Lakshmi's comments.

Mimi Zohar (8):
  ima: rename IMA_ACTION_FLAGS to IMA_NONACTION_FLAGS
  ima: define ima_max_digest_data struct without a flexible array
    variable
  fs-verity: define a function to return the integrity protected file
    digest
  ima: define a new template field 'd-type' and a new template
    'ima-ngv2'
  ima: permit fsverity's file digests in the IMA measurement list
  ima: define signature version 3
  ima: support fs-verity file digest based version 3 signatures
  fsverity: update the documentation

 Documentation/ABI/testing/ima_policy      |  30 +++++-
 Documentation/filesystems/fsverity.rst    |  22 +++--
 Documentation/security/IMA-templates.rst  |  11 ++-
 fs/verity/Kconfig                         |   1 +
 fs/verity/fsverity_private.h              |   7 --
 fs/verity/measure.c                       |  41 ++++++++
 include/linux/fsverity.h                  |  18 ++++
 security/integrity/digsig.c               |   3 +-
 security/integrity/ima/ima_api.c          |  49 ++++++++--
 security/integrity/ima/ima_appraise.c     | 112 +++++++++++++++++++++-
 security/integrity/ima/ima_init.c         |   5 +-
 security/integrity/ima/ima_main.c         |   7 +-
 security/integrity/ima/ima_policy.c       |  66 +++++++++++--
 security/integrity/ima/ima_template.c     |   3 +
 security/integrity/ima/ima_template_lib.c |  28 +++++-
 security/integrity/ima/ima_template_lib.h |   2 +
 security/integrity/integrity.h            |  39 +++++++-
 17 files changed, 388 insertions(+), 56 deletions(-)

-- 
2.27.0

[PATCH v5 1/8] ima: rename IMA_ACTION_FLAGS to IMA_NONACTION_FLAGS

[Mimi Zohar]

Simple policy rule options, such as fowner, uid, or euid, can be checked
immediately, while other policy rule options, such as requiring a file
signature, need to be deferred.

The 'flags' field in the integrity_iint_cache struct contains the policy
action', 'subaction', and non action/subaction.

action: measure/measured, appraise/appraised, (collect)/collected,
        audit/audited
subaction: appraise status for each hook (e.g. file, mmap, bprm, read,
        creds)
non action/subaction: deferred policy rule options and state

Rename the IMA_ACTION_FLAGS to IMA_NONACTION_FLAGS.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_main.c   | 2 +-
 security/integrity/ima/ima_policy.c | 2 +-
 security/integrity/integrity.h      | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8ed6da428328..7c80dfe2c7a5 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -263,7 +263,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
 		/* reset appraisal flags if ima_inode_post_setattr was called */
 		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
 				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
-				 IMA_ACTION_FLAGS);
+				 IMA_NONACTION_FLAGS);
 
 	/*
 	 * Re-evaulate the file if either the xattr has changed or the
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 90f528558adc..a0f3775cbd82 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -712,7 +712,7 @@ int ima_match_policy(struct user_namespace *mnt_userns, struct inode *inode,
 				     func, mask, func_data))
 			continue;
 
-		action |= entry->flags & IMA_ACTION_FLAGS;
+		action |= entry->flags & IMA_NONACTION_FLAGS;
 
 		action |= entry->action & IMA_DO_MASK;
 		if (entry->action & IMA_APPRAISE) {
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 547425c20e11..d045dccd415a 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -30,8 +30,8 @@
 #define IMA_HASH		0x00000100
 #define IMA_HASHED		0x00000200
 
-/* iint cache flags */
-#define IMA_ACTION_FLAGS	0xff000000
+/* iint policy rule cache flags */
+#define IMA_NONACTION_FLAGS	0xff000000
 #define IMA_DIGSIG_REQUIRED	0x01000000
 #define IMA_PERMIT_DIRECTIO	0x02000000
 #define IMA_NEW_FILE		0x04000000
-- 
2.27.0

Re: [PATCH v5 1/8] ima: rename IMA_ACTION_FLAGS to IMA_NONACTION_FLAGS

[Stefan Berger]

On 2/11/22 16:43, Mimi Zohar wrote:
> Simple policy rule options, such as fowner, uid, or euid, can be checked
> immediately, while other policy rule options, such as requiring a file
> signature, need to be deferred.
>
> The 'flags' field in the integrity_iint_cache struct contains the policy
> action', 'subaction', and non action/subaction.
>
> action: measure/measured, appraise/appraised, (collect)/collected,
>          audit/audited
> subaction: appraise status for each hook (e.g. file, mmap, bprm, read,
>          creds)
> non action/subaction: deferred policy rule options and state
>
> Rename the IMA_ACTION_FLAGS to IMA_NONACTION_FLAGS.
>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Re: [PATCH v5 1/8] ima: rename IMA_ACTION_FLAGS to IMA_NONACTION_FLAGS

[Mimi Zohar]

On Mon, 2022-02-14 at 15:03 -0500, Stefan Berger wrote:
> On 2/11/22 16:43, Mimi Zohar wrote:
> > Simple policy rule options, such as fowner, uid, or euid, can be checked
> > immediately, while other policy rule options, such as requiring a file
> > signature, need to be deferred.
> >
> > The 'flags' field in the integrity_iint_cache struct contains the policy
> > action', 'subaction', and non action/subaction.
> >
> > action: measure/measured, appraise/appraised, (collect)/collected,
> >          audit/audited
> > subaction: appraise status for each hook (e.g. file, mmap, bprm, read,
> >          creds)
> > non action/subaction: deferred policy rule options and state
> >
> > Rename the IMA_ACTION_FLAGS to IMA_NONACTION_FLAGS.
> >
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

Thanks, Stefan.  Both 1/8 & 2/8 cleanup are now queued in next-
integrity.

-- 
thanks,

Mimi

[PATCH v5 2/8] ima: define ima_max_digest_data struct without a flexible array variable

[Mimi Zohar]

To support larger hash digests in the 'iint' cache, instead of defining
the 'digest' field as the maximum digest size, the 'digest' field was
defined as a flexible array variable.  The "ima_digest_data" struct was
wrapped inside a local structure with the maximum digest size.  But
before adding the record to the iint cache, memory for the exact digest
size was dynamically allocated.

The original reason for defining the 'digest' field as a flexible array
variable is still valid for the 'iint' cache use case.  Instead of
wrapping the 'ima_digest_data' struct in a local structure define
'ima_max_digest_data' struct.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_api.c          | 10 ++++------
 security/integrity/ima/ima_init.c         |  5 +----
 security/integrity/ima/ima_main.c         |  5 +----
 security/integrity/ima/ima_template_lib.c |  5 +----
 security/integrity/integrity.h            | 10 ++++++++++
 5 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
index 5b220a2fe573..c6805af46211 100644
--- a/security/integrity/ima/ima_api.c
+++ b/security/integrity/ima/ima_api.c
@@ -217,14 +217,11 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 	const char *audit_cause = "failed";
 	struct inode *inode = file_inode(file);
 	const char *filename = file->f_path.dentry->d_name.name;
+	struct ima_max_digest_data hash;
 	int result = 0;
 	int length;
 	void *tmpbuf;
 	u64 i_version;
-	struct {
-		struct ima_digest_data hdr;
-		char digest[IMA_MAX_DIGEST_SIZE];
-	} hash;
 
 	/*
 	 * Always collect the modsig, because IMA might have already collected
@@ -239,8 +236,9 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
 
 	/*
 	 * Detecting file change is based on i_version. On filesystems
-	 * which do not support i_version, support is limited to an initial
-	 * measurement/appraisal/audit.
+	 * which do not support i_version, support was originally limited
+	 * to an initial measurement/appraisal/audit, but was modified to
+	 * assume the file changed.
 	 */
 	i_version = inode_query_iversion(inode);
 	hash.hdr.algo = algo;
diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
index b26fa67476b4..63979aefc95f 100644
--- a/security/integrity/ima/ima_init.c
+++ b/security/integrity/ima/ima_init.c
@@ -47,12 +47,9 @@ static int __init ima_add_boot_aggregate(void)
 	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
 	struct ima_event_data event_data = { .iint = iint,
 					     .filename = boot_aggregate_name };
+	struct ima_max_digest_data hash;
 	int result = -ENOMEM;
 	int violation = 0;
-	struct {
-		struct ima_digest_data hdr;
-		char digest[TPM_MAX_DIGEST_SIZE];
-	} hash;
 
 	memset(iint, 0, sizeof(*iint));
 	memset(&hash, 0, sizeof(hash));
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 7c80dfe2c7a5..c6412dec3810 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -874,10 +874,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
 					    .buf = buf,
 					    .buf_len = size};
 	struct ima_template_desc *template;
-	struct {
-		struct ima_digest_data hdr;
-		char digest[IMA_MAX_DIGEST_SIZE];
-	} hash = {};
+	struct ima_max_digest_data hash;
 	char digest_hash[IMA_MAX_DIGEST_SIZE];
 	int digest_hash_len = hash_digest_size[ima_hash_algo];
 	int violation = 0;
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 5a5d462ab36d..7155d17a3b75 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -307,10 +307,7 @@ static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
 int ima_eventdigest_init(struct ima_event_data *event_data,
 			 struct ima_field_data *field_data)
 {
-	struct {
-		struct ima_digest_data hdr;
-		char digest[IMA_MAX_DIGEST_SIZE];
-	} hash;
+	struct ima_max_digest_data hash;
 	u8 *cur_digest = NULL;
 	u32 cur_digestsize = 0;
 	struct inode *inode;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index d045dccd415a..daf49894fd7d 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -15,6 +15,7 @@
 #include <linux/types.h>
 #include <linux/integrity.h>
 #include <crypto/sha1.h>
+#include <crypto/hash.h>
 #include <linux/key.h>
 #include <linux/audit.h>
 
@@ -110,6 +111,15 @@ struct ima_digest_data {
 	u8 digest[];
 } __packed;
 
+/*
+ * Instead of wrapping the ima_digest_data struct inside a local structure
+ * with the maximum hash size, define ima_max_digest_data struct.
+ */
+struct ima_max_digest_data {
+	struct ima_digest_data hdr;
+	u8 digest[HASH_MAX_DIGESTSIZE];
+} __packed;
+
 /*
  * signature format v2 - for using with asymmetric keys
  */
-- 
2.27.0

Re: [PATCH v5 2/8] ima: define ima_max_digest_data struct without a flexible array variable

[Stefan Berger]

On 2/11/22 16:43, Mimi Zohar wrote:
> To support larger hash digests in the 'iint' cache, instead of defining
> the 'digest' field as the maximum digest size, the 'digest' field was
> defined as a flexible array variable.  The "ima_digest_data" struct was
> wrapped inside a local structure with the maximum digest size.  But
> before adding the record to the iint cache, memory for the exact digest
> size was dynamically allocated.
>
> The original reason for defining the 'digest' field as a flexible array
> variable is still valid for the 'iint' cache use case.  Instead of
> wrapping the 'ima_digest_data' struct in a local structure define
> 'ima_max_digest_data' struct.
>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   security/integrity/ima/ima_api.c          | 10 ++++------
>   security/integrity/ima/ima_init.c         |  5 +----
>   security/integrity/ima/ima_main.c         |  5 +----
>   security/integrity/ima/ima_template_lib.c |  5 +----
>   security/integrity/integrity.h            | 10 ++++++++++
>   5 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/security/integrity/ima/ima_api.c b/security/integrity/ima/ima_api.c
> index 5b220a2fe573..c6805af46211 100644
> --- a/security/integrity/ima/ima_api.c
> +++ b/security/integrity/ima/ima_api.c
> @@ -217,14 +217,11 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   	const char *audit_cause = "failed";
>   	struct inode *inode = file_inode(file);
>   	const char *filename = file->f_path.dentry->d_name.name;
> +	struct ima_max_digest_data hash;
>   	int result = 0;
>   	int length;
>   	void *tmpbuf;
>   	u64 i_version;
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[IMA_MAX_DIGEST_SIZE];
> -	} hash;
>   
>   	/*
>   	 * Always collect the modsig, because IMA might have already collected
> @@ -239,8 +236,9 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,
>   
>   	/*
>   	 * Detecting file change is based on i_version. On filesystems
> -	 * which do not support i_version, support is limited to an initial
> -	 * measurement/appraisal/audit.
> +	 * which do not support i_version, support was originally limited
> +	 * to an initial measurement/appraisal/audit, but was modified to
> +	 * assume the file changed.
>   	 */
>   	i_version = inode_query_iversion(inode);
>   	hash.hdr.algo = algo;
> diff --git a/security/integrity/ima/ima_init.c b/security/integrity/ima/ima_init.c
> index b26fa67476b4..63979aefc95f 100644
> --- a/security/integrity/ima/ima_init.c
> +++ b/security/integrity/ima/ima_init.c
> @@ -47,12 +47,9 @@ static int __init ima_add_boot_aggregate(void)
>   	struct integrity_iint_cache tmp_iint, *iint = &tmp_iint;
>   	struct ima_event_data event_data = { .iint = iint,
>   					     .filename = boot_aggregate_name };
> +	struct ima_max_digest_data hash;
>   	int result = -ENOMEM;
>   	int violation = 0;
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[TPM_MAX_DIGEST_SIZE];
> -	} hash;
>   
>   	memset(iint, 0, sizeof(*iint));
>   	memset(&hash, 0, sizeof(hash));
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 7c80dfe2c7a5..c6412dec3810 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -874,10 +874,7 @@ int process_buffer_measurement(struct user_namespace *mnt_userns,
>   					    .buf = buf,
>   					    .buf_len = size};
>   	struct ima_template_desc *template;
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[IMA_MAX_DIGEST_SIZE];
> -	} hash = {};
> +	struct ima_max_digest_data hash;


It looks like the initialization wasn't necessary.

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>


>   	char digest_hash[IMA_MAX_DIGEST_SIZE];
>   	int digest_hash_len = hash_digest_size[ima_hash_algo];
>   	int violation = 0;
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 5a5d462ab36d..7155d17a3b75 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -307,10 +307,7 @@ static int ima_eventdigest_init_common(const u8 *digest, u32 digestsize,
>   int ima_eventdigest_init(struct ima_event_data *event_data,
>   			 struct ima_field_data *field_data)
>   {
> -	struct {
> -		struct ima_digest_data hdr;
> -		char digest[IMA_MAX_DIGEST_SIZE];
> -	} hash;
> +	struct ima_max_digest_data hash;
>   	u8 *cur_digest = NULL;
>   	u32 cur_digestsize = 0;
>   	struct inode *inode;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d045dccd415a..daf49894fd7d 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -15,6 +15,7 @@
>   #include <linux/types.h>
>   #include <linux/integrity.h>
>   #include <crypto/sha1.h>
> +#include <crypto/hash.h>
>   #include <linux/key.h>
>   #include <linux/audit.h>
>   
> @@ -110,6 +111,15 @@ struct ima_digest_data {
>   	u8 digest[];
>   } __packed;
>   
> +/*
> + * Instead of wrapping the ima_digest_data struct inside a local structure
> + * with the maximum hash size, define ima_max_digest_data struct.
> + */
> +struct ima_max_digest_data {
> +	struct ima_digest_data hdr;
> +	u8 digest[HASH_MAX_DIGESTSIZE];
> +} __packed;
> +
>   /*
>    * signature format v2 - for using with asymmetric keys
>    */

[PATCH v5 3/8] fs-verity: define a function to return the integrity protected file digest

[Mimi Zohar]

Define a function named fsverity_get_digest() to return the verity file
digest and the associated hash algorithm (enum hash_algo).

Acked-by: Eric Biggers <ebiggers@google.com>
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 fs/verity/Kconfig            |  1 +
 fs/verity/fsverity_private.h |  7 ------
 fs/verity/measure.c          | 41 ++++++++++++++++++++++++++++++++++++
 include/linux/fsverity.h     | 18 ++++++++++++++++
 4 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/fs/verity/Kconfig b/fs/verity/Kconfig
index 24d1b54de807..54598cd80145 100644
--- a/fs/verity/Kconfig
+++ b/fs/verity/Kconfig
@@ -3,6 +3,7 @@
 config FS_VERITY
 	bool "FS Verity (read-only file-based authenticity protection)"
 	select CRYPTO
+	select CRYPTO_HASH_INFO
 	# SHA-256 is implied as it's intended to be the default hash algorithm.
 	# To avoid bloat, other wanted algorithms must be selected explicitly.
 	# Note that CRYPTO_SHA256 denotes the generic C implementation, but
diff --git a/fs/verity/fsverity_private.h b/fs/verity/fsverity_private.h
index a7920434bae5..c6fb62e0ef1a 100644
--- a/fs/verity/fsverity_private.h
+++ b/fs/verity/fsverity_private.h
@@ -14,7 +14,6 @@
 
 #define pr_fmt(fmt) "fs-verity: " fmt
 
-#include <crypto/sha2.h>
 #include <linux/fsverity.h>
 #include <linux/mempool.h>
 
@@ -26,12 +25,6 @@ struct ahash_request;
  */
 #define FS_VERITY_MAX_LEVELS		8
 
-/*
- * Largest digest size among all hash algorithms supported by fs-verity.
- * Currently assumed to be <= size of fsverity_descriptor::root_hash.
- */
-#define FS_VERITY_MAX_DIGEST_SIZE	SHA512_DIGEST_SIZE
-
 /* A hash algorithm supported by fs-verity */
 struct fsverity_hash_alg {
 	struct crypto_ahash *tfm; /* hash tfm, allocated on demand */
diff --git a/fs/verity/measure.c b/fs/verity/measure.c
index f0d7b30c62db..f832aaa41326 100644
--- a/fs/verity/measure.c
+++ b/fs/verity/measure.c
@@ -57,3 +57,44 @@ int fsverity_ioctl_measure(struct file *filp, void __user *_uarg)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fsverity_ioctl_measure);
+
+/**
+ * fsverity_get_digest() - get a verity file's digest
+ * @inode: inode to get digest of
+ * @digest: (out) pointer to the digest
+ * @alg: (out) pointer to the hash algorithm enumeration
+ *
+ * Return the file hash algorithm and digest of an fsverity protected file.
+ *
+ * Return: 0 on success, -errno on failure
+ */
+int fsverity_get_digest(struct inode *inode,
+			u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
+			enum hash_algo *alg)
+{
+	const struct fsverity_info *vi;
+	const struct fsverity_hash_alg *hash_alg;
+	int i;
+
+	vi = fsverity_get_info(inode);
+	if (!vi)
+		return -ENODATA; /* not a verity file */
+
+	hash_alg = vi->tree_params.hash_alg;
+	memset(digest, 0, FS_VERITY_MAX_DIGEST_SIZE);
+
+	/* convert the verity hash algorithm name to a hash_algo_name enum */
+	i = match_string(hash_algo_name, HASH_ALGO__LAST, hash_alg->name);
+	if (i < 0)
+		return -EINVAL;
+	*alg = i;
+
+	if (WARN_ON_ONCE(hash_alg->digest_size != hash_digest_size[*alg]))
+		return -EINVAL;
+	memcpy(digest, vi->file_digest, hash_alg->digest_size);
+
+	pr_debug("file digest %s:%*phN\n", hash_algo_name[*alg],
+		 hash_digest_size[*alg], digest);
+
+	return 0;
+}
diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
index b568b3c7d095..9a1b70cc7318 100644
--- a/include/linux/fsverity.h
+++ b/include/linux/fsverity.h
@@ -12,8 +12,16 @@
 #define _LINUX_FSVERITY_H
 
 #include <linux/fs.h>
+#include <crypto/hash_info.h>
+#include <crypto/sha2.h>
 #include <uapi/linux/fsverity.h>
 
+/*
+ * Largest digest size among all hash algorithms supported by fs-verity.
+ * Currently assumed to be <= size of fsverity_descriptor::root_hash.
+ */
+#define FS_VERITY_MAX_DIGEST_SIZE	SHA512_DIGEST_SIZE
+
 /* Verity operations for filesystems */
 struct fsverity_operations {
 
@@ -131,6 +139,9 @@ int fsverity_ioctl_enable(struct file *filp, const void __user *arg);
 /* measure.c */
 
 int fsverity_ioctl_measure(struct file *filp, void __user *arg);
+int fsverity_get_digest(struct inode *inode,
+			u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
+			enum hash_algo *alg);
 
 /* open.c */
 
@@ -170,6 +181,13 @@ static inline int fsverity_ioctl_measure(struct file *filp, void __user *arg)
 	return -EOPNOTSUPP;
 }
 
+static inline int fsverity_get_digest(struct inode *inode,
+				      u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
+				      enum hash_algo *alg)
+{
+	return -EOPNOTSUPP;
+}
+
 /* open.c */
 
 static inline int fsverity_file_open(struct inode *inode, struct file *filp)
-- 
2.27.0

Re: [PATCH v5 3/8] fs-verity: define a function to return the integrity protected file digest

[Eric Biggers]

On Fri, Feb 11, 2022 at 04:43:05PM -0500, Mimi Zohar wrote:
> +/**
> + * fsverity_get_digest() - get a verity file's digest
> + * @inode: inode to get digest of
> + * @digest: (out) pointer to the digest
> + * @alg: (out) pointer to the hash algorithm enumeration
> + *
> + * Return the file hash algorithm and digest of an fsverity protected file.
> + *
> + * Return: 0 on success, -errno on failure
> + */
> +int fsverity_get_digest(struct inode *inode,
> +			u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
> +			enum hash_algo *alg)
> +{
> +	const struct fsverity_info *vi;
> +	const struct fsverity_hash_alg *hash_alg;
> +	int i;
> +
> +	vi = fsverity_get_info(inode);
> +	if (!vi)
> +		return -ENODATA; /* not a verity file */

Sorry for the slow reviews; I'm taking a look again now.  One question about
something I missed earlier: is the file guaranteed to have been opened before
this is called?  fsverity_get_info() only returns a non-NULL value if the file
has been opened at least once since the inode has been loaded into memory.  If
the inode has just been loaded into memory without being opened, for example due
to a call to stat(), then fsverity_get_info() will return NULL.

If the file is guaranteed to have been opened, then the code is fine, but the
comment for fsverity_get_digest() perhaps should be updated to mention this
assumption, given that it takes a struct inode rather than a struct file.

If the file is *not* guaranteed to have been opened, then it would be necessary
to make fsverity_get_digest() call ensure_verity_info() to set up the
fsverity_info.

- Eric

Re: [PATCH v5 3/8] fs-verity: define a function to return the integrity protected file digest

[Mimi Zohar]

On Wed, 2022-02-23 at 15:59 -0800, Eric Biggers wrote:
> On Fri, Feb 11, 2022 at 04:43:05PM -0500, Mimi Zohar wrote:
> > +/**
> > + * fsverity_get_digest() - get a verity file's digest
> > + * @inode: inode to get digest of
> > + * @digest: (out) pointer to the digest
> > + * @alg: (out) pointer to the hash algorithm enumeration
> > + *
> > + * Return the file hash algorithm and digest of an fsverity protected file.
> > + *
> > + * Return: 0 on success, -errno on failure
> > + */
> > +int fsverity_get_digest(struct inode *inode,
> > +			u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
> > +			enum hash_algo *alg)
> > +{
> > +	const struct fsverity_info *vi;
> > +	const struct fsverity_hash_alg *hash_alg;
> > +	int i;
> > +
> > +	vi = fsverity_get_info(inode);
> > +	if (!vi)
> > +		return -ENODATA; /* not a verity file */
> 
> Sorry for the slow reviews; I'm taking a look again now.  One question about
> something I missed earlier: is the file guaranteed to have been opened before
> this is called?  fsverity_get_info() only returns a non-NULL value if the file
> has been opened at least once since the inode has been loaded into memory.  If
> the inode has just been loaded into memory without being opened, for example due
> to a call to stat(), then fsverity_get_info() will return NULL.
> 
> If the file is guaranteed to have been opened, then the code is fine, but the
> comment for fsverity_get_digest() perhaps should be updated to mention this
> assumption, given that it takes a struct inode rather than a struct file.
> 
> If the file is *not* guaranteed to have been opened, then it would be necessary
> to make fsverity_get_digest() call ensure_verity_info() to set up the
> fsverity_info.

Yes, fsverity_get_digest() is called as a result of a syscall - open,
execve, mmap, etc.   
Refer to the LSM hooks security_bprm_check() and security_mmap_file().
ima_file_check() is called directly in do_open().

Mimi

Re: [PATCH v5 3/8] fs-verity: define a function to return the integrity protected file digest

[Eric Biggers]

On Wed, Feb 23, 2022 at 08:21:01PM -0500, Mimi Zohar wrote:
> On Wed, 2022-02-23 at 15:59 -0800, Eric Biggers wrote:
> > On Fri, Feb 11, 2022 at 04:43:05PM -0500, Mimi Zohar wrote:
> > > +/**
> > > + * fsverity_get_digest() - get a verity file's digest
> > > + * @inode: inode to get digest of
> > > + * @digest: (out) pointer to the digest
> > > + * @alg: (out) pointer to the hash algorithm enumeration
> > > + *
> > > + * Return the file hash algorithm and digest of an fsverity protected file.
> > > + *
> > > + * Return: 0 on success, -errno on failure
> > > + */
> > > +int fsverity_get_digest(struct inode *inode,
> > > +			u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
> > > +			enum hash_algo *alg)
> > > +{
> > > +	const struct fsverity_info *vi;
> > > +	const struct fsverity_hash_alg *hash_alg;
> > > +	int i;
> > > +
> > > +	vi = fsverity_get_info(inode);
> > > +	if (!vi)
> > > +		return -ENODATA; /* not a verity file */
> > 
> > Sorry for the slow reviews; I'm taking a look again now.  One question about
> > something I missed earlier: is the file guaranteed to have been opened before
> > this is called?  fsverity_get_info() only returns a non-NULL value if the file
> > has been opened at least once since the inode has been loaded into memory.  If
> > the inode has just been loaded into memory without being opened, for example due
> > to a call to stat(), then fsverity_get_info() will return NULL.
> > 
> > If the file is guaranteed to have been opened, then the code is fine, but the
> > comment for fsverity_get_digest() perhaps should be updated to mention this
> > assumption, given that it takes a struct inode rather than a struct file.
> > 
> > If the file is *not* guaranteed to have been opened, then it would be necessary
> > to make fsverity_get_digest() call ensure_verity_info() to set up the
> > fsverity_info.
> 
> Yes, fsverity_get_digest() is called as a result of a syscall - open,
> execve, mmap, etc.   
> Refer to the LSM hooks security_bprm_check() and security_mmap_file().
> ima_file_check() is called directly in do_open().

stat() is a syscall too, so the question is not whether this is being called as
a result of a syscall, but rather whether it's only being called while the file
is open (or at least previously opened).  Is the answer to that "yes"?

- Eric

Re: [PATCH v5 3/8] fs-verity: define a function to return the integrity protected file digest

[Mimi Zohar]

On Wed, 2022-02-23 at 17:26 -0800, Eric Biggers wrote:
> On Wed, Feb 23, 2022 at 08:21:01PM -0500, Mimi Zohar wrote:
> > On Wed, 2022-02-23 at 15:59 -0800, Eric Biggers wrote:
> > > On Fri, Feb 11, 2022 at 04:43:05PM -0500, Mimi Zohar wrote:
> > > > +/**
> > > > + * fsverity_get_digest() - get a verity file's digest
> > > > + * @inode: inode to get digest of
> > > > + * @digest: (out) pointer to the digest
> > > > + * @alg: (out) pointer to the hash algorithm enumeration
> > > > + *
> > > > + * Return the file hash algorithm and digest of an fsverity protected file.
> > > > + *
> > > > + * Return: 0 on success, -errno on failure
> > > > + */
> > > > +int fsverity_get_digest(struct inode *inode,
> > > > +			u8 digest[FS_VERITY_MAX_DIGEST_SIZE],
> > > > +			enum hash_algo *alg)
> > > > +{
> > > > +	const struct fsverity_info *vi;
> > > > +	const struct fsverity_hash_alg *hash_alg;
> > > > +	int i;
> > > > +
> > > > +	vi = fsverity_get_info(inode);
> > > > +	if (!vi)
> > > > +		return -ENODATA; /* not a verity file */
> > > 
> > > Sorry for the slow reviews; I'm taking a look again now.  One question about
> > > something I missed earlier: is the file guaranteed to have been opened before
> > > this is called?  fsverity_get_info() only returns a non-NULL value if the file
> > > has been opened at least once since the inode has been loaded into memory.  If
> > > the inode has just been loaded into memory without being opened, for example due
> > > to a call to stat(), then fsverity_get_info() will return NULL.
> > > 
> > > If the file is guaranteed to have been opened, then the code is fine, but the
> > > comment for fsverity_get_digest() perhaps should be updated to mention this
> > > assumption, given that it takes a struct inode rather than a struct file.
> > > 
> > > If the file is *not* guaranteed to have been opened, then it would be necessary
> > > to make fsverity_get_digest() call ensure_verity_info() to set up the
> > > fsverity_info.
> > 
> > Yes, fsverity_get_digest() is called as a result of a syscall - open,
> > execve, mmap, etc.   
> > Refer to the LSM hooks security_bprm_check() and security_mmap_file().
> > ima_file_check() is called directly in do_open().
> 
> stat() is a syscall too, so the question is not whether this is being called as
> a result of a syscall, but rather whether it's only being called while the file
> is open (or at least previously opened).  Is the answer to that "yes"?

yes

[PATCH v5 4/8] ima: define a new template field 'd-type' and a new template 'ima-ngv2'

[Mimi Zohar]

In preparation to differentiate between regular IMA file hashes and
fs-verity's file digests, define a new template field named 'd-type'.
Define a new template named 'ima-ngv2', which includes the new 'd-type'
field.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_template.c     |  3 +++
 security/integrity/ima/ima_template_lib.c | 13 +++++++++++++
 security/integrity/ima/ima_template_lib.h |  2 ++
 3 files changed, 18 insertions(+)

diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
index db1ad6d7a57f..b321342e5bee 100644
--- a/security/integrity/ima/ima_template.c
+++ b/security/integrity/ima/ima_template.c
@@ -19,6 +19,7 @@ enum header_fields { HDR_PCR, HDR_DIGEST, HDR_TEMPLATE_NAME,
 static struct ima_template_desc builtin_templates[] = {
 	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
 	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
+	{.name = "ima-ngv2", .fmt = "d-ng|n-ng|d-type"},
 	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
 	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
 	{.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"},
@@ -40,6 +41,8 @@ static const struct ima_template_field supported_fields[] = {
 	 .field_show = ima_show_template_digest_ng},
 	{.field_id = "n-ng", .field_init = ima_eventname_ng_init,
 	 .field_show = ima_show_template_string},
+	{.field_id = "d-type", .field_init = ima_eventdigest_type_init,
+	 .field_show = ima_show_template_string},
 	{.field_id = "sig", .field_init = ima_eventsig_init,
 	 .field_show = ima_show_template_sig},
 	{.field_id = "buf", .field_init = ima_eventbuf_init,
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index 7155d17a3b75..a213579e825e 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -383,6 +383,19 @@ int ima_eventdigest_ng_init(struct ima_event_data *event_data,
 					   hash_algo, field_data);
 }
 
+/*
+ * This function writes the digest type of an event.
+ */
+int ima_eventdigest_type_init(struct ima_event_data *event_data,
+			      struct ima_field_data *field_data)
+{
+	static const char * const digest_type[] = {"ima"};
+
+	return ima_write_template_field_data(digest_type[0],
+					     strlen(digest_type[0]),
+					     DATA_FMT_STRING, field_data);
+}
+
 /*
  * This function writes the digest of the file which is expected to match the
  * digest contained in the file's appended signature.
diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
index c71f1de95753..539a5e354925 100644
--- a/security/integrity/ima/ima_template_lib.h
+++ b/security/integrity/ima/ima_template_lib.h
@@ -38,6 +38,8 @@ int ima_eventname_init(struct ima_event_data *event_data,
 		       struct ima_field_data *field_data);
 int ima_eventdigest_ng_init(struct ima_event_data *event_data,
 			    struct ima_field_data *field_data);
+int ima_eventdigest_type_init(struct ima_event_data *event_data,
+			      struct ima_field_data *field_data);
 int ima_eventdigest_modsig_init(struct ima_event_data *event_data,
 				struct ima_field_data *field_data);
 int ima_eventname_ng_init(struct ima_event_data *event_data,
-- 
2.27.0

Re: [PATCH v5 4/8] ima: define a new template field 'd-type' and a new template 'ima-ngv2'

[Stefan Berger]

On 2/11/22 16:43, Mimi Zohar wrote:
> In preparation to differentiate between regular IMA file hashes and
> fs-verity's file digests, define a new template field named 'd-type'.
> Define a new template named 'ima-ngv2', which includes the new 'd-type'
> field.
>
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>   security/integrity/ima/ima_template.c     |  3 +++
>   security/integrity/ima/ima_template_lib.c | 13 +++++++++++++
>   security/integrity/ima/ima_template_lib.h |  2 ++
>   3 files changed, 18 insertions(+)
>
> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> index db1ad6d7a57f..b321342e5bee 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -19,6 +19,7 @@ enum header_fields { HDR_PCR, HDR_DIGEST, HDR_TEMPLATE_NAME,
>   static struct ima_template_desc builtin_templates[] = {
>   	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
>   	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
> +	{.name = "ima-ngv2", .fmt = "d-ng|n-ng|d-type"},
>   	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
>   	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
>   	{.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"},
> @@ -40,6 +41,8 @@ static const struct ima_template_field supported_fields[] = {
>   	 .field_show = ima_show_template_digest_ng},
>   	{.field_id = "n-ng", .field_init = ima_eventname_ng_init,
>   	 .field_show = ima_show_template_string},
> +	{.field_id = "d-type", .field_init = ima_eventdigest_type_init,
> +	 .field_show = ima_show_template_string},
>   	{.field_id = "sig", .field_init = ima_eventsig_init,
>   	 .field_show = ima_show_template_sig},
>   	{.field_id = "buf", .field_init = ima_eventbuf_init,
> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index 7155d17a3b75..a213579e825e 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -383,6 +383,19 @@ int ima_eventdigest_ng_init(struct ima_event_data *event_data,
>   					   hash_algo, field_data);
>   }
>   
> +/*
> + * This function writes the digest type of an event.
> + */
> +int ima_eventdigest_type_init(struct ima_event_data *event_data,
> +			      struct ima_field_data *field_data)
> +{
> +	static const char * const digest_type[] = {"ima"};

This array makes sense with 6/8.

Acked-by: Stefan Berger <stefanb@linux.ibm.com>


> +
> +	return ima_write_template_field_data(digest_type[0],
> +					     strlen(digest_type[0]),
> +					     DATA_FMT_STRING, field_data);
> +}
> +
>   /*
>    * This function writes the digest of the file which is expected to match the
>    * digest contained in the file's appended signature.
> diff --git a/security/integrity/ima/ima_template_lib.h b/security/integrity/ima/ima_template_lib.h
> index c71f1de95753..539a5e354925 100644
> --- a/security/integrity/ima/ima_template_lib.h
> +++ b/security/integrity/ima/ima_template_lib.h
> @@ -38,6 +38,8 @@ int ima_eventname_init(struct ima_event_data *event_data,
>   		       struct ima_field_data *field_data);
>   int ima_eventdigest_ng_init(struct ima_event_data *event_data,
>   			    struct ima_field_data *field_data);
> +int ima_eventdigest_type_init(struct ima_event_data *event_data,
> +			      struct ima_field_data *field_data);
>   int ima_eventdigest_modsig_init(struct ima_event_data *event_data,
>   				struct ima_field_data *field_data);
>   int ima_eventname_ng_init(struct ima_event_data *event_data,

Re: [PATCH v5 4/8] ima: define a new template field 'd-type' and a new template 'ima-ngv2'

[Eric Biggers]

On Fri, Feb 11, 2022 at 04:43:06PM -0500, Mimi Zohar wrote:
> In preparation to differentiate between regular IMA file hashes and
> fs-verity's file digests, define a new template field named 'd-type'.
> Define a new template named 'ima-ngv2', which includes the new 'd-type'
> field.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  security/integrity/ima/ima_template.c     |  3 +++
>  security/integrity/ima/ima_template_lib.c | 13 +++++++++++++
>  security/integrity/ima/ima_template_lib.h |  2 ++
>  3 files changed, 18 insertions(+)
> 
> diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> index db1ad6d7a57f..b321342e5bee 100644
> --- a/security/integrity/ima/ima_template.c
> +++ b/security/integrity/ima/ima_template.c
> @@ -19,6 +19,7 @@ enum header_fields { HDR_PCR, HDR_DIGEST, HDR_TEMPLATE_NAME,
>  static struct ima_template_desc builtin_templates[] = {
>  	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
>  	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
> +	{.name = "ima-ngv2", .fmt = "d-ng|n-ng|d-type"},
>  	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
>  	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
>  	{.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"},
> @@ -40,6 +41,8 @@ static const struct ima_template_field supported_fields[] = {
>  	 .field_show = ima_show_template_digest_ng},
>  	{.field_id = "n-ng", .field_init = ima_eventname_ng_init,
>  	 .field_show = ima_show_template_string},
> +	{.field_id = "d-type", .field_init = ima_eventdigest_type_init,
> +	 .field_show = ima_show_template_string},
>  	{.field_id = "sig", .field_init = ima_eventsig_init,
>  	 .field_show = ima_show_template_sig},
>  	{.field_id = "buf", .field_init = ima_eventbuf_init,

I notice that the "d-ng" field already contains both the hash algorithm and the
hash itself, in the form <algorithm>:<hash>.  Wouldn't it make more sense to
define a "d-ngv2" field that contains <type>:<algorithm>:<hash>?  After all,
both the type and algorithm are required to interpret the hash.

Or in other words, what about the hash type is different from the hash algorithm
that would result in them needing different handling here?

- Eric

Re: [PATCH v5 4/8] ima: define a new template field 'd-type' and a new template 'ima-ngv2'

[Mimi Zohar]

On Wed, 2022-02-23 at 16:32 -0800, Eric Biggers wrote:
> On Fri, Feb 11, 2022 at 04:43:06PM -0500, Mimi Zohar wrote:
> > In preparation to differentiate between regular IMA file hashes and
> > fs-verity's file digests, define a new template field named 'd-type'.
> > Define a new template named 'ima-ngv2', which includes the new 'd-type'
> > field.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > ---
> >  security/integrity/ima/ima_template.c     |  3 +++
> >  security/integrity/ima/ima_template_lib.c | 13 +++++++++++++
> >  security/integrity/ima/ima_template_lib.h |  2 ++
> >  3 files changed, 18 insertions(+)
> > 
> > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> > index db1ad6d7a57f..b321342e5bee 100644
> > --- a/security/integrity/ima/ima_template.c
> > +++ b/security/integrity/ima/ima_template.c
> > @@ -19,6 +19,7 @@ enum header_fields { HDR_PCR, HDR_DIGEST, HDR_TEMPLATE_NAME,
> >  static struct ima_template_desc builtin_templates[] = {
> >  	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
> >  	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
> > +	{.name = "ima-ngv2", .fmt = "d-ng|n-ng|d-type"},
> >  	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
> >  	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
> >  	{.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"},
> > @@ -40,6 +41,8 @@ static const struct ima_template_field supported_fields[] = {
> >  	 .field_show = ima_show_template_digest_ng},
> >  	{.field_id = "n-ng", .field_init = ima_eventname_ng_init,
> >  	 .field_show = ima_show_template_string},
> > +	{.field_id = "d-type", .field_init = ima_eventdigest_type_init,
> > +	 .field_show = ima_show_template_string},
> >  	{.field_id = "sig", .field_init = ima_eventsig_init,
> >  	 .field_show = ima_show_template_sig},
> >  	{.field_id = "buf", .field_init = ima_eventbuf_init,
> 
> I notice that the "d-ng" field already contains both the hash algorithm and the
> hash itself, in the form <algorithm>:<hash>.  Wouldn't it make more sense to
> define a "d-ngv2" field that contains <type>:<algorithm>:<hash>?  After all,
> both the type and algorithm are required to interpret the hash.
> 
> Or in other words, what about the hash type is different from the hash algorithm
> that would result in them needing different handling here?

Thanks, Eric.  I really like your suggestion.  Currently, type is
defined as either "ima" or "verity".   Are you ok with prefixing each
record with these strings?

-- 
thanks,

Mimi

Re: [PATCH v5 4/8] ima: define a new template field 'd-type' and a new template 'ima-ngv2'

[Eric Biggers]

On Thu, Feb 24, 2022 at 11:16:51AM -0500, Mimi Zohar wrote:
> On Wed, 2022-02-23 at 16:32 -0800, Eric Biggers wrote:
> > On Fri, Feb 11, 2022 at 04:43:06PM -0500, Mimi Zohar wrote:
> > > In preparation to differentiate between regular IMA file hashes and
> > > fs-verity's file digests, define a new template field named 'd-type'.
> > > Define a new template named 'ima-ngv2', which includes the new 'd-type'
> > > field.
> > > 
> > > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> > > ---
> > >  security/integrity/ima/ima_template.c     |  3 +++
> > >  security/integrity/ima/ima_template_lib.c | 13 +++++++++++++
> > >  security/integrity/ima/ima_template_lib.h |  2 ++
> > >  3 files changed, 18 insertions(+)
> > > 
> > > diff --git a/security/integrity/ima/ima_template.c b/security/integrity/ima/ima_template.c
> > > index db1ad6d7a57f..b321342e5bee 100644
> > > --- a/security/integrity/ima/ima_template.c
> > > +++ b/security/integrity/ima/ima_template.c
> > > @@ -19,6 +19,7 @@ enum header_fields { HDR_PCR, HDR_DIGEST, HDR_TEMPLATE_NAME,
> > >  static struct ima_template_desc builtin_templates[] = {
> > >  	{.name = IMA_TEMPLATE_IMA_NAME, .fmt = IMA_TEMPLATE_IMA_FMT},
> > >  	{.name = "ima-ng", .fmt = "d-ng|n-ng"},
> > > +	{.name = "ima-ngv2", .fmt = "d-ng|n-ng|d-type"},
> > >  	{.name = "ima-sig", .fmt = "d-ng|n-ng|sig"},
> > >  	{.name = "ima-buf", .fmt = "d-ng|n-ng|buf"},
> > >  	{.name = "ima-modsig", .fmt = "d-ng|n-ng|sig|d-modsig|modsig"},
> > > @@ -40,6 +41,8 @@ static const struct ima_template_field supported_fields[] = {
> > >  	 .field_show = ima_show_template_digest_ng},
> > >  	{.field_id = "n-ng", .field_init = ima_eventname_ng_init,
> > >  	 .field_show = ima_show_template_string},
> > > +	{.field_id = "d-type", .field_init = ima_eventdigest_type_init,
> > > +	 .field_show = ima_show_template_string},
> > >  	{.field_id = "sig", .field_init = ima_eventsig_init,
> > >  	 .field_show = ima_show_template_sig},
> > >  	{.field_id = "buf", .field_init = ima_eventbuf_init,
> > 
> > I notice that the "d-ng" field already contains both the hash algorithm and the
> > hash itself, in the form <algorithm>:<hash>.  Wouldn't it make more sense to
> > define a "d-ngv2" field that contains <type>:<algorithm>:<hash>?  After all,
> > both the type and algorithm are required to interpret the hash.
> > 
> > Or in other words, what about the hash type is different from the hash algorithm
> > that would result in them needing different handling here?
> 
> Thanks, Eric.  I really like your suggestion.  Currently, type is
> defined as either "ima" or "verity".   Are you ok with prefixing each
> record with these strings?
> 

As I mentioned before I think "full_hash" would be more descriptive than "ima".
(All of this is part of IMA.)  It's up to you though.

- Eric

Re: [PATCH v5 4/8] ima: define a new template field 'd-type' and a new template 'ima-ngv2'

[Mimi Zohar]

On Thu, 2022-02-24 at 10:46 -0800, Eric Biggers wrote:
> > Thanks, Eric.  I really like your suggestion.  Currently, type is
> > defined as either "ima" or "verity".   Are you ok with prefixing each
> > record with these strings?
> > 
> 
> As I mentioned before I think "full_hash" would be more descriptive than "ima".
> (All of this is part of IMA.)  It's up to you though.

Sorry, to me that doesn't sense.  Either we use "full_hash" and
"Merkle-tree", or "ima" and "verity".

-- 
thanks,

Mimi

[PATCH v5 5/8] ima: permit fsverity's file digests in the IMA measurement list

[Mimi Zohar]

Permit fsverity's file digest (a hash of struct fsverity_digest) to be
included in the 'd-ng' field of the IMA measurement list, based on the
new measurement policy rule 'digest_type=verity' option.

To differentiate between an unsigned regular IMA file hash and an
unsigned fsverity's file digest stored in the 'd-ng' field of the
measurement list, it is recommended to include the 'd-type' template
field.

The following policy rule requires fsverity file digests and specifies
the new 'ima-ngv2' template, which contains the new 'd-type' 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      | 10 ++++++
 Documentation/security/IMA-templates.rst  |  7 ++++
 security/integrity/ima/ima_api.c          | 39 +++++++++++++++++++++--
 security/integrity/ima/ima_policy.c       | 38 +++++++++++++++++++++-
 security/integrity/ima/ima_template_lib.c |  9 +++++-
 security/integrity/integrity.h            |  4 ++-
 6 files changed, 102 insertions(+), 5 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 839fab811b18..ff3c906738cb 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,10 @@ Description:
 		security.ima xattr of a file:
 
 			appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512
+
+		Example of 'measure' rule requiring fs-verity's digests on a
+		particular filesystem with indication of type of digest in
+		the measurement list.
+
+			measure func=FILE_CHECK digest_type=verity \
+				fsuuid=... template=ima-ngv2
diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index 1a91d92950a7..1e3fe986764e 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -69,6 +69,8 @@ descriptors by adding their identifier to the format string
    algorithm (field format: [<hash algo>:]digest, where the digest
    prefix is shown only if the hash algorithm is not SHA1 or MD5);
  - 'd-modsig': the digest of the event without the appended modsig;
+ - 'd-type': differentiate between fs-verity's Merkle tree based file hash
+   from a regular IMA file hash measurement.
  - 'n-ng': the name of the event, without size limitations;
  - 'sig': the file signature, or the EVM portable signature if the file
    signature is not found;
@@ -106,3 +108,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..68c93a1cb876 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,23 @@ 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)
+{
+	u8 verity_digest[FS_VERITY_MAX_DIGEST_SIZE];
+	enum hash_algo verity_alg;
+	int ret;
+
+	ret = fsverity_get_digest(iint->inode, verity_digest, &verity_alg);
+	if (ret)
+		return ret;
+	if (hash->hdr.algo != verity_alg)
+		return -EINVAL;
+	hash->hdr.length = hash_digest_size[verity_alg];
+	memcpy(hash->hdr.digest, verity_digest, hash->hdr.length);
+	return 0;
+}
+
 /*
  * ima_collect_measurement - collect file measurement
  *
@@ -242,14 +260,31 @@ 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:
+			iint->flags |= IMA_VERITY_DIGEST;
+			break;
+		case -ENODATA:
+			audit_cause = "no-verity-digest";
+			result = -EINVAL;
+			break;
+		case -EINVAL:
+		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..28aca1f9633b 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-type 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-type",
+				     "verity rules should include d-type");
+	}
+
 	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 a213579e825e..d370fca04de4 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -389,7 +389,14 @@ int ima_eventdigest_ng_init(struct ima_event_data *event_data,
 int ima_eventdigest_type_init(struct ima_event_data *event_data,
 			      struct ima_field_data *field_data)
 {
-	static const char * const digest_type[] = {"ima"};
+	static const char * const digest_type[] = {"ima", "verity"};
+
+	if (event_data->iint->flags & IMA_VERITY_DIGEST) {
+		return ima_write_template_field_data(digest_type[1],
+						     strlen(digest_type[1]),
+						     DATA_FMT_STRING,
+						     field_data);
+	}
 
 	return ima_write_template_field_data(digest_type[0],
 					     strlen(digest_type[0]),
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index daf49894fd7d..39a999877013 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,8 @@
 #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
 #define IMA_MODSIG_ALLOWED	0x20000000
 #define IMA_CHECK_BLACKLIST	0x40000000
+#define IMA_VERITY_REQUIRED	0x80000000
+#define IMA_VERITY_DIGEST	0x00800000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_HASH | IMA_APPRAISE_SUBMASK)
-- 
2.27.0

Re: [PATCH v5 5/8] ima: permit fsverity's file digests in the IMA measurement list

[Eric Biggers]

On Fri, Feb 11, 2022 at 04:43:07PM -0500, Mimi Zohar wrote:
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 839fab811b18..ff3c906738cb 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,10 @@ Description:
>  		security.ima xattr of a file:
>  
>  			appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512
> +
> +		Example of 'measure' rule requiring fs-verity's digests on a
> +		particular filesystem with indication of type of digest in
> +		the measurement list.
> +
> +			measure func=FILE_CHECK digest_type=verity \
> +				fsuuid=... template=ima-ngv2
> diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
> index 1a91d92950a7..1e3fe986764e 100644
> --- a/Documentation/security/IMA-templates.rst
> +++ b/Documentation/security/IMA-templates.rst
> @@ -69,6 +69,8 @@ descriptors by adding their identifier to the format string
>     algorithm (field format: [<hash algo>:]digest, where the digest
>     prefix is shown only if the hash algorithm is not SHA1 or MD5);
>   - 'd-modsig': the digest of the event without the appended modsig;
> + - 'd-type': differentiate between fs-verity's Merkle tree based file hash
> +   from a regular IMA file hash measurement.
>   - 'n-ng': the name of the event, without size limitations;
>   - 'sig': the file signature, or the EVM portable signature if the file
>     signature is not found;
> @@ -106,3 +108,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=``.

Is there more IMA documentation elsewhere, or is this everything?  These files
are hard to follow.

> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index daf49894fd7d..39a999877013 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,8 @@
>  #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
>  #define IMA_MODSIG_ALLOWED	0x20000000
>  #define IMA_CHECK_BLACKLIST	0x40000000
> +#define IMA_VERITY_REQUIRED	0x80000000
> +#define IMA_VERITY_DIGEST	0x00800000

How about defining these flags in numerical order?

- Eric

Re: [PATCH v5 5/8] ima: permit fsverity's file digests in the IMA measurement list

[Mimi Zohar]

> diff --git a/security/integrity/integrity.h
b/security/integrity/integrity.h
> > index daf49894fd7d..39a999877013 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,8 @@
> >  #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
> >  #define IMA_MODSIG_ALLOWED	0x20000000
> >  #define IMA_CHECK_BLACKLIST	0x40000000
> > +#define IMA_VERITY_REQUIRED	0x80000000
> > +#define IMA_VERITY_DIGEST	0x00800000
> 
> How about defining these flags in numerical order?

Originally I increased the flags size, but I'd like to avoid as much
patch churn as possible for the namespacing patch set.

Mimi

[PATCH v5 6/8] ima: define signature version 3

[Mimi Zohar]

To disambiguate the signed data stored in the 'security.ima' xattr,
define signature version 3 as the hash of the ima_file_id structure.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/digsig.c           |  3 ++-
 security/integrity/ima/ima_appraise.c | 35 +++++++++++++++++++++++++++
 security/integrity/integrity.h        | 20 +++++++++++++--
 3 files changed, 55 insertions(+), 3 deletions(-)

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 3b06a01bd0fd..fd8f77d92a62 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -74,7 +74,8 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
 		/* v1 API expect signature without xattr type */
 		return digsig_verify(keyring, sig + 1, siglen - 1, digest,
 				     digestlen);
-	case 2:
+	case 2: /* regular file data hash based sginature */
+	case 3: /* struct ima_file_id data base signature */
 		return asymmetric_verify(keyring, sig, siglen, digest,
 					 digestlen);
 	}
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 17232bbfb9f9..c2b429c141a7 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -225,6 +225,33 @@ int ima_read_xattr(struct dentry *dentry,
 	return ret;
 }
 
+/*
+ * calc_file_id_hash - calculate the hash of the ima_file_id struct data
+ * @type: xattr type [enum evm_ima_xattr_type]
+ * @algo: hash algorithm [enum hash_algo]
+ * @digest: pointer to the digest to be hashed
+ * @hash: (out) pointer to the hash
+ *
+ * IMA signature version 3 disambiguates the data that is signed by
+ * indirectly signing the hash of the ima_file_id structure data.
+ *
+ * Return 0 on success, error code otherwise.
+ */
+static int calc_file_id_hash(enum evm_ima_xattr_type type,
+			     enum hash_algo algo, const u8 *digest,
+			     struct ima_digest_data *hash)
+{
+	struct ima_file_id file_id = {.hash_algorithm = algo};
+	uint unused = HASH_MAX_DIGESTSIZE - hash_digest_size[algo];
+
+	memcpy(file_id.hash, digest, hash_digest_size[algo]);
+
+	hash->algo = algo;
+	hash->length = hash_digest_size[algo];
+
+	return ima_calc_buffer_hash(&file_id, sizeof(file_id) - unused, hash);
+}
+
 /*
  * xattr_verify - verify xattr digest or signature
  *
@@ -236,6 +263,7 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 			struct evm_ima_xattr_data *xattr_value, int xattr_len,
 			enum integrity_status *status, const char **cause)
 {
+	struct signature_v2_hdr *sig;
 	int rc = -EINVAL, hash_start = 0;
 
 	switch (xattr_value->type) {
@@ -274,6 +302,13 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 		break;
 	case EVM_IMA_XATTR_DIGSIG:
 		set_bit(IMA_DIGSIG, &iint->atomic_flags);
+
+		sig = (typeof(sig))xattr_value;
+		if (sig->version != 2) {
+			*cause = "invalid-signature-version";
+			*status = INTEGRITY_FAIL;
+			break;
+		}
 		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
 					     (const char *)xattr_value,
 					     xattr_len,
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 39a999877013..bd38bd451b19 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -94,7 +94,7 @@ struct evm_xattr {
 	u8 digest[SHA1_DIGEST_SIZE];
 } __packed;
 
-#define IMA_MAX_DIGEST_SIZE	64
+#define IMA_MAX_DIGEST_SIZE	HASH_MAX_DIGESTSIZE
 
 struct ima_digest_data {
 	u8 algo;
@@ -123,7 +123,11 @@ struct ima_max_digest_data {
 } __packed;
 
 /*
- * signature format v2 - for using with asymmetric keys
+ * signature header format v2 - for using with asymmetric keys
+ *
+ * signature format:
+ * version 2: regular file data hash based signature
+ * version 3: struct ima_file_id data based signature
  */
 struct signature_v2_hdr {
 	uint8_t type;		/* xattr type */
@@ -134,6 +138,18 @@ struct signature_v2_hdr {
 	uint8_t sig[];		/* signature payload */
 } __packed;
 
+/*
+ * IMA signature version 3 disambiguates the data that is signed, by
+ * indirectly signing the hash of the ima_file_id structure data.
+ *
+ * (The hash of the ima_file_id structure is only of the portion used.)
+ */
+struct ima_file_id {
+	__u8 hash_type;		/* xattr type [enum evm_ima_xattr_type] */
+	__u8 hash_algorithm;	/* Digest algorithm [enum hash_algo] */
+	__u8 hash[HASH_MAX_DIGESTSIZE];
+} __packed;
+
 /* integrity data associated with an inode */
 struct integrity_iint_cache {
 	struct rb_node rb_node;	/* rooted in integrity_iint_tree */
-- 
2.27.0

Re: [PATCH v5 6/8] ima: define signature version 3

[Eric Biggers]

On Fri, Feb 11, 2022 at 04:43:08PM -0500, Mimi Zohar wrote:
> To disambiguate the signed data stored in the 'security.ima' xattr,
> define signature version 3 as the hash of the ima_file_id structure.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  security/integrity/digsig.c           |  3 ++-
>  security/integrity/ima/ima_appraise.c | 35 +++++++++++++++++++++++++++
>  security/integrity/integrity.h        | 20 +++++++++++++--
>  3 files changed, 55 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> index 3b06a01bd0fd..fd8f77d92a62 100644
> --- a/security/integrity/digsig.c
> +++ b/security/integrity/digsig.c
> @@ -74,7 +74,8 @@ int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
>  		/* v1 API expect signature without xattr type */
>  		return digsig_verify(keyring, sig + 1, siglen - 1, digest,
>  				     digestlen);
> -	case 2:
> +	case 2: /* regular file data hash based sginature */
> +	case 3: /* struct ima_file_id data base signature */
>  		return asymmetric_verify(keyring, sig, siglen, digest,
>  					 digestlen);
>  	}
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 17232bbfb9f9..c2b429c141a7 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -225,6 +225,33 @@ int ima_read_xattr(struct dentry *dentry,
>  	return ret;
>  }
>  
> +/*
> + * calc_file_id_hash - calculate the hash of the ima_file_id struct data
> + * @type: xattr type [enum evm_ima_xattr_type]
> + * @algo: hash algorithm [enum hash_algo]
> + * @digest: pointer to the digest to be hashed
> + * @hash: (out) pointer to the hash
> + *
> + * IMA signature version 3 disambiguates the data that is signed by
> + * indirectly signing the hash of the ima_file_id structure data.
> + *
> + * Return 0 on success, error code otherwise.
> + */
> +static int calc_file_id_hash(enum evm_ima_xattr_type type,
> +			     enum hash_algo algo, const u8 *digest,
> +			     struct ima_digest_data *hash)
> +{
> +	struct ima_file_id file_id = {.hash_algorithm = algo};
> +	uint unused = HASH_MAX_DIGESTSIZE - hash_digest_size[algo];
> +
> +	memcpy(file_id.hash, digest, hash_digest_size[algo]);
> +
> +	hash->algo = algo;
> +	hash->length = hash_digest_size[algo];
> +
> +	return ima_calc_buffer_hash(&file_id, sizeof(file_id) - unused, hash);
> +}
> +

calc_file_id_hash() isn't used in this patch.

Perhaps this patch should be merged with the following one?  I don't understand
the point of this patch on its own.

- Eric

[PATCH v5 7/8] ima: support fs-verity file digest based version 3 signatures

[Mimi Zohar]

Instead of calculating a regular file hash and verifying the signature
stored in the 'security.ima' xattr against the calculated file hash, get
fs-verity's file digest and verify the signature (version 3) stored in
'security.ima' against the digest.

The policy rule 'appraise_type=' option is extended to support 'sigv3',
which is initiality limited to fs-verity.

The fs-verity 'appraise' rules are identified by the 'digest-type=verity'
option and require the 'appraise_type=sigv3' option.  The following
'appraise' policy rule requires fsverity file digests.  (The rule may be
constrained, for example based on a fsuuid or LSM label.)

Basic fs-verity policy rule example:
  appraise func=BPRM_CHECK digest_type=verity appraise_type=sigv3

Lastly, for IMA to differentiate between the original IMA signature
from an fs-verity signature a new 'xattr_type' named IMA_VERITY_DIGSIG
is defined.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 Documentation/ABI/testing/ima_policy      | 28 ++++++--
 Documentation/security/IMA-templates.rst  |  4 +-
 security/integrity/ima/ima_appraise.c     | 79 +++++++++++++++++++++--
 security/integrity/ima/ima_policy.c       | 26 ++++++--
 security/integrity/ima/ima_template_lib.c |  3 +-
 security/integrity/integrity.h            |  5 +-
 6 files changed, 127 insertions(+), 18 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index ff3c906738cb..508053b8dd0a 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -47,7 +47,7 @@ Description:
 			fgroup:= decimal value
 		  lsm:  are LSM specific
 		  option:
-			appraise_type:= [imasig] [imasig|modsig]
+			appraise_type:= [imasig] | [imasig|modsig] | [sigv3]
 			appraise_flag:= [check_blacklist]
 			Currently, blacklist check is only for files signed with appended
 			signature.
@@ -153,9 +153,27 @@ Description:
 
 			appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512
 
-		Example of 'measure' rule requiring fs-verity's digests on a
-		particular filesystem with indication of type of digest in
-		the measurement list.
+		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 \
-				fsuuid=... template=ima-ngv2
+				template=ima-ngv2
+
+		Example of 'measure' and 'appraise' rules requiring fs-verity
+		signatures (version 3) stored in security.ima xattr.
+
+		The 'measure' rule specifies the 'ima-sig' template option,
+		which includes the file signature in the measurement list.
+
+			measure func=BPRM_CHECK digest_type=verity \
+				template=ima-sig
+
+		The 'appraise' rule specifies the type and signature version
+		(sigv3) required.
+
+			appraise func=BPRM_CHECK digest_type=verity \
+				appraise_type=sigv3
+
+		All of these policy rules could, for example, be constrained
+		either based on a filesystem's UUID (fsuuid) or based on LSM
+		labels.
diff --git a/Documentation/security/IMA-templates.rst b/Documentation/security/IMA-templates.rst
index 1e3fe986764e..fe9bc2595fa2 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -72,8 +72,8 @@ descriptors by adding their identifier to the format string
  - 'd-type': differentiate between fs-verity's Merkle tree based file hash
    from a regular IMA file hash measurement.
  - 'n-ng': the name of the event, without size limitations;
- - 'sig': the file signature, or the EVM portable signature if the file
-   signature is not found;
+ - 'sig': the file signature, based on either the file's/fsverity's digest[1],
+   or the EVM portable signature if the file signature is not found;
  - 'modsig' the appended file signature;
  - 'buf': the buffer data that was used to generate the hash without size limitations;
  - 'evmsig': the EVM portable signature;
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index c2b429c141a7..71e27dba01ab 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -13,7 +13,9 @@
 #include <linux/magic.h>
 #include <linux/ima.h>
 #include <linux/evm.h>
+#include <linux/fsverity.h>
 #include <keys/system_keyring.h>
+#include <uapi/linux/fsverity.h>
 
 #include "ima.h"
 
@@ -183,13 +185,18 @@ enum hash_algo ima_get_hash_algo(const struct evm_ima_xattr_data *xattr_value,
 		return ima_hash_algo;
 
 	switch (xattr_value->type) {
+	case IMA_VERITY_DIGSIG:
+		sig = (typeof(sig))xattr_value;
+		if (sig->version != 3 || xattr_len <= sizeof(*sig) ||
+		    sig->hash_algo >= HASH_ALGO__LAST)
+			return ima_hash_algo;
+		return sig->hash_algo;
 	case EVM_IMA_XATTR_DIGSIG:
 		sig = (typeof(sig))xattr_value;
 		if (sig->version != 2 || xattr_len <= sizeof(*sig)
 		    || sig->hash_algo >= HASH_ALGO__LAST)
 			return ima_hash_algo;
 		return sig->hash_algo;
-		break;
 	case IMA_XATTR_DIGEST_NG:
 		/* first byte contains algorithm id */
 		ret = xattr_value->data[0];
@@ -235,15 +242,22 @@ int ima_read_xattr(struct dentry *dentry,
  * IMA signature version 3 disambiguates the data that is signed by
  * indirectly signing the hash of the ima_file_id structure data.
  *
+ * Signing the ima_file_id struct is currently only supported for
+ * IMA_VERITY_DIGSIG type xattrs.
+ *
  * Return 0 on success, error code otherwise.
  */
 static int calc_file_id_hash(enum evm_ima_xattr_type type,
 			     enum hash_algo algo, const u8 *digest,
 			     struct ima_digest_data *hash)
 {
-	struct ima_file_id file_id = {.hash_algorithm = algo};
+	struct ima_file_id file_id = {
+		.hash_type = IMA_VERITY_DIGSIG, .hash_algorithm = algo};
 	uint unused = HASH_MAX_DIGESTSIZE - hash_digest_size[algo];
 
+	if (type != IMA_VERITY_DIGSIG)
+		return -EINVAL;
+
 	memcpy(file_id.hash, digest, hash_digest_size[algo]);
 
 	hash->algo = algo;
@@ -263,6 +277,7 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 			struct evm_ima_xattr_data *xattr_value, int xattr_len,
 			enum integrity_status *status, const char **cause)
 {
+	struct ima_max_digest_data hash;
 	struct signature_v2_hdr *sig;
 	int rc = -EINVAL, hash_start = 0;
 
@@ -274,7 +289,10 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 	case IMA_XATTR_DIGEST:
 		if (*status != INTEGRITY_PASS_IMMUTABLE) {
 			if (iint->flags & IMA_DIGSIG_REQUIRED) {
-				*cause = "IMA-signature-required";
+				if (iint->flags & IMA_VERITY_REQUIRED)
+					*cause = "verity-signature-required";
+				else
+					*cause = "IMA-signature-required";
 				*status = INTEGRITY_FAIL;
 				break;
 			}
@@ -303,6 +321,12 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 	case EVM_IMA_XATTR_DIGSIG:
 		set_bit(IMA_DIGSIG, &iint->atomic_flags);
 
+		if (iint->flags & (IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED)) {
+			*cause = "verity-signature-required";
+			*status = INTEGRITY_FAIL;
+			break;
+		}
+
 		sig = (typeof(sig))xattr_value;
 		if (sig->version != 2) {
 			*cause = "invalid-signature-version";
@@ -331,6 +355,44 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
 		} else {
 			*status = INTEGRITY_PASS;
 		}
+		break;
+	case IMA_VERITY_DIGSIG:
+		set_bit(IMA_DIGSIG, &iint->atomic_flags);
+
+		if (iint->flags & IMA_DIGSIG_REQUIRED) {
+			if (!(iint->flags & IMA_VERITY_REQUIRED)) {
+				*cause = "IMA-signature-required";
+				*status = INTEGRITY_FAIL;
+				break;
+			}
+		}
+
+		sig = (typeof(sig))xattr_value;
+		if (sig->version != 3) {
+			*cause = "invalid-signature-version";
+			*status = INTEGRITY_FAIL;
+			break;
+		}
+
+		rc = calc_file_id_hash(IMA_VERITY_DIGSIG, iint->ima_hash->algo,
+				       iint->ima_hash->digest, &hash.hdr);
+		if (rc) {
+			*cause = "sigv3-hashing-error";
+			*status = INTEGRITY_FAIL;
+			break;
+		}
+
+		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,
+					     (const char *)xattr_value,
+					     xattr_len, hash.digest,
+					     hash.hdr.length);
+		if (rc) {
+			*cause = "invalid-verity-signature";
+			*status = INTEGRITY_FAIL;
+		} else {
+			*status = INTEGRITY_PASS;
+		}
+
 		break;
 	default:
 		*status = INTEGRITY_UNKNOWN;
@@ -431,8 +493,15 @@ int ima_appraise_measurement(enum ima_hooks func,
 		if (rc && rc != -ENODATA)
 			goto out;
 
-		cause = iint->flags & IMA_DIGSIG_REQUIRED ?
-				"IMA-signature-required" : "missing-hash";
+		if (iint->flags & IMA_DIGSIG_REQUIRED) {
+			if (iint->flags & IMA_VERITY_REQUIRED)
+				cause = "verity-signature-required";
+			else
+				cause = "IMA-signature-required";
+		} else {
+			cause = "missing-hash";
+		}
+
 		status = INTEGRITY_NOLABEL;
 		if (file->f_mode & FMODE_CREATED)
 			iint->flags |= IMA_NEW_FILE;
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 28aca1f9633b..d3006cc22ab1 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1311,6 +1311,12 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
 	    !(entry->flags & IMA_MODSIG_ALLOWED))
 		return false;
 
+	/* Ensure APPRAISE verity file implies a v3 signature */
+	if (entry->action == APPRAISE &&
+	    (entry->flags & IMA_VERITY_REQUIRED) &&
+	    !(entry->flags & IMA_DIGSIG_REQUIRED))
+		return false;
+
 	return true;
 }
 
@@ -1735,14 +1741,24 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			break;
 		case Opt_appraise_type:
 			ima_log_string(ab, "appraise_type", args[0].from);
-			if ((strcmp(args[0].from, "imasig")) == 0)
+			if ((strcmp(args[0].from, "imasig")) == 0) {
 				entry->flags |= IMA_DIGSIG_REQUIRED;
-			else if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
-				 strcmp(args[0].from, "imasig|modsig") == 0)
+			} else if (strcmp(args[0].from, "sigv3") == 0) {
+				/*
+				 * Only fsverity supports sigv3 for now.
+				 * No need to define a new flag.
+				 */
+				if (entry->flags & IMA_VERITY_REQUIRED)
+					entry->flags |= IMA_DIGSIG_REQUIRED;
+				else
+					result = -EINVAL;
+			} else if (IS_ENABLED(CONFIG_IMA_APPRAISE_MODSIG) &&
+				 strcmp(args[0].from, "imasig|modsig") == 0) {
 				entry->flags |= IMA_DIGSIG_REQUIRED |
 						IMA_MODSIG_ALLOWED;
-			else
+			} else {
 				result = -EINVAL;
+			}
 			break;
 		case Opt_appraise_flag:
 			ima_log_string(ab, "appraise_flag", args[0].from);
@@ -2186,6 +2202,8 @@ int ima_policy_show(struct seq_file *m, void *v)
 	if (entry->flags & IMA_DIGSIG_REQUIRED) {
 		if (entry->flags & IMA_MODSIG_ALLOWED)
 			seq_puts(m, "appraise_type=imasig|modsig ");
+		else if (entry->flags & IMA_VERITY_REQUIRED)
+			seq_puts(m, "appraise_type=sigv3 ");
 		else
 			seq_puts(m, "appraise_type=imasig ");
 	}
diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
index d370fca04de4..ecbe61c53d40 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -495,7 +495,8 @@ 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))
+	if (!xattr_value ||
+	    !(xattr_value->type & (EVM_IMA_XATTR_DIGSIG | IMA_VERITY_DIGSIG)))
 		return ima_eventevmsig_init(event_data, field_data);
 
 	return ima_write_template_field_data(xattr_value, event_data->xattr_len,
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index bd38bd451b19..b3267384c028 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -80,6 +80,7 @@ enum evm_ima_xattr_type {
 	EVM_IMA_XATTR_DIGSIG,
 	IMA_XATTR_DIGEST_NG,
 	EVM_XATTR_PORTABLE_DIGSIG,
+	IMA_VERITY_DIGSIG,
 	IMA_XATTR_LAST
 };
 
@@ -140,7 +141,9 @@ struct signature_v2_hdr {
 
 /*
  * IMA signature version 3 disambiguates the data that is signed, by
- * indirectly signing the hash of the ima_file_id structure data.
+ * indirectly signing the hash of the ima_file_id structure data,
+ * containing either the fsverity_descriptor struct digest or, in the
+ * future, the regular IMA file hash.
  *
  * (The hash of the ima_file_id structure is only of the portion used.)
  */
-- 
2.27.0

Re: [PATCH v5 7/8] ima: support fs-verity file digest based version 3 signatures

[Eric Biggers]

On Fri, Feb 11, 2022 at 04:43:09PM -0500, Mimi Zohar wrote:
> Instead of calculating a regular file hash and verifying the signature
> stored in the 'security.ima' xattr against the calculated file hash, get
> fs-verity's file digest and verify the signature (version 3) stored in
> 'security.ima' against the digest.
> 
> The policy rule 'appraise_type=' option is extended to support 'sigv3',
> which is initiality limited to fs-verity.
> 
> The fs-verity 'appraise' rules are identified by the 'digest-type=verity'
> option and require the 'appraise_type=sigv3' option.  The following
> 'appraise' policy rule requires fsverity file digests.  (The rule may be
> constrained, for example based on a fsuuid or LSM label.)
> 
> Basic fs-verity policy rule example:
>   appraise func=BPRM_CHECK digest_type=verity appraise_type=sigv3
> 
> Lastly, for IMA to differentiate between the original IMA signature
> from an fs-verity signature a new 'xattr_type' named IMA_VERITY_DIGSIG
> is defined.

I'm having a hard time understanding this patch.  Can you please describe the
motivation for doing things, not just the things themselves, and make sure the
explanation is understandable to someone who isn't an IMA expert?

> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index ff3c906738cb..508053b8dd0a 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -47,7 +47,7 @@ Description:
>  			fgroup:= decimal value
>  		  lsm:  are LSM specific
>  		  option:
> -			appraise_type:= [imasig] [imasig|modsig]
> +			appraise_type:= [imasig] | [imasig|modsig] | [sigv3]
>  			appraise_flag:= [check_blacklist]
>  			Currently, blacklist check is only for files signed with appended
>  			signature.
> @@ -153,9 +153,27 @@ Description:
>  
>  			appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512
>  
> -		Example of 'measure' rule requiring fs-verity's digests on a
> -		particular filesystem with indication of type of digest in
> -		the measurement list.
> +		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 \
> -				fsuuid=... template=ima-ngv2
> +				template=ima-ngv2
> +
> +		Example of 'measure' and 'appraise' rules requiring fs-verity
> +		signatures (version 3) stored in security.ima xattr.
> +
> +		The 'measure' rule specifies the 'ima-sig' template option,
> +		which includes the file signature in the measurement list.
> +
> +			measure func=BPRM_CHECK digest_type=verity \
> +				template=ima-sig
> +
> +		The 'appraise' rule specifies the type and signature version
> +		(sigv3) required.
> +
> +			appraise func=BPRM_CHECK digest_type=verity \
> +				appraise_type=sigv3
> +
> +		All of these policy rules could, for example, be constrained
> +		either based on a filesystem's UUID (fsuuid) or based on LSM
> +		labels.

Is there documentation for what the appraise_type argument means, or does it
just need to be reverse engineered from the above example?

> + - 'sig': the file signature, based on either the file's/fsverity's digest[1],
> +   or the EVM portable signature if the file signature is not found;

This sentence doesn't make sense.  How can it be the file signature if the
"file signature is not found"?

> @@ -303,6 +321,12 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
>  	case EVM_IMA_XATTR_DIGSIG:
>  		set_bit(IMA_DIGSIG, &iint->atomic_flags);
>  
> +		if (iint->flags & (IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED)) {
> +			*cause = "verity-signature-required";
> +			*status = INTEGRITY_FAIL;
> +			break;
> +		}

Shouldn't this check whether *both* of these flags are set?

> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 28aca1f9633b..d3006cc22ab1 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -1311,6 +1311,12 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
>  	    !(entry->flags & IMA_MODSIG_ALLOWED))
>  		return false;
>  
> +	/* Ensure APPRAISE verity file implies a v3 signature */
> +	if (entry->action == APPRAISE &&
> +	    (entry->flags & IMA_VERITY_REQUIRED) &&
> +	    !(entry->flags & IMA_DIGSIG_REQUIRED))
> +		return false;

This comment doesn't seem to match the code.

> diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> index d370fca04de4..ecbe61c53d40 100644
> --- a/security/integrity/ima/ima_template_lib.c
> +++ b/security/integrity/ima/ima_template_lib.c
> @@ -495,7 +495,8 @@ 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))
> +	if (!xattr_value ||
> +	    !(xattr_value->type & (EVM_IMA_XATTR_DIGSIG | IMA_VERITY_DIGSIG)))
>  		return ima_eventevmsig_init(event_data, field_data);

This is OR-ing together values that aren't bit flags.

- Eric

Re: [PATCH v5 7/8] ima: support fs-verity file digest based version 3 signatures

[Mimi Zohar]

On Wed, 2022-02-23 at 17:24 -0800, Eric Biggers wrote:
> On Fri, Feb 11, 2022 at 04:43:09PM -0500, Mimi Zohar wrote:
> > Instead of calculating a regular file hash and verifying the signature
> > stored in the 'security.ima' xattr against the calculated file hash, get
> > fs-verity's file digest and verify the signature (version 3) stored in
> > 'security.ima' against the digest.
> > 
> > The policy rule 'appraise_type=' option is extended to support 'sigv3',
> > which is initiality limited to fs-verity.
> > 
> > The fs-verity 'appraise' rules are identified by the 'digest-type=verity'
> > option and require the 'appraise_type=sigv3' option.  The following
> > 'appraise' policy rule requires fsverity file digests.  (The rule may be
> > constrained, for example based on a fsuuid or LSM label.)
> > 
> > Basic fs-verity policy rule example:
> >   appraise func=BPRM_CHECK digest_type=verity appraise_type=sigv3
> > 
> > Lastly, for IMA to differentiate between the original IMA signature
> > from an fs-verity signature a new 'xattr_type' named IMA_VERITY_DIGSIG
> > is defined.
> 
> I'm having a hard time understanding this patch.  Can you please describe the
> motivation for doing things, not just the things themselves, and make sure the
> explanation is understandable to someone who isn't an IMA expert?

Ok, will be updated in the next version.

> 
> > diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> > index ff3c906738cb..508053b8dd0a 100644
> > --- a/Documentation/ABI/testing/ima_policy
> > +++ b/Documentation/ABI/testing/ima_policy
> > @@ -47,7 +47,7 @@ Description:
> >  			fgroup:= decimal value
> >  		  lsm:  are LSM specific
> >  		  option:
> > -			appraise_type:= [imasig] [imasig|modsig]
> > +			appraise_type:= [imasig] | [imasig|modsig] | [sigv3]
> >  			appraise_flag:= [check_blacklist]
> >  			Currently, blacklist check is only for files signed with appended
> >  			signature.
> > @@ -153,9 +153,27 @@ Description:
> >  
> >  			appraise func=SETXATTR_CHECK appraise_algos=sha256,sha384,sha512
> >  
> > -		Example of 'measure' rule requiring fs-verity's digests on a
> > -		particular filesystem with indication of type of digest in
> > -		the measurement list.
> > +		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 \
> > -				fsuuid=... template=ima-ngv2
> > +				template=ima-ngv2
> > +
> > +		Example of 'measure' and 'appraise' rules requiring fs-verity
> > +		signatures (version 3) stored in security.ima xattr.
> > +
> > +		The 'measure' rule specifies the 'ima-sig' template option,
> > +		which includes the file signature in the measurement list.
> > +
> > +			measure func=BPRM_CHECK digest_type=verity \
> > +				template=ima-sig
> > +
> > +		The 'appraise' rule specifies the type and signature version
> > +		(sigv3) required.
> > +
> > +			appraise func=BPRM_CHECK digest_type=verity \
> > +				appraise_type=sigv3
> > +
> > +		All of these policy rules could, for example, be constrained
> > +		either based on a filesystem's UUID (fsuuid) or based on LSM
> > +		labels.
> 
> Is there documentation for what the appraise_type argument means, or does it
> just need to be reverse engineered from the above example?

The original definition was defined in Backus–Naur form.  Other
information was subsequently included which resulted in making it less
readable.  Perhaps for now, the explanation could be indented:

                        appraise_type:= [imasig] | [imasig|modsig] | [sigv3]
                            where 'imasig' is the original or v2 signature,
                            where 'modsig' is an appended signature,
                            where 'sigv3' is the IMA v3 signature.

> 
> > + - 'sig': the file signature, based on either the file's/fsverity's digest[1],
> > +   or the EVM portable signature if the file signature is not found;
> 
> This sentence doesn't make sense.  How can it be the file signature if the
> "file signature is not found"?

EVM protects file metadata including security xattrs.  In order to make
EVM signatures portable, the i_ino and i_generation, which are
filesystem specific, couldn't be included.  Instead EVM portable
signature requires including 'security.ima', which may be a file hash
or signature.  Requiring both EVM and IMA signatures was considered
unnecessary, maybe even redundant.  When 'security.ima' is a file hash,
the 'sig' field may contain the EVM signature.

I've updated to be:
 - 'sig': the file signature, based on either the file's/fsverity's
digest[1],
   or the EVM portable signature, if 'security.ima' contains a file
hash.

> 
> > @@ -303,6 +321,12 @@ static int xattr_verify(enum ima_hooks func, struct integrity_iint_cache *iint,
> >  	case EVM_IMA_XATTR_DIGSIG:
> >  		set_bit(IMA_DIGSIG, &iint->atomic_flags);
> >  
> > +		if (iint->flags & (IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED)) {
> > +			*cause = "verity-signature-required";
> > +			*status = INTEGRITY_FAIL;
> > +			break;
> > +		}
> 
> Shouldn't this check whether *both* of these flags are set?

Yes, thank you.

> 
> > diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> > index 28aca1f9633b..d3006cc22ab1 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -1311,6 +1311,12 @@ static bool ima_validate_rule(struct ima_rule_entry *entry)
> >  	    !(entry->flags & IMA_MODSIG_ALLOWED))
> >  		return false;
> >  
> > +	/* Ensure APPRAISE verity file implies a v3 signature */
> > +	if (entry->action == APPRAISE &&
> > +	    (entry->flags & IMA_VERITY_REQUIRED) &&
> > +	    !(entry->flags & IMA_DIGSIG_REQUIRED))
> > +		return false;
> 
> This comment doesn't seem to match the code.h

Agreed, the comment will be updated in the next version.

> > diff --git a/security/integrity/ima/ima_template_lib.c b/security/integrity/ima/ima_template_lib.c
> > index d370fca04de4..ecbe61c53d40 100644
> > --- a/security/integrity/ima/ima_template_lib.c
> > +++ b/security/integrity/ima/ima_template_lib.c
> > @@ -495,7 +495,8 @@ 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))
> > +	if (!xattr_value ||
> > +	    !(xattr_value->type & (EVM_IMA_XATTR_DIGSIG | IMA_VERITY_DIGSIG)))
> >  		return ima_eventevmsig_init(event_data, field_data);
> 
> This is OR-ing together values that aren't bit flags.

Updated.

thanks,

Mimi

[PATCH v5 8/8] fsverity: update the documentation

[Mimi Zohar]

Update the fsverity documentation related to IMA signature support.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 Documentation/filesystems/fsverity.rst | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/Documentation/filesystems/fsverity.rst b/Documentation/filesystems/fsverity.rst
index 1d831e3cbcb3..28a47488848e 100644
--- a/Documentation/filesystems/fsverity.rst
+++ b/Documentation/filesystems/fsverity.rst
@@ -74,8 +74,12 @@ authenticating the files is up to userspace.  However, to meet some
 users' needs, fs-verity optionally supports a simple signature
 verification mechanism where users can configure the kernel to require
 that all fs-verity files be signed by a key loaded into a keyring; see
-`Built-in signature verification`_.  Support for fs-verity file hashes
-in IMA (Integrity Measurement Architecture) policies is also planned.
+`Built-in signature verification`_.
+
+IMA supports including fs-verity file digests and signatures in the
+IMA (Integrity Measurement Architecture) measurement list and
+verifying fs-verity based file signatures stored as security.ima
+xattrs, based on policy.
 
 User API
 ========
@@ -653,13 +657,13 @@ weren't already directly answered in other parts of this document.
     hashed and what to do with those hashes, such as log them,
     authenticate them, or add them to a measurement list.
 
-    IMA is planned to support the fs-verity hashing mechanism as an
-    alternative to doing full file hashes, for people who want the
-    performance and security benefits of the Merkle tree based hash.
-    But it doesn't make sense to force all uses of fs-verity to be
-    through IMA.  As a standalone filesystem feature, fs-verity
-    already meets many users' needs, and it's testable like other
-    filesystem features e.g. with xfstests.
+    IMA supports the fs-verity hashing mechanism as an alternative
+    to doing full file hashes, for people who want the performance
+    and security benefits of the Merkle tree based hash.  But it
+    doesn't make sense to force all uses of fs-verity to be through
+    IMA.  As a standalone filesystem feature, fs-verity already meets
+    many users' needs, and it's testable like other filesystem
+    features e.g. with xfstests.
 
 :Q: Isn't fs-verity useless because the attacker can just modify the
     hashes in the Merkle tree, which is stored on-disk?
-- 
2.27.0