Closed Bug 502832 (CVE-2009-2662) Opened 16 years ago Closed 16 years ago

TM: Crash [@ memcpy]

Categories

(Core :: JavaScript Engine, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking1.9.1 --- .2+
status1.9.1 --- .2-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)

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?
!exploitable says this second stack is exploitable as well.
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.
OS: Windows XP → All
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.
sayrer: Can we get an owner for this bug?
Assignee: general → jorendorff
blocking1.9.1: --- → needed
Flags: wanted1.9.1.x+
Flags: blocking1.9.1.1?
Flags: blocking1.9.1.1-
Whiteboard: [sg:investigate]
Attached patch v1 (obsolete) — Splinter Review
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)
Attached patch v2Splinter Review
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)
Any way to capture the crashing page and attach, or better, reduce first?
Whiteboard: [sg:investigate] → [sg:critical]
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+
This is going to be an unpopular comment given yesterday but did you measure the performance impact?
(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.
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.
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.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
(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
(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
(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)
Please, please, please get a reduced testcase in this bug before we lose the live site.
Flags: in-testsuite?
Keywords: qawanted
Attached file unreduced testcase.tar.bz2 (obsolete) —
this reproduced the stack.
(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.
(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.
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.
Attached file Linux stack
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.
Attached file half-localized testcase off bc's (obsolete) —
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
Attachment #390190 - Attachment description: localized testcase off bc's → half-localized testcase off bc's
(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.
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
Someone else please feel free to take over - that's all for my day. :-(
Attachment #390205 - Attachment is obsolete: true
Whiteboard: [sg:critical] → [sg:critical] [needs someone to continue to fully reduce the partial testcases]
Can we get this fix into 3.5.2?
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 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+
v 1.9.1 mac/win/linux
Keywords: verified1.9.1
Group: core-security
Flags: wanted1.9.0.x-
Alias: CVE-2009-2662
blocking1.9.1: needed → .2+
Depends on: 521880
Crash Signature: [@ memcpy]
Filter on qa-project-auto-change: Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite? → in-testsuite-
Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: