The default bug view has changed. See this FAQ.

XPCNativeWrapper pollution using chrome js

RESOLVED FIXED in mozilla1.9beta5

Status

()

Core
Security
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({testcase, verified1.8.1.17})

unspecified
mozilla1.9beta5
testcase, verified1.8.1.17
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
blocking1.8.1.15 -
blocking1.8.1.17 +
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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+

Updated

9 years ago
Flags: tracking1.9? → blocking1.9?

Updated

9 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
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...
Attachment #306639 - Flags: superreview?(jonas)
Attachment #306639 - Flags: review?(mrbkap)
Comment on attachment 306639 [details] [diff] [review]
Disallow content from loading chrome:// JS

SOLD!
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

a1.9b4=beltzner
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.
Status: NEW → RESOLVED
Last Resolved: 9 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Looks like a fix will need more trunk baking to catch regressions, blocking 1.8.1.13 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
Status: REOPENED → NEW
Attachment #306639 - Flags: approval1.9?
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9beta5
(Assignee)

Comment 18

9 years ago
Created attachment 307584 [details] [diff] [review]
Ewww

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 ;-).

/be
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
(Assignee)

Comment 21

9 years ago
Created attachment 307785 [details] [diff] [review]
Updated to comments
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 ;-).

/be
Attachment #307785 - Flags: review?(brendan) → review+

Updated

9 years ago
Attachment #307785 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 23

9 years ago
Created attachment 307822 [details] [diff] [review]
What I'm about to check in
(Assignee)

Comment 24

9 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical] → [sg:critical][needs branch patch]
This didn't make 1.8.1.15. Pushing to 1.8.1.16. 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+
(Assignee)

Comment 26

9 years ago
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.
Attachment #326302 - Flags: superreview?(jst)
Attachment #326302 - Flags: review?(jst)

Updated

9 years ago
Attachment #326302 - Flags: superreview?(jst)
Attachment #326302 - Flags: superreview+
Attachment #326302 - Flags: review?(jst)
Attachment #326302 - Flags: review+
(Assignee)

Updated

9 years ago
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 1.8.1.17, a=dveditz for release-drivers.
Attachment #326302 - Flags: approval1.8.1.17? → approval1.8.1.17+
(Assignee)

Comment 28

9 years ago
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:1.8.1.17pre) Gecko/2008081403 BonEcho/2.0.0.17pre for branch.
Keywords: fixed1.8.1.17 → verified1.8.1.17

Comment 30

9 years ago
Created attachment 336284 [details] [diff] [review]
for 1.8.0 (just context adjustments)

a=asac for 1.8.0.15
Attachment #336284 - Flags: approval1.8.0.15+

Updated

9 years ago
Flags: blocking1.8.0.15+
Group: core-security
I believe this caused the crash in bug 457788 (see regression window there).
Depends on: 457788
Depends on: 452295
You need to log in before you can comment on or make changes to this bug.