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

RESOLVED FIXED in Firefox 18

Status

()

Core
Layout
--
critical
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: mats, Assigned: mats)

Tracking

({crash, csectype-uaf, sec-critical})

unspecified
mozilla19
x86_64
Linux
crash, csectype-uaf, sec-critical
Points:
---

Firefox Tracking Flags

(firefox18+ fixed, firefox19+ fixed, firefox-esr1018+ fixed, firefox-esr1718+ fixed)

Details

(Whiteboard: [asan][qa-][adv-main18+][adv-esr17+][adv-esr10+])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Created attachment 676246 [details]
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.

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

ACTUAL RESULTS

==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
...
(Assignee)

Comment 1

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

Comment 3

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

Comment 4

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

Comment 5

5 years ago
Created attachment 678059 [details] [diff] [review]
fix

Explicitly remove the LayerManagerDataProperty() from a destroyed frame also when destroying the shell.
Assignee: nobody → matspal
Attachment #678059 - Flags: review?(roc)
Attachment #678059 - Flags: review?(roc) → review+
(Assignee)

Comment 6

5 years ago
Comment on attachment 678059 [details] [diff] [review]
fix

[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?
unknown

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]
fix

sec-approval+.

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

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8d002c4d6d
https://hg.mozilla.org/mozilla-central/rev/ea8d002c4d6d

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?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox19: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: sec-critical

Updated

5 years ago
status-firefox-esr10: --- → affected
status-firefox18: --- → affected
status-firefox-esr17: --- → affected
tracking-firefox-esr10: --- → 18+
tracking-firefox18: --- → +
tracking-firefox19: --- → +
tracking-firefox-esr17: --- → 18+
(In reply to Mats Palmgren [:mats] from comment #8)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ea8d002c4d6d

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]
fix

[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]
fix

Low risk , well tested patch , tracking 18. Approving on aurora.
Attachment #678059 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2636c23bd023
status-firefox18: affected → fixed
status-firefox-esr17: affected → fixed
Keywords: verifyme
(Assignee)

Comment 14

5 years ago
Comment on attachment 678059 [details] [diff] [review]
fix

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

thank you - please go ahead and uplift this to esr10 branch
Attachment #678059 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
https://hg.mozilla.org/releases/mozilla-esr17/rev/1f276706e60f
https://hg.mozilla.org/releases/mozilla-esr10/rev/70ce65da3666

(status-esr17 was accidentally set to fixed in comment 13. I received approval from bajaj to land it there as well)
status-firefox-esr10: affected → fixed
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:
https://hg.mozilla.org/mozilla-central/diff/25af0febf329/layout/base/FrameLayerBuilder.h#l1.115

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.
What he said. Backed out.

https://hg.mozilla.org/releases/mozilla-esr10/rev/2cab6f96723d
https://hg.mozilla.org/releases/mozilla-esr17/rev/26392b810c4b
status-firefox-esr10: fixed → affected
status-firefox-esr17: fixed → affected
(Assignee)

Comment 19

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/9bd7cdfc311f
I'll push it to esr17 also - as soon as my clone is complete...
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/releases/mozilla-esr17/rev/741b75ca2e09
status-firefox-esr10: affected → fixed
status-firefox-esr17: affected → fixed
(Assignee)

Comment 21

5 years ago
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:
  https://people.mozilla.com/~choller/firefox/asan/

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.
(Assignee)

Comment 24

5 years ago
(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, https://developer.mozilla.org/en/Building_Firefox_with_Address_Sanitizer describes also how to do manual builds or try pushes with ASan support.
Group: core-security
Keywords: csectype-uaf
You need to log in before you can comment on or make changes to this bug.