963198 - (CVE-2014-1508) out of bounds read in libxul.so!gfxContext::Polygon

Status: VERIFIED FIXED

(Core :: MathML, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: C9881027-223471-mathml.html

Found by the BlackBerry Security Automated Analysis Team's fuzzing framework ALF.

==7859==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffa141ed10 at pc 0x7fb96506196c bp 0x7fffa141e850 sp 0x7fffa141e848
READ of size 8 at 0x7fffa141ed10 thread T0
    #0 0x7fb96506196b (libxul.so!gfxContext::Polygon(gfxPoint const*, unsigned int)+0x39b)
	Line 66 of "/builds/slave/m-in-l64-asan-0000000000000000/build/gfx/thebes/gfx2DGlue.h"
    #1 0x7fb9684f94a3 (libxul.so!nsDisplayNotation::Paint(nsDisplayListBuilder*, nsRenderingContext*)+0xa43)
	Line 803 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/mathml/nsMathMLmencloseFrame.cpp"
    #2 0x7fb967e481c7 (libxul.so!mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, nsIntRect const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, nsIntPoint const&, float, float, int)+0xe47)
	Line 3487 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/FrameLayerBuilder.cpp"
    #3 0x7fb967e4a4dd (libxul.so!mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*)+0x15fd)
	Line 3649 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/FrameLayerBuilder.cpp"
    #4 0x7fb965118b41 (libxul.so!mozilla::layers::BasicThebesLayer::PaintBuffer(gfxContext*, nsIntRegion const&, nsIntRegion const&, nsIntRegion const&, bool, mozilla::layers::DrawRegionClip, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*)+0x81)
	Line 114 of "/builds/slave/m-in-l64-asan-0000000000000000/build/gfx/layers/basic/BasicThebesLayer.h"
    #5 0x7fb9650fa5aa (libxul.so!mozilla::layers::BasicThebesLayer::Validate(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*)+0x69a)
	Line 208 of "/builds/slave/m-in-l64-asan-0000000000000000/build/gfx/layers/basic/BasicThebesLayer.cpp"
    #6 0x7fb9650ed4bd (libxul.so!non-virtual thunk to mozilla::layers::BasicContainerLayer::Validate(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*)+0x8d)
	Line 124 of "/builds/slave/m-in-l64-asan-0000000000000000/build/gfx/layers/basic/BasicContainerLayer.cpp"
    #7 0x7fb9650ed4bd (libxul.so!non-virtual thunk to mozilla::layers::BasicContainerLayer::Validate(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*)+0x8d)
	Line 124 of "/builds/slave/m-in-l64-asan-0000000000000000/build/gfx/layers/basic/BasicContainerLayer.cpp"
    #8 0x7fb9650f061f (libxul.so!mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags)+0x56f)
	Line 597 of "/builds/slave/m-in-l64-asan-0000000000000000/build/gfx/layers/basic/BasicLayerManager.cpp"
    #9 0x7fb967f256ef (libxul.so!nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const+0x13bf)
	Line 1231 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsDisplayList.cpp"
    #10 0x7fb967f2424a (libxul.so!nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const+0x24a)
	Line 1076 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsDisplayList.cpp"
    #11 0x7fb967f75d95 (libxul.so!nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int)+0x2205)
	Line 2339 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsLayoutUtils.cpp"
    #12 0x7fb967dee741 (libxul.so!PresShell::Paint(nsView*, nsRegion const&, unsigned int)+0xe11)
	Line 5847 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsPresShell.cpp"
    #13 0x7fb966d8e87c (libxul.so!nsViewManager::ProcessPendingUpdatesForView(nsView*, bool)+0x3fc)
	Line 420 of "/builds/slave/m-in-l64-asan-0000000000000000/build/view/src/nsViewManager.cpp"
    #14 0x7fb967e121d0 (libxul.so!nsRefreshDriver::Tick(long, mozilla::TimeStamp)+0x2ba0)
	Line 1207 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp"
    #15 0x7fb967e16030 (libxul.so!mozilla::RefreshDriverTimer::Tick()+0x1f0)
	Line 168 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/base/nsRefreshDriver.cpp"
    #16 0x7fb963af5d83 (libxul.so!nsTimerImpl::Fire()+0x6b3)
	Line 551 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp"
    #17 0x7fb963af64e6 (libxul.so!nsTimerEvent::Run()+0x66)
	Line 635 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsTimerImpl.cpp"
    #18 0x7fb963aedcf7 (libxul.so!nsThread::ProcessNextEvent(bool, bool*)+0xbd7)
	Line 637 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/threads/nsThread.cpp"
    #19 0x7fb9639c66d1 (libxul.so!NS_ProcessNextEvent(nsIThread*, bool)+0xb1)
	Line 263 of "/builds/slave/m-in-l64-asan-0000000000000000/build/xpcom/glue/nsThreadUtils.cpp"
    #20 0x7fb9642e7a81 (libxul.so!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)+0x311)
	Line 95 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/glue/MessagePump.cpp"
    #21 0x7fb9642592f3 (libxul.so!MessageLoop::Run()+0x1c3)
	Line 226 of "/builds/slave/m-in-l64-asan-0000000000000000/build/ipc/chromium/src/base/message_loop.cc"
    #22 0x7fb966421a5c (libxul.so!nsBaseAppShell::Run()+0x5c)
	Line 161 of "/builds/slave/m-in-l64-asan-0000000000000000/build/widget/xpwidgets/nsBaseAppShell.cpp"
    #23 0x7fb969111676 (libxul.so!nsAppStartup::Run()+0xc6)
	Line 276 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/components/startup/nsAppStartup.cpp"
    #24 0x7fb968f29ca5 (libxul.so!XREMain::XRE_mainRun()+0x1de5)
	Line 4023 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #25 0x7fb968f2abda (libxul.so!XREMain::XRE_main(int, char**, nsXREAppData const*)+0x4fa)
	Line 4091 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #26 0x7fb968f2bb0b (libxul.so!XRE_main+0x3ab)
	Line 4331 of "/builds/slave/m-in-l64-asan-0000000000000000/build/toolkit/xre/nsAppRunner.cpp"
    #27 0x459dcd (firefox!main+0x94d)
	Line 280 of "/builds/slave/m-in-l64-asan-0000000000000000/build/browser/app/nsBrowserApp.cpp"
    #28 0x7fb97416676c (libc.so.6!__libc_start_main+0xec)
	Line 226 of "libc-start.c"
    #29 0x45934c (firefox!_start+0x28)
Address 0x7fffa141ed10 is located in stack of thread T0 at offset 784 in frame
    #0 0x7fb9684f8a6f (libxul.so!nsDisplayNotation::Paint(nsDisplayListBuilder*, nsRenderingContext*)+0xf)
	Line 739 of "/builds/slave/m-in-l64-asan-0000000000000000/build/layout/mathml/nsMathMLmencloseFrame.cpp"
  This frame has 11 object(s):
    [32, 64) 'rect'
    [96, 112) ''
    [160, 176) ''
    [224, 288) ''
    [320, 336) ''
    [384, 400) ''
    [448, 464) ''
    [512, 528) ''
    [576, 592) ''
    [640, 656) ''
    [704, 784) 'p'
HINT: this may be a false positive if your program uses some custom stack unwind mechanism or swapcontext
      (longjmp and C++ exceptions *are* supported)
Shadow bytes around the buggy address:
  0x10007427bd50: f2 f2 f2 f2 00 00 f4 f4 f2 f2 f2 f2 00 00 00 00
  0x10007427bd60: 00 00 00 00 f2 f2 f2 f2 00 00 f4 f4 f2 f2 f2 f2
  0x10007427bd70: 00 00 f4 f4 f2 f2 f2 f2 00 00 f4 f4 f2 f2 f2 f2
  0x10007427bd80: 00 00 f4 f4 f2 f2 f2 f2 00 00 f4 f4 f2 f2 f2 f2
  0x10007427bd90: 00 00 f4 f4 f2 f2 f2 f2 00 00 00 00 00 00 00 00
=>0x10007427bda0: 00 00[f4]f4 f3 f3 f3 f3 00 00 00 00 00 00 00 00
  0x10007427bdb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007427bdc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10007427bdd0: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
  0x10007427bde0: 00 00 f4 f4 f2 f2 f2 f2 00 00 00 00 f2 f2 f2 f2
  0x10007427bdf0: 00 00 00 00 f3 f3 f3 f3 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:     fa
  Heap right redzone:    fb
  Freed heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe
==7859==ABORTING

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Don't mix up byte-size and array-length

Thanks a lot for the report.

This bug was introduced by bug 875294 's patch, and has Target Milestone 24 which suggests that it might be on ESR24 and on B2G>=1.2 (I haven't checked).

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8364635 [details] [diff] [review]
Don't mix up byte-size and array-length

Stealing review at bjacob's request.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f2628d41325

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Oops, forgot about the new security-bugs-landing-rules. Sorry about that.

Comment 5

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

Well, crap. ("New" is about a year old now, BTW!)

How far back does this go? Can you go and mark this as sec-approval?, copy the template, and fill out the questions we ask without actually setting sec-approval?

We'll need to know if this affects Aurora, Beta, Release, and ESR24. Up above, it sounds like it *might* affect ESR24 but now that we've checked a sec-critical into trunk, we need to figure that out.

Comment 6

[Daniel Veditz [:dveditz]]

The original bug checked in the "updiagonalarrow" feature, but checked in a test for "updiagonalstrike"; our own TBPL ASAN tests (not even fuzzing) would have caught this if we'd had the right test.

But where does the exploitable condition happen? We're reading too many points in Polygon() so we're going to draw crap, but isn't that the end of it?

Comment 7

[Daniel Veditz [:dveditz]]

filed bug 963324 for fixing the test

Comment 8

[Christoph Diehl [:posidron]]

(In reply to Daniel Veditz [:dveditz] from comment #6)
> The original bug checked in the "updiagonalarrow" feature, but checked in a
> test for "updiagonalstrike"; our own TBPL ASAN tests (not even fuzzing)
> would have caught this if we'd had the right test.
> 
> But where does the exploitable condition happen? We're reading too many
> points in Polygon() so we're going to draw crap, but isn't that the end of
> it?

It is a read of a full 64bit address. In the first case we need to assume that this can be misused to predict memory locations for bypassing ASLR and it is more painful to upgrade a security issue than to downgrade it.

Comment 9

[Carsten Book [:Tomcat]]

landed on central as https://hg.mozilla.org/mozilla-central/rev/7f2628d41325

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Al Billings [:abillings] from comment #5)
> Can you go and mark this as sec-approval?, copy
> the template, and fill out the questions we ask without actually setting
> sec-approval?

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

I am not a security expert, so I am only trying to think aloud here, but other people might see things that I don't.

Short version: the patch does not make it very easy by itself to build an exploit, because the bug at hand here is not the easiest to exploit, because by itself it only allows to cause some rendering to depend on stack values, and for the attacker to get at that rendering requires more effort.

Long version: what the patch gives away is that MathML can be used to draw a polygon whose vertices are read from the stack. An attacker could hope to use that to read stack values, if they know a way to read back the pixels drawn for that polygon. In theory, I believe, it should not be possible to read back pixels from the rendering of MathML content. In practice though, we all know how many security flaws exist around the rules that are supposed to prevent reading back pixel values. I'm thinking of e.g.
  http://contextis.co.uk/files/Browser_Timing_Attacks.pdf
and while we have seen many recent improvements in e.g. SVG filters resilience to timing attacks recently on mozilla-central, the release, beta, aurora, ESR and B2G1.2 channels have received fewer fixes. Just to give the most recent example, bug 941887 is not yet fixed on release, beta, aurora, ESR, or B2G1.2.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No, there is no comment, no helpful commit message, no tests.

> Which older supported branches are affected by this flaw?

All of the following trees have the bug:
 mozilla-aurora
 mozilla-beta
 mozilla-release
 mozilla-esr24
 mozilla-b2g28_v1_3
 mozilla-b2g26_v1_2

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

I believe that the 1-line patch landed on central should apply to all supported branches. The only thing I'm relying on is that in all branches, this file (recursively) includes the header that defines MOZ_ARRAY_LENGTH.

> How likely is this patch to cause regressions; how much testing does it need?

Not risky.

Comment 11

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

Thanks.

We should get this nominated for ESR24 and Aurora though it is only likely to be approved for Aurora now. As of last night, we've released our last Beta build and will be build release candidates for the release in a week and a half soon.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

(I am not taking further action at this point because you asked me not to actually setting sec-approval. Hope I'm getting this right. I also shouldn't be really needed anymore, since the patch should apply cleanly to all branches).

Comment 13

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

Yes, sec-approval is for trunk and it is in there. You should nominate it for Aurora if you want it to go there. I think it should so it will be on Beta in a week and a half.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Are we not going to take this on beta at this point? What about ESR24? What about b2g 1.2 and 1.3 trees?

Comment 15

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

It is too late for Beta and ESR24 in this cycle. The last beta of anything was last night and we are making RC builds in the next couple of days. We ship in a week and a half. Generally, we take very little in the last two weeks of the ship cycle.

I can't speak to B2G since I'm not directly involved there.

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8364635 [details] [diff] [review]
Don't mix up byte-size and array-length

[Approval Request Comment]
All the questions are answered in comment 10.

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8364635 [details] [diff] [review]
Don't mix up byte-size and array-length

[Approval Request Comment]
See comment 10.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8364635 [details] [diff] [review]
Don't mix up byte-size and array-length

b2g28 = aurora until 28 goes to beta.
https://wiki.mozilla.org/Release_Management/B2G_Landing

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9bf1936426ef

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

:heycam tells me that in principle it is possible to put MathML in a SVG foreignObject. This means that the attack scheme described in comment 10 should be practical.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Applies cleanly to b2g26/esr24. Looks like it's missing the esr24 nomination, though.

Comment 22

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8364635 [details] [diff] [review]
Don't mix up byte-size and array-length

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: It is now known to be a sec:high since the stack data leaked to MathML rendering can be readily accessed by an attacker by putting that MathML content in a SVG foreignObject and passing that to canvas.drawImage.
User impact if declined: information (stack data) leakage
Fix Landed on Version: 29, and now aurora 28
Risk to taking this patch (and alternatives if risky): not risky, trivial 1-line patch
String or UUID changes made by this patch: none

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/3dad7802cc14

Comment 24

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> Applies cleanly to b2g26/esr24. Looks like it's missing the esr24
> nomination, though.

That's because we just built ESR24 (or are building it). ESR24 will need to wait, it missed the train.

Shouldn't this bug be marked as sec-high based on comment 22?

Comment 25

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Al Billings [:abillings] from comment #24)
> Shouldn't this bug be marked as sec-high based on comment 22?

If information leakage bugs are still sec-high then yes.

Comment 26

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

Might be a sec-moderate. Dan?

Comment 27

[Daniel Veditz [:dveditz]]

This seems more like a stepping-stone to worse things than a way to slurp up all your memory, sec-moderate seem OK to me. We should still fix it everywhere and I'm happy to call it sec-high if that's what it takes to do that.

Comment 29

[Daniel Veditz [:dveditz]]

Since this potentially leaks the address needed to defeat ASLR we're raising this back up to sec-high

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/c9ffd147d6b5

Comment 31

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

Confirmed crash in Fx27 release.
Verified fixed in Fx28, 2014-03-13.
Verified fixed in Fx29, 2014-03-13.
Verified fixed in Fx24esrpre, 2014-02-26.