Closed Bug 1170750 Opened 5 years ago Closed 5 years ago

Buffer overflow using %f with dosprintf (js/src/jsprf.cpp:812)

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: nbp, Assigned: nbp)

Details

Attachments

(2 files, 2 obsolete files)

The issue reported here is not risky, as only debug builds or debugging API (Jit Inspector / onIonCompilation) are able to to trigger these issues.

Still, with the recent modification of Bug 1147403, we are now able to reproduce this issue quite easily, by running the octane benchmark with a debug build, while adding the following lines at the top of "run.js":


  var __g = newGlobal();
  __g.parent = this;
  __g.eval(`
    var dbg = new Debugger();
    var parentw = dbg.addDebuggee(parent);
    dbg.onIonCompilation = function (graph) {
      print('Compiled ' + graph.scripts[0].displayName + ':' + graph.scripts[0].startLine);
    };
  `);


I opened this bug as a secure bug (sec-want), such that we can double check other implementation of dosprintf, such as the one in xpcom.

I also think that  %f  should just be banned as it can produce large outputs, and that we can replace most of  %f  by something like  %.20g.
Attachment #8614276 - Flags: review?(jdemooij)
(In reply to Nicolas B. Pierron [:nbp] from comment #0)
> I also think that  %f  should just be banned as it can produce large
> outputs, and that we can replace most of  %f  by something like  %.20g.

In fact %.20g is useless, as we are going to print digits which cannot be represented as a double argument which is used when interpreting a %f, %e, %E, %g or %g.  To be precise:

  js> Math.log(2) * 52 / Math.log(10)
  15.65355977452702

Thus, I think that we can safely replace all the  %.20g  by  %.16g  without any precision lost.
(In reply to Nicolas B. Pierron [:nbp] from comment #0)
> […], such that we can double check
> other implementation of dosprintf, such as the one in xpcom.

Apparently this case is handled by returning a buffer overflow error.
 => Opening this issue.

https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/misc/prdtoa.c#3417
Group: javascript-core-security
Keywords: sec-want
Attachment #8614276 - Attachment is obsolete: true
Attachment #8614276 - Flags: review?(jdemooij)
Attachment #8614646 - Flags: review?(jdemooij)
This patch looks fine, but I don't understand the crash. Where do we incorrectly assume %f will result in at most X characters? Shouldn't we fix that instead?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Jan de Mooij [:jandem] from comment #5)
> This patch looks fine, but I don't understand the crash. Where do we
> incorrectly assume %f will result in at most X characters? Shouldn't we fix
> that instead?

Yes, I guess I can increase the buffer size enough to fit the largest double.  But I also think that this is unnecessary to have such float representation, as this increase the string size to provide no additional information.

https://dxr.mozilla.org/mozilla-central/source/js/src/jsprf.cpp#295,321
(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> Yes, I guess I can increase the buffer size enough to fit the largest
> double.  But I also think that this is unnecessary to have such float
> representation, as this increase the string size to provide no additional
> information.

I think the right fix is to use snprintf (or something similar) instead of sprintf. snprintf accepts the target buffer size and writes until that. Unfortunately MSVC < 2015 doesn't support snprintf so we have to use _snprintf or something, but that's easy to ifdef.
Attached patch Use snprintf instead of sprintf. (obsolete) — Splinter Review
Attachment #8617288 - Flags: review?(jdemooij)
Comment on attachment 8617288 [details] [diff] [review]
Use snprintf instead of sprintf.

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

::: js/src/jsprf.cpp
@@ +25,5 @@
>  #include "jsutil.h"
>  
> +#if defined(_MSC_VER) && _MSC_VER < 1900
> +#define snprintf _snprintf
> +#endif

mfbt/Snprintf.h does this better, please use that.

@@ +320,5 @@
> +    memset(fout, 0, sizeof(fout));
> +    snprintf(fout, sizeof(fout), fin, d);
> +    // Explicitly null-terminate fout because on Windows snprintf doesn't
> +    // append a null-terminator if the buffer is too small.
> +    fout[sizeof(fout) - 1] = '\0';

Using Snprintf.h means that you don't have to do all of this; you might even be able to use snprintf_literal to avoid the sizeof.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #10)
> @@ +320,5 @@
> > +    memset(fout, 0, sizeof(fout));
> > +    snprintf(fout, sizeof(fout), fin, d);
> > +    // Explicitly null-terminate fout because on Windows snprintf doesn't
> > +    // append a null-terminator if the buffer is too small.
> > +    fout[sizeof(fout) - 1] = '\0';
> 
> Using Snprintf.h means that you don't have to do all of this; you might even
> be able to use snprintf_literal to avoid the sizeof.

Thanks for the review :)
I did that to do the follow previous patches (see comment 8).
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8614646 [details] [diff] [review]
Replace %f by %.16g in js/src.

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

r=me with comment below addressed.

::: js/src/jsprf.cpp
@@ +491,5 @@
>  
>          case 'e':
>          case 'f':
> +            MOZ_CRASH("Do not use %f, replace them by %.16g.");
> +            break;

This is no longer needed with the second patch right?
Attachment #8614646 - Flags: review?(jdemooij) → review+
Comment on attachment 8617288 [details] [diff] [review]
Use snprintf instead of sprintf.

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

I like Nathan's suggestion; I didn't know about Snprintf.h but it looks useful. The NSPR code doesn't use it but I think that's fine, jsprf.cpp uses more JS/MFBT things like Vector.
Attachment #8617288 - Flags: review?(jdemooij)
New version of the patrch, which use mbft snprintf_literal instead of using
a macro.
Attachment #8617288 - Attachment is obsolete: true
Attachment #8620873 - Flags: review?(jdemooij)
Marking as leave-open for the second-patch.
Keywords: leave-open
Comment on attachment 8620873 [details] [diff] [review]
Use mfbt snprintf_literal instead of sprintf.

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

Nice
Attachment #8620873 - Flags: review?(jdemooij) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/27001d6f6aad
https://hg.mozilla.org/mozilla-central/rev/ad85daa487cf
Assignee: nobody → nicolas.b.pierron
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.