375250 - Reject (JSON is fixed now) E4X masquerading as JS source

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jesse Ruderman]

Brendan pointed out that a malicious page could use a custom XML constructor to read the contents of any well-formed XHTML page.  (Behind firewalls, as sent to a logged-in user, etc.)

The attack would be similar to a well-known attack against sites that return plain JSON data at some URLs (e.g. for non-XML use of XMLHttpRequest).  In that attack, a plain-JSON URL from a victim site is loaded into a page that uses a custom Object or Array constructor.  This attack is similar but uses a custom XML constructor instead, and would work against many more sites.

I suggested preventing this attack by only supporting XML literals in scripts served with MIME types that are reasonable for JavaScript.  Brendan says that's "{application,text}/{ecma,java}script per RFC 4329".

This would create three levels of E4X literal support depending on mime type:
  text/javascript;e4x=1  --  element and comment literals
  text/javascript        --  element literals
  other                  --  no e4x literals
but I think that's acceptable.

Any sites that get broken by this change (sites using E4X literals *and* a bogus mime type for JavaScript) could get a useful message like "E4X element literals can only be used when the script's mime type is something like text/javascript" in the error console.

Security-sensitive because I don't know whether this affects browsers other than Firefox.  It doesn't affect Firefox yet because SpiderMonkey does not yet support custom XML constructors (even though the E4X spec requires it).

Comment 1

[Brendan Eich [:brendan]]

It's not necessary to customize the XML constructor and fix SpiderMonkey to follow E4X's spec to call the current binding of XML via operator new when evaluating an XML initialiser -- it's enough to just generate a script tag loading an XML file inside a firewall, or otherwise accessed via some XSS or CSRF-like loophole. This bug's on the right track: we shouldn't process such content as JS, let alone E4X, if it isn't served with an RFC 4329 MIME type.

/be

Comment 2

[Jesse Ruderman]

Are you saying this can be exploited without custom XML constructors, where the XML is just part of a "nearly-useless" expression statement?  How?  Is that true for plain-JSON too?

Comment 3

[Brendan Eich [:brendan]]

Sorry, I thought I saw a way, but the value of the expression statement can't be recovered. We are saved by our (unintended) bug of not constructing XML literals using the current value of the XML constructor. Whew!

/be

Comment 4

[Jesse Ruderman]

See also bug 376957, "Prevent data leaks from cross-site JSON loads (JavaScript literals)".

Comment 5

[Jesse Ruderman]

Brendan, are we going to make the change you suggested in comment 1 for Gecko 1.9?  2.0?

Comment 6

[chris hofmann]

getting late for 1.9.  should decide soon.

Comment 7

[Brendan Eich [:brendan]]

I'm not going to have time for this. If this bug is morphing into what I wrote up in comment 1, it's really a content bug, not a JS engine bug. Cc'ing sicking.

/be

Comment 8

[Brendan Eich [:brendan]]

This still seems low-priority to me, given the inability to override the XML constructor used by JS when parsing E4X literals, and the inability to get a whole expression statement evaluated into a value that the bad guy can extract. Jesse, let me know if these "givens" are weak or wrong.

/be

Comment 9

[Mike Schroepfer]

Moving off nom list given comment 7

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So we currently don't check the mimetype at all of remotely loaded scripts. I would suggest that we create a whitelist of mimetypes and if the server sent mimetype doesn't specify javascript, possibly even javascript with e4x, we don't allow e4x at all, but we do still execute the javascript.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Giving to myself since this scares me a little. But given that there are no known exploits giving a low priority for now.

Comment 12

[Brendan Eich [:brendan]]

We want any program loaded via a <script src=> tag that consists entirely of a single object, array, or XML initialiser to be an error. This violates the Ecma spec, but such programs are useless and probably an attack such as the twitter one:

http://www.thespanner.co.uk/2009/01/07/i-know-what-your-friends-did-last-summer/

If the "script" is actually loaded with a known JS MIME type, then we could allow this -- for whatever reason, the page author asked for it and the sourced URL was served with a JS content type, so it probably is kosher.

It's unlikely that sensitive JSON or XML data was returned due to a misconfigured server with a content type of application/javascript. But we could reject single literal programs sourced with a JS type too, just to be paranoid. If an existing web app misloaded a literal and it harmlessly worked in past and current Firefox releases, we'd just be putting an error in the console. The next script tag in the page would load without the proposed error causing problems. Comments?

/be

Comment 13

[Brendan Eich [:brendan]]

We must not break the JS API, which allows JS_EvaluateScript(..., "[1,2,3]", ...) and the like. XBL relies on this.

If we also want to do this only for sourced scripts served with non-JS content types, then this patch is better done in content/base/src/nsScriptLoader.cpp, and sicking is the perfect owner :-P. I'd rather we do it there to start with anyway, as other JS embeddings don't need the hair in SpiderMonkey.

/be

Comment 14

[Giorgio Maone [:ma1]]

FYI, I just implemented a countermeasure against both JSON and E4X hijacking attempts in latest NoScript development build, 1.8.8.95 from http://noscript.net/getit#devel

NoScript now registers a filtering listener on cross-site script requests, to check if the source code starts (spaces aside) with "[" or "<". 
If either it starts with "[" and can be parsed as valid JSON, or it just starts with "<" (assuming XML), the request gets neutered.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Whatever fix we deploy for this we need to apply to the web-workers importScript API. It's essentially the same thing as <script src=>, just with a different API. But it has exactly the same security model.

Comment 16

[Johnny Stenback (:jst)]

So after talking to Brendan and Jonas it seems like what we need to do here is file a bug on the JS engine to add an API to check whether a compiled script is a pure JSON literal, and if so drop it on the floor, and if it isn't we'll just evaluate the already compiled script. That way we don't really end up doing any more work than we are doing right now. In addition, we should probably also always refuse to execute scripts loaded with a application/json mimetype, as an additional guard here. Though doing that may prevent legitimate (though that's probably questionable) use of application/json for sending things like x={...} in a script file for cross origin communication purposes etc.

Ben, since you're low on blockers, I'll let you investigate this. But let me know if you need help, or if you have other more important stuff taking up all your time :)

Comment 17

[Johnny Stenback (:jst)]

Ben, please see above comment.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #16)
> So after talking to Brendan and Jonas it seems like what we need to do here is
> file a bug on the JS engine to add an API to check whether a compiled script is
> a pure JSON literal, and if so drop it on the floor, and if it isn't we'll just
> evaluate the already compiled script.

Strongly suggest that we do *not* add a JS_Compile* API, since that will (a) lose JSOPTION_COMPILE_N_GO optimizations by default (i.e., without extra option-setting code). Instead, how about an option to reject "pure" [1] JSON literals as whole programs? JSOPTION_NO_JSON_LITERAL_PROGRAMS would be the clearest name.

Then we avoid the compile+execute API overhead, the loss of compile-n-go without explicit option-setting advice optimization, and the complexity of compile API usage followed by JS_ExecuteScript.

/be

Comment 19

[Brendan Eich [:brendan]]

[1] Programs whose source compiles as a JSON (RFC 4627, http://www.ietf.org/rfc/rfc4627.txt) literals, ignoring possible effects achieved via prototype setter usage.

BTW, ES3.1 seems to specify array literal property initialisation as bypassing the prototype and potential setters there, so we should follow that spec too. Separate JS bug, please file.

/be

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #19)
> BTW, ES3.1 seems to specify array literal property initialisation as bypassing
> the prototype and potential setters there, so we should follow that spec too.
> Separate JS bug, please file.

Brendan, does this apply to both array and object literals? If so, wouldn't just changing that behavior be enough to fix the security issue here?

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #20)
> (In reply to comment #19)
> > BTW, ES3.1 seems to specify array literal property initialisation as bypassing
> > the prototype and potential setters there, so we should follow that spec too.
> > Separate JS bug, please file.
> 
> Brendan, does this apply to both array and object literals?

Yes.

> If so, wouldn't
> just changing that behavior be enough to fix the security issue here?

I think so, but we need to be careful (watch points, some other extension or combination that we haven't thought of yet). Defense in depth says your pants *will* fall down without belt and suspenders. Murphy was an optimist.

/be

Comment 22

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Per discussion with brendan, jst, and sicking, we're going to bump this down to P2 and wait on fixing the bug mentioned in comment 19, bug 474501.

Comment 23

[Johnny Stenback (:jst)]

This is belts and braces for the security bug at large here, the JS engine side of things will be fixed, and that should be enough for Firefox 3.1. Not blocking on this one.

Comment 24

[Brendan Eich [:brendan]]

Morphing slightly.

/be

Comment 25

[Brendan Eich [:brendan]]

Taking, since we need a JS-level fix to be centralize and minimize the check. Comment 23 is really talking about JSON stealing prevention, which has been done as noted in the dependencies and Summary. But E4X is not protected yet, and it's not possible to protect in the same way.

My plan is to break E4X if the ;e4x=1 MIME parameter is not used on one of the known JavaScript script types.

/be

Comment 26

[Brendan Eich [:brendan]]

Attachment: proposed fix

NB: this will break javascript:<foo>xml only here!</foo> and the like.

/be

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #25)
> My plan is to break E4X if the ;e4x=1 MIME parameter is not used on one of the
> known JavaScript script types.

I changed my mind, as should be obvious from the patch. I don't want to rely on MIME type or parameter. This uses brute force and simply makes any program that consists entirely of XML literals (comment, CDATA, PI, element, even the E4X-only <><a/><b/><c/></> XMLList literal, which is not valid XML) an error.

/be

Comment 28

[Igor Bukanov]

(In reply to comment #27)
> This uses brute force and simply makes any program that
> consists entirely of XML literals (comment, CDATA, PI, element, even the
> E4X-only <><a/><b/><c/></> XMLList literal, which is not valid XML) an error.

The patch also rejects programs that consists of XML with embedded expressions:

<xml>{print("Hello from XML")}</xml>

These can have observable side-effects. In addition the patch breaks currently held invariant that uneval(xml) returns something that can be passed to eval. This suggests to disable whole-xml scripts only when they come from the <script> tag, not for each and every compileScript invocation.

Comment 29

[Igor Bukanov]

(In reply to comment #18)
> JSOPTION_NO_JSON_LITERAL_PROGRAMS would be the clearest name.

Given the above it should simply be JSOPTION_NO_LITERAL_PROGRAMS to reject any literal-only programs.

Comment 30

[Igor Bukanov]

Comment on attachment 374958 [details] [diff] [review]
proposed fix

>+    if (xmlTrees == allTrees) {

This rejects an empty program.

Comment 31

[Brendan Eich [:brendan]]

Attachment: fix, v2

D'oh -- that's why I added counters, but I forgot to check allTrees != 0.

/be

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 374977 [details] [diff] [review]
fix, v2

Will rework to change the right case and only that case. New patch in a bit.

/be

Comment 33

[Brendan Eich [:brendan]]

Attachment: fix, v3

Notice that content/base/src/nsScriptLoader.cpp passes null for aRetValue to nsJSContext::EvaluateString. This patch tidies up that method to null-check aIsUndefined consistently, and to use standard underhanging multiline arguments indentation.

/be

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 374997 [details] [diff] [review]
fix, v3

Looking for extra DOM review.

/be

Comment 35

[Igor Bukanov]

Comment on attachment 374997 [details] [diff] [review]
fix, v3

>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>+#if JS_HAS_XML_SUPPORT
>+    uintN allTrees = 0, xmlTrees = 0;
>+#endif

Nit: this can be replaced with one boolean flag like onlyXmlTrees and initializing pn to NULL so the script can be rejected after the loop if pn && onlyXmlTrees.

Comment 36

[Brendan Eich [:brendan]]

Attachment: fix, v4

Refreshed, and with Igor's suggestion.

/be

Comment 37

[Brendan Eich [:brendan]]

Attachment: fix, v5

Important bug fix!

No longer testable in the shell directly -- you have to load("/tmp/only.xml") where that file contains, e.g., <x/> all by itself.

/be

Comment 38

[Brendan Eich [:brendan]]

Comment on attachment 375035 [details] [diff] [review]
fix, v5

Asking for Igor's review again.

/be

Comment 39

[Igor Bukanov]

Comment on attachment 375035 [details] [diff] [review]
fix, v5

>+#if JS_HAS_XML_SUPPORT
>+        if (PN_TYPE(pn) != TOK_SEMI || !TREE_TYPE_IS_XML(PN_TYPE(pn->pn_kid)))
>+            onlyXML = false;
>+#endif

Now a valid script script like ;;;; will report an error about xml-only script.

Comment 40

[Brendan Eich [:brendan]]

(In reply to comment #39)
> (From update of attachment 375035 [details] [diff] [review])
> >+#if JS_HAS_XML_SUPPORT
> >+        if (PN_TYPE(pn) != TOK_SEMI || !TREE_TYPE_IS_XML(PN_TYPE(pn->pn_kid)))
> >+            onlyXML = false;
> >+#endif
> 
> Now a valid script script like ;;;; will report an error about xml-only script.

No, it's worse: pn->pn_kid is null is such cases. But if it were non-null and not of XML type, then onlyXML would be set to false and there'd be no problem. New patch anon.

/be

Comment 41

[Brendan Eich [:brendan]]

Attachment: fix, v6

Comment 42

[Brendan Eich [:brendan]]

A few nits beyond, no need for v7 -- here's the interdiff:

diff -u b/js/src/jsscan.cpp b/js/src/jsscan.cpp
--- b/js/src/jsscan.cpp
+++ b/js/src/jsscan.cpp
@@ -1566,7 +1566,7 @@
          *
          * But this still leaves XML resources with particular line structure
          * vulnerable to being loaded as script, so for Firefox 3.5 and beyond,
-         * we reject programs whose entire source consists of XML literal. See:
+         * we reject programs whose source consists only of XML literals. See:
          *
          * https://bugzilla.mozilla.org/show_bug.cgi?id=336551
          *
diff -u b/js/src/jsscan.h b/js/src/jsscan.h
--- b/js/src/jsscan.h
+++ b/js/src/jsscan.h
@@ -151,7 +151,7 @@
 #define TOKEN_TYPE_IS_XML(tt) \
     ((tt) == TOK_AT || (tt) == TOK_DBLCOLON || (tt) == TOK_ANYNAME)
 
-#define TREE_TYPE_IS_XML(tt) \
+#define TREE_TYPE_IS_XML(tt)                                                  \
     ((tt) == TOK_XMLCOMMENT || (tt) == TOK_XMLCDATA || (tt) == TOK_XMLPI ||   \
      (tt) == TOK_XMLELEM || (tt) == TOK_XMLLIST)
 
/be

Comment 43

[Brendan Eich [:brendan]]

Attachment: interdiff v6 and what I want to commit

Sorry, that last inline interdiff wrapped badly -- never again.

/be

Comment 44

[Brendan Eich [:brendan]]

Fixed:

http://hg.mozilla.org/tracemonkey/rev/07674a45272a
http://hg.mozilla.org/mozilla-central/rev/38134a98f9ef

/be

Comment 45

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c4ba70070012