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

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?

(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.

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

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.

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.

Tracking for 43+ since this is a regression from 42 and sec-critical. Too late to fix in 43 though.

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.

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.

(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.

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?

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

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.

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

Comment on attachment 8703050 [details] [diff] [review] patch Sec fix that security team wants us to take, Beta44+

I'll cut beta/aurora/esr specific patches up asap.

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef269a2009e1 https://hg.mozilla.org/releases/mozilla-beta/rev/ee694a1db741

Why didn't we take this sec-critical on ESR38 since it is affected and tracking?

(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.

Yes, this should go into 38.6.0esr. Thanks Al. Timothy, please request uplift to esr38

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.

Comment on attachment 8703050 [details] [diff] [review] patch Fix for sec-critical crash, please uplift to esr38 for 38.6.0.

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

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

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.

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