928798 - Heap-buffer-overflow in nsSVGTextFrame2::ResolvePositions

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Atte Kettunen]

Attachment: Repro-file

Tested on:

OS:Ubuntu 12.04

Firefox: ASAN build from https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1382322354/

ASAN-report:

==2874==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x619000628de1 at pc 0x7fe55ce705ff bp 0x7fff2819a2f0 sp 0x7fff2819a2e8
READ of size 1 at 0x619000628de1 thread T0
    #0 0x7fe55ce705fe in nsSVGTextFrame2::ResolvePositions(nsIContent*, unsigned int, bool, bool&, nsTArray<gfxPoint>&) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGTextFrame2.cpp:4294:0
    #1 0x7fe55ce7030b in nsSVGTextFrame2::ResolvePositions(nsIContent*, unsigned int, bool, bool&, nsTArray<gfxPoint>&) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGTextFrame2.cpp:4363:0
    #2 0x7fe55ce716d9 in nsSVGTextFrame2::ResolvePositions(nsTArray<gfxPoint>&, bool) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGTextFrame2.cpp:4408:0
    #3 0x7fe55ce76523 in nsSVGTextFrame2::DoGlyphPositioning() /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGTextFrame2.cpp:4848:0
    #4 0x7fe55ce6984a in UpdateGlyphPositioning /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGTextFrame2.cpp:5019:0
    #5 0x7fe55ce6984a in nsSVGTextFrame2::GetBBoxContribution(gfxMatrix const&, unsigned int) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGTextFrame2.cpp:3748:0
    #6 0x7fe55ce6a70c in non-virtual thunk to nsSVGTextFrame2::GetBBoxContribution(gfxMatrix const&, unsigned int) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGTextFrame2.cpp:3761:0
    #7 0x7fe55ce8c002 in nsSVGUtils::GetBBox(nsIFrame*, unsigned int) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGUtils.cpp:1244:0
    #8 0x7fe55b274505 in GetFrameBoundsForTransform /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsDisplayList.cpp:3832:0
    #9 0x7fe55b274505 in nsDisplayTransform::GetDeltaToMozTransformOrigin(nsIFrame const*, float, nsRect const*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsDisplayList.cpp:3923:0
    #10 0x7fe55b27593f in nsDisplayTransform::FrameTransformProperties::FrameTransformProperties(nsIFrame const*, float, nsRect const*) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/base/nsDisplayList.cpp:4037:0
.
.
.

Comment 1

[Atte Kettunen]

Attachment: full-ASAN-trace-opt-build

There is also some weird asan outputs, later on on the trace, so I add also the full trace here.

Weird ASAN-output:

==2874==AddressSanitizer CHECK failed: /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_allocator2.cc:228 "((id)) != (0)" (0x0, 0x0)
    #0 0x44bbf4 in __asan::AsanCheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) _asan_rtl_:0
    #1 0x450e21 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_common.cc:60:0
    #2 0x423122 in GetStackTraceFromId /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_allocator2.cc:228:0
    #3 0x423122 in __asan::AsanChunkView::GetAllocStack(__sanitizer::StackTrace*) /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_allocator2.cc:238:0
    #4 0x448ce6 in __asan::DescribeHeapAddress(unsigned long, unsigned long) _asan_rtl_:0
    #5 0x449dd4 in __asan_report_error _asan_rtl_:0
    #6 0x44ae46 in __asan_report_load1 _asan_rtl_:0
    #7 0x7fe55ce705fe in nsSVGTextFrame2::ResolvePositions(nsIContent*, unsigned int, bool, bool&, nsTArray<gfxPoint>&) /builds/slave/m-cen-l64-asan-000000000000000/build/layout/svg/nsSVGTextFrame2.cpp:4294:0
.
.
.

Comment 2

[Cameron McCormack (:heycam)]

I can take a look at this next week, once I'm back at my machine with asan builds available.

Comment 3

[Cameron McCormack (:heycam)]

We're calling in to nsSVGTextFrame2::GetBBoxContribution without its anonymous block child having been reflowed.  This is happening now due to bug 923193 needing the bounding box of the frame when computing the box for transform-origin.

Comment 4

[Cameron McCormack (:heycam)]

And scheduling of the PresShell::UpdateImageVisibility call, which is what builds display lists and ultimately causes GetBBoxContribution to be called, is racing against the scheduling of the nsSVGTextFrame2::ReflowSVG.

Comment 5

[Cameron McCormack (:heycam)]

I think also that our early exit check at the top of nsSVGTextFrame2::GetBBoxContribution, which should have caught this case and returned an empty bounding box, should be looking at the dirtiness of the nsSVGTextFrame2, not its anonymous block child.  The earlier nsSVGUtils::ScheduleReflowSVG call just sets dirty bits on the nsSVGTextFrame2.

Comment 6

[Cameron McCormack (:heycam)]

Attachment: patch

I'm guessing the consequences of returning an incorrect value to nsDisplayTransform::GetFrameBoundsForTransform doesn't matter for PresShell::UpdateImageVisibility's purpose.  But I also don't know whether it is bad or not that display lists are getting constructed for the dirty nsSVGTextFrame2.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

Building display lists for dirty trees needs to be supported. It doesn't necessarily need to produce the correct rendering, but it should be correct for the parts of the tree that aren't dirty.

Comment 8

[bhavana bajaj [:bajaj]]

Is this a firefox 27 regression ? Can we also get a security rating here to consider for esr.

Comment 9

[Cameron McCormack (:heycam)]

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

Comment 10

[Cameron McCormack (:heycam)]

Backed out (realised I forgot to request sec-approval): https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a356016ca4

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Isn't the cat out the bag already once it hits the first time?

Comment 12

[Cameron McCormack (:heycam)]

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

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

Pretty hard.  The code change is small and doesn't show what you would need to do to exploit the bug.  You'd need to know how nsSVGTextFrame2 works overall.

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

No.

Which older supported branches are affected by this flaw?

27

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

923193

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

The patch applies cleanly to mozilla-aurora.

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

Fairly unlikely.  IMO the manual testing I've done plus a green try run are sufficient.

Comment 13

[Cameron McCormack (:heycam)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Isn't the cat out the bag already once it hits the first time?

I guess so.  Sorry about that... :(

Comment 14

[Cameron McCormack (:heycam)]

Although going from patch to exploit would be quite hard, IMO.

Comment 15

[Cameron McCormack (:heycam)]

In terms of severity of this bug, it lets you write right off the end of nsSVGTextFrame2::mPositions, which is on the heap, and the offset into mPositions that ResolvePositions writes to is determined by the length of the content under the <text> element.

Comment 16

[Ed Morley [:emorley]]

(In reply to Cameron McCormack (:heycam) from comment #10)
> Backed out (realised I forgot to request sec-approval):
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a356016ca4

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/f532e4f26307

Comment 17

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

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

Well, since we (sort of) pwned ourselves by already checking it in once, there isn't much debate now. 

sec-approval+ for trunk. I also gave it Aurora approval since that is the other place affected.

Comment 18

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/238b3c81f42c
https://hg.mozilla.org/releases/mozilla-aurora/rev/27d0165a1097

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/238b3c81f42c

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out along with other stuff related to bug 923193.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5c39f4cf22

I'll fold this back in when we reland bug 923193.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Merge of backout.
https://hg.mozilla.org/mozilla-central/rev/3fb151446ec5

Going to re-resolve this based on comment 22 saying that this will be rolled into the re-landing of bug 923193.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Rather:
https://hg.mozilla.org/mozilla-central/rev/4a5c39f4cf22

Comment 25

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

Confirmed crash on ASan FF27, 2013-09-25.
Verified fixed on ASan FF27, 2013-11-20.
Verified fixed on ASan FF28, 2013-11-22.