Closed Bug 1018783 (CVE-2014-1545) Opened 6 years ago Closed 6 years ago
OOB write with sprintf and console functions
3.33 KB, patch
|Details | Diff | Splinter Review|
1.07 KB, patch
|Details | Diff | Splinter Review|
Repro:: a. <script>console.warn("%301f", "1");</script> b. <script>console.warn("%65416.123f", "a");</script> Code:: (fout is char fout;) 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)
Duplicate of this bug: 1019451
Is this something we have to fix in the console API? I don't have access to bug 1019451
No, this is an NSPR bug.
Assignee: nobody → wtc
Component: DOM → NSPR
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.
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."
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
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.
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.
> 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.
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? :(
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
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.
Attachment #8433797 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Note that Fx30 is currently on NSPR 4.10.4 RTM. Can we spin a 18.104.22.168 release?
Can't we just land the NSPR patch into our tree on the branches?
Fixed on the release branches by way of bug 1021287.
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.
You need to log in before you can comment on or make changes to this bug.