960017 - heap-buffer-overflow (read) at mozilla::gfx::ColorComponentAtPoint

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Aki Helin]

Attachment: ff-bofr-colorcomponent.svg

ASan spots a heap buffer overflow (read) when the attached SVG is opened in a recent Firefox build.

==15678==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f8775a17e91 at pc 0x7f87978c03d4
 bp 0x7fff6dae1ef0 sp 0x7fff6dae1ee8
READ of size 1 at 0x7f8775a17e91 thread T0
==15678==WARNING: Trying to symbolize code, but external symbolizer is not initialized!
    #0 0x7f87978c03d3 in mozilla::gfx::ColorComponentAtPoint(unsigned char const*, int, int, int, uns
igned long, long) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:2044
    #1 0x7f87978c1ed7 in mozilla::TemporaryRef<mozilla::gfx::DataSourceSurface> mozilla::gfx::FilterN
odeLightingSoftware<mozilla::gfx::(anonymous namespace)::DistantLightSoftware, mozilla::gfx::(anonymo
us namespace)::DiffuseLightingSoftware>::DoRender<float>(mozilla::gfx::IntRectTyped<mozilla::gfx::Unk
nownUnits> const&, float, float) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:3241
    #2 0x7f87978a6af0 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozil
la::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #3 0x7f87978a9cf5 in mozilla::gfx::FilterNodeSoftware::GetInputDataSourceSurface(unsigned int, mo
zilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::FilterNodeSoftware::Format
Hint, mozilla::gfx::ConvolveMatrixEdgeMode, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> co
nst*) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:691
    #4 0x7f87978bc0a9 in mozilla::gfx::FilterNodeCropSoftware::Render(mozilla::gfx::IntRectTyped<mozi
lla::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:2846
    #5 0x7f87978a6af0 in mozilla::gfx::FilterNodeSoftware::GetOutput(mozilla::gfx::IntRectTyped<mozil
la::gfx::UnknownUnits> const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cpp:605
    #6 0x7f87978a640a in mozilla::gfx::FilterNodeSoftware::Draw(mozilla::gfx::DrawTarget*, mozilla::g
fx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits
> const&, mozilla::gfx::DrawOptions const&) /home/aki/src/mozilla-aurora/gfx/2d/FilterNodeSoftware.cp
p:573
    #7 0x7f87937b0267 in mozilla::gfx::FilterSupport::RenderFilterDescription(mozilla::gfx::DrawTarge
t*, mozilla::gfx::FilterDescription const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const
&, mozilla::gfx::SourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozil
la::gfx::SourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx:
:SourceSurface*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, nsTArray<mozilla::Ref
Ptr<mozilla::gfx::SourceSurface> >&) /home/aki/src/mozilla-aurora/gfx/src/FilterSupport.cpp:1116
[...]

I started running Firefox tests recently and have now filed a few filter-related SVG bugs with different ASan traces. Sorry if these turn out to have the same root cause.

Filing as a security issue based on bug type.

Comment 1

[Andrew McCreight [:mccr8]]

Could you look at this, mstange?  Thanks.

Comment 2

[Markus Stange [:mstange]]

Probably also fixed by bug 944579. Aki, can you confirm?

Comment 3

[Aki Helin]

No, this one still happens in a fresh Aurora build for me.

Comment 4

[Markus Stange [:mstange]]

Attachment: access checks

Not sure whether we want this, since ASAN builds do this already. But this allows the checks to be run in regular debug builds, too.

Comment 5

[Markus Stange [:mstange]]

Attachment: fix bug

The reason for the out-of-bounds access is the bilinear sampling in ColorComponentAtPoint when called with fractional positions.

Comment 6

[Bas Schouten (:bas.schouten)]

Comment on attachment 8365996 [details] [diff] [review]
access checks

Review of attachment 8365996 [details] [diff] [review]:
-----------------------------------------------------------------

How bad is the perf impact of this? It seems to me like it could make certain filter constructions almost 'unusable' in a debug build? Then again, certain filters would probably already be unfeasible in non-opt builds.

Comment 7

[Markus Stange [:mstange]]

(In reply to Bas Schouten (:bas.schouten) from comment #6)
> How bad is the perf impact of this? It seems to me like it could make
> certain filter constructions almost 'unusable' in a debug build? Then again,
> certain filters would probably already be unfeasible in non-opt builds.

It doesn't seem too bad. Convolve matrix + lighting filters are already much slower in debug builds, I'd say we're between almost usable and fairly unusable for them (in debug builds). With the access control checks we're in the same range, they don't cause us to fall into the completely unusable range.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8365996 [details] [diff] [review]
access checks

[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?
They make it very clear that we're reading from a location that we shouldn't be reading from, but they don't make it clear how to trigger or exploit it.

Which older supported branches are affected by this flaw?
It started with Firefox 28, which has just moved to Beta.

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?
The patches apply cleanly on all affected branches. Risk is very low.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely, does not need much testing.

Comment 9

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

We need a security rating on this in order to approve it. How exploitable is this?

Comment 10

[Markus Stange [:mstange]]

Hard for me to say. The bug causes us to read from memory ranges slightly outside the parts that we allocated for a temporary image surface (i.e. max 1 pixel outside, in all directions, I think). The read values are then subject to a convolution operation which mixes them with values from inside the surface, and with the right setup the resulting values can be read by webpage JavaScript.

Comment 11

[Daniel Veditz [:dveditz]]

arbitrary-sized (or large) reads usually rate sec-high, but a byte inflation says this was a tiny information leak.

Comment 12

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

Since this is a sec-moderate, it can just go in. It doesn't need sec-approval.

Comment 13

[Aki Helin]

Too bad an exploit using this would be limited to just the immediate effect of getting one byte past bounds. Otherwise one could e.g. fill the js heap with something of interest and start reading one byte at a time from many different positions ;)

Comment 14

[Markus Stange [:mstange]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e44f7c6661a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fce0bb33afb

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/9e44f7c6661a
https://hg.mozilla.org/mozilla-central/rev/0fce0bb33afb

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Markus, can we get branch uplift requests please? :)

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8365996 [details] [diff] [review]
access checks

Sure!

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8365999 [details] [diff] [review]
fix bug

I'm requesting approval for both attachment 8365996 [details] [diff] [review] and attachment 8365999 [details] [diff] [review].

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 924102 / 924103
User impact if declined: small potential data leak
Testing completed (on m-c, etc.): 4 days on mozilla-central
Risk to taking this patch (and alternatives if risky): very low
String or IDL/UUID changes made by this patch: none

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f9e624cc1080
https://hg.mozilla.org/releases/mozilla-aurora/rev/ce65f2f4ee88

https://hg.mozilla.org/releases/mozilla-beta/rev/dbc276ddafdd
https://hg.mozilla.org/releases/mozilla-beta/rev/c9caee36b9d3

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/dbc276ddafdd
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/c9caee36b9d3

Comment 21

[u279076]

Matt, can you please ensure this gets verified with tomorrow's Beta?

Comment 22

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

Verified crash in ASan build of FF28, 2014-01-15.
Confirmed fixed in ASan builds of FF28, 29, 30, 2014-02-19.

Comment 23

[Daniel Veditz [:dveditz]]

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.