Closed Bug 507292 Opened 12 years ago Closed 12 years ago

Incorrect upvar access on trace involving top-level scripts


(Core :: JavaScript Engine, defect, P1)




Tracking Status
status1.9.2 --- beta1-fixed
blocking1.9.1 --- .3+
status1.9.1 --- .3-fixed


(Reporter: cbook, Assigned: gal)




(Keywords: crash, testcase, Whiteboard: [sg:critical?], fixed-in-tracemonkey)


(3 files, 4 obsolete files)


Steps to reproduce: Load

(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
00519bd4 8b02            mov     eax,dword ptr [edx]  ds:0023:c0000005=????????
0:000> !exploitable -v
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:
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.
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 "", 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 "", 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]
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



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 


LAST_CONTROL_TRANSFER:  from 005080e6 to 00519bd4

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

  4834:                 if (!js_InternNonIntElementId(cx, obj, rval, &id))
  4835:                     goto error;
  4836:             }
> 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)


FOLLOWUP_NAME:  MachineOwner


IMAGE_NAME:  js3250.dll


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
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 "", 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 "", 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
OS: Windows XP → All
Version: 1.9.1 Branch → Trunk
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.
This is the same kind of bug that was exploited with the heap spray. High priority to fix.
(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?
Maybe. This is probably another case of missing globals. Still investigating.
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?
Trying to figure that out now. Keep doing what you are doing for now. We will post an update asap.
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).
Keywords: qawanted
1.9.2 tip (post graydon's backout is crashing too now, so no QA help needed for the moment.
Keywords: qawanted
Attached file shell testcase (unreduced) (obsolete) —
Attachment #391746 - Attachment is patch: false
Attached file partially reduced testcase (obsolete) —
Attached file reduced testcase (obsolete) —
Attachment #391746 - Attachment is obsolete: true
Attachment #391749 - Attachment is obsolete: true
We have an upvar inside a constructor here.
Constructor is not necessary.
Attachment #391751 - Attachment is obsolete: true
Attached file fully reduced testcase
Attachment #391753 - Attachment is obsolete: true
The invocation of f has an extra arg. That seems to cause the upvar access of b in g to go wrong.
Keywords: testcase-wanted
Please block 3.5.3 on this. We are testing the patch right now.
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.
Verified in a TM debug build with patch applied.
Attachment #391770 - Flags: review?(dmandelin)
Attachment #391770 - Flags: approval1.9.1.3?
Attachment #391770 - Flags: review?(dmandelin) → review+
Summary: Probably Exploitable - Data from Faulting Address controls Code Flow starting at js3250!js_Interpret+0x00000000000103b4 → Incorrect upvar access on trace involving top-level scripts
Whiteboard: [sg:critical?]
Hardware: x86 → All
Whiteboard: [sg:critical?] → [sg:critical?], fixed-in-tracemonkey
Assignee: general → gal
Dmandelin did most of the work on this bug.
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.
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.
blocking1.9.1: ? → .3+
Flags: in-testsuite?
Flags: blocking1.9.2?
Keywords: testcase
Closed: 12 years ago
Resolution: --- → FIXED
Just to confirm, we do not need any upvar-related fixes on the 1.9.0 branch right?
Flags: wanted1.9.0.x-
Comment on attachment 391770 [details] [diff] [review]
patch based on dmandelin's stack math

Approved for, a=dveditz for release-drivers
Attachment #391770 - Flags: approval1.9.1.3? → approval1.9.1.3+
(In reply to comment #29)
> Just to confirm, we do not need any upvar-related fixes on the 1.9.0 branch
> right?


gal/sayrer: Is this going to land on 1.9.1 tonight? It's a blocker and code freeze was technically last night, but was extended one day...
I can't land. sayrer is on a plane. Asking around for help.
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.)
Carsten, can you verify that this is fixed in the 1.9.1 build from 8/17/2009?
This bug is basically supplanted by bug 510987. The problem is superficially mitigated here but underneath the hood, stack computation is still wrong.
verified on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20090819 Shiretoko/3.5.3pre - no crash
I should clarify: this is not fixed. It no longer crashes but the fix is incorrect.
Group: core-security
Blocking 1.9.2 P1, but bug 510987 is a better fix for this.
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P1
this was fixed before 1.9.2 branched
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.