Last Comment Bug 419848 - XPCNativeWrapper pollution using chrome js
: XPCNativeWrapper pollution using chrome js
: testcase, verified1.8.1.17
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: All All
: P2 normal (vote)
: mozilla1.9beta5
Assigned To: Blake Kaplan (:mrbkap)
: David Keeler [:keeler] (use needinfo?)
Depends on: 452295 457788
  Show dependency treegraph
Reported: 2008-02-27 07:41 PST by moz_bug_r_a4
Modified: 2008-10-13 11:22 PDT (History)
13 users (show)
jst: blocking1.9+
samuel.sidler+old: blocking1.8.1.15-
samuel.sidler+old: blocking1.8.1.17+
dveditz: wanted1.8.1.x+
samuel.sidler+old: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Disallow content from loading chrome:// JS (2.12 KB, patch)
2008-02-29 17:47 PST, Johnny Stenback (:jst,
jonas: review+
jonas: superreview+
mbeltzner: approval1.9b4-
Details | Diff | Splinter Review
Ewww (6.83 KB, patch)
2008-03-05 14:40 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Updated to comments (6.92 KB, patch)
2008-03-06 12:11 PST, Blake Kaplan (:mrbkap)
brendan: review+
jst: superreview+
Details | Diff | Splinter Review
What I'm about to check in (6.91 KB, patch)
2008-03-06 14:50 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Backport to 1.8 (10.79 KB, patch)
2008-06-23 06:14 PDT, Blake Kaplan (:mrbkap)
jst: review+
jst: superreview+
dveditz: approval1.8.1.17+
Details | Diff | Splinter Review
for 1.8.0 (just context adjustments) (9.86 KB, patch)
2008-08-31 17:05 PDT, Alexander Sack
Details | Diff | Splinter Review

Description moz_bug_r_a4 2008-02-27 07:41:08 PST
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] 2008-02-27 12:02:00 PST
Johnny, are you the best assignee for this?
Comment 6 Johnny Stenback (:jst, 2008-02-29 17:47:03 PST
Created attachment 306639 [details] [diff] [review]
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 2008-02-29 17:48:14 PST
Comment on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

Comment 8 Johnny Stenback (:jst, 2008-02-29 17:49:13 PST
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] (remove +bmo to email) 2008-02-29 20:18:39 PST
(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, 2008-02-29 22:17:24 PST
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] (remove +bmo to email) 2008-02-29 22:55:52 PST
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] 2008-03-02 22:15:11 PST
Comment on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-03 06:57:55 PST
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, 2008-03-03 20:18:41 PST
Fix checked in. No tests yet, if anyone has cycles, please step up.
Comment 15 Johnny Stenback (:jst, 2008-03-03 21:41:04 PST
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] 2008-03-04 11:11:01 PST
Looks like a fix will need more trunk baking to catch regressions, blocking this week is unrealistic and unsafe.
Comment 17 Johnny Stenback (:jst, 2008-03-04 20:59:58 PST
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) 2008-03-05 14:40:53 PST
Created attachment 307584 [details] [diff] [review]

I *really* wish the JS engine understood the concept of the "system" principal.
Comment 19 Brendan Eich [:brendan] 2008-03-05 18:38:16 PST
Please to be cc'ing me ;-).

Comment 20 Brendan Eich [:brendan] 2008-03-05 19:44:30 PST
Comment on attachment 307584 [details] [diff] [review]

>+    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.

Comment 21 Blake Kaplan (:mrbkap) 2008-03-06 12:11:19 PST
Created attachment 307785 [details] [diff] [review]
Updated to comments
Comment 22 Brendan Eich [:brendan] 2008-03-06 12:17:08 PST
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 ;-).

Comment 23 Blake Kaplan (:mrbkap) 2008-03-06 14:50:39 PST
Created attachment 307822 [details] [diff] [review]
What I'm about to check in
Comment 24 Blake Kaplan (:mrbkap) 2008-03-06 14:53:10 PST
Fix checked in.
Comment 25 Samuel Sidler (old account; do not CC) 2008-06-05 14:57:05 PDT
This didn't make Pushing to Hopefully Blake will get to this when he's back.
Comment 26 Blake Kaplan (:mrbkap) 2008-06-23 06:14:29 PDT
Created attachment 326302 [details] [diff] [review]
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] 2008-07-14 11:58:57 PDT
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, a=dveditz for release-drivers.
Comment 28 Blake Kaplan (:mrbkap) 2008-07-15 03:13:40 PDT
Fix checked into the 1.8 branch.
Comment 29 Al Billings [:abillings] 2008-08-14 18:33:44 PDT
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/2008081403 BonEcho/ for branch.
Comment 30 Alexander Sack 2008-08-31 17:05:25 PDT
Created attachment 336284 [details] [diff] [review]
for 1.8.0 (just context adjustments)

a=asac for
Comment 31 Karsten Düsterloh 2008-10-09 22:53:58 PDT
I believe this caused the crash in bug 457788 (see regression window there).

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