1375842 - AddressSanitizer: heap-buffer-overflow [@ hsw::convolve_vertically] with READ of size 32

Status: RESOLVED FIXED

(Core :: Graphics, defect, P1)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

The attached testcase causes a heap-buffer-overflow in mozilla-central rev 594cc32b6323.

==2513==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6060000f2c9f at pc 0x7f5820327599 bp 0x7f580afd5350 sp 0x7f580afd5348
READ of size 32 at 0x6060000f2c9f thread T17 (ImgDecoder #2)
    #0 0x7f5820327598 in hsw::convolve_vertically(short const*, int, unsigned char* const*, int, unsigned char*, bool) /home/worker/workspace/build/src/gfx/skia/skia/src/opts/SkOpts_hsw.cpp:57:54
    #1 0x7f5819c9e84d in operator() /home/worker/workspace/build/src/image/DownscalingFilter.h:283:16
    #2 0x7f5819c9e84d in WriteUnsafeComputedRow<unsigned int, (lambda at /home/worker/workspace/build/src/image/DownscalingFilter.h:281:53)> /home/worker/workspace/build/src/image/SurfacePipe.h:386
    #3 0x7f5819c9e84d in mozilla::image::DownscalingFilter<mozilla::image::SurfaceSink>::DownscaleInputRow() /home/worker/workspace/build/src/image/DownscalingFilter.h:281
    #4 0x7f5819c9e307 in mozilla::image::DownscalingFilter<mozilla::image::SurfaceSink>::DoAdvanceRow() /home/worker/workspace/build/src/image/DownscalingFilter.h:245:7
    #5 0x7f5819c97928 in AdvanceRow /home/worker/workspace/build/src/image/SurfacePipe.h:131:19
    #6 0x7f5819c97928 in DoWritePixelsToRow<unsigned int, (lambda at /home/worker/workspace/build/src/image/decoders/nsPNGDecoder.cpp:913:49)> /home/worker/workspace/build/src/image/SurfacePipe.h:499
    #7 0x7f5819c97928 in WritePixelsToRow<unsigned int, (lambda at /home/worker/workspace/build/src/image/decoders/nsPNGDecoder.cpp:913:49)> /home/worker/workspace/build/src/image/SurfacePipe.h:209
    #8 0x7f5819c97928 in WritePixelsToRow<unsigned int, (lambda at /home/worker/workspace/build/src/image/decoders/nsPNGDecoder.cpp:913:49)> /home/worker/workspace/build/src/image/SurfacePipe.h:619
    #9 0x7f5819c97928 in mozilla::image::nsPNGDecoder::WriteRow(unsigned char*) /home/worker/workspace/build/src/image/decoders/nsPNGDecoder.cpp:913

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: log.txt

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Lee, can you take a look at this?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Fallout from bug 1371689 maybe?

Comment 4

[Lee Salzman [:lsalzman]]

I have an idea of the source of this issue related to SIMD padding. This would imply there is a bug in Skia's own code related to the usage of convolution filters. I'll investigate this more thoroughly soon and post a fix.

Bug 1377427 should also be a dup of this, but I'll just mark it as a see also until I have time to verify for sure.

Comment 5

[Tyson Smith [:tsmith]]

This seems to be happening on launch now.
I just grabbed m-c 
Build ID: 20170704154817
Changeset: acbd9a64c0fa4e1980c7732735d4f4c2761ca4c9

Comment 8

[Daniel Veditz [:dveditz]]

From bug 1378393 this appears to be triggerable from web content (in an ASAN build):

"while testing http://legiaodosherois.uol.com.br/2017/vingadores-guerra-infinita-foto-pode-ter-revelado-detalhe-importante-sobre-o-homem-de-ferro-no-filme.html the asan-opt build run into a heap after use"

Comment 9

[Lee Salzman [:lsalzman]]

Attachment: increase SIMD padding for convolution filter to 31 bytes

Skia optionally uses 256 bit AVX for convolution, which means we need to increase our SIMD padding to accommodate this. Also centralizes the padding code so we only have to change it in one place.

Comment 10

[Lee Salzman [:lsalzman]]

Attachment: increase SIMD padding for convolution filter to 31 bytes

Remove erroneous const.

Comment 11

[Lee Salzman [:lsalzman]]

Does this patch fix things for you?

Comment 12

[Jesse Schwartzentruber (:truber)]

(In reply to Lee Salzman [:lsalzman] from comment #11)
> Does this patch fix things for you?

Built on 20f32734df750bddada9d1edca665c2ea53946f0 and the testcase no longer repros with your patch.

Comment 15

[Lee Salzman [:lsalzman]]

Comment on attachment 8884052 [details] [diff] [review]
increase SIMD padding for convolution filter to 31 bytes

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Only affects AVX2 (which is Haswell+). Can be used to do some small (no more than 15 bytes) memory overreads past the end of heap-allocated buffers. 

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch only mentions that we need to increase padding for SIMD operations with convolution filters, but it does not specifically point out that we're overreading.

> Which older supported branches are affected by this flaw?
> If not all supported branches, which bug introduced the flaw?
Bug 1371689, so only affects 56+, and is thus strictly confined to nightly for now.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Only affects nightly, so it would be advisable to deploy the fix now before a train flip so we can avoid having to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Extremely unlikely. Just adds some more padding to the end of image buffers. Verified by reporter to fix issue.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8884052 [details] [diff] [review]
increase SIMD padding for convolution filter to 31 bytes

Bugs that only affect m-c don't need sec-approval. Just land it.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Can we land a test too please? :)

Comment 18

[Lee Salzman [:lsalzman]]

Attachment: add crashtest for convolution filter

Just turns reporter's testcase into a crashtest. Nothing fancy.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/31f3099d7082
https://hg.mozilla.org/mozilla-central/rev/b501fbc1bbb0