Mimi Zohar [off-list ref] writes:

On Thu, 2017-07-06 at 19:17 -0300, Thiago Jung Bauermann wrote:

quoted

--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c

@@ -200,18 +200,40 @@ int ima_read_xattr(struct dentry *dentry,
  */
 int ima_appraise_measurement(enum ima_hooks func,
 			     struct integrity_iint_cache *iint,
-			     struct file *file, const unsigned char *filename,
-			     struct evm_ima_xattr_data *xattr_value,
-			     int xattr_len, int opened)
+			     struct file *file, const void *buf, loff_t size,
+			     const unsigned char *filename,
+			     struct evm_ima_xattr_data **xattr_value_,
+			     int *xattr_len_, int opened)
 {
 	static const char op[] = "appraise_data";
 	char *cause = "unknown";
 	struct dentry *dentry = file_dentry(file);
 	struct inode *inode = d_backing_inode(dentry);
 	enum integrity_status status = INTEGRITY_UNKNOWN;
-	int rc = xattr_len, hash_start = 0;
+	struct evm_ima_xattr_data *xattr_value = *xattr_value_;
+	int xattr_len = *xattr_len_, rc = xattr_len, hash_start = 0;
+	bool appraising_modsig = false;
+	void *xattr_value_evm;
+	size_t xattr_len_evm;
+
+	if (iint->flags & IMA_MODSIG_ALLOWED) {
+		/*
+		 * Not supposed to happen. Hooks that support modsig are
+		 * whitelisted when parsing the policy using
+		 * ima_hooks_supports_modsig.
+		 */
+		if (!buf || !size)
+			WARN_ONCE(true, "%s doesn't support modsig\n",
+				  func_tokens[func]);

ima _appraise_measurement() is getting kind of long. Is there any
reason we can't move this comment and test to ima_read_modsig()?

I didn't do that because then I would need to pass func as an argument
to ima_read_modsig just to print the warning above. But it does simplify
the code so it may be worth it. I'll make that change in v4.

quoted

@@ -229,8 +251,24 @@ 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 but we still call
+	 * evm_verifyxattr to check other security xattrs, if they exist.
+	 */
+	if (appraising_modsig) {
+		xattr_value_evm = NULL;
+		xattr_len_evm = 0;
+	} else {
+		xattr_value_evm = xattr_value;
+		xattr_len_evm = xattr_len;
+	}
+
+	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
+				 xattr_len_evm, iint);
+	if (appraising_modsig && status == INTEGRITY_FAIL) {
+		cause = "invalid-HMAC";
+		goto out;

"modsig" is special, because having any security xattrs is not
required. This test doesn't prevent status from being set to
"missing-HMAC". This test is redundant with the original tests below.

Indeed, that is wrong. I'm still a bit fuzzy about how EVM works and how
it interacts with IMA. The only way I can think of singling out modsig
without reintroduced the complex expression you didn't like in v2 is as
below. What do you think?

@@ -229,8 +241,25 @@ 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 but we still call
+	 * evm_verifyxattr to check other security xattrs, if they exist.
+	 */
+	if (appraising_modsig) {
+		xattr_value_evm = NULL;
+		xattr_len_evm = 0;
+	} else {
+		xattr_value_evm = xattr_value;
+		xattr_len_evm = xattr_len;
+	}
+
+	status = evm_verifyxattr(dentry, XATTR_NAME_IMA, xattr_value_evm,
+				 xattr_len_evm, iint);
+	if (appraising_modsig && (status == INTEGRITY_NOLABEL
+				  || status == INTEGRITY_NOXATTRS))
+		/* It's ok if there's no xattr in the case of modsig. */
+		;
+	else if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
 		if ((status == INTEGRITY_NOLABEL)
 		    || (status == INTEGRITY_NOXATTRS))
 			cause = "missing-HMAC";

quoted

+	} else if (status != INTEGRITY_PASS && status != INTEGRITY_UNKNOWN) {
 		if ((status == INTEGRITY_NOLABEL)
 		    || (status == INTEGRITY_NOXATTRS))
 			cause = "missing-HMAC";

@@ -281,6 +319,43 @@ int ima_appraise_measurement(enum ima_hooks func,
 			status = INTEGRITY_PASS;
 		}

Calling evm_verifyxattr() with the IMA xattr value prevents EVM from
having to re-read the IMA xattr, but isn't necessary.On modsig
signature verification failure, calling evm_verifyxattr() a second
time isn't necessary.

So even for the IMA xattr sig case, the evm_verifyxattr call in
ima_appraise_measurement is an optimization and can be skipped?

quoted

+	case IMA_MODSIG:
+		/*
+		 * To avoid being tricked into recursion, we don't allow a
+		 * modsig stored in the xattr.
+		 */
+		if (!appraising_modsig) {
+			status = INTEGRITY_UNKNOWN;
+			cause = "unknown-ima-data";
+
+			break;
+		}
+
+		rc = ima_modsig_verify(INTEGRITY_KEYRING_IMA, xattr_value);
+		if (!rc) {
+			iint->flags |= IMA_DIGSIG;
+			status = 
+
+			kfree(*xattr_value_);
+			*xattr_value_ = xattr_value;
+			*xattr_len_ = xattr_len;
+
+			break;
+		}

When including the appended signature in the measurement list, the
corresponding file hash needs to be included in the measurement list,
which might be different than the previously calculated file hash
based on the hash algorithm as defined in the IMA xattr.

Including the file hash and signature in the measurement list allows
the attestation server, with just a public key, to verify the file
signature against the file hash. No need for a white list.

ima_modsig_verify() must calculate the file hash in order to verify
the file signature. This file hash value somehow needs to be returned
in order for it to be included in the measurement list.

In that case, patch 6/7 "ima: Store measurement after appraisal" isn't
enough and we have to go back to v2's change in ima_main.c which ties
together the collect and appraise steps in process_measurement (In that
version I called the function measure_and_appraise but it should be
called collect_and_appraise instead). That is because if the modsig
verification fails, the hash needs to be recalculated for the xattr
signature verification.

Either that, or I add another call to ima_collect_measurement inside
ima_appraise_measurement if the modsig verification fails. Which do you
prefer?

quoted

+		/*
+		 * The appended signature failed verification. Let's try
+		 * reading a signature from the extended attribute instead.
+		 */
+
+		pr_debug("modsig didn't verify, trying the xattr signature\n");
+
+		ima_free_xattr_data(xattr_value);
+		iint->flags &= ~IMA_MODSIG_ALLOWED;
+
+		return ima_appraise_measurement(func, iint, file, buf, size,
+						filename, xattr_value_,
+						xattr_len_, opened);

Most of the code before "switch" needs to be done only once. Is
recursion necessary? Or can we just retry the "switch" using the IMA
xattr, assuming there is an IMA xattr?

I used recursion to avoid duplicating two blocks of code: the logic in
the "if (rc <= 0)" block which needs to be done again when verifying the
xattr sig, and also the logic of interpreting the return value of
evm_verifyxattr which I also thought needed to be done again, but you
seem to be saying that is just an optimization and can be skipped.

-- 
Thiago Jung Bauermann
IBM Linux Technology Center