Closed Bug 1341905 Opened 7 years ago Closed 7 years ago

CertVerifier::VerifyCertificateTransparencyPolicy assumes if builtChain is non-empty, then it has at least 2 elements (which it may not)

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Keywords: sec-audit, sec-moderate, Whiteboard: [psm-assigned][adv-main52+][post-critsmash-triage])

Attachments

(3 files)

Result
CertVerifier::VerifyCertificateTransparencyPolicy(
  NSSCertDBTrustDomain& trustDomain, const UniqueCERTCertList& builtChain,
  Input sctsFromTLS, Time time,
  /*optional out*/ CertificateTransparencyInfo* ctInfo)
{

...

  if (!builtChain || CERT_LIST_EMPTY(builtChain)) {             [0]
    return Result::FATAL_ERROR_INVALID_ARGS;
  }

...

  CERTCertListNode* endEntityNode = CERT_LIST_HEAD(builtChain); [1]
  if (!endEntityNode) {
    return Result::FATAL_ERROR_INVALID_ARGS;
  }
  CERTCertListNode* issuerNode = CERT_LIST_NEXT(endEntityNode); [2]
  if (!issuerNode) {
    // Issuer certificate is required for SCT verification.
    return Success;
  }

Due to the check at [0], we know [1] is safe, but [2] isn't, necessarily. Luckily, encountering a verified certificate chain of length 1 requires using certutil (or an add-on, I suppose) to add and trust an end-entity certificate to a your certdb and then visiting a site using that certificate. The number of people who have done this is vanishingly small.

Sidebar: these macros are terrible and we've misused them before (in almost the same way, I want to say). I believe the right thing to do is to test CERT_LIST_END rather than null check, but in any case we should consider making a wrapper or something that gives us a hard-to-do-the-wrong-thing API.
Attached patch patchSplinter Review
I went through PSM and found the potentially problematic uses of CERT_LIST_* APIs. For each of the non-CT-related changes in this patch, other parts of the code ensure we aren't currently doing something dangerous, but I added the extra checks to be safe and consistent.
Attachment #8840207 - Flags: review?(jjones)
Comment on attachment 8840207 [details] [diff] [review]
patch

The patch and test both look fine.

I'm digging through the other instances of CERT_LIST_HEAD throughout the tree to make sure we didn't miss any - just to double-check!

I'm not really sure that this is all that exploitable, given that it needs to add an EE cert to the DB, so it's probably low-ish criticality, but I agree it'd be good  to fix this before 52 releases.
Attachment #8840207 - Flags: review?(jjones) → review+
Keeler - I agree, you got everything in PSM.

Off in NSS-land, I see a place where it _looks like_ CERT_LIST_END should be semantically used, but it ... isn't:  http://searchfox.org/mozilla-central/source/security/nss/lib/certdb/stanpcertdb.c#533 Perhaps we should catch this too? Tim?
Flags: needinfo?(ttaubert)
Group: core-security → crypto-core-security
(In reply to J.C. Jones [:jcj] from comment #4)
> Off in NSS-land, I see a place where it _looks like_ CERT_LIST_END should be
> semantically used, but it ... isn't: 
> http://searchfox.org/mozilla-central/source/security/nss/lib/certdb/
> stanpcertdb.c#533 Perhaps we should catch this too? Tim?

Filed bug 1342061 that covers a few more places, thanks!
Flags: needinfo?(ttaubert)
Keywords: sec-audit
Thanks!

I'd call this a sec-moderate - could be a sec-high/critical, but requires complex, uncommon user interaction or a situation in which an attacker could already run arbitrary code.
Keywords: sec-moderate
https://hg.mozilla.org/mozilla-central/rev/37a4221a0512

Please request Aurora/Beta approval on this ASAP if you want it to get into 52.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(dkeeler)
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Approval Request Comment
[Feature/Bug causing the regression]: bug 1293231
[User impact if declined]: potential crashes in uncommon configurations
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes (well, it landed and the test passes)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this is a simple correctness change
[String changes made/needed]: none
Flags: needinfo?(dkeeler)
Attachment #8840995 - Flags: review+
Attachment #8840995 - Flags: approval-mozilla-beta?
Approval Request Comment
[Feature/Bug causing the regression]: bug 1293231
[User impact if declined]: potential crashes in uncommon configurations
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes (well, it landed and the test passes)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this is a simple correctness change
[String changes made/needed]: none
Attachment #8841017 - Flags: review+
Attachment #8841017 - Flags: approval-mozilla-aurora?
Comment on attachment 8841017 [details] [diff] [review]
1341905-for-aurora.diff

Fix a security issue. Aurora53+.
Attachment #8841017 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8840995 [details] [diff] [review]
1341905-for-beta.diff

avoid a crash, beta52+
Attachment #8840995 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [psm-assigned] → [psm-assigned][adv-main52+]
Flags: qe-verify+
Whiteboard: [psm-assigned][adv-main52+] → [psm-assigned][adv-main52+][post-critsmash-triage]
Group: crypto-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.