Hello Mimi,

Thanks for your review, and for queuing the other patches in this series.

Mimi Zohar [off-list ref] writes:

quoted

On Wed, 2017-06-07 at 22:49 -0300, Thiago Jung Bauermann wrote:

quoted

This patch introduces the modsig keyword to the IMA policy syntax to
specify that a given hook should expect the file to have the IMA signature
appended to it.

Thank you, Thiago. Appended signatures seem to be working proper now
with multiple keys on the IMA keyring.

Great news!

quoted

The length of this patch description is a good indication that this
patch needs to be broken up for easier review. A few
comments/suggestions inline below.

Ok, I will try to break it up, and also patch 5 as you suggested.

quoted

quoted

diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index 06554c448dce..9190c9058f4f 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c

@@ -48,11 +48,10 @@ static bool init_keyring __initdata;
 #define restrict_link_to_ima restrict_link_by_builtin_trusted
 #endif

-int integrity_digsig_verify(const unsigned int id, const char *sig, int siglen,
-			    const char *digest, int digestlen)
+struct key *integrity_keyring_from_id(const unsigned int id)
 {
-	if (id >= INTEGRITY_KEYRING_MAX || siglen < 2)
-		return -EINVAL;
+	if (id >= INTEGRITY_KEYRING_MAX)
+		return ERR_PTR(-EINVAL);

When splitting up this patch, the addition of this new function could
be a separate patch. The patch description would explain the need for
a new function.

Ok, will do for v3.

quoted

quoted

@@ -229,10 +234,14 @@ int ima_appraise_measurement(enum ima_hooks func,
 		goto out;
 	}

-	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
-		if ((status == INTEGRITY_NOLABEL)
-		    || (status == INTEGRITY_NOXATTRS))
+	/* Appended signatures aren't protected by EVM. */
+	status = evm_verifyxattr(dentry, XATTR_NAME_IMA,
+				 xattr_value->type == IMA_MODSIG ?
+				 NULL : xattr_value, rc, iint);
+	if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN &&
+	    !(xattr_value->type == IMA_MODSIG &&
+	      (status == INTEGRITY_NOLABEL || status == INTEGRITY_NOXATTRS))) {

This was messy to begin with, and now it is even more messy. For
appended signatures, we're only interested in INTEGRITY_FAIL. Maybe
leave the existing "if" clause alone and define a new "if" clause.

Ok, is this what you had in mind?

@@ -229,8 +237,14 @@ int ima_appraise_measurement(enum ima_hooks func,
 		goto out;
 	}

-	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value, rc, iint);
-	if ((status != INTEGRITY_PASS) && (status != INTEGRITY_UNKNOWN)) {
+	/* Appended signatures aren't protected by EVM. */
+	status = evm_verifyxattr(dentry, XATTR_NAME_IMA,
+				 xattr_value->type == IMA_MODSIG ?
+				 NULL : xattr_value, rc, iint);

More about this code further below.

@@ -266,6 +280,44 @@ int ima_appraise_measurement(enum ima_hooks func,
 		}
 		status = INTEGRITY_PASS;
 		break;
+	case IMA_MODSIG:
+		rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value);
+		if (!rc) {
+			iint->flags |= IMA_DIGSIG;
+			status = INTEGRITY_PASS;
+			break;
+		}
+
+		/*
+		 * The appended signature failed verification. Let's try
+		 * reading a signature from the extended attribute instead.
+		 */
+
+		ima_free_xattr_data(xattr_value);
+		xattr_value = NULL;
+
+		/* read 'security.ima' */
+		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
+		algo = iint->ima_hash->algo;
+
+		if (!xattr_value || xattr_value->type == IMA_MODSIG ||
+		    ima_get_hash_algo(xattr_value, xattr_len) != algo) {
+			iint->flags |= IMA_DIGSIG;
+
+			if (rc == -EOPNOTSUPP)
+				status = INTEGRITY_UNKNOWN;
+			else {
+				cause = "invalid-signature";
+				status = INTEGRITY_FAIL;
+			}
+
+			break;
+		}
+
+		pr_debug("Trying the xattr signature\n");
+
+		return ima_appraise_measurement(func, iint, file, filename,
+						xattr_value, xattr_len, opened);
 	case EVM_IMA_XATTR_DIGSIG:
 		iint->flags |= IMA_DIGSIG;
 		rc = integrity_digsig_verify(INTEGRITY_KEYRING_IMA,

quoted

quoted

@@ -406,7 +424,8 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
 			return -EINVAL;
 		ima_reset_appraise_flags(d_backing_inode(dentry),
-			 (xvalue->type == EVM_IMA_XATTR_DIGSIG) ? 1 : 0);
+					 xvalue->type == EVM_IMA_XATTR_DIGSIG ||
+					 xvalue->type == IMA_MODSIG);

Probably easier to read if we set a variable, before calling
ima_reset_appraise_flags.

Ok, will do in v3.

quoted

quoted

@@ -226,30 +282,23 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 		goto out_digsig;
 	}

-	template_desc = ima_template_desc_current();
-	if ((action & IMA_APPRAISE_SUBMASK) ||
-		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
-		/* read 'security.ima' */
-		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
-
-	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
-
-	rc = ima_collect_measurement(iint, file, buf, size, hash_algo);
-	if (rc != 0) {
-		if (file->f_flags & O_DIRECT)
-			rc = (iint->flags & IMA_PERMIT_DIRECTIO) ? 0 : -EACCES;
-		goto out_digsig;
-	}
-

There are four stages: collect measurement, store measurement,
appraise measurement and audit measurement. "Collect" needs to be
done if any one of the other stages is needed.

quoted

 	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);

+	if (iint->flags & IMA_MODSIG_ALLOWED)
+		rc = measure_and_appraise(file, buf, size, func, opened, action,
+					  iint, &xattr_value, &xattr_len,
+					  pathname, true);
+	if (!xattr_len)
+		rc = measure_and_appraise(file, buf, size, func, opened, action,
+					  iint, &xattr_value, &xattr_len,
+					  pathname, false);

I would rather see "collect" extended to support an appended signature
rather than trying to combine "collect" and "appraise" together.

I'm not sure I understand what you mean by "extend collect to support an
appended signature", but the fundamental problem is that we don't know
whether we need to fallback to the xattr sig until the appraise step
because that's when we verify the signature, and by then the collect and
store steps were already completed and would need to be done again if
the hash algorithm in the xattr sig is different.

@@ -227,10 +230,16 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	}

 	template_desc = ima_template_desc_current();
-	if ((action & IMA_APPRAISE_SUBMASK) ||
-		    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0)
-		/* read 'security.ima' */
-		xattr_len = ima_read_xattr(file_dentry(file), &xattr_value);
+	if (action & IMA_APPRAISE_SUBMASK ||
+	    strcmp(template_desc->name, IMA_TEMPLATE_IMA_NAME) != 0) {
+		if (iint->flags & IMA_MODSIG_ALLOWED)
+			ima_read_modsig(buf, &size, &xattr_value, &xattr_len);
+
+		if (!xattr_len)
+			/* read 'security.ima' */
+			xattr_len = ima_read_xattr(file_dentry(file),
+						   &xattr_value);
+	}

 	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);

@@ -257,7 +266,7 @@ static int process_measurement(struct file *file, char *buf, loff_t size,
 	if ((mask & MAY_WRITE) && (iint->flags & IMA_DIGSIG) &&
 	     !(iint->flags & IMA_NEW_FILE))
 		rc = -EACCES;
-	kfree(xattr_value);
+	ima_free_xattr_data(xattr_value);
 out_free:
 	if (pathbuf)
 		__putname(pathbuf);

quoted

quoted

+	if (rc)
+		goto out_digsig;
+
 	if (action & IMA_MEASURE)
 		ima_store_measurement(iint, file, pathname,
 				      xattr_value, xattr_len, pcr);
-	if (action & IMA_APPRAISE_SUBMASK)
-		rc = ima_appraise_measurement(func, iint, file, pathname,
-					      xattr_value, xattr_len, opened);

Moving appraisal before storing the measurement, should be a separate
patch with a clear explanation as to why it is needed.

Ok, will do in v3 if you don't like the restriction of both the modsig
and xattr sig having to use the same hash algorithm.