1018783 - (CVE-2014-1545) OOB write with sprintf and console functions

Status: VERIFIED FIXED

(NSPR :: NSPR, defect, P2)

Description

[Abhishek Arya]

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));

Comment 1

[Abhishek Arya]

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)

Comment 3

[Andrea Marchesini [:baku]]

Is this something we have to fix in the console API? I don't have access to bug 1019451

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, this is an NSPR bug.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 6

[Wan-Teh Chang]

Attachment: Patch: use snprintf

Comment 7

[Abhishek Arya]

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 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8433670 [details] [diff] [review]
Patch: use snprintf

Thank you!

Comment 9

[Wan-Teh Chang]

Attachment: Patch: use snprintf, v2

I added a comment to point out the explicit null termination.

Patch checked in: https://hg.mozilla.org/projects/nspr/rev/74eb616c618e

Comment 10

[Wan-Teh Chang]

Attachment: Fix compilation errors on Windows

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 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8433807 [details] [diff] [review]
Fix compilation errors on Windows

r=me

Comment 12

[(not currently active) Ted Mielczarek]

(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.

Comment 13

[Daniel Veditz [:dveditz]]

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.

Comment 14

[Andrea Marchesini [:baku]]

> 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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Specifically, in bug 965860.  So Firefox 30.

Comment 16

[Wan-Teh Chang]

Fix pushed to mozilla-inbound as part of NSPR_4_10_6_BETA3:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90b3655df3b9

Comment 17

[Boris Zbarsky [:bzbarsky]]

Argh.  We probably needed sec-approval here.  Now that this has landed, do we respin 30 for it?  :(

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

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)?

Comment 19

[Boris Zbarsky [:bzbarsky]]

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.

Comment 20

[Wan-Teh Chang]

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 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90b3655df3b9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Note that Fx30 is currently on NSPR 4.10.4 RTM. Can we spin a 4.10.4.1 release?

Comment 26

[(no longer active)]

Can't we just land the NSPR patch into our tree on the branches?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Fixed on the release branches by way of bug 1021287.

Comment 28

[Matt Wobensmith [:mwobensmith][:matt:]]

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.