Bugzilla

Andrew McCreight [:mccr8]

Updated

•

10 years ago

Keywords: csectype-bounds, sec-high

Comment 1

•

10 years ago

Nathan, do you know how we could hit a buffer overflow while calling Length() on an nsTArray? That seems weird to me.

Flags: needinfo?(nfroyd)

Nathan Froyd [:froydnj]

Comment 2

•

10 years ago

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Nathan, do you know how we could hit a buffer overflow while calling
> Length() on an nsTArray? That seems weird to me.

I'd guess somebody stomping on the Header pointer for the array.  Just from looking at the stacks above, that sounds plausible...

Flags: needinfo?(nfroyd)

David Bolter [:davidb] (NeedInfo me for attention)

Comment 3

•

10 years ago

Jonathan, can you recreate this bug? (Or find someone to take a look?)

Flags: needinfo?(jfkthame)

Jonathan Kew [:jfkthame]

Comment 4

•

10 years ago

(In reply to Andrew McCreight [:mccr8] from comment #1)
> Nathan, do you know how we could hit a buffer overflow while calling
> Length() on an nsTArray? That seems weird to me.

I don't think that's exactly what's happening. The problem here is that we've got a normal gfxTextRun, as created via gfxFontGroup::MakeTextRun; but then we're treating it as an nsTransformedTextRun, which is a subclass that has some additional fields. The nsTArray we're trying to look at in SetCapitalization is one of those added fields, but because this textrun was not an nsTransformedTextRun at all, the field isn't present, and the attempt to access its header ends up looking beyond the allocated size of the object. The problem isn't that the nsTArray we're examining has been stomped; it's that there was no nsTArray there to be examined.

The "ASSERTION: Text run should be transformed!" that's firing before the ASan error confirms this, too. That assertion is at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#969, and if it fires, then the static_cast immediately following is clearly invalid, and will lead directly to the failure here.

So we need to figure out why we haven't got the kind of textrun we expected here. Offhand, my guess is that perhaps we're failing to discard and rebuild a textrun at some point when style information changes dynamically. A reduced/simplified testcase that reproduces the assertion would be awesome... maybe something that involves changing line widths, so that we're constantly re-doing linebreaking, along with toggling the text-transform:capitalize property.

Flags: needinfo?(jfkthame)

Jonathan Kew [:jfkthame]

Comment 5

•

10 years ago

(In reply to Jonathan Kew (:jfkthame) from comment #4)
>  A reduced/simplified testcase that reproduces the assertion would be
> awesome... maybe something that involves changing line widths, so that we're
> constantly re-doing linebreaking, along with toggling the
> text-transform:capitalize property.

My naive attempt to make a testcase along these lines doesn't seem to hit the problem at all. The original testcase here does hit the assertion (and then crashes), but it's far from being easily debuggable. Mats, you have any insight on how to track this down?

Flags: needinfo?(mats)

Comment 6

•

10 years ago

Attached patch wip — Details — Splinter Review

As you guessed, it's a "text-transform:capitalize" style change
that makes the existing textrun bogus since it's of the wrong type
for the new style data. 

I don't know where we're supposed to detect this and discard the
old textrun, but DidSetStyleContext seems like a good candidate.

Flags: needinfo?(mats)

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

Comment 7

•

10 years ago

It also crashes a -release DEBUG build on Linux64 so I suspect it affects
all branches.

Severity: normal → critical

status-b2g18: --- → affected

status-b2g-v1.1hd: --- → affected

status-b2g-v1.2: --- → affected

status-b2g-v1.3: --- → affected

status-b2g-v1.3T: --- → affected

status-b2g-v1.4: --- → affected

status-b2g-v2.0: --- → affected

status-b2g-v2.1: --- → affected

status-firefox31: --- → affected

status-firefox32: --- → affected

status-firefox33: --- → affected

status-firefox34: --- → affected

status-firefox-esr24: --- → affected

status-firefox-esr31: --- → affected

status-seamonkey2.26: --- → affected

Keywords: crash, testcase

OS: Linux → All

Hardware: x86_64 → All

Updated

•

10 years ago

Blocks: 950526

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

Comment 8

•

10 years ago

It seems odd that that would be necessary given that nsStyleText::CalcDifference returns NS_STYLE_HINT_REFLOW which includes nsChangeHint_ClearAncestorIntrinsics and nsChangeHint_ClearDescendantIntrinsics, and nsTextFrame::MarkIntrinsicISizesDirty already calls ClearTextRuns.  I'm curious which step along that path isn't happening as it should be.

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

Comment 9

•

10 years ago

Also, if that WIP fixes things, than anything pre-Gecko 31 is not affected, since this would be a regression from bug 950526.

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

Comment 10

•

10 years ago

I think the frame in question had not been reflowed at all, so
RestyleManager::StyleChangeReflow took the early return.

Comment 11

•

10 years ago

Interesting.

I guess I see two possible fixes for that problem:

 (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part of FrameNeedsReflow

 (2) add a separate style change hint for clearing text runs

Presumably this will show up in the form of correctness bugs as well.

(I'm curious how we managed to build text runs for a frame and not reflow it before going back to the event loop, but I guess that should be allowable, given interruptible reflow.  Or do we build text runs in frame construction?)

Comment 12

•

10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #9)
> Also, if that WIP fixes things, than anything pre-Gecko 31 is not affected,
> since this would be a regression from bug 950526.

The WIP fixes the crash, so the unconditional ClearTextRuns() in
nsTextFrame::DidSetStyleContext in builds before bug 950526 should
make them unaffected.
https://bugzilla.mozilla.org/attachment.cgi?id=8393354&action=diff#a/layout/generic/nsTextFrame.cpp_sec2

FWIW, I confirmed by testing an ESR24 DEBUG build on Linux64 without crashing.

status-b2g18: affected → unaffected

status-b2g-v1.1hd: affected → ---

status-b2g-v1.2: affected → ---

status-b2g-v1.3: affected → ---

status-b2g-v1.3T: affected → ---

status-b2g-v1.4: affected → ---

status-b2g-v2.0: affected → ---

status-b2g-v2.1: affected → ---

status-seamonkey2.26: affected → ---

Comment 13

•

10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #11)
> (I'm curious how we managed to build text runs for a frame and not reflow it
> before going back to the event loop, but I guess that should be allowable,
> given interruptible reflow.  Or do we build text runs in frame construction?)

This is the stack where we create the initial text run:
#0  nsTextFrame::SetTextRun
#1  BuildTextRunsScanner::AssignTextRun
#2  BuildTextRunsScanner::BuildTextRunForFrames
#3  BuildTextRunsScanner::FlushFrames
#4  BuildTextRuns
#5  nsTextFrame::EnsureTextRun
#6  nsTextFrame::AddInlineMinISizeForFlow
#7  nsTextFrame::AddInlineMinISize
#8  nsBlockFrame::GetMinISize
#9  nsLayoutUtils::IntrinsicForContainer
... GetMinISize / IntrinsicForContainer ...
#23 nsLayoutUtils::IntrinsicForContainer
#24 nsBlockFrame::GetMinISize
#25 nsFrame::ShrinkWidthToFit
#26 nsContainerFrame::ComputeAutoSize
#27 nsFrame::ComputeSize
#28 nsHTMLReflowState::InitAbsoluteConstraints
#29 nsHTMLReflowState::InitConstraints
#30 nsHTMLReflowState::Init
#31 nsHTMLReflowState::nsHTMLReflowState
#32 nsAbsoluteContainingBlock::ReflowAbsoluteFrame
#33 nsAbsoluteContainingBlock::Reflow
#34 ViewportFrame::Reflow
#35 PresShell::DoReflow
...

The Reflow at #33 is interrupted before we reflow the text frame (at an
ancestor 4-5 levels up).  We exit ViewportFrame::Reflow and then the
restyling happens.  The next time we enter ViewportFrame::Reflow we
crash with the same stack up to FlushFrames, see comment 0.

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

Comment 14

•

10 years ago

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Aug 27-Sep 11) from comment #11)
> I guess I see two possible fixes for that problem:
> 
>  (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part
> of FrameNeedsReflow
> 
>  (2) add a separate style change hint for clearing text runs

Both those means clearing text runs for all text frame descendants, no?

It would be nice if we could avoid that on text frames where the
style context didn't change.

Comment 15

•

10 years ago

The properties that require clearing text runs are almost entirely inherited properties, so many if not most changes of those properties will also change all descendants.

When we consider optimization, we should consider it from the starting point of correct code, and consider it when we have reason to think that optimization is necessary.  I don't think it's worth trying to micro-optimize this one case by papering over the bug rather than fixing the real problem, which will show up in a bunch of other ways than just text-transform cases (although those might be the only ones that crash).

It might make some sense to take your patch as a *secondary* belt-and-suspenders fix for the bug.  (It would be nice to assert that we don't hit the need for it, but I think things happen in the wrong order to be able to do that.)

Updated

•

10 years ago

Flags: sec-bounty?

Updated

•

10 years ago

Flags: sec-bounty? → sec-bounty+

Keywords: regression

Comment 17

•

10 years ago

Any updates on a patch for this?

Assignee: nobody → mats

status-firefox31: affected → wontfix

status-firefox32: affected → wontfix

status-firefox35: --- → affected

Updated

•

10 years ago

Flags: needinfo?(mats)

Comment 18

•

10 years ago

I guess we could take my "wip" patch as a wallpaper for branches if you want.

The preferred fix, described in comment 15, is mostly style system work.
Cameron can take it perhaps?

Assignee: mats → nobody

Component: Layout: Text → CSS Parsing and Computation

Flags: needinfo?(mats) → needinfo?(cam)

Assignee

Updated

•

10 years ago

Assignee: nobody → cam

Status: NEW → ASSIGNED

Flags: needinfo?(cam)

Assignee

Comment 19

•

10 years ago

(In reply to David Baron [:dbaron] (UTC+2) (needinfo? for questions) (away/busy until Sep 11) from comment #11)
> I guess I see two possible fixes for that problem:
> 
>  (1) push the not-reflowed-at-all test deeper in, into the dirty-bits part
> of FrameNeedsReflow
> 
>  (2) add a separate style change hint for clearing text runs

I went with the first approach.  I tried adding a change hint initially, but since I'd need to treat this new hint in pretty much the same way as the existing Clear{Ancestor,Descendant}Intrinsics hint, it didn't seem worth it in the end.

Assignee

Comment 20

•

10 years ago

Attached patch patch — Details — Splinter Review

Attachment #8487740 - Flags: review?(dbaron)

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

Assignee

Comment 21

•

10 years ago

By the way, is there a way to write a test that relies on interruptible reflow interrupting at a certain point?

Updated

•

10 years ago

Attachment #8487740 - Flags: review?(dbaron) → review+

Assignee

Comment 22

•

10 years ago

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

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

Not obvious from the patch.

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?

Whichever branches are based on Gecko 31.

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

Bug 950526.

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

No but I suspect the patch will apply to older branches since the changed bits of code haven't been touched lately.

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

Unlikely to cause regressions.  Not sure how to make an automated regression test for it.  Manual testing shows that the ASAN issue with the test case in this bug is resolved by the patch.

Attachment #8487740 - Flags: sec-approval?

Comment 23

•

10 years ago

sec-approval+ for trunk. We should get Aurora, Beta, and ESR31 patches nominated as well.

status-firefox-esr24: affected → unaffected

tracking-firefox33: --- → +

tracking-firefox34: --- → +

tracking-firefox35: --- → +

tracking-firefox-esr31: --- → +

Updated

•

10 years ago

Attachment #8487740 - Flags: sec-approval? → sec-approval+

Updated

•

10 years ago

tracking-firefox-esr31: + → 33+

Assignee

Comment 24

•

10 years ago

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

Assignee

Comment 25

•

10 years ago

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

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version: 35
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: bug 950526
[User impact if declined]: sg:high bug remains
[Describe test coverage new/current, TBPL]: tested locally with the test case in this bug; only just landed on inbound
[Risks and why]: should be low, as we're only calling MarkIntrinsicISizesDirty on some frames where we didn't before
[String/UUID change made/needed]: none

Attachment #8487740 - Flags: approval-mozilla-esr31?

Attachment #8487740 - Flags: approval-mozilla-beta?

Attachment #8487740 - Flags: approval-mozilla-aurora?

Ryan VanderMeulen [:RyanVM]

Updated

•

10 years ago

Attachment #8487740 - Flags: approval-mozilla-esr31?

Attachment #8487740 - Flags: approval-mozilla-esr31+

Attachment #8487740 - Flags: approval-mozilla-beta?

Attachment #8487740 - Flags: approval-mozilla-beta+

Attachment #8487740 - Flags: approval-mozilla-aurora?

Attachment #8487740 - Flags: approval-mozilla-aurora+

Comment 26

•

10 years ago

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

Status: ASSIGNED → RESOLVED

Closed: 10 years ago

status-b2g18: unaffected → ---

status-b2g-v1.4: --- → unaffected

status-b2g-v2.0: --- → affected

status-b2g-v2.0M: --- → affected

status-b2g-v2.1: --- → affected

status-b2g-v2.2: --- → fixed

status-firefox35: affected → fixed

Flags: in-testsuite?

Resolution: --- → FIXED

Target Milestone: --- → mozilla35

Ryan VanderMeulen [:RyanVM]

Comment 27

•

10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/80e3a7e6be35
https://hg.mozilla.org/releases/mozilla-beta/rev/dafe68644b45
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/cfedc95605d4
https://hg.mozilla.org/releases/mozilla-esr31/rev/d7140f3afd61

status-b2g-v2.0: affected → fixed

status-b2g-v2.1: affected → fixed

status-firefox33: affected → fixed

status-firefox34: affected → fixed

status-firefox-esr31: affected → fixed

Matt Wobensmith [:mwobensmith][:matt:]

Comment 28

•

10 years ago

Confirmed crash on Fx31.1.0esr, ASan build
Verified fixed on Fx31.1.1esr.

status-firefox-esr31: fixed → verified

Ryan VanderMeulen [:RyanVM]

Comment 29

•

10 years ago

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/cfedc95605d4

status-b2g-v2.0M: affected → fixed

Kamil Jozwiak [:kjozwiak]

Comment 30

•

10 years ago

Reproduced the original issue using the following build that was mentioned in comment #0:
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1405918761/

fx35: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan-debug/1412330524/

fx34: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan-debug/1412347357/

fx33: Passed
- http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan-debug/1412347593/

Test Cases used on all the channels:

- Opened the poc from comment #0 in three different windows/tabs and let each page refresh several times
- Opened the poc from comment #0 in three different private window/tabs and let each page refresh several times
- Opened the poc from comment #0 in three different e10s window/tabs and let each page refresh several times (m-c only)

Status: RESOLVED → VERIFIED

status-firefox33: fixed → verified

status-firefox34: fixed → verified

status-firefox35: fixed → verified

Flags: qe-verify+

Updated

•

10 years ago

Whiteboard: [adv-main33+][adv-esr31.2+]

Christiane Ruetten [INACTIVE]

Updated

•

10 years ago

Alias: CVE-2014-1576

Updated

•

9 years ago

Whiteboard: [adv-main33+][adv-esr31.2+] → [adv-main33+][adv-esr31.2+][b2g-adv-main2.2-]

BMO Automation

Updated

•

9 years ago

Group: core-security → core-security-release