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)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: keeler, Assigned: keeler)
References
Details
(Keywords: sec-audit, sec-moderate, Whiteboard: [psm-assigned][adv-main52+][post-critsmash-triage])
Attachments
(3 files)
7.12 KB,
patch
|
jcj
:
review+
|
Details | Diff | Splinter Review |
7.08 KB,
patch
|
keeler
:
review+
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.07 KB,
patch
|
keeler
:
review+
gchang
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
Looks like this landed in bug 1293231, in 52.
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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+
Comment 4•7 years ago
|
||
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)
Updated•7 years ago
|
Group: core-security → crypto-core-security
Comment 5•7 years ago
|
||
(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)
Assignee | ||
Comment 6•7 years ago
|
||
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
Comment 8•7 years ago
|
||
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
status-firefox-esr52:
--- → affected
Flags: needinfo?(dkeeler)
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 9•7 years ago
|
||
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?
Assignee | ||
Comment 10•7 years ago
|
||
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 11•7 years ago
|
||
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 12•7 years ago
|
||
Comment on attachment 8840995 [details] [diff] [review] 1341905-for-beta.diff avoid a crash, beta52+
Attachment #8840995 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/75e0388a1c2ba34871f4e9b6bc3d4f372cc7fd70
Comment 14•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/68157e467e6c5e7b14cac0002610d271ac2d9610
Comment 15•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/68157e467e6c
Updated•7 years ago
|
Whiteboard: [psm-assigned] → [psm-assigned][adv-main52+]
Updated•7 years ago
|
Flags: qe-verify+
Whiteboard: [psm-assigned][adv-main52+] → [psm-assigned][adv-main52+][post-critsmash-triage]
Updated•7 years ago
|
Group: crypto-core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•