Closed Bug 648094 (CVE-2011-0084) Opened 13 years ago Closed 13 years ago

Mozilla Firefox SVGTextElement.getCharNumAtPosition Remote Code Execution Vulnerability (ZDI-CAN-1143)

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox5 - wontfix
firefox6 + fixed
firefox7 + fixed
blocking2.0 --- .x+
status2.0 --- .x-fixed
blocking1.9.2 --- .20+
status1.9.2 --- .20-fixed
status1.9.1 --- wanted

People

(Reporter: bsterne, Assigned: jwatt)

References

Details

(Keywords: verified1.9.2, Whiteboard: [sg:critical?])

Attachments

(5 files)

Attached file PoC (obsolete) —
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
Assignee: nobody → jwatt
Attached patch patchSplinter Review
Attachment #525550 - Flags: review?(dholbert)
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/
Attachment #525550 - Flags: review?(dholbert) → review+
Attachment #525550 - Flags: approval1.9.2.17?
blocking1.9.1: --- → .20+
blocking1.9.2: --- → .18+
Attachment #525550 - Flags: approval1.9.2.17? → approval1.9.2.18?
Is this a branch-only problem or does it affect trunk as well?
Whiteboard: [sg:critical?]
(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.
blocking1.9.1: .20+ → ---
Comment on attachment 525550 [details] [diff] [review]
patch

Approved for 1.9.2.18, a=dveditz for release-drivers
Attachment #525550 - Flags: approval1.9.2.18? → approval1.9.2.18+
Seems like the POC has had the exploit code stripped by Exchange Server (so says the contents of "poc..txt").
(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.
Pushed http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a58068a05d9a
Pushed http://hg.mozilla.org/releases/mozilla-1.9.1/rev/2acbfab37f69
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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.
Attached file Correct PoC
Password is ZDI-CAN-1143
Attached file 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Al, did you ever get around to verifying this?
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
If the checked-in patch doesn't fix the crash should we back it out, or carry on.
blocking1.9.2: .18+ → .19+
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.
*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?
Never mind. Got it unzipped on the command line.
Attached patch patch for 1.9.2Splinter Review
Attachment #539746 - Flags: review?(dholbert)
Attached patch patch for 1.9.1Splinter Review
Attachment #539747 - Flags: review?(dholbert)
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 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.
Attachment #539746 - Flags: review?(dholbert) → review+
Comment on attachment 539747 [details] [diff] [review]
patch for 1.9.1

r=dholbert, with the same hesitation about the "*_retval = -1;" chunks here.
Attachment #539747 - Flags: review?(dholbert) → review+
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
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
Is this patch going to make it into 3.6.18?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
(I think jhorak's reopening was just the bugzilla-not-updating-flags-on-page-reload bug.)
Attachment #525550 - Flags: checkin+
Attachment #539746 - Flags: checkin+
Attachment #539747 - Flags: checkin+
(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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Version: 1.9.2 Branch → Trunk
blocking2.0: --- → .x+
tracking-fennec: --- → ?
status2.0: --- → wanted
Daniel just went off on vacation, so requesting review from longsonr.
Attachment #539783 - Flags: review?
Attachment #539783 - Flags: review? → review?(longsonr)
Attachment #539783 - Flags: approval-mozilla-beta?
Attachment #539783 - Flags: approval-mozilla-aurora?
Attachment #539783 - Flags: review?(longsonr) → review+
Spun off bug 664708 so I can land this on trunk to cook so as to not hold up 5.0.
I doubt we'll take this for beta at this point, but we should consider this for aurora (6).
If we do respin Fx5 for this we need to take it on the 3.6.18 _relbranch as well.
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 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.
Attachment #539783 - Flags: approval-mozilla-beta?
Attachment #539783 - Flags: approval-mozilla-beta-
Attachment #539783 - Flags: approval-mozilla-aurora?
Attachment #539783 - Flags: approval-mozilla-aurora+
Attachment #539783 - Flags: approval2.0?
Alias: CVE-2011-0084
Comment on attachment 525550 [details] [diff] [review]
patch

Adjusting approval flag to reflect actual release it was checked into.
Attachment #525550 - Flags: approval1.9.2.18+ → approval1.9.2.19+
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
Attachment #539783 - Flags: approval2.0? → approval2.0+
Pushed http://hg.mozilla.org/releases/mozilla-2.0/rev/f5b873872bcd

Resolving FIXED to put on the verification queue.
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
jhorak: comment 36 answers your question - no, it will be in 3.6.19.
tracking-fennec: ? → ---
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.
Keywords: verified1.9.2
Group: core-security
You need to log in before you can comment on or make changes to this bug.