Potential memory corruption in script_exec

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: moz_bug_r_a4, Assigned: Brian Crowder)

Tracking

({verified1.8.0.10, verified1.8.1.2})

Trunk
verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

10 years ago
Created attachment 251633 [details]
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)
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Whiteboard: [sg:critical?]
Critical security bugs must have owners. If you can't work on this bug help us find another active owner for it.
Assignee: general → brendan
(Assignee)

Comment 2

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

Updated

10 years ago
Assignee: brendan → crowder
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
(Assignee)

Comment 4

10 years ago
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?
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 years ago
Created attachment 253000 [details] [diff] [review]
reordering again

Same strategy as fixes in bug 367120
Attachment #253000 - Flags: review?(brendan)
(Assignee)

Comment 6

10 years ago
The patch from comment 5 also applies cleanly on both branches.
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 7

10 years ago
heh.  I THOUGHT I had already put this patch on a bug somewhere.  Turns out I had.  Pretty sure these are the same bug.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 367118
(Assignee)

Updated

10 years ago
Attachment #253000 - Flags: review?(brendan)
Whiteboard: [sg:critical?] → [sg:dupe 367118]
(Reporter)

Comment 8

10 years ago
The patch in bug 367118 does not fix this.  script_exec needs to be fixed, too.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(Assignee)

Comment 9

10 years ago
Created attachment 253102 [details] [diff] [review]
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.
Attachment #253000 - Attachment is obsolete: true
Attachment #253102 - Flags: review?(brendan)
Attachment #253102 - Flags: approval1.8.1.2?
Attachment #253102 - Flags: approval1.8.0.10?
(Assignee)

Updated

10 years ago
Whiteboard: [sg:dupe 367118] → [sg:critical]
Comment on attachment 253102 [details] [diff] [review]
same reordering concept

a=dveditz for 1.8/1.8.0 branches pending brendan's r+
Attachment #253102 - Flags: approval1.8.1.2?
Attachment #253102 - Flags: approval1.8.1.2+
Attachment #253102 - Flags: approval1.8.0.10?
Attachment #253102 - Flags: approval1.8.0.10+
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
Attachment #253102 - Flags: review?(brendan) → review+
(Reporter)

Comment 12

10 years ago
(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.)
(Assignee)

Comment 13

10 years ago
Created attachment 253125 [details] [diff] [review]
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.
Attachment #253102 - Attachment is obsolete: true
Attachment #253125 - Flags: review+
Attachment #253125 - Flags: approval1.8.1.2?
Attachment #253125 - Flags: approval1.8.0.10?
(Assignee)

Updated

10 years ago
Attachment #253125 - Attachment is patch: true
Attachment #253125 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 14

10 years ago
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)
(Assignee)

Comment 15

10 years ago
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
(Assignee)

Comment 16

10 years ago
Created attachment 253134 [details] [diff] [review]
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
<      }
Attachment #253134 - Flags: review+
(Assignee)

Comment 17

10 years ago
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
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Comment on attachment 253125 [details] [diff] [review]
Moved instanceof check up

a=dveditz for 1.8/1.8.0 branches
Attachment #253125 - Flags: approval1.8.1.2?
Attachment #253125 - Flags: approval1.8.1.2+
Attachment #253125 - Flags: approval1.8.0.10?
Attachment #253134 - Flags: approval1.8.0.10+
(Assignee)

Updated

10 years ago
Keywords: fixed1.8.0.10, fixed1.8.1.2

Comment 19

10 years ago
Created attachment 254260 [details]
js1_5/extensions/regress-367119-01.js

Comment 20

10 years ago
Created attachment 254261 [details]
js1_5/extensions/regress-367119-02.js

Updated

10 years ago
Flags: in-testsuite+
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.
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2

Comment 22

10 years ago
verified fixed 2007-02-17 1.9.0 windows/mac*/linux
Status: RESOLVED → VERIFIED
Group: security

Comment 23

10 years ago
/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
You need to log in before you can comment on or make changes to this bug.