Closed Bug 1375842 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Graphics, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: truber, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [gfx-noted])

Attachments

(4 files, 1 obsolete file)

Attached file 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
Flags: in-testsuite?
Attached file log.txt
Lee, can you take a look at this?
Flags: needinfo?(lsalzman)
Fallout from bug 1371689 maybe?
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.
See Also: → 1377427
Blocks: 1377005
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Flags: needinfo?(lsalzman)
Priority: -- → P1
Whiteboard: [gfx-noted]
Blocks: 1377427
This seems to be happening on launch now.
I just grabbed m-c 
Build ID: 20170704154817
Changeset: acbd9a64c0fa4e1980c7732735d4f4c2761ca4c9
Duplicate of this bug: 1377895
Duplicate of this bug: 1378393
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"
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.
Attachment #8884051 - Flags: review?(jmuizelaar)
Remove erroneous const.
Attachment #8884051 - Attachment is obsolete: true
Attachment #8884051 - Flags: review?(jmuizelaar)
Attachment #8884052 - Flags: review?(jmuizelaar)
Does this patch fix things for you?
Flags: needinfo?(jschwartzentruber)
Attachment #8884052 - Flags: review?(jmuizelaar) → review+
(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.
Flags: needinfo?(jschwartzentruber)
Duplicate of this bug: 1377005
Duplicate of this bug: 1377427
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.
Attachment #8884052 - Flags: sec-approval?
Blocks: 1371689
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.
Attachment #8884052 - Flags: sec-approval?
Can we land a test too please? :)
Just turns reporter's testcase into a crashtest. Nothing fancy.
Attachment #8884343 - Flags: review?(jmuizelaar)
Attachment #8884343 - Flags: review?(jmuizelaar) → review+
Flags: in-testsuite? → in-testsuite+
Group: gfx-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.