589103 - Crash [@ JS_GetGlobalForScopeChain] or [@ js::JSScriptedProxyHandler::has]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

print(__proto__ = Proxy.create(this, ""))

crashes js debug shell on TM changeset b22e82ce2364 without -j at JS_GetGlobalForScopeChain and crashes js opt shell without -j at js::JSScriptedProxyHandler::has

Seems to be a recursive overflow... Backtrace is very long. s-s just to be safe.

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0xbf7fffdc
0x00014466 in JS_GetGlobalForScopeChain (cx=0x50a3a0) at ../jsapi.cpp:1813
1813    JS_GetGlobalForScopeChain(JSContext *cx)
(gdb) x/i $eip
0x14466 <JS_GetGlobalForScopeChain+7>:  call   0x1446b <JS_GetGlobalForScopeChain+12>

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0xbf7ffffc
0x000c587c in js::JSScriptedProxyHandler::has ()
(gdb) x/i $eip
0xc587c <_ZN2js22JSScriptedProxyHandler3hasEP9JSContextP8JSObjectiPb+12>:       call   0xc5881 <_ZN2js22JSScriptedProxyHandler3hasEP9JSContextP8JSObjectiPb+17>

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   42738:6ca8580eb84f
user:        Andreas Gal
date:        Thu May 27 12:03:25 2010 -0700
summary:     Implement iterate trap for proxy handlers (568413, r=brendan).

Comment 2

[Andreas Gal :gal]

This should block.

Comment 3

[Andreas Gal :gal]

sayrer, can I do this after the feature freeze? (probably trivial to fix)

Comment 4

[Brendan Eich [:brendan]]

This is a stack overflow -- not exploitable AFAIK on our supported OSes.

The general problem is a loop of the form

  proxy --[handler]--> object --[proto]--+
    ^                                    |
    +------------------------------------+

This shows an easily fixed failure to JS_CHECK_RECURSION in GetTrap. Patch next.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: fix, plus naming sanity

GetTrap has wrappers but they confusingly lack the "Get-" prefix, so are named as if they wrap Trap, which actually calls the trap. They do not, they just get and validate certain kinds of traps (Fundamental, Derived). So I prefixed.

/be

Comment 6

[Andreas Gal :gal]

Comment on attachment 480521 [details] [diff] [review]
fix, plus naming sanity

Does this remove the need for some of the other recursion checks?

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

In bug 600139, Igor says "stack overflow is exploitable on some platforms" -- that surprised me, but I'm the first to admit that I'm not au courant on all the latest exploit techniques.  re-closing and copying igor for more detail or references on such exploitation!

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Comment on attachment 480521 [details] [diff] [review]
> fix, plus naming sanity
> 
> Does this remove the need for some of the other recursion checks?

There was only one other one, in Trap (which calls a trap). It may not be necessary since js::Interpret has one, but IIRC it was necessary anyway... yes, see bug 566790, where you can make a small recursive loop through native-only code.

We don't check recursion under native lookup-property, but we could. So far, __proto__ cycle detection has been enough in practice, although someone mean (i.e., a fuzzer :-) could link a bunch of unrelated objects together into a very long list. So an alternative patch would check in js_LookupPropertyWithFlags and any other such object-op. Comments?

/be

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #7)
> In bug 600139, Igor says "stack overflow is exploitable on some platforms"

That was about GC stack quota, where live GC-things may not be marked. It was not about general stack overflow, IIUC.

In the past, with a non-standard embedding on Windows using SEH, or on a buggy Linux kernel variant, stack overflow was demonstrated to be exploitable. Those are not supported platforms, AFAIK.

I hate to see stack overflow bugs debase the sg:crit coin, so I hope someone (Jesse, maybe) can adjudicate and update this bug again if appropriate.

/be

Comment 10

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/760b8a9c9a34

/be

Comment 11

[Igor Bukanov]

(In reply to comment #7)
> In bug 600139, Igor says "stack overflow is exploitable on some platforms" --

On Linux the defense against stack overflow is a single 4K guard page. To state that stack overflow is harmless on Linux one has to show that it is not possible to push more than 4K of uninitialized data on the stack near the stack limit. Do we  monitor that FF codebase does not contain something like char buffer[10*1000] that is not used immediately? And of cause any use of alloca or other variable-stack allocations must be watched (jstracer.cpp still uses alloca for example).

Comment 12

[Jesse Ruderman]

It's easier to catch all of those than to catch all too-much-recursion avenues, IMO.  Want to file a static analysis bug blocking bug 430328?

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> It's easier to catch all of those than to catch all too-much-recursion avenues,
> IMO.  Want to file a static analysis bug blocking bug 430328?

See bug 601522.

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/760b8a9c9a34

Comment 15

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929