Closed Bug 1031542 Opened 5 years ago Closed 5 years ago

Crash on https://admissions.tpa.edu.tw with mozilla::pkix enabled due to CheckKeyUsage null pointer dereference

Categories

(Core :: Security: PSM, defect, major)

31 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox30 --- unaffected
firefox31 + verified
firefox32 + verified
firefox33 --- verified
firefox-esr24 --- unaffected

People

(Reporter: mwobensmith, Assigned: briansmith)

References

Details

(4 keywords)

Attachments

(2 files, 1 obsolete file)

On Fx31, default setting of security.mozilla.use_mozillapkix_verification set to true, this site crashes.

https://admissions.tpa.edu.tw

If I disable that pref, the site either loads normally, or gives this cert error:

admissions.tpa.edu.tw uses an invalid security certificate. The certificate does not come from a trusted source. (Error code: sec_error_inadequate_key_usage)
Mac OS stack
The patch for bug 1006812 will fix this issue. I suggest the patch for bug 1006812 be uplifted to mozilla-aurora and mozilla-beta.

The problem is this code:

> 64  SECItem tmpItem;
> 65  Result rv = MapSECStatus(SEC_QuickDERDecodeItem(arena, &tmpItem,
> 66                              SEC_ASN1_GET(SEC_BitStringTemplate),
> 67                              encodedKeyUsage));
> 68  if (rv != Success) {
> 69    return rv;
> 70  }
> 71
> 72  // TODO XXX: Why is tmpItem.len > 1?
> 73
> 74  KeyUsages allowedKeyUsages = tmpItem.data[0];
> 75  if ((allowedKeyUsages & requiredKeyUsagesIfPresent)
> 76        != requiredKeyUsagesIfPresent) {
> 77    return Fail(RecoverableError, SEC_ERROR_INADEQUATE_KEY_USAGE);
> 78  }

On line 74, tmpItem.data is nullptr and tmpItem.len is 0. Thus, we have a null pointer dereference AND a buffer overflow.

Matt, your stack trace doesn't show the symbol/line information for the Gecko code in question. See if the information at https://developer.mozilla.org/en-US/docs/Using_the_Mozilla_symbol_server allows you to generate more informative stack traces. Here is the stack trace from MSVC2013 (relevant thread only):

>	xul.dll!mozilla::pkix::CheckKeyUsage(mozilla::pkix::EndEntityOrCA endEntityOrCA, bool isTrustAnchor, const SECItemStr * encodedKeyUsage, unsigned int requiredKeyUsagesIfPresent, PLArenaPool * arena) Line 74	C++
 	xul.dll!mozilla::pkix::CheckIssuerIndependentProperties(mozilla::pkix::TrustDomain & trustDomain, mozilla::pkix::BackCert & cert, __int64 time, mozilla::pkix::EndEntityOrCA endEntityOrCA, unsigned int requiredKeyUsagesIfPresent, SECOidTag requiredEKUIfPresent, SECOidTag requiredPolicy, unsigned int subCACount, mozilla::pkix::TrustDomain::TrustLevel * trustLevelOut) Line 509	C++
 	xul.dll!mozilla::pkix::BuildForward(mozilla::pkix::TrustDomain & trustDomain, mozilla::pkix::BackCert & subject, __int64 time, mozilla::pkix::EndEntityOrCA endEntityOrCA, unsigned int requiredKeyUsagesIfPresent, SECOidTag requiredEKUIfPresent, SECOidTag requiredPolicy, const SECItemStr * stapledOCSPResponse, unsigned int subCACount, mozilla::pkix::ScopedPtr<CERTCertListStr,&CERT_DestroyCertList> & results) Line 217	C++
 	xul.dll!mozilla::pkix::BuildCertChain(mozilla::pkix::TrustDomain & trustDomain, CERTCertificateStr * certToDup, __int64 time, mozilla::pkix::EndEntityOrCA endEntityOrCA, unsigned int requiredKeyUsagesIfPresent, SECOidTag requiredEKUIfPresent, SECOidTag requiredPolicy, const SECItemStr * stapledOCSPResponse, mozilla::pkix::ScopedPtr<CERTCertListStr,&CERT_DestroyCertList> & results) Line 343	C++
 	xul.dll!mozilla::psm::BuildCertChainForOneKeyUsage(mozilla::pkix::TrustDomain & trustDomain, CERTCertificateStr * cert, __int64 time, unsigned int ku1, unsigned int ku2, unsigned int ku3, SECOidTag eku, SECOidTag requiredPolicy, const SECItemStr * stapledOCSPResponse, mozilla::pkix::ScopedPtr<CERTCertListStr,&CERT_DestroyCertList> & builtChain) Line 209	C++
 	xul.dll!mozilla::psm::CertVerifier::MozillaPKIXVerifyCert(CERTCertificateStr * cert, const __int64 usage, const __int64 time, void * pinArg, const unsigned int flags, const SECItemStr * stapledOCSPResponse, mozilla::pkix::ScopedPtr<CERTCertListStr,&CERT_DestroyCertList> * validationChain, SECOidTag * evOidPolicy) Line 336	C++
 	xul.dll!mozilla::psm::CertVerifier::VerifySSLServerCert(CERTCertificateStr * peerCert, const SECItemStr * stapledOCSPResponse, __int64 time, void * pinarg, const char * hostname, bool saveIntermediatesInPermanentDatabase, mozilla::pkix::ScopedPtr<CERTCertListStr,&CERT_DestroyCertList> * certChainOut, SECOidTag * evOidPolicy) Line 831	C++
 	xul.dll!mozilla::psm::`anonymous namespace'::AuthCertificate(mozilla::psm::CertVerifier & certVerifier, mozilla::psm::TransportSecurityInfo * infoObject, CERTCertificateStr * cert, SECItemStr * stapledOCSPResponse, unsigned int providerFlags, __int64 time) Line 978	C++
 	xul.dll!mozilla::psm::`anonymous namespace'::SSLServerCertVerificationJob::Run() Line 1115	C++
 	xul.dll!nsThreadPool::Run() Line 211	C++
 	xul.dll!nsThread::ProcessNextEvent(bool mayWait, bool * result) Line 721	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * thread, bool mayWait) Line 263	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 336	C++
 	xul.dll!MessageLoop::RunHandler() Line 223	C++
 	xul.dll!MessageLoop::Run() Line 197	C++
 	xul.dll!nsThread::ThreadFunc(void * arg) Line 325	C++
 	nss3.dll!_PR_NativeRunThread(void * arg) Line 419	C
 	nss3.dll!pr_root(void * arg) Line 90	C
 	[External Code]
Severity: critical → major
Depends on: 1006812
Attached patch bug-1031542.patch (obsolete) — Splinter Review
Assignee: nobody → brian
Status: NEW → ASSIGNED
Attachment #8447454 - Flags: review?(dkeeler)
What happens when you try to load this site in Firefox 30 with default preferences? Does the page load or not? In Firefox Nightly, the site will fail to load.

Kathleen, is this CA in our CA program? "O = Government Root Certification Authority
C = TW," friendly name "TW Government Root Certification Authority" according to Windows.

This certificate has a "key usage" extension with an empty value. This is saying that the certificate is not valid for any key usage--i.e. never, ever trust this certificate. If the certificate doesn't want to be limited in which key usages it is used for, it should omit the key usage extension. The encoding of the key usage bit string is { 0x03, 0x01, 0x00 } (bit string, length 1, zero padding bits, with no value bits). Perhaps this type of error should go into the problematic practices / things to watch out for section of the CA inclusion documentation.
Flags: needinfo?(mwobensmith)
Comment on attachment 8447454 [details] [diff] [review]
bug-1031542.patch

Test is not testing the thing I thought it was.
Attachment #8447454 - Flags: review?(dkeeler)
The exact encoding used in the test certificate is used in the test here.
Attachment #8447454 - Attachment is obsolete: true
Attachment #8447486 - Flags: review?(dkeeler)
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #4)
> What happens when you try to load this site in Firefox 30 with default
> preferences? Does the page load or not? In Firefox Nightly, the site will
> fail to load.

In Fx30, clean profile, I see the sec_error_inadequate_key_usage error.

In Fx31, mozilla::pkix disabled, I get either that error or a successful page load. Perhaps there is some sort of caching issue here that causes the latter behavior? I am not sure.

Also, sorry for the bad stack, my debug environment is hosed and that was the best I could do in a pinch. Thanks for the link, also.
Flags: needinfo?(mwobensmith)
Comment on attachment 8447486 [details] [diff] [review]
bug-1031542.patch

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

Ah, this makes more sense. r=me with nit addressed.

::: security/pkix/test/gtest/pkixcheck_CheckKeyUsage_tests.cpp
@@ +167,5 @@
> +  ASSERT_BAD(CheckKeyUsage(EndEntityOrCA::MustBeCA, &DER,
> +                           KeyUsage::keyCertSign));
> +}
> +
> +

nit: unnecessary blank line
Attachment #8447486 - Flags: review?(dkeeler) → review+
Group: core-security
Keywords: sec-moderate
https://hg.mozilla.org/integration/mozilla-inbound/rev/32dde3c7eabb

Try run for Mozilla-Aurora uplift:
https://tbpl.mozilla.org/?tree=Try&rev=b73c8f4b17e3

Try run for Mozilla-Beta uplift:
https://tbpl.mozilla.org/?tree=Try&rev=d0a923101831
Summary: Crash on https://admissions.tpa.edu.tw with mozilla::pkix enabled → Crash on https://admissions.tpa.edu.tw with mozilla::pkix enabled due to CheckKeyUsage null pointer dereference
Target Milestone: --- → mozilla33
https://hg.mozilla.org/mozilla-central/rev/32dde3c7eabb
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Verified fixed on today's build of Fx31.
Latest nightly/aurora, Win 7 - no longer crashes with the test link, but the page is not loading.
Is this ok ?
Flags: needinfo?(brian)
Paul, if the site isn't loading, we can leave this alone for now. It was verified in Fx31, so we're pretty confident that it has been fixed. You can always return to the site later to see if it's reachable or not.

If by "not loading" you mean that we get an SSL error, then we would consider that fixed and verified. The crash would happen before that point.
Flags: needinfo?(brian)
(In reply to Matt Wobensmith from comment #14)
> If by "not loading" you mean that we get an SSL error, then we would
> consider that fixed and verified. The crash would happen before that point.
Thanks Matt.
Verified fixed FF 32.0a2 (2014-07-21), 33.0a1 (2014-07-21) Win 7
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.