Last Comment Bug 648094 - (CVE-2011-0084) Mozilla Firefox SVGTextElement.getCharNumAtPosition Remote Code Execution Vulnerability (ZDI-CAN-1143)
(CVE-2011-0084)
: Mozilla Firefox SVGTextElement.getCharNumAtPosition Remote Code Execution Vul...
Status: RESOLVED FIXED
[sg:critical?]
: verified1.9.2
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Jonathan Watt [:jwatt] (back in October - email directly if necessary)
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
+
fixed
+
fixed
.x+
.x-fixed
.20+
.20-fixed
wanted


Attachments
patch (3.49 KB, patch)
2011-04-12 16:43 PDT, Jonathan Watt [:jwatt] (back in October - email directly if necessary)
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] (back in October - email directly if necessary)
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] (back in October - email directly if necessary)
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] (back in October - email directly if necessary)
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]
PoC

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:

NS_IMETHODIMP
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;
  point->GetX(&xPos);
  point->GetY(&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),
    mInError(PR_FALSE)
{
  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
CTM)
  const nsStyleFont* fontData = GetStyleFont();

  ...
}

And here we reach a dangling pointer.

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

This vulnerability was discovered by:
    * regenrecht
Comment 1 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-04-12 16:43:21 PDT
Created attachment 525550 [details] [diff] [review]
patch
Comment 2 Daniel Holbert [:dholbert] 2011-04-12 17:01:33 PDT
Comment on attachment 525550 [details] [diff] [review]
patch

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);   \

s/path/point/
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]
patch

Approved for 1.9.2.18, a=dveditz for release-drivers
Comment 6 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 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] (back in October - email directly if necessary) 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 8 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-05-08 10:41:02 PDT
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a58068a05d9a
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2acbfab37f69
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").

Exactly.

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 1.9.2.18 build 1.

1.9.2.17 crash: https://crash-stats.mozilla.com/report/index/a5645969-15b8-4c3e-8756-132eb2110615

1.9.2.18 crash: https://crash-stats.mozilla.com/report/index/42292d43-ac1b-47e7-af78-81adf2110615
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] (back in October - email directly if necessary) 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] (back in October - email directly if necessary) 2011-06-15 10:59:52 PDT
*sigh* Or I would if I could extract the PoC, but I'm getting "Unable to unarchive poc.zip...Error 1 - Operation not permitted." Can someone attach the files for me please?
Comment 17 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-06-15 11:01:47 PDT
Never mind. Got it unzipped on the command line.
Comment 18 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-06-16 01:15:19 PDT
Created attachment 539746 [details] [diff] [review]
patch for 1.9.2
Comment 19 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 2011-06-16 01:31:25 PDT
Created attachment 539747 [details] [diff] [review]
patch for 1.9.1
Comment 20 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 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.

jhorak@redhat.com - 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] (back in October - email directly if necessary) 2011-06-16 03:01:14 PDT
The "*_retval = -1;" is fair. I've pushed to the branches without that part.

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d4d366ac8d9b
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0efb6d76d419
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] (back in October - email directly if necessary) 2011-06-16 03:13:07 PDT
Setting resolution to fixed to put this back on the verification queue.

jhorak@redhat.com - 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] (back in October - email directly if necessary) 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] (back in October - email directly if necessary) 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] (back in October - email directly if necessary) 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] (back in October - email directly if necessary) 2011-06-16 08:01:25 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/f6baa2c0d4f4
Comment 31 Johnny Stenback (:jst, jst@mozilla.com) 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] (back in October - email directly if necessary) 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, jst@mozilla.com) 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] (back in October - email directly if necessary) 2011-06-17 05:06:29 PDT
Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/c97c4dd25158
Comment 36 Daniel Veditz [:dveditz] 2011-06-21 00:16:52 PDT
Comment on attachment 525550 [details] [diff] [review]
patch

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] (back in October - email directly if necessary) 2011-06-21 06:16:14 PDT
Pushed http://hg.mozilla.org/releases/mozilla-2.0/rev/f5b873872bcd

Resolving FIXED to put on the verification queue.
Comment 39 Jonathan Watt [:jwatt] (back in October - email directly if necessary) 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 1.9.2.20: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.20pre) Gecko/20110726 Namoroka/3.6.20pre.

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