Closed Bug 419848 Opened 17 years ago Closed 17 years ago

XPCNativeWrapper pollution using chrome js


(Core :: Security, defect, P2)






(Reporter: moz_bug_r_a4, Assigned: mrbkap)



(Keywords: testcase, verified1.8.1.17, Whiteboard: [sg:critical])


(4 files, 2 obsolete files)

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.)
Flags: blocking1.9?
Flags: blocking1.8.1.13?
Keywords: testcase
OS: Windows XP → All
Hardware: PC → All
Whiteboard: [sg:critical]
Johnny, are you the best assignee for this?
Assignee: dveditz → jst
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.13?
Flags: blocking1.8.1.13+
Flags: tracking1.9? → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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...
Attachment #306639 - Flags: superreview?(jonas)
Attachment #306639 - Flags: review?(mrbkap)
Comment on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

Attachment #306639 - Flags: superreview?(jonas)
Attachment #306639 - Flags: superreview+
Attachment #306639 - Flags: review?(mrbkap)
Attachment #306639 - Flags: review+
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.
Attachment #306639 - Flags: approval1.9b4?
(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?

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.
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 on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

Attachment #306639 - Flags: approval1.9b4? → approval1.9b4+
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.
Attachment #306639 - Flags: approval1.9b4-
Attachment #306639 - Flags: approval1.9b4+
Attachment #306639 - Flags: approval1.9?
Fix checked in. No tests yet, if anyone has cycles, please step up.
Closed: 17 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
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
Resolution: FIXED → ---
Looks like a fix will need more trunk baking to catch regressions, blocking this week is unrealistic and unsafe.
Flags: blocking1.8.1.13+ → blocking1.8.1.14+
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.
Assignee: jst → mrbkap
Target Milestone: --- → mozilla1.9beta5
Attached patch Ewww (obsolete) — Splinter Review
I *really* wish the JS engine understood the concept of the "system" principal.
Attachment #306639 - Attachment is obsolete: true
Attachment #307584 - Flags: superreview?(jst)
Attachment #307584 - Flags: review?(brendan)
Please to be cc'ing me ;-).

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.

Attachment #307584 - Attachment is obsolete: true
Attachment #307785 - Flags: superreview?(jst)
Attachment #307785 - Flags: review?(brendan)
Attachment #307584 - Flags: superreview?(jst)
Attachment #307584 - Flags: review?(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 ;-).

Attachment #307785 - Flags: review?(brendan) → review+
Attachment #307785 - Flags: superreview?(jst) → superreview+
Fix checked in.
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical] → [sg:critical][needs branch patch]
This didn't make Pushing to Hopefully Blake will get to this when he's back.
Flags: blocking1.8.1.16+
Flags: blocking1.8.1.15-
Flags: blocking1.8.1.15+
Attached patch Backport to 1.8Splinter Review
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.
Attachment #326302 - Flags: superreview?(jst)
Attachment #326302 - Flags: review?(jst)
Attachment #326302 - Flags: superreview?(jst)
Attachment #326302 - Flags: superreview+
Attachment #326302 - Flags: review?(jst)
Attachment #326302 - Flags: review+
Attachment #326302 - Flags: approval1.8.1.16?
Whiteboard: [sg:critical][needs branch patch] → [sg:critical]
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.
Attachment #326302 - Flags: approval1.8.1.17? → approval1.8.1.17+
Fix checked into the 1.8 branch.
Keywords: fixed1.8.1.17
Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv: Gecko/2008081403 BonEcho/ for branch.
a=asac for
Attachment #336284 - Flags: approval1.8.0.15+
Flags: blocking1.8.0.15+
Group: core-security
I believe this caused the crash in bug 457788 (see regression window there).
Depends on: 452295
You need to log in before you can comment on or make changes to this bug.