711653 - (CVE-2012-0456) SVGFilter out of bounds read (Address Sanitizer)

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Atte Kettunen]

Attachment: reduced_heap_firefox.svg

User Agent: Mozilla/5.0 (Ubuntu; X11; Linux x86_64; rv:10.0a2) Gecko/20111214 Firefox/10.0a2
Build ID: 20111214081512

Steps to reproduce:

OS: Ubuntu 11.04 x64 
Version: Nightly - 11.0a1 (2011-12-17)

Open one of the attached svg-files trough html with the following content 

example:
<img src="reduce_heap_firefox.svg" width=50 height=50>




Actual results:

With the reduced case Address Sanitizer reports:

==7843== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f52ae049193 at pc 0x7f52d0e832e3 bp 0x7fff11229800 sp 0x7fff112297f8
READ of size 1 at 0x7f52ae049193 thread T0
    #0 0x7f52d0e832e3 (/home/attekett/src/objdir-ff-asan/toolkit/library/libxul.so+0x1d392e3)
0x7f52ae049193 is located 3 bytes to the right of 16-byte region [0x7f52ae049180,0x7f52ae049190)
allocated by thread T0 here:
    #0 0x40b0de (/home/attekett/src/objdir-ff-asan/dist/bin/firefox-bin+0x40b0de)
    #1 0x7f52d1a970ac (/home/attekett/src/objdir-ff-asan/toolkit/library/libxul.so+0x294d0ac)
    #2 0x7f52d1a96d01 (/home/attekett/src/objdir-ff-asan/toolkit/library/libxul.so+0x294cd01)
==7843== ABORTING
Stats: 48M malloced (74M for red zones) by 230316 calls
Stats: 3M realloced by 15011 calls
Stats: 26M freed by 123878 calls
Stats: 0M really freed by 0 calls
Stats: 152M (38929 full pages) mmaped in 38 calls
  mmaps   by size class: 8:212979; 9:24573; 10:8190; 11:6141; 12:2048; 13:1024; 14:512; 15:256; 16:192; 17:32; 18:48; 19:8; 20:4; 
  mallocs by size class: 8:193922; 9:20891; 10:7322; 11:5292; 12:1426; 13:631; 14:497; 15:133; 16:131; 17:20; 18:45; 19:5; 20:1; 
  frees   by size class: 8:101631; 9:12645; 10:4978; 11:2738; 12:874; 13:472; 14:346; 15:102; 16:66; 17:15; 18:8; 19:3; 
  rfrees  by size class: 
Stats: malloc large: 71 small slow: 893
Shadow byte and word:
  0x1fea55c09232: fb
  0x1fea55c09230: 00 00 fb fb fb fb fb fb
More shadow bytes:
  0x1fea55c09210: 00 00 00 00 00 00 00 00
  0x1fea55c09218: 00 fb fb fb fb fb fb fb
  0x1fea55c09220: fa fa fa fa fa fa fa fa
  0x1fea55c09228: fa fa fa fa fa fa fa fa
=>0x1fea55c09230: 00 00 fb fb fb fb fb fb
  0x1fea55c09238: fb fb fb fb fb fb fb fb
  0x1fea55c09240: fa fa fa fa fa fa fa fa
  0x1fea55c09248: fa fa fa fa fa fa fa fa
  0x1fea55c09250: 00 fb fb fb fb fb fb fb

Comment 1

[Atte Kettunen]

Attachment: non-reduced svg-file

Comment 2

[Atte Kettunen]

Original non-reduced file gives little different output from ASAN.

Address sanitizer report:

==8082== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7fbeeaf73a8f at pc 0x7fbf16ee12e3 bp 0x7fff7977a5a0 sp 0x7fff7977a598
READ of size 1 at 0x7fbeeaf73a8f thread T0
    #0 0x7fbf16ee12e3 (/home/attekett/src/objdir-ff-asan/toolkit/library/libxul.so+0x1d392e3)
0x7fbeeaf73a8f is located 3 bytes to the right of 12-byte region [0x7fbeeaf73a80,0x7fbeeaf73a8c)
allocated by thread T0 here:
    #0 0x40b0de (/home/attekett/src/objdir-ff-asan/dist/bin/firefox-bin+0x40b0de)
    #1 0x7fbf17af50ac (/home/attekett/src/objdir-ff-asan/toolkit/library/libxul.so+0x294d0ac)
    #2 0x7fbf17af4d01 (/home/attekett/src/objdir-ff-asan/toolkit/library/libxul.so+0x294cd01)
==8082== ABORTING
Stats: 123M malloced (136M for red zones) by 341231 calls
Stats: 8M realloced by 21667 calls
Stats: 91M freed by 212066 calls
Stats: 0M really freed by 0 calls
Stats: 284M (72738 full pages) mmaped in 71 calls
  mmaps   by size class: 8:262128; 9:57337; 10:16380; 11:20470; 12:3072; 13:2048; 14:1792; 15:384; 16:384; 17:160; 18:64; 19:8; 20:4; 
  mallocs by size class: 8:249745; 9:51039; 10:15476; 11:18055; 12:2584; 13:1733; 14:1682; 15:340; 16:358; 17:147; 18:61; 19:8; 20:3; 
  frees   by size class: 8:139988; 9:39932; 10:12071; 11:15184; 12:1843; 13:908; 14:1454; 15:290; 16:234; 17:138; 18:17; 19:5; 20:2; 
  rfrees  by size class: 
Stats: malloc large: 219 small slow: 1806
Shadow byte and word:
  0x1ff7dd5ee751: 4
  0x1ff7dd5ee750: 00 04 fb fb fb fb fb fb
More shadow bytes:
  0x1ff7dd5ee730: 00 00 00 00 00 00 00 00
  0x1ff7dd5ee738: 00 fb fb fb fb fb fb fb
  0x1ff7dd5ee740: fa fa fa fa fa fa fa fa
  0x1ff7dd5ee748: fa fa fa fa fa fa fa fa
=>0x1ff7dd5ee750: 00 04 fb fb fb fb fb fb
  0x1ff7dd5ee758: fb fb fb fb fb fb fb fb
  0x1ff7dd5ee760: fa fa fa fa fa fa fa fa
  0x1ff7dd5ee768: fa fa fa fa fa fa fa fa
  0x1ff7dd5ee770: 00 fb fb fb fb fb fb fb

Comment 3

[Daniel Veditz [:dveditz]]

Christian: can you test this one, please.

Comment 4

[Christian Holler (:decoder)]

I confirmed this with an older asan opt build but cannot tell from that output if it's a false positive or a valid bug. I'm building a more recent debug build now to check if one can see more there.

Comment 5

[Christian Holler (:decoder)]

Attachment: Symbolized ASAN trace

I was able to get a symbolized ASAN trace from an ASAN debug build I just created (unfortunately make package seems to strip such that the symbols cannot be retrieved anymore with addr2line afterwards).

From the trace I'd guess it's a valid bug, at least worth investigating for the SVG people.

Comment 6

[Daniel Veditz [:dveditz]]

ASAN output is saying it's an out of bounds read rather than a heap buffer overflow.
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGFilters.cpp#4918

Comment 8

[Daniel Holbert [:dholbert]]

Sorry for the inactivity here -- I somehow overlooked the assigned-to-me bugmail.  Looking into it now.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Daniel Veditz from comment #6)
> ASAN output is saying it's an out of bounds read rather than a heap buffer
> overflow.
> http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/
> nsSVGFilters.cpp#4918

So using the testcase from bug 719779, I hit that line of code with backtraces like this:
{
#0  Convolve3x3 (index=0x7f1a99128a2b "", stride=4, kernel=0x7f1ab8ab615b) at nsSVGFilters.cpp:4918
#1  0x00007f1ab76c3d9e in GenerateNormal (N=0x7fff9993bbe0, data=0x7f1a99128a20 "", stride=4, surfaceWidth=1, surfaceHeight=4, x=0, y=2, surfaceScale=1) at nsSVGFilters.cpp:4979
#2  0x00007f1ab76c4632 in nsSVGFELightingElement::Filter (this=0x7f1a97f92e80, instance=0x7f1aa8113d40, aSources=..., aTarget=0x7f1a8f50b140, rect=...) at nsSVGFilters.cpp:5081
#3  0x00007f1ab76c7e93 in nsSVGFESpecularLightingElement::Filter (this=0x7f1a97f92e80, instance=0x7f1aa8113d40, aSources=..., aTarget=0x7f1a8f50b140, rect=...) at nsSVGFilters.cpp:5419
#4  0x00007f1ab762c8dd in nsSVGFilterInstance::Render
}

I think we're basically trying to index into a 3x3 grid of pixels, to do some spec-prescribed math for computing a lighting normal-vector at the center of the grid based on the color values in the surrounding pixels.

In this particular case, our source data has a width of 1px -- at level #2 in the backtrace above, we have:
> (gdb) p info.mSource.mRawPtr->mSize
>  <mozilla::gfx::BaseSize<int, nsIntSize>> = {
>    width = 1, 
>    height = 4
...so it might be that we're trying to access pixels to the left or right of our 1px-wide buffer, which ends up being an out-of-bounds read.  Not totally sure, though... this code is tricky.

Comment 10

[Robert Longson [:longsonr]]

In the GenerateNormal that calls Convolve3x3 we check for the size of the surface and index into the Kx array that tells us whether to use the pixels before/after this one or not. 

The spec doesn't say what to do if the top left corner is also the top right corner. We may need an extra matrix for this case and make up something sensible with appropriate zeros to avoid going off the edges.

Comment 11

[Robert Longson [:longsonr]]

Do you need any more help with this Daniel?

Comment 12

[Daniel Holbert [:dholbert]]

I'm splitting time between flexbox & everything else right now, and I'm intending to pick up this bug here in the next day or two.  (I tried getting an ASAN build going yesterday to reproduce the issue myself, but it crashes as soon as Firefox window opens, sadly. :-/ )

If you're interested in taking a crack at fixing this, you're more than welcome to! :) You know the filter code a lot better than I do, so if you have the time/interest to dig into this, you might have more/quicker success...

Otherwise, though, I plan on digging a bit more in the next day or so (per your suggestion at the end of comment 10) and see what I can do.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: prevent this in the future: add assertions [DO NOT LAND until fix makes it to releases]

Assuming I'm correctly understanding how these arguments relate to the underlying buffer (which I think I am), this patch adds some debug-only code to assert whenever we're about to read outside our source buffer.

When I view the attached testcase  ( reduced_heap_firefox.svg ) via <img> as indicated in comment 0, it fails the following assertion (from this patch) 4 times per window-invalidate:
{
###!!! ASSERTION: out of bounds read (after buffer): 'valPtr < maxData', file mozilla/content/svg/content/src/nsSVGFilters.cpp, line 4924
}

NOTE: Breakdown of the 4 assertions:
====================================
There are 2 problematic GenerateNormal calls (one for y=2 and one for y=3).  Each of those calls has 2 Convolve3x3 calls which trigger this assertion failure in their final loop.

In each assertion-failure, we have valPtr == maxData + 3.

Comment 14

[Daniel Holbert [:dholbert]]

(If it wasn't clear: the point of patch 1 is to catch this issue in debug builds without needing ASAN.)

Comment 15

[Robert Longson [:longsonr]]

Attachment: patch

Your patch doesn't fix the issue in production and highlights that we have an issue. This should fix it.

Comment 16

[Jonathan Watt [:jwatt]]

Robert, you seem to have attached the file rather than its diff.

Comment 17

[Robert Longson [:longsonr]]

Attachment: patch

oops

Comment 18

[Robert Longson [:longsonr]]

We can't check in Daniel's change as it hightlights our vulnerability without actually fixing it. Someone could combine his change with mine locally to check that there are no asserts on the testcase.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 596284 [details] [diff] [review]
patch

(In reply to Robert Longson from comment #15)
> Your patch doesn't fix the issue in production

Right, hence the "not a fix" annotation. :) (My patch doesn't "fix" the issue in debug builds, either -- it simply asserts and then proceeds to do the read, so we can tell that there was a problem.)

> This should fix it.

Excellent! One question:
>+  // degenerate cases
>+  if (surfaceWidth == 1 || surfaceHeight == 1) {
>+    N[0] = 0;
>+    N[1] = 0;
>+    N[2] = 255;
>+    return;
>+  }

So just to be sure I understand -- this represents a normal vector pointing directly out of the page, correct?  It'd probably be worth adding a comment to state that, just for clarity's sake.

Anyway, assuming "yes", then this seems sensible to me. (The 0 values also make sense if you collapse the matrices from the spec together.)  Looks good.

I haven't tested it with my patch yet, but I'll do that sometime tomorrow.

Comment 20

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #18)
> We can't check in Daniel's change as it hightlights our vulnerability
> without actually fixing it.

Agreed -- we definitely shouldn't check in my assertion patch until this is this fixed.  We might even want to hold off on the assertions until the fix has made it to release builds.

(I'll give our test suites a local runthrough with the patch at some point to be sure we don't fail the assertions in other existing tests for whatever reason)

Comment 21

[Robert Longson [:longsonr]]

Attachment: patch [addressing review comment]

Comment 22

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> 
> Excellent! One question:
> >+  // degenerate cases
> >+  if (surfaceWidth == 1 || surfaceHeight == 1) {
> >+    N[0] = 0;
> >+    N[1] = 0;
> >+    N[2] = 255;
> >+    return;
> >+  }
> 
> So just to be sure I understand -- this represents a normal vector pointing
> directly out of the page, correct?  It'd probably be worth adding a comment
> to state that, just for clarity's sake.

Yes, the positive z axis is in the direction of the viewer of the content.

Comment 23

[Robert Longson [:longsonr]]

Feel free to land my patch once you're happy with it. We should probably push this to aurora/beta too.

Comment 24

[Robert Longson [:longsonr]]

3.6 will be affected so do we take it there as well?

Comment 25

[Daniel Holbert [:dholbert]]

I tested my patch & robert's patch in conjunction, and I confirmed that his patch fixes the assertions that my patch adds.

decoder, could you confirm that robert's patch makes ASAN happy? (I would, but my attempt at an ASAN build kept startup crashing. :-/  )

(In reply to Robert Longson from comment #24)
> 3.6 will be affected so do we take it there as well?

I'm not sure, now that ESR has been released. (We will want this on ESR, though, of course.)

I do notice that the status1.9.2 field doesn't have an "affected" option -- only "unaffected" and "wontfix".  Perhaps dveditz knows whether we're doing more 3.6 releases...?

Comment 26

[Daniel Holbert [:dholbert]]

(er, meant to CC dveditz for the second half of prev. comment)

Comment 27

[Daniel Holbert [:dholbert]]

dveditz confirms in an IRL conversation that we're shipping one more 3.6 release, so we should include this fix in that.

I'm gonna push longsonr's fix to Try as a test-bustage sanity-check, and then land on m-i and request branch approvals immediately after.

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #20)
> (I'll give our test suites a local runthrough with the patch at some point
> to be sure we don't fail the assertions in other existing tests for whatever
> reason)

(BTW: I confirmed that my assertions-patch doesn't fire at all on our existing testsuites. So I think we'll definitely will want to add an automated test with my assertions-patch at some point after the fix is released, to keep this from regressing.)

Comment 29

[Daniel Holbert [:dholbert]]

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

Comment 30

[Daniel Holbert [:dholbert]]

Comment on attachment 596290 [details] [diff] [review]
patch [addressing review comment]

[Approval Request Comment]
Regression caused by (bug #):
  Not a (recent) regression -- goes at least as far back as 3.6

User impact if declined:
 Uninitialized memory read --> possible data leakage

Testing completed (on m-c, etc.):
 Try run: mostly done, passed R + C + M-2 on one platform so far.
 Just landed on m-i; assuming things will be successful there.

Risk to taking this patch (and alternatives if risky):
 Low risk of breaking certain lighting filters on 1px-wide surfaces, if any content happens to depend on the existing behavior. (However, our existing behavior for this was probably random/unreliable anyway, due to the uninitialized-memory-read.)  This isn't a use case that's worth worrying about, IMHO.

Comment 31

[Daniel Holbert [:dholbert]]

> Testing completed (on m-c, etc.):
Also: I did local testing w/ valgrind to verify that it actually fixes the bug. (It does.)

Comment 32

[Daniel Holbert [:dholbert]]

(reassigning to longsonr, since he wrote the fix, though I can take care of landings per comment 23)

Comment 33

[Christian Holler (:decoder)]

Confirmed that with the patch, ASan no longer detects any error here.

Comment 36

[Robert Longson [:longsonr]]

pushed to central: https://hg.mozilla.org/mozilla-central/rev/f28f16d674c9

Comment 37

[Alex Keybl [:akeybl]]

Dan - do we need this on FF11/12 given this has been a problem since 3.6? Are there any variables that have changed since 3.6 to warrant taking this?

Comment 38

[Daniel Veditz [:dveditz]]

Giving this one a security impact of "moderate". Possible memory sniffing, but seems pretty hard to get much of value with discontiguous 3-byte chunks.

(In reply to Alex Keybl [:akeybl] from comment #37)
> Dan - do we need this on FF11/12 given this has been a problem since 3.6?
> Are there any variables that have changed since 3.6 to warrant taking this?

The length of time a bug has lurked without being discovered isn't the right question, we didn't know about this bug until just now. Impact is moderate but discoverability is high since ASan is a new hot tool people are playing with. We should fix this in Aurora (Fx12) for sure. The patch is targeted, small, and safe so it won't hurt to take it in Beta (Fx11) but it probably won't kill us if it doesn't make it.

Comment 39

[Daniel Holbert [:dholbert]]

Landed on aurora/beta/1.9.2:
 https://hg.mozilla.org/releases/mozilla-aurora/rev/38b308bf6864
 https://hg.mozilla.org/releases/mozilla-beta/rev/cdd5673bf5b4
 https://hg.mozilla.org/releases/mozilla-1.9.2/rev/57f74ecff483

I'd argue that we should land this on ESR, too -- do I need to do anything to request approval for that?  (This already has tracking/status flags for ESR set -- if ESR approval is granted, will it just be noted in a comment here at some point?)

Comment 40

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage comment]

This bug is being tracked for landing on ESR branch.  Please land patches on http://hg.mozilla.org/releases/mozilla-esr10/by Thursday March 1, 2012 in order to be ready for go-to-build on Friday March 2, 2012.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.

Comment 41

[Daniel Holbert [:dholbert]]

Landed on ESR:
  https://hg.mozilla.org/releases/mozilla-esr10/rev/e0c6ef11e7ea

> See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.

That page doesn't seem to include any "after landing, [do this]" steps -- should I set status-firefox-esr10 to "fixed" now that it's landed there?  (Not doing so yet, to be on the safe side)

Comment 42

[Alex Keybl [:akeybl]]

(In reply to Daniel Holbert [:dholbert] from comment #41)
> Landed on ESR:
>   https://hg.mozilla.org/releases/mozilla-esr10/rev/e0c6ef11e7ea
> 
> > See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more information.
> 
> That page doesn't seem to include any "after landing, [do this]" steps --
> should I set status-firefox-esr10 to "fixed" now that it's landed there? 
> (Not doing so yet, to be on the safe side)

This was right - looks like Mbrubeck fixed the landing instructions on the wiki.

Comment 43

[u279076]

Verified fixed with Firefox 10.0.3esr.

Comment 44

[u279076]

Verified fixed with Firefox 3.6.28build1.

Comment 45

[u279076]

Verified fixed in Firefox 11.0RC build 2 and Firefox 12.0a2 2012-03-12

Comment 47

[Jesse Ruderman]

dholbert, did you ever land the assertions++ patch in comment 13?

Comment 48

[Daniel Holbert [:dholbert]]

I did not -- thanks for the reminder! We need better ways to 

It's bitrotted from the integer-type-naming switchover, but other than that it still applies.  I'll give it a Try run and land it, with the reduced testcase from bug 719779 (attachment 596857 [details]), assuming that it passes.

Comment 49

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #48)
> I did not -- thanks for the reminder! We need better ways to 

*We need better ways to keep track of that. :-/ (or maybe I just need to add calendar reminders for stuff like this)

Comment 50

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #48)
> with the reduced
> testcase from bug 719779 (attachment 596857 [details]), assuming that it passes.

OK -- so I did some local testing w/ the fix reverted, to be sure that my assertion patch is useful.

Turns out that even with longsonr's fix reverted, this testcase no longer triggers the code in question, so it won't fail the assertions in my assertion-patch.  In particular: we never get any calls to nsSVGFilterInstance::Render() (which is what we're expecting to call nsSVGFELightingElement::Filter() --> GenerateNormal() --> Convolve3x3() --> out-of-bounds-read)

I think this is because of an optimization (maybe from display lists), since the filtered element in the testcase has  transform="matrix(100,-2562303,3000,100,300,300)", which places it well outside the rendered viewport. :)  If I remove that transform, then we *do* get those calls (and nothing goes wrong, because the huge transform is part of what's required to trigger the bug).

Anyway -- I did verify that that testcase *does* fail my assertions in a targeted old build, from the parent of Robert's cset in comment 36 (w/ the assertions-patch layered on top).

Ideally it'd be cool to have a testcase that still reproduces this in an up-to-date build with the fix reverted, but absent such a testcase, I'll just proceed with the plan at the end of comment 48.

Comment 51

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=786b9d40936c

Comment 52

[Daniel Holbert [:dholbert]]

Landed assertion patch:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed3d140f9fc
and the test from duplicate bug 719779 (that changeset noted over on that bug).

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ed3d140f9fc