Last Comment Bug 221526 - JS Script.thaw is a security hole
: JS Script.thaw is a security hole
: fixed1.4.1, fixed1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
P1 blocker (vote)
: mozilla1.5final
Assigned To: Brendan Eich [:brendan]
: Phil Schwartau
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: 224532
  Show dependency treegraph
Reported: 2003-10-07 15:27 PDT by Brendan Eich [:brendan]
Modified: 2011-08-05 22:35 PDT (History)
8 users (show)
brendan: blocking1.4.1+
brendan: blocking1.5+
bob: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

quick and safe #ifdef fix (2.99 KB, patch)
2003-10-07 15:27 PDT, Brendan Eich [:brendan]
brendan: review+
asa: approval1.4.1+
brendan: approval1.5+
Details | Diff | Splinter Review

Description User image Brendan Eich [:brendan] 2003-10-07 15:27:27 PDT
Script.thaw deserializes a script from a string.  You can then run the script. 
A savvy attacker who knows JS bytecode and engine internals, and the target
machine architecture, can call a native function!

Patch in a second.

Comment 1 User image Brendan Eich [:brendan] 2003-10-07 15:27:56 PDT
Created attachment 132831 [details] [diff] [review]
quick and safe #ifdef fix

This is in the 1.5 branch already.

Comment 2 User image Brendan Eich [:brendan] 2003-10-07 15:33:05 PDT
Comment on attachment 132831 [details] [diff] [review]
quick and safe #ifdef fix

Shaver had to catch a cab, but he gave r= over IRC.

Comment 3 User image Asa Dotzler [:asa] 2003-10-07 15:37:08 PDT
Comment on attachment 132831 [details] [diff] [review]
quick and safe #ifdef fix

a=asa (on behalf of drivers) or checkin to 1.4.1
Comment 4 User image Brendan Eich [:brendan] 2003-10-07 15:43:05 PDT
This is so embarrassing, I think I'm going to go get drunk.  This hole has been
around since rev 3.2 of jsscript.c on 4/24/98 (Mozilla began, in general, with
rev 3.1 on 3/31/98).  The script freeze/thaw stuff was used by servers including
some that shaver was working on at the time.  We never included it in any
security review, for some reason.

Shaver, pour one for me at dinner tonight!

*Very heavy sigh*

Comment 5 User image Brendan Eich [:brendan] 2003-10-07 15:44:50 PDT
Did I say "call a native function" in comment 0?  I meant "forge a native
function and then call any native code it likes."

See why I need a drink?

Comment 6 User image Brendan Eich [:brendan] 2003-10-07 16:03:06 PDT
Ok, I've checked into the 1.4 and 1.5 branches as well as the trunk.  Marking
this fixed, but still waiting for respin update on 1.5.

Comment 7 User image Brendan Eich [:brendan] 2003-10-07 16:05:49 PDT
So the fix excludes Script.prototype.freeze/thaw and Script.thaw from being
compiled into the MOZILLA_CLIENT embedding of SpiderMonkey.  Other browser
embedders need to be apprised.  In general, any web browser embedding should
follow our lead and defined MOZILLA_CLIENT, I think.

Comment 8 User image Brendan Eich [:brendan] 2003-10-07 16:49:08 PDT
Cc'ing bryner, who is helping respin Firebird.

Cc'ing scole, whom I hereby swear to secrecy, and ask whether any planetweb
remnant exists that might need the patch.  Steve, do you recall whether
planetweb defined MOZILLA_CLIENT in its SpiderMonkey embedding?

Comment 9 User image Steven Cole 2003-10-07 17:11:48 PDT
No, Planetweb never #defined MOZILLA_CLIENT.  (It's not clear that we want to,
given the other uses of MOZILLA_CLIENT in the engine.)

I'm not too worried about this, though.  Can the 'custom native function' be
present in the spoof string?  Or does it have to reside somewhere else in the
running binary image of the embedding?  If it has to live in the embedding
already, then I'm not worried at all. 

In any event, our embeddings don't (generally speaking) contain much that needs
to be protected, and the hardware architecture details are usually kept pretty
much under wraps.  (Not that that would stop a determined hacker, of course.)

I'll let management at PW know the story, though.  

[This needs more attention by embedders than hiding behind a security-bug will
give, though.  Should freeze and thaw be turned off unless explicitly turned on,
perhaps?  Then you don't really have to announce it, and the people who don't
pay lots of attention get the fix, too.]

Comment 10 User image Brendan Eich [:brendan] 2003-10-07 17:25:18 PDT
Steve: the forged native function and its native code come from the string being
thawed into a script.  So this is a wide-open hole, if the attacker knows the JS
engine's internals and the target architecture.

Good point about turning freeze and thaw off in general.  We'll need a separate
JS_HAS_SCRIPT_FREEZE_THAW macro in jsconfig.h, defined to 0 for all versions
configured there.  I'll ponder that.

Comment 11 User image Brendan Eich [:brendan] 2003-10-07 17:27:28 PDT
MOZILLA_CLIENT is actually good to define for all web browser embeddings of the
SpiderMonkey engine, because it turns off ECMA-standard things that are unsafe
on (Script.thaw), or incompatible (escape, unescape) with, the web.

Comment 12 User image Christopher Blizzard (:blizzard) 2003-10-08 07:18:08 PDT
Now open as:
Comment 13 User image Brendan Eich [:brendan] 2003-10-08 08:09:52 PDT
I overstated the ease of exploit: there doesn't seem to be a way to serialize a
native function, or to corrupt a deserialized scripted function so that it
morphs into a native function.  That means any exploit will have to use buffer
overrun techniques (malloc size parameter wraparound).  Not easy, but possible.

I'm changing the comment above script_freeze like so:

- * we take them out of the Mozilla client altogether.  Script.thaw is a hole
- * wide enough that you could hack your own native function and call it!
+ * we take them out of the Mozilla client altogether.  Fortunately, there is
+ * no way to serialize a native function (see fun_xdrObject in jsfun.c).

I'll make this change to the trunk and all the branches, unless someone thinks I
should not make it in an old branch.  I'd like the branches to be consistent,

Comment 14 User image Christopher Aillon (sabbatical, not receiving bugmail) 2003-11-24 07:30:54 PST
Comment 15 User image Brendan Eich [:brendan] 2003-12-01 11:22:35 PST
Per comment 13, this is not huge, just as big as any hard-to-exploit but
presumably exploitable malloc-size-param-overflow bug.

Comment 16 User image Steven Cole 2003-12-01 16:59:13 PST
Since discussion is (sort of) active on this bug again: Is it appropriate to
think about/work on the jsconfig.h change mentioned in comment 10?  Or is a new
bug a better spot for that?

Comment 17 User image Brendan Eich [:brendan] 2003-12-01 17:42:44 PST
scole: I did that. shows it.

Comment 18 User image Steven Cole 2003-12-01 19:22:24 PST
>sigh<.   That's what I get for not watching checkins as closely as I once did.
 Sorry for the spam, all...


Note You need to log in before you can comment on or make changes to this bug.