898832 - crash in mozilla::dom::NodeBinding::get_childNodes

Status: VERIFIED FIXED

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

Description

[Scoobidiver (away)]

It's #48 browser crasher in 23.0b8 and #15 in the first day of 23.0b9.

Using mozregression, the regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b980d32c366f&tochange=ea059733677c
and the working range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d2a7cfa34154&tochange=8ea92aeab783
It might be fixed by bug 884401's patch.

Signature 	mozilla::dom::NodeBinding::get_childNodes More Reports Search
UUID 	e4aa87e5-24e7-4e0e-8da1-db5842130728
Date Processed	2013-07-28 08:05:27.125889
Uptime	22
Last Crash	27 seconds before submission
Install Age 	51551 since version was first installed.
Install Time 	2013-07-27 17:46:07
Product 	Firefox
Version 	23.0
Build ID 	20130725195523
Release Channel 	beta
OS 	Windows NT
OS Version 	6.1.7601 Service Pack 1
Build Architecture 	x86
Build Architecture Info 	GenuineIntel family 6 model 23 stepping 10 | 2
Crash Reason 	EXCEPTION_ACCESS_VIOLATION_EXEC
Crash Address 	0x12143c20
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x2a42, AdapterSubsysID: 02051025, AdapterDriverVersion: 8.15.10.2202
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 

Frame 	Module 	Signature 	Source
0 		@0x12143c20 	
1 	xul.dll 	mozilla::dom::NodeBinding::get_childNodes 	obj-firefox/dom/bindings/NodeBinding.cpp
2 	mozjs.dll 	ToLowerCaseHelper 	js/src/jsstr.cpp
3 		@0x1 	
4 		@0x1f8c03bc

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Adom%3A%3ANodeBinding%3A%3Aget_childNodes

Comment 1

[Boris Zbarsky [:bzbarsky]]

That stack is not useful.... :(

Comment 2

[Alice0775 White]

Regression window
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/c9737a4136cf
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130507 Firefox/23.0 ID:20130507225450
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e698d3534b96
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20130507 Firefox/23.0 ID:20130508010349
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c9737a4136cf&tochange=e698d3534b96

In local build(ubuntu)
Last Good: ef2134c93dae
First Bad: 92a5a4a9b76b

Regressed by:
92a5a4a9b76b	Hannes Verschore — Bug 864468: IonMonkey: Skip argument type checks when type is known to match, r=jandem

Working window
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5458a7880db5
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130619 Firefox/24.0 ID:20130619062021
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/259e68f8843d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20130619 Firefox/24.0 ID:20130619072522
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5458a7880db5&tochange=259e68f8843d

Fixed by 
 259e68f8843d	Andrea Marchesini — Bug 841442 - Move HTMLFormElement to WebIDL, r=bz

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  That's very odd...  What testcase did you use to reproduce this?  Were we incorrectly invoking the specialized getter on a form in some cases???

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, nevermind, the testcase is in the url field; it's just not mentioned in the comments.

So comment 3 is correct: we're invoking mozilla::dom::NodeBinding::get_childNodes directly from jitcode but 'obj' is an XPConnect object (in particular it has obj->getClass()->addProperty == XPC_WN_Helper_AddProperty and so forth).  It's an XPConnect wrapper for a <form>: it has obj->getClass()->name as "HTMLFormElement".

Jandem, Hannes, I would expect that this is exploitable if one tries hard enough... how are we managing to fail the TI bits here?  Furthermore, I would expect it to be possible to write exploits for trunk and 24; just have to get non-DOM objects through this codepath.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and I don't understand how bug 864468 would trigger this, since we're dealing with a property getter, not a method call... unless the problem is that we're now screwing up the types of things inside a method somehow?

Comment 6

[Jan de Mooij [:jandem]]

Hannes can you take a look?

Comment 7

[Hannes Verschore [:h4writer]]

Attachment: bug898832-debug-argument-check

I created a patch that will assert in debug build if we end up with wrong type information after skipping the argument check. This should make it possible for fuzzers to find quite easily if Bug 864468 is indeed the blame and normally should give a much easier testcase.
(Ran jit-tests, but nothing failed)

@gkw, decoder: could you run a fuzzer with this patch? It sets a breakpoint if something wrongs happens. It would also be awesome to get the testcase if it fails.

Comment 8

[Boris Zbarsky [:bzbarsky]]

With the attached patch, on the testcase for this bug I definitely hit SIGTRAP from jitcode in a debugger, for what that's worth.

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Will track this while investigation is underway but we're about to build our final beta today so unless something exploitable is uncovered that would make this chemspill-worthy we'll probably wontfix this for FF23

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: bug898832-debug-argument-check (for tip)

Comment 11

[Christian Holler (:decoder)]

Fuzzer found the following testcase after 3 days:


try {
var fib = new Intl.Collator(it, options);
} catch(exc0) {}
var functions = {
    toLocaleTimeString: Date.prototype.toLocaleTimeString
};
var locales = [ [NaN], ["de_DE"] ];
Object.getOwnPropertyNames(functions).forEach(function (p) {
    var f = functions[p];
    locales.forEach(function (locales) {
        try {
            var format = new Intl.DateTimeFormat(locales);
        } catch (e) {        }
        try {
            var result = f.call(new Date(), locales);
        } catch (e) {        }
    });
});



Causes sigtrap for me on a 32 bit optimized build (I removed the #ifdef DEBUG, but it might also reproduce on a debug build). Hannes, is that what you wanted me to find?

Comment 12

[Hannes Verschore [:h4writer]]

(In reply to Christian Holler (:decoder) from comment #11)
> Fuzzer found the following testcase after 3 days:
\0/, this indeed trips on the argumentcheck, THANKS!

Comment 13

[Hannes Verschore [:h4writer]]

Attachment: bug898832-argcheck

That's the culprit. If the argument was expected to be a specific object, we also removed the argument when it was only typed an object (not specific). So the called script would assume a specific object, but could get anything.

This is very bad and hackers will be pleased to try to exploit this. So I think we want this in all versions. I also think this might be worthy to get into FF23 (that is now on his last beta). Fix is really simple and shouldn't introduce something new. So would be very low risk, but it removes a big potential vector for hackers

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 783853 [details] [diff] [review]
bug898832-debug-argument-check (for tip)

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

I like to get this patch also in, now that we found the culprit. I will only land it after the fix goes in, to make sure we don't give extra tools to hackers to find this issue.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Hannes, I assume you verified that this fixes the original testcase here?

Comment 16

[Hannes Verschore [:h4writer]]

(In reply to Boris Zbarsky (:bz) from comment #15)
> Hannes, I assume you verified that this fixes the original testcase here?

Oh good call. I haven't been able to test it yet. I have build my own build, but it doesn't crash here :(. The description posted above does indicate this fix would work. But I think you could reproduce right? Can you test it?

Comment 17

[Robert Kaiser]

(In reply to Hannes Verschore [:h4writer] from comment #13)
> I also think this might be worthy to get
> into FF23 (that is now on his last beta).

Actually, the release builds have already been built, so that QA has time to sign off before we release on Tuesday. Unless this is a 0-day, I don't think we can still shove it into 23. That said, if this is exploitable, 1) the security rating should be re-evaluated (maybe it's sec-crit then?) and 2) you should ask for sec-approval before landing anywhere.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I have build my own build, but it doesn't crash here :(

Is it a build from rev 5458a7880db5?  See comment 2, which changed DOM code to effectively mask this bug....

Comment 19

[Hannes Verschore [:h4writer]]

Changed to sec-crit. We assume an object and therefore the layout of it (e.g. how many slots we have). Resulting code can therefore specialize and will remove the otherwise needed checks. Here we can take an arbitrary object (that doesn't fit the description on what we compiled). So as a result we could be reading/writing in slots that the object doesn't have. So in uninitialized or unexpected memory. (This is just speculation and I didn't try to exploit it myself.)

(In reply to Boris Zbarsky (:bz) from comment #18)
> > I have build my own build, but it doesn't crash here :(
> 
> Is it a build from rev 5458a7880db5?  See comment 2, which changed DOM code
> to effectively mask this bug....

No, I tried 92a5a4a9b76b, that is the blamed revision.

Comment 20

[Hannes Verschore [:h4writer]]

Comment on attachment 785014 [details] [diff] [review]
bug898832-argcheck

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

I think the exploit isn't that easy to find, but when understanding the provided two-liner patch and IM code, it might be rather easy to know how to abuse this. There is still the question to create the exploit itself.

So I think this need to get fixed on all branches (what seems it will not happen for 23, since we are to far in the release). So if we land to 24/25 only, I really think we should mask this patch into another unrelated patch! (Not sure if that is done, but I find it rather tricky to only commit this to aurora/nightly and don't fix beta with the current patch). I thought a lot of people are eavesdropping the commits right?

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

* No comments
* Not sure what to do about the check-in comment. See above comment
* No tests

- Which older supported branches are affected by this flaw?
23/24/25 (beta/aurora/nightly)

- If not all supported branches, which bug introduced the flaw?
bug 864468

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

They should be all the same. (That codepath hasn't changed since the introduction)

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

Pure speculative this could introduce a speed regression. But since we don't hit this case often (= never) (hence fuzzing took that long to create a testcase, also I haven't seen it happening in the wild on various JS sites, while browser for 3 days to find a testcase and dedicated opening JS heavy sites). I very confident to say, testing should be ok with testing the testcase and running jit-tests.
(Still I need to test if it fixes the original problem, but I'm rather sure it is. If it isn't the same, I'll open another bug. This issue found here really needs to get fixed!)

Comment 21

[Boris Zbarsky [:bzbarsky]]

> No, I tried 92a5a4a9b76b, that is the blamed revision.

OK.  I had reproduced with 5458a7880db5, but I no longer have that build around...

Comment 22

[Boris Zbarsky [:bzbarsky]]

Er, actually I do still have it around.  But now it's no reproducing the crash on the page in the url field.  :(

Comment 23

[Daniel Veditz [:dveditz]]

Comment on attachment 785014 [details] [diff] [review]
bug898832-argcheck

The merge has already happened today, so now it will need to land on beta, aurora, and trunk to hit Fx 24/25/26; Fx23 is practically out the door so that will remain unfixed.

Might as well land now with the initial flurry of "fix the first beta" bugs, if anything it will stand out more later. sec-approval+

Comment 24

[Hannes Verschore [:h4writer]]

Comment on attachment 785014 [details] [diff] [review]
bug898832-argcheck

[Approval Request Comment]
- Bug caused by (feature/regressing bug #):
bug 864468

- User impact if declined:
Possibility to crash/exploit the browser

- Testing completed (on m-c, etc.):
Not landed on any branch yet. Jit-tests succeed + found testcase.

- Risk to taking this patch (and alternatives if risky):
Very small. This patch reenables the old path (that is used in most cases) a bit more eagerly. So we don't hit the potential exploit

String or IDL/UUID changes made by this patch: /

(This request is for FF24/25, since we just merged, not for FF23)

Comment 25

[Jan de Mooij [:jandem]]

Comment on attachment 783853 [details] [diff] [review]
bug898832-debug-argument-check (for tip)

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

Great!

::: js/src/ion/CodeGenerator.cpp
@@ +2162,5 @@
> +CodeGenerator::generateArgumentsChecks(bool bailout)
> +{
> +    // This function can be used the normal way to check the argument types,
> +    // before entering the function and bailout when arguemnts don't match.
> +    // For debug purpose, this is can also be used to force/check that the

Nit: "For debugging purposes, this can also..."

Comment 26

[Scoobidiver (away)]

It's #11 top browser crasher in the first days of 23.0 + a security issue. I think it should be in a chemspill.

Comment 27

[Hannes Verschore [:h4writer]]

I'm waiting for an update to push to aurora/beta! I want to get this in as quickly as possible. (But I want to do it all at the same time!).

I'm also for release chemspill. It is really a nasty security bug and apparently it is a #11 browser crash. I assume that will get investigated by security people finding the issue?

Comment 28

[Carsten Book [:Tomcat]]

(In reply to Hannes Verschore [:h4writer] from comment #27)
> I'm waiting for an update to push to aurora/beta! I want to get this in as
> quickly as possible. (But I want to do it all at the same time!).
> 
> I'm also for release chemspill. It is really a nasty security bug and
> apparently it is a #11 browser crash. I assume that will get investigated by
> security people finding the issue?

I filed Bug 895437 some time before that bug and seems this is really on real world sites - and this 2 issues seems to be same :(

Comment 29

[Carsten Book [:Tomcat]]

btw i made some investigation over in Bug https://bugzilla.mozilla.org/show_bug.cgi?id=895437#c16 and seems this problem is already fixed in aurora and beta.

When i filed bug 895437 i noticed that this crash didn't happen in aurora and nightly so maybe whatever fixed this is now uplifted from aurora -> beta and so fix the problem on beta

Comment 30

[Hannes Verschore [:h4writer]]

The same happened with this bug. Due to some changes we don't crash anymore. BUT the issue is not gone! This is still exploitable! Add patch "debug-arguments-check" and you'll still notice sigtraps. Decoder also found a minimal testcase in comment 11. I'm sure if crafted correctly we can crash the browser again or even worse...

Comment 32

[Scoobidiver (away)]

(In reply to Hannes Verschore [:h4writer] from comment #30)
> Due to some changes we don't crash anymore.
The ref. URL doesn't crash Firefox but comments for 23.0 Release state it's still about German forums.

(In reply to Scoobidiver from comment #26)
> It's #11 top browser crasher in the first days of 23.0
Now #14.

Comment 33

[bhavana bajaj [:bajaj]]

(In reply to Hannes Verschore [:h4writer] from comment #27)
> I'm waiting for an update to push to aurora/beta! I want to get this in as
> quickly as possible. (But I want to do it all at the same time!).
> 
Branch uplift Approval is awaiting m-c landing here.

> I'm also for release chemspill. It is really a nasty security bug and
> apparently it is a #11 browser crash. I assume that will get investigated by
> security people finding the issue?

Comment 34

[Hannes Verschore [:h4writer]]

I'm fine with just pushing. I'm just concerned that will increase awareness of the bug... But that's just my opinion. Like said in comment 20, it is quite easy to guess what is wrong and create a testcase by just reading the patch...

But this is only my first security bug that serious. So I'll just listen. If you think it is better to push to trunk now, I will do it immediately ...

Comment 35

[Daniel Veditz [:dveditz]]

Given comment 26 you could easily justify a check-in comment such as
"bug 898832 fix topcrash regression" or some such.

Comment 36

[Daniel Veditz [:dveditz]]

(In reply to bhavana bajaj [:bajaj] from comment #33)
> Branch uplift Approval is awaiting m-c landing here.

We shouldn't need to land on m-c for this change. The intent of that guideline is to shake out potential regressions before inflicting the branch with problems. This is a simple change that will be easy to back out if it does cause regressions, seems unlikely that it will cause regressions, and given it's a dangerous security bug the regressions are likely less bad than having this unfixed. And if there are subtle regressions it's more likely to be found by the huge beta audience, and the sooner we get it into their hands the better chance we have of finding any.

Comment 37

[Daniel Veditz [:dveditz]]

(In reply to Daniel Veditz [:dveditz] from comment #36)
> We shouldn't need to land on m-c for this change.

... before landing on branches. Of course we do need to land on m-c to fix the bug there.

Comment 38

[bhavana bajaj [:bajaj]]

Thanks Dan !

[:h4writer], Please go ahead and land everywhere, approving for branches now.

Comment 39

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/mozilla-central/rev/b31a5d8c5e7f
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c52efc15dfd
https://hg.mozilla.org/releases/mozilla-beta/rev/73848fcdb8bb

Comment 40

[Ryan VanderMeulen [:RyanVM]]

Seems like we should try to write a test for this?

Comment 41

[Hannes Verschore [:h4writer]]

We have one, comment 11. But we shouldn't make this viewable until FF23 is fixed!!

Comment 42

[u279076]

Is this wontfix for 23 given we already shipped Firefox 23.0.1 without this fix? Should the case be made for a 23.0.2 to take this fix?

Comment 43

[u279076]

Matt, can you please verify this is fixed for Firefox 24 through 26?

Comment 44

[Hannes Verschore [:h4writer]]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #42)
> Is this wontfix for 23 given we already shipped Firefox 23.0.1 without this
> fix? Should the case be made for a 23.0.2 to take this fix?

Given the small/easy fix, with almost no possibility to regressions, the reduction in crashes and removal of possible serious exploit. I would vote yes. (Off course it is not my decision. Just giving more reasons ;))

Comment 45

[Lukas Blakk [:lsblakk] use ?needinfo]

The wontfix was due to this not being ready for final beta of 23.0 - on its own (at #15 in the topcrash list at 0.48%) this is not a driver for another chemspill unless the security team has a different thinking it seems like we can wait 4 weeks to 24.0's release

Comment 46

[Matt Wobensmith [:mwobensmith][:matt:]]

I can't repro the original bug using decoder's test or the URL above. I've tried pretty much all builds, pre-2013-07-28, dbg/opt, 32/64, Mac/Win/Linux. No crash, assert, warning or anything, and sadly I can't seem to get the patch from attachment #1 [details] [diff] [review] applied to my own build, either.

So I'd like to appeal to decoder - or anyone who has been able to reliably reproduce - to verify that we can no longer do so, on any and all branches 24-26 if possible. Thank you.

Comment 47

[Christian Holler (:decoder)]

I verified this on nightly and aurora, but on beta, the patch that traps the failing condition doesn't apply. Hannes, can you rebase that patch so I can test on beta?

Comment 48

[Hannes Verschore [:h4writer]]

Attachment: bug898832-debug-argument-check (for beta, ff24)

Sure thing

Comment 49

[Christian Holler (:decoder)]

Verified using the new patch.

Please note that I *only* did jsshell verification based on the test in comment 11. I did not create browser builds to test the URL.

Comment 50

[Hannes Verschore [:h4writer]]

(In reply to Christian Holler (:decoder) from comment #49)
> Verified using the new patch.
> 
> Please note that I *only* did jsshell verification based on the test in
> comment 11. I did not create browser builds to test the URL.

The url is very fragile. I even think the site has changed and does not trigger it anymore.

Comment 51

[Hannes Verschore [:h4writer]]

Setting myself as needinfo to land debug patch when FF24 takes FF23 over. So we don't have an active exploitable version anymore.

Comment 52

[Hannes Verschore [:h4writer]]

Debug patch landed in bug 930989