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 +, David Howells wrote:
> Eric Biggers  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

[David Howells]

Eric Biggers  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

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

[Eric Biggers]

From: Eric Biggers 

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 
Cc: Paolo Valente 
Cc:  # v4.7+
Signed-off-by: Eric Biggers 
---
 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