Heap-buffer-overflow in nsSVGTextFrame2::ResolvePositions

VERIFIED FIXED in Firefox 27

Status

()

Core
SVG
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Atte Kettunen, Assigned: heycam)

Tracking

(4 keywords)

27 Branch
mozilla28
crash, csectype-bounds, sec-critical, testcase
Points:
---
Bug Flags:
sec-bounty +
in-testsuite ?

Firefox Tracking Flags

(firefox26 unaffected, firefox27+ verified, firefox28+ verified, firefox-esr17 unaffected, firefox-esr24 unaffected, b2g18 unaffected, b2g-v1.2 unaffected)

Details

(Whiteboard: [reporter-external][asan])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
Created attachment 819566 [details]
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
.
.
.
(Reporter)

Comment 1

5 years ago
Created attachment 819570 [details]
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
.
.
.
(Assignee)

Comment 2

5 years ago
I can take a look at this next week, once I'm back at my machine with asan builds available.
Assignee: nobody → cam
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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.
(Assignee)

Comment 6

5 years ago
Created attachment 823793 [details] [diff] [review]
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.
Attachment #823793 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
status-firefox27: --- → affected
status-firefox28: --- → affected
tracking-firefox28: --- → ?
tracking-firefox-esr17: --- → ?
(Assignee)

Updated

5 years ago
tracking-firefox27: --- → ?
tracking-firefox-esr17: ? → ---
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.
Attachment #823793 - Flags: review?(roc) → review+
Is this a firefox 27 regression ? Can we also get a security rating here to consider for esr.
Flags: sec-bounty?
Whiteboard: [reporter-external]
(Assignee)

Comment 10

5 years ago
Backed out (realised I forgot to request sec-approval): https://hg.mozilla.org/integration/mozilla-inbound/rev/b2a356016ca4
Version: unspecified → 27 Branch
Isn't the cat out the bag already once it hits the first time?
(Assignee)

Comment 12

5 years ago
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.
Attachment #823793 - Flags: sec-approval?
(Assignee)

Comment 13

5 years ago
(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... :(
(Assignee)

Comment 14

5 years ago
Although going from patch to exploit would be quite hard, IMO.
(Assignee)

Comment 15

5 years ago
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.

Updated

5 years ago
Severity: normal → critical
Flags: in-testsuite?
Keywords: crash, csec-bounds, sec-critical, testcase
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [reporter-external] → [reporter-external][asan]
(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 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.
Attachment #823793 - Flags: sec-approval?
Attachment #823793 - Flags: sec-approval+
Attachment #823793 - Flags: approval-mozilla-aurora+
status-b2g18: --- → ?
status-firefox26: --- → unaffected
status-firefox-esr17: --- → unaffected
status-firefox-esr24: --- → unaffected
tracking-firefox27: ? → +
tracking-firefox28: ? → +
status-b2g18: ? → unaffected
status-b2g-v1.2: --- → unaffected
(Assignee)

Updated

5 years ago
Duplicate of this bug: 933582
https://hg.mozilla.org/mozilla-central/rev/238b3c81f42c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox27: affected → fixed
status-firefox28: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Flags: sec-bounty? → sec-bounty+

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Confirmed crash on ASan FF27, 2013-09-25.
Verified fixed on ASan FF27, 2013-11-20.
Verified fixed on ASan FF28, 2013-11-22.
Status: RESOLVED → VERIFIED
status-firefox27: fixed → verified
status-firefox28: fixed → verified
Group: core-security
You need to log in before you can comment on or make changes to this bug.