958977 - heap-buffer-overflow (read) at mozilla::gfx::FilterProcessing::ApplyMorphologyHorizontal_SSE2

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Aki Helin]

Attachment: ff-bofr-applymorphologyhorizontal_sse2.svg

ASan reports a buffer overflow when the attached SVG file is opened. 

=================================================================
==30601==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x62b000301600 at pc 0x7fc508bce6b0 bp 0x7fffced6cab0 sp 0x7fffced6caa8
READ of size 16 at 0x62b000301600 thread T0
==30601==WARNING: Trying to symbolize code, but external symbolizer is not initialized!
    #0 0x7fc508bce6af in mozilla::gfx::FilterProcessing::ApplyMorphologyHorizontal_SSE2(unsigned char*, int, unsigned char*, int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::MorphologyOperator) /home/aki/src/mozilla-aurora/gfx/2d/FilterProcessingSSE2.cpp:44
    #1 0x7fc508c21d78 in mozilla::gfx::ApplyMorphology(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::DataSourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, int, int, mozilla::gfx::MorphologyOperator) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:1087
    #2 0x7fc508c19aa0 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #3 0x7fc508c1cca5 in mozilla::gfx::FilterNodeSoftware::GetInputDataSourceSurface(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::FilterNodeSoftware::FormatHint, mozilla::gfx::ConvolveMatrixEdgeMode, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:691
    #4 0x7fc508c2f059 in mozilla::gfx::FilterNodeCropSoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:2846
    #5 0x7fc508c19aa0 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #6 0x7fc508c1cca5 in mozilla::gfx::FilterNodeSoftware::GetInputDataSourceSurface(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::FilterNodeSoftware::FormatHint, mozilla::gfx::ConvolveMatrixEdgeMode, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:691
    #7 0x7fc508c2f612 in mozilla::gfx::FilterNodeUnpremultiplySoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:2903 
    #8 0x7fc508c19aa0 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #9 0x7fc508c1cca5 in mozilla::gfx::FilterNodeSoftware::GetInputDataSourceSurface(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::FilterNodeSoftware::FormatHint, mozilla::gfx::ConvolveMatrixEdgeMode, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const*) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:691
[...]

Filing as a potential security issue due to bug type.

Comment 1

[Markus Stange [:mstange]]

Probably the same as bug 944579.

Comment 2

[Andrew McCreight [:mccr8]]

Please undupe this if it isn't actually a dupe.

Comment 3

[Markus Stange [:mstange]]

Not actually a dupe per bug 944579 comment 21, unduping.

Comment 4

[Markus Stange [:mstange]]

Attachment: v1

This should do it.

Here's a simple example with morphology radius 1:
input image (4x1):  abcd
output image (2x1):  ef(gh)
The output image is only two pixels wide, but its storage has reserved four pixels because its stride has been aligned to 16 bytes (and each pixel is four bytes). Only pixels e and f will be used, g and h are only padding.
Now we want to have the following:
 e = min{a, b, c}
 f = min{b, c, e}
But our filter is overeager and also calculates
 g = min{c, d, ?}
 h = min{d, ?, ?}
Here's where the out-of-bounds access happens. However, as long as this doesn't cause a crash, it's completely harmless because the resulting pixels only end up in the alignment padding of the target surface. So while this bug is nice to fix, it's not a security hazard, as far as I can tell.

This patch replaces the out-of-bounds read by a zero-initialization of the relevant SIMD variable.

Comment 5

[Markus Stange [:mstange]]

Comment on attachment 8366256 [details] [diff] [review]
v1

This fails reftests, something must be wrong with the condition.

Comment 6

[Markus Stange [:mstange]]

Attachment: v2

This makes more sense and passes tests.

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8366614 [details] [diff] [review]
v2

[Security approval request comment]

This bug is not actually exploitable, as far as I can tell. We can read from a heap address that we didn't allocate into, but we never make use of the read value, it only gets processed as part of four-pixels-at-a-time SIMD processing for pixels that are only surface padding, and then ignored.


Which older supported branches are affected by this flaw?
Aurora

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch applies on Aurora without changes.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely. Not much further testing needed; I gave it lots of testing and we have morphology filter reftests in the tree.

Comment 8

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

By "Aurora," I assume you mean Firefox 28? We're just branching and building for next week's release so this may need to wait a few days.

Comment 9

[Markus Stange [:mstange]]

Yes, I mean Firefox 28.

Comment 10

[Daniel Veditz [:dveditz]]

Turns out to be harmless, but since the discussion mentions bug 944579 which we haven't released yet we should leave this hidden a bit longer.

Comment 11

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

Go ahead and get this in. I'm clearing sec-approval as it doesn't need it as a sec-other.

Comment 12

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3fd2eb28001

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3fd2eb28001

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Any reason to not backport this to Aurora/Beta?

Comment 15

[Markus Stange [:mstange]]

Nope, no reason, it should apply as-is and is not risky. I'll request approval.

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8366614 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 924102 / bug 924103
User impact if declined: none, only affects automated code-checking tools, as far as we know
Testing completed (on m-c, etc.): 5 days on mozilla-central
Risk to taking this patch (and alternatives if risky): extremely low
String or IDL/UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/546fa9fbaa1a
https://hg.mozilla.org/releases/mozilla-beta/rev/784092b96308

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/784092b96308

Comment 19

[u279076]

Matt, can you please determine if this needs testing before we release?

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on ASan build, 2014-01-12.
Verified fixed on ASan builds of Firefox 28/29/30, 2014-02-19.