Closed Bug 1143535 Opened 5 years ago Closed 5 years ago

Stack-buffer-overflow in nsCSSFrameConstructor::InterpretRubyWhitespace

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- unaffected
firefox37 --- unaffected
firefox38 + fixed
firefox39 + fixed
firefox-esr31 --- unaffected
b2g-v1.4 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- fixed

People

(Reporter: inferno, Assigned: xidorn)

Details

(4 keywords, Whiteboard: [asan])

Attachments

(3 files)

Attached file Testcase
Looks like another ruby issue, and also reproduces after fix from bug 1141919

=================================================================
==17310==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffe03c13e4 at pc 0x7f313c3c0cf6 bp 0x7fffe03c0d30 sp 0x7fffe03c0d28
READ of size 8 at 0x7fffe03c13e4 thread T0 (Web Content)
    #0 0x7f313c3c0cf5 in DoGetStyleDisplay /build/firefox/src/objdir-ff-asan/layout/base/../../dist/include/nsStyleStructList.h:144:1
    #1 0x7f313c3c0cf5 in StyleDisplay /build/firefox/src/objdir-ff-asan/layout/base/../../dist/include/nsStyleStructList.h:144
    #2 0x7f313c3c0cf5 in nsCSSFrameConstructor::InterpretRubyWhitespace(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator const&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator const&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:9732
    #3 0x7f313c3c051b in nsCSSFrameConstructor::WrapItemsInPseudoRubyLevelContainer(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsStyleContext*, nsIContent*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:9809:7
    #4 0x7f313c3bec11 in nsCSSFrameConstructor::CreateNeededPseudoInternalRubyBoxes(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsIFrame*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:9919:9
    #5 0x7f313c2929ff in ConstructFramesFromItemList /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:10232:3
    #6 0x7f313c2929ff in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:10439
    #7 0x7f313c2a6938 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:3887:9
    #8 0x7f313c2b0b95 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:5929:3
    #9 0x7f313c2a6810 in ConstructFramesFromItemList /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:10239:5
    #10 0x7f313c2a6810 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:3883
    #11 0x7f313c2b0b95 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:5929:3
    #12 0x7f313c2a6810 in ConstructFramesFromItemList /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:10239:5
    #13 0x7f313c2a6810 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:3883
    #14 0x7f313c2b0b95 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:5929:3
    #15 0x7f313c292adc in ConstructFramesFromItemList /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:10239:5
    #16 0x7f313c292adc in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:10439
    #17 0x7f313c2a6938 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:3887:9
    #18 0x7f313c2b0b95 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:5929:3
    #19 0x7f313c2bd812 in ConstructFramesFromItemList /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:10239:5
    #20 0x7f313c2bd812 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, bool) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:7249
    #21 0x7f313c2b76dc in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:6890:5
    #22 0x7f313c2be8ca in nsCSSFrameConstructor::CreateNeededFrames() /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:6912:5
    #23 0x7f313c230ebf in mozilla::RestyleManager::ProcessPendingRestyles() /build/firefox/src/layout/base/RestyleManager.cpp:1631:3
    #24 0x7f313c454405 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /build/firefox/src/layout/base/nsPresShell.cpp:4280:9
    #25 0x7f313c1c6747 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /build/firefox/src/layout/base/nsRefreshDriver.cpp:1599:11
    #26 0x7f313c1cf383 in TickDriver /build/firefox/src/layout/base/nsRefreshDriver.cpp:198:5
    #27 0x7f313c1cf383 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /build/firefox/src/layout/base/nsRefreshDriver.cpp:189
    #28 0x7f313685c89f in nsTimerImpl::Fire() /build/firefox/src/xpcom/threads/nsTimerImpl.cpp:631:7
    #29 0x7f313685d436 in nsTimerEvent::Run() /build/firefox/src/xpcom/threads/nsTimerImpl.cpp:724:3
    #30 0x7f3136852b16 in nsThread::ProcessNextEvent(bool, bool*) /build/firefox/src/xpcom/threads/nsThread.cpp:855:7
    #31 0x7f31368ac7f0 in NS_ProcessNextEvent(nsIThread*, bool) /build/firefox/src/xpcom/glue/nsThreadUtils.cpp:265:10
    #32 0x7f31371345fe in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /build/firefox/src/ipc/glue/MessagePump.cpp:99:21
    #33 0x7f31370e0fa1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #34 0x7f31370e0fa1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #35 0x7f31370e0fa1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #36 0x7f313bb72e1f in nsBaseAppShell::Run() /build/firefox/src/widget/nsBaseAppShell.cpp:164:3
    #37 0x7f313d790b13 in XRE_RunAppShell /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:743:12
    #38 0x7f31370e0fa1 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #39 0x7f31370e0fa1 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #40 0x7f31370e0fa1 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #41 0x7f313d78ff4c in XRE_InitChildProcess /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:580:7
    #42 0x4dc17e in content_process_main(int, char**) /build/firefox/src/ipc/app/../contentproc/plugin-container.cpp:211:19
    #43 0x7f3133dc0ec4 in __libc_start_main
    #44 0x41c438 in _start

Address 0x7fffe03c13e4 is located in stack of thread T0 (Web Content) at offset 804 in frame
    #0 0x7f313c29175f in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:10311

  This frame has 15 object(s):
    [32, 56) 'iter.i'
    [96, 112) '.sroa.0.i4'
    [128, 144) '.sroa.0.i'
    [160, 164) 'namespaceID.i.i'
    [176, 200) 'floatSaveState.sroa.8'
    [240, 328) 'itemsToConstruct'
    [368, 480) 'anonymousItems'
    [512, 536) 'insertion'
    [576, 616) 'iter'
    [656, 680) 'ancestorPusher'
    [720, 736) 'parentTag'
    [752, 768) 'kidTag'
    [784, 800) 'params' <== Memory access at offset 804 overflows this variable
    [816, 832) '' <== Memory access at offset 804 underflows this variable
    [848, 856) ''
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
Shadow bytes around the buggy address:
  0x10007c070220: f2 f2 f2 f2 00 00 f2 f2 00 00 f2 f2 04 f2 00 00
  0x10007c070230: 00 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00
  0x10007c070240: 00 f2 f2 f2 f2 f2 00 00 00 00 00 00 00 00 00 00
  0x10007c070250: 00 00 00 00 f2 f2 f2 f2 00 00 00 f2 f2 f2 f2 f2
  0x10007c070260: 00 00 00 00 00 f2 f2 f2 f2 f2 00 00 00 f2 f2 f2
=>0x10007c070270: f2 f2 00 00 f2 f2 00 00 f2 f2 00 00[f2]f2 00 00
  0x10007c070280: f2 f2 00 f3 f3 f3 f3 f3 00 00 00 00 00 00 00 00
  0x10007c070290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007c0702a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007c0702b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007c0702c0: 00 00 00 00 00 00 00 00 f1 f1 f1 f1 00 00 00 f2
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
==17310==ABORTING
Flags: needinfo?(quanxunzhen)
Attached patch patchSplinter Review
This patch should fix this specific stack overflow. But the testcase reveals that there are still some problem with the line break suppression.
Assignee: nobody → quanxunzhen
Flags: needinfo?(quanxunzhen)
Attachment #8577869 - Flags: review?(dbaron)
[Tracking Requested - why for this release]: security bug
Can you describe where and how the stack overflow occurs so we can assign
a security rating, please?

FYI, it's always a good idea to describe a bug with as much details as
possible in Bugzilla (for all bugs, but especially for security bugs)
since it serves as documentation and it helps the reviewer (and anyone
else reading this) understand the problem.  A detailed description
might help a reader realize that the bug can occur in other places /
situations that you might not know about for example.
Severity: normal → critical
Flags: needinfo?(quanxunzhen)
Keywords: crash, testcase
Whiteboard: [asan]
This happens when the assertion
> MOZ_ASSERT(!aStartIter.AtStart() && !aEndIter.IsDone());
is violated. See https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9728

It asserts that any leading/trailing whitespaces has been trimmed in nsCSSFrameConstructor::TrimLeadingAndTrailingWhitespaces called by nsCSSFrameConstructor::CreateNeededPseudoInternalRubyBoxes. But this call is bypassed because the whitespaces appear in a CSS pseudo element.

The condition changed in the patch meant to check against ruby pseudo frames, but accidentally only checked whether it is a pseudo one without confirming the type of the pseudo. With this fixed, that assertion should not be broken in the same way again.

Back to the original assertion. If that assertion fails, the returned reference of prevIter.item() or aEndIter.item() is dereffed from FrameConstructionItemList::mItems which is not of type FrameConstructionItem. This will cause this overflow.
Flags: needinfo?(quanxunzhen)
Crash + security issue, tracking.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #4)
> If that assertion fails, the returned
> reference of prevIter.item() or aEndIter.item() is dereffed from
> FrameConstructionItemList::mItems which is not of type
> FrameConstructionItem. This will cause this overflow.

Thanks, so I think that means we read the 'mDisplay' values from some
arbitrary memory location, which will either safely crash with a SEGV, 
or we pass those values to ComputeRubyWhitespaceType:
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9687
which will return one of the eRuby* enum values based on those bogus
mDisplay values.  The only caller of InterpretRubyWhitespace is
WrapItemsInPseudoRubyLevelContainer here:
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9808
which will execute one of the if-else branches based on that
eRuby* enum value.  This looks safe since the iterators on this
level are not affected and I don't see anything that could go
catastrophically wrong in any of the branches. Let me know if
I missed something.

I'd prefer to keep this bug hidden for now, and not land the
testcase yet, just in case we missed something.
Keywords: sec-low
I'm concerned about the ->StyleDisplay() call, which could potentially cause writing to an unexpected position.
Oh, I wasn't aware ->StyleDisplay() could cause memory writes.

Perhaps dbaron or cam could elaborate on how that works?
(in particular where and when writes can occur)
Flags: needinfo?(dbaron)
Flags: needinfo?(cam)
Is the issue that the nsStyleContext pointer we get is not valid, because of our incorrect casting?

If the style context you're calling StyleDisplay() on doesn't have the Display struct already cached, then we will at least have a write into the slot on the nsStyleContext to cache it.  That is basically going to do this on the style context (see the macro-defined DoGetStyleXXX functions in nsStyleContext.h):

  if (!mCachedResetData)
    mCachedResetData = allocate some space for cached reset structs;
  mCachedResetData[display] = a Display struct we get by asking the rule node;
Flags: needinfo?(cam)
If the rule node doesn't have the Display struct cached, then it will have to perform the cascade to compute a new Display struct.  If our nsStyleContext pointer is bad, then its mRuleNode member is also bad.  Not sure how far along WalkRuleTree we will get in that case, though I think we might not do any writes to the rule node or things it points to until the very end (in COMPUTE_END_INHERITED, where we maybe-cache the new struct on the rule node).
calling ->StyleDisplay() lazily computes the struct if it's not already cached, and caches it appropriately, so it does write to memory.  (And it aborts on out-of-memory.)  Not quite sure what the context of the question is, though.
Flags: needinfo?(dbaron)
dbaron, could you review this patch? or do you think I should probably ask someone else to review it? This part of code was initially reviewed by bz, but it seems that he is not reviewing patches at the moment.
Flags: needinfo?(dbaron)
I was planning to look at it, but I don't know the code very well, so it will take longer than if I knew it better.  Hopefully tomorrow.

Though if bz were available that might be better...
Flags: needinfo?(dbaron)
Here are some more details about the crash:

#0  nsStyleContext::DoGetStyleDisplay (this=0x0, aComputeData=true)
#1  nsStyleContext::StyleDisplay (this=0x0)
#2  nsCSSFrameConstructor::InterpretRubyWhitespace
#3  nsCSSFrameConstructor::WrapItemsInPseudoRubyLevelContainer
#4  nsCSSFrameConstructor::CreateNeededPseudoInternalRubyBoxes
#5  nsCSSFrameConstructor::ConstructFramesFromItemList
#6  nsCSSFrameConstructor::ProcessChildren
...

The stack data we're abusing is:
  FrameConstructionItemList itemsToConstruct;
in ProcessChildren.  We pass this this down by reference and the
ruby stuff create an iterator on it. The list is empty but we still
do 'prevIter.Prev()' on it:
https://dxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#9730
This is a PRCList so doing that gives you the address of the list
itself, i.e. &itemsToConstruct.mItems.  This is re-interpreted
as a FrameConstructionItem& (by the item() call) and we reference
its mStyleContext.  The size of FrameConstructionItem is quite
large so the location of mStyleContext is roughly ~1k above the
stack top in a Linux64 debug build.  (I would guess it's also
above the stack in an Opt build and a 32-bit build.)

FWIW, in my debug build I never get anything but a null-pointer
crash (mStyleContext).  I think we must assume though that it
could point at random memory locations .  IOW, we're using random
data as a mStyleContext* and thus its members like nsRuleNode too.
I suspect there are enough virtual calls during something like
WalkRuleTree to make this potentially exploitable.  Overwriting
a random memory location (through StyleDisplay()) with a pointer
to newly allocated style struct is perhaps less exploitable though?
Keywords: sec-lowsec-high
Attachment #8577869 - Flags: review?(dbaron) → review?(bzbarsky)
Comment on attachment 8577869 [details] [diff] [review]
patch

Please adjust the comment to talk about "ruby pseudo frames".

r=me
Attachment #8577869 - Flags: review?(bzbarsky) → review+
Comment on attachment 8577869 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1039006
[User impact if declined]: this security bug
[Describe test coverage new/current, TreeHerder]: will have crashtest later
[Risks and why]: no risk
[String/UUID change made/needed]: n/a
Attachment #8577869 - Flags: approval-mozilla-aurora?
Comment on attachment 8577869 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably easy to construct crash, but have no idea how easy is it to construct exploit.

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

Which older supported branches are affected by this flaw?
Only aurora and nightly.

If not all supported branches, which bug introduced the flaw?
Bug 1039006 which enables css ruby by default.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch can be applied to all affected version.

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

Approvals given.
Attachment #8577869 - Flags: sec-approval?
Attachment #8577869 - Flags: sec-approval+
Attachment #8577869 - Flags: approval-mozilla-aurora?
Attachment #8577869 - Flags: approval-mozilla-aurora+
May I land the patch now?
Flags: needinfo?(mats)
Yes, sec-approval+ means you can land.
Flags: needinfo?(mats)
https://hg.mozilla.org/mozilla-central/rev/8f257f3baf0c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Attached patch crashtestSplinter Review
Attachment #8580499 - Flags: review?(bzbarsky)
Comment on attachment 8580499 [details] [diff] [review]
crashtest

r=me
Attachment #8580499 - Flags: review?(bzbarsky) → review+
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
Group: core-security → core-security-release
Group: core-security-release
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.