Crash caused by buffer overflow when requesting new tab if FPU accuracy reduced.

RESOLVED DUPLICATE of bug 358569

Status

()

Core
JavaScript Engine
--
critical
RESOLVED DUPLICATE of bug 358569
12 years ago
11 years ago

People

(Reporter: bugzillla, Assigned: mrbkap)

Tracking

({crash})

1.0 Branch
x86
Windows 2000
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate] req's plugin, URL)

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

Firefox 1.5.* crashes when asking for a new tab when the current tab was running a plugin that has reduced the precision of the FPU. This happen on Mozilla, Opera or on previous versions of Firefox.

I have traced through the cause of the crash and the problem occurs in jsdtoa.c, js_dtoa(). I'm reporting this bug as a 'tabbed browser' bug though, because of the consequences. The details of the actual root cause of the bug are listed below under 'additional information'.

When a new tab is created, for some reason I didn't trace (and which presumably doesn't happen in the other Mozilla based browsers, or previous releases of Firefox) Javascript is invoked. This calls js_dtoa() with a large number. If a plugin (or possibly extension) has reduced the accuracy of the FPU, this will cause a buffer overrun in js_dtoa(), and most likely a crash. 

The floating point accuracy used by a plugin should not effect the floating point accuracy used by Firefox.

I have reported this as a security problem as a precaution, although it is unclear whether this buffer overrun could be exploited by a malicious plug-in to gain control of a machine. Whether it is a security risk or not will depend on how much influence the current page/plugin has on the number passed to js_dtoa() when the new tab is requested.



Reproducible: Always

Steps to Reproduce:
To reproduce the tabbed browser bug:

1. Dump the plugin dll http://www.krucible2.co.uk/FirefoxBug/npbasic.dll into your Firefox/plugins folder
2. Visit test webpage http://www.krucible2.co.uk/FirefoxBug/test.html
3. Ask for a new tab (Ctrl-t, or from File menu)

The plugin listed is just the Basic plugin sample from the SDK with the line '_controlfp( _PC_24, MCW_PC );' added to the top of PluginWinProc(). If you don't trust random plugins from strangers then you can simply add the line and build the plugin yourself.

To reproduce the isolated root cause, compile & run the following program ensuring that js3250.dll can be found.

#include "windows.h"
#include "float.h"

#include "jstypes.h"
#include "jsdtoa.h"

typedef JS_FRIEND_API(char *)
(*MYPROC)(char *buffer, size_t bufferSize, JSDToStrMode mode, int precision, double dval);

main()
{
    char buffer[1024];

    HINSTANCE dll = LoadLibrary("js3250.dll");
    if (dll)
    {
        MYPROC JS_dtostr = (MYPROC )GetProcAddress( dll, "JS_dtostr" );
        if (JS_dtostr)
        {
            _controlfp( _PC_24, MCW_PC );
            JS_dtostr(buffer, 26, DTOSTR_STANDARD, 0, 1153311361076.0000);
        }
    }
}
Actual Results:  
For both tabbed browser bug and root cause the application will crash.

Expected Results:  
For the tabbed browser bug, opened a new tab.
For the root cause bug, put the value "1153311361076" in the buffer.

ROOT CAUSE SUMMARY
==================

If the FPU is set to 24-bit precision, and a large number is passed to js_dtoa() it will overrun the buffer provided for the answer. 


ROOT CAUSE DETAIL
==================

The exact problem is that in lines 2384/2385: 

jsdtoa.c line 2383: for (i = 1;;i++) {
jsdtoa.c line 2384:   L = (Long)( d / ds);
jsdtoa.c line 2385:   d -= L*ds;
..
L + '0' is added to output buffer.
..
jsdtoa.c line 2408:   if (!(d*=10.))
jsdtoa.c line 2409:     break;
jsdtoa.c line 2410:  }

where,
    L is an int;
    d is a double containing the number to convert to a string
    ds is a double containing 1x10^n, where n is the number of digits in d

Line 2384 takes the passed number, and uses an integer cast division to get the highest order digit. 

Line 2385 then removes the number we've dealt with from the original passed value.

Line 2408 multiplies the remaining value of d by ten to shift the first digit back into position to be 'extracted' by division by ds. If there are no digits left (and each iteration of the loop should remove 1 digit), we're done.

So the function might be expected to run something like this:

Provide it with d = 597. ds is set to 100, to extract the digit in the 'hundreds' position.

Run the loop and we get: 

L = (Long)( d / ds);   // 597/100 = 5.97, rounded down to a long is 5, so L = 5
d -= L*ds;             // L*ds = 5 * 100 = 500, so 597 - 500 = 97 so d = 97
// L is appended to buffer, making it "5"
if (!(d*=10.))         // 97 * 10 = 970. Not zero, so loop back.

L = (Long)( d / ds);   // 970/100 = 9.70, rounded down to a long is 9, so L = 9
d -= L*ds;             // L*ds = 9 * 100 = 900, so 970 - 900 = 70 so d = 70
// L is appended to buffer, making it "59"
if (!(d*=10.))         // 70 * 10 = 700. Not zero, so loop back.

L = (Long)( d / ds);   // 700/100 = 7.0, so L = 7
d -= L*ds;             // L*ds = 7 * 100 = 700, so 700 - 700 = 0 so d = 0
// L is appended to buffer, making it "597"
if (!(d*=10.))         // 0 * 10 = 0, so break out of loop. 

All done. The buffer is "597". That's how it works, with each loop the highest digit is removed and a '0' comes in at the low end to replace it. One iteration for each digit, and the whole number is parsed.

The problem happens when d is so large that the result of (L*ds) cannot be accurately represented. In this case, when (L*ds) is subtracted from d then the top digit is not simply removed, every digit in the number can change.

Say d = 1000000000000. ds would also be 1000000000000 in this case.

Run the loop and we get: 

L = (Long)( d / ds);   // L = 1
d -= L*ds;             // L*ds  should equal 1000000000000, with d then 0. 
// With precision crippled however, it is actually 999999995904, leaving d as 4096. 

The loop then carries on with this new, wrong number. On the next iteration it gets jumbled some more, and so on, the point being that it NEVER manages to reduce the number to zero, NEVER leaves the loop, and so keeps happily appending characters to the finite buffer supplied until the application explodes.
(Reporter)

Comment 1

12 years ago
First paragraph of description should read:

"This doesn't happen on Mozilla, Opera or on previous versions of Firefox."
(Reporter)

Comment 2

12 years ago
Created attachment 229955 [details]
Plugin DLL that demonstrates crash on new tab

A plugin that causes the bug

Comment 3

12 years ago
Is there any default way for the FPU precision to be reduced? This sounds like a contrived problem to me.
Assignee: nobody → general
Component: Tabbed Browser → JavaScript Engine
Product: Firefox → Core
QA Contact: tabbed.browser → general
Version: unspecified → Trunk
(Reporter)

Comment 4

12 years ago
Not sure what you mean. This is a real problem that really took me a real week to really track down, as it only really becomes really apparent if you look at the disassembly and registers. Really.

The FPU precision of the x86 can be varied at will using the fldcw assembler instruction (so it potentially applies to Linux as well), and is a routine optimisation for 3D applications. This is also a clear change in Firefox's behaviour as this has only come about recently, and doesn't happen in other browsers.
Component: JavaScript Engine → Tabbed Browser
Product: Core → Firefox
Version: Trunk → 1.0 Branch

Comment 5

12 years ago
Is this setting per-machine, and not per-process? Please do not change the component again.
Component: Tabbed Browser → JavaScript Engine
Product: Firefox → Core
Version: 1.0 Branch → 1.8 Branch
(Reporter)

Comment 6

12 years ago
Good question - don't know.
Component: JavaScript Engine → Tabbed Browser
Product: Core → Firefox
Version: 1.8 Branch → 1.0 Branch

Updated

12 years ago
Component: Tabbed Browser → Tabbed Browser
Product: Firefox → Core

Updated

12 years ago
Component: Tabbed Browser → JavaScript Engine
(Reporter)

Comment 7

12 years ago
Right, having had little success with documentation, I ran some experiments. I had two separate processes printing out the fpu precision, with two different answers, so it must be on a per thread (or at least per-process), rather than per-machine basis.

I'm thinking that there might be two separate but related bugs here.

1) FPU accuracy changes in plugin affect FPU accuracy of Firefox.
2) Calling js_dtoa with single-precision FPU accuracy can cause a buffer overrun.

Sorry about the changing category - I've got this bug page constantly open in my browser and F5 refresh wasn't bringing in your changes to the category, although curiously it was bringing your updated comments.

For safety reasons I'm now navigating away and back again before adding comments.

Comment 8

12 years ago
> 1) FPU accuracy changes in plugin affect FPU accuracy of Firefox.

Let's call this a bug in the plugin. Plugins really shouldn't be changing the FPU accuracy in somebody else's process.

> 2) Calling js_dtoa with single-precision FPU accuracy can cause a buffer
> overrun.

If we can safely say that Firefox should have control over its FPU precision, then this is merely a design choice, not a bug ;-)

I think we should open this bug up unless there's evidence that FPU precision is not per-process or that attackers can modify the Firefox FPU precision from outside of compiled code (plugins).
(Reporter)

Comment 9

12 years ago
> Plugins really shouldn't be changing the FPU accuracy in somebody else's process.

It depends on how much you want to 'sandbox' against dodgy plugin behaviour. Running plugins in their own thread, for example, would prevent this happening. Having settings in a plugin able to affect the parent like this just feels... unpleasant. 

> I think we should open this bug up unless there's evidence that FPU precision
is not per-process or that attackers can modify the Firefox FPU precision from
outside of compiled code (plugins).

The only vulnerable people I can think of would be those who had a plugin installed that 'leaked' fpu settings, although as this is new behaviour there could well be plugins that used to work, but which are now dodgy. Obviously anyone could then modify the FPU precision by designing a web-page that called that plugin. Does seem pretty unlikely, though.
We're obviously NOT worried about malicious plugins -- once the plugin is loaded it's game over if you don't trust the plugin. But it's not far-fetched to think that some game-playing or video plugin might be changing the FPU setting for performance reasons (on low-end machines for example, or as a play-back quality setting that some PC games have) without realizing it has any effect on Javascript.

The fact that this bug exhibits itself on tab changes is interesting but irrelevant. If an attacker knew a potential victim had a vulnerable plugin ("if (navigator.plugins['VulnerablePluginName']) /* attack */") there are all kinds of ways to force this conversion and overwrite the buffer. We're given the buffer size passed to js_dtoa, there's no excuse not to be checking that we fit. bufsize is checked on line 2262 and then not checked again until 460 lines later (l. 2723), and even then it's just an assert
Assignee: general → brendan
Status: UNCONFIRMED → NEW
Ever confirmed: true
Blake is taking this one for the team.

/be
Assignee: brendan → mrbkap
Whiteboard: [sg:moderate] req's plugin
(Reporter)

Comment 12

12 years ago
Is this going to fix just the js_dtoa() overrun, or actually isolate the plugin fpu settings from the main app fpu settings? 

There may be many functions that assume, entirely understandably, higher FPU accuracy, and js_dtoa() may just have been the one causing the crash in this case. Fix that one, and another might just pop up. 

The inherent problem is that a faulty plugin can 'leak' FPU settings across to the main application, changing the entire application context and possibly undermining a fundamental assumption of the entire codebase, ie that FPU accuracy is fixed, and under the application developer's control, when it is actually 'volatile'.
Keywords: crash
(Assignee)

Comment 13

11 years ago
Wasn't this fixed by bug 358569?
It was (and on branches, too).
Group: security
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 358569
You need to log in before you can comment on or make changes to this bug.