1337504 - (CVE-2017-5413) SEGV in ClearBidiControls

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Atte Kettunen]

Attachment: firefox-SEGV-ClearBidiControls.html

Tested on:

OS: Ubuntu 16.04
Firefox: ASAN build sourcestamp: 6d27535f4fe912068e0a0ac5854f7f39e94964a5

ASAN-trace:

ASAN:DEADLYSIGNAL
=================================================================
==15061==ERROR: AddressSanitizer: SEGV on unknown address 0x7fff31db57ac (pc 0x7f6b5302adfa bp 0x7ffd31db56d0 sp 0x7ffd31db5320 T0)
    #0 0x7f6b5302adf9 in ClearBidiControls /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:313:21
    #1 0x7f6b5302adf9 in ResolveParagraphWithinBlock /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:1226
    #2 0x7f6b5302adf9 in nsBidiPresUtils::TraverseFrames(nsBlockFrame*, nsBlockInFlowLineIterator*, nsIFrame*, BidiParagraphData*) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:1192
    #3 0x7f6b530295e6 in nsBidiPresUtils::Resolve(nsBlockFrame*) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:697:5
    #4 0x7f6b531e02b9 in ResolveBidi /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:7458:10
    #5 0x7f6b531e02b9 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1176
    #6 0x7f6b532438d0 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
    #7 0x7f6b53247f0e in nsColumnSetFrame::ReflowChildren(mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) /home/worker/workspace/build/src/layout/generic/nsColumnSetFrame.cpp:648:7
    #8 0x7f6b5324c896 in ReflowColumns /home/worker/workspace/build/src/layout/generic/nsColumnSetFrame.cpp:354:19
    #9 0x7f6b5324c896 in nsColumnSetFrame::FindBestBalanceBSize(mozilla::ReflowInput const&, nsPresContext*, nsColumnSetFrame::ReflowConfig&, nsColumnSetFrame::ColumnBalanceData&, mozilla::ReflowOutput&, nsCollapsingMargin&, bool&, bool&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsColumnSetFrame.cpp:984
    #10 0x7f6b5324d9dd in nsColumnSetFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsColumnSetFrame.cpp:1093:5
    #11 0x7f6b5320082d in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, unsigned int&, mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockReflowContext.cpp:306:3
    #12 0x7f6b531f6158 in nsBlockFrame::ReflowBlockFrame(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3466:7
    #13 0x7f6b531e9c26 in ReflowLine /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2835:5
    #14 0x7f6b531e9c26 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2374
    #15 0x7f6b531e0654 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, unsigned int&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1237:3
    #16 0x7f6b5320082d in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, unsigned int&, mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockReflowContext.cpp:306:3
.
.
.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: debug_log.txt

Comment 2

[Daniel Veditz [:dveditz]]

The relevant one appears to be
  Assertion failure: mEmbeddingStack.Length() (embedding/override underflow),
  at /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:305

Looks like those asserts were added by Xidorn in bug 1160847 so I guess we start there.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

So the issue here is that, there are some frames in overflow lines, while their continuations are in normal lines. Since nsBidiPresUtils::Resolve doesn't traverse into overflow lines, those frames do not get chance to push their bidi control, but their last continuations still try to pop the bidi control, this mismatch happens.

I'm not sure what should be fixed here. Should we make Resolve visit overflow lines? Or the caller of Resolve should ensure that there is no overflow lines? It seems that Resolve is called [1] before we draining the overflow list, so there is apparently chances that we will run the resolve algorithm when there are frames in overflow list. So probably we really should traverse overflow list as well in Resolve?

jfkthame, wdyt?

[1] https://dxr.mozilla.org/mozilla-central/rev/0eef1d5a39366059677c6d7944cfe8a97265a011/layout/generic/nsBlockFrame.cpp#1175-1176

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip

This seems like a more straightforward fix (it's what I suggested in bug 410857).
We might also want to ensure that overflow lines are drained before the other
two calls to ResolveBidi (in GetMin/PrefISize).  I'll look into that.

Comment 5

[Mats Palmgren (inactive)]

This should fix it:
https://hg.mozilla.org/try/rev/4a8a37819db4f699de68992330b1d638583b4ae0

I wonder if we could pass this off as a "correctness and performance" fix
in bug 410857 instead, to avoid drawing attention to this non-public bug?

Daniel, WDYT?

Comment 6

[Jonathan Kew [:jfkthame]]

Thanks for diagnosing this, Xidorn, and for the proposed patch, Mats! Taking it in bug 410857 sounds good, though if we want to uplift it we may need to quietly refer to this issue as justification.

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to Mats Palmgren (:mats) from comment #5)
> I wonder if we could pass this off as a "correctness and performance" fix
> in bug 410857 instead, to avoid drawing attention to this non-public bug?

Absolutely! stick the patch in that bug and get reviews over there. Don't need to link these bugs, we'll mark this one fixed when that one lands.

Comment 8

[Mats Palmgren (inactive)]

So, the root cause of the crash is that we call PopBidiControl with nothing
on the mEmbeddingStack, which executes:
mEmbeddingStack.TruncateLength(mEmbeddingStack.Length() - 1)

This leads to an mEmbeddingStack (AutoTArray<char16_t>) that looks like this internally:
(rr) p *aBpd->mEmbeddingStack.mHdr
$19 = {
  static sEmptyHdr = {
    static sEmptyHdr = <same as static member of an already seen type>, 
    mLength = 0, 
    mCapacity = 0, 
    mIsAutoArray = 0
  }, 
  mLength = 4294967295, 
  mCapacity = 16, 
  mIsAutoArray = 1
}

The 4294967295 there is due to the integer underflow.
We then crash in:
311       void ClearBidiControls()
312       {
313         for (char16_t c : Reversed(mEmbeddingStack)) {
314           AppendPopChar(c);
315         }

It seems to me the crash is caused by trying to READ the first char from
the reverse iterator, i.e. mEmbeddingStack[4294967295 - 1] or some such.
(GDB says the instruction we crash on is "movzwl (%rax),%esi", i.e. we're
trying to read something from memory into a register, IIUC.)

I suspect that with slightly different content one might be able to also
cause a PushBidiControl call, which calls mEmbeddingStack.AppendElement(aCh),
which I expect will simply OOM in this situation (EnsureCapacity(Length() + aCount)).
Either way, it seems unlikely this would be exploitable in any way.
So I suggest this is no more than sec-moderate.

Comment 9

[Mats Palmgren (inactive)]

I wonder if it'd be worth making nsTArray detect cases like this and abort.
Something like this:
https://hg.mozilla.org/try/rev/d94a80b6f90dfbcbc31fdf7a33a3677f9e068a9c
We might also want to check "newLen <= std::numeric_limits<uint32_t>::max()"
but that might already have been taken care of earlier?  (if the array is
growing then it's already checked in EnsureCapacity?)

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Mats Palmgren (:mats) from comment #9)
> I wonder if it'd be worth making nsTArray detect cases like this and abort.
> Something like this:
> https://hg.mozilla.org/try/rev/d94a80b6f90dfbcbc31fdf7a33a3677f9e068a9c
> We might also want to check "newLen <= std::numeric_limits<uint32_t>::max()"
> but that might already have been taken care of earlier?  (if the array is
> growing then it's already checked in EnsureCapacity?)

Why not make the MOZ_ASSERT in TruncateLength a MOZ_RELEASE_ASSERT?  Doing that might avoid badness in RemoveElementsAt calling DestructRange.  Are you trying to get more information out of the crash?

I think more checking in nsTArray would be a good thing.  I think we want to put the checking at a higher level than the try push patch if we can manage it, though.

Comment 11

[Mats Palmgren (inactive)]

(In reply to Nathan Froyd [:froydnj] from comment #10)
> I think more checking in nsTArray would be a good thing.  I think we want to
> put the checking at a higher level than the try push patch if we can manage
> it, though.

Yeah, putting them at the API entry points is better if we can afford it.
The one in TruncateLength doesn't seem that valuable though (other than
its nice error message), since the assertions in RemoveElementsAt would
mostly catch that:
https://dxr.mozilla.org/mozilla-central/rev/c7b015c488cfb2afbcff295a9639acd85df332f8/xpcom/ds/nsTArray.h#2098

The argument for asserting in lower level functions like ShiftData
is that it covers a lot more calls, and also internal invariants.
But yes, it would be lovely to have assertions at all API entry points.
(ReplaceElementsAt, I'm looking at you!)
We still might want to have MOZ_ASSERT for uint32_t overflow in
lower level functions though, given that mLength/mCapacity are 32-bit,
to ensure that the higher level assertions protects us from that.
Can't have too many MOZ_ASSERTs in code like this. :-)

I filed bug 1341575 for this, let's continue the discussion there.

Comment 12

[Mats Palmgren (inactive)]

This is now fixed by bug 410857.