Closed Bug 1165911 Opened 5 years ago Closed 5 years ago

GatherEKUTelemetry fails when parsing certs with no X.509v3 extensions at all

Categories

(Core :: Security: PSM, defect)

36 Branch
defect
Not set

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)

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 :)
David, does it ring a bell?
FWIW, I don't have a crash with 38.0.5 b2.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dkeeler)
Attached patch patch (obsolete) — Splinter Review
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 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+
Component: Untriaged → Security: PSM
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
Version: 38 Branch → 36 Branch
Duplicate of this bug: 1166129
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.
https://hg.mozilla.org/mozilla-central/rev/006a48240dfd
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Attached patch patch as landedSplinter Review
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?
[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?
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?
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 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 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+
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.