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

VERIFIED FIXED in Firefox 28

Status

()

defect
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: aki.helin, Assigned: mstange)

Tracking

({csectype-bounds, sec-other})

Trunk
mozilla30
x86_64
Linux
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox27 unaffected, firefox28+ verified, firefox29+ verified, firefox30+ verified, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
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.
Assignee

Comment 1

6 years ago
Probably the same as bug 944579.
Status: NEW → ASSIGNED
QA Contact: mstange
Assignee: nobody → mstange
Depends on: 944579
QA Contact: mstange
Please undupe this if it isn't actually a dupe.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
No longer depends on: 944579
Resolution: --- → DUPLICATE
Duplicate of bug: 944579
Assignee

Comment 3

6 years ago
Not actually a dupe per bug 944579 comment 21, unduping.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee

Updated

6 years ago
Status: REOPENED → ASSIGNED
Assignee

Comment 4

6 years ago
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #8366256 - Flags: review?(bas)
Assignee

Comment 5

6 years ago
Comment on attachment 8366256 [details] [diff] [review]
v1

This fails reftests, something must be wrong with the condition.
Attachment #8366256 - Flags: review?(bas)
Assignee

Comment 6

6 years ago
Posted patch v2Splinter Review
This makes more sense and passes tests.
Attachment #8366256 - Attachment is obsolete: true
Attachment #8366614 - Flags: review?(bas)
Attachment #8366614 - Flags: review?(bas) → review+
Assignee

Comment 7

6 years ago
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.
Attachment #8366614 - Flags: sec-approval?
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.
Assignee

Comment 9

6 years ago
Yes, I mean Firefox 28.
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.
Flags: sec-bounty-
Go ahead and get this in. I'm clearing sec-approval as it doesn't need it as a sec-other.
Attachment #8366614 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/f3fd2eb28001
Status: ASSIGNED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Any reason to not backport this to Aurora/Beta?
Flags: needinfo?(mstange)
Assignee

Comment 15

5 years ago
Nope, no reason, it should apply as-is and is not risky. I'll request approval.
Flags: needinfo?(mstange)
Assignee

Comment 16

5 years ago
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
Attachment #8366614 - Flags: approval-mozilla-beta?
Attachment #8366614 - Flags: approval-mozilla-aurora?
Attachment #8366614 - Flags: approval-mozilla-beta?
Attachment #8366614 - Flags: approval-mozilla-beta+
Attachment #8366614 - Flags: approval-mozilla-aurora?
Attachment #8366614 - Flags: approval-mozilla-aurora+
Matt, can you please determine if this needs testing before we release?
Flags: needinfo?(mwobensmith)
Confirmed crash on ASan build, 2014-01-12.
Verified fixed on ASan builds of Firefox 28/29/30, 2014-02-19.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mwobensmith)
Group: core-security
You need to log in before you can comment on or make changes to this bug.