Closed Bug 1216130 (CVE-2015-7204) Opened 9 years ago Closed 8 years ago

Simple var assignments can trigger "can't convert undefined to object" exception

Categories

(Core :: JavaScript Engine, defect)

41 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox41 --- wontfix
firefox42 --- wontfix
firefox43 + verified
firefox44 + fixed
firefox45 + fixed
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- affected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: cajus, Assigned: jandem)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main43+])

Attachments

(2 files)

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151015125802

Steps to reproduce:

With the change from Firefox 40.0.3 to 41.x (at least Linux/Windows), we're seeing issues where simple assignments are causing the mentioned exception. I was not able to extract a certain piece of code that is able to trigger the error, but maybe someone has a clue what changed between the mentioned versions in order to track it down.

Here's a code snipped from the qooxdoo framework (https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/data/SingleValueBinding.js#L152):

        // go through all property names
        for (var i = 0; i < propertyNames.length; i++) {
          ...
        }

The same error triggers at random other places where no real error can be seen from within the debugger. i.E. same file: https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/data/SingleValueBinding.js#L141.

There's a ticket on qooxdoo side with information about it: http://bugzilla.qooxdoo.org/show_bug.cgi?id=9219

Some people seem to confirm that it is related to a problem with "Dynamic Script Execution Order", but I cannot confirm this on our side. It does not fix it.


Actual results:

Firefox stops when processing "var i = 0;" while the debugger column pointer points to the "i". An exception is launched claiming: "can't convert undefined to object".

propertyNames is a non sparse array with exactly one string element in position "0" in the case where it bails out. Its length is "1". While I can reproduce this one in 100% of all cases, I can't easily share the application, because it needs some backend data to run. If I can provide more information, please let me know.


Expected results:

The index i should be assigned with "0" and the loop should just run.
If you can reliably reproduce, would you be willing to try http://mozilla.github.io/mozregression/ to narrow down the commit range when the problem was introduced?
Flags: needinfo?(cajus)
Yes. I was not aware of that tool. Nice. Trying out right now.
Flags: needinfo?(cajus)
31:28.72 LOG: MainThread Bisector INFO Narrowed inbound regression window from [e5d045d4, 32248713] (4 revisions) to [f8d7bb0b, 32248713] (2 revisions) (~1 steps left)
31:28.72 LOG: MainThread main INFO Oh noes, no (more) inbound revisions :(
31:28.72 LOG: MainThread Bisector INFO Last good revision: f8d7bb0b4f00bb424792f6a93ecf9a8d1669ed93
31:28.72 LOG: MainThread Bisector INFO First bad revision: 322487136b28a0c136642d39b8fa7091f1c55dee
31:28.72 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f8d7bb0b4f00bb424792f6a93ecf9a8d1669ed93&tochange=322487136b28a0c136642d39b8fa7091f1c55dee
Thanks, that rather narrows it down.  ;)

Brian, Jan, could you take a look, please?
Blocks: 1162199
Flags: needinfo?(jdemooij)
Flags: needinfo?(bhackett1024)
I can look at this, but I need some way to reproduce the problem on my own machine.
I can arrange that. Will send you the information to your mail address privately.
You should have some information now. Sadly the provided version does not reproduce the error as quick as the developer version does. Let me know if you need further information/assistance.
(In reply to Cajus Pollmeier from comment #7)
> You should have some information now. Sadly the provided version does not
> reproduce the error as quick as the developer version does. Let me know if
> you need further information/assistance.

Hi, I've tried a couple different builds on OS X 10.9 and have not been able to reproduce the error.  Having access to a version that is 100% reproducible would be nice, but other than that there are a few things to try:

1. Can you try setting the JIT_OPTION_disableUnboxedObjects environment variable to 'true' in a recent nightly and see if the error reproduces?  This will at least determine whether unboxed objects are really the problem here.

2. The debugger might be getting confused about where the exception was thrown from (that 'var i = 0' is the first instruction in the try block).  What happens if you remove the try/catch blocks?

3. Unboxed objects are used for literals like { x:0, y:1 } and objects created by constructors like 'new Foo()'.  If these objects are given inconsistent types for any of their properties (like writing 'a.f = 0; a.f = someObject') then they won't use an unboxed representation.  This can act as a workaround if I'm not able to reproduce the error and the problematic object can be determined.
(In reply to Brian Hackett (:bhackett) from comment #8)
> Hi, I've tried a couple different builds on OS X 10.9 and have not been able
> to reproduce the error.  Having access to a version that is 100%
> reproducible would be nice, but other than that there are a few things to
> try:
> 
> 1. Can you try setting the JIT_OPTION_disableUnboxedObjects environment
> variable to 'true' in a recent nightly and see if the error reproduces? 
> This will at least determine whether unboxed objects are really the problem
> here.

The error is not reproducible with JIT_OPTION_disableUnboxedObjects=true, it is reproducible using JIT_OPTION_disableUnboxedObjects=false. Even if it does not really help with debugging, I've shot screencasts of these cases:

https://ferdi.naasa.net/oc/index.php/s/MWW5bDQb4NxNaJL
https://ferdi.naasa.net/oc/index.php/s/RdOWfw8q5rUxRg9

On OS/X you may need vlc to play ogv. I can't check in the moment.

BTW: I'm not able to load the source/debug version with the Firefox nightly, so the software is the same you're getting from test box I've provided.

> 2. The debugger might be getting confused about where the exception was
> thrown from (that 'var i = 0' is the first instruction in the try block). 
> What happens if you remove the try/catch blocks?

If I remove the try/catch block, it stops here:

https://github.com/qooxdoo/qooxdoo/blob/master/framework/source/class/qx/data/SingleValueBinding.js#L141

with the same message. Which is interestingly outside the try/catch block. The return value of the called method can be evaluated in the console and returns and array: [""]

> 3. Unboxed objects are used for literals like { x:0, y:1 } and objects
> created by constructors like 'new Foo()'.  If these objects are given
> inconsistent types for any of their properties (like writing 'a.f = 0; a.f =
> someObject') then they won't use an unboxed representation.  This can act as
> a workaround if I'm not able to reproduce the error and the problematic
> object can be determined.

Hmm. I don't see how I could use this as a workaround. I'm getting many tickets these days, all with the same exception triggered from various places of the application. So the only solution for now is to blacklist Firefox version > 40.

Anyway - if I can do anything to track this down (playing with the debugger, changing code, compiling, whatever), please let me know. Thanks!
I don't know if that helps: it was important to clear the cache to get the error initially in the screencasts. When I didn't to it, it took some time to run into it.

The error is not related to the first menu entry. It is just a relative good way to reproduce. Randomly navigating thru the application, selecting/editing things will create that exception sooner or later.
Managed to do load the debug version. Here's a shot with that version and the try/catch commented out:

https://ferdi.naasa.net/oc/index.php/s/YdCFjepPxwvN4mQ
Are there any news on this one? I'm having a similar problem, probably caused by the same reason, as the bisector tool yields the same result as in comment #3. The error seems to be thrown at the same lines in SingleValueBinding.js so it's likely to be the same bug.

Unfortunately, it's really inconsistend to reproduce, so I'm afraid I can't provide anything helpful...
For those who can reproduce this, does it fail more reliably if you go to about:config, search for the option below, set it to false and restart Firefox?

javascript.options.ion.offthread_compilation
Flags: needinfo?(cajus)
Flags: needinfo?(bader)
(In reply to Jan de Mooij [:jandem] from comment #13)
> For those who can reproduce this, does it fail more reliably if you go to
> about:config, search for the option below, set it to false and restart
> Firefox?
> 
> javascript.options.ion.offthread_compilation

Does not seem to make any difference in my application.
Flags: needinfo?(bader)
Hm okay, thanks. It'd be very useful if you (or Cajus) could give me access to that page..
Sadly, I'm not allowed to. But from #6 and #7 I learn that Cajus made his accessible?
(In reply to Jan de Mooij [:jandem] from comment #15)
> It'd be very useful if you (or Cajus) could give me access to that page..

Send you an email with the neccessary information.
I was able to reproduce and debug this with some effort. Minimal testcase below. Thanks a lot for the bug report!

The problem is that StoreUnboxedObjectOrNullPolicy inserts MToObjectOrNull if the value isn't known to be object/null. MToObjectOrNull calls ToObjectSlow, which throws an exception for `undefined`. This was probably done for typed objects but is bogus for unboxed objects.

Fixing this should be pretty straight-forward now, so I might as well take this.

function f() {
    var arr = [], p = {};
    for (var i=0; i<5000; i++) {
	var x = (i < 4700) ? p : undefined;
	var o = {x: x};
	if (i < 5) {
	    o.x = undefined;
	    o.x = p;
	}
	arr.push(o);
    }
}
f();
Assignee: nobody → jdemooij
Flags: needinfo?(cajus)
Flags: needinfo?(bhackett1024)
@Jan: good to know that you can reproduce it. I would have been lucky to be the one to provide you these 13 lines of code to trigger the error, but if you've really no idea what it could come from, it starts to be a bit complicated...
Marking s-s just to be sure. I was unable to write a test that asserts/crashes but it's complicated enough that someone else might find one.
Group: javascript-core-security
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: sec-high
Attached patch Part 1 - Fix bugSplinter Review
The problem is that PropertyWriteNeedsTypeBarrier does some filtering of values stored to unboxed object properties, but we didn't do this if we called TryAddTypeBarrierForWrite and broke out of the loop.

The patch moves this check into a separate loop so that we always execute it for all object groups.
Flags: needinfo?(jdemooij)
Attachment #8679962 - Flags: review?(bhackett1024)
Attachment #8679967 - Flags: review?(bhackett1024)
[Tracking Requested - why for this release]:
(In reply to Cajus Pollmeier from comment #19)
> I would have been lucky to be
> the one to provide you these 13 lines of code to trigger the error, but if
> you've really no idea what it could come from, it starts to be a bit
> complicated...

For sure. The test looks simple but it relies on implementation details and heuristics and it took me some time to write it, for someone not familiar with the engine it'd be much harder :)

Thanks again for the bug report and other info! FF 42 will be released next week so we're probably too late for that, we should be able to fix this in 43 though.
Comment on attachment 8679962 [details] [diff] [review]
Part 1 - Fix bug

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

Thanks Jan!
Attachment #8679962 - Flags: review?(bhackett1024) → review+
Attachment #8679967 - Flags: review?(bhackett1024) → review+
Comment on attachment 8679962 [details] [diff] [review]
Part 1 - Fix bug

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
Not easily. I was unable to come up with a test that crashed, but it doesn't mean it's not possible.

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

* Which older supported branches are affected by this flaw?
41+

* If not all supported branches, which bug introduced the flaw?
Bug 1162199.

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be very easy to backport and low risk: the patch just moves some code in one loop into its own loop.

* How likely is this patch to cause regressions; how much testing does it need?
Unlikely. One or two Nightlies and the usual fuzzing should be enough.
Attachment #8679962 - Flags: sec-approval?
Tracking for 43+ since this is sec-high.
Sec-approval+ for checkin on Nov 17, two weeks into the cycle. This is to reduce the time window of vulnerability after the checkin.
Whiteboard: [checkin on 11/17]
Attachment #8679962 - Flags: sec-approval? → sec-approval+
Comment on attachment 8679962 [details] [diff] [review]
Part 1 - Fix bug

Approval Request Comment
[Feature/regressing bug #]: Bug 1162199.
[User impact if declined]: Correctness bugs and potential security bugs.
[Describe test coverage new/current, TreeHerder]: Fixes the testcase + broken website.
[Risks and why]: Low risk. Some code is moved out of a loop and into its own loop.
[String/UUID change made/needed]: None.
Attachment #8679962 - Flags: approval-mozilla-beta?
Attachment #8679962 - Flags: approval-mozilla-aurora?
Whiteboard: [checkin on 11/17]
https://hg.mozilla.org/mozilla-central/rev/6624bab7b2dc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Group: javascript-core-security → core-security-release
Can you please explain what target milestone -> mozilla45 means? Does that mean that I get customer reports until april/may 2016 that the application crashes in firefox without having any chance to work around the problem?
(In reply to Cajus Pollmeier from comment #32)
> Can you please explain what target milestone -> mozilla45 means? Does that
> mean that I get customer reports until april/may 2016 that the application
> crashes in firefox without having any chance to work around the problem?

I requested approval to uplift this to Firefox 43 and 44. Firefox 43 will be released December 15th, so hopefully the fix will make it into that release.
(In reply to Cajus Pollmeier from comment #32)
> without having any chance to work around the problem?

Oh and one way to work around it is to go to about:config, enter javascript.options.ion in the search bar and toggle it to 'false'.
Jan, thanks for the reply. Since this is my "first time firefox bug", I was just unsure what these tags mean in practice. Suspending to desk and started hoping ;-)
(In reply to Cajus Pollmeier from comment #35)
> Jan, thanks for the reply. Since this is my "first time firefox bug", I was
> just unsure what these tags mean in practice.

No worries and thanks for reporting this bug!

(I forgot to mention, setting javascript.options.ion to 'false' disables the optimizing JIT compiler for all websites, so it will make certain websites/applications slower...)
Comment on attachment 8679962 [details] [diff] [review]
Part 1 - Fix bug

Fix for javascript issue, has had some user testing, ok to uplift to aurora and beta.
Attachment #8679962 - Flags: approval-mozilla-beta?
Attachment #8679962 - Flags: approval-mozilla-beta+
Attachment #8679962 - Flags: approval-mozilla-aurora?
Attachment #8679962 - Flags: approval-mozilla-aurora+
Reproduce with the minimal testcase via comment 18 on 42.0 RC build 2 → ‘TypeError: can't convert undefined to object’ thrown by _display:28:6 is visible in the Browser Console.

Verified fixed with 43.0b9 build 2 (Build ID: 20151203163240), across platforms [1].

[1] Windows 7 64-bit, Mac OS X 10.11 and Ubuntu 14.04 32-bit
Whiteboard: [adv-main43+]
Alias: CVE-2015-7204
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.