419848 - XPCNativeWrapper pollution using chrome js

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[moz_bug_r_a4]

When content loads a script from a chrome: url by using a <script> element,
unlike chrome XBL binding, the script principal is the main document's
principal though the script filename is the chrome: url.  Thus, the fixes for
bug 369211, bug 387881, bug 411092 and bug 411093 can be circumvented. 

(A script that was loaded from the fastload file cannot be used for this bug's
trick, since its script principal is the system principal.)

Comment 5

[Daniel Veditz [:dveditz]]

Johnny, are you the best assignee for this?

Comment 6

[Johnny Stenback (:jst)]

Attachment: Disallow content from loading chrome:// JS

This stops content code from ever loading JS scripts, and probably fixes bug 419846 as well. We'd probably want this in ASAP, i.e. for beta4 if we can...

Comment 7

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

Comment on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

SOLD!

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

The sooner we get this in the more testing we'll get. So if at all possible I'd like this one for beta4.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to comment #8)
> (From update of attachment 306639 [details] [diff] [review])
> The sooner we get this in the more testing we'll get.

If so, why doesn't this patch come with at least an initial round of testing?

Comment 10

[Johnny Stenback (:jst)]

Like what? This isn't something we can test for here, we know what it might break, we just don't know if anything out there depends on that.

Comment 11

[Jeff Walden [:Waldo]]

Well, for example, there's no test of including a script element for some chrome: script and checking it fails, which seems like a prerequisite to guaranteeing it works for what it's supposed to do and will continue to work in the future.  I agree there's nothing to do about sites that actually try to use chrome: scripts and do so with good intentions except to wait for complaints.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

a1.9b4=beltzner

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

Didn't land in time for beta 4, and Jeff's comments about tests make me think we should take this in a post-b4 nightly, instead.

Comment 14

[Johnny Stenback (:jst)]

Fix checked in. No tests yet, if anyone has cycles, please step up.

Comment 15

[Johnny Stenback (:jst)]

Had to back this out, our feed code depends on this :(

Security Error: Content at about:feeds may not load or link to chrome://browser/content/feeds/subscribe.js

Comment 16

[Daniel Veditz [:dveditz]]

Looks like a fix will need more trunk baking to catch regressions, blocking 1.8.1.13 this week is unrealistic and unsafe.

Comment 17

[Johnny Stenback (:jst)]

Blake has a plan for this that doesn't involve restricting loading of chrome:// JS from content (which would still be a good idea), reassigning.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Ewww

I *really* wish the JS engine understood the concept of the "system" principal.

Comment 19

[Brendan Eich [:brendan]]

Please to be cc'ing me ;-).

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 307584 [details] [diff] [review]
Ewww

>+    if (aPrincipal) {
>+      uint32 flags = JS_GetScriptFilenameFlags(script);
>+      nsIScriptSecurityManager *ssm = nsContentUtils::GetSecurityManager();
>+
>+      // Use the principal for the filename if it shouldn't be receiving
>+      // implicit XPCNativeWrappers.
>+      PRBool system;
>+      if ((flags & JSFILENAME_PROTECTED) &&

Split into an if testing just that flag, and then get the ssm and do the IsSystemPrincipal call, to avoid costing the unprotected case the GetSecurityManager call?

>+        js_ComputeFilename(cx, caller, principals, &filename, &lineno);

May as well return filename directly, avoid an out param. Comment infallibility in .h file as usual.

/be

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 307785 [details] [diff] [review]
Updated to comments

>+js_ComputeFilename(JSContext *cx, JSStackFrame *caller,
>+                   JSPrincipals *principals, uintN *linenop)
>+{
>+    uint32 flags;
>+
>+    flags = JS_GetScriptFilenameFlags(caller->script);
>+    if (!(flags & JSFILENAME_PROTECTED) ||
>+        !strcmp(principals->codebase, "[System Principal]")) {
>+        *linenop = js_PCToLineNumber(cx, caller->script, caller->pc);
>+        return caller->script->filename;
>+    }
>+
>+    *linenop = 0;
>+    return principals->codebase;

I gave this nit in person but for the record: please invert the condition to (protected && not-system-principal) and put the odd (*linenop = 0; principals->codebase) case in the then clause, indented as punishment for its abnormality ;-).

/be

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm about to check in

Comment 24

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 25

[Samuel Sidler (old account; do not CC)]

This didn't make 1.8.1.15. Pushing to 1.8.1.16. Hopefully Blake will get to this when he's back.

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Backport to 1.8

Note that neither JSFILENAME_PROTECTED nor nsIScriptSecurityManager::IsSystemPrincipal exist on the 1.8 branch. Also, the changes (on trunk) to nsJSTimeoutHandler.cpp moved to nsGlobalWindow.cpp.

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 326302 [details] [diff] [review]
Backport to 1.8

>+        strcmp(principals->codebase, "[System Principal]")) {

Using a raw string vaguely worries me here, could we use a shared definition with CAPS so we don't have to worry about them getting out of sync?

Having js depend on external stuff (like caps) is verboten, but by using this string in this way you're implicitly depending on it anyway. Maybe js could export it to caps? At the very least we should comment in both places that they need to match.

Approved for 1.8.1.17, a=dveditz for release-drivers.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branch.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.17pre) Gecko/2008081403 BonEcho/2.0.0.17pre for branch.

Comment 30

[Alexander Sack]

Attachment: for 1.8.0 (just context adjustments)

a=asac for 1.8.0.15

Comment 31

[Karsten Düsterloh]

I believe this caused the crash in bug 457788 (see regression window there).