Closed
Bug 502832
(CVE-2009-2662)
Opened 16 years ago
Closed 16 years ago
TM: Crash [@ memcpy]
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bc, Assigned: jorendorff)
References
()
Details
(Keywords: crash, verified1.9.1, Whiteboard: [sg:critical] [needs someone to continue to fully reduce the partial testcases])
Crash Data
Attachments
(3 files, 4 obsolete files)
|
3.22 KB,
patch
|
graydon
:
review+
samuel.sidler+old
:
approval1.9.1.2+
|
Details | Diff | Splinter Review |
|
7.74 KB,
text/plain
|
Details | |
|
419.11 KB,
text/html
|
Details |
see bug 502449 comment 3 and bug 502672. Filing new bug and making sensitive due to stacks etc.
start browser, load http://bonds.finam.ru/issue/info/ , close browser.
on windows only afaict.
on win2k3/1.9.1 build from this weekend with vc 8 express I got.
Exploitability Classification: EXPLOITABLE
Recommended Bug Title: Exploitable - User Mode Write AV starting at
MSVCR80D!memcpy+0x000000000000005a (Hash=0x1f2a4941.0x39393718)
User mode write access violations that are not near NULL are exploitable.
ChildEBP RetAddr
0012c558 004e80c2 js3250!js_Invoke+0x95c
0012c5a4 0050aeea js3250!js_fun_apply+0x2c2
0012ccd4 004f70cc js3250!js_Interpret+0x1179a
0012cdb0 004e80c2 js3250!js_Invoke+0x95c
0012cdfc 0050aeea js3250!js_fun_apply+0x2c2
0012d52c 004f70cc js3250!js_Interpret+0x1179a
0012d608 004e80c2 js3250!js_Invoke+0x95c
0012d654 0050aeea js3250!js_fun_apply+0x2c2
0012dd84 004f70cc js3250!js_Interpret+0x1179a
0012de60 012f6c57 js3250!js_Invoke+0x95c
with fresh builds on windows xp and vc 8 pro and 1.9.1/1.9.2 and tracemonkey
> msvcr80d.dll!memcpy(unsigned char * dst=0x05dc3bdc, unsigned char * src=0x0012b380, unsigned long count=4201) Line 188 Asm
js3250.dll!TraceRecorder::snapshot(ExitType exitType=TIMEOUT_EXIT) Line 2948 + 0x1a bytes C++
js3250.dll!TraceRecorder::TraceRecorder(JSContext * cx=0x04f99268, VMSideExit * _anchor=0x00000000, nanojit::Fragment * _fragment=0x06a7f958, TreeInfo * ti=0x06a8b4e8, unsigned int stackSlots=4201, unsigned int ngslots=0, JSTraceType_ * typeMap=0x06886068, VMSideExit * innermostNestedGuard=0x00000000, unsigned char * outer=0x00000000, unsigned long outerArgc=0) Line 1637 + 0xa bytes C++
js3250.dll!js_StartRecorder(JSContext * cx=0x04f99268, VMSideExit * anchor=0x00000000, nanojit::Fragment * f=0x06a7f958, TreeInfo * ti=0x06a8b4e8, unsigned int stackSlots=4201, unsigned int ngslots=0, JSTraceType_ * typeMap=0x06886068, VMSideExit * expectedInnerExit=0x00000000, unsigned char * outer=0x00000000, unsigned long outerArgc=0) Line 4140 + 0x4b bytes C++
js3250.dll!js_RecordTree(JSContext * cx=0x04f99268, JSTraceMonitor * tm=0x00d264c8, nanojit::Fragment * f=0x06a7f958, unsigned char * outer=0x00000000, unsigned long outerArgc=0, JSObject * globalObj=0x061f3be0, unsigned long globalShape=61629, Queue<unsigned short> * globalSlots=0x00d30758, unsigned long argc=4196) Line 4440 + 0x3c bytes C++
js3250.dll!js_MonitorLoopEdge(JSContext * cx=0x04f99268, unsigned int & inlineCallCount=0) Line 5517 + 0x28 bytes C++
js3250.dll!js_Interpret(JSContext * cx=0x04f99268) Line 3940 + 0x668 bytes C++
js3250.dll!js_Invoke(JSContext * cx=0x04f99268, unsigned int argc=4196, int * vp=0x0699c044, unsigned int flags=0) Line 1397 + 0x9 bytes C++
js3250.dll!js_fun_apply(JSContext * cx=0x04f99268, unsigned int argc=4196, int * vp=0x07e71514) Line 2080 + 0x13 bytes C++
js3250.dll!js_Interpret(JSContext * cx=0x04f99268) Line 5210 + 0x17 bytes C++
js3250.dll!js_Invoke(JSContext * cx=0x04f99268, unsigned int argc=1, int * vp=0x07e71118, unsigned int flags=0) Line 1397 + 0x9 bytes C++
js3250.dll!js_fun_apply(JSContext * cx=0x04f99268, unsigned int argc=1, int * vp=0x07e710e0) Line 2080 + 0x13 bytes C++
js3250.dll!js_Interpret(JSContext * cx=0x04f99268) Line 5210 + 0x17 bytes C++
js3250.dll!js_Invoke(JSContext * cx=0x04f99268, unsigned int argc=1, int * vp=0x07e710d4, unsigned int flags=0) Line 1397 + 0x9 bytes C++
js3250.dll!js_fun_apply(JSContext * cx=0x04f99268, unsigned int argc=1, int * vp=0x07e7109c) Line 2080 + 0x13 bytes C++
js3250.dll!js_Interpret(JSContext * cx=0x04f99268) Line 5210 + 0x17 bytes C++
js3250.dll!js_Invoke(JSContext * cx=0x04f99268, unsigned int argc=1, int * vp=0x07e7106c, unsigned int flags=0) Line 1397 + 0x9 bytes C++
js3250.dll!js_fun_apply(JSContext * cx=0x04f99268, unsigned int argc=1, int * vp=0x07e71034) Line 2080 + 0x13 bytes C++
js3250.dll!js_Interpret(JSContext * cx=0x04f99268) Line 5210 + 0x17 bytes C++
js3250.dll!js_Invoke(JSContext * cx=0x04f99268, unsigned int argc=1, int * vp=0x07e71028, unsigned int flags=0) Line 1397 + 0x9 bytes C++
Flags: blocking1.9.1.1?
| Reporter | ||
Comment 1•16 years ago
|
||
!exploitable says this second stack is exploitable as well.
Comment 2•16 years ago
|
||
Bob,
For what it's worth, I was able to reproduce this on Linux (Mozilla/5.0 X11; U; Linux; i686; en-US; rv:1.9.1) Gecko/20090624 Firefox 3.5) but not latest 1.9.2 trunk. So far not able to reproduce on Mac.
| Reporter | ||
Comment 3•16 years ago
|
||
Updated•16 years ago
|
OS: Windows XP → All
Comment 4•16 years ago
|
||
I was able to repro this once on http://bonds.finam.ru/trades/today/ on Ubuntu Linux with Fx 3.5 but I couldn't repro it again.
Comment 5•16 years ago
|
||
sayrer: Can we get an owner for this bug?
Updated•16 years ago
|
Assignee: general → jorendorff
Updated•16 years ago
|
blocking1.9.1: --- → needed
status1.9.1:
--- → wanted
Flags: wanted1.9.1.x+
Flags: blocking1.9.1.1?
Flags: blocking1.9.1.1-
Whiteboard: [sg:investigate]
| Assignee | ||
Comment 6•16 years ago
|
||
This one-line patch avoids crashing in memcpy. But the alloca call preceding this is just asking for crashes, too.
Graydon, do you already have a patch somewhere that eliminates alloca calls in jstracer? If so, I think we should land it. If not, I can patch just this one call.
Attachment #388497 -
Flags: review?(graydon)
| Assignee | ||
Comment 7•16 years ago
|
||
This version allocates that memory from cx->tempPool rather than alloca.
Attachment #388497 -
Attachment is obsolete: true
Attachment #388526 -
Flags: review?(graydon)
Attachment #388497 -
Flags: review?(graydon)
Comment 8•16 years ago
|
||
Any way to capture the crashing page and attach, or better, reduce first?
Whiteboard: [sg:investigate] → [sg:critical]
Comment 9•16 years ago
|
||
Comment on attachment 388526 [details] [diff] [review]
v2
Fine by me. That's the general thing I was doing with alloca()-squashing anyways.
Attachment #388526 -
Flags: review?(graydon) → review+
Comment 10•16 years ago
|
||
This is going to be an unpopular comment given yesterday but did you measure the performance impact?
| Assignee | ||
Comment 11•16 years ago
|
||
(In reply to comment #10)
> This is going to be an unpopular comment given yesterday but did you measure
> the performance impact?
bench.sh shows nothing significant, but it's rather noisy. I'll run SunSpider and v8 in a sec.
| Assignee | ||
Comment 12•16 years ago
|
||
SunSpider shows no change. On v8, I get "not conclusive: might be *1.007x as slow*", but every test is slower by a few tenths of a percent.
| Assignee | ||
Comment 13•16 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/e51fb83dbd64
I don't see how this crash could actually be exploited. Still, I'm not terribly comfortable having likely stack overruns in the product.
Graydon had the fix for this bug three months ago and we did not land it. The sentiment at the time was that the actual crash (bug 484693) had been fixed, and Graydon's patch removing the other alloca calls was an unnecessary perf loss.
Do we still feel that way?
Do we have some reason to believe the remaining alloca calls are safe? I took a look, and I'm not prepared to vouch for them.
I think the performance benefit of the remaining alloca calls is likely not worth the time it would take to check that they are safe. In other words, if we were to remove them (and I think we should), it would not be worth our time to write--and review with appropriate care--the patches to re-add them.
Comment 14•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 15•16 years ago
|
||
(In reply to comment #13)
> http://hg.mozilla.org/tracemonkey/rev/e51fb83dbd64
>
> I don't see how this crash could actually be exploited. Still, I'm not terribly
> comfortable having likely stack overruns in the product.
>
> Graydon had the fix for this bug three months ago and we did not land it. The
> sentiment at the time was that the actual crash (bug 484693) had been fixed,
> and Graydon's patch removing the other alloca calls was an unnecessary perf
> loss.
>
> Do we still feel that way?
>
> Do we have some reason to believe the remaining alloca calls are safe? I took a
> look, and I'm not prepared to vouch for them.
>
> I think the performance benefit of the remaining alloca calls is likely not
> worth the time it would take to check that they are safe. In other words, if we
> were to remove them (and I think we should), it would not be worth our time to
> write--and review with appropriate care--the patches to re-add them.
FWIW, I stumbled upon another test URL on 3.5.1
EXPLOITABLE: Exploitable - User Mode Write AV starting at MSVCR80D!memcpy+0x000000000000005a (Hash=0x1f2a4941.0x11133665): http://www.ylighting.com/kitchendining-allitems-price.html: EXIT STATUS: NORMAL (119.310000 seconds)
http://www.ylighting.com/kitchendining-allitems-price.html
Comment 16•16 years ago
|
||
(In reply to comment #13)
> Do we still feel that way?
Not I -- we shipped, we have a bit more time to be safer and fix any perf regressions.
/be
Comment 17•16 years ago
|
||
(In reply to comment #16)
> (In reply to comment #13)
> > Do we still feel that way?
>
> Not I -- we shipped, we have a bit more time to be safer and fix any perf
> regressions.
>
> /be
I'm thus guessing that some version of the patch here can be ported in time for 1.9.1.2 then, since it's now baking on m-c (so far for a day or so)
Comment 18•16 years ago
|
||
Please, please, please get a reduced testcase in this bug before we lose the live site.
Flags: in-testsuite?
| Reporter | ||
Comment 19•16 years ago
|
||
this reproduced the stack.
Comment 20•16 years ago
|
||
(In reply to comment #19)
> Created an attachment (id=390151) [details]
> unreduced testcase.tar.bz2
>
> this reproduced the stack.
I'm getting a crash with 3.5.1 debug build on Linux - way to go, Bob! I'm on it with Lithium now.
Comment 21•16 years ago
|
||
(In reply to comment #20)
> (In reply to comment #19)
> > Created an attachment (id=390151) [details] [details]
> > unreduced testcase.tar.bz2
> >
> > this reproduced the stack.
>
> I'm getting a crash with 3.5.1 debug build on Linux - way to go, Bob! I'm on it
> with Lithium now.
CC'ing Jesse too.
| Reporter | ||
Comment 22•16 years ago
|
||
Gary, Thanks. I've been trying to reduce it on WinXp and have been having a hard time. Did you get the same stack? We need to make sure the reduced test reproduces the exact stack.
Comment 23•16 years ago
|
||
I must admit too, even with your testcase, it's highly unreliable - I need to execute a random set of actions on the webpage to get it to crash. Still working on it.
Comment 24•16 years ago
|
||
This half-localized ~10k-liner crashes Linux reliably without random actions - I wrote in a setTimeout auto-reload to get it to crash. (Thanks gal)
Attachment #390151 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #390190 -
Attachment description: localized testcase off bc's → half-localized testcase off bc's
Comment 25•16 years ago
|
||
(In reply to comment #24)
> Created an attachment (id=390190) [details]
> localized testcase off bc's
Remember to have a local copy and change the path at the top "window.location" variable to reflect the path on your local copy. (I'm on Ubuntu 9.04 btw) I am unable to reproduce this crash on Mac 10.5 so far.
Comment 26•16 years ago
|
||
I'm now tearing my hair out at localizing/reducing this - it's still not fully localized yet.
Remember to have a local copy then change the window.location path at the top of the testcase to your location.
(Any suggestions to help is greatly appreciated!)
Attachment #390190 -
Attachment is obsolete: true
Comment 27•16 years ago
|
||
Someone else please feel free to take over - that's all for my day. :-(
Attachment #390205 -
Attachment is obsolete: true
Updated•16 years ago
|
Whiteboard: [sg:critical] → [sg:critical] [needs someone to continue to fully reduce the partial testcases]
Comment 29•16 years ago
|
||
Can we get this fix into 3.5.2?
| Assignee | ||
Comment 30•16 years ago
|
||
Comment on attachment 388526 [details] [diff] [review]
v2
Sure. I have this sitting in a patch queue ready to push to mozilla-1.9.1 actually.
Attachment #388526 -
Flags: approval1.9.1.2?
Comment 31•16 years ago
|
||
Comment on attachment 388526 [details] [diff] [review]
v2
Approved for 1.9.1.2. a=ss for release-drivers.
Please land and use the ".2-fixed" option of the status1.9.1 flag to indicate fixed status.
Attachment #388526 -
Flags: approval1.9.1.2? → approval1.9.1.2+
| Assignee | ||
Comment 32•16 years ago
|
||
Updated•16 years ago
|
Group: core-security
Flags: wanted1.9.0.x-
Updated•16 years ago
|
Alias: CVE-2009-2662
Updated•16 years ago
|
blocking1.9.1: needed → .2+
Updated•14 years ago
|
Crash Signature: [@ memcpy]
Comment 36•12 years ago
|
||
Filter on qa-project-auto-change:
Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite? → in-testsuite-
Comment 37•11 years ago
|
||
Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task
Keywords: qawanted,
testcase-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•