Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1193543 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [GIT PULL] MODSIGN: Use PKCS#7 for module signatures David Howells <dhowells@redhat.com> - 2015-07-28 00:50 +0200
    Re: [GIT PULL] MODSIGN: Use PKCS#7 for module signatures Andy Lutomirski <luto@amacapital.net> - 2015-07-28 01:20 +0200
      Re: [GIT PULL] MODSIGN: Use PKCS#7 for module signatures David Howells <dhowells@redhat.com> - 2015-07-28 11:10 +0200
    Re: [GIT PULL] MODSIGN: Use PKCS#7 for module signatures David Woodhouse <dwmw2@infradead.org> - 2015-07-28 11:20 +0200
      Re: [GIT PULL] MODSIGN: Use PKCS#7 for module signatures David Howells <dhowells@redhat.com> - 2015-07-28 11:30 +0200

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

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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1193564

On Mon, Jul 27, 2015 at 3:43 PM, David Howells <dhowells@redhat.com> wrote:
> 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?
>

Seems sensible.

How would it cause kexec problems?  I can only see it being a problem
if Authenticode can't handle authattrs, right?  There shouldn't be any
legacy PKCS7 kexec images whatsoever, because no existing kernel will
boot them or generate them.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1193913

Andy Lutomirski <luto@amacapital.net> wrote:

> How would it cause kexec problems?  I can only see it being a problem
> if Authenticode can't handle authattrs, right?  There shouldn't be any
> legacy PKCS7 kexec images whatsoever, because no existing kernel will
> boot them or generate them.

I was wondering if it is possible to get an mscode message that doesn't have
authattrs in it.  I guess we can just ignore the possibility until the matter
arises, if it does, and deal with it then as appropriate.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1193921

[Multipart message — attachments visible in raw view] — view raw

On Mon, 2015-07-27 at 23:43 +0100, David Howells wrote:
> 
>     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.

There doesn't seem to be a lot of point in this part. If the
authenticated attribute isn't being *checked*, then the attacker
doesn't need to drop it at all. There's no point in merely requiring
its *existence*.

As part of the firmware signatures, if we are asked to check the
filename then yes we should require it to be present *and* match. But
if we aren't checking (which we can't for modules since we don't know
what's being loaded), why require it to be present at all?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[toc] | [prev] | [next] | [standalone]

#1193927

David Woodhouse <dwmw2@infradead.org> wrote:

> As part of the firmware signatures, if we are asked to check the
> filename then yes we should require it to be present *and* match. But
> if we aren't checking (which we can't for modules since we don't know
> what's being loaded), why require it to be present at all?

For firmware, that's in the next set of patches, at the tag
fwsign-pkcs7-20150720.

For modules we could require it not to be present since, as you say, there's
no way generally for the kernel check the module name requested.  The only
thing it could really do is to extract the expected name from the PKCS#7 and
compare it against the name in the modinfo structure *after* checking the
signature.  This would require passing the module name to sign-file too.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web