diff mbox series

[v6,4/5] ima: support fs-verity file digest based version 3 signatures

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

Commit Message

Mimi Zohar March 18, 2022, 6:21 p.m. UTC
IMA may verify a file's integrity against a "good" value stored in the
'security.ima' xattr or as an appended signature, based on policy.  When
the "good value" is stored in the xattr, the xattr may contain a file
hash or signature.  In either case, the "good" value is preceded by a
header.  The first byte of the xattr header indicates the type of data
- hash, signature - stored in the xattr.  To support storing fs-verity
signatures in the 'security.ima' xattr requires further differentiating
the fs-verity signature from the existing IMA signature.

In addition the signatures stored in 'security.ima' xattr, need to be
disambiguated.  Instead of directly signing the fs-verity digest, a new
signature version 3 is defined as the hash of the ima_file_id structure,
which identifies the type of signature and the digest.

The IMA policy defines "which" files are to be measured, verified, and/or
audited.  For those files being verified, the policy rules indicate "how"
the file should be verified.  For example to require a file be signed,
the appraise policy rule must include the 'appraise_type' option.

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

The policy rule must also indicate the type of signature, if not the IMA
default, by specifying the digest type:

	digest_type:= [verity]

The following policy rule requires fsverity signatures.  The rule may be
constrained, for example, based on a fsuuid or LSM label.

      appraise func=BPRM_CHECK digest_type=verity appraise_type=sigv3

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 Documentation/ABI/testing/ima_policy      |  27 ++++-
 Documentation/security/IMA-templates.rst  |   4 +-
 security/integrity/digsig.c               |   3 +-
 security/integrity/ima/ima_appraise.c     | 114 +++++++++++++++++++++-
 security/integrity/ima/ima_policy.c       |  30 +++++-
 security/integrity/ima/ima_template_lib.c |   4 +-
 security/integrity/integrity.h            |  23 ++++-
 7 files changed, 190 insertions(+), 15 deletions(-)

Comments

Stefan Berger March 21, 2022, 1:10 p.m. UTC | #1
On 3/18/22 14:21, Mimi Zohar wrote:
> IMA may verify a file's integrity against a "good" value stored in the
> 'security.ima' xattr or as an appended signature, based on policy.  When
> the "good value" is stored in the xattr, the xattr may contain a file
> hash or signature.  In either case, the "good" value is preceded by a
> header.  The first byte of the xattr header indicates the type of data
> - hash, signature - stored in the xattr.  To support storing fs-verity
> signatures in the 'security.ima' xattr requires further differentiating
> the fs-verity signature from the existing IMA signature.
> 
> In addition the signatures stored in 'security.ima' xattr, need to be
> disambiguated.  Instead of directly signing the fs-verity digest, a new
> signature version 3 is defined as the hash of the ima_file_id structure,
> which identifies the type of signature and the digest.

Would it not be enough to just differentiat by the type of signature 
rather than also bumping the version? It's still signature_v2_hdr but a 
new type IMA_VERITY_DIGSIG is introduced there that shoud be sufficient 
to indicate that a different method for calculating the hash is to be 
used than for anything that existed before? sigv3 would then become the 
more obvious veriftysig... ?
Mimi Zohar March 25, 2022, 12:31 p.m. UTC | #2
On Mon, 2022-03-21 at 09:10 -0400, Stefan Berger wrote:
> 
> On 3/18/22 14:21, Mimi Zohar wrote:
> > IMA may verify a file's integrity against a "good" value stored in the
> > 'security.ima' xattr or as an appended signature, based on policy.  When
> > the "good value" is stored in the xattr, the xattr may contain a file
> > hash or signature.  In either case, the "good" value is preceded by a
> > header.  The first byte of the xattr header indicates the type of data
> > - hash, signature - stored in the xattr.  To support storing fs-verity
> > signatures in the 'security.ima' xattr requires further differentiating
> > the fs-verity signature from the existing IMA signature.
> > 
> > In addition the signatures stored in 'security.ima' xattr, need to be
> > disambiguated.  Instead of directly signing the fs-verity digest, a new
> > signature version 3 is defined as the hash of the ima_file_id structure,
> > which identifies the type of signature and the digest.
> 
> Would it not be enough to just differentiat by the type of signature 
> rather than also bumping the version? It's still signature_v2_hdr but a 
> new type IMA_VERITY_DIGSIG is introduced there that shoud be sufficient 
> to indicate that a different method for calculating the hash is to be 
> used than for anything that existed before? sigv3 would then become the 
> more obvious veriftysig... ?

One of Eric's concerns was that, "an attacker (who controls the file's
contents and IMA xattr) [could] replace the file with one with a
differrent content and still be able to pass the IMA check."  His
solution was to only allow one signature version on a running system.  
For the complete description of the attack, refer to Eric's comments on
v3.

Instead of only allowing one signature version on a running system,
subsequent versions of this patch set addressed his concern, by
limiting the signature version based on policy.
Stefan Berger March 25, 2022, 1:49 p.m. UTC | #3
On 3/25/22 08:31, Mimi Zohar wrote:
> On Mon, 2022-03-21 at 09:10 -0400, Stefan Berger wrote:
>>
>> On 3/18/22 14:21, Mimi Zohar wrote:
>>> IMA may verify a file's integrity against a "good" value stored in the
>>> 'security.ima' xattr or as an appended signature, based on policy.  When
>>> the "good value" is stored in the xattr, the xattr may contain a file
>>> hash or signature.  In either case, the "good" value is preceded by a
>>> header.  The first byte of the xattr header indicates the type of data
>>> - hash, signature - stored in the xattr.  To support storing fs-verity
>>> signatures in the 'security.ima' xattr requires further differentiating
>>> the fs-verity signature from the existing IMA signature.
>>>
>>> In addition the signatures stored in 'security.ima' xattr, need to be
>>> disambiguated.  Instead of directly signing the fs-verity digest, a new
>>> signature version 3 is defined as the hash of the ima_file_id structure,
>>> which identifies the type of signature and the digest.
>>
>> Would it not be enough to just differentiat by the type of signature
>> rather than also bumping the version? It's still signature_v2_hdr but a
>> new type IMA_VERITY_DIGSIG is introduced there that shoud be sufficient
>> to indicate that a different method for calculating the hash is to be
>> used than for anything that existed before? sigv3 would then become the
>> more obvious veriftysig... ?
> 
> One of Eric's concerns was that, "an attacker (who controls the file's
> contents and IMA xattr) [could] replace the file with one with a

Reference: 
https://lore.kernel.org/linux-integrity/20220126000658.138345-1-zohar@linux.ibm.com/T/#m8929fa29fbdfc875dbf5f384a4c082d303d2040e

This seem to describe the root user. A restrictions of root's power 
maybe that root may not have access to the file signing key use on the 
local system... ?

> differrent content and still be able to pass the IMA check."  His

Is this a scenario of concern? : /usr/bin/foobar is signed by verity and 
there's a rule in the IMA policy that would appraise this file. Can root 
now remove /usr/bin/foobar and copy the regularly signed /usr/bin/bash 
to /usr/bin/foobar along with bash's security.ima and have it execute 
either since there's no appraise rule covering non-fsverity signatures 
or due to a rule that covers non-fsverity signatures?

Since the signature header of security.ima is not signed root could also 
just rewrite the header and modify the signature type (and also version) 
and circumvent appraisal rules specific to fsverity.

> solution was to only allow one signature version on a running system.
> For the complete description of the attack, refer to Eric's comments on
> v3.


I am trying to figure out a concrete scenario that one has to defend 
against what seems to be the power of the root user. A more concrete 
example may be helpful.

> 
> Instead of only allowing one signature version on a running system,
> subsequent versions of this patch set addressed his concern, by
> limiting the signature version based on policy.
>
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 2e0c501ce9c8..acd17183ad8c 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -47,7 +47,11 @@  Description:
 			fgroup:= decimal value
 		  lsm:  are LSM specific
 		  option:
-			appraise_type:= [imasig] [imasig|modsig]
+			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.
+
 			appraise_flag:= [check_blacklist]
 			Currently, blacklist check is only for files signed with appended
 			signature.
@@ -158,3 +162,24 @@  Description:
 
 			measure func=FILE_CHECK digest_type=verity \
 				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-sigv2' template option,
+		which includes the indication of type of digest and the file
+		signature in the measurement list.
+
+			measure func=BPRM_CHECK digest_type=verity \
+				template=ima-sigv2
+
+
+		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 2d4789dc7750..06cddcd510da 100644
--- a/Documentation/security/IMA-templates.rst
+++ b/Documentation/security/IMA-templates.rst
@@ -73,8 +73,8 @@  descriptors by adding their identifier to the format string
         where the digest type is either "ima" or "verity".
  - 'd-modsig': the digest of the event without the appended modsig;
  - 'n-ng': the name of the event, without size limitations;
- - 'sig': the file signature, or the EVM portable signature if the file
-   signature is not found;
+ - '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.
  - '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/digsig.c b/security/integrity/digsig.c
index 3b06a01bd0fd..56472bc8e44c 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 signature */
+	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..e76d88b309a2 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];
@@ -225,6 +232,40 @@  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.
+ *
+ * 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_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;
+	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,7 +277,10 @@  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;
+	int mask;
 
 	switch (xattr_value->type) {
 	case IMA_XATTR_DIGEST_NG:
@@ -246,7 +290,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;
 			}
@@ -274,6 +321,20 @@  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);
+
+		mask = IMA_DIGSIG_REQUIRED | IMA_VERITY_REQUIRED;
+		if ((iint->flags & mask) == mask) {
+			*cause = "verity-signature-required";
+			*status = INTEGRITY_FAIL;
+			break;
+		}
+
+		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,
@@ -296,6 +357,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;
@@ -396,8 +495,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 c6b0454b2e25..fbdea5c0cdc6 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -1311,6 +1311,16 @@  static bool ima_validate_rule(struct ima_rule_entry *entry)
 	    !(entry->flags & IMA_MODSIG_ALLOWED))
 		return false;
 
+	/*
+	 * To avoid 'appraise_type=sigv3' and 'digest_type=verity' ordering
+	 * requirements, ensure both have been specified for file verity
+	 * signatures.
+	 */
+	if (entry->action == APPRAISE &&
+	    (entry->flags & IMA_VERITY_REQUIRED) &&
+	    !(entry->flags & IMA_DIGSIG_REQUIRED))
+		return false;
+
 	return true;
 }
 
@@ -1735,14 +1745,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 +2206,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 0cff6658a4c2..80fd2d908315 100644
--- a/security/integrity/ima/ima_template_lib.c
+++ b/security/integrity/ima/ima_template_lib.c
@@ -530,7 +530,9 @@  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 &&
+	     xattr_value->type != 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 d42a01903f08..44418f0ec0ab 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -79,6 +79,7 @@  enum evm_ima_xattr_type {
 	EVM_IMA_XATTR_DIGSIG,
 	IMA_XATTR_DIGEST_NG,
 	EVM_XATTR_PORTABLE_DIGSIG,
+	IMA_VERITY_DIGSIG,
 	IMA_XATTR_LAST
 };
 
@@ -93,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;
@@ -122,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 */
@@ -133,6 +138,20 @@  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,
+ * 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.)
+ */
+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 */