As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact
Last Comment Bug 648090 - (CVE-2011-0083) Mozilla Firefox SVGPathSegList.replaceItem Remote Code Execution Vulnerability (ZDI-CAN-1142)
: Mozilla Firefox SVGPathSegList.replaceItem Remote Code Execution Vulnerabilit...
: 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]
: Jet Villegas (:jet)
Depends on:
  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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

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

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 

    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);
  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 User image 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 User image 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 User image Daniel Veditz [:dveditz] 2011-04-06 16:15:01 PDT
Created attachment 524301 [details]
PoC (try 2)
Comment 4 User image Jonathan Watt [:jwatt] 2011-04-06 17:28:53 PDT
Classes that have this bug:


Classes that do NOT have this bug:

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

> nsSVGNumberList::ReplaceItem(nsIDOMSVGNumber *newItem,
>                              PRUint32 index,
>                              nsIDOMSVGNumber **_retval)
> {
>-  nsresult rv = RemoveItem(index, _retval);
>-  if (NS_FAILED(rv))
>-    return rv;
>+  if (index >= mNumbers.Length()) {
>+  }
>-  return InsertElementAt(newItem, index);
>+  WillModify();
>+  NS_RELEASE(mNumbers[index]);
>+  mNumbers[index] = newItem;
>+  NS_ADDREF(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 User image Jonathan Watt [:jwatt] 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 User image Brandon Sterne (:bsterne) 2011-04-19 08:30:12 PDT
Comment on attachment 525534 [details] [diff] [review]

Granting approval for and (assuming the same patch applies there) for release-drivers.
Comment 9 User image Jonathan Watt [:jwatt] 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 User image Jonathan Watt [:jwatt] 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 User image Jonathan Watt [:jwatt] 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 User image 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 User image 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 User image 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 User image 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 User image Jonathan Watt [:jwatt] 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 User image Jonathan Watt [:jwatt] 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 19 User image Al Billings [:abillings] 2011-06-07 17:11:24 PDT
Loading the PoC in, I get this crash:

In (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: 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.