Bugzilla

Comment 3

•

7 years ago

"sec-audit" is not "investigate a known bug", it's "we fixed a bug, let's look to see if other places use this bad pattern". This is a demonstrable crash, nothing to "audit".

buffer overflows are using sec-high or sec-critical. Even if it's only reading random crap into a glyph that's then displayed, it might be possible to get that data back out and a memory leak can lead to e.g. ASLR bypasses.

Ryan VanderMeulen [:RyanVM]

Updated

•

7 years ago

Keywords: sec-audit → sec-high

Updated

•

7 years ago

status-firefox51: --- → wontfix

status-firefox52: --- → affected

status-firefox-esr45: --- → affected

status-firefox-esr52: --- → affected

tracking-firefox52: --- → ?

tracking-firefox-esr45: --- → ?

tracking-firefox-esr52: --- → ?

Daniel Holbert [:dholbert]

Comment 4

•

7 years ago

Offhand, this looks like the sort of thing we get when we use a "stale" textrun that no longer corresponds to the frame(s) it is associated with. Mats has done an awesome job of tracking down some stuff like this in the past; any insights here?

(I'll go update an ASAN build and see if I can reproduce this shortly...)

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

Comment 5

•

7 years ago

I can't reproduce in my locally-built ASAN build (on Linux64), nor can I reproduce in one of our published ASAN trunk builds (from "mozilla-central optimized builds" link on https://developer.mozilla.org/en-US/docs/Mozilla/Testing/Firefox_and_Address_Sanitizer )

That leads me to believe that either:
 (a) there's a platform/environment difference that matters for making this reproducible.
 (b) OR perhaps Ivan is using an older build of Firefox and this has already been fixed in trunk.
 (c) OR perhaps the bug only reproduces a small fraction of the time.

Ivan, could you clarify:
 - what platform you're testing on.
 - what version of Firefox you were testing (either a changeset ID, or a link to where you got the source used, would be great. If you still have the build around, the "Built from" link at about:buildconfig would be awesome.
 - how reproducible the issue is for you (does it fail 100% of the time?)

(Alternately/also, if anyone else has tested this & has been able to reproduce, please say so.)

Flags: needinfo?(ifratric)

Daniel Holbert [:dholbert]

Assignee

Comment 6

•

7 years ago

Yeah, I can reproduce it in both an Opt ASAN build and a debug build on Linux64.
Manically resizing the window while also reloading the page seems to do it for me.

Looks like an array-index-out-of-bounds on gfxShapedText::mDetailedGlyphs.
My brain is tired though, so I'm not really sure what's going on...

Flags: needinfo?(ifratric)

Comment 7

•

7 years ago

Thanks -- resizing my browser (horizontally, from small to medium-sized) just after a reload seems to trigger the issue for me too.

Ivan Fratric

Reporter

Comment 8

•

7 years ago

Sorry about the unreliable PoC, it worked reliably on my config and I wanted to report ASAP, but it seems it is quite dependent on the window size / layout. For me it works reliably with the window.innerWidth x window.innerHeight = 960 x 827.

I confirmed the bug on mozilla-central optimized build (just did it again on the latest build) on Ubuntu 64 and in addition to this also on a custom ASan build of mozilla-central (though that one is a couple of days old) and on an ASan build of Firefox firefox-51.0.1.

I'm also attaching the original PoC as well as a slightly modified PoC that triggers OOB read that is even more outside the the bounds of the buffer than the original, so it also crashes "normal" (non-ASan) release of Firefox 51.0.1 (at least in when rendered with the same window size). Spaces and newlines in the PoC are quite important so in this case it's better to attach than copy / paste.

To comment a bit on the security impact: What is read out of bound are glyph widths. I haven't tried to exploit this further but presumably reading incorrect glyph widths would cause the differences in page layout which an attacker could use as oracle to determine values in memory they wouldn't normally be able to read. This might result in ASLR bypass, reading data cross-domain etc. Wouldn't be the most elegant exploit ever but seems like a plausible scenario.

Ivan Fratric

Reporter

Comment 9

•

7 years ago

Attached file Original PoC — Details

Ivan Fratric

Reporter

Comment 10

•

7 years ago

Attached file Modified PoC that accesses more out-of-bounds data — Details

Daniel Holbert [:dholbert]

Comment 11

•

7 years ago

With both PoCs, I hit these 4 assertion failures before ASAN crashes:
{
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', file layout/base/nsLayoutUtils.cpp, line 7479
###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'start == end || IsInLetterFrame(aSubtreeRoot)', file layout/base/nsLayoutUtils.cpp, line 7479
###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file gfx/thebes/gfxSkipChars.cpp, line 23
###!!! ASSERTION: Substring out of range: 'aRange.end <= GetLength()', file gfx/thebes/gfxTextRun.cpp, line 1040
}

The latter two assertions in particular sound like they're reporting an out-of-bounds read -- probably the same root issue -- in a way that can be detected/debugged in a normal (non-ASAN) debug build.

Keywords: assertion

Summary: out-of-bounds read in gfxTextRun → ASAN: out-of-bounds read in gfxTextRun (after "ASSERTION: Invalid offset" and "ASSERTION: Substring out of range")

Comment 12

•

7 years ago

Yes - on a (non-ASAN) debug build, I see the same sequence of assertions, followed by a fatal assertion and crash when trying to dereference a null UniquePtr.

The immediate failure occurs when we end up trying to call GetAdvanceWidth() on a textrun for a range that corresponds to a bunch of trailing spaces (from nsTextFrame::TrimTrailingWhiteSpace), but the textrun was created only for the visible text, not the (to-be-trimmed) trailing spaces, and so we read far off the end of its glyph array.

But I haven't worked back sufficiently to figure out why we end up in that situation. The PoC is a bit erratic for me; sometimes I can get it to assert/crash very quickly, other times it seems hard to reproduce. I tried to record a crash with rr, but then couldn't get replay to work; will try updating my rr and see if that helps.

Assignee

Comment 13

•

7 years ago

Attached patch wip — Details — Splinter Review

It seems the problem is that we're not clearing out the text runs
properly when deleting text frame next-in-flows.  The problem
starts when we have a couple of text frames like so:

Text(1)"aaaaaaaaaaaaaaaaaa\n"@7fee15785f30 next-in-flow=7fee1577b0f0 {0,0,41040,4380} [state=0000004020220000] [content=7fee1f3af1f0] [sc=7fee15767860:-moz-text] [run=7fee0f11b420][0,19,F] 
(rr) p mTextRun->GetLength()
$71 = 19
(rr) p canTrimTrailingWhitespace
$73 = true
Text(1)"\n\n"@7fee1577b0f0 prev-in-flow=7fee15785f30 {0,0,0,4380} [state=4000000090620004] [content=7fee1f3af1f0] [sc=7fee15767860:-moz-text] [run=0][19,2,T] 

We reflow the first continuation and find that it's complete.
This is because the 2nd continuation only has insignificant whitespace.
So we delete that continuation and end up in nsContinuingTextFrame::DestroyFrom:
http://searchfox.org/mozilla-central/rev/546a05fec017cb785fca62a5199d42812a6a1fd3/layout/generic/nsTextFrame.cpp#4425
Since these two frames have the same style context and IsInTextRunUserData()
is false, we don't clear the text runs.  So the first continuation still
has a text run that maps 19 characters, but it's content length is now 21.
Everything falls apart from there...

So this WIP tests for this condition more directly by comparing the text runs.
If we share the text run with mPrevContinuation then everything is fine since
that text run already maps the content that frame will now cover when its
continuation is destroyed.

There are still a few bogus assertions with this patch:
ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->IsEmpty() || IsInLetterFrame(aSubtreeRoot)', file layout/base/nsLayoutUtils.cpp, line 7476
This is because nsTextFrame::IsEmpty() is quite broken for text continuations.

Flags: needinfo?(mats)

Assignee

Comment 14

•

7 years ago

I'm not sure yet if 'hasPrevContinuationWithDifferentTextRun' needs to be part of
the condition for clearing text runs for 'this' frame.  That's what we did
before so I included it, but my gut feeling is that it can be removed.

Assignee

Comment 15

•

7 years ago

Hmm, it's a bit odd though that GetInFlowContentLength() returns 19,
when mContent->TextLength() is 21.  Maybe we didn't update the cached
property at some point? and we really should have reported incomplete
status from the reflow?

Assignee

Comment 16

•

7 years ago

Right, it seems the value in the nsGkAtoms::flowlength property is bogus.
When we first cache it we have frame tree like so:
Text(1)"aaaaaaaaaaaaaaaaaa\n"@7f623fee4140 next-continuation=7f62401aa370 {0,0,41040,4380} [state=0001004020020000] [content=7f623ef18860] [sc=7f623fee49f0:-moz-text] [run=7f62401920e0][0,19,F] 
Text(1)"\n"@7f62401aa370 prev-continuation=7f623fee4140 next-in-flow=7f62401aa3e8 {0,0,0,4380} [state=40010000b0620000] [content=7f623ef18860] [sc=7f623fee49f0:-moz-text] [run=7f624034f860][19,1,F] 
Text(1)"\n"@7f62401aa3e8 prev-in-flow=7f62401aa370 {0,0,0,4380} [state=40000000b0620004] [content=7f623ef18860] [sc=7f623fee49f0:-moz-text] [run=7f624034f8f0][20,1,T] 

Note that the middle frame there is a non-fluid continuation.
At this point, 19 is the correct value, but it seems we then
change the continuations without clearing this cached value...

Assignee

Comment 17

•

7 years ago

Yes, it changes here:
#0  operator|= (aLeft=@0x7f62401aa3b0: 4611686021386600452, aRight=nsFrameState::NS_FRAME_IS_FLUID_CONTINUATION) at nsFrameState.h:43
#1  nsIFrame::AddStateBits (this=0x7f62401aa370, aBits=nsFrameState::NS_FRAME_IS_FLUID_CONTINUATION) at nsIFrame.h:1829
#2  nsTextFrame::SetNextInFlow (this=0x7f623fee4140, aNextInFlow=0x7f62401aa370) at layout/generic/nsTextFrame.h:119
#3  MakeContinuationFluid (aFrame=0x7f623fee4140, aNext=0x7f62401aa370) at layout/base/nsBidiPresUtils.cpp:564
#4  nsBidiPresUtils::TraverseFrames (aLineIter=0x7ffe7e4bd080, aCurrentFrame=0x7f623fee4140, aBpd=0x7ffe7e4bd0b0) at layout/base/nsBidiPresUtils.cpp:1166
...

Assignee

Comment 18

•

7 years ago

Attached patch wip2 (obsolete) — Details — Splinter Review

This fixes the crash and all the assertions.  The frame that previously
reflowed as complete will now be incomplete since it now sees that 19 != 21.

Assignee

Comment 19

•

7 years ago

Attached patch wip2 (obsolete) — Details — Splinter Review

(Right file this time.)

Attachment #8843463 - Attachment is obsolete: true

Comment 20

•

7 years ago

Attached patch Make gfxSkipCharsIterator a bit more robust (obsolete) — Details — Splinter Review

Your diagnosis & patch for the problem here looks good; but I was wondering if it might be worth also doing something like this, to make gfxSkipCharsIterator handle bad situations more safely. AFAICT this would have prevented the OOB access here, leaving us with potentially incorrect layout but not an actual crash or vulnerability. In a debug build, it would still assert (as was already happening), so we're not just papering over the underlying bug, but a release build would have a better chance of surviving it safely. Given our history of bugs where we have textruns that don't quite match the content they're supposed to, this may be a useful mitigation?

Attachment #8843571 - Flags: feedback?(mats)

Assignee

Comment 21

•

7 years ago

Comment on attachment 8843571 [details] [diff] [review]
Make gfxSkipCharsIterator a bit more robust

This is a good idea.  The problem is we don't really know what the caller
does with the faulty offset/textrun after this, so a MOZ_RELEASE_ASSERT
is generally safer.  I see three alternatives:

A. handle it + MOZ_ASSERT (this patch)
B. A + gfxCriticalError (or some such - to get a signal from release
   builds that this occurs in the wild)
C. MOZ_RELEASE_ASSERT

I think I'd prefer C in this case, because that increases the chance of
detecting any remaining/future errors.  I think fuzz testers generally
use non-DEBUG builds, so they wouldn't catch an error with A (unless
there is also a detectable subsequent error).

Attachment #8843571 - Flags: feedback?(mats) → feedback+

Assignee

Comment 22

•

7 years ago

Attached patch part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes. — Details — Splinter Review

This should fix this bug and be very low risk to uplift.
We should also take some variation of your patch as part 2.

(I still think there's something off with the logic in
nsContinuingTextFrame::DestroyFrom, but it would be a more
risky change and I'll have to think about that more.)

Assignee: nobody → mats

Attachment #8843464 - Attachment is obsolete: true

Attachment #8843917 - Flags: review?(jfkthame)

Comment 23

•

7 years ago

Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

Review of attachment 8843917 [details] [diff] [review]:
-----------------------------------------------------------------

Agreed, this looks very low-risk, so should be fine to uplift.

Attachment #8843917 - Flags: review?(jfkthame) → review+

Comment 24

•

7 years ago

(In reply to Mats Palmgren (:mats) from comment #21)
> Comment on attachment 8843571 [details] [diff] [review]
> Make gfxSkipCharsIterator a bit more robust
> 
> This is a good idea.  The problem is we don't really know what the caller
> does with the faulty offset/textrun after this, so a MOZ_RELEASE_ASSERT
> is generally safer.  I see three alternatives:
> 
> A. handle it + MOZ_ASSERT (this patch)
> B. A + gfxCriticalError (or some such - to get a signal from release
>    builds that this occurs in the wild)
> C. MOZ_RELEASE_ASSERT
> 
> I think I'd prefer C in this case, because that increases the chance of
> detecting any remaining/future errors.  I think fuzz testers generally
> use non-DEBUG builds, so they wouldn't catch an error with A (unless
> there is also a detectable subsequent error).

Some fuzz testers do run debug builds, don't they? I'm sure I've seen plenty of fuzzer-generated bug reports that are essentially "this testcase triggers a [debug-mode] assertion".

While MOZ_RELEASE_ASSERT is safest, I'm a little nervous of doing it right away, as it might result in more frequent crashes for situations that are currently fairly harmless in the wild. E.g. consider an off-by-one error that causes us to try and access character/glyph data one position past the end of the textrun; this might result in slightly incorrect layout or rendering, but seems unlikely to be exploitable. If we have such a bug, we'd certainly want to fix it, but I'm not sure it would justify forcing a release-mode crash.

At this point I'd lean towards adding a gfxCriticalError, so that we can see if this ever appears in the wild, and then re-assess accordingly.

Comment 25

•

7 years ago

Attached patch part 2 - Record attempted misuse of gfxSkipCharsIterator via a gfxCriticalError — Details — Splinter Review

Attachment #8844051 - Flags: review?(mats)

Updated

•

7 years ago

Attachment #8843571 - Attachment is obsolete: true

Assignee

Comment 26

•

7 years ago

> Some fuzz testers do run debug builds, don't they?

Yes, I think our fuzz team do, but I think external testers usually use ASAN
or plain Opt builds.  IME, they also tend to find different sets of bugs, as
the recent streak of bidi-related bugs show.

> E.g. consider an off-by-one error that causes us to try and access character/glyph data
> one position past the end of the textrun; this might result in slightly incorrect layout
> or rendering, but seems unlikely to be exploitable.

Right, but this off-by-one error is a sign that our data is in an inconsistent
state so all bets are off.  There's no way of knowing whether this state could
lead to more serious issues.

I think the correct way to handle out-of-bounds errors in general is process
termination - that's how it's handled in nsTArray for example.  Considering
our user base is increasingly on e10s it's not that catastrophic for the user.

> At this point I'd lean towards adding a gfxCriticalError

Fair enough, given that we don't handle it at all at the moment we can
start with B and then replace it with C later on.

Assignee

Comment 27

•

7 years ago

Comment on attachment 8844051 [details] [diff] [review]
part 2 - Record attempted misuse of gfxSkipCharsIterator via a gfxCriticalError

Thanks, r=mats

Attachment #8844051 - Flags: review?(mats) → review+

Assignee

Comment 28

•

7 years ago

Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

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

That would probably be hard.

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

There's no direct correlation with what kind of content/conditions
would cause the crash.

Which older supported branches are affected by this flaw?

All.

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

Should be very easy to backport.

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

Very low risk, we're just ditching a stale cached value in part 1, and adding
an out-of-bounds runtime check in part 2.

Attachment #8843917 - Flags: sec-approval?

Assignee

Updated

•

7 years ago

Attachment #8843917 - Attachment description: fix → part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

Assignee

Updated

•

7 years ago

Severity: normal → critical

Component: Graphics: Text → Layout: Text

Keywords: crash

OS: Unspecified → All

Hardware: Unspecified → All

Assignee

Updated

•

7 years ago

Flags: needinfo?(milan)

Comment 29

•

7 years ago

sec-approval+ for trunk. We'll want branch patches made and nominated as well.

status-firefox52: affected → wontfix

status-firefox55: --- → affected

tracking-firefox52: ? → ---

tracking-firefox55: --- → +

tracking-firefox-esr45: ? → 53+

tracking-firefox-esr52: ? → 53+

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Updated

•

7 years ago

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

Comment 30

•

7 years ago

https://hg.mozilla.org/mozilla-central/rev/0f7465d1f1bb
https://hg.mozilla.org/mozilla-central/rev/80d9e84735e7

Status: NEW → RESOLVED

Closed: 7 years ago

status-firefox55: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

Liz Henry (:lizzard) (relman/hg->git project)

Assignee

Comment 31

•

7 years ago

Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]: crash or reading out-of-bounds memory contents
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]: Sure, I don't see why not.  Just load the attached testcases and verify it doesn't crash.
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:no
[Why is the change risky/not risky?]: we're just ditching a stale cached value in part 1, and adding
an out-of-bounds runtime check in part 2
[String changes made/needed]:none

(the uplift request is for both parts, for all branches that takes security fixes)

Attachment #8843917 - Flags: approval-mozilla-esr52?

Attachment #8843917 - Flags: approval-mozilla-esr45?

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

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

Comment 32

•

7 years ago

Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

Sec-high fix, let's take this up to beta right away. Should be in the build for beta 2 next week.

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

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

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

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

Ryan VanderMeulen [:RyanVM]

Comment 33

•

7 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/ec851660c406
https://hg.mozilla.org/releases/mozilla-aurora/rev/858b0ab812df

https://hg.mozilla.org/releases/mozilla-beta/rev/ddbca773eb53
https://hg.mozilla.org/releases/mozilla-beta/rev/ef9c28e77417

status-firefox53: affected → fixed

status-firefox54: affected → fixed

Flags: in-testsuite?

Julien Cristau [:jcristau]

Updated

•

7 years ago

Group: core-security → core-security-release

Comment 34

•

7 years ago

Comment on attachment 8843917 [details] [diff] [review]
part 1 - Invalidate the cached flow length when the next-in-flow/continuation changes.

sec-high fix for both esr branches

Attachment #8843917 - Flags: approval-mozilla-esr52?

Attachment #8843917 - Flags: approval-mozilla-esr52+

Attachment #8843917 - Flags: approval-mozilla-esr45?

Attachment #8843917 - Flags: approval-mozilla-esr45+

Ryan VanderMeulen [:RyanVM]

Comment 35

•

7 years ago

uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/3bc981f85a17
https://hg.mozilla.org/releases/mozilla-esr52/rev/4f752b0e5920

https://hg.mozilla.org/releases/mozilla-esr45/rev/08bc7a3330e4
https://hg.mozilla.org/releases/mozilla-esr45/rev/8c61ebe37f1b

status-firefox-esr45: affected → fixed

status-firefox-esr52: affected → fixed

Updated

•

7 years ago

Whiteboard: [adv-main53+][adv-esr52.1+][adv-esr45.9+]

Mihai Boldan, Desktop QA [:mboldan]

Updated

•

7 years ago

Alias: CVE-2017-5447

Andrei Vaida [:avaida]

Comment 36

•

7 years ago

Flagging this for manual testing, instructions in Comment 31.

Flags: qe-verify+

Comment 37

•

7 years ago

I managed to reproduce this issue on Firefox 45.1.2 ESR ASAN build, under Ubuntu 16.04x64.

The crash is no longer reproducible on Firefox 55.0a1(2017-04-12) ASAN build, Firefox 54.0a2(2017-04-12) ASAN build, 53.0b12 ASAN build, 53.0 ASAN build, Firefox 45.9.0 ESR ASAN build, Firefox 52.1.0 ESR ASAN build, 55.0a1(2017-04-12) build, Firefox 54.0a2(2017-04-12), Firefox 53.0, Firefox 45.8.0 ESR or on Firefox 52.1.0 ESR.
Tests were performed under Ubuntu 16.04x64.

Status: RESOLVED → VERIFIED

status-firefox53: fixed → verified

status-firefox54: fixed → verified

status-firefox55: fixed → verified

status-firefox-esr45: fixed → verified

status-firefox-esr52: fixed → verified

Flags: qe-verify+

Andrew McCreight [:mccr8]

Updated

•

7 years ago

URL: https://bugs.chromium.org/p/project-z...

Updated

•

7 years ago

Group: core-security-release

Keywords: csectype-bounds, testcase

Pulsebot

Comment 38

•

6 years ago

Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5e810e1c07c
Add crashtests.  r=mats