671097 - CERT_ExtractPublicKey, SECKEY_ExtractPublicKey, SECKEY_UpdateCertPQG, etc. modify certificates, are not thread safe, not PKIX-enabled

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

SECKEY_ExtractPublicKey modifies the certificates that are passed in when those certs are (EC?)DSA certs. Because of this, it isn't thread safe if multiple threads are trying to extract the public key of the same certificate.

Also, it isn't compatible with PKIX processing because it uses CERT_FindCertIssuer. In theory, this, combined with the above threading problems, could cause the resulting public key to have inconsistent and possible unsafe (cryptographically useless) PQG parameters. For this reason, I marked this bug core-security.

I propose the folllowing:

* Create a new structure CERTValidatedCertChain.

* Modify CERT_PKIXVerifyCert to optionally construct and return a CERTValidatedCertChain to the caller.

* Create a new function to extract the public key from a validated cert chain:

      SECKEYPublicKey * CERT_ExtractPublicKeyValidated(
              const CERTValidatedCertChain * certChain);

* Replace all internal-to-NSS uses of CERT_ExtractPublicKey with CERT_ExtractPublicKeyValidated.

* Rewrite CERT_ExtractPublicKey in terms of CERT_ExtractPublicKeyValidated.

* Deprecate CERT_ExtractPublicKey.

* In Gecko, switch all callers (if any) to CERT_ExtractPublicKeyValidated.

Note that the latest NIST test suite for PKIX still tests for PQG parameter inheritance [1], so I am assuming that we cannot just disable the PQG inheritance support. However, I noticed some products (in particular, WebSphere [1]) do not support this feature.

[1] http://csrc.nist.gov/groups/ST/crypto_apps_infra/documents/PKITS.pdf
[2] http://publib.boulder.ibm.com/infocenter/wmqv7/v7r0/index.jsp?topic=/com.ibm.mq.csqzas.doc/sy12820_.htm

Comment 1

[Wan-Teh Chang]

Isn't a validated certificate chain just a list of certificates?  NSS already has
two certificate list types (CERTCertList and CERTCertificateList).

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to comment #1)
> Isn't a validated certificate chain just a list of certificates?  NSS
> already has
> two certificate list types (CERTCertList and CERTCertificateList).

Anybody can construct a CERTCertList or a CERTCertificateList. By creating an opaque type that wraps one of those, that only certhigh can construct, we ensure at compile time that nobody is using the key from a validated cert when they are using the non-deprecated APIs.

Also, I would like to store the validated certificate usage in the CERTValidatedCertChain, so that we can have a signature like this:

    SECKEYPublicKey * CERT_ExtractPublicKeyValidated(
              const CERTValidatedCertChain * certChain,
              SECCertificateUsage intendedUsages);

This way, the API can automatically enforce proper validation before usage.

For example, in Gecko we have a bug where we construct and use a certificate chain (from the HTTP cache) without validating it first. I am hoping to prevent such bugs by using only CERTValidatedCertChain in Gecko. And also, the use of CERTValidatedCertChain (vs. CERTCertificate directly) will make it clear that the code has been modified to use libpkix instead of the old certificate path validation.

Comment 3

[Daniel Veditz [:dveditz]]

What makes this a bug that needs to be hidden?

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:bsmith) from comment #0)
> * Create a new function to extract the public key from a validated cert
> chain:
> 
>       SECKEYPublicKey * CERT_ExtractPublicKeyValidated(
>               const CERTValidatedCertChain * certChain);
> 
> * Replace all internal-to-NSS uses of CERT_ExtractPublicKey with
> CERT_ExtractPublicKeyValidated.

This would not work for libssl:

1. libssl doesn't ever know about the cert chain.

2. In async cert validation mode, libssl doesn't wait for the certificate to be validated before using the public key of the cert, as a performance optimization. But the key cannot be extracted until after validation, for any certs that are using the PQG inheritance feature.

Maybe we should provide an option for the application to disable libssl's public key extraction in favor of the application doing the extraction itself (during certificate validation). One way of doing this would be to add a new ephemeral key validation callback. The callback would be given the signature to verify, and it would return SECSuccess if the signature is valid, SECFailure if the signature fails to verify, or SECWouldBlock to do the signature verification asynchronously. The callback would basically just call the CERT_ExtractPublicKeyValidated function, and then return the result of the signature verification operation.

Comment 5

[Robert Relyea]

So ideally we would fill the in PQG parameters in chain building in libpkix, once we have a good chain from root to leaf. In practice the only time we have a problem is someone introduced cooked certs to create a DOS attack (that is you could add cooked certs to force a chain to always fail).

The race itself could be handled pretty easily:

1) lock before checking to see if the pqg paramters are empty. If they are not empty, unlock and return them.

2) unlock.

3) make the recursive call.

4) lock. Make sure the pqg parameters are still empty. If they are empty update them, 

5) unlock.

6) return the extracted pqg parameters.


This is really the simplest way to handle things. 

We could also update the pqg parameters in libpkix chain building as we walk the chain from root to leave. Same lock scheme (lock before updating the parameters).

The down side is that if we call the extract interface before we call any libpkix chain building we could hit a case where someone had bad certs and corrupted our PQG chain. NOTE: in real chains, it doesn't matter which chain you get, the PQG parameters have to match (The key was issued by one set of PQG parameters and can't arbitrarily be used with another, so all chains should produce the same PQG). Anyway I don't see this DOS to be a big issue. The old API is vunerable to a similiar case for RSA (where you could add your own crafted root cert and disrupt the chain of another cert). If you implement the above, apps can completely avoid this issue by simply never extracting the public key from an unvalidated certificate. We could enforce this by creating ing CERT_ExtractPublickeyAlreadyValidate() with simply returns the key in the leaf cert, and if the PQG params are empty returns NULL.

I would be very much against anything that required the application to deal with things differently (or even care that it was using DSA to begin with).

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Since Firefox 31, Firefox and Thunderbird have rejected any certificate that relies on DSA/ECDSA parameter inheritance, because mozilla::pkix does not allow DSA parameter inheritance. In particular, mozilla::pkix uses SECKEY_ExtractPublicKey instead of CERT_ExtractPublicKey. We've received no complaints about this. Therefore, I suggest that we drop parameter inheritance support in NSS:

 SECKEYPublicKey *
 CERT_ExtractPublicKey(CERTCertificate *cert)
 {
-     SECStatus rv;
- 
-     if (!cert) {
-         PORT_SetError(SEC_ERROR_INVALID_ARGS);
- 	return NULL;
-     }
-     rv = SECKEY_UpdateCertPQG(cert);
-     if (rv != SECSuccess) return NULL;
-     return seckey_ExtractPublicKey(&cert->subjectPublicKeyInfo);
+     return SECKEY_ExtractPublicKey(&cert->subjectPublicKeyInfo);
 }

Here is the current mozilla::pkix code:

https://hg.mozilla.org/mozilla-central/annotate/c70f62375f7d/security/pkix/lib/pkixnss.cpp#l43

I think we should resolve this bug soon so it can be opened up.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #6)
> Since Firefox 31, Firefox and Thunderbird have rejected any certificate that
> relies on DSA/ECDSA parameter inheritance, because mozilla::pkix does not
> allow DSA parameter inheritance. In particular, mozilla::pkix uses
> SECKEY_ExtractPublicKey instead of CERT_ExtractPublicKey.

I should clarify this: SECKEY_ExtractPublicKey may also modify the parameter that is passed in. However, in mozilla::pkix, we pass in a unshared temporary SECKEYPublicKey structure to SECKEY_ExtractPublicKey, so there's no race condition as far as mozilla::pkix is concerned.

However, other parts of Gecko use CERT_ImportCerts and other functions with a shared SECKEYPublicKey and/or shared CERTCertificate. And, that may be the case in Chrome and other apps too. Note that there are many functions that indirectly call these functions, including in particular ssl3_HandleCertificate in libssl.

Comment 8

[Robert Relyea]

The modified parameter could be fixed by moving the SECITEM_CopyItem(_) call between 

os = 
and
DER_ConvertBitString()

   /* Convert bit string length from bits to bytes */
    os = spki->subjectPublicKey;
    DER_ConvertBitString (&os);

becomes:
   /* Convert bit string length from bits to bytes */
    os = spki->subjectPublicKey;
    rv = SECITEM_CopyItem(arena, &newOs, &os);
    if ( rv != SECSuccess )
        goto loser;
    DER_ConvertBitString (&newOs);


Add the loser; label right before SECKEY_DestroyPublicKey.

Comment 9

[Robert Relyea]

 SECKEYPublicKey *
 CERT_ExtractPublicKey(CERTCertificate *cert)
 {
-     SECStatus rv;
- 
-     if (!cert) {
-         PORT_SetError(SEC_ERROR_INVALID_ARGS);
- 	return NULL;
-     }
-     rv = SECKEY_UpdateCertPQG(cert);
-     if (rv != SECSuccess) return NULL;
-     return seckey_ExtractPublicKey(&cert->subjectPublicKeyInfo);
+     return SECKEY_ExtractPublicKey(&cert->subjectPublicKeyInfo);
 }

At this point I'm OK with this change (if we have to revert it in RHEL, it would be an easy reversion). The only caveat is you should call the underbar version of seckey_ExtractPublicKey unless you've also colapsed SECKEY_ExtractPublicKey and seckey_ExtractPublicKey.

bob

Comment 10

[Franziskus Kiefer [:franziskus]]

Attachment: Bug671097.patch

Hi Bob, I put the comment in a patch to get this moving. Can you review it? thanks

Comment 11

[Robert Relyea]

Comment on attachment 8724760 [details] [diff] [review]
Bug671097.patch

Review of attachment 8724760 [details] [diff] [review]:
-----------------------------------------------------------------

This is the basic patch that I agreed to comment 8 a year and a half ago. I see no reason to deny this now. Thanks Franziskus for pushing this forward. (and thanks bugzilla for keeping the history so I didn't have to rethink this again:).

Comment 12

[Franziskus Kiefer [:franziskus]]

thanks for the review Bob.
https://hg.mozilla.org/projects/nss/rev/939c111a293d

Comment 13

[Franziskus Kiefer [:franziskus]]

I suspect this being responsible for failures like [1]. I backed out the patch as we're close to the 3.24 release.
https://hg.mozilla.org/projects/nss/rev/ed9dda348bcc

Bob, are those tests that are using PQG parameter inheritance? How do we deal with them?

[1] https://bot.nss-crypto.org:8011/builders/2-osx106-x64-OPT/builds/774/steps/shell/logs/stdio

Comment 14

[Robert Relyea]

pkits.sh: #3469: Valid DSA Parameter Inheritance Test5 (255)  - FAILED
pkits.sh: #3470: Valid DSA Parameter Inheritance Test5 (1)  - FAILED
pkits.sh: #3471: Valid DSA Parameter Inheritance Test5 (2)  - FAILED
pkits.sh: #7402: Valid DSA Parameter Inheritance Test5 (255)  - FAILED
pkits.sh: #7403: Valid DSA Parameter Inheritance Test5 (1)  - FAILED
pkits.sh: #3469: Valid DSA Parameter Inheritance Test5 (255)  - FAILED
pkits.sh: #3470: Valid DSA Parameter Inheritance Test5 (1)  - FAILED
pkits.sh: #3471: Valid DSA Parameter Inheritance Test5 (2)  - FAILED
pkits.sh: #7402: Valid DSA Parameter Inheritance Test5 (255)  - FAILED
pkits.sh: #7403: Valid DSA Parameter Inheritance Test5 (1)  - FAILED
TB [2016-04-21 04:25:29] NSS - tests - 64 bits - OPT FAILED

These are exactly the tests refered to in comment 1:

"Note that the latest NIST test suite for PKIX still tests for PQG parameter inheritance [1], so I am assuming that we cannot just disable the PQG inheritance support. However, I noticed some products (in particular, WebSphere [1]) do not support this feature."

Since we decided that we didn't need to process chaining in comment 6 I think we can just turn those tests off.

bob

Comment 15

[Franziskus Kiefer [:franziskus]]

Attachment: Bug671097.patch

disabling inheritance test

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 8744265 [details] [diff] [review]
Bug671097.patch

r=kaie for disabling the tests

Comment 17

[Franziskus Kiefer [:franziskus]]

https://hg.mozilla.org/projects/nss/rev/81ac6ae397ea