Closed
Bug 1165911
Opened 9 years ago
Closed 9 years ago
GatherEKUTelemetry fails when parsing certs with no X.509v3 extensions at all
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox38 | --- | wontfix |
firefox38.0.5 | --- | wontfix |
firefox39 | --- | fixed |
firefox40 | --- | fixed |
firefox41 | --- | fixed |
firefox-esr31 | --- | unaffected |
firefox-esr38 | --- | fixed |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | fixed |
b2g-master | --- | fixed |
People
(Reporter: dkg, Assigned: keeler)
References
Details
Attachments
(3 files, 1 obsolete file)
10.40 KB,
text/plain
|
Details | |
4.48 KB,
patch
|
keeler
:
review+
lizzard
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
4.49 KB,
patch
|
keeler
:
review+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-esr38+
jocheng
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:37.0) Gecko/20100101 Firefox/37.0 Iceweasel/37.0.2 Build ID: 20150420231403 Steps to reproduce: with iceweasel 38.0-2 (from debian), visit https://mentors.debian.net/ in safe-mode. (this is noted on https://bugs.debian.org/782772) The segmentation fault appears to be from a dereference of endEntityCert->extensions when no extensions are present in the X.509v3 certificate (which is the case for at least one of the certs offered by mentors.debian.net). http://sources.debian.net/src/iceweasel/38.0-2/security/manager/ssl/src/SSLServerCertVerification.cpp/?hl=1024#L1047 I'm attaching the certificates currently offered from https://mentors.debian.net, as recorded via "gnutls-cli --print-cert mentors.debian.net" I ran into the same problem with 37.0.2, fwiw. Actual results: SEGFAULT Expected results: no segfault :)
Comment 1•9 years ago
|
||
David, does it ring a bell? FWIW, I don't have a crash with 38.0.5 b2.
Status: UNCONFIRMED → NEW
status-firefox38:
--- → affected
Ever confirmed: true
Flags: needinfo?(dkeeler)
See Also: → http://bugs.debian.org/782772
Assignee | ||
Comment 2•9 years ago
|
||
Ah, yep - missed a null check. I added some extra safety checks as well. Eventually we shouldn't be relying on NSS to extract these values - we should use mozilla::pkix.
Assignee: nobody → dkeeler
Status: NEW → ASSIGNED
Flags: needinfo?(dkeeler)
Attachment #8607137 -
Flags: review?(cykesiopka.bmo)
Comment 3•9 years ago
|
||
Comment on attachment 8607137 [details] [diff] [review] patch Review of attachment 8607137 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, but is this (should it be) tested anywhere? ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +911,3 @@ > if (rv != SECSuccess || !isBuiltIn) { > PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, > + ("BR telemetry: issuer of '%s' is not a built-in root (or " Nit: maybe "root" or "root cert" instead of "issuer"?
Attachment #8607137 -
Flags: review?(cykesiopka.bmo) → review+
Updated•9 years ago
|
Component: Untriaged → Security: PSM
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Version: 38 Branch → 36 Branch
Assignee | ||
Comment 6•9 years ago
|
||
Thanks for the review. Testing this would require something like obtaining a certificate issued by a built-in CA with no extensions (which shouldn't exit, according to the BRs). I think we could probably manage it, but the benefit to effort ratio would be small.
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/006a48240dfd
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Updated•9 years ago
|
Blocks: 968817
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → affected
status-b2g-master:
--- → affected
status-firefox39:
--- → affected
status-firefox40:
--- → affected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → affected
Assignee | ||
Comment 8•9 years ago
|
||
Approval Request Comment [Feature/regressing bug #]: bug 968817 [User impact if declined]: potential null-dereference crashes on systems with custom built-in root certificates [Describe test coverage new/current, TreeHerder]: manually tested [Risks and why]: low - this just does some additional null checks [String/UUID change made/needed]: none
Attachment #8607137 -
Attachment is obsolete: true
Attachment #8608368 -
Flags: review+
Attachment #8608368 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 9•9 years ago
|
||
[Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: potential null-dereference crashes on systems with custom built-in root certificates Fix Landed on Version: 41 Risk to taking this patch (and alternatives if risky): low - this just does some additional null checks String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8608369 -
Flags: review+
Attachment #8608369 -
Flags: approval-mozilla-esr38?
Attachment #8608369 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8608369 [details] [diff] [review] patch for beta, esr-38 This also applies to b2g37 (v2.2) NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] see previous comments
Attachment #8608369 -
Flags: approval-mozilla-b2g37?
Updated•9 years ago
|
Comment 11•9 years ago
|
||
Comment on attachment 8608368 [details] [diff] [review] patch as landed Approved for uplift to aurora to improve stability
Attachment #8608368 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 12•9 years ago
|
||
Comment on attachment 8608369 [details] [diff] [review] patch for beta, esr-38 Approved for uplift to beta, should improve stability
Attachment #8608369 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 13•9 years ago
|
||
Comment on attachment 8608369 [details] [diff] [review] patch for beta, esr-38 Also taking this for esr 38.
Attachment #8608369 -
Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Updated•9 years ago
|
Comment 16•9 years ago
|
||
Comment on attachment 8608369 [details] [diff] [review] patch for beta, esr-38 Approved for uplift to b2g37 to improve stability. Please NI if causing any issue.
Attachment #8608369 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in
before you can comment on or make changes to this bug.
Description
•