A site that serves private data to users using JSON are often vulnerable to the following attack. A malicious site redefines Array or Object, then loads a JSON file as a script from another site using e.g. <script src="https://mail.victimsite.com/address-book.json"></script>. The JSON file is interpreted as an expression statement, where the expression is an array or object literal. This causes the constructor to be called with "this" being the array/object that's about to be populated with properties. It's counterintuitive that serving data that could be interpreted as JavaScript, even if it's just an expression literal, makes that data accessible to other sites. I don't think web developers think about JSON literals as being JavaScript; they might not even be intended for parsing with JavaScript (JSON is gaining popularity as a lightweight alternative to XML). So it would be nice to change web browsers to prevent the vulnerability instead of relying on web developers to protect their sites against cross-site <script src> loads. It might be possible to prevent this kind of attack from succeeding by changing the browser, rather than putting the responsibility entirely on web sites to ensure that data they serve can't be interpreted as JavaScript. Here are some potential fixes: A) Ignore "Array = ..." and "Array.prototype.h setter= ...". This might violate ECMAScript. B) Ignore those for unused array/object literals (expression statements where the entire expression is the array/object) (discard them as if they were useless statements). C) Ignore those for cross-domain script loads. D) Ignore those for scripts served with non-ECMAScript MIME types (similar to a fix I proposed for bug 375250). E) Refuse to parse non-ECMAScript mime types as JavaScript at all (as Brendan suggested in bug 375250 comment 1). I don't know whether JSON data is typically served with an ECMAScript mime type, so I don't know how effective the last two would be against this kind of attack. http://jibbering.com/blog/?p=514 suggests application/json (and recommends against text/html) but doesn't explain why he chose application/json over text/javascript.

While this attack isn't new, it has gotten a lot of attention in the last week: http://www.fortify.com/news-events/releases/2007/2007-04-02.jsp http://www.fortifysoftware.com/security-resources/javascripthijacking.jsp http://it.slashdot.org/article.pl?sid=07/04/02/1113242 http://getahead.org/blog/joe/2007/03/05/json_is_not_as_safe_as_people_think_it_is.html It would be great if we could protect Firefox users from the attack.

http://developer.mozilla.org/es4/clarification/which_prototype.html I'm all for making String/Array/Object/Boolean/Date readonly on the global object and referring to those specific values in algorithms. A quick-and-dirty search shows this doesn't trivially break anything (for the Array case) in code Google can find: http://www.google.com/codesearch?as_q=%5CsArray%5Cs*%3D&btnG=Search+Code&hl=en&as_lang=javascript&as_license_restrict=i&as_license=&as_package=&as_filename=&as_case=y If ES4 doesn't jump for that, I'd say just use the original value of the "Array"/etc. property of the global object in the relevant algorithms. Apparently Opera does it already in at least some situations, and nobody's complained.

(In reply to comment #0) > A site that serves private data to users using JSON are often vulnerable to the > following attack. A malicious site redefines Array or Object, then loads a > JSON file as a script from another site using e.g. <script > src="https://mail.victimsite.com/address-book.json"></script>. The JSON file > is interpreted as an expression statement, where the expression is an array or > object literal. This causes the constructor to be called with "this" being the > array/object that's about to be populated with properties. The site hosting the JSON file has made a crucial error in not securing assets in the JSON data, of course. Pointing this out doesn't reduce the severity of this bug in the real world, but it needs to be noted. > It might be possible to prevent this kind of attack from succeeding by changing > the browser, rather than putting the responsibility entirely on web sites to > ensure that data they serve can't be interpreted as JavaScript. Or that JSON data be protected from access by random scripts. > Here are some potential fixes: > > A) Ignore "Array = ..." and "Array.prototype.h setter= ...". This might > violate ECMAScript. ES4/JS2 is trying to make Array, etc., immutably bound in the global object, but this won't help ES3 and below. The setter= (__defineSetter__) extension is not part of ES3 or ES4, but SpiderMonkey has supported it for years (> 7 years now IIRC) and we're loath to break even that much backward compatibility. > B) Ignore those for unused array/object literals (expression statements where > the entire expression is the array/object) (discard them as if they were > useless statements). This is a clever idea and it may be worthwhile. More thought required. > C) Ignore those for cross-domain script loads. This is perhaps less likely to break existing content, and it's a good idea for security (integrity). > D) Ignore those for scripts served with non-ECMAScript MIME types (similar to a > fix I proposed for bug 375250). It might be better to ignore JSON data altogether if not served with a good MIME type. Anyone know of sites that would break if we did that? > E) Refuse to parse non-ECMAScript mime types as JavaScript at all (as Brendan > suggested in bug 375250 comment 1). Ah, there it is. > I don't know whether JSON data is typically served with an ECMAScript mime > type, so I don't know how effective the last two would be against this kind of > attack. http://jibbering.com/blog/?p=514 suggests application/json (and > recommends against text/html) but doesn't explain why he chose application/json > over text/javascript. See http://www.ietf.org/rfc/rfc4627.txt -- it's better to use a more precise type, and application/json is standardized by an RFC. /be

(In reply to comment #2) > If ES4 doesn't jump for that, ES4 is doing that. > I'd say just use the original value of the > "Array"/etc. property of the global object in the relevant algorithms. > Apparently Opera does it already in at least some situations, What situations? ES3 is clear that {p:42} in an expression context is short for t = new Object; t.p = 42 for a gensym'ed t, similarly for array initialisers. > and nobody's complained. Yeah, good for Opera -- this paves the way for type checking in strict mode (the type checker has to be able to reason about type bindings), but it's not enough for JSON safety given prototype setters. /be

(BTW, sorry for losing your cc: waldo -- I noted the collision and meant to get back here and re-add you, but children interrupted me for a bit.) Bob, can you and Jesse investigate the MIME type situation? Thanks, /be

(In reply to comment #4) > ES4 is doing that. Yay! > What situations? ES3 is clear that {p:42} in an expression context is short > for t = new Object; t.p = 42 for a gensym'ed t, similarly for array > initialisers. From the URL, Lars T Hansen 2006/05/03 00:46: "Opera actually flaunts the standard and interprets “as if by new Array()” by calling the internal array constructor, not by looking up Array in the global environment." To be clear, is B saying that for the Array/Object constructors when invoked while parsing/interpreting a literal, a nested invocation of [[Put]] uses the intrinsic setter, not any user-defined setter? It would make self-hosting a little harder (need some sort of wacky private constructor behavior), but it'd deal with a little bit of the problem nicely. Does anyone rely on Array.prototype setters for array literals? (Also, you didn't lose the CC -- and I watch the default assignee/QA contact, so I usually only CC if it's security-ish and might Go Away or if I'm commenting already.)

(In reply to comment #5) > Bob, can you and Jesse investigate the MIME type situation? Thanks, In doing a fairly large scan of the top alexa sites, I found a limited number of urls that obviously refer to json data: The results for content-type are: 51 application/json 23 application/x-javascript 20 text/html 15 text/x-json 8 text/javascript 4 undefined 1 text/plain

Waldo, you want to take this? I don't know what the right answer is, and comment 7 is depressing me, but we can probably figure out a better path forward. /be

Can do.

Ideally servers should deny access to all JS files (including json ones) by looking at the referer headers. Except in the cases where it wants to allow cross site requests of course. However I agree it is easy to forget which files can be interpreted as a JS file. Maybe we could add a request header saying that we're performing a cross-site <script> request. It would then be much easier to block those on a server level. Ideal would be if we could talk the apache people into making it easy to configure the server to block. We could add similar headers for cross-site CSS and image requests. That would make it easier for server admins to prevent hot-linking images, which is a royal pain today. These headers should not include the url of the requesting site since that would encourage privacy firewalls to block the header. Sites that want to allow certain sites can use the new header in addition to the referer header. This won't of course fix old browsers, but neither does anything else suggested so far.

(In reply to comment #10) > Ideally servers should deny access to all JS files (including json ones) by > looking at the referer headers. Except in the cases where it wants to allow > cross site requests of course. What happens when user use a Referer-Spoof extension, like https://addons.mozilla.org/en-US/firefox/addon/953 https://addons.mozilla.org/en-US/firefox/addon/1093 https://addons.mozilla.org/en-US/firefox/addon/4513 https://addons.mozilla.org/en-US/firefox/addon/1999

(In reply to comment #4) > Yeah, good for Opera -- this paves the way for type checking in strict mode > (the type checker has to be able to reason about type bindings), but it's not > enough for JSON safety given prototype setters. Could ES4 setters be used the same way? Taking the Opera approach a bit further: when evaluating ({x:0, y:1}), why not create the new object with a null __proto__, then assign the properties, and lastly assign newobj.__proto__ = Object.prototype?

I'm going to try working on this over the next week-ish; we'll see what happens after that time and whether this is something I can reasonably finish "soon", given classes and the rest of my schedule.

This seems like the least invasive fix for the use-original-bindings part of the bug; getters and setters can come after this. We seem to be running a bit low on JSCLASS_* bits. New bytecodes might be overkill here, but longer-term I think it makes sense to have better bytecode for literals, particularly for making [...] and Array(...) equally fast in bug 396191.

Comment on attachment 281032 [details] [diff] [review] First step: Array/Object readonly/dontdelete/dontenum + use the global bindings Quick comments: * Make all bindings of class names in global objects readonly and permanent, no need for another JSCLASS_ flag. * Add only one bytecode, taking a JSProto_Object or JSProto_Array JOF_BYTE operand, instead of JSOP_NEWARRAY or JSOP_NEWOBJECT. Don't call it JSOP_NEW* -- perhaps JSOP_PUSHCLASS? Ugh. * Nit on jsemit.c: multiline if condition triggers bracing of then/else, even if then and else (if present) are one-liners. /be

Better than JSOP_PUSHCLASS: JSOP_CALLCTOR (after JSOP_CALLNAME). /be

I still think the header idea would be a good one, but maybe a separate bug would be better for that.

(In reply to comment #17) > I still think the header idea would be a good one, but maybe a separate bug > would be better for that. Not maybe, definitely :-P -- file it! /be

If it makes sense, we could file a blocking bug for the "make all standard class constructors have immutable global bindings" work. /be

jwalden: ping. /be

Picking this patch up to revive it, but I have a quick question: > * Make all bindings of class names in global objects readonly and permanent, > no need for another JSCLASS_ flag. By this, you mean that the js_DefineFunction call in JS_InitClass -should- always make the ctor function JSPROP_READONLY | JSPROP_PERMANENT? Is that a safe API change? Am I misunderstanding you?

Blah, I talked to Brendan about this yesterday, probably should have commented too. I'm still on this at least for the moment, definitely through this weekend (which extends through Monday for me), after attending to one other bug that Jesse's been on my case about for over a month. If I'm not done with this bug by then, with a patch that's basically what we want but perhaps still in need of some number of tweaks, I'll put what I have here and punt to someone with more time.

I've made some good progress on this, and I basically have everything working -- just fighting the decompiler at this point. I'm currently trying to combine JSOP_CALLCTOR and JSOP_NEWINIT into a single opcode, since as far as I can tell there's no need for two. If I can make that work by the end of the week, that's what I'll post; if it doesn't work, I have a different version of the patch which doesn't combine the two which seems to work fine. Finally, if that fails, I have a patch which treats the decompiler as though it were infected with the plague and does its best not to touch it, which also works. *One* of these patches will work correctly -- just trying to give the best one (the one that doesn't add an extra opcode) a chance to succeed before falling back to the others.

I think I have a full patch -- just running it through all the testsuites I can find right now before submitting it (and that's *all* of each testsuite, including slow-n.tests). In the meantime, here's the testcase I used while debugging; there aren't any value things tested here because I expect the test suites to be far more thorough than I'd be, and it didn't seem worth my time -- interpreting's the easy part :-) . I ran this manually during testing, but it could be converted to a SpiderMonkey-specific test pretty easily.

This patch makes all function bindings in the global environment readonly/dontdelete, and it rewrites object/array literals and array comprehensions to use a new bytecode JSOP_CALLCTOR. This bytecode takes an operand indicating either array or object, and it performs the same bookkeeping as JSOP_CALLNAME+JSOP_NEWINIT did previously. This works with nearly everything I throw at it. It fails, however, when I call a function and pass an empty (only empty, as far as I can tell) array or object as the last parameter: function a() { } a({}); a([]); a(1, []); a(1, {}); // &c. Assertion failure: vp + 2 + argc <= (jsval *) cx->stackPool.current->avail, at jsinterp.c:1097 I really shouldn't spend any more time on this at the moment, so I'm dumping this in the hopes that someone will see the obvious flaw in my patch and can fix it. *Something* in JSOP_INITELEM or JSOP_INITPROP is correcting an omission in JSOP_CALLCTOR or do_new, but I don't know what. This patch will also fix bug 379523, for what it's worth, by replacing an exact-index string check with an opcode check.

The bug in the last patch was, as far as I could tell, that there exists no way to signal that an opcode requires a certain number of stack slots but only uses a smaller number when the opcode has been fully processed. I admit to being surprised; I did my best to minimize this cost with a macro that should compile into as near nothingness as possible. I ran this against the JS tests; the only new failure was js1_5/extensions/regress-325269.js, but since that test relies on globally rebinding Array it should be expected to fail. I didn't bother trying to figure out whether or how it could be changed to actually work.

Comment on attachment 290172 [details] [diff] [review] Patch, passes tests JS_InitClass should use the right attributes /ab initio/ -- no need to race and mutate (it's gonna cost, possibly noticeably). For maximum API compat, could do this only if JSCLASS_FIXED_BINDING is set among clasp->flags. /be

(In reply to comment #27) > (From update of attachment 290172 [details] [diff] [review]) > JS_InitClass should use the right attributes /ab initio/ -- no need to race and > mutate (it's gonna cost, possibly noticeably). Not to mention the code bloat -- that JS_InitClass|JS_SetPropertyAttributes pipe s a non-trivial source (and I bet object) code tax. /be

(In reply to comment #28) > > JS_InitClass should use the right attributes /ab initio/ > > Not to mention the code bloat -- that JS_InitClass|JS_SetPropertyAttributes > pipe s a non-trivial source (and I bet object) code tax. Given that that's exactly what I did in the first patch, with the same backwards-compat arguments presented with respect to not making the constructor binding immutable-by-default, it would have been nice to have decided on that at the start. ;-)

Sorry, I should have reviewed the bug -- I'm not certain we need API compat in this regard, but I am certain we don't want the source and object bloat of the InitClass|SetPropertyAttribute pair all over. And the race is just bad news, for efficiency and consistency. Let's try to reason about API compat vs. ES4 future. If the JS API survives into ES4 times, JS_InitClass will make a real class object, so will want to bind it with a fixed (RO+DD) binding. Do we really want the JSCLASS_FIXED_BINDING or whatever flag then? I think not. So simplest approach is to change JS_InitClass. /be

(In reply to comment #30) > Let's try to reason about API compat vs. ES4 future. If the JS API survives > into ES4 times, JS_InitClass will make a real class object, so will want to > bind it with a fixed (RO+DD) binding. I'm not so sure about this. Given the various bells and whistles that classes have (dynamic, traditional, final, anything else I've missed from not following the list or skimming the overview closely enough), I'm initially skeptical that we'd overload JS_InitClass like this instead of defining a new method. > Do we really want the JSCLASS_FIXED_BINDING or whatever flag then? If we overload JS_InitClass or if people actually don't mind the incompatibility, then no, it doesn't make sense. I'm skeptical of both, but I'm not the most qualified person to express an opinion on either subject. Should we run the incompatibility issue by the newsgroup and see who, if anyone, would have incompatibility problems? If nobody complains I don't mind, but I'd want more than guesses before making an incompatible change.

Comment on attachment 290181 [details] [diff] [review] With a JSCLASS_FIXED_BINDING flag to avoid source bloat >? .gdb_history >Index: jsapi.c >=================================================================== >RCS file: /cvsroot/mozilla/js/src/jsapi.c,v >retrieving revision 3.374 >diff -p -u -8 -r3.374 jsapi.c >--- jsapi.c 19 Nov 2007 17:15:45 -0000 3.374 >+++ jsapi.c 26 Nov 2007 06:40:00 -0000 >@@ -2678,17 +2678,20 @@ JS_InitClass(JSContext *cx, JSObject *ob > NULL); > if (!named) > goto bad; > } > > ctor = proto; > } else { > /* Define the constructor function in obj's scope. */ >- fun = js_DefineFunction(cx, obj, atom, constructor, nargs, 0); >+ fun = js_DefineFunction(cx, obj, atom, constructor, nargs, >+ (clasp->flags & JSCLASS_FIXED_BINDING) >+ ? JSPROP_READONLY | JSPROP_PERMANENT >+ : 0); This is in the else clause -- the then clause is for the constructor == NULL case, and it could use similar love: (clasp->flags & (JSCLASS_IS_ANONYMOUS | JSCLASS_FIXED_BINDING)) ? JSPROP_READONLY | JSPROP_PERMANENT : 0, > /* >+ * Indicates that the constructor defined on the object provided to >+ * JS_InitClass should be readonly and permanent. >+ */ >+#define JSCLASS_FIXED_BINDING (1<<(JSCLASS_HIGH_FLAGS_SHIFT+4)) Per above, the comment here needs adjusting from "constructor" to "class object". >- JSCLASS_HAS_PRIVATE | JSCLASS_HAS_CACHED_PROTO(JSProto_Array), >+ JSCLASS_HAS_PRIVATE | JSCLASS_FIXED_BINDING | >+ JSCLASS_HAS_CACHED_PROTO(JSProto_Array), Pervasive nit: my inner child would be less whiny if you put JSCLASS_FIXED_BINDING last in the | chain. >+++ jsemit.c 26 Nov 2007 06:40:01 -0000 >@@ -2094,17 +2094,17 @@ CheckSideEffects(JSContext *cx, JSTreeCo > * TOK_LB binary trees of 3 or more nodes are flattened into lists > * to avoid too much recursion. All such lists must be presumed > * to be useful because each index operation could invoke a getter > * (the JSOP_ARGUMENTS special case below, in the PN_BINARY case, > * does not apply here: arguments[i][j] might invoke a getter). > * > * Array and object initializers (TOK_RB and TOK_RC lists) must be > * considered useful, because they are sugar for constructor calls >- * (to Array and Object, respectively). >+ * (to the ECMA Array and Object bindings, respectively). Ick, please don't change this comment. Citing "ECMA" is pure eyestrain, and "bindings" is pedantic. > /* > * Emit code for [a, b, c] of the form: >- * t = new Array; t[0] = a; t[1] = b; t[2] = c; t; >+ * t = new <global>.Array; t[0] = a; t[1] = b; t[2] = c; t; Ditto, or similar: this <global>.Array seems to imply a different bytecode format from what is really happening (one that gets and pushes <global>, then gets the value of the 'Array' property). Bets to let this comment alone, I think. > /* > * Emit code for {p:a, '%q':b, 2:c} of the form: >- * t = new Object; t.p = a; t['%q'] = b; t[2] = c; t; >+ * t = new <global>.Object; t.p = a; t['%q'] = b; t[2] = c; t; Ditto. > # define DO_NEXT_OP(n) do { METER_OP_PAIR(op, pc[n]); \ > op = (JSOp) *(pc += (n)); \ > DO_OP(); } while (0) >+# define DO_EXPECTED_OP(n, OP) \ >+ do { \ >+ METER_OP_PAIR(op, pc[n]); \ >+ JS_ASSERT(*(pc + (n)) == (OP)); \ >+ op = (OP); \ >+ pc += (n); \ >+ JS_EXTENSION_(goto *jumpTable[OP]); \ >+ } while (0) SpiderMonkey style justifies \ to column 79, always, and uses four-space indentation. But is this new macro really necessary? I'm now questioning the JSOP_CALLCTOR addition. Better to morph JSOP_NEWINIT to do the combination in one VM cycle. >+ BEGIN_CASE(JSOP_CALLCTOR) >+ i = GET_INT8(pc); >+ JS_ASSERT(i == JSProto_Array || i == JSProto_Object); >+ ok = JS_GetMethod(cx, cx->globalObject, >+ i == JSProto_Array ? "Array" : "Object", Nit: house style parenthesizes ?: conditions unless primary expressions. >+ &obj, &rval); >+ if (!ok) >+ goto out; >+ JS_ASSERT(VALUE_IS_FUNCTION(cx, rval)); >+ PUSH_OPND(rval); >+ PUSH_OPND(OBJECT_TO_JSVAL(cx->globalObject)); cx->globalObject is the wrong object. Consider running on the cx for Window A, where the code running is a function lexically scoped to Window B. You want B's global object. Should use JS_GetGlobalForObject starting from fp->scopeChain. > OPDEF(JSOP_INT8, 227, "int8", NULL, 2, 0, 1, 16, JOF_INT8) > OPDEF(JSOP_INT32, 228, "int32", NULL, 5, 0, 1, 16, JOF_INT32) >+ >+/* Special bytecode to construct an Array/Object using the global binding. */ >+OPDEF(JSOP_CALLCTOR, 229, "callctor", NULL, 2, 0, 2, 19, JOF_BYTE) Nit: make the columns line up with the previous rows' columns. Bug: JOF_INT8, not JOF_BYTE. Thanks for the typo fixes in comments! This is close, with the above fixed I'll r+a quickly. /be

(In reply to comment #32) > > /* > > * Emit code for [a, b, c] of the form: > >- * t = new Array; t[0] = a; t[1] = b; t[2] = c; t; > >+ * t = new <global>.Array; t[0] = a; t[1] = b; t[2] = c; t; > > Ditto, or similar: this <global>.Array seems to imply a different bytecode > format from what is really happening (one that gets and pushes <global>, then > gets the value of the 'Array' property). But of course, that's what you are doing with JSOP_CALLCTOR|JSOP_NEWINIT. Sorry for misreading, but I can still claim victory ;-). We should not have two ops where one will do, so please do morph JSOP_NEWINIT. Also, we could avoid looking up the constructor by using js_GetClassObject on fp->scopeChain (no need to use JS_GetGlobalForObject). This reminds me: given the fixed bindings, the JSCLASS_GLOBAL_FLAGS trick of memoizing formerly mutable binding values in reserved slots is no longer needed. It does give faster lookups, though. So long as JSCLASS_FIXED_BINDING is opt-in, it could be considered useful, but we don't let embedders opt out of the setting of JSCLASS_FIXED_BINDING in all the built-in classes. Seems we have one too many knobs here. /be

(In reply to comment #32) > Better to morph JSOP_NEWINIT to do the combination in one VM cycle. Yeah, I wasn't thinking there'd be APIs to do this without overusing stack space, as mentioned earlier. > Nit: house style parenthesizes ?: conditions unless primary expressions. I think I hit all of these and fixed them to be correct, but a double-check would be good. > cx->globalObject is the wrong object. Consider running on the cx for Window A, > where the code running is a function lexically scoped to Window B. You want > B's global object. Should use JS_GetGlobalForObject starting from > fp->scopeChain. Or the solution in this patch, which I think avoids the problem entirely, maybe? I'm also not entirely sure that my proto/parent arguments to js_NewObject are the correct ones -- please enlighten if they aren't. > Bug: JOF_INT8, not JOF_BYTE. You sure? That change makes me hit "Assertion failure: op == JSOP_INT8, at jsopcode.c:371" in a way that looks like it'd hit for every JSOP_CALLCTOR. No problems for me if I leave it as JOF_BYTE, as in this patch.

One other change: the |ss->opcodes[ss->top - X]| checks for JSOP_DEFSHARP are unnecessary since we never PushOff the sharp sprinting, as verified by some printfs for both arrays and objects.

JOF_BYTE means 1-byte code, JOF_INT8 means 2-byte sequence: [op, int8-immediate]. /be

Waldo, you got Igor's back on that disassembler generalization by assertion removal (and maybe code sharing with other int-printing code) for JOF_INT8? /be

Comment on attachment 291314 [details] [diff] [review] With JOF_INT8 Really want to keep the name JSOP_NEWINIT, it's better than JSOP_CALLCTOR (ties the op to the initialiser syntax, so narrows the operand to Object or Array at present); plus the patch is overlarge due to this renaming. I need to study the decompiler change more to do a real review, but if you can fix that, I'll stamp a smaller patch by virtue of restoring JSOP_NEWINIT. /be

Comment on attachment 291330 [details] [diff] [review] s/CALLCTOR/NEWINIT/ > BEGIN_CASE(JSOP_NEWINIT) >- argc = 0; >+ i = GET_INT8(pc); >+ JS_ASSERT(i == JSProto_Array || i == JSProto_Object); >+ obj = (i == JSProto_Array) >+ ? js_NewArrayObject(cx, 0, NULL) >+ : js_NewObject(cx, &js_ObjectClass, NULL, NULL); Nit: that's a fine underhanging multiline ?: expression style, but it just ain't prevailing style, which puts ? and : under the first char of the condition (usually a '('). Wouldn't it be great to pre-allocate the array based on the number of property initialisers? Could do likewise for the object's dslots array. Please file followup bugs, and see bug 396191 ("Array literals are significantly slower than new Array()") and please feel free to take these bugs, I think they are in scope for 1.9 (safe enough perf win). The idea would be to encode the population of the array or object as another immediate operand (so a custom JOF_NEWINIT format, I suspect). Separate bug (singular) seems ok on reflection, and you could just morph bug 396191 to be more general and use it. > sn = js_GetSrcNote(jp->script, pc); >+ /* NB: must not overwrite todo after this! */ Nit: blank line before comment-on-its-own-line (or multiline comment). Note without nit-picking: the JSOP_INITPROP isFirst local in jsopcode.c is single-use, could be eliminated by using ss->opcodes[ss->top] in the Sprint arglist -- but that breaks symmetry with JSOP_INITELEM, which needs isFirst early and twice. Only nit this leaves me to pick: put a blank line before the do_initprop: label, just to let the code breathe a bit. Nice work, looking forward to further optimization for object and array initialisers -- and no regressions from this patch! (knock on wood...) /be

Of course, calling Array(e1, ... eN) to optimize [e1, ... eN] works only if there are no holes. See bug 260106. Calling something internal to preallocate obj->dslots means a new jsobj.c internal API, but that's doable. It might be worthwhile to eliminate JSOP_INITPROP and use the interpreter stack, so that {p1:v1, ... pN:vN} translates into a memoized scope or at least lastProp-linked property tree path for p1...pN (since those props have known ids, attrs, and even slots), and the v1...vN values on the operand stack. Then the new object's dslots could be populated by copying from the stack. Just mumbling, hope this both makes sense and gets into the other bug. /be

Comment on attachment 291330 [details] [diff] [review] s/CALLCTOR/NEWINIT/ >+ if (Sprint(&ss->sprinter, "%s", rval) < 0) Late-breaking fix request: use SprintCString here, never use "%s" as the fmt. /be

Patch with changes is in! We still have more to do with things like considering what to do about setters on Array.prototype, so I'm leaving this open for now. I'll file the followup(s) on optimizing array literals further probably tomorrow -- keyboard's being wonky now, and it's also getting late here.

this caused Bug 406736

this also caused bug 406769

this caused bug 407323

I think "fixing" this "security problem" is an error. Honestly, if you serve any json content (or in any other format) into the web without protecting it in any way you can trivially implement a proxy service which calls a js function with the contents as a string (or any other structure). So I can circumvent this for the whole web by creating a service which can be called like http://example.com/getAnything.php?url=whateverurl to fetch any file returning a well formed js file with a "valueStolen(<url>, <value>)" function call. I'm quite tempted to do it right now. Having this "security" features might make people think their information is "secure". And this misfeature of not allowing you to redefine the [] and {} literals seems totally unlike js. I mean, if you change Object and Array and then use it all over the place you will start to wonder what it's happening.

Note that you setting up a proxy like that is not going to expose users to the same attack. The thing is that when the browser sends a request for a script, it will also include any cookies and authorization information that the user has for that site. So if I log in to my bank, and then 5 minutes later go to evil.com, evil.com can then point a <script> tag at my bank and get potentially get personal information using this hack. That obviously would not be possible with your proxy. Not arguing one way or the other about the "unjavascriptness" though. I'll leave that up to others.

First, let's get the security argument straight: meaningful security consists *in part* in "defense in depth". Each link in a chain of cause and effect should have affordable defensiveness. That's the surface justification for this change. But look deeper: http://wiki.ecmascript.org/doku.php?id=clarification:which_prototype Combined with varying browser behavior (is Object memoized at startup and that value used for object initialisers? Contrary to ES3, but how about looking up Object in the scope chain? Not all browsers do that) and the need for type name invariance for any kind of soundness or safety property provable from the ES4/JS2 type system, and as the docs at http://www.ecmascript.org/ say, the new edition of the language locks down typename bindings. See in particular http://www.ecmascript.org/es4/spec/incompatibilities.pdf section 1.4 "Top-level Names are Read-Only, not Meta-Programming Hooks". The fact is that users can't count on cross-browser behavior in rebinding the standard constructors. I'd be only a little surprised if this breaks anything. We spidered the Alexa top 1000 four or so levels deep, and the only standard constructor object rebound by a script that we detected was Error, in an obsolete MSN .js file. That file is being removed, my contact at Windows Live says. But this is only suggestive, along with the inconsistencies in ES3 and browser implementations, that we can make this change without causing much pain. If the change proves too painful, we can do it only when opt-in versioning is used (the RFC 4329 version parameter). /be

Another point, about "totally unlike js" -- ES3 is inconsistent too, in locking down Object.prototype (that property is ReadOnly and DontDelete), while allowing the Object property in the global object to be overwritten or replaced. Don't mix up the mutability of the object named Object.prototype -- that's good and very "js-like". The only issue is can you count on "Object" to mean what it should? There's a general school of thought, which has merit, that maintains the user is smarter. This is a variation on "evolution is smarter than you are". But there are lots of free names to use for an evolved Object or Array. Rebinding the standard properties is more likely a mistake, or an attack. The language makes guarantees about some things (which is why the standard constructors' prototypes are RO+DD, while user-defined functions' prototype properties are read/write and deleteable). /be

ok, not having the cookies really limits the usefulness of the proxy. That alone weakens my arguments to about 10% of the original :). However this doesn't really fix any side effect you might have done on the server by the result of you action. I think this would be more properly fixed by educating the developers of ways into which a secret token can be included into the request body (as opposed to the headers). I'm worried that these kind of band-aid actions might eventually make the problems worse for developers instead of better (I'm thinking of several things php developers used like "magic quotes", transparent session ids added to all links). These things had IMHO made a lot of web developers not aware of the real problem and caused the problem that probably most web pages nowadays are susceptible to XSS problems. This wouldn't be new to mozilla: you changed the web by educating the audience about standards in the past. Re Bredan: ok, it's not likely to break anything now. It seems to be the direction. This reduces my argument to about 2% :).

Is there a final tally on which global objects are being restricted? From this patch I count: Array, Boolean, Date, Math, Number, Object, RegExp, and String. (The XML stuff was removed in bug 407323) However, Brendan, you mentioned that Error was being redefined on an MSN page, as if that may have caused problems - is Error (and other global objects) also being accounted for here?

I think that was an oversight on my part; changing the 0 at the end of this line to JSPROP_READONLY | JSPROP_PERMANENT should fix it: http://mxr.mozilla.org/mozilla/source/js/src/jsexn.c#1055

Would like to get this into Fx3b2 to match the other changes, so we get cleaner yes/no test results and don't have to wait for b3 to find out. The Error override in that old MSN .js file is unused, my source at Windows Live says. We want to get this change in tonight, but once again we'll need help from Reed or Gavin, if it's approved. /be

That latest patch has r+jwalden, per comment 54. /be

Comment on attachment 292361 [details] [diff] [review] followup fix to jsexn.c a=schrep only if we are sure this is near zero risk - we are out of time for any additional patches

Landed attachment 292361 [details] [diff] [review] on the trunk at schrep's request: mozilla/js/src/jsexn.c 3.94 I've also triggered new nightlies.

I assume the new failure in js1_5/Exceptions/regress-342359.js is expected. I'll update the test to check if ReferenceError is writable.

(In reply to comment #59) > I assume the new failure in js1_5/Exceptions/regress-342359.js is expected. > I'll update the test to check if ReferenceError is writable. Yes, you can't override the *Error standard constructors either now. /be

dev-doc-needed: http://ejohn.org/blog/re-securing-json/ could be mentioned somewhere in http://developer.mozilla.org/en/docs/Updating_web_applications_for_Firefox_3

A good point was brought up in a comment on my blog post: What about XMLHttpRequest? If there was ever a worry about snooping on data transmissions, this would be it. Although, this might effect extensions like Firebug. Is this within the scope of this bug?

XMLHttpRequest is subject to same-origin checks, afaik (cf. bug 389508), so what's the problem?

(In reply to comment #62) > A good point was brought up in a comment on my blog post: What about > XMLHttpRequest? You can always get to the real XMLHttpRequest via |XPCNativeWrapper(window).XMLHttpRequest|.

(In reply to comment #64) > (In reply to comment #62) > > A good point was brought up in a comment on my blog post: What about > > XMLHttpRequest? > > You can always get to the real XMLHttpRequest via > |XPCNativeWrapper(window).XMLHttpRequest|. Blake is talking about chrome code, e.g. a Firefox addon. Worries about XHR are far afield from this bug, and need more justification to take up bug bandwidth. /be

I think we're done here. Learned something, restored compatibility, time to move on. Waldo, what do you say? /be

Can you add a comment summarizing what changed and what didn't change? It's hard for me to tell what is protected and what isn't since there are so many (fixed) bugs blocking this one.

Waldo made {p:42} evaluate using the original value of the Object constructor, same with ['foo']. That leaves proto-getter attacks possible, but eliminates shadowing and replacing attacks. /be

(In reply to comment #68) > That leaves proto-getter attacks possible But what about disabling getters and setter for (Object|Array|String|...).prototype?

Also, what about the simple case '2'. Will that use the Number constructor?

(In reply to comment #70) > Also, what about the simple case '2'. Will that use the Number constructor? No.

I think this bug has suffered enough.

Docs updated.

(In reply to comment #69) > (In reply to comment #68) > > That leaves proto-getter attacks possible > > But what about disabling getters and setter for > (Object|Array|String|...).prototype? Time to revisit this hint? http://www.thespanner.co.uk/2009/01/07/i-know-what-your-friends-did-last-summer/

(In reply to comment #74) > (In reply to comment #69) > > (In reply to comment #68) > > > That leaves proto-getter attacks possible > > > > But what about disabling getters and setter for > > (Object|Array|String|...).prototype? > > Time to revisit this hint? > http://www.thespanner.co.uk/2009/01/07/i-know-what-your-friends-did-last-summer/ No, time to refuse to treat applicaton/json and other non-js MIME types as JS. Also, twitter failed to authenticate. What a surprise. /be

Isn't that a huge risk that starting to enforce mimetype on <script> (even if just on cross-site script) would break a large number of websites?

We can't reject no content type or text/plain (this should be obvious, sorry if I didn't make it clear enough). The idea is to reject an explicit application/json content type from the server, in the response to a script src= generated request. /be

The remaining "setter" attack was fixed in bug 474501, I think.