heap-buffer-overflow (read) at mozilla::gfx::(anonymous namespace)::PowCache::Pow

VERIFIED FIXED in Firefox 28

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: aki.helin, Assigned: mstange)

Tracking

({csectype-bounds, regression, sec-moderate})

Trunk
mozilla30
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty -
in-testsuite ?

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)

ASan spots a heap-buffer-overflow when the attached SVG is opened. The repro is a valid unmodified SVG sample file, so this may be a trivial recent regression.

==28207==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x617000215576 at pc 0x7f1c59fa1b48 bp 0x7fff92bffc20 sp 0x7fff92bffc18
READ of size 2 at 0x617000215576 thread T0
==28207==WARNING: Trying to symbolize code, but external symbolizer is not initialized!
    #0 0x7f1c59fa1b47 in mozilla::gfx::(anonymous namespace)::PowCache::Pow(unsigned short) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:84
    #1 0x7f1c59fa3807 in mozilla::TemporaryRef<mozilla::gfx::DataSourceSurface> mozilla::gfx::FilterNodeLightingSoftware<mozilla::gfx::(anonymous namespace)::SpotLightSoftware, mozilla::gfx::(anonymous namespace)::DiffuseLightingSoftware>::DoRender<int>(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, int, int) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:3249
    #2 0x7f1c59f82810 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #3 0x7f1c59f85a15 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 0x7f1c59f97dc9 in mozilla::gfx::FilterNodeCropSoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:2846
    #5 0x7f1c59f82810 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #6 0x7f1c59f85a15 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 0x7f1c59f98382 in mozilla::gfx::FilterNodeUnpremultiplySoftware::Render(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:2903
[...]

Filing as a potential security issue based on bug type.
Markus, you think this is yours?
Flags: needinfo?(mstange)
Definitely.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Flags: needinfo?(mstange)
Posted patch v1 (obsolete) — Splinter Review
I must have been asleep when I wrote this code. I've added some asserts now that would definitely have caught this. The main issue here was the multiplication by 255 before calling mPowCache.Pow().
Attachment #8361774 - Flags: review?(bas)
Attachment #8361774 - Flags: review?(bas) → review+
What is the ultimate result of this bad reference? Looks like we include bogus lighting values in an SVG image? Or are there objects that get manipulated (especially if we'd call virtual methods on it) based on this cache value?
(In reply to Daniel Veditz [:dveditz] from comment #4)
> What is the ultimate result of this bad reference? Looks like we include
> bogus lighting values in an SVG image?

Exactly. If we don't crash first.
It's not only SVG images, though; you can apply filters to arbitrary html content.

> Or are there objects that get
> manipulated (especially if we'd call virtual methods on it) based on this
> cache value?

Nope, not to my knowledge. At least the filter code is very careful not to let the color values influence the code's behavior in any way.
Posted patch v2Splinter Review
I wasn't completely happy with the previous patch. This improves it a bit:
 - mPowTable now has a place for the "1.0" value.
 - the values in mPowTable are uint16_t, so sOutputIntPrecisionBits can go up to 15.
 - more overflow asserts
 - removed the "* 255.5 / 256" leftover that made it into the previous patch
Attachment #8361774 - Attachment is obsolete: true
Attachment #8365956 - Flags: review?(bas)
Blocks: 961517
Attachment #8365956 - Flags: review?(bas) → review+
Comment on attachment 8365956 [details] [diff] [review]
v2

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

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Somewhat, but it doesn't give any hints on how to exploit it.

Which older supported branches are affected by this flaw?
28 + 29

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch applies on those branches as-is.

How likely is this patch to cause regressions; how much testing does it need?
Unlikely, doesn't need much testing. Any regressions would be contained to lighting filter rendering, and lighting filters aren't really used anywhere except in some SVG demos.
Attachment #8365956 - Flags: sec-approval?
Comment on attachment 8365956 [details] [diff] [review]
v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 924102 / bug 924103
User impact if declined: small potential data leak
Testing completed (on m-c, etc.): only local testing
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none
Attachment #8365956 - Flags: approval-mozilla-beta?
Attachment #8365956 - Flags: approval-mozilla-aurora?
Comment on attachment 8365956 [details] [diff] [review]
v2

sec-approval+ and branch approvals. We have a short window on beta but I'm going by your safety comments. Please make sure trunk is green before checking into branches.
Attachment #8365956 - Flags: sec-approval?
Attachment #8365956 - Flags: sec-approval+
Attachment #8365956 - Flags: approval-mozilla-beta?
Attachment #8365956 - Flags: approval-mozilla-beta+
Attachment #8365956 - Flags: approval-mozilla-aurora?
Attachment #8365956 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 961517
https://hg.mozilla.org/mozilla-central/rev/05e5db54dd89
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Matt, can you please make sure this is verified fixed?
Flags: needinfo?(mwobensmith)
Keywords: verifyme
Confirmed crash in ASan Fx28, 2014-03-04.
Verified fix in ASan Fx28, 29, 30 built on 2014-03-11.
Status: RESOLVED → VERIFIED
Flags: needinfo?(mwobensmith)
(In reply to Matt Wobensmith from comment #16)
> Confirmed crash in ASan Fx28, 2014-03-04.
> Verified fix in ASan Fx28, 29, 30 built on 2014-03-11.

Thanks Matt.
Flags: sec-bounty?
I'm not sure this could be successfully used to read memory values. Can you reverse the value of memory by undoing the lighting effects on a canvas?
Without additional demonstration of potential exploitability we feel this bug does not qualify for the security bug bounty. If you disagree and have further demonstrations of a useful info leak please mail our security address and we'll renominate this for a bounty.
Flags: sec-bounty? → sec-bounty-
Group: core-security
You need to log in before you can comment on or make changes to this bug.