989183 - (CVE-2014-1524) Global-buffer-overflow in nsXBLProtoImpl::InstallImplementation

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Abhishek Arya]

Attachment: Testcase

I have fuzzPriv extension that makes this easier to reproduce.

>==359==ERROR: AddressSanitizer: global-buffer-overflow on address 0x7fcc9dcee2c8 at pc 0x7fcc97373f8f bp 0x7fffec68f930 sp 0x7fffec68f928
>READ of size 4 at 0x7fcc9dcee2c8 thread T0
>    #0 0x7fcc97373f8e in Hold dom/xbl/nsXBLService.h:159
>    #1 0x7fcc97373f8e in AddRef dom/xbl/nsXBLService.h:161
>    #2 0x7fcc97373f8e in assign_with_AddRef objdir-ff-asan/dom/xbl/../../dist/include/nsAutoPtr.h:865
>    #3 0x7fcc97373f8e in operator= objdir-ff-asan/dom/xbl/../../dist/include/nsAutoPtr.h:957
>    #4 0x7fcc97373f8e in fromJSClass dom/xbl/nsXBLBinding.h:73
>    #5 0x7fcc97373f8e in nsXBLProtoImpl::InstallImplementation(nsXBLPrototypeBinding*, nsXBLBinding*) dom/xbl/nsXBLProtoImpl.cpp:66
>    #6 0x7fcc973a1eb4 in nsXBLService::LoadBindings(nsIContent*, nsIURI*, nsIPrincipal*, nsXBLBinding**, bool*) dom/xbl/nsXBLService.cpp:516
>    #7 0x7fcc9856fd4a in nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int, bool, nsStyleContext*, unsigned int, nsTArray<nsIAnonymousContentCreator::ContentInfo>*, nsCSSFrameConstructor::FrameConstructionItemList&) layout/base/nsCSSFrameConstructor.cpp:5207
>    #8 0x7fcc98592cc2 in nsCSSFrameConstructor::BuildInlineChildItems(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, bool, bool) layout/base/nsCSSFrameConstructor.cpp:10714
>    #9 0x7fcc9857289f in nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState&, nsIContent*, nsIFrame*, nsIAtom*, int, bool, nsStyleContext*, unsigned int, nsTArray<nsIAnonymousContentCreator::ContentInfo>*, nsCSSFrameConstructor::FrameConstructionItemList&) layout/base/nsCSSFrameConstructor.cpp:5394
>    #10 0x7fcc9858d15f in nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, nsIFrame*, nsCSSFrameConstructor::FrameConstructionItemList&) layout/base/nsCSSFrameConstructor.cpp:5148
>    #11 0x7fcc9859e2fc in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, bool) layout/base/nsCSSFrameConstructor.cpp:6748
>    #12 0x7fcc9859923c in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) layout/base/nsCSSFrameConstructor.cpp:6427
>    #13 0x7fcc985992a8 in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) layout/base/nsCSSFrameConstructor.cpp:6434
>    #14 0x7fcc985992a8 in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) layout/base/nsCSSFrameConstructor.cpp:6434
>    #15 0x7fcc9859f9e9 in nsCSSFrameConstructor::CreateNeededFrames() layout/base/nsCSSFrameConstructor.cpp:6449
>    #16 0x7fcc984c2477 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) layout/base/nsPresShell.cpp:4042
>    #17 0x7fcc984f7673 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) layout/base/nsRefreshDriver.cpp:1142
>    #18 0x7fcc984fbd34 in TickDriver layout/base/nsRefreshDriver.cpp:168
>    #19 0x7fcc984fbd34 in mozilla::RefreshDriverTimer::Tick() layout/base/nsRefreshDriver.cpp:160
>    #20 0x7fcc94035fc4 in nsTimerImpl::Fire() xpcom/threads/nsTimerImpl.cpp:551
>    #21 0x7fcc9403664e in nsTimerEvent::Run() xpcom/threads/nsTimerImpl.cpp:635
>    #22 0x7fcc9402d4e0 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:694
>    #23 0x7fcc93ef8d5a in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
>    #24 0x7fcc947f0159 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:95
>    #25 0x7fcc94799130 in RunInternal ipc/chromium/src/base/message_loop.cc:226
>    #26 0x7fcc94799130 in RunHandler ipc/chromium/src/base/message_loop.cc:219
>    #27 0x7fcc94799130 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:193
>    #28 0x7fcc96906447 in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:164
>    #29 0x7fcc9971ccb8 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:276
>    #30 0x7fcc9958c1a3 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4019
>    #31 0x7fcc9958d08d in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:4088
>    #32 0x7fcc9958dedd in XRE_main toolkit/xre/nsAppRunner.cpp:4300
>    #33 0x48c6e7 in do_main browser/app/nsBrowserApp.cpp:282
>    #34 0x48c6e7 in main browser/app/nsBrowserApp.cpp:643
>    #35 0x7fcca259276c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
>    #36 0x48bbbc in _start
>
>0x7fcc9dcee2c8 is located 0 bytes to the right of global variable 'JSObject::class_' from 'objdir-ff-asan/js/src/Unified_cpp_js_src7.cpp' (0x7fcc9dcee160) of size 360
>SUMMARY: AddressSanitizer: global-buffer-overflow ??:0 ??
>Shadow bytes around the buggy address:
>  0x0ffa13b95c00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ffa13b95c10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ffa13b95c20: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 00 00 00 00
>  0x0ffa13b95c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ffa13b95c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>=>0x0ffa13b95c50: 00 00 00 00 00 00 00 00 00[f9]f9 f9 f9 f9 f9 f9
>  0x0ffa13b95c60: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ffa13b95c70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ffa13b95c80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>  0x0ffa13b95c90: 00 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 f9 00 00 00 00
>  0x0ffa13b95ca0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>Shadow byte legend (one shadow byte represents 8 application bytes):
>  Addressable:           00
>  Partially addressable: 01 02 03 04 05 06 07
>  Heap left redzone:       fa
>  Heap right redzone:      fb
>  Freed heap region:       fd
>  Stack left redzone:      f1
>  Stack mid redzone:       f2
>  Stack right redzone:     f3
>  Stack partial redzone:   f4
>  Stack after return:      f5
>  Stack use after scope:   f8
>  Global redzone:          f9
>  Global init order:       f6
>  Poisoned by user:        f7
>  Contiguous container OOB:fc
>  ASan internal:           fe
>==359==ABORTING
>
>

Comment 1

[Bobby Holley (:bholley)]

Ugh, this is a regression from bug 825392. I didn't see that cast.

Comment 2

[Bobby Holley (:bholley)]

Oh, I see. The issue isn't with the cast at all.

Comment 3

[Bobby Holley (:bholley)]

So, this isn't a regression at all. As it turns out, nobody ever bothered to check that the XBL class object that we look up on the global is actually an XBL class object. And the property isn't permanent, so script can just redefine it and cause XBL to grab an arbitrary object, and cast its JSClass to an nsXBLJSClass. :-(

This is probably going to need backporting everywhere, so for now I'm going to do something simple. But we _really_ just need to nix this whole mechanism. I'll get a bug on file.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Tests. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Stop using ye olde JS_InitClass for XBL class objects. v1

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8399654 [details] [diff] [review]
Tests. v1

(Because bz has played with js protos a lot more than me and this code is super fragile.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8399655 [details] [diff] [review]
Stop using ye olde JS_InitClass for XBL class objects. v1

Man.  We still have JS_InitClass callers in our code.  <sigh>.

Do we really need JSPROP_ENUMERATE?  If not, let's nix it.

r=me either way.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8399654 [details] [diff] [review]
Tests. v1

r=me

Comment 9

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] from comment #7)
> Comment on attachment 8399655 [details] [diff] [review]
> Stop using ye olde JS_InitClass for XBL class objects. v1
> 
> Man.  We still have JS_InitClass callers in our code.  <sigh>.
> 
> Do we really need JSPROP_ENUMERATE?  If not, let's nix it.

Oh, I'd assumed that the property was enumerable before. Looks like it actually wasn't. Happy to kill that.

Comment 10

[Bobby Holley (:bholley)]

jst pointed out that this doesn't fully do the trick, because someone can still predefine properties with the same name. So I think we need to check the JSClass of the object.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Check for nsXBLJSClass. v1

I think we should land both patches on trunk, and backport just this one.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8400031 [details] [diff] [review]
Check for nsXBLJSClass. v1

r=me

Comment 13

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=ead9d87a4233

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 8400031 [details] [diff] [review]
Check for nsXBLJSClass. v1

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

The memory hazard is pretty obvious. The attacker would then have to exploit the memory hazard, but people are pretty good at that.

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

The mochitest demonstrates more clearly how to trigger the issue, but I can avoid landing the test for now.

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?

Backports should be trivial.

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

Unlikely to cause regressions.

Comment 15

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

Comment on attachment 8400031 [details] [diff] [review]
Check for nsXBLJSClass. v1

sec-approval+ for trunk. Let's get this on Aurora, Beta, and ESR24 once it lands there.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8399655 [details] [diff] [review]
Stop using ye olde JS_InitClass for XBL class objects. v1

This patch also breaks tests. I'm going to punt on it for this bug and land the vestiges of it in bug 990290.

Comment 17

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/651662279e95

Comment 18

[Bobby Holley (:bholley)]

Attachment: Tests. v3 r=bz

Given that we're not landing this now (for security sensitivity), and given
that the patch in bug 990290 makes this test obsolete, it's unlikely to ever
see the light of day. Nevertheless, I'm uploading the current version in my
tree for posterity.

Comment 19

[Carsten Book [:Tomcat]]

landed as https://hg.mozilla.org/mozilla-central/rev/651662279e95

Comment 20

[Bobby Holley (:bholley)]

Comment on attachment 8400031 [details] [diff] [review]
Check for nsXBLJSClass. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): longstanding
User impact if declined: security bugs
Testing completed (on m-c, etc.): just landed to m-c
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: None

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a2231750b004
https://hg.mozilla.org/releases/mozilla-beta/rev/13e065bdc821
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/56fd5f3f4a5b
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/94a9f58af4a3

This has conflicts on esr24 due to bug 910823. Not sure what the right way forward on that is.

Comment 22

[Bobby Holley (:bholley)]

https://hg.mozilla.org/releases/mozilla-esr24/rev/cabd05576483

I can't seem to do a full esr24 build locally, so somebody should make sure to run the unit test in this bug and make sure that it actually passes.

Comment 23

[Bogdan Maris, Desktop QA]

I used ASAN build from 2014-03-27 with https://www.squarefree.com/extensions/domFuzzLite3.xpi installed but was unable to get the error from comment 0 so can`t reproduce this issue in order to see if it`s fixed. Am I doing something wrong here? Is that the add-on that you used Arya?

Comment 24

[Abhishek Arya]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #23)
> I used ASAN build from 2014-03-27 with
> https://www.squarefree.com/extensions/domFuzzLite3.xpi installed but was
> unable to get the error from comment 0 so can`t reproduce this issue in
> order to see if it`s fixed. Am I doing something wrong here? Is that the
> add-on that you used Arya?

Yes that is the only add-on i was using. It used to reproduce fine on my machine. Maybe ask Bobby (dev) for a better repro. Otherwise, I confirmed using my repro on trunk that the bug is fixed.

Comment 25

[Bobby Holley (:bholley)]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #23)
> I used ASAN build from 2014-03-27 with
> https://www.squarefree.com/extensions/domFuzzLite3.xpi installed but was
> unable to get the error from comment 0 so can`t reproduce this issue in
> order to see if it`s fixed. Am I doing something wrong here? Is that the
> add-on that you used Arya?

My request for verification was to run the mochitest I attached on esr24, because I wasn't able to do a local build of that branch. See comment 22.

Comment 27

[Bogdan Maris, Desktop QA]

I ran the attached test on latest Nightly, Aurora and Firefox 29 beta (all ASAN builds) but I was not able to do so on esr24. I got '30:17.60 make[5]: *** [libxul.so] Error 1' when building. All tests were a PASS. Leaving verifyme until someone can check the esr24 build.

Comment 28

[Ioana (away)]

Bogdan, please change the status to Verified once you have verified a bug on the Target Milestone version.

Comment 29

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

I've tried to reproduce this bug on two ASan builds of Fx29 - 2014-03-01 and 2014-03-25 - but I can't. I am using the domfuzz add-on. The original bug file does not crash. That's not unexpected; memory bugs are often intermittent.

However, bholley's test case passes on these builds. I'd assume that would fail.

Bobby - should your tests fail pre-patch? Or are they just a sanity check to make sure that we hit some code and don't do anything bad?

Comment 30

[Bobby Holley (:bholley)]

(In reply to Matt Wobensmith from comment #29)
> I've tried to reproduce this bug on two ASan builds of Fx29 - 2014-03-01 and
> 2014-03-25 - but I can't. I am using the domfuzz add-on. The original bug
> file does not crash. That's not unexpected; memory bugs are often
> intermittent.
> 
> However, bholley's test case passes on these builds. I'd assume that would
> fail.
> 
> Bobby - should your tests fail pre-patch? Or are they just a sanity check to
> make sure that we hit some code and don't do anything bad?

They should fail, at least on a local debug build, which should assert.

Comment 31

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

I've run the test cases in ESR24, local build as of 2014-04-11, and they pass, so I'm marking verified for that branch.

Ideally I'd like to see the failure first, to then confirm fixed. Unfortunately for this bug, that might not be possible.

Comment 32

[Bobby Holley (:bholley)]

(In reply to Matt Wobensmith from comment #31)
> I've run the test cases

Which? Inferno's or mine?

> in ESR24, local build as of 2014-04-11

A debug build?

> Ideally I'd like to see the failure first, to then confirm fixed.

Did you try a debug build from before this patch?

Comment 33

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

I ran Abhishek's test and your mochitest in a local debug build of ESR24, built today. Both passed, so I'm marking verified for that branch.

However, as I mentioned in comment 29, I did not see either test fail (or assert) in the Fx29 builds mentioned there. Both were debug builds. I'm not sure how much more effort I will put into that.

Comment 35

[Bobby Holley (:bholley)]

See comment 18.