367119 - Potential memory corruption in script_exec

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[moz_bug_r_a4]

Attachment: testcase

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsscript.c&rev=3.129&mark=271,277,340#262

In script_exec, we retrieve |script| before calling js_ValueToObject(cx,
argv[0], &scopeobj).  During the call to js_ValueToObject, |script| can be
freed by calling script.compile().

  var s = new Script("");
  var o = {
    valueOf : function() {
      s.compile("");
      return {};
    }
  };
  s.exec(o);

---
js> var s = new Script(""), o = {
  valueOf : function() {
    s.compile("");
    Array(11).join(Array(11).join(Array(101).join("aaaaa")));
    return {};
  }
}; s.exec(o); void 0;


Breakpoint 2, script_exec (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648c8, rval=0xbfbcddb0) at jsscript.c:277
277             if (!js_ValueToObject(cx, argv[0], &scopeobj))
(gdb) p script
$1 = (JSScript *) 0x8163ae8
(gdb) p/x *script
$2 = {code = 0x8163b18, length = 0x1, main = 0x8163b18, version = 0x0,
  numGlobalVars = 0x0, atomMap = {vector = 0x0, length = 0x0},
  filename = 0x81624c1, lineno = 0x1, depth = 0x0, trynotes = 0x0,
  principals = 0x0, object = 0x815b1c0}
(gdb) c
Continuing.

Breakpoint 3, script_exec (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648c8, rval=0xbfbcddb0) at jsscript.c:279
279             argv[0] = OBJECT_TO_JSVAL(scopeobj);
(gdb) p/x *script
$3 = {code = 0x610061, length = 0x610061, main = 0x610061, version = 0x61,
  numGlobalVars = 0x61, atomMap = {vector = 0x610061, length = 0x610061},
  filename = 0x610061, lineno = 0x610061, depth = 0x610061,
  trynotes = 0x610061, principals = 0x610061, object = 0x610061}
(gdb) c
Continuing.

Program received signal SIGTRAP, Trace/breakpoint trap.
JS_Assert (
    s=0x812310c "version == JSVERSION_DEFAULT || version >= JSVERSION_ECMA_3",
    file=0x8123100 "jscntxt.c", ln=180) at jsutil.c:63
63          abort();
(gdb) bt
#0  JS_Assert (
    s=0x812310c "version == JSVERSION_DEFAULT || version >= JSVERSION_ECMA_3",
    file=0x8123100 "jscntxt.c", ln=180) at jsutil.c:63
#1  0x08068bee in js_OnVersionChange (cx=0x81556a8) at jscntxt.c:180
#2  0x08068c0d in js_SetVersion (cx=0x81556a8, version=97) at jscntxt.c:188
#3  0x0809a809 in js_Interpret (cx=0x81556a8,
    pc=0x610061 <Address 0x610061 out of bounds>, result=0xbfbcdca4)
    at jsinterp.c:2272
#4  0x080999f2 in js_Execute (cx=0x81556a8, chain=0x815b260, script=0x8163ae8,
    down=0xbfbce344, flags=32, result=0xbfbcddb0) at jsinterp.c:1607
#5  0x080f78f0 in script_exec (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648c8, rval=0xbfbcddb0) at jsscript.c:340
#6  0x080990e5 in js_Invoke (cx=0x81556a8, argc=1, flags=0) at jsinterp.c:1348
#7  0x080a93a4 in js_Interpret (cx=0x81556a8, pc=0x8164854 ":",
    result=0xbfbce364) at jsinterp.c:4070
#8  0x080999f2 in js_Execute (cx=0x81556a8, chain=0x815a020, script=0x81647f0,
    down=0x0, flags=0, result=0xbfbcf434) at jsinterp.c:1607
#9  0x0805fe17 in JS_ExecuteScript (cx=0x81556a8, obj=0x815a020,
    script=0x81647f0, rval=0xbfbcf434) at jsapi.c:4212
#10 0x08049760 in Process (cx=0x81556a8, obj=0x815a020, filename=0x0,
    forceTTY=0) at js.c:268
#11 0x08049ecb in ProcessArgs (cx=0x81556a8, obj=0x815a020, argv=0xbfbcf5c8,
    argc=0) at js.c:489
---Type <return> to continue, or q <return> to quit---
#12 0x0804e5b7 in main (argc=0, argv=0xbfbcf5c8, envp=0xbfbcf5cc) at js.c:3167
(gdb)

Comment 1

[Daniel Veditz [:dveditz]]

Critical security bugs must have owners. If you can't work on this bug help us find another active owner for it.

Comment 2

[Brian Crowder]

Is this fixed by the removal of the script object?  Brendan have you looked at that patch?  That's in bug 355820, btw.

Comment 3

[Brendan Eich [:brendan]]

Yes, removing (unifdef'ing for JS_HAS_SCRIPT_OBJECT == 0, or just #define'ing that jsconfig.h macro to 0) will fix this and other bugs. But that's an API change, and we try to avoid those on the stable branches.

I'll go look at that bug now.

/be

Comment 4

[Brian Crowder]

For branches, I'm wondering if a reasonable strategy for this would be to put a flag on the script object when it is executing, such that it cannot be compiled over, and throws an exception instead?

Comment 5

[Brian Crowder]

Attachment: reordering again

Same strategy as fixes in bug 367120

Comment 6

[Brian Crowder]

The patch from comment 5 also applies cleanly on both branches.

Comment 7

[Brian Crowder]

heh.  I THOUGHT I had already put this patch on a bug somewhere.  Turns out I had.  Pretty sure these are the same bug.

Comment 8

[moz_bug_r_a4]

The patch in bug 367118 does not fix this.  script_exec needs to be fixed, too.

Comment 9

[Brian Crowder]

Attachment: same reordering concept

Weird, in my testing with the compile fix, this didn't crash.  Can you please try this patch out, moz_bug?  Same reordering concept as the other two.  Asking for branch approval now so I can land all three of these together, pending r+ from brendan.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 253102 [details] [diff] [review]
same reordering concept

a=dveditz for 1.8/1.8.0 branches pending brendan's r+

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 253102 [details] [diff] [review]
same reordering concept

> static JSBool
> script_exec(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>-    JSScript *script;
>     JSObject *scopeobj, *parent;
>     JSStackFrame *fp, *caller;
>     JSPrincipals *principals;
>-
>-    if (!JS_InstanceOf(cx, obj, &js_ScriptClass, argv))
>-        return JS_FALSE;

The instanceof check should always come first. r=me with that, post a patch-to-commit and no need to re-solicit review. Thanks,

/be

Comment 12

[moz_bug_r_a4]

(In reply to comment #9)
> Weird, in my testing with the compile fix, this didn't crash.  Can you please
> try this patch out, moz_bug?

I've tested with jsshell. script_exec patch fixes this. (script_compile patch
does not.)

Comment 13

[Brian Crowder]

Attachment: Moved instanceof check up

I'm going to hope my branch-approval is grandfathered in, regardless of this tweak, and just land this tonight with the other two.  dveditz or whoever, please re-stamp when it's convenient, or let me know if it's not for some reason and I will back out.

Comment 14

[Brian Crowder]

Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.131; previous revision: 3.130
done

(leaving open until I land on branches.. soon)

Comment 15

[Brian Crowder]

Moz1.8:

Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.79.2.20; previous revision: 3.79.2.19
done

Comment 16

[Brian Crowder]

Attachment: Moz 1.8.0 branch port

This one I think is okay to land, the diffs are pretty minor:

<      if (!js_CheckPrincipalsAccess(cx, scopeobj, principals,
<                                    CLASS_ATOM(cx, Script))) {
---
>      if (!js_CheckPrincipalsAccess(cx, scopeobj, principals, js_script_exec))
72d63
<      }

Comment 17

[Brian Crowder]

Moz 1.8.0:

Checking in jsscript.c;
/cvsroot/mozilla/js/src/jsscript.c,v  <--  jsscript.c
new revision: 3.79.2.5.2.3; previous revision: 3.79.2.5.2.2
done

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 253125 [details] [diff] [review]
Moved instanceof check up

a=dveditz for 1.8/1.8.0 branches

Comment 19

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-367119-01.js

Comment 20

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-367119-02.js

Comment 21

[juan becerra [:juanb]]

Verified using testcase in comment #0 on:

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.10pre) Gecko/20070206 Firefox/1.5.0.10pre
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.2pre) Gecko/20070206 BonEcho/2.0.0.2pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.10pre) Gecko/20070208 Firefox/1.5.0.10pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.2pre) Gecko/20070207 BonEcho/2.0.0.2pre

Also on latest linux trunk.

Comment 22

[Bob Clary [:bc] (inactive)]

verified fixed 2007-02-17 1.9.0 windows/mac*/linux

Comment 23

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367119-01.js,v  <--  regress-367119-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-367119-02.js,v  <--  regress-367119-02.js
initial revision: 1.1