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

RESOLVED FIXED in mozilla1.9.1

Status

()

P2
major
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: jruderman, Assigned: brendan)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:want] fixed-in-tracemonkey)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

12 years ago
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
(Reporter)

Comment 2

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

Comment 3

12 years ago
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
(Reporter)

Comment 4

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

Updated

11 years ago
Depends on: 376957
(Reporter)

Comment 5

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

Comment 6

11 years ago
getting late for 1.9.  should decide soon.
Flags: blocking1.9?
(Assignee)

Comment 7

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

Comment 8

11 years ago
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

11 years ago
Moving off nom list given comment 7
Flags: blocking1.9? → blocking1.9-
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.
Giving to myself since this scares me a little. But given that there are no known exploits giving a low priority for now.
Assignee: general → jonas
Flags: blocking1.9- → blocking1.9+
Priority: -- → P5
Flags: tracking1.9+ → wanted-next+
Priority: P5 → --
(Reporter)

Updated

11 years ago
Group: core-security
(Assignee)

Comment 12

10 years ago
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
Severity: normal → major
Flags: blocking1.9.1?
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Summary: Reject E4X literals in scripts served with non-script mime types → Reject JSON and E4X masquerading as JS source
Target Milestone: --- → mozilla1.9.1
(Assignee)

Comment 13

10 years ago
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
Assignee: jonas → nobody
Component: JavaScript Engine → Content
QA Contact: general → content
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.
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.
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 :)
Flags: blocking1.9.1? → blocking1.9.1+
Ben, please see above comment.
Assignee: nobody → bent.mozilla
(Assignee)

Comment 18

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

Comment 19

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

Comment 21

10 years ago
(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
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.
No longer depends on: 474501
Priority: P1 → P2
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.
No longer depends on: 474501
Flags: blocking1.9.1+ → blocking1.9.1-
Whiteboard: [sg:want]
(Assignee)

Updated

10 years ago
Blocks: 336551
(Assignee)

Comment 24

10 years ago
Morphing slightly.

/be
Flags: blocking1.9.1- → blocking1.9.1?
Summary: Reject JSON and E4X masquerading as JS source → Reject (JSON is fixed now) E4X masquerading as JS source
(Assignee)

Updated

10 years ago
Assignee: bent.mozilla → general
Status: NEW → ASSIGNED
Component: Content → JavaScript Engine
Priority: P2 → P1
QA Contact: content → general
(Assignee)

Comment 25

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

Comment 26

10 years ago
Posted patch proposed fix (obsolete) — Splinter Review
NB: this will break javascript:<foo>xml only here!</foo> and the like.

/be
Assignee: general → brendan
Attachment #374958 - Flags: review?(igor)
(Assignee)

Comment 27

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

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

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

10 years ago
Comment on attachment 374958 [details] [diff] [review]
proposed fix

>+    if (xmlTrees == allTrees) {

This rejects an empty program.
Attachment #374958 - Flags: review?(igor) → review-

Updated

10 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Priority: P1 → P2
(Assignee)

Comment 31

10 years ago
Posted patch fix, v2 (obsolete) — Splinter Review
D'oh -- that's why I added counters, but I forgot to check allTrees != 0.

/be
Attachment #374958 - Attachment is obsolete: true
Attachment #374977 - Flags: review?(igor)
(Assignee)

Comment 32

10 years ago
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
Attachment #374977 - Attachment is obsolete: true
Attachment #374977 - Flags: review?(igor)
(Assignee)

Comment 33

10 years ago
Posted patch fix, v3 (obsolete) — Splinter Review
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
Attachment #374997 - Flags: review?(igor)
(Assignee)

Comment 34

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

Looking for extra DOM review.

/be
Attachment #374997 - Flags: review?(mrbkap)

Comment 35

10 years ago
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.
Attachment #374997 - Flags: review?(igor) → review+
(Assignee)

Comment 36

10 years ago
Posted patch fix, v4 (obsolete) — Splinter Review
Refreshed, and with Igor's suggestion.

/be
Attachment #374997 - Attachment is obsolete: true
Attachment #375032 - Flags: review?(mrbkap)
Attachment #374997 - Flags: review?(mrbkap)
(Assignee)

Comment 37

10 years ago
Posted patch fix, v5 (obsolete) — Splinter Review
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
Attachment #375032 - Attachment is obsolete: true
Attachment #375035 - Flags: review?(mrbkap)
Attachment #375032 - Flags: review?(mrbkap)
(Assignee)

Comment 38

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

Asking for Igor's review again.

/be
Attachment #375035 - Flags: review?(igor)

Comment 39

10 years ago
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.
Attachment #375035 - Flags: review?(igor) → review-
(Assignee)

Comment 40

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

Comment 41

10 years ago
Posted patch fix, v6Splinter Review
Attachment #375035 - Attachment is obsolete: true
Attachment #375042 - Flags: review?(igor)
Attachment #375035 - Flags: review?(mrbkap)
(Assignee)

Updated

10 years ago
Attachment #375042 - Flags: review?(mrbkap)

Updated

10 years ago
Attachment #375042 - Flags: review?(igor) → review+
(Assignee)

Comment 42

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

Comment 43

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

/be
Attachment #375042 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 44

10 years ago
Fixed:

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

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [sg:want] → [sg:want] fixed-in-tracemonkey

Updated

10 years ago
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.