1092363 - (CVE-2015-0826) Heap-buffer-overflow in nsTransformedTextRun::SetCapitalization

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Atte Kettunen]

Attachment: repro-file.html

Tested on:

OS: Ubuntu 14.04

Firefox: ASAN build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1414761627/


From stack-trace I would say this is related to bug 1041512

ASAN-trace:

==21046==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60c000352148 at pc 0x7f5f34d4cb35 bp 0x7ffff9536760 sp 0x7ffff9536758
READ of size 8 at 0x60c000352148 thread T0
    #0 0x7f5f34d4cb34 in Length /builds/slave/m-cen-l64-asan-000000000000000/build/obj-firefox/layout/generic/../../dist/include/nsTArray.h:330:0
    #1 0x7f5f34d4cb34 in IsEmpty /builds/slave/m-cen-l64-asan-000000000000000/build/obj-firefox/layout/generic/../../dist/include/nsTArray.h:333:0
    #2 0x7f5f34d4cb34 in nsTransformedTextRun::SetCapitalization(unsigned int, unsigned int, bool*, gfxContext*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextRunTransformations.cpp:60:0
    #3 0x7f5f319942ca in nsLineBreaker::AppendText(nsIAtom*, char16_t const*, unsigned int, unsigned int, nsILineBreakSink*) /builds/slave/m-cen-l64-asan-000000000000000/build/dom/base/nsLineBreaker.cpp:291:0
    #4 0x7f5f31994cfb in nsLineBreaker::AppendText(nsIAtom*, unsigned char const*, unsigned int, unsigned int, nsILineBreakSink*) /builds/slave/m-cen-l64-asan-000000000000000/build/dom/base/nsLineBreaker.cpp:327:0
    #5 0x7f5f34d04465 in BuildTextRunsScanner::SetupBreakSinksForTextRun(gfxTextRun*, void const*, unsigned int) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:2418:0
    #6 0x7f5f34cfa893 in BuildTextRunsScanner::SetupLineBreakerContext(gfxTextRun*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:2323:0
    #7 0x7f5f34cf8a93 in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:1462:0
    #8 0x7f5f34d015fd in BuildTextRunsScanner::ScanFrame(nsIFrame*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:1702:0
    #9 0x7f5f34d017dc in BuildTextRunsScanner::ScanFrame(nsIFrame*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:1712:0
    #10 0x7f5f34d06ee5 in BuildTextRuns /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:1385:0
    #11 0x7f5f34d06ee5 in nsTextFrame::EnsureTextRun(nsTextFrame::TextRunType, gfxContext*, nsIFrame*, nsLineList_iterator const*, unsigned int*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:2578:0
.
.
.
0x60c000352148 is located 12 bytes to the right of 124-byte region [0x60c0003520c0,0x60c00035213c)
allocated by thread T0 here:
    #0 0x471d71 in malloc _asan_rtl_:0
    #1 0x7f5f3139afc9 in AllocateStorageForTextRun /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxTextRun.cpp:108:0
    #2 0x7f5f3139afc9 in Create /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxTextRun.cpp:125:0
    #3 0x7f5f3139afc9 in gfxFontGroup::MakeSpaceTextRun(gfxTextRunFactory::Parameters const*, unsigned int) /builds/slave/m-cen-l64-asan-000000000000000/build/gfx/thebes/gfxTextRun.cpp:1907:0
    #4 0x7f5f34cfe45d in MakeTextRun<unsigned char> /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:566:0
    #5 0x7f5f34cfe45d in BuildTextRunsScanner::BuildTextRunForFrames(void*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:2164:0
    #6 0x7f5f34cf9120 in BuildTextRunsScanner::FlushFrames(bool, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:1481:0
    #7 0x7f5f34d015fd in BuildTextRunsScanner::ScanFrame(nsIFrame*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/generic/nsTextFrame.cpp:1702:0
.
.
.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Mats, any idea why https://hg.mozilla.org/mozilla-central/rev/4e3e41e40b7e didn't help here?

Comment 2

[Mats Palmgren (inactive)]

Attachment: restyle log + frame dump + stack

(In reply to David Baron [:dbaron] (UTC-7) (from comment #1)
> Mats, any idea why https://hg.mozilla.org/mozilla-central/rev/4e3e41e40b7e
> didn't help here?

Because DidSetStyleContext isn't called (nor is MarkIntrinsicISizesDirty)
on a bunch of text frames that have been restyled.

Here's a restyle log and the final frame tree when we start the reflow
that crashes.  Note that the text frame (7fffae0a1c30, black-on-grey)
that we reflow in this case is fine - it has a null text run. As the
stack shows, we poke around the frame tree to see if we can find an
existing text run that we can use, and bump into one of the text frames
with an invalid text run (7fffae0a2508, blue).

The interesting part is the restyle log.  It shows that we never call
RestyleContentChildren on its parent (7fffae0a2a78, lime). We do call
RestyleSelf on that chain though: magenta, cyan, lime, white, yellow,
but we only call RestyleContentChildren for the latter two.  I suspect
we should have called RestyleContentChildren also for the former.

Comment 3

[Mats Palmgren (inactive)]

Attachment: debug improvements

Here's a patch that detects this bug in VerifyStyleTree.
I also added printing aParent in RestyleContentChildren to make it easier
to read the restyle log output.

Comment 4

[Mats Palmgren (inactive)]

Attachment: disable-IME-assert.diff

You probably want to apply this patch temporarily as well if you're
debugging this, because that assert fails for this test.

Comment 5

[Mats Palmgren (inactive)]

Masayuki-san, the testcase in this bug triggers the IME assertion (in bug 1047588) quite reliably.

Comment 6

[Mats Palmgren (inactive)]

I'm guessing this is a regression from bug 931668 as I can reproduce it on Aurora, but not
Beta or Release branches.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mats Palmgren (:mats) from comment #5)
> Masayuki-san, the testcase in this bug triggers the IME assertion (in bug
> 1047588) quite reliably.

Oh, that's what I'm creating a patch for. I still testing my patch, though, perhaps, it might help this.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I landed a patch for fixing bug 1047588 (https://hg.mozilla.org/integration/mozilla-inbound/rev/f86dbba73d43). So, you don't need attachment 8515485 [details] [diff] [review] anymore. Sorry for the delay to fix that.

Comment 9

[Cameron McCormack (:heycam)]

Attachment: small test that triggers nsTextFrame::DidSetStyleContext assertion

While trying to reduce the test case a bit, I noticed that we can trigger the assertion in nsTextFrame::DidSetStyleContext with this very simple document.  It's not really a problem though since we'll have a reflow in the change list that we'll process soon after.  So maybe it is better to have this check only in VerifyStyleTree?

Comment 10

[Cameron McCormack (:heycam)]

Also I don't seem to run into the VerifyStyleTree assertion from your comment 3 patch, at least here on OS X.

Comment 11

[Mats Palmgren (inactive)]

(In reply to Cameron McCormack (:heycam) from comment #9)
> While trying to reduce the test case a bit, I noticed that we can trigger
> the assertion in nsTextFrame::DidSetStyleContext with this very simple
> document.

The assertion in DidSetStyleContext gives a false positive when we
actually do get a MarkIntrinsicISizesDirty call that would fix it.

> So maybe it is better to have this check only in VerifyStyleTree?

It would be better (no false positives) if it weren't for the fact
that we don't always run VerifyStyleTree on the full tree, AIUI.


(In reply to Cameron McCormack (:heycam) from comment #10)
> Also I don't seem to run into the VerifyStyleTree assertion from your
> comment 3 patch, at least here on OS X.

Try NSPR_LOG_MODULES=styleverifytree:1 (or higher)

Comment 12

[Cameron McCormack (:heycam)]

(In reply to Mats Palmgren (:mats) from comment #11)
> The assertion in DidSetStyleContext gives a false positive when we
> actually do get a MarkIntrinsicISizesDirty call that would fix it.

OK.  Seems like we'd get a lot of false positives then -- it's going to trigger any time we change the value of text-transform isn't it?

> > So maybe it is better to have this check only in VerifyStyleTree?
> 
> It would be better (no false positives) if it weren't for the fact
> that we don't always run VerifyStyleTree on the full tree, AIUI.

Fair enough.

> (In reply to Cameron McCormack (:heycam) from comment #10)
> > Also I don't seem to run into the VerifyStyleTree assertion from your
> > comment 3 patch, at least here on OS X.
> 
> Try NSPR_LOG_MODULES=styleverifytree:1 (or higher)

Thanks that works.

Comment 13

[Mats Palmgren (inactive)]

(In reply to Cameron McCormack (:heycam) from comment #12)
> OK.  Seems like we'd get a lot of false positives then -- it's going to
> trigger any time we change the value of text-transform isn't it?

Shouldn't that have generated lot's of assertions for existing tests then
when it landed (there was none)?  It seems unlikely we don't have
a single test changing the value of text-transform.

> (In reply to Mats Palmgren (:mats) from comment #11)
> > It would be better (no false positives) if it weren't for the fact
> > that we don't always run VerifyStyleTree on the full tree, AIUI.

OTOH, I see that we do call VerifyStyleTree unconditionally on some
frames in RestyleManager.  If that set includes the frames we call
DidSetStyleContext on, then we can replace the assertion.

For the crash testcase in this bug though, I'm pretty sure we need
to run VerifyStyleTree on the full tree to trigger that assertion.

Comment 14

[Cameron McCormack (:heycam)]

Attachment: reduced test that triggers the VerifyStyleTree assertion

Here's a reduced test that triggers the assertion in VerifyStyleTree.

Comment 15

[Cameron McCormack (:heycam)]

Attachment: restyle log from reduced test

And here's the restyle logging output.  It's surprising that when the text frames are restyled at the end that we end up returning eRestyleResult_Stop when we really should have had different style data (the text-transform change) and therefore returned eRestyleResult_Continue.

Comment 16

[Cameron McCormack (:heycam)]

Attachment: even smaller test case

Here's a smaller test case that triggers the VerifyStyleTree assertion if you change it to check for mTextTransform != NS_STYLE_TEXT_TRANSFORM_NONE (rather than looking for 'capitalize' specifically).

Comment 17

[Cameron McCormack (:heycam)]

So here's what happening with the smaller test case.  The relevant part of the frame tree starts off as (after dropping the lines):

Block(body)(1)@11c370010 [content=1003ed380] [sc=11c370a60]<
  Inline(span)(0)@11c370830 next=11c3b3250 IBSplitSibling=11c3b3250 [content=11c3697d0] [sc=11c370788]<
    Text(0)"a"@11c3709a0 [content=11c369860] [sc=11c36fd58:-moz-non-element] [run=11a357400][0,1,T]
  >
  Block(span)(0)@11c3b3250 next=11c3b3358 IBSplitSibling=11c3b3358 IBSplitPrevSibling=11c370830
                           [content=11c3697d0] [sc=11c370f40:-moz-anonymous-block,parent=11c370788]<
    Block(div)(1)@11c370c70 [content=1003edba0] [sc=11c3708f8,parent=11c370788]<
      Text(0)"b"@11c370db0 [state=00000040b0600000] [content=11c3698f0] [sc=11c370d08:-moz-non-element]
                           [run=11c36c080][0,1,T]
    >
  >
  Inline(span)(0)@11c3b3358 IBSplitPrevSibling=11c3b3250 [content=11c3697d0] [sc=11c370788]<>
>

and the style context tree starting from the <span> looks like this:

11c370788(5) Text=11c370a10(dependent) parent=11c370a60          <span>
  11c370f40(1) Text=11c370a10(dependent) :-moz-anonymous-block   <span>'s IB split block wrapper
  11c3708f8(2) Text=11c370a10(dependent)                         <div>
    11c370d08(1) Text=11c370a10(dependent) :-moz-non-element     "b"
  11c36fd58(1) Text=11c370a10(dependent) :-moz-non-element       "a"

Then:

1. We have two restyles pending: one for the <span> and one for the <div>.
2. We start the restyle process by restyling the <span>.  It doesn't have any style changes, which means
   we end up swapping all of its structs to the new style context we just created for it.  We then
   recurse down and restyle the "a" frame, which again has no style changes (though here we return
   eRestyleResult_Stop and just leave its old style context there).
3. We get into this if statement:

     http://hg.mozilla.org/mozilla-central/file/tip/layout/base/RestyleManager.cpp#l2626

   The purpose of this ClearCachedInheritedStyleDataOnDescendants call is to ensure we don't leave
   descendants of the old style context with old cached struct pointers that were swapped into the new
   style context (which is what we did in step 2).  This ends up clearing the cached Text struct out
   from "b"'s style context.
4. When we eventually get to restyling "b", nsStyleContext::CalcDifference doesn't compare the Text structs
   because we don't have one cached on the old style context, since we cleared it in step 3.  But that
   makes us think that we never did request the Text struct and that it's OK to ignore.  Which it isn't.

So there are a few problems:

(a) I had thought ClearCachedInheritedStyleDataOnDescendants should only be run in situations where
    some other strong reference to a style context is kept, because normally oldContext (by the end of
    ElementRestyler::Restyle) should be about to go away.  This was intended to be an optimisation,
    but it looks like it could be stronger, since here we have a continuation with the same style,
    which we haven't restyled yet (the final <span> continuation in the frame tree above, the one
    after the anonymous block).

(b) ClearCachedInheritedStyleDataOnDescendants in fact isn't just an optimisation, but has an observable
    effect.  As mentioned in step 4, we interpret null cached struct pointers on a style context to
    mean that we never requested the struct and thus we don't need to compare it to the new style
    context (which would, if we forced its Text struct to be computed, have the new text-transform value).

(c) When we fill in aEqualStructs in nsStyleContext::CalcStyleDifference, we set bits indicating equality
    even for cases where the cached struct pointers were null.  This makes it easier back up in
    ElementRestyler::RestyleSelf to see if any structs were swapped.  But it also means we can wastefully
    swap null pointers between old/new style contexts in the SwapStyleData call that follows.  We use
    this same bitfield of swapped structs when calling ClearCachedInheritedStyleDataOnDescendants so
    we do a bit of wasted work in there too.

The main thing to do is fix (b) first.  Probably the easiest thing to do is not to write nulls into the cached style data blindly until we reach an owned non-null struct pointer, but to write nulls only over the struct pointers that we moved into the new style context.

Comment 18

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=09a7a3e8e8ed

Comment 19

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #17)
> ElementRestyler::Restyle) should be about to go away.  This was intended
> to be an optimisation, but it looks like it could be stronger, since here we
> have a continuation with the same style, which we haven't restyled yet (the
> final <span> continuation in the frame tree above, the one after the
> anonymous block).

That's not quite right.  We *have* restyled the continuation (we've gone through the restyle-all-same-style-continuations, after all).  But we haven't restyled the IB anonymous block, whose child's style context still inherits from the <span>'s old style context.  That's where the remaining reference comes from.

Comment 20

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #18)
> https://tbpl.mozilla.org/?tree=Try&rev=09a7a3e8e8ed

On top of a non-broken inbound revision this time: https://tbpl.mozilla.org/?tree=Try&rev=f08a1993bf44

Comment 21

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8522689 [details] [diff] [review]
patch

Please call the aStyleContext parameter to
ClearCachedInheritedStyleDataOnDescendants
and DoClearCachedInheritedStyleDataOnDescendants
aNewContext instead.

>+      } else if (aStyleContext->GetCachedStyleData(i) == aStyleContext) {
(twice)

This test doesn't make sense.  You're comparing a struct to a style
context, which will never be true.

(It compiles because the style struct is a void*.)


I'm still working on understanding the main issue here, but marking
review- on the basis of that test.


sorry for the delay getting to this.

Comment 23

[Cameron McCormack (:heycam)]

When I fix the cached struct comparisons, my newly added reftest fails.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What did you fix them to be?

Comment 25

[Cameron McCormack (:heycam)]

      } else if (mCachedInheritedData.mStyleStructs[i] ==
                   aStyleContext->GetCachedStyleData(i)) {

and similar for the other case.

Comment 26

[Mats Palmgren (inactive)]

What's the status of this bug? will the fix be ready for v35?

Comment 27

[Lukas Blakk [:lsblakk] use ?needinfo]

We don't have any more betas left for Desktop 35, so depending on the risk we might not be able to take this.  Please nominate with risk/reward evaluation.

Comment 28

[Cameron McCormack (:heycam)]

Sorry this fell off my radar.  I don't have a fix right now; the r-ed patch, after fixing the problem, didn't work, so it's going to need more investigation.  The only quick fix workaround I think of is to neuter bug 931668 by setting |RestyleResult result = eRestyleResult_Continue;| unconditionally in ElementRestyler::RestyleSelf and to skip the struct swapping.  Feels risky without beta testing though and will be a performance regression (though probably not noticed on Talos).  Doing this avoids the ASAN assertion and then reaches Mats' comment 1 assertion (and safe handling).

David/Mats what do you think?

Comment 29

[Cameron McCormack (:heycam)]

Attachment: disable bug 931668 optimisation

Like this.

Comment 30

[Mats Palmgren (inactive)]

I think we should take the wallpaper in bug 1099110 as well.  It might reveal
that we have some bug related to 'text-transform' but it seems worth that risk.
I'm hoping it would make a crash like this non-exploitable, or even not crash
at all.

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment 28 makes it clear this is a miss for 35, wontfixing.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Do we still need to do this if we take bug 1099110?  (I'd certainly rather not disable it if we can avoid it.)

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8543122 [details] [diff] [review]
disable bug 931668 optimisation

Though I suppose comment 17 item 4 is pretty bad, so we should probably disable for now (but figure out how to fix it!).

Comment 34

[Al Billings [:abillings - ex-MoCo]]

Cameron, can this land soon on trunk and then on affected branches? This will need the sec-approval? form answered.

Comment 35

[Cameron McCormack (:heycam)]

Comment on attachment 8543122 [details] [diff] [review]
disable bug 931668 optimisation

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily 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?

No.

Which older supported branches are affected by this flaw?

All based on Firefox 35 or later.

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

bug 931668

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

The patch applies as is on beta and aurora.

How likely is this patch to cause regressions; how much testing does it need?

I think not that likely; it reverts to pre-bug 931668 restyling, although it does so in a manner differently from backing out all of those patches.  So there is a small risk I think.

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8543122 [details] [diff] [review]
disable bug 931668 optimisation

Discussed this in IRC. Giving branch approvals and sec-approval+

Comment 37

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed9225015e22

Comment 38

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/79fcabb42355
https://hg.mozilla.org/releases/mozilla-beta/rev/126d92ac00e9

Comment 39

[Cameron McCormack (:heycam)]

Do I need to do anything about b2g-v2.2 and b2g-v3.0?  I don't know what repositories they correspond to.

Comment 40

[Daniel Holbert [:dholbert]]

It looks like this caused a 5-15% regression in animation / tab-animation tests, FWIW.  (At least, I got a handful of perf-regression automated emails for having pushed a no-net-code-changes push right before this bug here's push.  Most of the regression emails had this regression range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=490ab676c5a4&tochange=ed9225015e22
...including only my no-net-code-changes push & this bug's push. (so, this bug's push is the only thing potentially responsible)

The regression emails I got (so far) were:
  Mozilla-Inbound-Non-PGO - Customization Animation Tests - WINNT 5.1 (ix) - 5.39%
  Mozilla-Inbound-Non-PGO - Tab Animation Test - WINNT 5.1 (ix) - 4.66%
  Mozilla-Inbound-Non-PGO - Customization Animation Tests - Ubuntu HW 12.04 - 15.8%
  Mozilla-Inbound-Non-PGO - Customization Animation Tests - Ubuntu HW 12.04 x64 - 14.2%
  Mozilla-Inbound-Non-PGO - Tab Animation Test - Ubuntu HW 12.04 x64 - 6.59%
  Mozilla-Inbound-Non-PGO - Customization Animation Tests - WINNT 6.1 (ix) - 12.4%
  Mozilla-Inbound-Non-PGO - Customization Animation Tests - WINNT 6.2 x64 - 21.5%

I'm not (necessarily) suggesting that any of this means we should back this out, since it's fixing a security bug, and we've got to weight that against the perf hit. Just mentioning it here, since it seems worth noting.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

(In reply to Cameron McCormack (:heycam) from comment #39)
> Do I need to do anything about b2g-v2.2 and b2g-v3.0?  I don't know what
> repositories they correspond to.

No to both. 3.0 is trunk and 2.2 is b2g37 (which gets merges from Aurora during this cycle).

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/79fcabb42355

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed9225015e22

I assume that fixing the underlying bug and re-enabling the optimization is going to happen in a new bug. Feel free to reopen otherwise.

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed bug 1125391 on reënabling the optimization.

Comment 45

[Cameron McCormack (:heycam)]

I am continuing the investigation into the underlying issue in bug 1127198.