Closed Bug 806483 Opened 7 years ago Closed 7 years ago

Heap-use-after-free (read) nsIFrame::GetStyleContext


(Core :: Layout, defect, critical)

Not set



Tracking Status
firefox18 + fixed
firefox19 + fixed
firefox-esr10 18+ fixed
firefox-esr17 18+ fixed


(Reporter: mats, Assigned: mats)


(Keywords: crash, csectype-uaf, sec-critical, Whiteboard: [asan][qa-][adv-main18+][adv-esr17+][adv-esr10+])


(2 files)

Attached file ASAN dump
ASAN debug build on Linux64, compiled with
ac_add_options --enable-optimize="-faddress-sanitizer -Dxmalloc=myxmalloc -DDEBUG_TRACEMALLOC_PRESARENA"
ac_add_options --enable-debug

That is, disabling the pres shell arena.

1. run Firefox with a fresh profile
2. click on a menu item, say Tools, then close it
3. exit


==8983== ERROR: AddressSanitizer heap-use-after-free on address 0x7ff31bfc6aa0 at pc 0x7ff35b68f777 bp 0x7fff120279d0 sp 0x7fff120279c8
READ of size 8 at 0x7ff31bfc6aa0 thread T0
    #0 0x7ff35b68f777 in nsIFrame::GetStyleContext() const ???:0
    #1 0x7ff35b68f2c3 in nsIFrame::PresContext() const ../../dist/include/nsIFrame.h:572
    #2 0x7ff35b962a99 in nsIFrame::Properties() const layout/svg/../generic/nsIFrame.h:2619
    #3 0x7ff35b965dd5 in ~DisplayItemData layout/base/FrameLayerBuilder.cpp:142
    #4 0x7ff35b9aa50a in mozilla::FrameLayerBuilder::DisplayItemData::Release() layout/base/FrameLayerBuilder.h:498
    #5 0x7ff35b9aa1de in ~nsRefPtr layout/base/../../dist/include/nsAutoPtr.h:874
    #6 0x7ff35b97a183 in ~nsRefPtr layout/base/../../dist/include/nsAutoPtr.h:875
    #7 0x7ff35ba1074d in ~nsRefPtrHashKey layout/base/../../dist/include/nsHashKeys.h:294
    #8 0x7ff35ba10633 in ~nsRefPtrHashKey layout/base/../../dist/include/nsHashKeys.h:294
    #9 0x7ff35ba0ff64 in nsTHashtable<nsRefPtrHashKey<mozilla::FrameLayerBuilder::DisplayItemData> >::s_ClearEntry(PLDHashTable*, PLDHashEntryHdr*) layout/base/../../dist/include/nsTHashtable.h:464
    #10 0x7ff3676a1e3e in PL_DHashTableFinish xpcom/build/pldhash.cpp:363
    #11 0x7ff35ba0f503 in ~nsTHashtable layout/base/../../dist/include/nsTHashtable.h:385
    #12 0x7ff35ba0f3a3 in ~nsTHashtable layout/base/../../dist/include/nsTHashtable.h:385
    #13 0x7ff35ba0f289 in ~LayerManagerData layout/base/FrameLayerBuilder.cpp:185
    #14 0x7ff35ba0eff3 in ~LayerManagerData layout/base/FrameLayerBuilder.cpp:185
    #15 0x7ff35ba0f10b in ~LayerManagerData layout/base/FrameLayerBuilder.cpp:183
    #16 0x7ff35b9c3de2 in LayerManagerUserDataDestroy layout/base/../../dist/include/Layers.h:119
    #17 0x7ff35b8c3ca3 in mozilla::gfx::UserData::Destroy() gfx/2d/UserData.h:98
    #18 0x7ff3683d6d64 in mozilla::layers::LayerManager::Destroy() gfx/layers/Layers.h:164
    #19 0x7ff3656bc710 in nsWindow::Destroy() widget/gtk2/nsWindow.cpp:618
    #20 0x7ff35fdb1f40 in nsView::DestroyWidget() view/src/nsView.cpp:108
    #21 0x7ff35fdb0ab0 in ~nsView view/src/nsView.cpp:92
    #22 0x7ff35fdb021b in ~nsView view/src/nsView.cpp:43
    #23 0x7ff35fdb1042 in nsIView::Destroy() view/src/nsView.cpp:152
    #24 0x7ff35c426031 in nsFrame::DestroyFrom(nsIFrame*) layout/generic/nsFrame.cpp:674
    #25 0x7ff35c84237a in nsSplittableFrame::DestroyFrom(nsIFrame*) layout/generic/nsSplittableFrame.cpp:44
    #26 0x7ff35c3d23a9 in nsContainerFrame::DestroyFrom(nsIFrame*) layout/generic/nsContainerFrame.cpp:238
    #27 0x7ff35d430435 in nsBoxFrame::DestroyFrom(nsIFrame*) layout/xul/base/src/nsBoxFrame.cpp:949
    #28 0x7ff35d58d140 in nsMenuPopupFrame::DestroyFrom(nsIFrame*) layout/xul/base/src/nsMenuPopupFrame.cpp:1827
(for posterity, the above is for mozilla-central rev 324985f4c4ea)
Mats want to take a stab at rating this one? Do you think this is triggerable in other ways (not user action)?
> Mats want to take a stab at rating this one?

It looks like [sg:critical (mitigated by frame poisoning)] to me, but I don't
know deeply enough about LayerManager stuff to say for sure.

> Do you think this is triggerable in other ways (not user action)?

The bug occurs while destroying a pres shell, which you can do from script by
removing an <iframe> from the document for example.  I don't know what content
would trigger it.
The bug is reproducible in rev d26f6917e986 (2012-05-24).  I tried building some
revs from 2011 as well, but they failed to build under memory/.  The code involved
is from 2010 or so, eg. bug 579276.
Attached patch fixSplinter Review
Explicitly remove the LayerManagerDataProperty() from a destroyed frame also when destroying the shell.
Assignee: nobody → matspal
Attachment #678059 - Flags: review?(roc)
Comment on attachment 678059 [details] [diff] [review]

[Security approval request comment]
How easily can the security issue be deduced from the patch?
not at all

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

Which older supported branches are affected by this flaw?
the bug was introduced in 2010 or so

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

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

How likely is this patch to cause regressions; how much testing does it need?
low risk
Attachment #678059 - Flags: sec-approval?
Comment on attachment 678059 [details] [diff] [review]


Could someone suggest a security rating for this issue while we're here?
Attachment #678059 - Flags: sec-approval? → sec-approval+

Resolving fixed, but this outstanding:

(In reply to Al Billings [:abillings] from comment #7)
> sec-approval+.
> Could someone suggest a security rating for this issue while we're here?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(In reply to Mats Palmgren [:mats] from comment #8)

Looks like this patch is been on m-c for a week now. Please uplift the patch to aurora as soon as we are comfortable with the bake time/testing on m-c .
Comment on attachment 678059 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): DLBI
User impact if declined: possible crash/security issue
Testing completed (on m-c, etc.): one week landed
Risk to taking this patch (and alternatives if risky): Simple patch with relatively low risk
String or UUID changes made by this patch: None
Attachment #678059 - Flags: approval-mozilla-aurora?
Comment on attachment 678059 [details] [diff] [review]

Low risk , well tested patch , tracking 18. Approving on aurora.
Attachment #678059 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 678059 [details] [diff] [review]

see comment 6 and comment 11
Attachment #678059 - Flags: approval-mozilla-esr10?
Comment on attachment 678059 [details] [diff] [review]

thank you - please go ahead and uplift this to esr10 branch
Attachment #678059 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
This failed to build on esr10, with this build error:
/builds/slave/m-esr10-osx64-dbg/build/layout/base/FrameLayerBuilder.h:415: error: 'static const mozilla::FramePropertyDescriptor* mozilla::FrameLayerBuilder::LayerManagerDataProperty()' is protected

/builds/slave/m-esr10-osx64-dbg/build/layout/base/nsPresShell.cpp:2294: error: within this context

Looks like this function did indeed used to be protected, but it was made public (on trunk) in this change:

But on esr10, that function is still protected & hence can't be called directly by nsPresShell.

The fix could be as simple as just moving the LayerManagerDataProperty declaration to a "public" chunk of the header file, but it's probably worth a once-over from mats and/or mattwoodrow to be sure that's a good idea.
I'll push it to esr17 also - as soon as my clone is complete...
It requires a special ASAN build to see this problem so it's not easily verifiable.
Keywords: verifyme
(In reply to Mats Palmgren [:mats] from comment #21)
> It requires a special ASAN build to see this problem so it's not easily
> verifiable.

Thanks Mats. Flagging this [qa-].
Whiteboard: [asan] → [asan][qa-]
Whiteboard: [asan][qa-] → [asan][qa-][adv-main18+][adv-esr17+][adv-esr10+]
(In reply to Mats Palmgren [:mats] from comment #21)
> It requires a special ASAN build to see this problem so it's not easily
> verifiable.

Note that Christian Holler (:decoder) generates ASAN nightly builds and posts them here:

Currently, it looks like the oldest build there is from 12/26/2012, so they probably won't help for verification in this bug's case. But in the future, we may be able to grab ASAN nightlies from there for fix-verification.
(In reply to Daniel Holbert [:dholbert] from comment #23)
> Note that Christian Holler (:decoder) generates ASAN nightly builds...

Yes, but they wont help verifying this bug.  What I meant with "special" is an
ASan build that was built with the extra -DDEBUG_TRACEMALLOC_PRESARENA.
(btw, I'm fixing that in bug 827150)
If necessary, describes also how to do manual builds or try pushes with ASan support.
Group: core-security
You need to log in before you can comment on or make changes to this bug.