Closed Bug 734288 (CVE-2012-0470) Opened 12 years ago Closed 12 years ago

ASAN: Heap-buffer-overflow WRITE of size 1 at nsSVGFEDiffuseLightingElement::LightPixel

Categories

(Core :: SVG, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla13
Tracking Status
firefox10 --- affected
firefox11 + affected
firefox12 + verified
firefox13 --- verified
firefox14 --- verified
firefox-esr10 12+ verified

People

(Reporter: attekett, Assigned: dholbert)

References

Details

(Keywords: regression, Whiteboard: [asan][sg:crititcal][release drivers: see comment 24][qa+])

Attachments

(3 files, 1 obsolete file)

Attached image Repro-file —
==5860== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f23312cdda8 at pc 0x7f235665c341 bp 0x7fff1d8b5550 sp 0x7fff1d8b5548
WRITE of size 1 at 0x7f23312cdda8 thread T0
    #0 0x7f235665c341 in nsSVGFEDiffuseLightingElement::LightPixel(float const*, float const*, unsigned int, unsigned char*) /home/attekett/src/content/svg/content/src/nsSVGFilters.cpp:5298
    #1 0x7f235665ace9 in nsSVGFELightingElement::Filter(nsSVGFilterInstance*, nsTArray<nsSVGFE::Image const*, nsTArrayDefaultAllocator> const&, nsSVGFE::Image const*, nsIntRect const&) /home/attekett/src/content/svg/content/src/nsSVGFilters.cpp:5094
    #2 0x7f23565acf04 in nsSVGFilterInstance::Render(gfxASurface**) /home/attekett/src/layout/svg/base/src/nsSVGFilterInstance.cpp:529
    #3 0x7f23565a7a93 in nsSVGFilterFrame::FilterPaint(nsRenderingContext*, nsIFrame*, nsSVGFilterPaintCallback*, nsIntRect const*) /home/attekett/src/layout/svg/base/src/nsSVGFilterFrame.cpp:264
    #4 0x7f23565df61f in nsSVGUtils::PaintFrameWithEffects(nsRenderingContext*, nsIntRect const*, nsIFrame*) /home/attekett/src/layout/svg/base/src/nsSVGUtils.cpp:1141
    #5 0x7f23565a2f8e in nsSVGDisplayContainerFrame::PaintSVG(nsRenderingContext*, nsIntRect const*) /home/attekett/src/layout/svg/base/src/nsSVGContainerFrame.cpp:175
    #6 0x7f23565ceb59 in nsSVGOuterSVGFrame::Paint(nsDisplayListBuilder const*, nsRenderingContext*, nsRect const&, nsPoint) /home/attekett/src/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:672
    #7 0x7f23565ce8bb in nsDisplaySVG::Paint(nsDisplayListBuilder*, nsRenderingContext*) /home/attekett/src/layout/svg/base/src/nsSVGOuterSVGFrame.cpp:566
    #8 0x7f23556aa9de in mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*) /home/attekett/src/layout/base/FrameLayerBuilder.cpp:2250
    #9 0x7f235747013b in mozilla::layers::BasicThebesLayer::PaintBuffer(gfxContext*, nsIntRegion const&, nsIntRegion const&, nsIntRegion const&, bool, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*) /home/attekett/src/gfx/layers/basic/BasicLayers.cpp:570
    #10 0x7f23574696db in mozilla::layers::BasicShadowableThebesLayer::PaintBuffer(gfxContext*, nsIntRegion const&, nsIntRegion const&, nsIntRegion const&, bool, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*) /home/attekett/src/gfx/layers/basic/BasicLayers.cpp:2395
    #11 0x7f23574627b5 in mozilla::layers::BasicThebesLayer::PaintThebes(gfxContext*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) /home/attekett/src/gfx/layers/basic/BasicLayers.cpp:770
    #12 0x7f23574671fa in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) /home/attekett/src/gfx/layers/basic/BasicLayers.cpp:1951
    #13 0x7f2357467356 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) /home/attekett/src/gfx/layers/basic/BasicLayers.cpp:1967
    #14 0x7f2357467356 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) /home/attekett/src/gfx/layers/basic/BasicLayers.cpp:1967
    #15 0x7f2357465af1 in mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) /home/attekett/src/gfx/layers/basic/BasicLayers.cpp:1658
    #16 0x7f235746db49 in mozilla::layers::BasicShadowLayerManager::EndTransaction(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) /home/attekett/src/gfx/layers/basic/BasicLayers.cpp:3398
    #17 0x7f23557607f7 in nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) /home/attekett/src/layout/base/nsLayoutUtils.cpp:1852
    #18 0x7f235579ccfd in PresShell::Paint(nsIView*, nsIWidget*, nsRegion const&, nsIntRegion const&, bool) /home/attekett/src/layout/base/nsPresShell.cpp:5409
    #19 0x7f23560513e4 in nsViewManager::Refresh(nsView*, nsIWidget*, nsIntRegion const&, bool) /home/attekett/src/view/src/nsViewManager.cpp:376
    #20 0x7f23560531e4 in nsViewManager::DispatchEvent(nsGUIEvent*, nsIView*, nsEventStatus*) /home/attekett/src/view/src/nsViewManager.cpp:813
    #21 0x7f235604dd61 in HandleEvent /home/attekett/src/view/src/nsView.cpp:159
    #22 0x7f2356d813fb in nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) /home/attekett/src/widget/gtk2/nsWindow.cpp:524
    #23 0x7f2356d95a92 in expose_event_cb /home/attekett/src/widget/gtk2/nsWindow.cpp:5393
    #24 0x7f2351dae578 in _gtk_marshal_BOOLEAN__BOXED /build/buildd/gtk+2.0-2.24.4/gtk/gtkmarshalers.c:90
0x7f23312cdda8 is located 0 bytes to the right of 40-byte region [0x7f23312cdd80,0x7f23312cdda8)
allocated by thread T0 here:
    #0 0x40cf8c in posix_memalign ??:0
    #1 0x7f23573f0dec in TryAllocAlignedBytes /home/attekett/src/gfx/thebes/gfxImageSurface.cpp:117
    #2 0x7f23573f0a8a in gfxImageSurface::gfxImageSurface(nsIntSize const&, gfxASurface::gfxImageFormat) /home/attekett/src/gfx/thebes/gfxImageSurface.cpp:139
==5860== ABORTING
Stats: 56M malloced (83M for red zones) by 243696 calls
Stats: 3M realloced by 15734 calls
Stats: 33M freed by 132010 calls
Stats: 0M really freed by 0 calls
Stats: 176M (45077 full pages) mmaped in 44 calls
  mmaps   by size class: 8:229362; 9:24573; 10:8190; 11:8188; 12:2048; 13:1024; 14:768; 15:256; 16:256; 17:64; 18:64; 19:8; 20:4;
  mallocs by size class: 8:207343; 9:20575; 10:6910; 11:5610; 12:1534; 13:763; 14:511; 15:145; 16:203; 17:34; 18:60; 19:5; 20:3;
  frees   by size class: 8:110634; 9:11973; 10:4318; 11:2910; 12:920; 13:593; 14:367; 15:112; 16:138; 17:29; 18:11; 19:3; 20:2;
  rfrees  by size class:
Stats: malloc large: 102 small slow: 971
Shadow byte and word:
  0x1fe466259bb5: fb
  0x1fe466259bb0: 00 00 00 00 00 fb fb fb
More shadow bytes:
  0x1fe466259b90: 00 00 00 00 00 00 00 00
  0x1fe466259b98: 00 fb fb fb fb fb fb fb
  0x1fe466259ba0: fa fa fa fa fa fa fa fa
  0x1fe466259ba8: fa fa fa fa fa fa fa fa
=>0x1fe466259bb0: 00 00 00 00 00 fb fb fb
  0x1fe466259bb8: fb fb fb fb fb fb fb fb
  0x1fe466259bc0: fa fa fa fa fa fa fa fa
  0x1fe466259bc8: fa fa fa fa fa fa fa fa
  0x1fe466259bd0: 00 fb fb fb fb fb fb fb
Confirmed :)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [asan]
Attached patch something to try (obsolete) — — Splinter Review
Can anyone try adding this on linux and see if it asserts on the testcase. This is not a fix or something to land, especially as it's wrong when not applied to this example.
(In reply to Robert Longson from comment #2)
> Created attachment 604439 [details] [diff] [review]
> something to try
> 
> Can anyone try adding this on linux and see if it asserts on the testcase.
> This is not a fix or something to land, especially as it's wrong when not
> applied to this example.

Trying this now, but it will take a bit :)
This bug's testcase crashes my linux 64-bit debug build, with an invalid free:
> *** glibc detected *** ./dist/bin/firefox: free(): invalid next size (fast): 0x0000000002e0fb90 ***
--> Setting as [sg:critical]

Robert: Both of your assertions are triggered by this testcase (they're the last thing printed before the invalid free message above):
###!!! ASSERTION: bad I think!: 'aDataRect.width <= scaledSize.width && aDataRect.height <= scaledSize.height', file ../../../../../mozilla/content/svg/content/src/nsSVGFilters.cpp, line 174
###!!! ASSERTION: just checking!: 'aDataRect.width == scaledSize.width && aDataRect.height == scaledSize.height', file ../../../../../mozilla/content/svg/content/src/nsSVGFilters.cpp, line 176
Whiteboard: [asan] → [asan][sg:crititcal?]
Whiteboard: [asan][sg:crititcal?] → [asan][sg:crititcal]
Attached file backtrace of invalid free —
Here's the backtrace of the invalid free that I hit, for reference.  The value is deleted in gfxImageSurface::~gfxImageSurface.
What are all the member values of aDataRect and scaledSize?
(gdb) p aDataRect.width
$10 = 60
(gdb) p aDataRect.height
$11 = 60
(gdb) p scaledSize.width
$12 = 1
(gdb) p scaledSize.height
$13 = 10

Also, the invalid free seems to be somewhat sporadic -- I can't reproduce that crash 100% of the time. (It happened the first few times I tried this morning, but now I'm not hitting it.  Glad I saved the backtrace when I did. :))
(OK, just hit the invalid-free crash again after a few reloads.)

(For my future reference, in case the crash's reproducibility depends on something machine-specific / glibc-version-dependent: the crash is on my Lenovo laptop, which is running Ubuntu 12.04 beta.  I've also got "--disable-jemalloc" in that build -- that likely helps glibc detect the invalid free.)
Are those aDataRect values before or after 

   r.Scale(1 / kernelX, 1 / kernelY);
   r.RoundOut();

What is kernelX & kernelY?

I can't get a crash on Windows or any issues.
I was sampling the values right at the assertions you added. (however, aDataRect is const & doesn't change, so it doesn't particularly matter. Its value is the same at 

> What is kernelX & kernelY?

(gdb) p kernelX
$3 = 3600
(gdb) p kernelY
$4 = 6-

> I can't get a crash on Windows or any issues.

Might be worth trying with --disable-jemalloc.  That turned a second non-crashing build into a crashing build on my work desktop. (w/ Ubuntu 11.10 64-bit)

If you have a few minutes and want to do further investigation over IRC, feel free to sign on & ping me -- I've got it trapped in a debugger at the point of assertion-failure right now.
(In reply to Daniel Holbert [:dholbert] from comment #10)
> I was sampling the values right at the assertions you added. (however,
> aDataRect is const & doesn't change, so it doesn't particularly matter. Its
> value is the same at 

(er "the same at the beginning of nsSVGFE::SetupScalingFilter as when the assertion fails")
(In reply to Daniel Holbert [:dholbert] from comment #10)
> (gdb) p kernelX
> $3 = 3600
> (gdb) p kernelY
> $4 = 6-
(sorry, not sure how that "-" got in there.  kernelY is just "6")
What are the values of r after roundOut?
after roundOut, r.width = 1, r.height = 11 (so -- 1 unit taller than scaledSize)

r starts out w/ size 60,60.  (matching aRect)

The scale() call puts it at:
    width = 0.016666667070239782, 
    height = 10.000000298023224

and then roundOut bumps it up to:
    width = 1, 
    height = 11
Attached patch fix v1 [r=longsonr] — — Splinter Review
Just talked this through with longsonr over IRC.

The badness is that |r| is larger than scaledSize, as noted in prev comment.

This happens due to float error, resulting from being in different number systems at different times.

scaledSize is calculated as follows:
>  gfxIntSize scaledSize =
>    nsSVGUtils::ConvertToSurfaceSize(gfxSize(aTarget->mImage->Width() / kernelX,
>                                             aTarget->mImage->Height() / kernelY),
(where ConvertToSurfaceSize does some rounding)

There, Height() = (PRInt32)60, and kernelY is (float)6, so we do that division in float coordinates, producing (float)10, and then we convert that to a double as an argument to gfxSize.

In contrast, r is calculated as follows:
>  gfxRect r(aDataRect.x, aDataRect.y, aDataRect.width, aDataRect.height);
>  r.Scale(1 / kernelX, 1 / kernelY);
>  r.RoundOut();

After the first line there, |r| is 0,0,60,60 all as double values. We pass in (PRInt32)1 / (float)60 as the second argument to Scale -- so that gives us (float)(1/60) -- and then we convert that to a double and multiply that by (double)60, which we'd _hope_ would produce 10, but apparently doesn't quite.

So in the |r| case, we've got a mix of float-division followed by double-multiplication that causes a slightly different result from pure-float-division that we do for |scaledSize|.

If we simply make kernelX and kernelY into doubles (aka gfxFloat), then _all_ of the math above will be in double-space, and everything works out OK.  (Both heights end up as 10, and we don't crash.)

longsonr granted r+ on this over IRC (and then had to head to sleep, as it's late in his timezone).  I intend to land this ASAP so it can make the uplift next week; I'll request branch approval after landing.
Assignee: nobody → dholbert
Attachment #604439 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #604527 - Flags: review+
(In reply to Daniel Holbert [:dholbert] from comment #15)
> In contrast, r is calculated as follows:
> >  gfxRect r(aDataRect.x, aDataRect.y, aDataRect.width, aDataRect.height);
> >  r.Scale(1 / kernelX, 1 / kernelY);
> >  r.RoundOut();
> 
> After the first line there, |r| is 0,0,60,60 all as double values. We pass
> in (PRInt32)1 / (float)60 as the second argument to Scale -- so that gives
> us (float)(1/60) -- and then we convert that to a double and multiply that
> by (double)60, which we'd _hope_ would produce 10, but apparently doesn't
> quite.

er that should have been "(PRInt32)1 / (float)6" and "(float)(1/6)" (not /60)
(also, FWIW, we realized the asserts in "something to try" were bogus -- they should've had s/aDataRect/r/. For the record -- after I swapped "r" into them, they do fail pre-patch & pass post-patch.  We don't actually want to land those assertions, though, because they assume x==y==0 & hence aren't valid in general -- only for this testcase.)
Ultimately, once this has reached our users, we should add the testcase as a crashtest.  (It won't crash jemalloc-enabled builds, per comment 8, but who knows if we'll use jemalloc forever, and it's still worth having this in the testsuite for robustness.)
Flags: in-testsuite?
Landed: https://hg.mozilla.org/mozilla-central/rev/a521a6586e53
Target Milestone: --- → mozilla13
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
So, now to determine which branches are affected -- the various pieces of involved code here were last-changed at different times, but I suspect the change that triggered this was this tweak from Bug 696579:
http://hg.mozilla.org/mozilla-central/rev/ebd6501de883
> -  r.Scale(gfxFloat(scaledSize.width)/aTarget->mImage->Width(),
> -          gfxFloat(scaledSize.height)/aTarget->mImage->Height());
> +  r.Scale(1 / kernelX, 1 / kernelY);

...which changed those division operations to be done in float-space (and then passed to Scale & converted to doubles) instead of being done in double-space.

I'll do a targeted build to verify that hypothesis.
(In reply to Daniel Holbert [:dholbert] from comment #21)
> I suspect the
> change that triggered this was this tweak from Bug 696579:
> http://hg.mozilla.org/mozilla-central/rev/ebd6501de883

Yup -- confirmed with a targeted build that the testcase here did regress with ebd6501de883, but not because of the chunk I quoted in comment 21 -- it was actually because of the _other_ chunk of that cset, which modified ConvertToSurfaceSize.  That other chunk resulted in |scaledSize| getting a width of 1, whereas it previously got a width of 0. And when it had a width of 0, we'd return early, in this clause:
>  if (overflow || scaledSize.width <= 0 || scaledSize.height <= 0)
>    return result;
and completely skip the r.Scale() call and everything after it.
Blocks: 696579
So, Firefox 10/11/12 are all affected by this, as is ESR 10.  Previous builds should be unaffected.  Setting status flags accordingly.
Comment on attachment 604527 [details] [diff] [review]
fix v1 [r=longsonr]

[Requesting explicit r=longsonr, just so the branch drivers don't have to take my word for it on the "r=longsonr granted over IRC". :) ]

Requesting branch approval. Per previous comment, this affects Firefox 10, 11 (current beta), and 12 (current Aurora).  So -- we definitely need to backport this for 12 and ESR, and if possible, it'd be nice to backport this to Firefox 11 (which is about to be released). I assume Firefox 11 is essentially closed to further patches, since we're days from its release, but if we end up taking another few patches or needing to firedrill, this patch is about as safe as it gets & would be a good candidate for inclusion IMHO.

[Approval Request Comment]
Regression caused by (bug #):
  Bug 696579
User impact if declined:
  Heap corruption, possibly allowing invalid frees & remote code execution
  (see comment 4)
Testing completed (on m-c, etc.):
  Local testing, just landed on m-c.
Risk to taking this patch (and alternatives if risky):
  Very low-risk. Just replaces 2 float values with gfxFloat a.k.a.
  double, for better precision in arithmetic.
String changes made by this patch:
  None
Attachment #604527 - Attachment description: fix v1 [r=longsonr] → fix v1 [r=longsonr granted over IRC]
Attachment #604527 - Flags: review?(longsonr)
Attachment #604527 - Flags: approval-mozilla-beta?
Attachment #604527 - Flags: approval-mozilla-aurora?
Attachment #604527 - Flags: approval-mozilla-esr10?
(decoder: assuming that you've got a 64-bit linux ASAN setup -- if you could try to reproduce the ASAN warning here and then verify that the patch fixes it, that would be much appreciated!)
(In reply to Daniel Holbert [:dholbert] from comment #25)
> (decoder: assuming that you've got a 64-bit linux ASAN setup -- if you could
> try to reproduce the ASAN warning here and then verify that the patch fixes
> it, that would be much appreciated!)

Building with the patch now :)
Thanks! (ah, I see in comment 1 that you already reproduced the issue w/o the patch)
(In reply to Daniel Holbert [:dholbert] from comment #27)
> Thanks! (ah, I see in comment 1 that you already reproduced the issue w/o
> the patch)

Fix confirmed, yw :)
Great! Marking verified, then.
Status: RESOLVED → VERIFIED
Attachment #604527 - Flags: review?(longsonr) → review+
Attachment #604527 - Attachment description: fix v1 [r=longsonr granted over IRC] → fix v1 [r=longsonr]
(In reply to Daniel Holbert [:dholbert] from comment #17)
> (also, FWIW, we realized the asserts in "something to try" were bogus --
> they should've had s/aDataRect/r/. For the record -- after I swapped "r"
> into them, they do fail pre-patch & pass post-patch.  We don't actually want
> to land those assertions, though, because they assume x==y==0 & hence aren't
> valid in general -- only for this testcase.)

Daniel is repeating what I told him here. In fact I'm not sure effect x != 0 and y != 0 would have, that's just a more complicated case.
Attachment #604527 - Flags: review+
Whiteboard: [asan][sg:crititcal] → [asan][sg:crititcal][release drivers: see comment 24]
Comment on attachment 604527 [details] [diff] [review]
fix v1 [r=longsonr]

[Triage Comment]
Approved for Beta 12, but denying for Aurora since it now has the fix. Also tracking for ESR10, but we don't want to land there until we're sure whether or not we need to chemspill for FF11 and want to take this as a ridealong.
Attachment #604527 - Flags: approval-mozilla-esr10?
Attachment #604527 - Flags: approval-mozilla-beta?
Attachment #604527 - Flags: approval-mozilla-beta+
Attachment #604527 - Flags: approval-mozilla-aurora?
Attachment #604527 - Flags: approval-mozilla-aurora-
Comment on attachment 604527 [details] [diff] [review]
fix v1 [r=longsonr]

[Triage Comment]
Please go ahead and land to ESR branch, see https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details.
Attachment #604527 - Flags: approval-mozilla-esr10+
Whiteboard: [asan][sg:crititcal][release drivers: see comment 24] → [asan][sg:crititcal][release drivers: see comment 24][qa+]
Alias: CVE-2012-0470
Group: core-security
I'm able to reproduce the crash loading the test case on FF 12 2012-03-08-mozilla-aurora-debug and FF 13 2012-03-08-mozilla-central-debug builds.
No crashes on Ubuntu 12.04 64-bit on:
FF 12 2012-05-25-mozilla-release-debug
FF 13 2012-05-25-mozilla-beta-debug
FF 14 2012-05-25-mozilla-aurora-debug
FF 10.0.5 ESR 2012-05-25-mozilla-esr10-debug
Marking as verified fixed.
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.