[PATCH 0/9] PKCS#7 / X.509 fixes and cleanups

[PATCH 0/9] PKCS#7 / X.509 fixes and cleanups

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Hi David, here is another set of fixes and cleanups for the PKCS#7 and
X.509 code:

Patches 1-5 fix the BUG_ON() in public_key_verify_signature() reported
by Paolo Valente as well as several other bugs I found.  Notably both
the PKCS#7 certificate chain verification and certificate blacklisting
were broken.  With the certificate chaining bug (introduced in v4.7) it
is almost possible to bypass PKCS#7 signature verification completely,
though I couldn't quite make it work.

Patches 6-9 remove dead code related to the unsupported_key and
unsupported_sig flags on struct x509_certificate's.  It's possible these
flags should be fixed instead, so that we handle unrecognized or
unsupported algorithms more gracefully.  But evidently no one is
complaining about the current behavior, and removing the dead code is
strictly an improvement as it doesn't actually change the behavior...

Eric Biggers (9):
  PKCS#7: fix certificate chain verification
  PKCS#7: fix certificate blacklisting
  PKCS#7: fix direct verification of SignerInfo signature
  X.509: fix BUG_ON() when hash algorithm is unsupported
  X.509: fix NULL dereference when restricting key with unsupported_sig
  PKCS#7: remove unnecessary check for NULL sinfo->sig->hash_algo
  X.509: remove never-set ->unsupported_key flag
  X.509: remove dead code that set ->unsupported_sig
  X.509: self_signed implies !unsupported_sig

 crypto/asymmetric_keys/pkcs7_trust.c     |  1 +
 crypto/asymmetric_keys/pkcs7_verify.c    | 36 +++++++++-----------------------
 crypto/asymmetric_keys/public_key.c      |  4 +++-
 crypto/asymmetric_keys/restrict.c        | 21 ++++++++++++-------
 crypto/asymmetric_keys/x509_parser.h     |  3 +--
 crypto/asymmetric_keys/x509_public_key.c | 18 ----------------
 6 files changed, 28 insertions(+), 55 deletions(-)

-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 0/9] PKCS#7 / X.509 fixes and cleanups

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

Hi David, here is another set of fixes and cleanups for the PKCS#7 and
X.509 code:

Patches 1-5 fix the BUG_ON() in public_key_verify_signature() reported
by Paolo Valente as well as several other bugs I found.  Notably both
the PKCS#7 certificate chain verification and certificate blacklisting
were broken.  With the certificate chaining bug (introduced in v4.7) it
is almost possible to bypass PKCS#7 signature verification completely,
though I couldn't quite make it work.

Patches 6-9 remove dead code related to the unsupported_key and
unsupported_sig flags on struct x509_certificate's.  It's possible these
flags should be fixed instead, so that we handle unrecognized or
unsupported algorithms more gracefully.  But evidently no one is
complaining about the current behavior, and removing the dead code is
strictly an improvement as it doesn't actually change the behavior...

Eric Biggers (9):
  PKCS#7: fix certificate chain verification
  PKCS#7: fix certificate blacklisting
  PKCS#7: fix direct verification of SignerInfo signature
  X.509: fix BUG_ON() when hash algorithm is unsupported
  X.509: fix NULL dereference when restricting key with unsupported_sig
  PKCS#7: remove unnecessary check for NULL sinfo->sig->hash_algo
  X.509: remove never-set ->unsupported_key flag
  X.509: remove dead code that set ->unsupported_sig
  X.509: self_signed implies !unsupported_sig

 crypto/asymmetric_keys/pkcs7_trust.c     |  1 +
 crypto/asymmetric_keys/pkcs7_verify.c    | 36 +++++++++-----------------------
 crypto/asymmetric_keys/public_key.c      |  4 +++-
 crypto/asymmetric_keys/restrict.c        | 21 ++++++++++++-------
 crypto/asymmetric_keys/x509_parser.h     |  3 +--
 crypto/asymmetric_keys/x509_public_key.c | 18 ----------------
 6 files changed, 28 insertions(+), 55 deletions(-)

-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 1/9] PKCS#7: fix certificate chain verification

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

When pkcs7_verify_sig_chain() is building the certificate chain for a
SignerInfo using the certificates in the PKCS#7 message, it is passing
the wrong arguments to public_key_verify_signature().  Consequently,
when the next certificate is supposed to be used to verify the previous
certificate, the next certificate is actually used to verify itself.

An attacker can use this bug to create a bogus certificate chain that
has no cryptographic relationship between the beginning and end.

Fortunately I couldn't quite find a way to use this to bypass the
overall signature verification, though it comes very close.  Here's the
reasoning: due to the bug, every certificate in the chain beyond the
first actually has to be self-signed (where "self-signed" here refers to
the actual key and signature; an attacker might still manipulate the
certificate fields such that the self_signed flag doesn't actually get
set, and thus the chain doesn't end immediately).  But to pass trust
validation (pkcs7_validate_trust()), either the SignerInfo or one of the
certificates has to actually be signed by a trusted key.  Since only
self-signed certificates can be added to the chain, the only way for an
attacker to introduce a trusted signature is to include a self-signed
trusted certificate.

But, when pkcs7_validate_trust_one() reaches that certificate, instead
of trying to verify the signature on that certificate, it will actually
look up the corresponding trusted key, which will succeed, and then try
to verify the *previous* certificate, which will fail.  Thus, disaster
is narrowly averted (as far as I could tell).

Fixes: 6c2dc5ae4ab7 ("X.509: Extract signature digest and make self-signed cert checks earlier")
Cc: <stable@vger.kernel.org> # v4.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 39e6de0c2761..2f6a768b91d7 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -270,7 +270,7 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 				sinfo->index);
 			return 0;
 		}
-		ret = public_key_verify_signature(p->pub, p->sig);
+		ret = public_key_verify_signature(p->pub, x509->sig);
 		if (ret < 0)
 			return ret;
 		x509->signer = p;
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 1/9] PKCS#7: fix certificate chain verification

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

When pkcs7_verify_sig_chain() is building the certificate chain for a
SignerInfo using the certificates in the PKCS#7 message, it is passing
the wrong arguments to public_key_verify_signature().  Consequently,
when the next certificate is supposed to be used to verify the previous
certificate, the next certificate is actually used to verify itself.

An attacker can use this bug to create a bogus certificate chain that
has no cryptographic relationship between the beginning and end.

Fortunately I couldn't quite find a way to use this to bypass the
overall signature verification, though it comes very close.  Here's the
reasoning: due to the bug, every certificate in the chain beyond the
first actually has to be self-signed (where "self-signed" here refers to
the actual key and signature; an attacker might still manipulate the
certificate fields such that the self_signed flag doesn't actually get
set, and thus the chain doesn't end immediately).  But to pass trust
validation (pkcs7_validate_trust()), either the SignerInfo or one of the
certificates has to actually be signed by a trusted key.  Since only
self-signed certificates can be added to the chain, the only way for an
attacker to introduce a trusted signature is to include a self-signed
trusted certificate.

But, when pkcs7_validate_trust_one() reaches that certificate, instead
of trying to verify the signature on that certificate, it will actually
look up the corresponding trusted key, which will succeed, and then try
to verify the *previous* certificate, which will fail.  Thus, disaster
is narrowly averted (as far as I could tell).

Fixes: 6c2dc5ae4ab7 ("X.509: Extract signature digest and make self-signed cert checks earlier")
Cc: <stable@vger.kernel.org> # v4.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 39e6de0c2761..2f6a768b91d7 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -270,7 +270,7 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 				sinfo->index);
 			return 0;
 		}
-		ret = public_key_verify_signature(p->pub, p->sig);
+		ret = public_key_verify_signature(p->pub, x509->sig);
 		if (ret < 0)
 			return ret;
 		x509->signer = p;
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 2/9] PKCS#7: fix certificate blacklisting

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If there is a blacklisted certificate in a SignerInfo's certificate
chain, then pkcs7_verify_sig_chain() sets sinfo->blacklisted and returns
0.  But, pkcs7_verify() fails to handle this case appropriately, as it
actually continues on to the line 'actual_ret = 0;', indicating that the
SignerInfo has passed verification.  Consequently, PKCS#7 signature
verification ignores the certificate blacklist.

Fix this by not considering blacklisted SignerInfos to have passed
verification.

Also fix the function comment with regards to when 0 is returned.

Fixes: 03bb79315ddc ("PKCS#7: Handle blacklisted certificates")
Cc: <stable@vger.kernel.org> # v4.12+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 2f6a768b91d7..97c77f66b20d 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -366,8 +366,7 @@ static int pkcs7_verify_one(struct pkcs7_message *pkcs7,
  *
  *  (*) -EBADMSG if some part of the message was invalid, or:
  *
- *  (*) 0 if no signature chains were found to be blacklisted or to contain
- *	unsupported crypto, or:
+ *  (*) 0 if a signature chain passed verification, or:
  *
  *  (*) -EKEYREJECTED if a blacklisted key was encountered, or:
  *
@@ -423,8 +422,11 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 
 	for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) {
 		ret = pkcs7_verify_one(pkcs7, sinfo);
-		if (sinfo->blacklisted && actual_ret == -ENOPKG)
-			actual_ret = -EKEYREJECTED;
+		if (sinfo->blacklisted) {
+			if (actual_ret == -ENOPKG)
+				actual_ret = -EKEYREJECTED;
+			continue;
+		}
 		if (ret < 0) {
 			if (ret == -ENOPKG) {
 				sinfo->unsupported_crypto = true;
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 2/9] PKCS#7: fix certificate blacklisting

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If there is a blacklisted certificate in a SignerInfo's certificate
chain, then pkcs7_verify_sig_chain() sets sinfo->blacklisted and returns
0.  But, pkcs7_verify() fails to handle this case appropriately, as it
actually continues on to the line 'actual_ret = 0;', indicating that the
SignerInfo has passed verification.  Consequently, PKCS#7 signature
verification ignores the certificate blacklist.

Fix this by not considering blacklisted SignerInfos to have passed
verification.

Also fix the function comment with regards to when 0 is returned.

Fixes: 03bb79315ddc ("PKCS#7: Handle blacklisted certificates")
Cc: <stable@vger.kernel.org> # v4.12+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 2f6a768b91d7..97c77f66b20d 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -366,8 +366,7 @@ static int pkcs7_verify_one(struct pkcs7_message *pkcs7,
  *
  *  (*) -EBADMSG if some part of the message was invalid, or:
  *
- *  (*) 0 if no signature chains were found to be blacklisted or to contain
- *	unsupported crypto, or:
+ *  (*) 0 if a signature chain passed verification, or:
  *
  *  (*) -EKEYREJECTED if a blacklisted key was encountered, or:
  *
@@ -423,8 +422,11 @@ int pkcs7_verify(struct pkcs7_message *pkcs7,
 
 	for (sinfo = pkcs7->signed_infos; sinfo; sinfo = sinfo->next) {
 		ret = pkcs7_verify_one(pkcs7, sinfo);
-		if (sinfo->blacklisted && actual_ret = -ENOPKG)
-			actual_ret = -EKEYREJECTED;
+		if (sinfo->blacklisted) {
+			if (actual_ret = -ENOPKG)
+				actual_ret = -EKEYREJECTED;
+			continue;
+		}
 		if (ret < 0) {
 			if (ret = -ENOPKG) {
 				sinfo->unsupported_crypto = true;
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 3/9] PKCS#7: fix direct verification of SignerInfo signature

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If none of the certificates in a SignerInfo's certificate chain match a
trusted key, nor is the last certificate signed by a trusted key, then
pkcs7_validate_trust_one() tries to check whether the SignerInfo's
signature was made directly by a trusted key.  But, it actually fails to
set the 'sig' variable correctly, so it actually verifies the last
signature seen.  That will only be the SignerInfo's signature if the
certificate chain is empty; otherwise it will actually be the last
certificate's signature.

This is not by itself a security problem, since verifying any of the
certificates in the chain should be sufficient to verify the SignerInfo.
Still, it's not working as intended so it should be fixed.

Fix it by setting 'sig' correctly for the direct verification case.

Fixes: 757932e6da6d ("PKCS#7: Handle PKCS#7 messages that contain no X.509 certs")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_trust.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
index 1f4e25f10049..598906b1e28d 100644
--- a/crypto/asymmetric_keys/pkcs7_trust.c
+++ b/crypto/asymmetric_keys/pkcs7_trust.c
@@ -106,6 +106,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
 		pr_devel("sinfo %u: Direct signer is key %x\n",
 			 sinfo->index, key_serial(key));
 		x509 = NULL;
+		sig = sinfo->sig;
 		goto matched;
 	}
 	if (PTR_ERR(key) != -ENOKEY)
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 3/9] PKCS#7: fix direct verification of SignerInfo signature

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

If none of the certificates in a SignerInfo's certificate chain match a
trusted key, nor is the last certificate signed by a trusted key, then
pkcs7_validate_trust_one() tries to check whether the SignerInfo's
signature was made directly by a trusted key.  But, it actually fails to
set the 'sig' variable correctly, so it actually verifies the last
signature seen.  That will only be the SignerInfo's signature if the
certificate chain is empty; otherwise it will actually be the last
certificate's signature.

This is not by itself a security problem, since verifying any of the
certificates in the chain should be sufficient to verify the SignerInfo.
Still, it's not working as intended so it should be fixed.

Fix it by setting 'sig' correctly for the direct verification case.

Fixes: 757932e6da6d ("PKCS#7: Handle PKCS#7 messages that contain no X.509 certs")
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_trust.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/crypto/asymmetric_keys/pkcs7_trust.c b/crypto/asymmetric_keys/pkcs7_trust.c
index 1f4e25f10049..598906b1e28d 100644
--- a/crypto/asymmetric_keys/pkcs7_trust.c
+++ b/crypto/asymmetric_keys/pkcs7_trust.c
@@ -106,6 +106,7 @@ static int pkcs7_validate_trust_one(struct pkcs7_message *pkcs7,
 		pr_devel("sinfo %u: Direct signer is key %x\n",
 			 sinfo->index, key_serial(key));
 		x509 = NULL;
+		sig = sinfo->sig;
 		goto matched;
 	}
 	if (PTR_ERR(key) != -ENOKEY)
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 4/9] X.509: fix BUG_ON() when hash algorithm is unsupported

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The X.509 parser mishandles the case where the certificate's signature's
hash algorithm is not available in the crypto API.  In this case,
x509_get_sig_params() doesn't allocate the cert->sig->digest buffer;
this part seems to be intentional.  However,
public_key_verify_signature() is still called via
x509_check_for_self_signed(), which triggers the 'BUG_ON(!sig->digest)'.

Fix this by making public_key_verify_signature() return -ENOPKG if the
hash buffer has not been allocated.

Reproducer when all the CONFIG_CRYPTO_SHA512* options are disabled:

    openssl req -new -sha512 -x509 -batch -nodes -outform der \
        | keyctl padd asymmetric desc @s

Fixes: 6c2dc5ae4ab7 ("X.509: Extract signature digest and make self-signed cert checks earlier")
Reported-by: Paolo Valente <paolo.valente@linaro.org>
Cc: Paolo Valente <paolo.valente@linaro.org>
Cc: <stable@vger.kernel.org> # v4.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/public_key.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index de996586762a..e929fe1e4106 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -79,9 +79,11 @@ int public_key_verify_signature(const struct public_key *pkey,
 
 	BUG_ON(!pkey);
 	BUG_ON(!sig);
-	BUG_ON(!sig->digest);
 	BUG_ON(!sig->s);
 
+	if (!sig->digest)
+		return -ENOPKG;
+
 	alg_name = sig->pkey_algo;
 	if (strcmp(sig->pkey_algo, "rsa") == 0) {
 		/* The data wangled by the RSA algorithm is typically padded
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 4/9] X.509: fix BUG_ON() when hash algorithm is unsupported

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The X.509 parser mishandles the case where the certificate's signature's
hash algorithm is not available in the crypto API.  In this case,
x509_get_sig_params() doesn't allocate the cert->sig->digest buffer;
this part seems to be intentional.  However,
public_key_verify_signature() is still called via
x509_check_for_self_signed(), which triggers the 'BUG_ON(!sig->digest)'.

Fix this by making public_key_verify_signature() return -ENOPKG if the
hash buffer has not been allocated.

Reproducer when all the CONFIG_CRYPTO_SHA512* options are disabled:

    openssl req -new -sha512 -x509 -batch -nodes -outform der \
        | keyctl padd asymmetric desc @s

Fixes: 6c2dc5ae4ab7 ("X.509: Extract signature digest and make self-signed cert checks earlier")
Reported-by: Paolo Valente <paolo.valente@linaro.org>
Cc: Paolo Valente <paolo.valente@linaro.org>
Cc: <stable@vger.kernel.org> # v4.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/public_key.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/crypto/asymmetric_keys/public_key.c b/crypto/asymmetric_keys/public_key.c
index de996586762a..e929fe1e4106 100644
--- a/crypto/asymmetric_keys/public_key.c
+++ b/crypto/asymmetric_keys/public_key.c
@@ -79,9 +79,11 @@ int public_key_verify_signature(const struct public_key *pkey,
 
 	BUG_ON(!pkey);
 	BUG_ON(!sig);
-	BUG_ON(!sig->digest);
 	BUG_ON(!sig->s);
 
+	if (!sig->digest)
+		return -ENOPKG;
+
 	alg_name = sig->pkey_algo;
 	if (strcmp(sig->pkey_algo, "rsa") = 0) {
 		/* The data wangled by the RSA algorithm is typically padded
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 5/9] X.509: fix NULL dereference when restricting key with unsupported_sig

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The asymmetric key type allows an X.509 certificate to be added even if
its signature's hash algorithm is not available in the crypto API.  In
that case 'payload.data[asym_auth]' will be NULL.  But the key
restriction code failed to check for this case before trying to use the
signature, resulting in a NULL pointer dereference in
key_or_keyring_common() or in restrict_link_by_signature().

Fix this by returning -ENOPKG when the signature is unsupported.

Reproducer when all the CONFIG_CRYPTO_SHA512* options are disabled and
keyctl has support for the 'restrict_keyring' command:

    keyctl new_session
    keyctl restrict_keyring @s asymmetric builtin_trusted
    openssl req -new -sha512 -x509 -batch -nodes -outform der \
        | keyctl padd asymmetric desc @s

Fixes: a511e1af8b12 ("KEYS: Move the point of trust determination to __key_link()")
Cc: <stable@vger.kernel.org> # v4.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/restrict.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 86fb68508952..7c93c7728454 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -67,8 +67,9 @@ __setup("ca_keys=", ca_keys_setup);
  *
  * Returns 0 if the new certificate was accepted, -ENOKEY if we couldn't find a
  * matching parent certificate in the trusted list, -EKEYREJECTED if the
- * signature check fails or the key is blacklisted and some other error if
- * there is a matching certificate but the signature check cannot be performed.
+ * signature check fails or the key is blacklisted, -ENOPKG if the signature
+ * uses unsupported crypto, or some other error if there is a matching
+ * certificate but the signature check cannot be performed.
  */
 int restrict_link_by_signature(struct key *dest_keyring,
 			       const struct key_type *type,
@@ -88,6 +89,8 @@ int restrict_link_by_signature(struct key *dest_keyring,
 		return -EOPNOTSUPP;
 
 	sig = payload->data[asym_auth];
+	if (!sig)
+		return -ENOPKG;
 	if (!sig->auth_ids[0] && !sig->auth_ids[1])
 		return -ENOKEY;
 
@@ -139,6 +142,8 @@ static int key_or_keyring_common(struct key *dest_keyring,
 		return -EOPNOTSUPP;
 
 	sig = payload->data[asym_auth];
+	if (!sig)
+		return -ENOPKG;
 	if (!sig->auth_ids[0] && !sig->auth_ids[1])
 		return -ENOKEY;
 
@@ -222,9 +227,9 @@ static int key_or_keyring_common(struct key *dest_keyring,
  *
  * Returns 0 if the new certificate was accepted, -ENOKEY if we
  * couldn't find a matching parent certificate in the trusted list,
- * -EKEYREJECTED if the signature check fails, and some other error if
- * there is a matching certificate but the signature check cannot be
- * performed.
+ * -EKEYREJECTED if the signature check fails, -ENOPKG if the signature uses
+ * unsupported crypto, or some other error if there is a matching certificate
+ * but the signature check cannot be performed.
  */
 int restrict_link_by_key_or_keyring(struct key *dest_keyring,
 				    const struct key_type *type,
@@ -249,9 +254,9 @@ int restrict_link_by_key_or_keyring(struct key *dest_keyring,
  *
  * Returns 0 if the new certificate was accepted, -ENOKEY if we
  * couldn't find a matching parent certificate in the trusted list,
- * -EKEYREJECTED if the signature check fails, and some other error if
- * there is a matching certificate but the signature check cannot be
- * performed.
+ * -EKEYREJECTED if the signature check fails, -ENOPKG if the signature uses
+ * unsupported crypto, or some other error if there is a matching certificate
+ * but the signature check cannot be performed.
  */
 int restrict_link_by_key_or_keyring_chain(struct key *dest_keyring,
 					  const struct key_type *type,
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 5/9] X.509: fix NULL dereference when restricting key with unsupported_sig

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The asymmetric key type allows an X.509 certificate to be added even if
its signature's hash algorithm is not available in the crypto API.  In
that case 'payload.data[asym_auth]' will be NULL.  But the key
restriction code failed to check for this case before trying to use the
signature, resulting in a NULL pointer dereference in
key_or_keyring_common() or in restrict_link_by_signature().

Fix this by returning -ENOPKG when the signature is unsupported.

Reproducer when all the CONFIG_CRYPTO_SHA512* options are disabled and
keyctl has support for the 'restrict_keyring' command:

    keyctl new_session
    keyctl restrict_keyring @s asymmetric builtin_trusted
    openssl req -new -sha512 -x509 -batch -nodes -outform der \
        | keyctl padd asymmetric desc @s

Fixes: a511e1af8b12 ("KEYS: Move the point of trust determination to __key_link()")
Cc: <stable@vger.kernel.org> # v4.7+
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/restrict.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/crypto/asymmetric_keys/restrict.c b/crypto/asymmetric_keys/restrict.c
index 86fb68508952..7c93c7728454 100644
--- a/crypto/asymmetric_keys/restrict.c
+++ b/crypto/asymmetric_keys/restrict.c
@@ -67,8 +67,9 @@ __setup("ca_keys=", ca_keys_setup);
  *
  * Returns 0 if the new certificate was accepted, -ENOKEY if we couldn't find a
  * matching parent certificate in the trusted list, -EKEYREJECTED if the
- * signature check fails or the key is blacklisted and some other error if
- * there is a matching certificate but the signature check cannot be performed.
+ * signature check fails or the key is blacklisted, -ENOPKG if the signature
+ * uses unsupported crypto, or some other error if there is a matching
+ * certificate but the signature check cannot be performed.
  */
 int restrict_link_by_signature(struct key *dest_keyring,
 			       const struct key_type *type,
@@ -88,6 +89,8 @@ int restrict_link_by_signature(struct key *dest_keyring,
 		return -EOPNOTSUPP;
 
 	sig = payload->data[asym_auth];
+	if (!sig)
+		return -ENOPKG;
 	if (!sig->auth_ids[0] && !sig->auth_ids[1])
 		return -ENOKEY;
 
@@ -139,6 +142,8 @@ static int key_or_keyring_common(struct key *dest_keyring,
 		return -EOPNOTSUPP;
 
 	sig = payload->data[asym_auth];
+	if (!sig)
+		return -ENOPKG;
 	if (!sig->auth_ids[0] && !sig->auth_ids[1])
 		return -ENOKEY;
 
@@ -222,9 +227,9 @@ static int key_or_keyring_common(struct key *dest_keyring,
  *
  * Returns 0 if the new certificate was accepted, -ENOKEY if we
  * couldn't find a matching parent certificate in the trusted list,
- * -EKEYREJECTED if the signature check fails, and some other error if
- * there is a matching certificate but the signature check cannot be
- * performed.
+ * -EKEYREJECTED if the signature check fails, -ENOPKG if the signature uses
+ * unsupported crypto, or some other error if there is a matching certificate
+ * but the signature check cannot be performed.
  */
 int restrict_link_by_key_or_keyring(struct key *dest_keyring,
 				    const struct key_type *type,
@@ -249,9 +254,9 @@ int restrict_link_by_key_or_keyring(struct key *dest_keyring,
  *
  * Returns 0 if the new certificate was accepted, -ENOKEY if we
  * couldn't find a matching parent certificate in the trusted list,
- * -EKEYREJECTED if the signature check fails, and some other error if
- * there is a matching certificate but the signature check cannot be
- * performed.
+ * -EKEYREJECTED if the signature check fails, -ENOPKG if the signature uses
+ * unsupported crypto, or some other error if there is a matching certificate
+ * but the signature check cannot be performed.
  */
 int restrict_link_by_key_or_keyring_chain(struct key *dest_keyring,
 					  const struct key_type *type,
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 6/9] PKCS#7: remove unnecessary check for NULL sinfo->sig->hash_algo

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The PKCS#7 parser is guaranteed to set ->sig->hash_algo for every
SignerInfo, since pkcs7_sig_note_digest_algo() is a mandatory action in
the PKCS#7 ASN.1 grammar, and it returns an error code if an
unrecognized DigestAlgorithmIdentifier is given rather than leaving the
algorithm as NULL.  Therefore, remove the unnecessary NULL check.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 97c77f66b20d..a9e03f5c52e7 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -33,9 +33,6 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
 
 	kenter(",%u,%s", sinfo->index, sinfo->sig->hash_algo);
 
-	if (!sinfo->sig->hash_algo)
-		return -ENOPKG;
-
 	/* Allocate the hashing algorithm we're going to need and find out how
 	 * big the hash operational data will be.
 	 */
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 6/9] PKCS#7: remove unnecessary check for NULL sinfo->sig->hash_algo

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The PKCS#7 parser is guaranteed to set ->sig->hash_algo for every
SignerInfo, since pkcs7_sig_note_digest_algo() is a mandatory action in
the PKCS#7 ASN.1 grammar, and it returns an error code if an
unrecognized DigestAlgorithmIdentifier is given rather than leaving the
algorithm as NULL.  Therefore, remove the unnecessary NULL check.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index 97c77f66b20d..a9e03f5c52e7 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -33,9 +33,6 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
 
 	kenter(",%u,%s", sinfo->index, sinfo->sig->hash_algo);
 
-	if (!sinfo->sig->hash_algo)
-		return -ENOPKG;
-
 	/* Allocate the hashing algorithm we're going to need and find out how
 	 * big the hash operational data will be.
 	 */
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 7/9] X.509: remove never-set ->unsupported_key flag

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The X.509 parser is guaranteed to set cert->pub->pkey_algo, since
x509_extract_key_data() is a mandatory action in the X.509 ASN.1
grammar, and it returns an error code if an unrecognized
AlgorithmIdentifier is given rather than leaving the algorithm as NULL.

Therefore, remove the dead code which handled this algorithm being NULL.
This results in the ->unsupported_key flag never being set at all, so
remove that too.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c    | 3 ---
 crypto/asymmetric_keys/x509_parser.h     | 1 -
 crypto/asymmetric_keys/x509_public_key.c | 9 ---------
 3 files changed, 13 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index a9e03f5c52e7..beb47fd2fca5 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -196,9 +196,6 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 			return 0;
 		}
 
-		if (x509->unsupported_key)
-			goto unsupported_crypto_in_x509;
-
 		pr_debug("- issuer %s\n", x509->issuer);
 		sig = x509->sig;
 		if (sig->auth_ids[0])
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index e373e7483812..217341276ae0 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -40,7 +40,6 @@ struct x509_certificate {
 	bool		seen;			/* Infinite recursion prevention */
 	bool		verified;
 	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
-	bool		unsupported_key;	/* T if key uses unsupported crypto */
 	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
 	bool		blacklisted;
 };
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 9338b4558cdc..514007932ec9 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -34,9 +34,6 @@ int x509_get_sig_params(struct x509_certificate *cert)
 
 	pr_devel("==>%s()\n", __func__);
 
-	if (!cert->pub->pkey_algo)
-		cert->unsupported_key = true;
-
 	if (!sig->pkey_algo)
 		cert->unsupported_sig = true;
 
@@ -173,12 +170,6 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 
 	pr_devel("Cert Issuer: %s\n", cert->issuer);
 	pr_devel("Cert Subject: %s\n", cert->subject);
-
-	if (cert->unsupported_key) {
-		ret = -ENOPKG;
-		goto error_free_cert;
-	}
-
 	pr_devel("Cert Key Algo: %s\n", cert->pub->pkey_algo);
 	pr_devel("Cert Valid period: %lld-%lld\n", cert->valid_from, cert->valid_to);
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 7/9] X.509: remove never-set ->unsupported_key flag

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The X.509 parser is guaranteed to set cert->pub->pkey_algo, since
x509_extract_key_data() is a mandatory action in the X.509 ASN.1
grammar, and it returns an error code if an unrecognized
AlgorithmIdentifier is given rather than leaving the algorithm as NULL.

Therefore, remove the dead code which handled this algorithm being NULL.
This results in the ->unsupported_key flag never being set at all, so
remove that too.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c    | 3 ---
 crypto/asymmetric_keys/x509_parser.h     | 1 -
 crypto/asymmetric_keys/x509_public_key.c | 9 ---------
 3 files changed, 13 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index a9e03f5c52e7..beb47fd2fca5 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -196,9 +196,6 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 			return 0;
 		}
 
-		if (x509->unsupported_key)
-			goto unsupported_crypto_in_x509;
-
 		pr_debug("- issuer %s\n", x509->issuer);
 		sig = x509->sig;
 		if (sig->auth_ids[0])
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index e373e7483812..217341276ae0 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -40,7 +40,6 @@ struct x509_certificate {
 	bool		seen;			/* Infinite recursion prevention */
 	bool		verified;
 	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
-	bool		unsupported_key;	/* T if key uses unsupported crypto */
 	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
 	bool		blacklisted;
 };
diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 9338b4558cdc..514007932ec9 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -34,9 +34,6 @@ int x509_get_sig_params(struct x509_certificate *cert)
 
 	pr_devel("=>%s()\n", __func__);
 
-	if (!cert->pub->pkey_algo)
-		cert->unsupported_key = true;
-
 	if (!sig->pkey_algo)
 		cert->unsupported_sig = true;
 
@@ -173,12 +170,6 @@ static int x509_key_preparse(struct key_preparsed_payload *prep)
 
 	pr_devel("Cert Issuer: %s\n", cert->issuer);
 	pr_devel("Cert Subject: %s\n", cert->subject);
-
-	if (cert->unsupported_key) {
-		ret = -ENOPKG;
-		goto error_free_cert;
-	}
-
 	pr_devel("Cert Key Algo: %s\n", cert->pub->pkey_algo);
 	pr_devel("Cert Valid period: %lld-%lld\n", cert->valid_from, cert->valid_to);
 
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 8/9] X.509: remove dead code that set ->unsupported_sig

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The X.509 parser is guaranteed to set cert->sig->pkey_algo and
cert->sig->hash_algo, since x509_note_pkey_algo() is a mandatory action
in the X.509 ASN.1 grammar, and it returns an error code if an
unrecognized AlgorithmIdentifier is given rather than leaving the
algorithms as NULL.

Therefore, remove the dead code which handled these algorithm strings
being NULL.

Note that cert->unsupported_sig can still be set if the hash algorithm
cannot be allocated from the crypto API.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 514007932ec9..1a7c63003bc6 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -34,15 +34,6 @@ int x509_get_sig_params(struct x509_certificate *cert)
 
 	pr_devel("==>%s()\n", __func__);
 
-	if (!sig->pkey_algo)
-		cert->unsupported_sig = true;
-
-	/* We check the hash if we can - even if we can't then verify it */
-	if (!sig->hash_algo) {
-		cert->unsupported_sig = true;
-		return 0;
-	}
-
 	sig->s = kmemdup(cert->raw_sig, cert->raw_sig_size, GFP_KERNEL);
 	if (!sig->s)
 		return -ENOMEM;
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 8/9] X.509: remove dead code that set ->unsupported_sig

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The X.509 parser is guaranteed to set cert->sig->pkey_algo and
cert->sig->hash_algo, since x509_note_pkey_algo() is a mandatory action
in the X.509 ASN.1 grammar, and it returns an error code if an
unrecognized AlgorithmIdentifier is given rather than leaving the
algorithms as NULL.

Therefore, remove the dead code which handled these algorithm strings
being NULL.

Note that cert->unsupported_sig can still be set if the hash algorithm
cannot be allocated from the crypto API.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/x509_public_key.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/crypto/asymmetric_keys/x509_public_key.c b/crypto/asymmetric_keys/x509_public_key.c
index 514007932ec9..1a7c63003bc6 100644
--- a/crypto/asymmetric_keys/x509_public_key.c
+++ b/crypto/asymmetric_keys/x509_public_key.c
@@ -34,15 +34,6 @@ int x509_get_sig_params(struct x509_certificate *cert)
 
 	pr_devel("=>%s()\n", __func__);
 
-	if (!sig->pkey_algo)
-		cert->unsupported_sig = true;
-
-	/* We check the hash if we can - even if we can't then verify it */
-	if (!sig->hash_algo) {
-		cert->unsupported_sig = true;
-		return 0;
-	}
-
 	sig->s = kmemdup(cert->raw_sig, cert->raw_sig_size, GFP_KERNEL);
 	if (!sig->s)
 		return -ENOMEM;
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 9/9] X.509: self_signed implies !unsupported_sig

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The self_signed flag on a certificate implies we verified its signature.
Hence, the signature cannot have been unsupported.

Remove the dead code that resulted from this oversight.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c | 18 +++---------------
 crypto/asymmetric_keys/x509_parser.h  |  2 +-
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index beb47fd2fca5..c23255240b93 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -206,13 +206,10 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 				 sig->auth_ids[1]->len, sig->auth_ids[1]->data);
 
 		if (x509->self_signed) {
-			/* If there's no authority certificate specified, then
-			 * the certificate must be self-signed and is the root
-			 * of the chain.  Likewise if the cert is its own
-			 * authority.
+			/*
+			 * If the certificate is self-signed, then it is the
+			 * root of the chain.
 			 */
-			if (x509->unsupported_sig)
-				goto unsupported_crypto_in_x509;
 			x509->signer = x509;
 			pr_debug("- self-signed\n");
 			return 0;
@@ -275,15 +272,6 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 		x509 = p;
 		might_sleep();
 	}
-
-unsupported_crypto_in_x509:
-	/* Just prune the certificate chain at this point if we lack some
-	 * crypto module to go further.  Note, however, we don't want to set
-	 * sinfo->unsupported_crypto as the signed info block may still be
-	 * validatable against an X.509 cert lower in the chain that we have a
-	 * trusted copy of.
-	 */
-	return 0;
 }
 
 /*
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 217341276ae0..1294cc2c855d 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -39,7 +39,7 @@ struct x509_certificate {
 	unsigned	index;
 	bool		seen;			/* Infinite recursion prevention */
 	bool		verified;
-	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
+	bool		self_signed;		/* T if self-signed */
 	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
 	bool		blacklisted;
 };
-- 
2.16.0.rc1.238.g530d649a79-goog

[PATCH 9/9] X.509: self_signed implies !unsupported_sig

[Eric Biggers]

From: Eric Biggers <ebiggers@google.com>

The self_signed flag on a certificate implies we verified its signature.
Hence, the signature cannot have been unsupported.

Remove the dead code that resulted from this oversight.

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 crypto/asymmetric_keys/pkcs7_verify.c | 18 +++---------------
 crypto/asymmetric_keys/x509_parser.h  |  2 +-
 2 files changed, 4 insertions(+), 16 deletions(-)

diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index beb47fd2fca5..c23255240b93 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -206,13 +206,10 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 				 sig->auth_ids[1]->len, sig->auth_ids[1]->data);
 
 		if (x509->self_signed) {
-			/* If there's no authority certificate specified, then
-			 * the certificate must be self-signed and is the root
-			 * of the chain.  Likewise if the cert is its own
-			 * authority.
+			/*
+			 * If the certificate is self-signed, then it is the
+			 * root of the chain.
 			 */
-			if (x509->unsupported_sig)
-				goto unsupported_crypto_in_x509;
 			x509->signer = x509;
 			pr_debug("- self-signed\n");
 			return 0;
@@ -275,15 +272,6 @@ static int pkcs7_verify_sig_chain(struct pkcs7_message *pkcs7,
 		x509 = p;
 		might_sleep();
 	}
-
-unsupported_crypto_in_x509:
-	/* Just prune the certificate chain at this point if we lack some
-	 * crypto module to go further.  Note, however, we don't want to set
-	 * sinfo->unsupported_crypto as the signed info block may still be
-	 * validatable against an X.509 cert lower in the chain that we have a
-	 * trusted copy of.
-	 */
-	return 0;
 }
 
 /*
diff --git a/crypto/asymmetric_keys/x509_parser.h b/crypto/asymmetric_keys/x509_parser.h
index 217341276ae0..1294cc2c855d 100644
--- a/crypto/asymmetric_keys/x509_parser.h
+++ b/crypto/asymmetric_keys/x509_parser.h
@@ -39,7 +39,7 @@ struct x509_certificate {
 	unsigned	index;
 	bool		seen;			/* Infinite recursion prevention */
 	bool		verified;
-	bool		self_signed;		/* T if self-signed (check unsupported_sig too) */
+	bool		self_signed;		/* T if self-signed */
 	bool		unsupported_sig;	/* T if signature uses unsupported crypto */
 	bool		blacklisted;
 };
-- 
2.16.0.rc1.238.g530d649a79-goog

Re: [PATCH 0/9] PKCS#7 / X.509 fixes and cleanups

[David Howells]

I presume you don't have this in a git tree somewhere that I can pull?

David

Re: [PATCH 4/9] X.509: fix BUG_ON() when hash algorithm is unsupported

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> The X.509 parser mishandles the case where the certificate's signature's
> hash algorithm is not available in the crypto API.  In this case,
> x509_get_sig_params() doesn't allocate the cert->sig->digest buffer; this
> part seems to be intentional.

Well, yes, that would be intentional: we can't digest the digestibles without
access to a hash algorithm to do so and we can't allocate a digest buffer
without knowing how big it should be.

> Fix this by making public_key_verify_signature() return -ENOPKG if the
> hash buffer has not been allocated.

Hmmm...  I'm not sure that this is the right place to do this, since it
obscures a potential invalid argument within the kernel.

I'm more inclined that the users of X.509 certs should check
x509->unsupported_sig (pkcs7_verify_sig_chain() does this already partially).

David

Re: [PATCH 6/9] PKCS#7: remove unnecessary check for NULL sinfo->sig->hash_algo

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> The PKCS#7 parser is guaranteed to set ->sig->hash_algo for every
> SignerInfo, since pkcs7_sig_note_digest_algo() is a mandatory action in
> the PKCS#7 ASN.1 grammar, and it returns an error code if an
> unrecognized DigestAlgorithmIdentifier is given rather than leaving the
> algorithm as NULL.  Therefore, remove the unnecessary NULL check.

Actually, we might be better off deferring ENOPKG generation as we might have
multiple signatures, at least one of which does have a digest algorithm that
we can handle.

David

Re: [PATCH 8/9] X.509: remove dead code that set ->unsupported_sig

[David Howells]

Eric Biggers <ebiggers3@gmail.com> wrote:

> The X.509 parser is guaranteed to set cert->sig->pkey_algo and
> cert->sig->hash_algo, since x509_note_pkey_algo() is a mandatory action
> in the X.509 ASN.1 grammar, and it returns an error code if an
> unrecognized AlgorithmIdentifier is given rather than leaving the
> algorithms as NULL.

I'm leaning towards ENOPKG production here being deferred so that X.509 certs
that we can't verify can still be built into the kernel or loaded from
'trusted' sources.

Let me think about this a bit more.

David

Re: [PATCH 4/9] X.509: fix BUG_ON() when hash algorithm is unsupported

[Eric Biggers]

Hi David,

On Thu, Feb 08, 2018 at 03:07:30PM +0000, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > The X.509 parser mishandles the case where the certificate's signature's
> > hash algorithm is not available in the crypto API.  In this case,
> > x509_get_sig_params() doesn't allocate the cert->sig->digest buffer; this
> > part seems to be intentional.
> 
> Well, yes, that would be intentional: we can't digest the digestibles without
> access to a hash algorithm to do so and we can't allocate a digest buffer
> without knowing how big it should be.
> 
> > Fix this by making public_key_verify_signature() return -ENOPKG if the
> > hash buffer has not been allocated.
> 
> Hmmm...  I'm not sure that this is the right place to do this, since it
> obscures a potential invalid argument within the kernel.
> 
> I'm more inclined that the users of X.509 certs should check
> x509->unsupported_sig (pkcs7_verify_sig_chain() does this already partially).
> 

Well, either way using BUG_ON() is inappropriate for recoverable problems where
an error code can be returned.  In this case we can simply indicate that the
signature verification failed.  Note that unprivileged users can reach this
BUG_ON(), and it also appears to be reachable in at least 3 different ways...
So it really has to be fixed.

And considering that the ->unsupported_sig and ->unsupported_key flags are
almost completely broken anyway, it sounds like there would have to be a second
patchset to actually fix them.  So I'd rather you just took the more important
fixes in patches 1-5 now as-is, and then a patchset to make certificates with
unsupported algorithms be accepted in more cases can be done separately.

Thanks,

Eric

Re: [PATCH 4/9] X.509: fix BUG_ON() when hash algorithm is unsupported

[Eric Biggers]

Hi David,

On Thu, Feb 08, 2018 at 03:07:30PM +0000, David Howells wrote:
> Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> > The X.509 parser mishandles the case where the certificate's signature's
> > hash algorithm is not available in the crypto API.  In this case,
> > x509_get_sig_params() doesn't allocate the cert->sig->digest buffer; this
> > part seems to be intentional.
> 
> Well, yes, that would be intentional: we can't digest the digestibles without
> access to a hash algorithm to do so and we can't allocate a digest buffer
> without knowing how big it should be.
> 
> > Fix this by making public_key_verify_signature() return -ENOPKG if the
> > hash buffer has not been allocated.
> 
> Hmmm...  I'm not sure that this is the right place to do this, since it
> obscures a potential invalid argument within the kernel.
> 
> I'm more inclined that the users of X.509 certs should check
> x509->unsupported_sig (pkcs7_verify_sig_chain() does this already partially).
> 

Well, either way using BUG_ON() is inappropriate for recoverable problems where
an error code can be returned.  In this case we can simply indicate that the
signature verification failed.  Note that unprivileged users can reach this
BUG_ON(), and it also appears to be reachable in at least 3 different ways...
So it really has to be fixed.

And considering that the ->unsupported_sig and ->unsupported_key flags are
almost completely broken anyway, it sounds like there would have to be a second
patchset to actually fix them.  So I'd rather you just took the more important
fixes in patches 1-5 now as-is, and then a patchset to make certificates with
unsupported algorithms be accepted in more cases can be done separately.

Thanks,

Eric