Bug 989183 (CVE-2014-1524)

Global-buffer-overflow in nsXBLProtoImpl::InstallImplementation

VERIFIED FIXED in Firefox 29

Status

()

defect
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: inferno, Assigned: bholley)

Tracking

({csectype-bounds, sec-high})

Trunk
mozilla31
x86_64
All
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite -

Firefox Tracking Flags

(firefox28 wontfix, firefox29+ verified, firefox30+ verified, firefox31+ verified, firefox-esr24+ verified, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

Details

(Whiteboard: [adv-main29+][adv-esr24.5+])

Attachments

(3 attachments, 2 obsolete attachments)

Reporter

Description

5 years ago
Posted file 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
>
>
Reporter

Updated

5 years ago
Summary: Gglobal-buffer-overflow in nsXBLProtoImpl::InstallImplementation → Global-buffer-overflow in nsXBLProtoImpl::InstallImplementation
Assignee

Comment 1

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

Comment 2

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

Updated

5 years ago
Flags: sec-bounty?
Assignee

Comment 3

5 years ago
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.
No longer blocks: 825392
Keywords: regression
Assignee

Comment 4

5 years ago
Posted patch Tests. v1 (obsolete) — Splinter Review
Attachment #8399654 - Flags: review?(bugs)
Attachment #8399655 - Flags: review?(bugs) → review?(bzbarsky)
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.)
Attachment #8399654 - Flags: review?(bugs) → review?(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.
Attachment #8399655 - Flags: review?(bzbarsky) → review+
Comment on attachment 8399654 [details] [diff] [review]
Tests. v1

r=me
Attachment #8399654 - Flags: review?(bzbarsky) → review+
Assignee

Comment 9

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

Comment 10

5 years ago
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.
Assignee

Comment 11

5 years ago
I think we should land both patches on trunk, and backport just this one.
Attachment #8400031 - Flags: review?(bzbarsky)
Comment on attachment 8400031 [details] [diff] [review]
Check for nsXBLJSClass. v1

r=me
Attachment #8400031 - Flags: review?(bzbarsky) → review+
Assignee

Comment 14

5 years ago
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.
Attachment #8400031 - Flags: sec-approval?
Assignee

Updated

5 years ago
Flags: in-testsuite?
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.
Attachment #8400031 - Flags: sec-approval? → sec-approval+
Assignee

Updated

5 years ago
Blocks: 990290
Assignee

Comment 16

5 years ago
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.
Attachment #8399655 - Attachment is obsolete: true
Assignee

Comment 18

5 years ago
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.
Attachment #8399654 - Attachment is obsolete: true
Attachment #8400296 - Flags: review+
landed as https://hg.mozilla.org/mozilla-central/rev/651662279e95
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Assignee

Comment 20

5 years ago
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
Attachment #8400031 - Flags: approval-mozilla-esr24?
Attachment #8400031 - Flags: approval-mozilla-beta?
Attachment #8400031 - Flags: approval-mozilla-aurora?
Attachment #8400031 - Flags: approval-mozilla-beta?
Attachment #8400031 - Flags: approval-mozilla-beta+
Attachment #8400031 - Flags: approval-mozilla-aurora?
Attachment #8400031 - Flags: approval-mozilla-aurora+
Attachment #8400031 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Assignee

Comment 22

5 years ago
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.
Flags: needinfo?(bobbyholley)
Keywords: verifyme
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?
Flags: needinfo?(inferno)
Reporter

Comment 24

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

Comment 25

5 years ago
(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.
Flags: sec-bounty? → sec-bounty+
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

5 years ago
Bogdan, please change the status to Verified once you have verified a bug on the Target Milestone version.
Status: RESOLVED → VERIFIED
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?
Assignee

Comment 30

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

Comment 32

5 years ago
(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?
Flags: needinfo?(mwobensmith)
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.
Flags: needinfo?(mwobensmith)
Whiteboard: [adv-main29+][adv-esr24.5+]
Alias: CVE-2014-1524
Assignee

Comment 35

5 years ago
See comment 18.
Flags: in-testsuite? → in-testsuite-
Group: core-security
You need to log in before you can comment on or make changes to this bug.