367120 - Potential memory corruption in script_toSource and script_toString

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=84,95-98#72
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsscript.c&rev=3.129&mark=142,149-152#133

In script_toSource and script_toString, we retrieve |script| before calling
js_ValueToECMAUint32(cx, argv[0], &indent)).  During the call to
js_ValueToECMAUint32, |script| can be freed by calling script.compile().

  var s = new Script("");
  var o = {
    valueOf : function() {
      s.compile("");
    }
  };
  s.toSource(o);
or
  s.toString(o);

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

Breakpoint 2, script_toString (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648a8, rval=0xbf91eda0) at jsscript.c:149
149         if (argc && !js_ValueToECMAUint32(cx, argv[0], &indent))
(gdb) p script
$1 = (JSScript *) 0x8163af0
(gdb) p/x *script
$2 = {code = 0x8163b20, length = 0x1, main = 0x8163b20, version = 0x0,
  numGlobalVars = 0x0, atomMap = {vector = 0x0, length = 0x0},
  filename = 0x8162481, lineno = 0x1, depth = 0x0, trynotes = 0x0,
  principals = 0x0, object = 0x815b1c0}
(gdb) c
Continuing.

Breakpoint 3, script_toString (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648a8, rval=0xbf91eda0) at jsscript.c:151
151         str = JS_DecompileScript(cx, script, "Script.prototype.toString",
(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 SIGSEGV, Segmentation fault.
0x080c85ec in Decompile (ss=0xbf91ec58,
    pc=0x610061 <Address 0x610061 out of bounds>, nb=6357089)
    at jsopcode.c:1614
1614            op = (JSOp) *pc;
(gdb) bt
#0  0x080c85ec in Decompile (ss=0xbf91ec58,
    pc=0x610061 <Address 0x610061 out of bounds>, nb=6357089)
    at jsopcode.c:1614
#1  0x080d2513 in js_DecompileCode (jp=0x81632e0, script=0x8163af0,
    pc=0x610061 <Address 0x610061 out of bounds>, len=6357089, pcdepth=0)
    at jsopcode.c:4192
#2  0x080d26a1 in js_DecompileScript (jp=0x81632e0, script=0x8163af0)
    at jsopcode.c:4212
#3  0x0805fc74 in JS_DecompileScript (cx=0x81556a8, script=0x8163af0,
    name=0x81304c2 "Script.prototype.toString", indent=0) at jsapi.c:4158
#4  0x080f7293 in script_toString (cx=0x81556a8, obj=0x815b1c0, argc=1,
    argv=0x81648a8, rval=0xbf91eda0) at jsscript.c:151
#5  0x080990e5 in js_Invoke (cx=0x81556a8, argc=1, flags=0) at jsinterp.c:1348
#6  0x080a93a4 in js_Interpret (cx=0x81556a8, pc=0x816485c ":",
    result=0xbf91f354) at jsinterp.c:4070
#7  0x080999f2 in js_Execute (cx=0x81556a8, chain=0x815a020, script=0x81647f8,
    down=0x0, flags=0, result=0xbf920424) at jsinterp.c:1607
#8  0x0805fe17 in JS_ExecuteScript (cx=0x81556a8, obj=0x815a020,
    script=0x81647f8, rval=0xbf920424) at jsapi.c:4212
#9  0x08049760 in Process (cx=0x81556a8, obj=0x815a020, filename=0x0,
    forceTTY=0) at js.c:268
#10 0x08049ecb in ProcessArgs (cx=0x81556a8, obj=0x815a020, argv=0xbf9205b8,
    argc=0) at js.c:489
---Type <return> to continue, or q <return> to quit---
#11 0x0804e5b7 in main (argc=0, argv=0xbf9205b8, envp=0xbf9205bc) 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]

Attachment: Moving the value conversions earlier.

Moving the value conversions earlier.

Comment 3

[Brian Crowder]

Sorry for spam; I meant to add:  This is a patch against the trunk.  To test it, since the Script removal is checked in, you'll need to manually turn it back on in jsconfig.h.  I will port to branches if moz_bug doesn't defeat this.

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 252964 [details] [diff] [review]
Moving the value conversions earlier.

>Index: jsscript.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsscript.c,v
>retrieving revision 3.130
>diff -u -p -8 -r3.130 jsscript.c
>--- jsscript.c	25 Jan 2007 23:08:52 -0000	3.130
>+++ jsscript.c	26 Jan 2007 23:53:20 -0000
>@@ -74,31 +74,32 @@ script_toSource(JSContext *cx, JSObject 
> {
>     JSScript *script;
>     size_t i, j, k, n;
>     char buf[16];
>     jschar *s, *t;
>     uint32 indent;
>     JSString *str;
> 
>+    indent = 0;
>+    if (argc && !js_ValueToECMAUint32(cx, argv[0], &indent))
>+        return JS_FALSE;

Nit: move indent's decl up to first place to match use order, same for toString.

Great spot-fix, thanks.

/be

Comment 5

[Brian Crowder]

Attachment: moved index decls to match use-order as suggested

This is the patch I am landing on the main trunk.  Branch patches coming.

Comment 6

[Brian Crowder]

Went to go check-in; but this is one of my first interesting security-bugs, so I stopped myself.  Seems like it might be a good idea for this one to have some sort of cover-bug?

Comment 7

[Brian Crowder]

Patch applies cleanly on both branches as-is.  Please advice with regard to landing strategy.

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 252995 [details] [diff] [review]
moved index decls to match use-order as suggested

a=dveditz for 1.8/1.8.0 branches.

You could use a public cover bug, but what would it say? You could also check in under the attachment number rather than bug number (without saying it's an attachment number -- we'll know). The checkin comment should be vague in any case, like "bail earlier if we can't get the value"

Comment 9

[Brian Crowder]

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

Comment 10

[Brian Crowder]

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

Comment 11

[Brian Crowder]

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

Comment 12

[Bob Clary [:bc] (inactive)]

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

Comment 13

[Bob Clary [:bc] (inactive)]

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

Comment 14

[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, and latest Mac 2002pre nightly.

Comment 15

[Brian Crowder]

Another one Taras might find interesting.  Similar bad pattern with pointer-usage.

Comment 16

[Bob Clary [:bc] (inactive)]

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

Comment 17

[Bob Clary [:bc] (inactive)]

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

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