Closed Bug 923902 Opened 11 years ago Closed 4 years ago

Let's secure chrome internal pages with Content Security Policy

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1449872

People

(Reporter: freddy, Assigned: decoder)

References

(Depends on 2 open bugs)

Details

(Whiteboard: [IdeasIntoAction])

Attachments

(2 files, 2 obsolete files)

Log output from stdout and the browser console
(deleted), text/plain
Details
csp.patch
(deleted), patch
Details | Diff | Splinter Review 
Let's secure chrome internal pages with Content Security Policy.
Historically, we have had XSS issues in a lot of new features that are behind chrome pages (i.e., reader, newtab, ..).
No longer blocks: 663570
Depends on: 663570
Thanks Masatoshi for changing the fields ;-)
Adding a few more people to CC..
This here defines how about: pages are handled:
http://mxr.mozilla.org/mozilla-central/source/browser/metro/components/AboutRedirector.js

This part is what bypasses nsIContentPolicy checks for system principals right now:
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentPolicyUtils.h#158
Devdatta Akhawe said it's detrimental for performance to remove this bypass, so we might just go with a whitelist of pages to not bypass this check for.
Depends on: 923920
I had only guessed it was a performance optimization. I think someone like bz would know why nsICP isn't enforced for system principals. It is also hard to guess what the compat hit would be.

You can just try enforcing it and see what does and doesn't break and if the perf tests complain. This is a broader change beyond CSP though: maybe retitle the bug? For example, I would love it if the internal pages also blocked load of scripts over http (mixed-content blocker).
I've talked to Olli and he said it's indeed done for performance reasons. We discussed this a bit and I will try to implement a whitelist approach. That means we're going to match the nsIURI loaded and if it's on the whitelist, apply CSP.

Not sure if I'll succeed with that, but I'll give it a try.
Attached patch WIP patch (doesn't actually work ^_^) (obsolete) (deleted) — — Splinter Review 
This patch is WIP. It modifies the short-circuit code to not return early if the page URI matches something we want to secure. Right now, about:memory is hardcoded in there. Furthermore, this code adds a CSP to each matched chrome page in InitCSP. I recycled the existing pref for privileged apps and you might need to set that pref.

Unfortunately inline scripts still work afterwards and Sid said he could throw some of his debugging magic on it to tell me what I'm missing. It's surely something simple and embarrassing ;)
Assignee: nobody → choller
Status: NEW → ASSIGNED
Attachment #814236 - Flags: feedback?(sstamm)
Comment on attachment 814236 [details] [diff] [review]
WIP patch (doesn't actually work ^_^)

At first I thought that you might need to modify nsIContentSecurityPolicy's getAllowsInlineScript method. But you're setting the policy on the document itself so as long as the policy is being parsed correctly, the script checks should work.

You could look at what getAllowsInlineScript is returning in the CSP code.. you might need to add an extra check somewhere, it's definitely possible that chrome documents don't bother checking the CSP before executing the script.

The check at http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsScriptLoader.cpp#625 is a good place to debug IMO, you could see if this is getting hit and what it's returning...
grobinson has spent a fair amount of time digging into inline scripts and CSP for his in-progress script-nonce work, he might be a good person to ask as well.. ;)
I'm a bit skeptical of this. IMO, if something doesn't need to do anything privileged, we just shouldn't be loading it with System Principal. And if it does, no amount of CSP will prevent an XSSed page from accessing privileged things, which tend not to be at all sandboxed from other privileged things.

Can we just load these pages with a different principal?
I couldn't come up with a use case where this would help a lot either, that's why I CC-ed you. Also according to http://www.w3.org/TR/CSP/#processing-model "Enforcing a CSP policy should not interfere with the operation of user-supplied scripts such as third-party user-agent add-ons and JavaScript bookmarklets.".

Conceptually system principal means in gecko that 'pass all security check', so if we want to add any kind of restrictions like CSP to these pages, I would prefer inventing a different principal than restricting system.
The use case for this is simple: If a chrome page works with untrusted content and doesn't sanitize it properly before e.g. displaying it, then that opens the door for cross site scripting. We've had these vulnerabilities before, in newtab and in the Android reader mode (which is now unprivileged, but it was privileged at that time).

At least for the latter attack I know that CSP would have prevented it. Note that this does not prevent all forms of attacks, it's still possible to construct a vulnerable page where CSP wouldn't help you (just like it's possible to have web pages where XSS works in the presence of CSP). But it mitigates some cases at least.

Chrome uses CSP by default for all of their extensions but I also think they use it for their internal (privileged) pages.

Maybe Brian can also add something here.
Yeah, thinking about it, CSP would help in certain situations to prevent malicious script from being executed (like the script-src directive, along with the implicit restriction on eval). Effectively, it amounts to the chrome page giving up certain DOM features as more risk than they're worth.

Other CSP features, like connect-src, wouldn't do a damn thing, of course. I wonder if we could disable those checks for privileged scopes or something.
We talked to Olli and he also considered inventing a new Principal. Decoder and I are not the experts here, so your feedback is highly appreciated. But we should make sure we do not misunderstand the attack vectors here:

For internal websites that simply don't *need* the privileges, we should of course stop running them with a System principal. The AboutRedirector file lined in comment 3 shows that some about pages run without high privileges. That surely mitigates one part of the problems (and is not part of this bug; we should indeed regularly audit our internal pages and make sure they're all least-privileged).

There are some websites which really need to call privileged JavaScript, like about:newtab which reads the History and about:memory which sees other internal stuff. It makes a lot of sense to protect those websites from Cross-Site Scripting themselves. A good way to achieve this would be a CSP that disallows inline JavaScript completely (and if it requires a tiny bit of rewriting, I have just volunteered to take care of that). We also don't really care about external images and some other CSP features. If there's an easier way to impose those restriction with our current nsIContentPolicy interfaces, let's totally do that instead!
(In reply to Frederik Braun [:freddyb] from comment #13)
> We talked to Olli and he also considered inventing a new Principal.

Speaking as the module owner here (CAPS), I'm not wild about this idea.

> For internal websites that simply don't *need* the privileges, we should of
> course stop running them with a System principal. The AboutRedirector file
> lined in comment 3 shows that some about pages run without high privileges.
> That surely mitigates one part of the problems (and is not part of this bug;
> we should indeed regularly audit our internal pages and make sure they're
> all least-privileged).

Agreed. I believe about:home is one example of an unprivileged in-browser page.

> There are some websites which really need to call privileged JavaScript,
> like about:newtab which reads the History and about:memory which sees other
> internal stuff. It makes a lot of sense to protect those websites from
> Cross-Site Scripting themselves. A good way to achieve this would be a CSP
> that disallows inline JavaScript completely (and if it requires a tiny bit
> of rewriting, I have just volunteered to take care of that). We also don't
> really care about external images and some other CSP features. If there's an
> easier way to impose those restriction with our current nsIContentPolicy
> interfaces, let's totally do that instead!

Can someone explain to me why we can't make inline script protections apply to the system principal?
(In reply to Bobby Holley (:bholley) from comment #14)
> Can someone explain to me why we can't make inline script protections apply
> to the system principal?

I thought about this and I realised that my concerns here were totally invalid. Letting CSP apply for chrome pages seems just fine to me. Addons those might want to extend these pages use evalInSandbox anyway, which will still work just fine. So adding a new principal makes no sense. The original approach seems just fine.
OK, going to pick up this thread again. We now agree that we don't need a new principal. How, implementation wise, would one apply inline script protections to the sys principal? Would it be just for some documents or for all?
(In reply to Frederik Braun [:freddyb] from comment #16)
> OK, going to pick up this thread again. We now agree that we don't need a
> new principal. How, implementation wise, would one apply inline script
> protections to the sys principal? Would it be just for some documents or for
> all?

Yes, the system principal is a singleton, so this would need to live on scope. It could probably go on xpc::Scriptability, which lives on the compartment private.
Attached patch bug-923902.patch (WIP & doesn't work) (obsolete) (deleted) — — Splinter Review 
I have unbitrotten decoder's patch, but there seems to be something missing. There must be another method that's bypassing the CSP implementation for our example. It might be something with SystemPrincipals, XUL Pages, about URIs.. :)

I am attaching the patch itself as well as the debug output with the `security.csp.debug pref` set to true.

Feedback appreciated.
Attachment #814236 - Attachment is obsolete: true
Attachment #814236 - Flags: feedback?(sstamm)
Attachment #8345889 - Flags: feedback?(grobinson)
Comment on attachment 8345889 [details] [diff] [review]
bug-923902.patch (WIP & doesn't work)

Maybe Olli knows?
Attachment #8345889 - Flags: feedback?(bugs)
Comment on attachment 8345889 [details] [diff] [review]
bug-923902.patch (WIP & doesn't work)


>   rv = principal->SetCsp(csp);
>   NS_ENSURE_SUCCESS(rv, rv);
> #ifdef PR_LOGGING
>   PR_LOG(gCspPRLog, PR_LOG_DEBUG,
>          ("Inserted CSP into principal %p", principal));
> #endif
>+    nsAutoCString aspec;
>+    nsIURI* uriptr;
>+
>+    principal->GetURI(&uriptr);
You leak uriptr here


>+bool nsDocument::IsChromeCSPDocument() {
bool
nsDocument::IsChromeCSPDocument()
{



Not clear to me what kind of feedback you're asking.
Attachment #8345889 - Flags: feedback?(bugs)
My question was stated in comment 18: Is there another bail-out criterion we're missing that causes us the CSP not to work?
not work in which case? In the inline <script> alert() </script> case?
Yes. Basic inline JavaScript in about:memory (patched into aboutMemory.xhtml as per the attachment).
The policy I'm attaching to the document (through a pref) is |default-src: 'self'|..
Thanks for the pointers Olli, that was exactly the missing piece of the puzzle. After I copied GetCSP/SetCSP from nsPrincipal, my patch is working. Now I guess the next steps would be to figure out how we want to maintain the list of chrome pages to protect.

And of course I don't know if the change I made to nsSystemPrincipal is ok, but I assume it's not a problem (performance wise) to have GetCSP/SetCSP like in nsPrincipal if in other places, system pages are exempted from CSP anyways.
I totally missed bholley's comment. So we cannot use nsSystemPrincipal. I'll look into xpc::Scriptability tomorrow :)
I looked over all of the code today that calls GetCsp and it seems cleaner and more feasible to me to introduce a second system principal that we restrict with a single CSP (we want one CSP for all chrome pages anyway).

@bholley,smaug: Can you elaborate on why we would or would not want to do it that way? I also looked at xpc::Scriptability and I didn't understand how to get the scope object in all places where CSP is used.
(In reply to Christian Holler (:decoder) from comment #28)
> I looked over all of the code today that calls GetCsp and it seems cleaner
> and more feasible to me to introduce a second system principal that we
> restrict with a single CSP (we want one CSP for all chrome pages anyway).
> 
> @bholley,smaug: Can you elaborate on why we would or would not want to do it
> that way?

In general, adding a second system principal will break the a lot of stuff, because there's a lot of code that assumes it's a singleton and that pointer comparisons will work. I really don't think we want to do this.

> I also looked at xpc::Scriptability and I didn't understand how to
> get the scope object in all places where CSP is used.

If you give me some examples of the places you're confused about, I can point you in the right direction.
(In reply to Bobby Holley (:bholley) from comment #29)
> 
> If you give me some examples of the places you're confused about, I can
> point you in the right direction.

Sounds great, thanks!

I was looking for example at these places:

http://mxr.mozilla.org/mozilla-central/source/caps/src/nsScriptSecurityManager.cpp#377 (Principal seems to be taken directly out of the JSContext).

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1658 (Principal is already part of the class)

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.cpp#339 (No clue what exactly this does :D)

But even for the regular case where we have a doc and take the principal from there, I still don't understand how it would work with xpc::Scriptability. I would need to get this scope object first right?

I first had the idea to put a CSP right into nsDocument itself, but for the cases mentioned above, it isn't possible to get a doc either (or at least it seems so).
(In reply to Christian Holler (:decoder) from comment #30)

> http://mxr.mozilla.org/mozilla-central/source/caps/src/
> nsScriptSecurityManager.cpp#377 (Principal seems to be taken directly out of
> the JSContext).

You can just get the global via JS::CurrentGlobalOrNull(cx), or the compartment via js::GetContextCompartment(cx);

Alternatively, you can just use mozilla::dom::GetIncumbentGlobal, which will give you the global corresponding to the incumbent settings object. What does the CSP spec say to do?

> http://mxr.mozilla.org/mozilla-central/source/content/base/src/
> nsXMLHttpRequest.cpp#1658 (Principal is already part of the class)

XHR is a little tricky. But you should be able to first check IsSystemXHR, and if not, use the scope of the document. 

> http://mxr.mozilla.org/mozilla-central/source/layout/style/nsFontFaceLoader.
> cpp#339 (No clue what exactly this does :D)

I've never seen this either, but you can probably claw your way to the global via mPresContext.

> But even for the regular case where we have a doc and take the principal
> from there, I still don't understand how it would work with
> xpc::Scriptability. I would need to get this scope object first right?

Yes. You can pull the scope off the document via GetScopeObject().
 
> I first had the idea to put a CSP right into nsDocument itself, but for the
> cases mentioned above, it isn't possible to get a doc either (or at least it
> seems so).

Hm. I'm not super familiar with the CSP load paths, but I'd imagine we might run into trouble with the fact that we might need to store and query CSP early on in a load, before we've necessarily bootstrapped a global. So maybe storing it on the compartment is the wrong approach. Maybe we should store it on the channel? What do you think, Boris?

You can always get the document from the global (if it exists) by QIing the native global to nsPIDOMWindow, and doing GetDoc(). From the document, you can access the channel too.
Flags: needinfo?(bzbarsky)
Right now we set up CSP on the document in nsDocument::StartDocumentLoad.  This is called from nsContentDLF::CreateDocument, which is called from nsContentDLF::CreateInstance.  That's invoked from nsDocShell::NewContentViewerObj.

I guess we probably fire unload on the previous page between doing that and setting up the new inner window (which happens when we Init() the new content viewer later down in nsDocShell::CreateContentViewer.  But I would think that nothing of interest should happen on the new document until after that SetNewDocument call on the global...  On the other hand, by that point it's too late: we've already done InitCSP.

Really all we need here is a way to ask, in InitCSP, what the CSP for this document should be, right?  We already have special-casing in there for apps.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #32)
> 
> Really all we need here is a way to ask, in InitCSP, what the CSP for this
> document should be, right?  We already have special-casing in there for apps.

No, I don't think that works (this is what I'm doing already). In InitCSP, we apply the CSP to the principal, and the chrome pages use one shared principal instance. Once applied to the principal, everything using that principal is affected, which is far more than we wanted.
Forgot to set needinfo for comment 33 :)
Flags: needinfo?(bzbarsky)
Oh, I see.

Well, what behavior do you want, exactly?  We hang it off the principal so that things like about:blank and srcdoc and data: iframes use the same CSP.

If we want that for chrome, we need some object that would be inherited along with the principal but that CSP could hang off of.  If not, you could store stuff on the document itself.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (Vacation Dec 19 to Jan 1) from comment #35)
> Oh, I see.
> 
> Well, what behavior do you want, exactly?  We hang it off the principal so
> that things like about:blank and srcdoc and data: iframes use the same CSP.
> 
> If we want that for chrome, we need some object that would be inherited
> along with the principal but that CSP could hang off of.  If not, you could
> store stuff on the document itself.

Is the principal-inheriting machinery localized enough that we could just instrument it to explicitly inherit the CSP as well? If so, we could just hang it off the document, which seems like the natural place for this assuming it's been set up by the time we need to apply the CSP.
Not terribly.  Also, it's done via setting the .owner on the channel... we could add a new .owner value which is something that holds a principal and CSP, but it'd take some surgery.
Depends on: 957980
Depends on: 960728
No longer depends on: 960728
Comment on attachment 8345889 [details] [diff] [review]
bug-923902.patch (WIP & doesn't work)

Review of attachment 8345889 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately, I don't have a clear idea of the best way to resolve the principals situation. Why can't we treat all chrome:// documents as a single class, and universally apply some sensible CSP (like we do with privileged/certified apps)? Or is there a compelling reason to allow different policies for different Chrome pages?

::: content/base/public/nsContentPolicyUtils.h
@@ +165,5 @@
>                    nsCOMPtr<nsPIDOMWindow> win = do_QueryInterface(context);   \
>                    n = win ? win->GetExtantDoc() : nullptr;                    \
>                }                                                               \
> +		                                                              \
> +	      nsIDocument* d = NULL;                                          \

s/d/doc (makes it easier to read)

@@ +166,5 @@
>                    n = win ? win->GetExtantDoc() : nullptr;                    \
>                }                                                               \
> +		                                                              \
> +	      nsIDocument* d = NULL;                                          \
> +	      if (n)                                                          \

Following conventions in this macro, this if statement should have curly braces.

@@ +170,5 @@
> +	      if (n)                                                          \
> +		  d = n->OwnerDoc();                                          \
> +		                                                              \
> +	      if (n && d->IsChromeCSPDocument()) {                            \
> +		      fprintf(stderr, "DEBUG: Chrome CSP true\n");            \

Nit: indented too far

::: content/base/public/nsIDocument.h
@@ +659,5 @@
>    bool DidDocumentOpen() {
>      return mDidDocumentOpen;
>    }
>  
> +  virtual bool IsChromeCSPDocument() = 0;

Why does this need to be public?

::: content/base/src/nsDocument.cpp
@@ +2708,5 @@
>      }
>    }
>  
> +  if (applyChromeCSP) {
> +    nsAdoptingString chromeCSP = Preferences::GetString("security.apps.privileged.CSP.default");

This pref is only available on B2G (it's set in b2g/app/b2g.js). You should instead define a new pref with the specific policy for Chrome pages (also not good to conflate the threat models) in browser/app/profile/firefox.js (AIUI this should make the pref available across all platforms).

@@ +2772,5 @@
>    PR_LOG(gCspPRLog, PR_LOG_DEBUG,
>           ("Inserted CSP into principal %p", principal));
>  #endif
> +    nsAutoCString aspec;
> +    nsIURI* uriptr;

Declare in order of use (so uriptr should come first)

@@ +2777,5 @@
> +
> +    principal->GetURI(&uriptr);
> +
> +    if (uriptr) {
> +    uriptr->GetAsciiSpec(aspec);

Nit: indent this block (and the one below)

@@ +7090,5 @@
>  
> +bool nsDocument::IsChromeCSPDocument() {
> +  // TODO: XXX: Implement here
> +  nsIURI* origURI = this->GetOriginalURI();
> +  

Nit: avoid trailing whitespace. I recommend changing your editor's settings to highlight it.

@@ +7092,5 @@
> +  // TODO: XXX: Implement here
> +  nsIURI* origURI = this->GetOriginalURI();
> +  
> +  if (origURI) {
> +    nsAutoCString specStr;

s/specStr/origURISpec (or something else more descriptive)

@@ +7100,5 @@
> +      return false;
> +    }
> +    fprintf(stderr, "DEBUG XXX: %s\n", specStr.get());
> +
> +    if (!strcmp("about:memory", specStr.get())) {

Prefer

    if (origURISPec.equalsLiteral("about:memory"))
Attachment #8345889 - Flags: feedback?(grobinson)
(In reply to Boris Zbarsky [:bz] from comment #37)
> Not terribly.  Also, it's done via setting the .owner on the channel... we
> could add a new .owner value which is something that holds a principal and
> CSP, but it'd take some surgery.

This may be worth it; this may also remov some of the special casing (see bug 947831).
Attached patch csp.patch (deleted) — — Splinter Review 
@grobinson: Thanks for the feedback, I'll incorporate most of the style stuff as soon as we're anywhere close to something working. Right now, not even the strategy is clear.

This patch is the last patch rebased and I changed some stuff around how we determine if a chrome page should be protected or not (added a method in nsContentUtils and use it also in nsCSPService).

This wasn't working and until now I didn't know why, but looking at the code again I just found the issue (I guess) and fixed it in this patch. It's still untested though because we're about to have the meeting for this bug now :)
Attachment #8345889 - Attachment is obsolete: true
Depends on: 965413
Depends on: 965637

This is resolved, right? :)

Flags: needinfo?(ckerschb)

(In reply to Frederik Braun [:freddy] from comment #41)

This is resolved, right? :)

Yes, we applied a CSP to all about: pages within Bug 1449872 and friends.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Flags: needinfo?(ckerschb)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: