Assertion failure: mCurrentOutLine < mTargetSize.height (Past end of output), at c:/Users/mozilla/debug-builds/mozilla-central/image/Downscaler.cpp:198

VERIFIED FIXED in Firefox 44, Firefox OS v2.5

Status

()

VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: cbook, Assigned: tnikkel)

Tracking

(Blocks: 1 bug, 5 keywords)

unspecified
mozilla46
assertion, crash, csectype-bounds, regression, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox42 wontfix, firefox43+ wontfix, firefox44+ fixed, firefox45+ fixed, firefox46+ verified, firefox-esr3844+ verified, firefox-esr45 verified, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.5 fixed, b2g-v2.2r unaffected, b2g-master fixed)

Details

(Whiteboard: [gfx-noted][adv-main44+][adv-esr38.6+], URL)

Attachments

(6 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8686598 [details]
windbg information

Found via bughunter on http://www.bottari.pl/

STR:

-> Load http://www.bottari.pl/ 
-->
Assertion failure: mCurrentOutLine < mTargetSize.height (Past end of output), at
 c:/Users/mozilla/debug-builds/mozilla-central/image/Downscaler.cpp:198


filing as s-s bug just in case
(Reporter)

Updated

3 years ago
Summary: Assertion failure: mCurrentOutLine < mTargetSize.height (Past end of output), at c:/Users/mozilla/debug-builds/mozilla-central/image/Downscaler.cpp:198 Assertion failure: mCurrentOutLine < mTargetSize.height (Past end of output), at c:/Users/mozilla/deb → Assertion failure: mCurrentOutLine < mTargetSize.height (Past end of output), at c:/Users/mozilla/debug-builds/mozilla-central/image/Downscaler.cpp:198
Group: core-security → gfx-core-security
Flags: needinfo?(seth)
Keywords: csectype-bounds, sec-critical
the windbg output looks like we're dealing with a null object, which might not be so serious (because we'll safely crash when trying to use it). Could it be non-null?
Whiteboard: [gfx-noted]
(In reply to Daniel Veditz [:dveditz] from comment #1)
> the windbg output looks like we're dealing with a null object, which might
> not be so serious (because we'll safely crash when trying to use it). Could
> it be non-null?

I think the windbg output is from a debug build, which is crashing on an assertion.

Comment 3

3 years ago
Created attachment 8696213 [details]
asan report

Please note Windows builds show exploitability ratings of low with this url and that the asan builds show heap-buffer-overflow

Comment 4

3 years ago
Created attachment 8696215 [details]
beta skia::ConvolveVertically_SSE2_impl<int>

Comment 5

3 years ago
Created attachment 8696216 [details]
nightly convolveVertically_SSE2<int>
Assignee: nobody → milan
Created attachment 8702360 [details] [diff] [review]
Avoid using bad data when decoding. r=tnikkel

Leaving needinfo on Seth, he can decide if this should have been taken care of at the caller level, but this patch doesn't hurt and takes the security aspect away.  We will still assert in debug, but should not have ASAN errors.
Attachment #8702360 - Flags: review?(tnikkel)
I've been able to reproduce the assertion+crash in debug builds, but not opt builds.

Regression range: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0c777aabd5d082884809819132f8d5df8a1e1698&tochange=03be986cf1aa2e548ae6ced12975d16dbca1c248

Regression from bug 1151359.
Blocks: 1151359
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-v2.5: --- → affected
status-b2g-master: --- → affected
status-firefox42: --- → wontfix
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox-esr38: --- → unaffected
status-firefox-esr45: --- → affected
tracking-firefox43: --- → ?
tracking-firefox44: --- → ?
tracking-firefox45: --- → ?
tracking-firefox46: --- → ?
tracking-firefox-esr45: --- → ?
Keywords: crash, regression
Tracking for 43+ since this is a regression from 42 and sec-critical. 
Too late to fix in 43 though.
status-firefox43: affected → wontfix
tracking-firefox43: ? → +
tracking-firefox44: ? → +
tracking-firefox45: ? → +
tracking-firefox46: ? → +
(Assignee)

Comment 9

3 years ago
Created attachment 8703050 [details] [diff] [review]
patch

We are downscaling a ~500x30000 image to 2x2 (our decode size prediction logic is getting tricked by the size of things on the page before the final version of the page).

When doing such huge downscales the filter doesn't need to consider every input row to produce the output rows. So we produce the last output row before the last input row has been committed. We just need to ignore these trailing input rows, and increment our input row counter.
Assignee: milan → tnikkel
Attachment #8702360 - Attachment is obsolete: true
Attachment #8702360 - Flags: review?(tnikkel)
Attachment #8703050 - Flags: review?(seth)
(Assignee)

Updated

3 years ago
Flags: in-testsuite?
Flags: needinfo?(seth)
Comment on attachment 8703050 [details] [diff] [review]
patch

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

::: image/Downscaler.cpp
@@ +215,3 @@
>      }
>  
> +    MOZ_ASSERT(mCurrentOutLine < mTargetSize.height,

Is mCurrentOutLine changing inside this new if (mCurrentOutLine < mTargetSIze.height) check?  If not, we may not need this assert.
Attachment #8703050 - Flags: feedback+
(Assignee)

Comment 11

3 years ago
(In reply to Milan Sreckovic [:milan] from comment #10)
> Comment on attachment 8703050 [details] [diff] [review]
> patch
> 
> Review of attachment 8703050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/Downscaler.cpp
> @@ +215,3 @@
> >      }
> >  
> > +    MOZ_ASSERT(mCurrentOutLine < mTargetSize.height,
> 
> Is mCurrentOutLine changing inside this new if (mCurrentOutLine <
> mTargetSIze.height) check?  If not, we may not need this assert.

No, I don't think it's changing. I just went for the simplest/most conservative patch by putting the existing code (mostly) unchanged inside an if. I'm fine to remove it.
Review ping; sec-critical.
Flags: needinfo?(seth)

Comment 13

3 years ago
We are still seeing:

* crashes [@ skia::ConvolveVertically_SSE2_impl<int> ]

* Assertion failure: mCurrentOutLine < mTargetSize.height (Past end of output) [@ mozilla::image::Downscaler::CommitRow ]

* AddressSanitizer: heap-buffer-overflow's
==18637==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020000dce20 at pc 0x7f1e36665fbc bp 0x7f1e1de08f00 sp 0x7f1e1de08ef8
READ of size 4 at 0x6020000dce20 thread T23 (ImgDecoder #2)
==18637==AddressSanitizer: while reporting a bug found another one.Ignoring.
    #0 0x7f1e36665fbb in operator-> /builds/slave/m-cen-l64-asan-000000000000000/build/src/gfx/2d/convolver.h:120


On Beta/44, Aurora/45, Nightly/46 and we are two weeks out from releasing 44.

Who else can review this?
Flags: needinfo?(seth)
Attachment #8703050 - Flags: review?(seth) → review+
(Assignee)

Comment 14

3 years ago
Comment on attachment 8703050 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? If they understand the Downscaler code it's probably not that hard to figure out what situation triggers the problem (downscaling a very large image to a very small size).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? not a bulls-eye, but it's hard to obscure what causes this problem from a knowledgeable attacker.

Which older supported branches are affected by this flaw?
Bug 1045929 introduced this code, the target milestone of that bug is 38.

If not all supported branches, which bug introduced the flaw?
Bug 1045929 added the affected code, but this specific bug is blamed on bug 1151359.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch is simple, backports should be simple to create.

How likely is this patch to cause regressions; how much testing does it need?
should be pretty low risk
Attachment #8703050 - Flags: sec-approval?
status-firefox-esr38: unaffected → affected
tracking-firefox-esr38: --- → ?
Comment on attachment 8703050 [details] [diff] [review]
patch

sec-approval+

[Triage Comment]
granting a=dveditz for aurora. Since we're close to the end of the line (this week) please request beta approval ASAP. We'll also want to land this on the corresponding ESR version.
Attachment #8703050 - Flags: sec-approval?
Attachment #8703050 - Flags: sec-approval+
Attachment #8703050 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 16

3 years ago
Comment on attachment 8703050 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1045929 added the affected code, but this specific bug is blamed on bug 1151359.
[User impact if declined]: crashes, sec issue
[Describe test coverage new/current, TreeHerder]: none, can't land a sec testcase
[Risks and why]: this should be pretty safe
[String/UUID change made/needed]: none
Attachment #8703050 - Flags: approval-mozilla-beta?
Comment on attachment 8703050 [details] [diff] [review]
patch

Sec fix that security team wants us to take, Beta44+
Attachment #8703050 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 19

3 years ago
I'll cut beta/aurora/esr specific patches up asap.
(Assignee)

Comment 20

3 years ago
The attached patch applies cleanly to aurora and beta.

While applying it to esr38 I found bug 1146335 which is a fix for a similar issue, we should probably just uplift that bug too.
Merge to m-c somehow didn't get marked in here...
https://hg.mozilla.org/mozilla-central/rev/164acbd4bafc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
status-firefox-esr45: affected → fixed
tracking-firefox-esr38: ? → 44+
tracking-firefox-esr45: ? → ---
(Reporter)

Comment 23

3 years ago
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/ee694a1db741
status-b2g-v2.5: affected → fixed
status-b2g-master: affected → fixed
Why didn't we take this sec-critical on ESR38 since it is affected and tracking?
Flags: needinfo?(tnikkel)
Whiteboard: [gfx-noted] → [gfx-noted][adv-main44+]
Flags: needinfo?(lhenry)
(Assignee)

Comment 25

3 years ago
(In reply to Al Billings [:abillings] from comment #24)
> Why didn't we take this sec-critical on ESR38 since it is affected and
> tracking?

I have no idea. Was I supposed to do something for that? I thought I had done everything to get this on esr.
Flags: needinfo?(tnikkel)
Yes, this should go into 38.6.0esr. Thanks Al. Timothy, please request uplift to esr38
Flags: needinfo?(lhenry) → needinfo?(tnikkel)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8703050 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec crit
User impact if declined: crashes, security problem
Fix Landed on Version: m-c 46, uplifted to 45 and 44
Risk to taking this patch (and alternatives if risky): pretty low risk
String or UUID changes made by this patch: none

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

This patch needs bug 1146335 uplifted as well before it can apply. That bug fixes a similar type of issue, so we should take it anyway.
Flags: needinfo?(tnikkel)
Attachment #8703050 - Flags: approval-mozilla-esr38?
Comment on attachment 8703050 [details] [diff] [review]
patch

Fix for sec-critical crash, please uplift to esr38 for 38.6.0.
Attachment #8703050 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
Whiteboard: [gfx-noted][adv-main44+] → [gfx-noted][adv-main44+][adv-esr38.6+]
This busted esr38. I'm guessing I'd just need to switch out the "supports_sse2()" with "true" like it previously was[1].


1. https://hg.mozilla.org/releases/mozilla-esr38/rev/c47640f24251#l1.29
Flags: needinfo?(tnikkel)
Backed out in https://hg.mozilla.org/releases/mozilla-esr38/rev/999c13acb40e until that happens so builds are green. Here's the failure, for the record: https://treeherder.mozilla.org/logviewer.html#?job_id=53020&repo=mozilla-esr38
status-firefox-esr38: fixed → affected
(Assignee)

Comment 32

3 years ago
Created attachment 8709821 [details] [diff] [review]
esr patch

That argument should just stay as true. Another fix changed it from true to supports_sse2(), but I guess supports_sse2() was added after esr38 was cut.
Flags: needinfo?(tnikkel)
(Reporter)

Comment 33

3 years ago
https://hg.mozilla.org/releases/mozilla-esr38/rev/94a95291d095
status-firefox-esr38: affected → fixed
Group: gfx-core-security → core-security-release
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1208100

Comment 35

3 years ago
Reproduced with Nightly debug from 2015-12-15, under Mac OS X 10.11.1 ⇒ “Assertion failure: mCurrentOutLine < mTargetSize.height (Past end of output), at /builds/slave/m-cen-m64-d-000000000000000000/build/src/image/Downscaler.cpp:202” is displayed via Terminal and with Nightly from 2015-11-12 I get a crash with [@ skia::ConvolveVertically_SSE2_impl<T> ] signature [1].
Verified fixed with 46.0b11 (Build ID: 20160414152344), latest esr38 (Build ID: 20160420001510) and esr45 (Build ID: 20160420001509) tinderbox builds, across platforms [2].


[1] bp-a06b9bd9-b03a-4b13-be59-890fe2160420
[2] Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 14.04 64-bit
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
status-firefox-esr38: fixed → verified
status-firefox-esr45: fixed → verified
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.