Bug 648094 (CVE-2011-0084)

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

RESOLVED FIXED

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bsterne, Assigned: jwatt)

Tracking

({verified1.9.2})

Trunk
verified1.9.2
Points:
---

Firefox Tracking Flags

(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)

Details

(Whiteboard: [sg:critical?])

Attachments

(5 attachments)

(Reporter)

Description

7 years ago
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
(Assignee)

Updated

6 years ago
Assignee: nobody → jwatt
(Assignee)

Comment 1

6 years ago
Created attachment 525550 [details] [diff] [review]
patch
Attachment #525550 - Flags: review?(dholbert)
(Assignee)

Updated

6 years ago
Blocks: 648160
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+
(Assignee)

Updated

6 years ago
Attachment #525550 - Flags: approval1.9.2.17?

Updated

6 years ago
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?
status1.9.1: --- → wanted
status1.9.2: --- → wanted
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+ → ---
status2.0: --- → unaffected
status-firefox5: --- → unaffected
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+
(Assignee)

Comment 6

6 years ago
Seems like the POC has had the exploit code stripped by Exchange Server (so says the contents of "poc..txt").
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED
status1.9.1: wanted → .20-fixed
status1.9.2: wanted → .18-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.
(Reporter)

Comment 10

6 years ago
Created attachment 538061 [details]
Correct PoC

Password is ZDI-CAN-1143

Comment 11

6 years ago
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.

Updated

6 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 12

6 years ago
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.
status1.9.1: .20-fixed → wanted
status1.9.2: .18-fixed → wanted
blocking1.9.2: .18+ → .19+
(Assignee)

Comment 15

6 years ago
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.
(Assignee)

Comment 16

6 years ago
*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?
(Assignee)

Comment 17

6 years ago
Never mind. Got it unzipped on the command line.
(Assignee)

Comment 18

6 years ago
Created attachment 539746 [details] [diff] [review]
patch for 1.9.2
Attachment #539746 - Flags: review?(dholbert)
(Assignee)

Comment 19

6 years ago
Created attachment 539747 [details] [diff] [review]
patch for 1.9.1
Attachment #539747 - Flags: review?(dholbert)
(Assignee)

Comment 20

6 years ago
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+
(Assignee)

Comment 23

6 years ago
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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED

Comment 24

6 years ago
Is this patch going to make it into 3.6.18?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 25

6 years ago
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
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(I think jhorak's reopening was just the bugzilla-not-updating-flags-on-page-reload bug.)
(Assignee)

Updated

6 years ago
Attachment #525550 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #539746 - Flags: checkin+
(Assignee)

Updated

6 years ago
Attachment #539747 - Flags: checkin+
(Assignee)

Comment 27

6 years ago
(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
status2.0: unaffected → ---
status-firefox5: unaffected → ---
Resolution: FIXED → ---
Version: 1.9.2 Branch → Trunk
(Assignee)

Updated

6 years ago
blocking2.0: --- → .x+
tracking-fennec: --- → ?
status2.0: --- → wanted
status-firefox5: --- → affected
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox5: --- → +
tracking-firefox6: --- → ?
tracking-firefox7: --- → ?
(Assignee)

Comment 28

6 years ago
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.
Attachment #539783 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #539783 - Flags: review? → review?(longsonr)
(Assignee)

Updated

6 years ago
Attachment #539783 - Flags: approval-mozilla-beta?
Attachment #539783 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #539783 - Flags: review?(longsonr) → review+
(Assignee)

Comment 29

6 years ago
Spun off bug 664708 so I can land this on trunk to cook so as to not hold up 5.0.
(Assignee)

Comment 30

6 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/f6baa2c0d4f4
I doubt we'll take this for beta at this point, but we should consider this for aurora (6).
status1.9.2: wanted → .19-fixed
status-firefox7: affected → fixed
tracking-firefox6: ? → +
tracking-firefox7: ? → +
If we do respin Fx5 for this we need to take it on the 3.6.18 _relbranch as well.
(Assignee)

Comment 33

6 years ago
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+
(Assignee)

Comment 35

6 years ago
Pushed http://hg.mozilla.org/releases/mozilla-aurora/rev/c97c4dd25158
status-firefox6: affected → fixed
(Assignee)

Updated

6 years ago
Attachment #539783 - Flags: approval2.0?
Alias: CVE-2011-0084
tracking-firefox5: + → -
status-firefox5: affected → wontfix
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+
(Assignee)

Comment 38

6 years ago
Pushed http://hg.mozilla.org/releases/mozilla-2.0/rev/f5b873872bcd

Resolving FIXED to put on the verification queue.
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 39

6 years ago
jhorak: comment 36 answers your question - no, it will be in 3.6.19.
(Assignee)

Updated

6 years ago
status2.0: wanted → .x-fixed
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.