Closed Bug 1368652 (CVE-2017-7792) Opened 7 years ago Closed 7 years ago

GetDefaultOIDFormat: buffer overflow caused by long OIDs

Categories

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

47 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 55+ fixed
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 + fixed
firefox56 + fixed

People

(Reporter: frase, Assigned: keeler)

References

Details

(Keywords: crash, regression, sec-high, Whiteboard: [psm-assigned][adv-main55+][adv-esr52.3+][post-critsmash-triage])

Crash Data

Attachments

(5 files, 2 obsolete files)

Attached file cp-bigoid.pem
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170420084331

Steps to reproduce:

View a cert with a really long OID.

You can reproduce using attached file cp-bigoid.pem.  Go to certificate import dialog,
then "View Cert".


Actual results:

The GetDefaultOIDFormat printing function overflows the buffer, causing
segfault.


Expected results:

The OID should have been printed properly.

Or at the very least, Firefox should not have crashed :)
Component: Untriaged → Security: PSM
Product: Firefox → Core
It regressed:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=efe7d026ac641759838dd3897c37892e37e5b244&tochange=b942c98f56c4c2926b8b81b98425072a091bbf7b

Probably Cykesiopka — Bug 1253108 - Enable ESLint "strict" rule for PSM. r=keeler

CR FF53:
https://crash-stats.mozilla.com/report/index/ad400cdf-b8df-417c-869e-9f9ab1170530
Blocks: 1253108
Status: UNCONFIRMED → NEW
Crash Signature: [@ GetDefaultOIDFormat ]
Has Regression Range: --- → yes
Has STR: --- → yes
Ever confirmed: true
Keywords: crash, regression
Version: 53 Branch → 48 Branch
It's good practice to file buffer overflow bugs as security-sensitive until they're investigated and fixed.
Group: crypto-core-security
I believe this was caused by bug 1197314. If snprintf is given a string that does not fit in the given buffer, it doesn't overflow the buffer, but it returns the size of of what it would have written. PR_snprintf returns what it did write.

This is almost certainly exploitable, but it requires some user interaction. I think it could be done with some simple timing-based click-jacking, though, hence sec-high.
Assignee: nobody → dkeeler
Blocks: 1197314
No longer blocks: 1253108
Keywords: sec-high
Priority: -- → P1
Whiteboard: [psm-assigned]
It's possible, it was just a guess from me.

Is there a reason why the entire browser crashes even with e10s enabled?
I think the offending code is in the parent process rather than the child, so the whole browser goes down.
Version: 48 Branch → 47 Branch
Attached patch patch (obsolete) — Splinter Review
Attachment #8872581 - Attachment is obsolete: true
Attachment #8873125 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8873125 [details] [diff] [review]
patch

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

Cool - this is much saner.

r+ with or without my suggestions below.

::: security/manager/ssl/nsNSSCertHelper.cpp
@@ +209,5 @@
>  
>  static nsresult
>  GetDefaultOIDFormat(SECItem* oid, nsAString& outString, char separator)
>  {
> +  nsCString result;

Unless I misread the code of the callers, I think we can just truncate and append to |outString| directly.

@@ +251,5 @@
>          unsigned long one = std::min(val / 40, 2UL); // never > 2
>          unsigned long two = val - (one * 40);
>  
> +        nsPrintfCString formatted("%lu%c%lu", one, separator, two);
> +        result.Append(formatted);

I believe we can use |AppendPrintf| instead, and save having to use |nsPrintfCString|s.

::: security/manager/ssl/tests/mochitest/browser/browser_certViewer.js
@@ +125,5 @@
>    yield BrowserTestUtils.closeWindow(win);
>  });
>  
> +add_task(function* testLongOID() {
> +  // This certificate has a certificatePolicies extension with a policy with a

I assume we're going to check this in later per the sec bug approval process?
Attachment #8873125 - Flags: review?(cykesiopka.bmo) → review+
Yes, you will need to ask for sec-approval. We are getting close to release (the RC 54 build is next week).
Thanks!

(In reply to :Cykesiopka from comment #8)
> Comment on attachment 8873125 [details] [diff] [review]
> ::: security/manager/ssl/nsNSSCertHelper.cpp
> @@ +209,5 @@
> >  
> >  static nsresult
> >  GetDefaultOIDFormat(SECItem* oid, nsAString& outString, char separator)
> >  {
> > +  nsCString result;
> 
> Unless I misread the code of the callers, I think we can just truncate and
> append to |outString| directly.

Ah - good call.

> @@ +251,5 @@
> >          unsigned long one = std::min(val / 40, 2UL); // never > 2
> >          unsigned long two = val - (one * 40);
> >  
> > +        nsPrintfCString formatted("%lu%c%lu", one, separator, two);
> > +        result.Append(formatted);
> 
> I believe we can use |AppendPrintf| instead, and save having to use
> |nsPrintfCString|s.

Fixed.

> ::: security/manager/ssl/tests/mochitest/browser/browser_certViewer.js
> @@ +125,5 @@
> >    yield BrowserTestUtils.closeWindow(win);
> >  });
> >  
> > +add_task(function* testLongOID() {
> > +  // This certificate has a certificatePolicies extension with a policy with a
> 
> I assume we're going to check this in later per the sec bug approval process?

Yeah, I suppose I should have split this into two patches from the start.
Attached patch bug fix patchSplinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Fairly easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The test is in a separate patch now. The check-in comment is a bit vague.

Which older supported branches are affected by this flaw?

All (except esr45 if that hasn't already been eol'd)

If not all supported branches, which bug introduced the flaw?

Bug 1197314 (landed in 47)

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This applies cleanly to nightly, aurora, and beta. I'll prepare a patch for esr52 (which I think will also apply to release, but I'm assuming we're not going to do a dot release at this point). (The patch for esr52 should be straight-forward and not risky.)

How likely is this patch to cause regressions; how much testing does it need?

Not very likely, and in any case this is a rarely-used informational-only feature, so even if it ceases to display the right result, it's better than RCE.
Attachment #8873125 - Attachment is obsolete: true
Attachment #8873571 - Flags: sec-approval?
Attachment #8873571 - Flags: review+
Attached patch test patchSplinter Review
This is just the test.
Attachment #8873572 - Flags: review+
I think this may have missed the window to make the Firefox 54 release, which means checkin will be delayed at least a few weeks.

NI? Ritu and Liz to confirm.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
In fact, it has definitely missed the window. I'm giving sec-approval for checkin on June 27, two weeks into the new cycle. It is too late for 54.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Whiteboard: [psm-assigned] → [psm-assigned][checkin on 6/27]
Attachment #8873571 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b274e6e81c8bd890dade81be1e989226ccad9fd2

Please request approval on the Beta and ESR52 patches when you get a chance.
Flags: needinfo?(dkeeler)
Whiteboard: [psm-assigned][checkin on 6/27] → [psm-assigned]
https://hg.mozilla.org/mozilla-central/rev/b274e6e81c8b
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8873571 [details] [diff] [review]
bug fix patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1197314 
[User impact if declined]: code execution with minimal user interaction
[Is this code covered by automated tests?]: not so much, but there is an associated patch with a test in this bug
[Has the fix been verified in Nightly?]: no
[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?]: not very
[Why is the change risky/not risky?]: this is a seldom-used display-only feature, so even if we break it it's better than RCE
[String changes made/needed]: none
Flags: needinfo?(dkeeler)
Attachment #8873571 - Flags: approval-mozilla-beta?
Comment on attachment 8873586 [details] [diff] [review]
bug fix for esr52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: this is sec-high
User impact if declined: code execution with minimal user interaction
Fix Landed on Version: 56
Risk to taking this patch (and alternatives if risky): not much - this is in a seldom-used display-only feature, and the fix is minimal and simple
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8873586 - Flags: approval-mozilla-esr52?
Group: crypto-core-security → core-security-release
Attachment #8873571 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8873586 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Whiteboard: [psm-assigned] → [psm-assigned][adv-main55+][adv-esr52.3+]
Alias: CVE-2017-7792
Flags: qe-verify+
Whiteboard: [psm-assigned][adv-main55+][adv-esr52.3+] → [psm-assigned][adv-main55+][adv-esr52.3+][post-critsmash-triage]
I was able to reproduce the crash [1] on an affected build (54.0.1, 20170628145605, 90f18f9c15f7) using the test case from Comment 0 on Ubuntu 16.04 x64.

The crash is no longer occurring on Ubuntu 16.04 x64, Windows 10 x64 and macOS 10.12 using the following builds:
    - 55.0 (20170731163142, aa08b24ffb91)
    - 56.0a1 (20170731175927, 87824406b9fe)
    - 52.2esr (20170731051123, 8e0ad123ef5e)

But there's a questionable behavior when opening the certificate in some cases: the "Downloading Certificate" dialog is not displayed, instead the epm file's page source is shown in the tab [2]. It happens on Ubuntu (only on 56.0a1) and Windows 10 (52.2esr, 55.0 and 56.0a1) on my machine. I'm not seeing it on macOS at all. David, is this expected?


[1] bp-c64873c4-1a02-4877-b8b5-b8c831170801
[2] https://screenshots.firefox.com/4MLT6v9DqPyPgEPN/null
Flags: needinfo?(dkeeler)
That's unexpected. Another way to verify would be to download the file and import it via the certificate manager (about:preferences -> Advanced or Privacy & Security, depending on the version of Firefox -> Certificates -> View Certificates -> Authorities -> Import).

Can you file a bug on the behavior you're seeing? (it may just be a server issue - I think the feature depends on the mime type the server sends)
Flags: needinfo?(dkeeler)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.