1238440 - (CVE-2016-1963) Firefox crashes when modifying a file read by FileReader through file input

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Oriol Brufau [:Oriol]]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:46.0) Gecko/20100101 Firefox/46.0
Build ID: 20160110030214

Steps to reproduce:

1. Make sure devtools.chrome.enabled is enabled and e10s is disabled
2. Open the browser console
3. Enter the following code

var encoder = new TextEncoder(),
    text1 = encoder.encode("hello world"),
    text2 = encoder.encode("hello world-----------------------------"),
    fileName = OS.Constants.Path.tmpDir + "\\---bug-crash-test-file.txt";
OS.File.writeAtomic(fileName, text1, {tmpPath: fileName + ".tmp"}).then(function() {
  var browser = gBrowser.getBrowserForTab(gBrowser.addTab());
  browser.addEventListener("load", function() {
    var doc = browser.contentDocument;
    doc.body.innerHTML = '<input type="file" />';
    var file = doc.querySelector('input');
    file.value = fileName;
    function readFile(callback) {
      var fr = new FileReader();
      fr.onload = function() {
        var data = fr.result;
        var array = new Int8Array(data);
        if(callback) callback();
      };
      fr.readAsArrayBuffer(file.files[0]);
    }
    readFile(function() {
      OS.File.writeAtomic(fileName, text2, {tmpPath: fileName + ".tmp"}).then(function() {
        readFile();
      });
    });
  }, true);
});


Actual results:

Sometimes Firefox crashes instantly.
Sometimes you must wait a bit (like 10 seconds).
Sometimes you must enter that code multiple times (like 5 times).
But eventually it always crashes.


Expected results:

Firefox shouldn't crash.

Note I used the privileged console in order to set the value of the input automatically. But the crash also happens when selecting the file manually in non-privileged pages.

Some crash logs:

https://crash-stats.mozilla.com/report/index/855b6114-b774-4971-b18b-68eb22160111
https://crash-stats.mozilla.com/report/index/dfeaa675-5868-432f-a9fd-b703f2160111
https://crash-stats.mozilla.com/report/index/42ae483a-aeeb-402f-baed-5a6762160111
https://crash-stats.mozilla.com/report/index/075415c1-c727-4b2d-952e-3b0f72160111
https://crash-stats.mozilla.com/report/index/38405638-19c4-428e-b1df-2cf4a2160111
https://crash-stats.mozilla.com/report/index/0c435e44-4876-4440-988e-5c9102160111

Crashes reproducible on WinXP and Win7. Nighty and Stable.

Comment 1

[YF (Yang)]

WFM on Fx44b7 Win10, crash with Fx46.0a1, a different crash signature.

Comment 2

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

Slightly different crash signatures but several in graphics.

Comment 3

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

[Tracking Requested - why for this release]: Crash confirmed by YF in F46 Nightly

Comment 4

[Liz Henry (:lizzard) (relman/hg->git project)]

Crash signatures are from 45, 44 is unaffected.

Comment 5

[u279076]

(In reply to YF (Yang) from comment #1)
> WFM on Fx44b7 Win10, crash with Fx46.0a1, a different crash signature.

Could you please narrow this down to a specific day's Nightly build?

Comment 6

[YF (Yang)]

Unfortunately, I tried the mozregression tool later, it gets a bigger range (e.g. 41.0a1) and sometimes crash without crash reporter. And, I can reproduce this problem in Fx44b7 if run repeatedly now.

I need a testcase with one page, it will be more clearly shows the purpose.

Comment 7

[Oriol Brufau [:Oriol]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #4)
> Crash signatures are from 45, 44 is unaffected.

I made the crash script from code of http://stackoverflow.com/q/32215538
The poster of that question said he ran into lots of crashes. That dates August 2015, so the issue precedes Firefox 44.

And I can reproduce the crash on 43 and 44
https://crash-stats.mozilla.com/report/index/de4174de-241e-4b18-973b-ae9e82160113
https://crash-stats.mozilla.com/report/index/248e6b2a-cb30-420a-b237-f63942160113

(In reply to YF (Yang) from comment #6)
> I need a testcase with one page, it will be more clearly shows the purpose.
What does "testcase with one page" mean?

Comment 8

[Oriol Brufau [:Oriol]]

I could reproduce a crash with 2013-01-14 nightly. I'm starting to think this bug exists since the implementation of FileReader.
But it's hard to test those too old builds because they don't have a browser console, and the manual steps (descrived in bug 1198095 comment 0) are tedious and seem to have a smaller likelihood to crash, so you must repeat them several times.

Comment 9

[YF (Yang)]

I don't know how to reproduce the "But the crash also happens when selecting the file manually in non-privileged pages.", I hope it can be reproduced with a testcase page without command.

Comment 10

[Oriol Brufau [:Oriol]]

(In reply to YF (Yang) from comment #9)
> I don't know how to reproduce the "But the crash also happens when selecting
> the file manually in non-privileged pages.", I hope it can be reproduced
> with a testcase page without command.

The manual steps are the same as bug 1198095 comment 0. Basically,

1. Create a file with some content, e.g. hello world
2. Go to https://jsfiddle.net/Lv5y9m2u/4/
3. Click the file input and choose the file created at step 1
4. Change the contents of the file, e.g. to hello world-----------------------------

Probably you will have to repeat a few times.

Comment 11

[Oriol Brufau [:Oriol]]

By the way, the crash is also reproducible on Linux, but use / instead of \\ in the file path:

    fileName = OS.Constants.Path.tmpDir + "/---bug-crash-test-file.txt";

(In reply to Emma Humphries [:emceeaich] (GMT-8) NEEDINFO? me from comment #2)
> Slightly different crash signatures but several in graphics.

The crash does not happen immediately, so I think it's a GC related crash, and that's why each signature is different. That several were in graphics was just by chance. I don't think this bug is related to graphics.

Comment 12

[YF (Yang)]

With comment 10, it is sometimes stopped to refresh, sometimes crash (multiple signatures).

Comment 13

[Oriol Brufau [:Oriol]]

I have found a way to reproduce the problem on old builds which don't have the browser console:

1. Open a new tab
2. Open the web console (it will be privileged because it's in a new tab)
3. Enter this code:

var gBrowser = Components.classes["@mozilla.org/appshell/window-mediator;1"]
               .getService(Components.interfaces.nsIWindowMediator)
               .getMostRecentWindow("navigator:browser").gBrowser;
var OS = Components.utils.import("resource://gre/modules/osfile.jsm", {}).OS;

4. Enter code from comment 0
5. Repeat until it crashes

Strangely, the code seems to work if I enter it in the error console, but there it doesn't crash.

Using these steps I can reproduce the crash on 2012-10-04 nightly.
I couldn't test previous builds because OS.File.writeAtomic doesn't work.

Comment 14

[Oriol Brufau [:Oriol]]

OK, I rebuilt my script in order to make it work on old Firefox.

1. Open new tab
2. Open web console
3. Enter this code

Components.utils.import("resource://gre/modules/NetUtil.jsm");
Components.utils.import("resource://gre/modules/FileUtils.jsm");
var gBrowser = Components.classes["@mozilla.org/appshell/window-mediator;1"]
               .getService(Components.interfaces.nsIWindowMediator)
               .getMostRecentWindow("navigator:browser").gBrowser,
    file = FileUtils.getFile("TmpD", ["---bug-crash-test-file.txt"]),
    converter = Components.classes["@mozilla.org/intl/scriptableunicodeconverter"]
                .createInstance(Components.interfaces.nsIScriptableUnicodeConverter);
converter.charset = "UTF-8";
function writeFile(text, callback) {
  var ostream = FileUtils.openSafeFileOutputStream(file);
  var istream = converter.convertToInputStream(text);
  NetUtil.asyncCopy(istream, ostream, function(status) {
    if (!Components.isSuccessCode(status)) throw 'fail';
    callback();
  });
}
writeFile("hello world", function() {
  var browser = gBrowser.getBrowserForTab(gBrowser.addTab());
  browser.addEventListener("load", function() {
    var doc = browser.contentDocument;
    doc.body.innerHTML = '<input type="file" />';
    var input = doc.querySelector('input');
    input.value = file.path;
    function readFile(callback) {
      var fr = new FileReader();
      fr.onload = function() {
        var data = fr.result;
        var array = new Int8Array(data);
        if(callback) callback();
      };
      fr.readAsArrayBuffer(input.files[0]);
    }
    readFile(function() {
      writeFile("hello world-----------------------------", readFile);
    });
  }, true);
});

4. Repeat until crash

With this I could crash 2012-09-01 Nightly, and 2012-08-31 didn't crash even after lots of tries.

Then, the pushlog is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fcc533f691e9&tochange=a21fd4d085ad

In that pushlog there is bug 786196, which is related to files. It might be the culprit.

Comment 15

[u279076]

(In reply to Oriol from comment #14)
> The pushlog is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fcc533f691e9&tochange=a21fd4d085ad
> 
> In that pushlog there is bug 786196, which is related to files. It might be the culprit.

Flagging :Luqman to check if the patch in bug 786196 could have caused this issue. Based on the range above this goes back to Firefox 18 so I don't think it needs to be tracked.

Comment 16

[Oriol Brufau [:Oriol]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #15)
> Flagging :Luqman to check if the patch in bug 786196 could have caused this issue.
I'm not much convinced it was that bug, but I don't know enough to make a better guess.
:Luqman doesn't seem active on bugzilla since 2013, so flagging :Yoric, the mentor who reviewed the patch in that bug.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Most likely unrelated to bug 786196, as that code isn't triggered during the execution of the sample. Actually, I'm almost sure that this code is not triggered at all in vanilla Firefox.

If it's memory access, it might be related to bug 785167, bug 720949, bug 786903 – I think that this piece of code is executed by all the samples.

Of course, this could be an older issue with OS.File or FileReader that was triggered by any of the patches in the changeset :/

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Any chance that this could be reproduced on an Asan or Valgrind build? If this is a memory error, this is probably the best way to corner it.

Comment 19

[Andrea Marchesini [:baku]]

I can reproduce this crash. This seems to be a FileReader bug. I'll take a look tomorrow.

Comment 20

[Luqman Aden [:Luqman]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #15)
> (In reply to Oriol from comment #14)
> > The pushlog is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=fcc533f691e9&tochange=a21fd4d085ad
> > 
> > In that pushlog there is bug 786196, which is related to files. It might be the culprit.
> 
> Flagging :Luqman to check if the patch in bug 786196 could have caused this
> issue. Based on the range above this goes back to Firefox 18 so I don't
> think it needs to be tracked.

As mentioned above, I don't think that the patch from bug 786196 is the cause. At least not directly since it only defines |File.prototype.flush| and doesn't actually use it anywhere outside of the attached test case.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: crash3.patch

Comment 22

[Andrew McCreight [:mccr8]]

Doesn't the FILE_AS_BINARY case have a similar issue when it does "if (uint64_t(oldLen) + aCount > UINT32_MAX)"?

Is this class exposed to content somehow, so a regular webpage could trigger this overflow?

Comment 23

[Andrea Marchesini [:baku]]

(In reply to Andrew McCreight [:mccr8] from comment #22)
> Doesn't the FILE_AS_BINARY case have a similar issue when it does "if
> (uint64_t(oldLen) + aCount > UINT32_MAX)"?

My test covers FILE_AS_BINARY because all the others are already covered by bug 1198095.

> Is this class exposed to content somehow, so a regular webpage could trigger
> this overflow?

Well, the user can do these steps manually editing a file that has been already used as Blob and then, the same blob used as input for a FileReader.

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8710481 [details] [diff] [review]
crash3.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't know, but for sure we are writing things in a non allocated buffer and the input is taken from a local file.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Absolutely, and a test is included.

Which older supported branches are affected by this flaw?

All?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Easy to backport.

How likely is this patch to cause regressions; how much testing does it need?

No regressions here.

Comment 25

[Al Billings [:abillings - ex-MoCo]]

This needs a security rating before it can get sec-approval. Dan?

Comment 26

[Daniel Veditz [:dveditz]]

What uses 0x2d2d2d2d as a fill pattern? most of the stacks in comment 0 and comment 7 show that.

It looks like memory corruption, but this functionality is limited to chrome code and it only happens if the user (or some other process) is changing the file locally so this couldn't be used in a remote exploit (at least not without a second, worse, "run code as chrome" bug). I'll call this sec-moderate

Comment 27

[Andrea Marchesini [:baku]]

Comment on attachment 8710481 [details] [diff] [review]
crash3.patch

no sec-approval needed for sec-moderate.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e3ec255c810

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Baku, Dan: This landed on Nightly. Is this something we should also to uplift Beta45 and Aurora46?

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8710481 [details] [diff] [review]
crash3.patch

Approval Request Comment
[Feature/regressing bug #]: FileReader
[User impact if declined]: A crash can occur. Very hard to reproduce.
[Describe test coverage new/current, TreeHerder]: test included
[Risks and why]: none
[String/UUID change made/needed]: none

In case this patch doesn't apply correctly, let me know and I provide a new version of it for m-a/m-b.

Comment 31

[Daniel Veditz [:dveditz]]

Yes, we should uplift this. Definitely to aurora and looks more than safe enough for beta.

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8710481 [details] [diff] [review]
crash3.patch

Many crashes with many signatures, has a test, taking it.
Should be in 45 beta 9

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6ba7aee0e87b

Comment 34

[Carsten Book [:Tomcat]]

has problems uplifting to beta 

warning: conflicts while merging dom/base/FileReader.cpp! (edit, then use 'hg resolve --mark')

Comment 35

[Andrea Marchesini [:baku]]

Attachment: m-b

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c66d572199a0

Comment 37

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on Nightly 2016-01-19 using the STR in comment 0.
Verified fixed FX 45, 46.0a2 (2016-03-02), 47.0a1 (2016-03-02), ESR 45 (2016-03-02) Win 7.