507292 - Incorrect upvar access on trace involving top-level scripts

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Carsten Book [:Tomcat]]

BuildID=20090727101705

Steps to reproduce: Load http://www.warcraftlatino.com/ulduar/rank.html

(c20.bb4): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=04859130 ebx=7ffdf000 ecx=072a0410 edx=c0000005 esi=0000000f edi=00c8f6f0
eip=00519bd4 esp=0012e718 ebp=0012ee44 iopl=0         nv up ei pl nz na pe nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010206
js3250!js_Interpret+0x103b4:
00519bd4 8b02            mov     eax,dword ptr [edx]  ds:0023:c0000005=????????
0:000> !exploitable -v
HostMachine\HostUser
Executing Processor Architecture is x86
Debuggee is in User Mode
Debuggee is a live user mode debugging session on the local machine
Event Type: Exception

Exception Faulting Address: 0xffffffffc0000005
First Chance Exception Type: STATUS_ACCESS_VIOLATION (0xC0000005)
Exception Sub-Type: Read Access Violation

Faulting Instruction:00519bd4 mov eax,dword ptr [edx]

Basic Block:
    00519bd4 mov eax,dword ptr [edx]
       Tainted Input Operands: edx
    00519bd6 mov ecx,dword ptr [eax+0ch]
       Tainted Input Operands: eax
    00519bd9 call ecx
       Tainted Input Operands: ecx, edx

Exception Hash (Major/Minor): 0x2a690d26.0x1548251d

Stack Trace:
js3250!js_Interpret+0x103b4
js3250!js_Execute+0x2e6
js3250!JS_EvaluateUCScriptForPrincipals+0xe7
gklayout!nsJSContext::EvaluateString+0x2c0
gklayout!nsScriptLoader::EvaluateScript+0x37a
gklayout!nsScriptLoader::ProcessRequest+0xfd
gklayout!nsScriptLoader::ProcessScriptElement+0x1040
gklayout!nsScriptElement::MaybeProcessScript+0x149
gklayout!nsHTMLScriptElement::MaybeProcessScript+0x24
gklayout!nsHTMLScriptElement::DoneAddingChildren+0x1f
gklayout!HTMLContentSink::ProcessSCRIPTEndTag+0xcf
gklayout!SinkContext::CloseContainer+0x31d
gklayout!HTMLContentSink::CloseContainer+0xa0
gkparser!CNavDTD::CloseContainer+0x18a
gkparser!CNavDTD::HandleEndToken+0x214
gkparser!CNavDTD::HandleToken+0x4ae
gkparser!CNavDTD::BuildModel+0x298
gkparser!nsParser::BuildModel+0xe2
gkparser!nsParser::ResumeParse+0x1bc
gkparser!nsParser::ContinueInterruptedParsing+0xe5
gklayout!nsContentSink::ContinueInterruptedParsingIfEnabled+0x54
gklayout!nsRunnableMethod<nsContentSink>::Run+0x24
xpcom_core!nsThread::ProcessNextEvent+0x1fa
xpcom_core!NS_ProcessNextEvent_P+0x53
gkwidget!nsBaseAppShell::Run+0x5d
tkitcmps!nsAppStartup::Run+0x6b
xul!XRE_main+0x3000
firefox!NS_internal_main+0x2b2
firefox!wmain+0x119
firefox!__tmainCRTStartup+0x1a6
firefox!wmainCRTStartup+0xd
kernel32!BaseProcessStart+0x23
Instruction Address: 0x0000000000519bd4

Description: Data from Faulting Address controls Code Flow
Short Description: TaintedDataControlsCodeFlow
Exploitability Classification: PROBABLY_EXPLOITABLE
Recommended Bug Title: Probably Exploitable - Data from Faulting Address controls Code Flow starting at js3250!js_Interpret+0x00000000000103b4 (Hash=0x2a690d26.0x1548251d)

The data from the faulting address is later used as the target for a branch.

Comment 1

[Carsten Book [:Tomcat]]

stack: 
js3250!js_Interpret(struct JSContext * cx = 0x04859130)+0x103b4 [c:\work\mozilla\builds\1.9.1\mozilla\js\src\jsinterp.cpp @ 4838]
js3250!js_Execute(struct JSContext * cx = 0x04859130, struct JSObject * chain = 0x072806c0, struct JSScript * script = 0x05d15bf0, struct JSStackFrame * down = 0x00000000, unsigned int flags = 0, int * result = 0x00000000)+0x2e6 [c:\work\mozilla\builds\1.9.1\mozilla\js\src\jsinterp.cpp @ 1622]
js3250!JS_EvaluateUCScriptForPrincipals(struct JSContext * cx = 0x04859130, struct JSObject * obj = 0x072806c0, struct JSPrincipals * principals = 0x05cb7e0c, unsigned short * chars = 0x05d0e358, unsigned int length = 0x7a8, char * filename = 0x05c72720 "http://www.warcraftlatino.com/ulduar/rank.html", unsigned int lineno = 0xf, int * rval = 0x00000000)+0xe7 [c:\work\mozilla\builds\1.9.1\mozilla\js\src\jsapi.cpp @ 5145]
gklayout!nsJSContext::EvaluateString(class nsAString_internal * aScript = 0x0012f0bc, void * aScopeObject = 0x072806c0, class nsIPrincipal * aPrincipal = 0x05cb7e08, char * aURL = 0x05c72720 "http://www.warcraftlatino.com/ulduar/rank.html", unsigned int aLineNo = 0xf, unsigned int aVersion = 0, class nsAString_internal * aRetValue = 0x00000000, int * aIsUndefined = 0x0012f07c)+0x2c0 [c:\work\mozilla\builds\1.9.1\mozilla\dom\src\base\nsjsenvironment.cpp @ 1631]
gklayout!nsScriptLoader::EvaluateScript(class nsScriptLoadRequest * aRequest = 0x05d0e2e0, class nsString * aScript = 0x0012f0bc)+0x37a [c:\work\mozilla\builds\1.9.1\mozilla\content\base\src\nsscriptloader.cpp @ 686]
gklayout!nsScriptLoader::ProcessRequest(class nsScriptLoadRequest * aRequest = 0x05d0e2e0)+0xfd [c:\work\mozilla\builds\1.9.1\mozilla\content\base\src\nsscriptloader.cpp @ 600]
gklayout!nsScriptLoader::ProcessScriptElement(class nsIScriptElement * aElement = 0x05d0d844)+0x1040 [c:\work\mozilla\builds\1.9.1\mozilla\content\base\src\nsscriptloader.cpp @ 554]
gklayout!nsScriptElement::MaybeProcessScript(void)+0x149 [c:\work\mozilla\builds\1.9.1\mozilla\content\base\src\nsscriptelement.cpp @ 193]
gklayout!nsHTMLScriptElement::MaybeProcessScript(void)+0x24 [c:\work\mozilla\builds\1.9.1\mozilla\content\html\content\src\nshtmlscriptelement.cpp @ 546]
gklayout!nsHTMLScriptElement::DoneAddingChildren(int aHaveNotified = 1)+0x1f [c:\work\mozilla\builds\1.9.1\mozilla\content\html\content\src\nshtmlscriptelement.cpp @ 484]
gklayout!HTMLContentSink::ProcessSCRIPTEndTag(class nsGenericHTMLElement * content = 0x05d0d820, int aMalformed = 0)+0xcf [c:\work\mozilla\builds\1.9.1\mozilla\content\html\document\src\nshtmlcontentsink.cpp @ 3145]
gklayout!SinkContext::CloseContainer(nsHTMLTag aTag = eHTMLTag_script (83), int aMalformed = 0)+0x31d [c:\work\mozilla\builds\1.9.1\mozilla\content\html\document\src\nshtmlcontentsink.cpp @ 1022]
gklayout!HTMLContentSink::CloseContainer(nsHTMLTag aTag = eHTMLTag_script (83))+0xa0 [c:\work\mozilla\builds\1.9.1\mozilla\content\html\document\src\nshtmlcontentsink.cpp @ 2396]
gkparser!CNavDTD::CloseContainer(nsHTMLTag aTag = eHTMLTag_script (83), int aMalformed = 0)+0x18a [c:\work\mozilla\builds\1.9.1\mozilla\parser\htmlparser\src\cnavdtd.cpp @ 2804]
gkparser!CNavDTD::HandleEndToken(class CToken * aToken = 0x05c7f6f0)+0x214 [c:\work\mozilla\builds\1.9.1\mozilla\parser\htmlparser\src\cnavdtd.cpp @ 1683]
gkparser!CNavDTD::HandleToken(class CToken * aToken = 0x05c7f6f0, class nsIParser * aParser = 0x05cb8578)+0x4ae [c:\work\mozilla\builds\1.9.1\mozilla\parser\htmlparser\src\cnavdtd.cpp @ 760]
gkparser!CNavDTD::BuildModel(class nsIParser * aParser = 0x05cb8578, class nsITokenizer * aTokenizer = 0x05cd4c70, class nsITokenObserver * anObserver = 0x00000000, class nsIContentSink * aSink = 0x05cbaf9c)+0x298 [c:\work\mozilla\builds\1.9.1\mozilla\parser\htmlparser\src\cnavdtd.cpp @ 332]
gkparser!nsParser::BuildModel(void)+0xe2 [c:\work\mozilla\builds\1.9.1\mozilla\parser\htmlparser\src\nsparser.cpp @ 2400]
gkparser!nsParser::ResumeParse(int allowIteration = 1, int aIsFinalChunk = 1, int aCanInterrupt = 1)+0x1bc [c:\work\mozilla\builds\1.9.1\mozilla\parser\htmlparser\src\nsparser.cpp @ 2273]
gkparser!nsParser::ContinueInterruptedParsing(void)+0xe5 [c:\work\mozilla\builds\1.9.1\mozilla\parser\htmlparser\src\nsparser.cpp @ 1773]

Comment 2

[Carsten Book [:Tomcat]]

FAULTING_IP: 
js3250!js_Interpret+103b4 [c:\work\mozilla\builds\1.9.1\mozilla\js\src\jsinterp.cpp @ 4838]
00519bd4 8b02            mov     eax,dword ptr [edx]

EXCEPTION_RECORD:  ffffffff -- (.exr ffffffffffffffff)
ExceptionAddress: 00519bd4 (js3250!js_Interpret+0x000103b4)
   ExceptionCode: c0000005 (Access violation)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000000
   Parameter[1]: c0000005
Attempt to read from address c0000005

FAULTING_THREAD:  00000bb4

DEFAULT_BUCKET_ID:  APPLICATION_FAULT

PROCESS_NAME:  firefox.exe

ERROR_CODE: (NTSTATUS) 0xc0000005 - The instruction at "0x%08lx" referenced memory at "0x%08lx". The memory could not be "%s".

READ_ADDRESS:  c0000005 

BUGCHECK_STR:  ACCESS_VIOLATION

LAST_CONTROL_TRANSFER:  from 005080e6 to 00519bd4

FOLLOWUP_IP: 
js3250!js_Interpret+103b4 [c:\work\mozilla\builds\1.9.1\mozilla\js\src\jsinterp.cpp @ 4838]
00519bd4 8b02            mov     eax,dword ptr [edx]

FAULTING_SOURCE_CODE:  
  4834:                 if (!js_InternNonIntElementId(cx, obj, rval, &id))
  4835:                     goto error;
  4836:             }
  4837: 
> 4838:             if (!OBJ_GET_PROPERTY(cx, obj, id, &rval))
  4839:                 goto error;
  4840:           end_getelem:
  4841:             regs.sp--;
  4842:             STORE_OPND(-1, rval);
  4843:           END_CASE(JSOP_GETELEM)


SYMBOL_STACK_INDEX:  0

FOLLOWUP_NAME:  MachineOwner

MODULE_NAME: js3250

IMAGE_NAME:  js3250.dll

DEBUG_FLR_IMAGE_TIMESTAMP:  4a6de171

SYMBOL_NAME:  js3250!js_Interpret+103b4

STACK_COMMAND:  ~0s ; kb

FAILURE_BUCKET_ID:  ACCESS_VIOLATION_js3250!js_Interpret+103b4

BUCKET_ID:  ACCESS_VIOLATION_js3250!js_Interpret+103b4

Comment 3

[Bob Clary [:bc] (inactive)]

crashes mac 1.9.1/1.9.2 debug 

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0xc0000005
0x002fbaee in js_Interpret (cx=0x1441200) at /work/mozilla/builds/1.9.1/mozilla/js/src/jsinterp.cpp:4838
4838	            if (!OBJ_GET_PROPERTY(cx, obj, id, &rval))
(gdb) bt
#0  0x002fbaee in js_Interpret (cx=0x1441200) at /work/mozilla/builds/1.9.1/mozilla/js/src/jsinterp.cpp:4838
#1  0x00312c82 in js_Execute (cx=0x1441200, chain=0x16f80aa0, script=0x1d2ad2d0, down=0x0, flags=0, result=0x0) at jsinterp.cpp:1622
#2  0x0028f201 in JS_EvaluateUCScriptForPrincipals (cx=0x1441200, obj=0x16f80aa0, principals=0x17489954, chars=0x15bd808, length=1960, filename=0x16e95938 "http://www.warcraftlatino.com/ulduar/rank.html", lineno=15, rval=0x0) at /work/mozilla/builds/1.9.1/mozilla/js/src/jsapi.cpp:5145
#3  0x134a7531 in nsJSContext::EvaluateString (this=0x1c56ea00, aScript=@0xbfffc974, aScopeObject=0x16f80aa0, aPrincipal=0x17489950, aURL=0x16e95938 "http://www.warcraftlatino.com/ulduar/rank.html", aLineNo=15, aVersion=0, aRetValue=0x0, aIsUndefined=0xbfffc8f4) at /work/mozilla/builds/1.9.1/mozilla/dom/src/base/nsJSEnvironment.cpp:1631
#4  0x1327bb50 in nsScriptLoader::EvaluateScript (this=0x17489870, aRequest=0x1c9436a0, aScript=@0xbfffc974) at /work/mozilla/builds/1.9.1/mozilla/content/base/src/nsScriptLoader.cpp:686
...

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Reproduced on Linux. The interpreter has a JSString that is tagged as an Object. With trace spew enabled this crashes in NativeToValue because it can't print the object classname.

Comment 5

[Andreas Gal :gal]

This is the same kind of bug that was exploited with the heap spray. High priority to fix.

Comment 6

[David Mandelin [:dmandelin]]

(In reply to comment #4)
> Reproduced on Linux. The interpreter has a JSString that is tagged as an
> Object. With trace spew enabled this crashes in NativeToValue because it can't
> print the object classname.

I had a bug with that basic cause before. Is there some way we can prevent this condition earlier with more asserts or a LIR validator?

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Maybe. This is probably another case of missing globals. Still investigating.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Andreas: we're currently at the testing phase of Firefox 3.5.2; we can stop to pick up this fix and respin at the cost of a day's work. I guess the question is: can someone get trivially from the previous exploit to this one, or do you think it can wait for 3.5.3 which would be released in early September?

Comment 9

[Andreas Gal :gal]

Trying to figure that out now. Keep doing what you are doing for now. We will post an update asap.

Comment 10

[Andreas Gal :gal]

This does not seem to crash on Tracemonkey tip debug, so we might have fixed this inadvertently (and very recently, if 1.9.2 still crashes). Could someone help out with bisecting this to the changeset in Tracemonkey that makes the crash go away? We will debug 1.9.1 in the meantime (where we can reproduce it).

Comment 11

[Andreas Gal :gal]

1.9.2 tip (post graydon's backout is crashing too now, so no QA help needed for the moment.

Comment 12

[Andreas Gal :gal]

Attachment: shell testcase (unreduced)

Comment 13

[Andreas Gal :gal]

Attachment: partially reduced testcase

Comment 14

[Andreas Gal :gal]

Attachment: reduced testcase

Comment 15

[Andreas Gal :gal]

We have an upvar inside a constructor here.

Comment 16

[Andreas Gal :gal]

Constructor is not necessary.

Comment 17

[Andreas Gal :gal]

Attachment: without type instability and constructor

Comment 18

[Andreas Gal :gal]

Attachment: fully reduced testcase

Comment 19

[Andreas Gal :gal]

The invocation of f has an extra arg. That seems to cause the upvar access of b in g to go wrong.

Comment 20

[Andreas Gal :gal]

Attachment: patch based on dmandelin's stack math

Comment 21

[Andreas Gal :gal]

Please block 3.5.3 on this. We are testing the patch right now.

Comment 22

[David Mandelin [:dmandelin]]

The bug is caused by an incorrect computation of |spoffset| in the special case of |callDepth == 0| (i.e., top-level script), as fixed in the patch:

     if (callDepth == 0)
-        fi->spoffset = 2 /*callee,this*/ + argc - fi->spdist;
+        fi->spoffset = -fp->script->nfixed;

Given that the correct computation is much simpler than the incorrect one, it's hard to explain what I was thinking the first time around. :-) The key invariant for |spoffset| is that:

    |native_start| + |spoffset| = |slots|

        where |native_start| is the address of the jsval referred to by the 
                             0th element in the native stack area
              |slots| is the address of the caller jsval slots area

(The purpose of this invariant is that FrameInfo::spdist allows us to navigate from caller &slots[0] to callee &slots[0], which is how we navigate the frames. So we need a way to get to the trace entry active frame &slots[0].)

|native_start| in this case is StackBase(fp) for the outermost frame. Thus, we should compute 

    spoffset = slots - StackBase(fp)
             = slots - (slots + nfixed)
             = -nfixed

The first time I wrote the code, I wasn't thinking about StackBase or nfixed. Also, I incorrectly assumed that at a top-level call site, the only things that would be on the stack would be the args, and from there I worked out the original computation, which is in fact correct when there are no other values on the stack, but is not correct in general.

Comment 23

[Andreas Gal :gal]

Verified in a TM debug build with patch applied.

Comment 24

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/ccef1758e1b8

Comment 25

[Andreas Gal :gal]

Dmandelin did most of the work on this bug.

Comment 26

[Bob Clary [:bc] (inactive)]

Attachment: slightly different test

this behaved differently for me on 1.9.1 and 1.9.2 from Andreas' test case, so I'm including here. It is fixed in the shell on tracemonkey just as Andreas' is.

Comment 27

[Samuel Sidler (old account; do not CC)]

I un-hid comments 5, 8, and 9. This is already a private bug. There's no reason for them to be private as well and it actually hurts us if we want to CC someone that doesn't normally have security bug access since they can't see those comments.

Comment 28

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ccef1758e1b8

Comment 29

[Daniel Veditz [:dveditz]]

Just to confirm, we do not need any upvar-related fixes on the 1.9.0 branch right?

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 391770 [details] [diff] [review]
patch based on dmandelin's stack math

Approved for 1.9.1.3, a=dveditz for release-drivers

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #29)
> Just to confirm, we do not need any upvar-related fixes on the 1.9.0 branch
> right?

Right.

/be

Comment 32

[Samuel Sidler (old account; do not CC)]

gal/sayrer: Is this going to land on 1.9.1 tonight? It's a 1.9.1.3 blocker and code freeze was technically last night, but was extended one day...

Comment 33

[Andreas Gal :gal]

I can't land. sayrer is on a plane. Asking around for help.

Comment 34

[Samuel Sidler (old account; do not CC)]

Waldo just landed a JS bug, might be able to land this as well. (I was going to ping him on IRC, but he's just signed off.)

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d6e6ae6ba52

Comment 36

[Al Billings [:abillings - ex-MoCo]]

Carsten, can you verify that this is fixed in the 1.9.1 build from 8/17/2009?

Comment 37

[David Anderson [:dvander] - inactive, e-mail if emergency]

This bug is basically supplanted by bug 510987. The problem is superficially mitigated here but underneath the hood, stack computation is still wrong.

Comment 38

[Carsten Book [:Tomcat]]

verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.3pre) Gecko/20090819 Shiretoko/3.5.3pre - no crash

Comment 39

[David Anderson [:dvander] - inactive, e-mail if emergency]

I should clarify: this is not fixed. It no longer crashes but the fix is incorrect.

Comment 40

[Damon Sicore (:damons)]

Blocking 1.9.2 P1, but bug 510987 is a better fix for this.

Comment 41

[Robert Sayre]

this was fixed before 1.9.2 branched

Comment 42

[Christian Holler (:decoder)]

Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.