1409440 - Crash in sse2::convolve_horizontally

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

This bug was filed from the Socorro interface and is 
report bp-7b0725ed-cf5d-4bf6-b5e2-555e90171016.
=============================================================

UAF/wildptr crashes starting in 56 (56b1 seems to be the earliest crash recorded).  Writes and reads.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

-> milan for triage

Note: this is in JPEG decoding, trying to write to the bitmap generally.  It appears something is letting the buffer go away

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

STR will be tough; it smells like a timing issue (perhaps with navigation?)

Comment 3

[Timothy Nikkel (:tnikkel)]

Is this any different then bug 1353620?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

maybe not - the one I looked at there was called from PNG decode; but I suspect this is a weakness in all the decoder paths (buffer gets yanked from underneath the decoder).

It also might be a signature change starting in 56

Comment 5

[Wennie]

Hi Milan:

I have assigned these security bugs to you to reassign them to appropriate developers in your team to investigate and fix them.

Thanks!

Wennie

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Look if this is still around after we do a Skia update in 60.

Comment 7

[Lee Salzman [:lsalzman]]

Looking at the crash report, you can clearly see this is the imagelib code sending a deallocated address into the Skia convolver code, rather than the Skia convolver code in particular doing anything wrong. It looks like mWindow itself is no longer valid, or mLinesInBuffer is overreading it into a currently freed section of the heap, so that when mWindow[mLinesInBuffer++] yields the bogus address in question (0xFFFFE5E5) to the convolver code, it goes boom.

I'm going to punt this over to Andrew and Timothy to look into.

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

I can see the mLinesInBuffer is not initialized in Downscaler::Downscaler called in Decoder::PostSize but it should be done by Downscaler::BeginFrame which, from what I can tell, is always called before we start using mDownscaler for anything.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

What seems more plausible to me is that mLinesInBuffer++ overflowed mWindow. Since mWindow is a UniquePtr of a pointer array instead of an nsTArray, it won't crash immediately if we overflow.

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

This happens with other decoders too that use the DownscalingFilter / SurfacePipe instead, e.g. PNG decoder:

https://crash-stats.mozilla.com/report/index/c202fb4a-3238-4e1a-b67a-c43af0180319

The relevant logic is more-or-less identical, so if there was a problem in Downscaler::mLinesInBuffer itself or the filter methods it derives the value from, this corroborates that.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

There is another crash variant for convolving vertically, e.g.:

https://crash-stats.mozilla.com/report/index/6f9e6f11-d155-44e0-944d-038080180321

Going through minidumps makes me question whether or not we actually overflowed mWindow in Downscaler::CommitRow. It should not be possible to overflow mWindow via mYFilter.ConvolveVertically, since it should be larger than the biggest filter, but we still get 0xe5e5e5e5.

In Downscaler::DownscaleInputLine we do some row swapping, and we don't do any bounds checking here. So in theory it could overflow mWindow, swap in 0xe5e5e5e5, but not crash there since it doesn't dereference it. Then we trip it on either the vertical or horizontal convolve. That said, I haven't manage to poke a hole in the swapping logic yet.

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1409440.patch, v1

I have not reproduced myself, so while it is an educated guess at a fix, this is still speculative based on the crash reports. I cannot conceive of an image original size and downscale size that will create the necessary conditions, but when we create the filters in ConvolutionFilter::ComputeResizeFilter, there is some rounding from floating point going on that may be playing into this.

I tweaked the algorithm to support the case where we already have more rows buffered in our window than the next filter requires. Before we would underflow mWindow when performing the row pointer swaps, and with this patch we will not perform the swap at all (because we have all of the data we need) and continue looping on DownscaleInputLine/Row.

Additionally, if by some happenstance mLinesInBuffer/mRowsInWindow exceeds the window capacity (it should never, by definition) when we recalculate it after a vertical convolution, I limit it to mWindowCapacity in the calculations, similar to how we prevent it from going negative (which it should also never). This should prevent any overflows during swapping. (By definition mWindowCapacity >= filterLength, so I didn't check for that again. The filter lengths are fixed at the time of calling ComputeResizeFilter.)

Finally, I release assert that the index is less than mWindowCapacity as a final failsafe. If we end hitting that release assert, then we know something else went wrong, but it is a safe crash.

Comment 13

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8962427 [details] [diff] [review]
0001-Bug-1409440.patch, v1

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

Nice catch to all of this.

::: image/Downscaler.cpp
@@ +201,5 @@
>  
>      MOZ_ASSERT(mCurrentOutLine < mTargetSize.height,
>                 "Writing past end of output");
>  
> +    while (mLinesInBuffer >= filterLength) {

I could definitely see use hitting this condition.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52c14d32d981

Judging by the crash frequency on Nightly, we may want to just proceed with the Beta uplift request before formal verification to get a better sense of whether there's more to be done here or not. It grafts cleanly as-landed, so go ahead and request approval when you're comfortable doing so.

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8962427 [details] [diff] [review]
0001-Bug-1409440.patch, v1

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: In rare cases it may overwrite memory elsewhere in the process while downscaling a JPEG during decoding. It may be possible to craft a JPEG / website to cause this consistently if you understand the mechanism.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: It is in nightly, but the low reproduction rate in the wild and no STR make verification difficult. Uplifting to beta will help confirm/deny the fix.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: The common case is unchanged. The patch only affects edge cases which are currently already broken and causing overwrites/crashes. 
[String changes made/needed]: None.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8962427 [details] [diff] [review]
0001-Bug-1409440.patch, v1

Approved for 60.0b8.

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8962427 [details] [diff] [review]
0001-Bug-1409440.patch, v1

It is only now that I am realizing I missed the sec-approval... Mea culpa x100 :(. It is already in mozilla-central.

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

The theory of what to do (craft an image of some size, and request downscale to a certain size) is very intuitive from the patch. What that combination is not clear to me.

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

The changes themselves are sufficient to indicate we overflowed.

> Which older supported branches are affected by this flaw?

I believe the issue was introduced in 38, and crash reports in the last few months suggest at most 40. So all branches.

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

Bug 1045929.

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

This code has not evolved much, and the patch applies to release and beta cleanly. It applies to esr52 *almost* cleanly; I will attach a patch for it forthwith. The changes were just to help the merge along, very small.

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

I believe it is unlikely to cause regressions. It is well contained, and should only change behaviour when it was already broken and causing the issue at hand.

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [esr52] 0001-Bug-1409440.patch, v1 [carries r=tnikkel]

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e724c32284b2

Comment 20

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

(In reply to Andrew Osmond [:aosmond] from comment #17)
> Comment on attachment 8962427 [details] [diff] [review]
> 0001-Bug-1409440.patch, v1
> 
> It is only now that I am realizing I missed the sec-approval... Mea culpa
> x100 :(. It is already in mozilla-central.

Sigh.

sec-approval+ after the fact. At least you didn't check in tests.

We need an ESR52 patch made and nominated?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8963283 [details] [diff] [review]
[esr52] 0001-Bug-1409440.patch, v1 [carries r=tnikkel]

Approved for ESR 52.8.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/2f3e1ccf1661

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

Looked at the crash reports on nightly since it landed, but I suspect it is unrelated. The pointer looks fine / aligned, and SIGBUS could easily just be Linux's overcommit was too aggressive. It was for a PNG, and assuming it has has alpha, then the first time we write to a downscaled PNG would be in the downscaler path:

https://crash-stats.mozilla.com/report/index/ccbcf90d-a08e-4f69-bc72-e6d730180330

The same install received a similar SIGBUS crash here:

https://crash-stats.mozilla.com/report/index/e96c964b-e1f1-4803-bb10-23bb50180330

This time for a JPEG, but it was when creating the frame, not in the downscaler. We memset for JPEGs since they lack alpha, and so the first time we write is sooner.

I also note the user has WebRender enabled which has a higher memory footprint at this time.

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Bug 1353620 was for skia::ConvolveHorizonally, and that signature seems to end with FF 55

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

andrew - I see a number of 60b crashes after this landed, such as https://crash-stats.mozilla.com/report/index/81e34421-e6e3-4983-a4c5-58f060180426 with wildptr addresses.  The e5e5 crashes do seem to have gone away (so far, volume is low).

https://crash-stats.mozilla.com/signature/?signature=sse2%3A%3Aconvolve_horizontally&date=%3E%3D2018-04-08T08%3A12%3A59.000Z&date=%3C2018-05-08T08%3A12%3A59.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-version&_sort=-date&page=1

There are a bunch of 61 (only) ConvolveVertically<> crashes with SIGBUS; these seem to have started when your patch landed.  (There are 2 before your patch in total, and 24 since it landed) - if it's not new, it spiked on your landing.

https://crash-stats.mozilla.com/signature/?platform=Linux&signature=sse2%3A%3AConvolveVertically%3CT%3E&date=%3E%3D2018-02-08T06%3A10%3A01.000Z&date=%3C2018-05-08T08%3A10%3A01.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1

Comment 27

[Andrew Osmond [:aosmond] (he/him)]

It is hard for me to tell if the remaining volume is just noise or a different problem. We release assert for the original problem, so I'm fairly confident we are not hitting that anymore.