Closed Bug 1018783 (CVE-2014-1545) Opened 10 years ago Closed 10 years ago

OOB write with sprintf and console functions

Categories

(NSPR :: NSPR, defect, P2)

defect

Tracking

(firefox29 unaffected, firefox30+ fixed, firefox31+ verified, firefox32+ verified, firefox-esr24 unaffected, b2g-v1.2 unaffected, b2g-v1.3 unaffected, b2g-v1.3T unaffected, b2g-v1.4 fixed, b2g-v2.0 fixed)

VERIFIED FIXED
4.10.6
Tracking Status
firefox29 --- unaffected
firefox30 + fixed
firefox31 + verified
firefox32 + verified
firefox-esr24 --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: inferno, Assigned: wtc)

References

Details

(Keywords: regression, sec-critical, Whiteboard: [adv-main30+])

Attachments

(2 files, 1 obsolete file)

Repro::
a.    <script>console.warn("%301f", "1");</script>
b.    <script>console.warn("%65416.123f", "a");</script>

Code:: (fout is char fout[300];)
    sprintf(fout, fin, d);

    /*
    ** This assert will catch overflow's of fout, when building with
    ** debugging on. At least this way we can track down the evil piece
    ** of calling code and fix it!
    */
    PR_ASSERT(strlen(fout) < sizeof(fout));
benjamin@15272   303/*
benjamin@15272   304** Convert a double precision floating point number into its printable
benjamin@15272   305** form.
benjamin@15272   306**
benjamin@15272   307** XXX stop using sprintf to convert floating point
benjamin@15272   308*/
benjamin@15272   309static int cvt_f(SprintfState *ss, double d, const char *fmt0, const char *fmt1)
Is this something we have to fix in the console API? I don't have access to bug 1019451
Flags: needinfo?(khuey)
No, this is an NSPR bug.
Assignee: nobody → wtc
Component: DOM → NSPR
Flags: needinfo?(khuey)
Product: Core → NSPR
Version: Trunk → 4.11
Is it simpler to just not call into nspr here instead of waiting for things to get fixed there?

But yes, the NSPR behavior here is clearly wrong.
Attached patch Patch: use snprintf (obsolete) — Splinter Review
Attachment #8433670 - Flags: superreview?(bzbarsky)
Attachment #8433670 - Flags: review?(inferno)
Comment on attachment 8433670 [details] [diff] [review]
Patch: use snprintf

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

I don't understand this code well enough, but use of snprintf should stop the security bug.

::: pr/src/io/prprf.c
@@ +330,5 @@
>      }
>  #endif
> +    memset(fout, 0, sizeof(fout));
> +    snprintf(fout, sizeof(fout), fin, d);
> +    fout[sizeof(fout) - 1] = '\0';

Good catch to explicitly null terminate. http://msdn.microsoft.com/en-us/library/2ts7cx93%28v=vs.110%29.aspx "If len > count, then count characters are stored in buffer, no null-terminator is appended, and a negative value is returned."
Attachment #8433670 - Flags: review?(inferno)
Comment on attachment 8433670 [details] [diff] [review]
Patch: use snprintf

Thank you!
Attachment #8433670 - Flags: superreview?(bzbarsky) → superreview+
I added a comment to point out the explicit null termination.

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/74eb616c618e
Attachment #8433670 - Attachment is obsolete: true
Attachment #8433797 - Flags: checked-in+
Patch checked in: https://hg.mozilla.org/projects/nspr/rev/e6667bfc82c3

Windows only has _snprintf. (I seem to remember snprintf worked before.)
Many files use this fix:
http://mxr.mozilla.org/mozilla-central/search?string=define%2Bsnprintf%2B_snprintf

MSVC also rejects the 0.0 / 0.0 expression.
Attachment #8433807 - Flags: review?(bzbarsky)
Attachment #8433807 - Flags: checked-in+
Comment on attachment 8433807 [details] [diff] [review]
Fix compilation errors on Windows

r=me
Attachment #8433807 - Flags: review?(bzbarsky) → review+
(In reply to Wan-Teh Chang from comment #10)
> Windows only has _snprintf. (I seem to remember snprintf worked before.)
> Many files use this fix:
> http://mxr.mozilla.org/mozilla-central/
> search?string=define%2Bsnprintf%2B_snprintf

We are trying to stop doing that because of the null-termination issue (bug 332006). If you're explicitly null-terminating then it's not a problem.
Clearly the NSPR bug has been around for a while, but did something recently change in the console code that exposed this? Otherwise quite the coincidence that inferno and Aki both filed bugs within a short time of each other. I ask because it may affect how far back we need to port this change, although I guess if we update NSPR we should do so on all the branches.
Flags: needinfo?(amarchesini)
Keywords: sec-critical
> Clearly the NSPR bug has been around for a while, but did something recently
> change in the console code that exposed this?

Yes. Recently the console API has been rewritten in C++ and the format-string support for float numbers has been implemented.
Flags: needinfo?(amarchesini)
Specifically, in bug 965860.  So Firefox 30.
Fix pushed to mozilla-inbound as part of NSPR_4_10_6_BETA3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b3655df3b9
Status: NEW → ASSIGNED
Priority: -- → P2
Hardware: x86_64 → All
Target Milestone: --- → 4.10.6
Version: 4.11 → other
Argh.  We probably needed sec-approval here.  Now that this has landed, do we respin 30 for it?  :(
Flags: needinfo?(lsblakk)
We can do a build #2 of the FF30 release candidate - this really should have gotten sec-approval - since our hand is forced here, let's get approval noms & uplift all the way to mozilla-release asap for a go to build first thing tomorrow morning.  Does it affect ESR24 (comment 15 suggests it does not)?
The underlying NSPR problem does, but the vector that exposed it to the web here does not.  So I don't think we need this on ESR24.
It is not worth merging this fix to any branch.

We are printing a floating point number, so the possible output bytes
are limited: '0'-'9', '+', '-', '.', 'e', 'E'. The risk seems very small.
Comment on attachment 8433797 [details] [diff] [review]
Patch: use snprintf, v2

[Approval Request Comment]
Regression caused by (bug #): Bug 965860
User impact if declined: Writing all over memory.  Bad idea.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):  Should be very low risk.
String or IDL/UUID changes made by this patch:  None
Attachment #8433797 - Flags: approval-mozilla-release?
Attachment #8433797 - Flags: approval-mozilla-beta?
Attachment #8433797 - Flags: approval-mozilla-aurora?
I strongly disagree, fwiw, since we're printing this at arbitrary offsets.  I think we should treat this sort of thing as exploitable unless proven otherwise.
https://hg.mozilla.org/mozilla-central/rev/90b3655df3b9
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #8433797 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8433797 - Flags: approval-mozilla-release?
Attachment #8433797 - Flags: approval-mozilla-release+
Attachment #8433797 - Flags: approval-mozilla-beta?
Attachment #8433797 - Flags: approval-mozilla-beta+
What's the proper way to uplift this to Fx30? IIUC, this fix has only landed on 4.10.6b3, but I thought we can't ship beta version of NSS/NSPR? Do we need to spin a separate point release that's safe for uplift to Fx30? I assume that for Fx31 we can just go with the beta for now and update to the final release once ready.
Flags: needinfo?(wtc)
Note that Fx30 is currently on NSPR 4.10.4 RTM. Can we spin a 4.10.4.1 release?
Can't we just land the NSPR patch into our tree on the branches?
Alias: CVE-2014-1545
Whiteboard: [adv-main30+]
Flags: needinfo?(wtc)
Confirmed crash in Fx31, 2014-06-04.
Verified fixed in Fx30, release.
Verified fixed in Fx31, release candidate.
Verified fixed in Fx32, 2014-07-14.
Status: RESOLVED → VERIFIED
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security
You need to log in before you can comment on or make changes to this bug.