Bug 1238440 (CVE-2016-1963)

Firefox crashes when modifying a file read by FileReader through file input

VERIFIED FIXED in Firefox 45

Status

()

--
critical
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: Oriol, Assigned: baku)

Tracking

(5 keywords)

18 Branch
mozilla47
crash, csectype-bounds, regression, sec-moderate, testcase
Points:
---

Firefox Tracking Flags

(firefox42 wontfix, firefox43 wontfix, firefox44 wontfix, firefox45 verified, firefox46 verified, firefox47 verified, firefox-esr38 wontfix, firefox-esr45 verified)

Details

(Whiteboard: [adv-main45+], URL)

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
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

3 years ago
WFM on Fx44b7 Win10, crash with Fx46.0a1, a different crash signature.
Severity: normal → critical
Component: Untriaged → Untriaged
Keywords: crash
Product: Firefox → Core
Slightly different crash signatures but several in graphics.
Component: Untriaged → Graphics
[Tracking Requested - why for this release]: Crash confirmed by YF in F46 Nightly
Status: UNCONFIRMED → NEW
status-firefox46: --- → affected
tracking-firefox46: --- → ?
Ever confirmed: true
Keywords: regressionwindow-wanted
Crash signatures are from 45, 44 is unaffected.
status-firefox44: --- → unaffected
status-firefox45: --- → affected
Component: Graphics → Graphics: Layers
(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?
Flags: needinfo?(yfdyh000)

Comment 6

3 years ago
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.
Flags: needinfo?(yfdyh000) → needinfo?(oriol-bugzilla)
(Reporter)

Comment 7

3 years ago
(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?
(Reporter)

Updated

3 years ago
Flags: needinfo?(yfdyh000)
(Reporter)

Comment 8

3 years ago
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

3 years ago
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.
Flags: needinfo?(yfdyh000)
(Reporter)

Comment 10

3 years ago
(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.
Flags: needinfo?(oriol-bugzilla)
(Reporter)

Comment 11

3 years ago
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

3 years ago
With comment 10, it is sometimes stopped to refresh, sometimes crash (multiple signatures).
status-firefox43: --- → ?
status-firefox44: unaffected → affected
Component: Graphics: Layers → DOM
Keywords: testcase
(Reporter)

Comment 13

3 years ago
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.
(Reporter)

Comment 14

3 years ago
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.
(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.
status-firefox42: --- → wontfix
status-firefox43: ? → affected
status-firefox-esr38: --- → wontfix
status-firefox-esr45: --- → affected
tracking-firefox46: ? → ---
Flags: needinfo?(me+bugzilla)
Keywords: regressionwindow-wanted → regression
Version: 46 Branch → 18 Branch
(Reporter)

Comment 16

3 years ago
(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.
Flags: needinfo?(dteller)
(Assignee)

Updated

3 years ago
Group: dom-core-security
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 :/
Flags: needinfo?(dteller)
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.
(Assignee)

Comment 19

3 years ago
I can reproduce this crash. This seems to be a FileReader bug. I'll take a look tomorrow.
Assignee: nobody → amarchesini
(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.
Flags: needinfo?(me+bugzilla)
(Assignee)

Comment 21

3 years ago
Created attachment 8710481 [details] [diff] [review]
crash3.patch
Attachment #8710481 - Flags: review?(khuey)
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?
Keywords: csectype-bounds
(Assignee)

Comment 23

3 years ago
(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.
(Assignee)

Comment 24

3 years ago
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.
Attachment #8710481 - Flags: sec-approval?
This needs a security rating before it can get sec-approval. Dan?
status-firefox43: affected → wontfix
status-firefox44: affected → wontfix
Flags: needinfo?(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
Flags: needinfo?(dveditz)
Keywords: sec-moderate
(Assignee)

Comment 27

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

no sec-approval needed for sec-moderate.
Attachment #8710481 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/2e3ec255c810
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Group: dom-core-security → core-security-release
Baku, Dan: This landed on Nightly. Is this something we should also to uplift Beta45 and Aurora46?
Flags: needinfo?(dveditz)
Flags: needinfo?(amarchesini)
(Assignee)

Comment 30

3 years ago
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.
Flags: needinfo?(amarchesini)
Attachment #8710481 - Flags: approval-mozilla-beta?
Attachment #8710481 - Flags: approval-mozilla-aurora?
Yes, we should uplift this. Definitely to aurora and looks more than safe enough for beta.
Flags: needinfo?(dveditz)
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
Attachment #8710481 - Flags: approval-mozilla-beta?
Attachment #8710481 - Flags: approval-mozilla-beta+
Attachment #8710481 - Flags: approval-mozilla-aurora?
Attachment #8710481 - Flags: approval-mozilla-aurora+
has problems uplifting to beta 

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

Comment 35

3 years ago
Created attachment 8722105 [details] [diff] [review]
m-b
Flags: needinfo?(amarchesini) → needinfo?(cbook)
https://hg.mozilla.org/releases/mozilla-beta/rev/c66d572199a0
status-firefox45: affected → fixed
Flags: needinfo?(cbook)
status-firefox-esr45: affected → fixed
Whiteboard: [adv-main45+]
Alias: CVE-2016-1963
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.
Status: RESOLVED → VERIFIED
status-firefox45: fixed → verified
status-firefox46: fixed → verified
status-firefox47: fixed → verified
status-firefox-esr45: fixed → verified
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.