Last Comment Bug 648094 - (CVE-2011-0084) Mozilla Firefox SVGTextElement.getCharNumAtPosition Remote Code Execution Vulnerability (ZDI-CAN-1143)
: Mozilla Firefox SVGTextElement.getCharNumAtPosition Remote Code Execution Vul...
: verified1.9.2
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonathan Watt [:jwatt]
: Jet Villegas (:jet)
Depends on:
Blocks: CVE-2011-2363
  Show dependency treegraph
Reported: 2011-04-06 13:24 PDT by Brandon Sterne (:bsterne)
Modified: 2011-09-26 23:44 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (3.49 KB, patch)
2011-04-12 16:43 PDT, Jonathan Watt [:jwatt]
dholbert: review+
dveditz: approval1.9.2.20+
jwatt: checkin+
Details | Diff | Splinter Review
backtrace for crash (1.85 KB, text/plain)
2011-06-15 09:35 PDT, Jan Horak
no flags Details
patch for 1.9.2 (1.09 KB, patch)
2011-06-16 01:15 PDT, Jonathan Watt [:jwatt]
dholbert: review+
jwatt: checkin+
Details | Diff | Splinter Review
patch for 1.9.1 (5.53 KB, patch)
2011-06-16 01:31 PDT, Jonathan Watt [:jwatt]
dholbert: review+
jwatt: checkin+
Details | Diff | Splinter Review
patch for 4.0, 5.0, 6.0, 7.0 and trunk (3.03 KB, patch)
2011-06-16 06:30 PDT, Jonathan Watt [:jwatt]
longsonr: review+
jst: approval‑mozilla‑aurora+
jst: approval‑mozilla‑beta-
dveditz: approval2.0+
Details | Diff | Splinter Review

Description Brandon Sterne (:bsterne) 2011-04-06 13:24:11 PDT
Created attachment 524245 [details]

ZDI-CAN-1143: Mozilla Firefox SVGTextElement.getCharNumAtPosition 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 text containers. The code within nsSVGGlyphFrame::GetCharNumAtPosition()
does not account for user defined getter methods modifying or destroying the parent object. An attacker can abuse this flaw to create a dangling
pointer which is referenced during the traversal of the SVG container hierarchy. This 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/nsSVGTextContentElement.cpp:

nsSVGTextContentElement::GetCharNumAtPosition(nsIDOMSVGPoint *point,
PRInt32 *_retval) {

  nsSVGTextContainerFrame* metrics = GetTextContainerFrame();
  if (metrics)
    *_retval = metrics->GetCharNumAtPosition(point);

  return NS_OK;

From layout/svg/base/src/nsSVGTextContainerFrame.cpp:

nsSVGTextContainerFrame::GetCharNumAtPosition(nsIDOMSVGPoint *point) {
  PRInt32 index = -1;
  PRInt32 offset = 0;
  nsISVGGlyphFragmentNode *node = GetFirstGlyphFragmentChildNode();

  while (node) {
    PRUint32 count = node->GetNumberOfChars();
    if (count > 0) {
      PRInt32 charnum = node->GetCharNumAtPosition(point);
      if (charnum >= 0) {
        index = charnum + offset;
      offset += count;
      // Keep going, multiple characters may match 
      // and we must return the last one
    node = GetNextGlyphFragmentChildNode(node);

  return index;

And later we reach layout/svg/base/src/nsSVGGlyphFrame.cpp:

nsSVGGlyphFrame::GetCharNumAtPosition(nsIDOMSVGPoint *point) {
  float xPos, yPos;

  nsRefPtr<gfxContext> tmpCtx = MakeTmpCtx();
  CharacterIterator iter(this, PR_FALSE);


Note that |point->GetX(&xPos)| will execute user provided implementation of 'x' property getter (as specified in interface nsIDOMSVGPoint). At that moment we can remove our SVGTextElement nodes and all the related frames will be deleted as well.

CharacterIterator::CharacterIterator(nsSVGGlyphFrame *aSource,
        PRBool aForceGlobalTransform)
  : mSource(aSource), mCurrentAdvance(0), mCurrentChar(-1),
  if (!aSource->EnsureTextRun(&mDrawScale, &mMetricsScale,
                              aForceGlobalTransform) ||
      !aSource->GetCharacterPositions(&mPositions, mMetricsScale)) {
    mInError = PR_TRUE;

nsSVGGlyphFrame::EnsureTextRun(float *aDrawScale, float *aMetricsScale,
                               PRBool aForceGlobalTransform) {
  // Compute the size at which the text should render (excluding the
  const nsStyleFont* fontData = GetStyleFont();


And here we reach a dangling pointer.

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

This vulnerability was discovered by:
    * regenrecht
Comment 1 Jonathan Watt [:jwatt] 2011-04-12 16:43:21 PDT
Created attachment 525550 [details] [diff] [review]
Comment 2 Daniel Holbert [:dholbert] 2011-04-12 17:01:33 PDT
Comment on attachment 525550 [details] [diff] [review]

looks good! Just one nit:

>+#define NS_ENSURE_NATIVE_SVG_POINT(obj, retval)                \
>+  {                                                            \
>+    nsresult rv;                                               \
>+    if (retval)                                                \
>+      *retval = nsnull;                                        \
>+    nsCOMPtr<nsISVGValue> path = do_QueryInterface(obj, &rv);   \

Comment 3 Daniel Veditz [:dveditz] 2011-04-28 14:03:11 PDT
Is this a branch-only problem or does it affect trunk as well?
Comment 4 Daniel Holbert [:dholbert] 2011-04-28 19:36:27 PDT
(In reply to comment #3)
> Is this a branch-only problem or does it affect trunk as well?

I'm pretty sure this is branch-only, since the affected classes (nsSVGLengthList & nsSVGPointList) were completely rewritten & renamed after 1.9.2 branched.

I'll let jwatt provide a definite response to comment 3, though.
Comment 5 Daniel Veditz [:dveditz] 2011-05-04 10:57:20 PDT
Comment on attachment 525550 [details] [diff] [review]

Approved for, a=dveditz for release-drivers
Comment 6 Jonathan Watt [:jwatt] 2011-05-08 10:35:39 PDT
Seems like the POC has had the exploit code stripped by Exchange Server (so says the contents of "poc..txt").
Comment 7 Jonathan Watt [:jwatt] 2011-05-08 10:36:47 PDT
(In reply to comment #3)
> Is this a branch-only problem or does it affect trunk as well?

It only affects 1.9.2 and earlier.
Comment 9 Al Billings [:abillings] 2011-06-07 17:13:51 PDT
(In reply to comment #6)
> Seems like the POC has had the exploit code stripped by Exchange Server (so
> says the contents of "poc..txt").


I need the PoC if I'm going to verify this fix and poc.html was removed from the attached zip file.
Comment 10 Brandon Sterne (:bsterne) 2011-06-08 10:54:18 PDT
Created attachment 538061 [details]
Correct PoC

Password is ZDI-CAN-1143
Comment 11 Jan Horak 2011-06-15 09:35:49 PDT
Created attachment 539564 [details]
backtrace for crash

I'm still getting crash with Firefox 3.6.18 with poc.html (same with 3.6.17). See attachment for backtrace.
Comment 12 christian 2011-06-15 09:39:30 PDT
Al, did you ever get around to verifying this?
Comment 13 Al Billings [:abillings] 2011-06-15 10:24:19 PDT
I tried just now, this somehow slipped through my fingers when finished blocking verification yesterday.

I can confirm that the PoC still crashes in build 1. crash: crash:
Comment 14 Daniel Veditz [:dveditz] 2011-06-15 10:24:42 PDT
If the checked-in patch doesn't fix the crash should we back it out, or carry on.
Comment 15 Jonathan Watt [:jwatt] 2011-06-15 10:37:14 PDT
I'll look into this. Now that we have access to the PoC I can check what's going on for myself. For now please leave the committed patch in, since I believe it does fix a bad bug.
Comment 16 Jonathan Watt [:jwatt] 2011-06-15 10:59:52 PDT
*sigh* Or I would if I could extract the PoC, but I'm getting "Unable to unarchive 1 - Operation not permitted." Can someone attach the files for me please?
Comment 17 Jonathan Watt [:jwatt] 2011-06-15 11:01:47 PDT
Never mind. Got it unzipped on the command line.
Comment 18 Jonathan Watt [:jwatt] 2011-06-16 01:15:19 PDT
Created attachment 539746 [details] [diff] [review]
patch for 1.9.2
Comment 19 Jonathan Watt [:jwatt] 2011-06-16 01:31:25 PDT
Created attachment 539747 [details] [diff] [review]
patch for 1.9.1
Comment 20 Jonathan Watt [:jwatt] 2011-06-16 01:46:10 PDT
I've checked the PoC in both 1.9.2 and 1.9.1 with the patches in the previous two comments applied. The PoC is now successfully blocked by a JavaScript exception.

Basically what the patches that already landed on 1.9.2 and 1.9.1 (comment 8) do is block all attack vectors that I could find that are of the same type as the one detailed in comment 0...but somehow when writing those patches the one case I managed to fail to fix was the one responsible for the original vulnerability! That would be hilarious if this wasn't a security bug - grrr. - many thanks for doing some independent verification! Much appreciated!
Comment 21 Daniel Holbert [:dholbert] 2011-06-16 02:38:56 PDT
Comment on attachment 539746 [details] [diff] [review]
patch for 1.9.2

Are you sure we want to bump the
  *_retval = -1;
line up to the beginning here?

I thought XPCOM style was to generally leave outparams untouched on failure.  (In the already-landed patch, the outparam-setting made a bit more sense, because there the outparam was a pointer to be populated with a newly-constructed object, and it's a bit scary to leave that uninitialized. Here, the outparam is just an integer.  And our failure rv will trigger a JS exception anyway, so I don't think (?) the outparam could even be used in JS.)

In the interests of having the fix be as targeted as possible, I'd lean against that particular change (unless it's required to fix this, & I assume it's not?).   But it's also not a big deal, and I trust your judgement, so r=dholbert either way.
Comment 22 Daniel Holbert [:dholbert] 2011-06-16 02:41:11 PDT
Comment on attachment 539747 [details] [diff] [review]
patch for 1.9.1

r=dholbert, with the same hesitation about the "*_retval = -1;" chunks here.
Comment 23 Jonathan Watt [:jwatt] 2011-06-16 03:01:14 PDT
The "*_retval = -1;" is fair. I've pushed to the branches without that part.
Comment 24 Jan Horak 2011-06-16 03:02:04 PDT
Is this patch going to make it into 3.6.18?
Comment 25 Jonathan Watt [:jwatt] 2011-06-16 03:13:07 PDT
Setting resolution to fixed to put this back on the verification queue. - I don't know the answer to that, but someone else who does will still reply even with this bug closed.
Comment 26 Daniel Holbert [:dholbert] 2011-06-16 03:14:52 PDT
(I think jhorak's reopening was just the bugzilla-not-updating-flags-on-page-reload bug.)
Comment 27 Jonathan Watt [:jwatt] 2011-06-16 06:07:04 PDT
(In reply to comment #7)
> It only affects 1.9.2 and earlier.

Wrong! The holes I patched in the first patch only exist on 1.9.2 and earlier, true, but the patch from comment 23 is needed on trunk and all other active branches too.
Comment 28 Jonathan Watt [:jwatt] 2011-06-16 06:30:07 PDT
Created attachment 539783 [details] [diff] [review]
patch for 4.0, 5.0, 6.0, 7.0 and trunk

Daniel just went off on vacation, so requesting review from longsonr.
Comment 29 Jonathan Watt [:jwatt] 2011-06-16 06:52:38 PDT
Spun off bug 664708 so I can land this on trunk to cook so as to not hold up 5.0.
Comment 30 Jonathan Watt [:jwatt] 2011-06-16 08:01:25 PDT
Comment 31 Johnny Stenback (:jst, 2011-06-16 13:39:12 PDT
I doubt we'll take this for beta at this point, but we should consider this for aurora (6).
Comment 32 Daniel Veditz [:dveditz] 2011-06-16 13:55:34 PDT
If we do respin Fx5 for this we need to take it on the 3.6.18 _relbranch as well.
Comment 33 Jonathan Watt [:jwatt] 2011-06-16 14:09:52 PDT
FWIW this patch is about as close to zero risk as it gets, so if we respin for something else I'd suggest we take it.
Comment 34 Johnny Stenback (:jst, 2011-06-16 14:42:24 PDT
Comment on attachment 539783 [details] [diff] [review]
patch for 4.0, 5.0, 6.0, 7.0 and trunk

Approved for aurora, but minusing for beta, it's too late there. We're leaving this tracking for 5 so that we can reconsider this if we respin 5 for other reasons.
Comment 35 Jonathan Watt [:jwatt] 2011-06-17 05:06:29 PDT
Comment 36 Daniel Veditz [:dveditz] 2011-06-21 00:16:52 PDT
Comment on attachment 525550 [details] [diff] [review]

Adjusting approval flag to reflect actual release it was checked into.
Comment 37 Daniel Veditz [:dveditz] 2011-06-21 00:17:22 PDT
Comment on attachment 539783 [details] [diff] [review]
patch for 4.0, 5.0, 6.0, 7.0 and trunk

Approved for the mozilla2.0 repository, a=dveditz for release-drivers
Comment 38 Jonathan Watt [:jwatt] 2011-06-21 06:16:14 PDT

Resolving FIXED to put on the verification queue.
Comment 39 Jonathan Watt [:jwatt] 2011-06-21 06:18:24 PDT
jhorak: comment 36 answers your question - no, it will be in 3.6.19.
Comment 40 Al Billings [:abillings] 2011-07-26 16:25:41 PDT
Still fixed in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20110726 Namoroka/3.6.20pre.

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