| Message ID | 20180207011012.5928-5-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
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
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
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