812161 - (CVE-2013-0767) Out of bounds read in nsSVGPathElement::GetPathLengthScale

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Abhishek Arya]

Attachment: Testcase

Reproduces on trunk.

=================================================================
==17302== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f39dafcdb51 at pc 0x7f3a091d1adf bp 0x7ffff97a5b90 sp 0x7ffff97a5b88
READ of size 1 at 0x7f39dafcdb51 thread T0
    #0 0x7f3a091d1ade in nsSVGNumber2::IsExplicitlySet() const src/content/svg/content/src/nsSVGNumber2.h:49
    #1 0x7f3a09663aa3 in nsSVGPathElement::GetPathLengthScale(nsSVGPathElement::PathLengthScaleForType) src/content/svg/content/src/nsSVGPathElement.cpp:404
    #2 0x7f3a08d9d082 in nsSVGTextPathFrame::GetOffsetScale() src/layout/svg/nsSVGTextPathFrame.cpp:146
    #3 0x7f3a08ceeb9a in nsSVGGlyphFrame::SetGlyphPosition(gfxPoint*, bool) src/layout/svg/nsSVGGlyphFrame.cpp:1329
    #4 0x7f3a08d95224 in nsSVGTextFrame::UpdateGlyphPositioning(bool) src/layout/svg/nsSVGTextFrame.cpp:474
    #5 0x7f3a08d978a9 in nsSVGTextFrame::ReflowSVG() src/layout/svg/nsSVGTextFrame.cpp:247
    #6 0x7f3a08d979cf in non-virtual thunk to nsSVGTextFrame::ReflowSVG() src/layout/svg/nsSVGTextFrame.cpp:253
    #7 0x7f3a08c63367 in nsSVGDisplayContainerFrame::ReflowSVG() src/layout/svg/nsSVGContainerFrame.cpp:271
    #8 0x7f3a08d51ea8 in nsSVGOuterSVGFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/svg/nsSVGOuterSVGFrame.cpp:450
    #9 0x7f3a02f1d545 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:842
    #10 0x7f3a02eee11c in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) src/layout/generic/nsInlineFrame.cpp:694
    #11 0x7f3a02eeb0d5 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:557
    #12 0x7f3a02ee7d25 in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsInlineFrame.cpp:409
    #13 0x7f3a02f1d545 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) src/layout/generic/nsLineLayout.cpp:842
    #14 0x7f3a02bd4a61 in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) src/layout/generic/nsBlockFrame.cpp:3721
    #15 0x7f3a02bcf0c5 in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) src/layout/generic/nsBlockFrame.cpp:3518
    #16 0x7f3a02bc1d19 in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:3372
    #17 0x7f3a02bb102a in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) src/layout/generic/nsBlockFrame.cpp:2479
    #18 0x7f3a02b97e57 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) src/layout/generic/nsBlockFrame.cpp:1996
    #19 0x7f3a02b8b143 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsBlockFrame.cpp:1041
    #20 0x7f3a02c777cc in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:952
    #21 0x7f3a02e49e29 in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsCanvasFrame.cpp:464
    #22 0x7f3a02c777cc in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:952
    #23 0x7f3a02dbfe28 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) src/layout/generic/nsGfxScrollFrame.cpp:433
    #24 0x7f3a02dc4986 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) src/layout/generic/nsGfxScrollFrame.cpp:533
    #25 0x7f3a02dc9020 in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsGfxScrollFrame.cpp:774
    #26 0x7f3a02c777cc in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) src/layout/generic/nsContainerFrame.cpp:952
    #27 0x7f3a031b7fea in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) src/layout/generic/nsViewportFrame.cpp:202
    #28 0x7f3a028eae9b in PresShell::DoReflow(nsIFrame*, bool) src/layout/base/nsPresShell.cpp:7532
    #29 0x7f3a0291a94d in PresShell::ProcessReflowCommands(bool) src/layout/base/nsPresShell.cpp:7673
    #30 0x7f3a0291949e in PresShell::FlushPendingNotifications(mozFlushType) src/layout/base/nsPresShell.cpp:3913
    #31 0x7f3a029bfc15 in nsRefreshDriver::Notify(nsITimer*) src/layout/base/nsRefreshDriver.cpp:403
    #32 0x7f3a0efeb393 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:485
    #33 0x7f3a0efec731 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:565
    #34 0x7f3a0efaf3de in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
    #35 0x7f3a0ec25a8f in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:221
    #36 0x7f3a0d218776 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #37 0x7f3a0f2976ae in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
    #38 0x7f3a0f2974f5 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
    #39 0x7f3a0f2973db in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
    #40 0x7f3a0c5f8a64 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #41 0x7f3a0b17fa32 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #42 0x7f3a006b4e14 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3822
    #43 0x7f3a006baae9 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3889
    #44 0x7f3a006bd860 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4083
    #45 0x40c1e6 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #46 0x409a20 in main src/browser/app/nsBrowserApp.cpp:279
    #47 0x7f3a1fb9176c in ?? ??:0
0x7f39dafcdb51 is located 9 bytes to the right of 264-byte region [0x7f39dafcda40,0x7f39dafcdb48)
allocated by thread T0 here:
    #0 0x4c3940 in malloc ??:?
    #1 0x7f3a20ba25f1 in moz_xmalloc src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7f3a096c7c31 in operator new(unsigned long) src/../../../../dist/include/mozilla/mozalloc.h:200
    #3 0x7f3a090eb1cb in NS_NewSVGElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/svg/content/src/nsSVGElementFactory.cpp:232
    #4 0x7f3a04598d6c in NS_NewElement(nsIContent**, already_AddRefed<nsINodeInfo>, mozilla::dom::FromParser) src/content/base/src/nsNameSpaceManager.cpp:212
    #5 0x7f3a06232a6b in nsXMLContentSink::CreateElement(unsigned short const**, unsigned int, nsINodeInfo*, unsigned int, nsIContent**, bool*, mozilla::dom::FromParser) src/content/xml/document/src/nsXMLContentSink.cpp:467
    #6 0x7f3a0623fff4 in nsXMLContentSink::HandleStartElement(unsigned short const*, unsigned short const**, unsigned int, int, unsigned int, bool) src/content/xml/document/src/nsXMLContentSink.cpp:997
    #7 0x7f3a0623ec5e in nsXMLContentSink::HandleStartElement(unsigned short const*, unsigned short const**, unsigned int, int, unsigned int) src/content/xml/document/src/nsXMLContentSink.cpp:951
    #8 0x7f3a06241b95 in non-virtual thunk to nsXMLContentSink::HandleStartElement(unsigned short const*, unsigned short const**, unsigned int, int, unsigned int) src/content/xml/document/src/nsXMLContentSink.cpp:953
Shadow byte and word:
  0x1fe73b5f9b6a: fb
  0x1fe73b5f9b68: 00 fb fb fb fb fb fb fb
More shadow bytes:
  0x1fe73b5f9b48: 00 00 00 00 00 00 00 00
  0x1fe73b5f9b50: 00 00 00 00 00 00 00 00
  0x1fe73b5f9b58: 00 00 00 00 00 00 00 00
  0x1fe73b5f9b60: 00 00 00 00 00 00 00 00
=>0x1fe73b5f9b68: 00 fb fb fb fb fb fb fb
  0x1fe73b5f9b70: fa fa fa fa fa fa fa fa
  0x1fe73b5f9b78: fa fa fa fa fa fa fa fa
  0x1fe73b5f9b80: fa fa fa fa fa fa fa fa
  0x1fe73b5f9b88: 00 00 00 00 00 00 00 00
Stats: 253M malloced (239M for red zones) by 412251 calls
Stats: 46M realloced by 24186 calls
Stats: 224M freed by 284857 calls
Stats: 189M really freed by 232799 calls
Stats: 230M (59085 full pages) mmaped in 438 calls
  mmaps   by size class: 7:147420; 8:45034; 9:13299; 10:5110; 11:6120; 12:1280; 13:960; 14:544; 15:224; 16:712; 17:456; 18:36; 19:35; 20:21;
  mallocs by size class: 7:267252; 8:86611; 9:23441; 10:8604; 11:16914; 12:2459; 13:1943; 14:1595; 15:413; 16:1542; 17:1345; 18:70; 19:40; 20:22;
  frees   by size class: 7:182188; 8:57665; 9:17102; 10:5439; 11:14824; 12:1611; 13:1505; 14:1424; 15:281; 16:1375; 17:1329; 18:57; 19:38; 20:19;
  rfrees  by size class: 7:155466; 8:44862; 9:10771; 10:3830; 11:11718; 12:1214; 13:1011; 14:1303; 15:216; 16:1017; 17:1295; 18:50; 19:37; 20:9;
Stats: malloc large: 3432 small slow: 5044
==17302== ABORTING

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

It's not enough just to check that the frame a textPath uses as a path is a pathGeometry frame as many graphics objects use pathGeometryFrames e.g. rect, circle. We must check and allow only an actual path frame.

Comment 2

[Robert Longson [:longsonr]]

We must check and allow only an actual path *element*.

Comment 3

[Robert Longson [:longsonr]]

The patch is hidden in this try https://tbpl.mozilla.org/?tree=Try&rev=bac82e46e543

Comment 4

[Robert Longson [:longsonr]]

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

[Security approval request comment]
How easily can the security issue be deduced from the patch? Seems possible to deduce that we've issues with textPath linking to non-path elements. I am familiar with the code though so maybe it seems easier than it actually is.

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? All of them, this bug has been around since at least Firefox 4 and almost certainly before that.

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

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

How likely is this patch to cause regressions; how much testing does it need? Regressions are very unlikely. textPaths are pretty well covered by existing reftests.

Comment 5

[Robert Longson [:longsonr]]

Thinking about this some more, although the patch does suggest that we've issues with textPath pointing to non-paths, it's a lot less obvious that you also need to call the DOM javascript method getPathLengthScale once you've done that.

Comment 6

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

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

sec-approval+

Are tests going to be updated to cover this scenario? If so, we should do it after the fix ships.

Comment 7

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/17c5efe9eab4

Comment 8

[Jonathan Watt [:jwatt]]

(In reply to Robert Longson from comment #5)
> the patch does suggest that we've
> issues with textPath pointing to non-paths

(Note that it's necessary to create an nsSVGTextPathProperty even if the element being pointed to is not a <path>, since it's the nsSVGTextPathProperty that watches for ID changes, and if the ID is moved to a valid <path> we need to be notified of that.)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/17c5efe9eab4

Comment 10

[Robert Longson [:longsonr]]

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

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: exploitable security hole, bad virtual fn call
Fix Landed on Version: 19
Risk to taking this patch (and alternatives if risky): very low.
String or UUID changes made by this patch: none

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown, likely present since Firefox 3 though
User impact if declined: exploitable security hole, bad virtual fn call.
Testing completed (on m-c, etc.): landed on m-c, checked manually that testcase no longer makes invalid calls.
Risk to taking this patch (and alternatives if risky): very low
String or UUID changes made by this patch: none

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

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

We can take the uplift to Aurora (18) - please land before Monday 11/19 merge day.  Also will leave the ESR17 nomination for now and we'll come back to it to get it landed in time for the ESR that ships with 18.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Should have mentioned - minusing for approval beta because we've shipped with it since FF4 and we've already gone to build with final 17.

Comment 13

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e69a5d9d0db

Comment 14

[Alex Keybl [:akeybl]]

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

Approving for ESR17. Please prepare a patch for ESR10 as well.

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: patch for ESR10

(In reply to Alex Keybl [:akeybl] from comment #14)
> Approving for ESR17. Please prepare a patch for ESR10 as well.

Here you go.  (It's the same as the trunk patch, with s/nullptr/nsnull/ and with the path changed from layout/svg to layout/svg/base/src/ to correct for bug 596753)

Comment 16

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/e0b6dfb1ef4d

Comment 17

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/ed34973f6e18

Comment 19

[u279076]

Kamil, can you please verify this is fixed with the latest ASan builds?

Comment 20

[Kamil Jozwiak [:kjozwiak]]

Hi Abhishek,

Could you help me reproduce the following issue? Attempted to reproduce it several times using the testcase that has been added without any luck. Tried using the below builds on both Windows 7 and Linux.

http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/16.0.2/win32/en-US/
http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/17.0.1/win32/en-US/
http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/17.0.3esr/win32/en-US/

Comment 21

[Abhishek Arya]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #20)
> Hi Abhishek,
> 
> Could you help me reproduce the following issue? Attempted to reproduce it
> several times using the testcase that has been added without any luck. Tried
> using the below builds on both Windows 7 and Linux.
> 
> http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/16.0.2/win32/en-US/
> http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/17.0.1/win32/en-US/
> http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/17.0.3esr/win32/en-
> US/

Kamil, this is a memory corruption bug and might not cause a crash in regular builds. You need to run it in an ASAN build (See instructions here - https://developer.mozilla.org/en-US/docs/Building_Firefox_with_Address_Sanitizer and builds here https://people.mozilla.com/~choller/firefox/asan/). This should also reproduce under valgrind.

Comment 22

[Kamil Jozwiak [:kjozwiak]]

I tried reproducing the issue using the builds that are available at the following location:

https://people.mozilla.com/~choller/firefox/asan/

The earliest build at that location is Firefox 19 which passed without any issues. I couldn't verify for the following builds:

- Firefox 18
- ESR10
- ESR17

I am going to mark this as verified but please let me know if its not OK.