Last Comment Bug 648090 - (CVE-2011-0083) Mozilla Firefox SVGPathSegList.replaceItem Remote Code Execution Vulnerability (ZDI-CAN-1142)
(CVE-2011-0083)
: Mozilla Firefox SVGPathSegList.replaceItem Remote Code Execution Vulnerabilit...
Status: RESOLVED FIXED
[sg:critical?]
: verified1.9.2
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 1.9.2 Branch
: All All
: -- normal (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-06 12:58 PDT by Brandon Sterne (:bsterne)
Modified: 2011-07-12 09:05 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
unaffected
unaffected
.18+
.18-fixed
.20+
.20-fixed


Attachments
patch (3.12 KB, patch)
2011-04-12 15:23 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
dholbert: review+
brandon: approval1.9.2.18+
brandon: approval1.9.1.20+
Details | Diff | Review

Description Brandon Sterne (:bsterne) 2011-04-06 12:58:24 PDT
Created attachment 524240 [details]
PoC

ZDI-CAN-1142: Mozilla Firefox SVGPathSegList.replaceItem Remote Code Execution Vulnerability

-- CVSS ----------------------------------------------------------------
9, (AV:N/AC:L/Au:N/C:P/I:P/A:C)

-- ABSTRACT ------------------------------------------------------------

TippingPoint has identified a vulnerability affecting the following 
products:

    Mozilla Firefox

-- VULNERABILITY DETAILS -----------------------------------------------

This vulnerability allows remote attackers to execute arbitrary code on vulnerable installations of Mozilla Firefox. User interaction is
required to exploit this vulnerability in that the target must visit a malicious page or open a malicious file.

The specific flaw exists within the code responsible for parsing SVG path segment objects. The function nsSVGPathSegList::ReplaceItem() does
not account for deletion of the segment object list within a user defined DOMAttrModified EventListener. Code within
nsSVGPathSegList::ReplaceItem() references the segment list without verifying that it was not deleted in the aforementioned callback. This
can be abused to create a dangling reference which can be leveraged to execute arbitrary code within the context of the browser.

Version(s)  tested: 3.6.13
Platform(s) tested: Win XP SP3

From content/svg/content/src/nsSVGPathSegList.cpp:

NS_IMETHODIMP nsSVGPathSegList::ReplaceItem(nsIDOMSVGPathSeg *newItem,
                                            PRUint32 index,
                                            nsIDOMSVGPathSeg **_retval)
{
  ...

  InsertElementAt(newItemSeg, index);
 
RemoveFromCurrentList(static_cast<nsSVGPathSeg*>(mSegments.ObjectAt(index+1)));
  NS_ADDREF(*_retval = newItem);

  return NS_OK;
}

During execution of |InsertElementAt| "DOMAttrModified" event is dispatched. User's provided event handler will be called. During that callback it is possible to delete whole content of |mSegments|, e.g. by calling method clear() on pathSegList object. As |mSegments| is defined as |nsCOMArray| and method |ObjectAt| does not do out-of-bound check of index argument, previously |ObjectAt| freed memory will be referenced.


-- CREDIT --------------------------------------------------------------

This vulnerability was discovered by:
    * regenrecht
Comment 1 Daniel Veditz [:dveditz] 2011-04-06 15:06:19 PDT
provisionally "critical" based on reputation, but looks like the PoC got stripped in mail.
Comment 2 Brandon Sterne (:bsterne) 2011-04-06 15:09:17 PDT
I've sent mail to the reporter requesting a re-send of the PoC.
Comment 3 Daniel Veditz [:dveditz] 2011-04-06 16:15:01 PDT
Created attachment 524301 [details]
PoC (try 2)
Comment 4 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-04-06 17:28:53 PDT
Classes that have this bug:

  nsSVGNumberList
  nsSVGPathSegList

Classes that do NOT have this bug:

  nsSVGLengthList - ReplaceItem() not implemented
  nsSVGPointList - ReplaceItem() not implemented
  nsSVGTransformList - Different implementation
Comment 5 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-04-12 15:23:13 PDT
Created attachment 525534 [details] [diff] [review]
patch
Comment 6 Daniel Holbert [:dholbert] 2011-04-12 16:06:28 PDT
Comment on attachment 525534 [details] [diff] [review]
patch

> nsSVGNumberList::ReplaceItem(nsIDOMSVGNumber *newItem,
>                              PRUint32 index,
>                              nsIDOMSVGNumber **_retval)
> {
>-  nsresult rv = RemoveItem(index, _retval);
>-  if (NS_FAILED(rv))
>-    return rv;
>+  if (index >= mNumbers.Length()) {
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
>+  }
> 
>-  return InsertElementAt(newItem, index);
>+  WillModify();
>+  NS_REMOVE_SVGVALUE_OBSERVER(mNumbers[index]);
>+  NS_RELEASE(mNumbers[index]);
>+  mNumbers[index] = newItem;
>+  NS_ADDREF(newItem);
>+  NS_ADD_SVGVALUE_OBSERVER(newItem);
>+  DidModify();

This ^ is a bit hard to parse.  Could you add a comment saying something like:
> // This is equivalent to RemoveItem() followed by InsertElementAt(), but
> // with the WillModify() / DidModify() notifications collapsed.

r=dholbert either way
Comment 7 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-04-19 06:43:17 PDT
What's the procedure for landing this on 1.9.1 and 1.9.2? I now have patches for both ready to push.
Comment 8 Brandon Sterne (:bsterne) 2011-04-19 08:30:12 PDT
Comment on attachment 525534 [details] [diff] [review]
patch

Granting approval for 1.9.2.18 and 1.9.1.20 (assuming the same patch applies there) for release-drivers.
Comment 9 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-04-19 08:44:20 PDT
Does approval mean I should push directly to 1.9.1 and 1.9.2, or is there some sort of shadow repo that I should be pushing to?
Comment 10 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-04-19 08:45:53 PDT
(In reply to comment #8)
> [snip] (assuming the same patch applies there) [snip]

The patch needs to be tweaked slightly, but it's essentially the same.
Comment 11 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-04-25 08:16:29 PDT
Daniel, can I direct comment 9 at you, or ask you to get the appropriate person to comment? Apparently security bugs aren't watched as closely as I expected so asking the wind doesn't work.
Comment 12 Daniel Veditz [:dveditz] 2011-04-28 13:57:04 PDT
Yes, please check approved patches directly into their respective branches. (shadow repo will be for trunk, as a holding pen until branch patches are ready)
Comment 13 Daniel Veditz [:dveditz] 2011-05-05 13:43:05 PDT
jst marked this tracking-firefox5+ but the version field says "1.9.2". Does this bug affect mozilla-central/aurora?
Comment 14 Johnathan Nightingale [:johnath] 2011-05-05 14:30:26 PDT
jwatt - jst's out for a while, can you tell us whether this impacts aurora as well? If so, can you request approval-aurora and land it there when you land it elsewhere?
Comment 15 Daniel Holbert [:dholbert] 2011-05-05 17:56:11 PDT
I'll defer to jwatt for the definitive answer, but AFAIK this doesn't affect Aurora / Firefox 5.  (nor does it affect Firefox 4)

The classes in question (comment 4) were completely rewritten after gecko 1.9.2
Comment 16 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-05-08 10:22:42 PDT
(In reply to comment #3)
> Created attachment 524301 [details]
> PoC (try 2)

This .zip is password protected, so I can't access it.
Comment 17 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-05-08 10:25:05 PDT
(In reply to comment #13)
> jst marked this tracking-firefox5+ but the version field says "1.9.2". Does
> this bug affect mozilla-central/aurora?

No. It only affects 1.9.2 and earlier.
Comment 18 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2011-05-08 10:40:10 PDT
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/14dbc2f02f79
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e97fac945e09
Comment 19 [On PTO until 6/29] 2011-06-07 17:11:24 PDT
Loading the PoC in 1.9.2.17, I get this crash: https://crash-stats.mozilla.com/report/index/43e40b3b-5762-4707-8995-165692110607

In 1.9.2.18pre (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.18pre) Gecko/20110607 Namoroka/3.6.18pre (.NET CLR 3.5.30729)), I get no crash and the PoC renders.

This looks to be fixed in 1.9.2.

Note You need to log in before you can comment on or make changes to this bug.