Re: [GIT PULL] MODSIGN: Use PKCS#7 for module signatures

[David Howells <dhowells@redhat.com>]

Andy Lutomirski <luto@kernel.org> wrote:

> With all this stuff applied, will the kernel accept PKCS#7 signatures that
> *don't* have authenticated attributes or that are otherwise cryptographically
> insecure in that they fail to provide the property that an attacker can't
> manipulate a valid signature on one message to look like a valid signature on
> a different message?

Hmmm...  That's easy enough to fix (see below).  However, will that cause
kexec problems, I wonder?  Does mscode require authattrs?

David
---
commit 44460686dfb0a4cca06f20e27988965e327e0f93
Author: David Howells <dhowells@redhat.com>
Date:   Mon Jul 27 23:32:03 2015 +0100

    PKCS#7: Require authenticated attributes
    
    Require there to be authenticated attributes in the PKCS#7/CMS message so
    that an attacker can't drop them to provide greater opportunity for
    manipulating the message.
    
    Suggested-by: Andy Lutomirski <luto@kernel.org>
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 404f89a0f852..be0fc3b49b43 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -30,6 +30,7 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
 	size_t digest_size, desc_size;
 	void *digest;
 	int ret;
+	u8 tag;
 
 	kenter(",%u,%u", sinfo->index, sinfo->sig.pkey_hash_algo);
 
@@ -70,43 +71,45 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
 	 * message digest attribute amongst them which corresponds to the
 	 * digest we just calculated.
 	 */
-	if (sinfo->msgdigest) {
-		u8 tag;
-
-		if (sinfo->msgdigest_len != sinfo->sig.digest_size) {
-			pr_debug("Sig %u: Invalid digest size (%u)\n",
-				 sinfo->index, sinfo->msgdigest_len);
-			ret = -EBADMSG;
-			goto error;
-		}
+	if (!sinfo->authattrs || !sinfo->msgdigest) {
+		pr_warn("Sig %u: No authenticatedAttrs\n", sinfo->index);
+		ret = -EKEYREJECTED;
+		goto error;
+	}
+	
+	if (sinfo->msgdigest_len != sinfo->sig.digest_size) {
+		pr_debug("Sig %u: Invalid digest size (%u)\n",
+			 sinfo->index, sinfo->msgdigest_len);
+		ret = -EBADMSG;
+		goto error;
+	}
 
-		if (memcmp(digest, sinfo->msgdigest, sinfo->msgdigest_len) != 0) {
-			pr_debug("Sig %u: Message digest doesn't match\n",
-				 sinfo->index);
-			ret = -EKEYREJECTED;
-			goto error;
-		}
+	if (memcmp(digest, sinfo->msgdigest, sinfo->msgdigest_len) != 0) {
+		pr_debug("Sig %u: Message digest doesn't match\n",
+			 sinfo->index);
+		ret = -EKEYREJECTED;
+		goto error;
+	}
 
-		/* We then calculate anew, using the authenticated attributes
-		 * as the contents of the digest instead.  Note that we need to
-		 * convert the attributes from a CONT.0 into a SET before we
-		 * hash it.
-		 */
-		memset(digest, 0, sinfo->sig.digest_size);
+	/* We then calculate anew, using the authenticated attributes
+	 * as the contents of the digest instead.  Note that we need to
+	 * convert the attributes from a CONT.0 into a SET before we
+	 * hash it.
+	 */
+	memset(digest, 0, sinfo->sig.digest_size);
 
-		ret = crypto_shash_init(desc);
-		if (ret < 0)
-			goto error;
-		tag = ASN1_CONS_BIT | ASN1_SET;
-		ret = crypto_shash_update(desc, &tag, 1);
-		if (ret < 0)
-			goto error;
-		ret = crypto_shash_finup(desc, sinfo->authattrs,
-					 sinfo->authattrs_len, digest);
-		if (ret < 0)
-			goto error;
-		pr_devel("AADigest = [%*ph]\n", 8, digest);
-	}
+	ret = crypto_shash_init(desc);
+	if (ret < 0)
+		goto error;
+	tag = ASN1_CONS_BIT | ASN1_SET;
+	ret = crypto_shash_update(desc, &tag, 1);
+	if (ret < 0)
+		goto error;
+	ret = crypto_shash_finup(desc, sinfo->authattrs,
+				 sinfo->authattrs_len, digest);
+	if (ret < 0)
+		goto error;
+	pr_devel("AADigest = [%*ph]\n", 8, digest);
 
 	sinfo->sig.digest = digest;
 	digest = NULL;