Closed Bug 1251152 Opened 8 years ago Closed 8 years ago

Implement Content Security Policy (CSP) for remote newtab

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox47 --- affected
firefox49 --- fixed

People

(Reporter: franziskus, Assigned: hchang)

References

Details

(Whiteboard: tpe-seceng,[domsecurity-active])

Attachments

(3 files, 19 obsolete files)

Part 1: Apply default CSP to signed contents
2.53 KB, patch
ckerschb
: review+
Details | Diff | Splinter Review
Part 2: Define default signed content CSP in all.js
1.29 KB, patch
oyiptong
: review+
Details | Diff | Splinter Review
Part 3: Test case
10.08 KB, patch
franziskus
: review+
Details | Diff | Splinter Review 
The remote newtab page should have a meta CSP attached. But in case something goes wrong and CSP is missing from the newtab page we should enforce a minimal CSP anyway.
Renaming for clarity in the tracking bug.
Summary: Add basic CSP to remote newtab → Implement Content Security Policy (CSP) for remote newtab
It seems this is a good first bug for me to step in. I am taking this bug, is it ok? :)
Flags: needinfo?(franziskuskiefer)
Thanks Henry for taking this on.

I had to look up what we actually agreed on and we don't want to add a minimal hard-coded CSP but do not load at all if there's no meta CSP in the file. So similar to the SRI case we expect there to be a CSP, if there's none, throw an error so that we can load a fallback.

@Christoph/Olivier can you confirm that's what we want to do?

The actual CSP on remote:newtab is tracked in https://github.com/mozilla/remote-newtab/issues/148
Flags: needinfo?(oyiptong)
Flags: needinfo?(mozilla)
Flags: needinfo?(franziskuskiefer)
Implementing CSP is contingent upon us building automation that allows us to run tests within a CSP-protected page.

I'll need to find the corresponding Marionette bug.
Flags: needinfo?(oyiptong)
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #3)
> Thanks Henry for taking this on.
> 
> I had to look up what we actually agreed on and we don't want to add a
> minimal hard-coded CSP but do not load at all if there's no meta CSP in the
> file. So similar to the SRI case we expect there to be a CSP, if there's
> none, throw an error so that we can load a fallback.
> 
> @Christoph/Olivier can you confirm that's what we want to do?

Yes, that's what we agreed on. Let's always ship a meta-csp. Enforcing that is actually pretty straight forward because that check only needs to happen for the top-level load. Unlike SRI checks, which have to be performed for subrequest loads.
Flags: needinfo?(mozilla)
Assignee: nobody → hchang
To sum up: what this bug requires are

0) Define a least strict CSP [1]
1) Make sure necko supports setting CSP http-equiv meta tag to the document prior to rendering.
2) If the content-signature protected document doesn't have (merged) CSP stricter than [1], load a fallback.

Question: have we defined the fallback yet?

[1] content="default-src 'none'; script-src '{{ origin }}', style-src: '{{ origin }}', img-src: '{{ origin }}'"
> 1) Make sure necko supports setting CSP http-equiv meta tag to the document
> prior to rendering.

Correction: This is not what necko is supposed to do and [1] has done it. 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMetaElement.cpp#139
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #3)
> Thanks Henry for taking this on.
> 
> I had to look up what we actually agreed on and we don't want to add a
> minimal hard-coded CSP but do not load at all if there's no meta CSP in the
> file. So similar to the SRI case we expect there to be a CSP, if there's
> none, throw an error so that we can load a fallback.
> 
> @Christoph/Olivier can you confirm that's what we want to do?
> 
> The actual CSP on remote:newtab is tracked in
> https://github.com/mozilla/remote-newtab/issues/148

Another question: what if there is a less strict CSP? For example: 

default-src 'none'; script-src '{{ origin }}' // only script-src, no img-src, style-src

I'd think we should still load a fallback in this case. 

Given that the original intention is to not have hard-coded CSP, how do we know if the joined CSP is the at at least the minimal one without any hard-coded CSP?
For reference, we can't enforce a strict set of CSP unless we have testing automation ready to run within CSP.

We are waiting for https://github.com/SeleniumHQ/selenium/issues/918

Which is blocked upon Firefox WebDriver
Per offline discussion with Olivier, we just do not load any subresource if there's no CSP in html http-equiv tag.
I have to make sure the fallback first. I'd like to let the fallback be just don't load the document at all and it's pretty simple. Another possible fallback might be still loading the top level document but blocking all the subresource load. What do you guys think? Thanks :)
Flags: needinfo?(mozilla)
Flags: needinfo?(franziskuskiefer)
(In reply to Henry Chang [:henry] from comment #11)
> I have to make sure the fallback first. I'd like to let the fallback be just
> don't load the document at all and it's pretty simple. Another possible
> fallback might be still loading the top level document but blocking all the
> subresource load. What do you guys think? Thanks :)

Most likely the site will fall apart completely if we don't load any JS, right? From a user experience perspective I think we should rather fallback and don't load the document at all. I think it feels pretty disturbing for users if we display a page without any subresources; ultimately that's Olivier's call though.
Flags: needinfo?(mozilla)
I agree with Christoph. Not loading the document at all is the best outcome for the user.
(In reply to Olivier Yiptong [:oyiptong] from comment #13)
> I agree with Christoph. Not loading the document at all is the best outcome
> for the user.

Also makes sense to me :)

BTW, it seems not as trivial as I imagine in the first place. We are not able to make decision until parsing out all <http-equiv> tags in <head> section. Also, what kind of fallback should we load? Just display an general network error page (like about:neterror) or redirect to a more informative page?
Flags: needinfo?(oyiptong)
(In reply to Henry Chang [:henry] from comment #6)
> To sum up: what this bug requires are
> 
> 0) Define a least strict CSP [1]

just to make sure, we don't want to do this :)

(In reply to Henry Chang [:henry] from comment #14)
> (In reply to Olivier Yiptong [:oyiptong] from comment #13)
> > I agree with Christoph. Not loading the document at all is the best outcome
> > for the user.
> 
> Also makes sense to me :)
> 
> BTW, it seems not as trivial as I imagine in the first place. We are not
> able to make decision until parsing out all <http-equiv> tags in <head>
> section. Also, what kind of fallback should we load? Just display an general
> network error page (like about:neterror) or redirect to a more informative
> page?

you probably want to define a new error that says NS_ERROR_CSP_MISSING (or the like) and then get docShell to load the fallback (the same as in the case of NS_ERROR_INVALID_SIGNATURE). The exact fallback is something that has to be implemented yet, but this should load about:blank (when bug 1226928 lands at some point).
Flags: needinfo?(franziskuskiefer)
(In reply to Henry Chang [:henry] from comment #6)
> To sum up: what this bug requires are
> 
> 1) Make sure necko supports setting CSP http-equiv meta tag to the document
> prior to rendering.

According to [1] CSP is not applied to anything that comes before the meta CSP tag. Thus I'd say we treat this like a missing CSP, fail, and load the fallback.

[1] https://www.w3.org/TR/CSP2/#delivery-html-meta-element
(In reply to Henry Chang [:henry] from comment #14)
> (In reply to Olivier Yiptong [:oyiptong] from comment #13)
> > I agree with Christoph. Not loading the document at all is the best outcome
> > for the user.
> 
> Also makes sense to me :)
> 
> BTW, it seems not as trivial as I imagine in the first place. We are not
> able to make decision until parsing out all <http-equiv> tags in <head>
> section. Also, what kind of fallback should we load? Just display an general
> network error page (like about:neterror) or redirect to a more informative
> page?

In the long term, handling this error by loading a local copy of the newtab page is a sensible solution.
We should also report that incident.

In the short term, loading about:blank may be ok, given that we're also doing so for the case when signature verification fails.

It will be very useful for us to send an observer notification in case CSP is found not present.
Flags: needinfo?(oyiptong)
Comment on attachment 8728351 [details] [diff] [review]
0001-Bug-1251152-Enforce-CSP-to-those-documents-which-hav.patch

Hi Nathan,

Would you still be available to review nsDocLoader? 

In this bug, we need to intercept the entire document loading and load an error page once CSP is not enforced in certain situations. Currently, the nsDocLoader already has "stop" function to do that but it always terminates the load with NS_BINDING_ABORTED. I'd like to extend "stop" function so that we can pass in custom error code to let docShell load a custom error page. Could you please give me some feedback about my patch? If you think there're other more suitable people to do the review, could you also let me know? Thanks :)
Attachment #8728351 - Flags: feedback?(nfroyd)
Attachment #8728352 - Flags: feedback?(franziskuskiefer)
Attachment #8728353 - Flags: feedback?(franziskuskiefer)
Hi Franziskus,

I attached a couple of patches to reveal my idea about this bug:

1) Let |nsIDocumentLoader::Stop| be able to stop with custom error code and then load custom error page.
2) Add a utility function to check if CSP should be enforced and stop document loading/load error page whenever needed.
3) Do the CSP enforcement check in every resource loading site.

The rest of task is to add a new error code and load a corresponding error page when docShell sees that error code.

What do you think of my design? Besides, who should I consult with to make the error page?

Thanks :)
Comment on attachment 8728351 [details] [diff] [review]
0001-Bug-1251152-Enforce-CSP-to-those-documents-which-hav.patch

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

I'm not the right person to review this, sorry. :)  Maybe ckerschb?
Attachment #8728351 - Flags: feedback?(nfroyd) → feedback?(mozilla)
Comment on attachment 8728351 [details] [diff] [review]
0001-Bug-1251152-Enforce-CSP-to-those-documents-which-hav.patch

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

::: dom/base/nsContentPolicyUtils.h
@@ +340,5 @@
> +NS_CheckContentPolicyEnforced(nsIDocument* aDocument)
> +{
> +  // Check if we should enforce CSP to this document.
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  aDocument->GetChannel()->GetLoadInfo(getter_AddRefs(loadInfo));

make sure to not deref a nullpointer.
NS_ENSURE_ARG(aDocument, false);

Nit, you can also simplify to
nsCOMPtr<nsILoadInfo> loadInfo = aDocument->GetChannel()->GetLoadInfo();

@@ +343,5 @@
> +  nsCOMPtr<nsILoadInfo> loadInfo;
> +  aDocument->GetChannel()->GetLoadInfo(getter_AddRefs(loadInfo));
> +  //bool shouldEnforceContentPolicy = loadInfo->GetVerifySignedContent();
> +  //if (!shouldEnforceContentPolicy) {
> +  //  return true;

nit: no need for the tmp variable
if (!loadInfo->GetVerifySignedContent()) {
 // bail out
}

@@ +348,5 @@
> +  //}
> +
> +  // Content policy needs to be enforced. Check if we have any policy.
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  aDocument->NodePrincipal()->GetCsp(getter_AddRefs(csp));

you can also simplify that, you don't even have to query the policyCount. If the csp object is non-null, we are good. something like:

if (csp) {
  // bail out - all good
}

@@ +362,5 @@
> +  }
> +
> +  nsIDocShell* docShell = aDocument->GetDocShell();
> +  nsCOMPtr<nsIDocumentLoader> docLoader = do_QueryInterface(docShell);
> +  docLoader->Stop(NS_ERROR_PHISHING_URI, 1);

I am not sure that NS_ERROR_PHISHING is the right error :-) maybe we should name it something like NS_ERROR_NO_CSP.

::: dom/base/nsContentUtils.cpp
@@ +3018,5 @@
>    int16_t decision = nsIContentPolicy::ACCEPT;
>  
> +  if (!NS_CheckContentPolicyEnforced(aLoadingDocument)) {
> +    return false;
> +  }

some of these callsites to NS_CheckContentLoadPolicy will disappear, because we are moving security checks into channel->asyncOpen2(). The better place to make sure we are enforcing CSP would potentially be here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#72

::: dom/base/nsObjectLoadingContent.cpp
@@ +1614,5 @@
>    nsIDocument* doc = thisContent->OwnerDoc();
>  
> +  if (!NS_CheckContentPolicyEnforced(doc)) {
> +    return false;
> +  }

As I said, it's probably to error prone to just add that check before every NS_CheckContentLoadPolicy() because we definitely miss a spot :-)

::: uriloader/base/nsIDocumentLoader.idl
@@ +16,5 @@
>   * (<iframe>, <frame>, etc). It is also responsible for sending
>   * nsIWebProgressListener notifications.
>   * XXXbz this interface should go away, we think...
>   */
> +[scriptable, uuid(9f03b255-961d-4d49-a813-39906fcee9a8)]

I think you don't have to ref the uuid anymore.
Attachment #8728351 - Flags: feedback?(mozilla)
Thanks Christoph!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #24)
> Comment on attachment 8728351 [details] [diff] [review]
> 0001-Bug-1251152-Enforce-CSP-to-those-documents-which-hav.patch
> 
> Review of attachment 8728351 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +348,5 @@
> > +  //}
> > +
> > +  // Content policy needs to be enforced. Check if we have any policy.
> > +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> > +  aDocument->NodePrincipal()->GetCsp(getter_AddRefs(csp));
> 
> you can also simplify that, you don't even have to query the policyCount. If
> the csp object is non-null, we are good. something like:
> 
> if (csp) {
>   // bail out - all good
> }
> 

But from what I read, CSP might be created with no policy added [1].

> @@ +362,5 @@
> > +  }
> > +
> > +  nsIDocShell* docShell = aDocument->GetDocShell();
> > +  nsCOMPtr<nsIDocumentLoader> docLoader = do_QueryInterface(docShell);
> > +  docLoader->Stop(NS_ERROR_PHISHING_URI, 1);
> 
> I am not sure that NS_ERROR_PHISHING is the right error :-) maybe we should
> name it something like NS_ERROR_NO_CSP.
> 

I just temporary use NS_ERROR_PHISHING to load an error page to convey my idea :)
Do you know if I should also make the error page or it's product people's call?

> ::: dom/base/nsContentUtils.cpp
> @@ +3018,5 @@
> >    int16_t decision = nsIContentPolicy::ACCEPT;
> >  
> > +  if (!NS_CheckContentPolicyEnforced(aLoadingDocument)) {
> > +    return false;
> > +  }
> 
> some of these callsites to NS_CheckContentLoadPolicy will disappear, because
> we are moving security checks into channel->asyncOpen2(). The better place
> to make sure we are enforcing CSP would potentially be here:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#72
> 
> ::: dom/base/nsObjectLoadingContent.cpp
> @@ +1614,5 @@
> >    nsIDocument* doc = thisContent->OwnerDoc();
> >  
> > +  if (!NS_CheckContentPolicyEnforced(doc)) {
> > +    return false;
> > +  }
> 
> As I said, it's probably to error prone to just add that check before every
> NS_CheckContentLoadPolicy() because we definitely miss a spot :-)
> 

I was aware of asyncOpen2 but it seems not all asyncOpen callsites have 
been updated. IMO the "CSP enforcement check" shouldn't mess up with the 
core CSP code like |nsContentPolicy::CheckPolicy|. It's beyond CSP. 
Do you also think so?

|NS_CheckContentLoadPolicy| might be a better place to me but it 
lacks |nsIDocument| for being able to stop document loading and 
redirect an error page. A solution might be pass in the |nsIDocument| 
to |NS_CheckContentLoadPolicy|. Do you think it's okay? 

> ::: uriloader/base/nsIDocumentLoader.idl
> @@ +16,5 @@
> >   * (<iframe>, <frame>, etc). It is also responsible for sending
> >   * nsIWebProgressListener notifications.
> >   * XXXbz this interface should go away, we think...
> >   */
> > +[scriptable, uuid(9f03b255-961d-4d49-a813-39906fcee9a8)]
> 
> I think you don't have to ref the uuid anymore.

Do you mean I don't need to refresh any IDL's UUID anymore or?

BTW, are you also able to review nsDocLoader? Thanks!


[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2834
My fault! |nsContentPolicy::CheckPolicy| is very likely to have |nsIDocument| in its argument! 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#106
Attachment #8728352 - Attachment is obsolete: true
Attachment #8728352 - Flags: feedback?(franziskuskiefer)
Attachment #8728353 - Attachment is obsolete: true
Attachment #8728353 - Flags: feedback?(franziskuskiefer)
Attachment #8728351 - Attachment is obsolete: true
Hi Christoph,

I re-attached 3 patches:

Part 1: Allow nsDocLoader to stop with custom error code.
Part 2: Add new error code to represent the CSP enforcement error and load CSP blocked page in nsDocShell
Part 3: Do the CSP enforcement check in nsContentPolciy::CheckPolicy

Could you have a look at my patches again and give me some feedbacks? Thanks!
Flags: needinfo?(mozilla)
Comment on attachment 8728836 [details] [diff] [review]
0001-Bug-1251152-Part-1-Allow-stopping-DocLoader-with-cus.patch

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

::: docshell/base/nsDocShell.cpp
@@ +5424,5 @@
>  
>      // XXXbz We could also pass |this| to nsIURILoader::Stop.  That will
>      // just call Stop() on us as an nsIDocumentLoader... We need fewer
>      // redundant apis!
> +    StopInternal();

please pass the default argument.

@@ +7921,5 @@
>      mLoadingURI = nullptr;
>  
>      // Stop any in-progress loading, so that we don't accidentally trigger any
>      // PageShow notifications from Embed() interrupting our loading below.
> +    StopInternal();

and here.

@@ +8466,5 @@
>    // loading until after data arrives, which is after STATE_START completes.
>  
>    RefPtr<RestorePresentationEvent> currentPresentationRestoration =
>      mRestorePresentationEvent.get();
> +  StopInternal();

and here.

::: uriloader/base/nsDocLoader.cpp
@@ +234,5 @@
>  
>  NS_IMETHODIMP
> +nsDocLoader::Stop(nsresult aReason, uint8_t aOptionalArgc)
> +{
> +  return aOptionalArgc ? StopInternal(aReason) : StopInternal();

please follow the principle of least privilege and also explicitly add the default argument; StopInternal(NS_BINDING_ABORTED);

@@ +349,5 @@
>  
>  void
>  nsDocLoader::Destroy()
>  {
> +  StopInternal();

also here, please fill the default arg.

::: uriloader/base/nsIDocumentLoader.idl
@@ +16,5 @@
>   * (<iframe>, <frame>, etc). It is also responsible for sending
>   * nsIWebProgressListener notifications.
>   * XXXbz this interface should go away, we think...
>   */
> +[scriptable, uuid(9f03b255-961d-4d49-a813-39906fcee9a8)]

nit: no need to ref uuids anymore.
Attachment #8728836 - Flags: feedback+
Comment on attachment 8728837 [details] [diff] [review]
0002-Bug-1251152-Part-2-Add-new-error-code-for-CSP-enforc.patch

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

You can fold that patch into the other with the docshell changes. btw, we need a docshell peer in the end to review those bits.
Comment on attachment 8728838 [details] [diff] [review]
0003-Bug-1251152-Part-3-Do-CSP-enforcement-check-before-t.patch

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

Those changes look way better now.

::: dom/base/nsContentPolicy.cpp
@@ +68,5 @@
>  
>  #endif // defined(DEBUG)
>  
> +static bool ShouldEnforceContentPolicy(nsIDocument* aDocument)
> +{

please make sure to not crash in case aDocument == nullptr;

@@ +100,5 @@
> +      doc = node->OwnerDoc();
> +  }
> +  if (!doc) {
> +      doc = do_QueryInterface(aContext);
> +  }

What if I pass nsIPrincipal as aContext?

@@ +137,5 @@
> +    if (doc && ShouldEnforceContentPolicy(doc) && !HasContentPolicy(doc)) {
> +      // Should we add an extra parameter to let the caller decides to
> +      // load an error page or not?
> +      nsIDocShell* docShell = doc->GetDocShell();
> +      nsCOMPtr<nsIDocumentLoader> docLoader = do_QueryInterface(docShell);

what guarantees that docLoader is not null?
Attachment #8728838 - Flags: feedback+
That is going to look pretty good - time to write some testcases.
Flags: needinfo?(mozilla)
Whiteboard: tpe-seceng
Comment on attachment 8732789 [details] [diff] [review]
0004-Bug-1251152-Part-4-Load-remote-newtab-fallback-page-.patch

Hi Chris,

I added a new patch to load the fallback page no matter what kind of remote newtab loading error is occurred. The idea is fairly simple: check if the failed URL is the remote newtab URL. If yes, load the fallback page. All the relevant info will be retrieved from aboutNewTabService.

Could you have a look? I'll be writing the test case based on patches. Thanks :)
Attachment #8732789 - Flags: feedback?(mozilla)
Whiteboard: tpe-seceng → tpe-seceng,[domsecurity-active]
Comment on attachment 8732789 [details] [diff] [review]
0004-Bug-1251152-Part-4-Load-remote-newtab-fallback-page-.patch

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

Please consult olivier for the aboutNewTabService to make sure he agrees with those changes. Otherwise this already looks pretty good.

::: docshell/base/nsDocShell.cpp
@@ +5161,5 @@
> +{
> +  aDidLoad = false;
> +
> +  nsCOMPtr<nsIAboutNewTabService> aboutNewTabService =
> +    do_GetService("@mozilla.org/browser/aboutnewtab-service;1");

nsCOMPtr<nsIAboutNewTabService> aboutNewTabService =
  do_GetService("@mozilla.org/browser/aboutnewtab-service;1", &rv);
NS_ENSURE_SUCCESS(rv, rv);

@@ +5169,5 @@
> +  nsAutoCString remoteURL;
> +  aboutNewTabService->GetRemoteURL(remoteURL);
> +  if (!failedURL.Equals(remoteURL)){
> +    return NS_OK;
> +  }

I suppose we really have to perform a string comparions here - ugh.

@@ +5185,5 @@
> +                    mozilla::net::RP_Default,
> +                    nullptr, INTERNAL_LOAD_FLAGS_INHERIT_OWNER, nullptr,
> +                    nullptr, NullString(), nullptr, nullptr, LOAD_NORMAL,
> +                    nullptr, true, NullString(), this, nullptr, nullptr,
> +                    nullptr);

Please align arguments and add comments on the right side of the arguments explaining what the nullptr arguments actually are.

Oh I see, you based function call on the semantic layout within nsDocshell. Well that's fine with me as well.

@@ +5190,5 @@
> +
> +  if (NS_SUCCEEDED(rv)) {
> +    aDidLoad = true;
> +  }
> +

NS_ENSURE_SUCCESS(rv, rv);
aDidLoad = true;
return NS_OK;

@@ +5235,5 @@
>  
> +  // We should load remote the remote newtab fallback for any kind of
> +  // loading error if the error is from remote new tab.
> +  {
> +    bool didLoad;

bool didLoad = false;

@@ +5236,5 @@
> +  // We should load remote the remote newtab fallback for any kind of
> +  // loading error if the error is from remote new tab.
> +  {
> +    bool didLoad;
> +    nsresult rv = MaybeLoadRemoteNewTabFallback(didLoad);

NS_ENSURE_SUCCESS(rv, rv);

@@ +5237,5 @@
> +  // loading error if the error is from remote new tab.
> +  {
> +    bool didLoad;
> +    nsresult rv = MaybeLoadRemoteNewTabFallback(didLoad);
> +    if (NS_SUCCEEDED(rv) && didLoad) {

if (didLoad) {
  return NS_OK;
}
Attachment #8732789 - Flags: feedback?(mozilla) → feedback+
Thanks Christoph! I'll consult Olivier regarding the change of nsIAboutNewTabService.idl.

Olivier, what do you think of my change of nsIAboutNewTabService? The main purpose is to let nsDocShell know if the remote newtab is being loaded and what to load as a fallback. Thanks!
Flags: needinfo?(oyiptong)
Just realized that we currently use about:blank when encountering the "invalid content signature" error [1] and the same assumption is already in the test cases [2]. So, I am going to file a followup bug to redirect to local about:newtab for whatever loading error and update the test cases at once. 


[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#7580
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/test/contentverifier/browser_verify_content_about_newtab.js#6
Hi Henry, bug 1246202 tracks the implementation of the actual fallback. Olivier will look into this soon. This is unfortunately not as easy as it sounds due to e10s and non-desktop FF builds.
I like the changes to nsIAboutNewTabService, but as Franziskus says the 4th part patch will not work.
nsDocShell will need to communicate with an instance of the service that's in the parent process.

In the patch, it obtains an instance of the service, which, in the e10s case, will not have the correct state.

Perhaps you could tackle that in the next in bug 1246202? :-)
Flags: needinfo?(oyiptong)
Attachment #8728836 - Attachment is obsolete: true
Attachment #8728837 - Attachment is obsolete: true
Attachment #8728838 - Attachment is obsolete: true
Attachment #8732789 - Attachment is obsolete: true
Attachment #8734271 - Flags: review?(mozilla)
Attachment #8734272 - Flags: review?(mozilla)
Attachment #8734273 - Flags: review?(mozilla)
Hi Christoph,

I've requested for the formal review of my patches. One test case is updated (original one with SRI enforcement) and one is added (content with sub-resource w/o CSP). Could you please have them a review?
Besides, I changed to load about:blank while having CSP enforcement error and will try to deal with it in Bug 1246202.

Thanks!
Comment on attachment 8734271 [details] [diff] [review]
0001-Bug-1251152-Part-1-Allow-stopping-DocLoader-with-cus.patch

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

This looks fine to me, but obviously we need review from a docshell peer for those changes.

::: docshell/base/nsDocShell.cpp
@@ +5404,5 @@
>  
>      // XXXbz We could also pass |this| to nsIURILoader::Stop.  That will
>      // just call Stop() on us as an nsIDocumentLoader... We need fewer
>      // redundant apis!
> +    StopInternal();

As already mentioned in my previous review, I personally prefer if the default argument is passed explicitly to StopInternal(). Not only here but in all the cases throughout that patch.

::: uriloader/base/nsIDocumentLoader.idl
@@ +16,5 @@
>   * (<iframe>, <frame>, etc). It is also responsible for sending
>   * nsIWebProgressListener notifications.
>   * XXXbz this interface should go away, we think...
>   */
> +[scriptable, uuid(9f03b255-961d-4d49-a813-39906fcee9a8)]

no need to update the uuid anymore.
Attachment #8734271 - Flags: review?(mozilla) → feedback+
Comment on attachment 8734272 [details] [diff] [review]
0002-Bug-1251152-Part-2-Do-CSP-enforcement-check.-Load-ab.patch

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

Overall looks pretty good, I would like to see my concerns addressed. Once I am fine with the patch we also need to find a dom peer to accept those changes.

::: dom/base/nsContentPolicy.cpp
@@ +69,5 @@
>  
>  #endif // defined(DEBUG)
>  
> +static bool ShouldEnforceContentPolicy(nsIDocument* aDocument)
> +{

What if aDocument is null? You are running in danger of derefing a nullptr.
if (!aDocument) {
  return false;
}

@@ +78,5 @@
> +  return loadInfo->GetVerifySignedContent();
> +}
> +
> +static bool HasContentPolicy(nsIDocument* aDocument)
> +{

same here, what if aDocument == nullptr?

@@ +102,5 @@
> +  }
> +  if (!doc) {
> +      doc = do_QueryInterface(aContext);
> +  }
> +  return doc.forget();

What if the doc is null at that point?

@@ +111,5 @@
> +{
> +  return (nsIContentPolicy::TYPE_INTERNAL_SCRIPT_PRELOAD == aContentType) ||
> +         (nsIContentPolicy::TYPE_INTERNAL_STYLESHEET_PRELOAD == aContentType) ||
> +         (nsIContentPolicy::TYPE_INTERNAL_IMAGE_PRELOAD == aContentType);
> +}

We have pre-defined functions for detecting preloads. You can use:
nsContentUtils::IsPreloadType().

@@ +139,5 @@
>                       "Context should be a DOM node or a DOM window!");
>      }
>  #endif
>  
> +    nsCOMPtr<nsIDocument> doc = GetDocumentFromContext(requestingContext);

Pretty sure we call content policies where the requestingContext is a nullptr. Did you run all the tests within dom/security/test/csp?

@@ +142,5 @@
>  
> +    nsCOMPtr<nsIDocument> doc = GetDocumentFromContext(requestingContext);
> +
> +    // Check if CSP needs to be enforced and handle the error.
> +    if (!IsPreloadedContentType(contentType) && doc &&

Why do we want to exclude preloads? I would imagine we only want to make sure we have a CSP for TYPE_DOCUMENT, no?

Also if you update the two static functions to deal with nullptr arguments you can remove the |&& doc| from the callsite.
Attachment #8734272 - Flags: review?(mozilla) → review-
Comment on attachment 8734273 [details] [diff] [review]
0003-Bug-1251152-Part-3-Update-test-case-for-SRI-and-add-.patch

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

Tests looks good to me. Since Franziskus initially wrote those tests he is probably the best reviewer.
Attachment #8734273 - Flags: review?(mozilla)
Attachment #8734273 - Flags: review?(franziskuskiefer)
Attachment #8734273 - Flags: feedback+

        Attachment #8734272 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8736117 [details] [diff] [review]
0002-Bug-1251152-Part-2-Do-CSP-enforcement-check.-Load-ab.patch

Hi Christoph,

Thanks fro your previous review! I've addressed your concerns in this updated patch. Besides, the reason to check the "preload type" is because appending CSP defined in meta tag might happen preloading resources. From what I understand, preloading a resource doesn't actually process it so it's safe to skip the CSP enforcement check for preloaded resources. (Another CSP check would be done whenever the resource is actually processed.)

However, I am not very clear to the timing of "preloading resources" and "parsing meta tag and appending CSP". (I just observed that fact in the test case.) If you have any concern regarding this, I am glad to dig it deeper to make everything as clear as possible.

Thanks :)

        Attachment #8736117 -
        Flags: review?(mozilla)

    
    

    
  
Comment on attachment 8734271 [details] [diff] [review]
0001-Bug-1251152-Part-1-Allow-stopping-DocLoader-with-cus.patch

Hi Nathan,

Sorry for bothering again. Could you please review the changes in this patch? It is to support stopping document load with custom error code so that we can handle it differently in docShell.

Thanks!

        Attachment #8734271 -
        Flags: review?(nfroyd)

    
    

    
  
Comment on attachment 8734273 [details] [diff] [review]
0003-Bug-1251152-Part-3-Update-test-case-for-SRI-and-add-.patch

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

lgtm from what it is testing. But wouldn't this fail the test case with file_about_newtab.html? file_about_newtab.html has no CSP and thus should be blocked. I think we can clean up a bit here and i) don't CSP with valid and invalid signatures, ii) use CSP with valid and invalid signatures. They can either contain other resources (with SRI) or not, that should be tested in the SRI specific tests.

        Attachment #8734273 -
        Flags: review?(franziskuskiefer)

    
    

    
  
Comment on attachment 8734271 [details] [diff] [review]
0001-Bug-1251152-Part-1-Allow-stopping-DocLoader-with-cus.patch

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

Patch sort of makes sense to me, but I'm not the right person to say yea or nay on this.  Bouncing review to smaug.

        Attachment #8734271 -
        Flags: review?(nfroyd) → review?(bugs)

    
    

    
  
Comment on attachment 8734271 [details] [diff] [review]
0001-Bug-1251152-Part-1-Allow-stopping-DocLoader-with-cus.patch

>   // Stop all loads in the loadgroup of this docloader
>-  void stop();
>+  [optional_argc]
>+    void stop([optional] in nsresult aReason);


I think I'd prefer to do this in a bit different way.
Perhaps add stopWithReason(in nsresult aReason)
and then the implementation of stop would just call
StopWithReason(NS_BINDING_ABORTED);
And the thing you call nsDocLoader::StopInternal, would be NS_IMETHODIMP nsDocLoader::StopWithReason(nsresult aReason).
Also document in .idl that stop() passes NS_BINDING_ABORTED to StopWithReason()

This all just to not use StopInternal() in so many places, where it is just about generic Stop()

And  NS_OBSERVER_ARRAY_NOTIFY_XPCOM_OBSERVERS(mChildList, nsDocLoader, StopWithReason, ()); should take the reason as a param, not
use the default param. So that () should become (aReason) I think.


Those fixed, r+

        Attachment #8734271 -
        Flags: review?(bugs) → review+

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #54)
> Comment on attachment 8734271 [details] [diff] [review]
> 0001-Bug-1251152-Part-1-Allow-stopping-DocLoader-with-cus.patch
> 
> >   // Stop all loads in the loadgroup of this docloader
> >-  void stop();
> >+  [optional_argc]
> >+    void stop([optional] in nsresult aReason);
> 
> 
> I think I'd prefer to do this in a bit different way.
> Perhaps add stopWithReason(in nsresult aReason)
> and then the implementation of stop would just call
> StopWithReason(NS_BINDING_ABORTED);
> And the thing you call nsDocLoader::StopInternal, would be NS_IMETHODIMP
> nsDocLoader::StopWithReason(nsresult aReason).
> Also document in .idl that stop() passes NS_BINDING_ABORTED to
> StopWithReason()
> 
> This all just to not use StopInternal() in so many places, where it is just
> about generic Stop()
> 
> And  NS_OBSERVER_ARRAY_NOTIFY_XPCOM_OBSERVERS(mChildList, nsDocLoader,
> StopWithReason, ()); should take the reason as a param, not
> use the default param. So that () should become (aReason) I think.
> 
> 
> Those fixed, r+

Thanks for the review :)

I've also done what you suggested in the first place then changed to the one you reviewer later on :p I will change to the one you prefer. Thanks!

    
    

    
  
Comment on attachment 8736117 [details] [diff] [review]
0002-Bug-1251152-Part-2-Do-CSP-enforcement-check.-Load-ab.patch

Henry, I don't have time to look at that changeset for the next 10 days. If you have incorporated my suggestions then hopefully smaug can review the final patch since he already reviewed the other one.

        Attachment #8736117 -
        Flags: review?(mozilla) → review?(bugs)

    
    

    
  
Comment on attachment 8736117 [details] [diff] [review]
0002-Bug-1251152-Part-2-Do-CSP-enforcement-check.-Load-ab.patch

> nsContentPolicy::CheckPolicy(CPMethod          policyMethod,
>                              SCPMethod         simplePolicyMethod,
>                              nsContentPolicyType contentType,
>                              nsIURI           *contentLocation,
>                              nsIURI           *requestingLocation,
>                              nsISupports      *requestingContext,
>                              const nsACString &mimeType,
>@@ -89,45 +134,58 @@ nsContentPolicy::CheckPolicy(CPMethod          policyMethod,
>     {
>         nsCOMPtr<nsIDOMNode> node(do_QueryInterface(requestingContext));
>         nsCOMPtr<nsIDOMWindow> window(do_QueryInterface(requestingContext));
>         NS_ASSERTION(!requestingContext || node || window,
>                      "Context should be a DOM node or a DOM window!");
>     }
> #endif
> 
>+    nsCOMPtr<nsIDocument> doc = GetDocumentFromContext(requestingContext);
>+
>+    // Check if CSP needs to be enforced and handle the error.
>+    if (!nsContentUtils::IsPreloadType(contentType) &&
>+         ShouldEnforceContentPolicy(doc) && !HasContentPolicy(doc)) {
>+      nsIDocShell* docShell = doc->GetDocShell();
>+      nsCOMPtr<nsIDocumentLoader> docLoader;
>+      if (docShell) {
>+        docLoader = do_QueryInterface(docShell);
>+      }
>+      if (docLoader) {
>+        nsCString contentURL;
>+        contentLocation->GetAsciiSpec(contentURL);
>+        MOZ_LOG(gConPolLog, LogLevel::Debug,
>+               ("CSP should be enforced! %s\n", contentURL.get()));
>+
>+        // Reject and stop doc loading.
>+        docLoader->Stop(NS_ERROR_CSP_NOT_ENFORCED, 1);
>+        *decision = nsIContentPolicy::REJECT_OTHER;
>+        return NS_OK;
>+      }
>+    }
I think I'm clearly missing some background information here.
I don't understand why the normal CSP stuff doesn't work in this case and I also don't understand why we need to
add this code to nsContentPolicy.
Why can't we put the code to CSPService::ShouldLoad and/or CSPService::ShouldProcess ?


nsContentPolicy should be generic content policy service, and should have as few special cases as possible. 
( if (mixedContentBlocker == entries[i] || cspService == entries[i]) { is already a bit annoying special case)


But maybe I'm misunderstanding something here. If so, please explain and ask review again :)

        Attachment #8736117 -
        Flags: review?(bugs) → review-

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #57)
> But maybe I'm misunderstanding something here. If so, please explain and ask
> review again :)

about:newtab needs to enforce CSP, which means the loading site has to be equipped with a CSP. We would never call CSP::ShouldLoad() in a case the the site does not have a CSP. In other words it's too late to enforce it within ShouldLoad().

Now that I think about it, potentially the other place might be: nsDocument::InitCSP(nsIChannel* aChannel).

    
    

    
  
Hmm, then I'm even more lost what the patch doing :)
CSP::ShouldLoad is just an implementation of nsIContentPolicy, and nsContentPolicy::CheckPolicy goes through all the contentpolicy impls.
What am I missing here?

    
    

    
  
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #52)
> Comment on attachment 8734273 [details] [diff] [review]
> 0003-Bug-1251152-Part-3-Update-test-case-for-SRI-and-add-.patch
> 
> Review of attachment 8734273 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm from what it is testing. But wouldn't this fail the test case with
> file_about_newtab.html? file_about_newtab.html has no CSP and thus should be
> blocked. I think we can clean up a bit here and i) don't CSP with valid and
> invalid signatures, ii) use CSP with valid and invalid signatures. They can
> either contain other resources (with SRI) or not, that should be tested in
> the SRI specific tests.

Hi Franziskus,

Since file_about_newtab.html doesn't have any subresource, it wouldn't be blocked at all. Is this not the hehavior you expect? Thanks :)
Flags: needinfo?(franziskuskiefer)

    
    

    
  
(In reply to Henry Chang [:henry] from comment #60)
> (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #52)
> > Review of attachment 8734273 [details] [diff] [review]:
> > -----------------------------------------------------------------
> Hi Franziskus,
> 
> Since file_about_newtab.html doesn't have any subresource, it wouldn't be
> blocked at all. Is this not the hehavior you expect? Thanks :)

We also need a CSP if there aren't any subresources to prevent script injection etc. So this is not really related to whether the page has subresources or not. A remote newtab without CSP must always fail to load.
Flags: needinfo?(franziskuskiefer)

    
    

    
  
Hi Olli,

Since the call path is:

nsContentPolicy::ShouldLoad()
  nsContentPolicy::CheckPolicy()
    For each CSP entries ==> Call CSPService::ShouldLoad (then nsCSPContext::ShouldLoad)

What Christoph meant in comment 58 is that |CSPService::ShouldLoad| wouldn't be called if there's no CSP entries assigned to a document (via either HTTP header or meta tag).

Isn't nsCSPContext::ShouldLoad the most essential CSP code and nsContentPolicy wrapping around CSPService with DOM related work? If so, wouldn't it be better to put the enforcement in |nsContentPolicy::CheckPolicy|?

BTW, I was hoping to put the enforcement in every callsite of |nsContentPolicy::ShouldLoad| in the first place to avoid introducing any exception in pure CSP code. However, we will definitely miss a spot if doing so.

Thanks!

    
    

    
  
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #61)
> (In reply to Henry Chang [:henry] from comment #60)
> > (In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #52)
> > > Review of attachment 8734273 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > Hi Franziskus,
> > 
> > Since file_about_newtab.html doesn't have any subresource, it wouldn't be
> > blocked at all. Is this not the hehavior you expect? Thanks :)
> 
> We also need a CSP if there aren't any subresources to prevent script
> injection etc. So this is not really related to whether the page has
> subresources or not. A remote newtab without CSP must always fail to load.

The inline script would be done CSP check before execution so my implementation is still safe for script injection. However, I am not sure if there's any other cases that I didn't consider. Maybe "unsafe-eval" is one thing that I need to take care.

    
    

    
  
(In reply to Henry Chang [:henry] from comment #62)
> Hi Olli,
> 
> Since the call path is:
> 
> nsContentPolicy::ShouldLoad()
>   nsContentPolicy::CheckPolicy()
>     For each CSP entries ==> Call CSPService::ShouldLoad (then
> nsCSPContext::ShouldLoad)
> 
> What Christoph meant in comment 58 is that |CSPService::ShouldLoad| wouldn't
> be called if there's no CSP entries assigned to a document (via either HTTP
> header or meta tag).

nsContentPolicy::CheckPolicy() doesn't decide which nsIContentPolicy objects it is calling.
It calls them all (unless someone rejects the load/process).
So I don't understand *why* CSPService::ShouldLoad isn't called? Is it that we haven't initiated
the CSPService yet and registered it as an nsIContentPolicy?


> BTW, I was hoping to put the enforcement in every callsite of
> |nsContentPolicy::ShouldLoad| in the first place to avoid introducing any
> exception in pure CSP code. However, we will definitely miss a spot if doing
> so.
Well, this is all about CSP, so whatever special case we add, it should be added to CSP code, not to generic
ContentPolicy code - if possible.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #64)
> (In reply to Henry Chang [:henry] from comment #62)
> > Hi Olli,
> > 
> > Since the call path is:
> > 
> > nsContentPolicy::ShouldLoad()
> >   nsContentPolicy::CheckPolicy()
> >     For each CSP entries ==> Call CSPService::ShouldLoad (then
> > nsCSPContext::ShouldLoad)
> > 
> > What Christoph meant in comment 58 is that |CSPService::ShouldLoad| wouldn't
> > be called if there's no CSP entries assigned to a document (via either HTTP
> > header or meta tag).
> 
> nsContentPolicy::CheckPolicy() doesn't decide which nsIContentPolicy objects
> it is calling.
> It calls them all (unless someone rejects the load/process).
> So I don't understand *why* CSPService::ShouldLoad isn't called? Is it that
> we haven't initiated
> the CSPService yet and registered it as an nsIContentPolicy?
> 

Hi Olli,

From what I understand, if one document hasn't been appended any policy, |count| at [1] will be 0 and no policy check (nsIContentPolicy::ShouldLoad, passed by a member function pointer at [2] and its impl is in nsCSPService.cpp [3]) will be done. So, we have to do the CSP enforcement check before |CSPService::ShouldLoad| is actually called. 

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#133
[2] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#241
[3] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#106

    
  
Flags: needinfo?(bugs)

    
    

    
  
So if we need nsCSPService to be running from the beginning, why not initialize that service early enough? And then do the CSP related stuff in it, and not put them to somewhat unrelated nsIContentPolicy.

And we have http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.cpp#1357
So CSPService should be up and running all the time, just like datadocument content policy and others.
Flags: needinfo?(bugs)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #66)
> So if we need nsCSPService to be running from the beginning, why not
> initialize that service early enough? And then do the CSP related stuff in
> it, and not put them to somewhat unrelated nsIContentPolicy.
> 
> And we have
> http://mxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutModule.
> cpp#1357
> So CSPService should be up and running all the time, just like datadocument
> content policy and others.

Hi Olli,

Sorry that I got a little confusing :( 

nsCSPService is up and running since beginning as you said but nsCSPService::ShouldLoad would be called only when a document has CSP directives. What you suggested in comment 57 (do CSP enforcement check in nsCSPService::ShouldLoad) doesn't work and it's because that a document may have no CSP [1].

My patch is to check if a document has CSP (under certain condition) whenever loading a subresource.

BTW, do you mind having a discussion on irc some time? I can explain it very detailedly to you :)

Thanks!

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsContentPolicy.cpp#133

    
    

    
  
I am really sorry, Olli. I have been misunderstanding some things and just realized the relationship between ContentPolicy and ContentSecurityPolicy. Let me study again and ping you later. Thanks!
Flags: needinfo?(bugs)

    
    

    
  
I guess you'll needinfo me again later then :)
Flags: needinfo?(bugs)

    
    

    
  


  

        Attachment #8736117 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8740858 [details] [diff] [review]
0002-Bug-1251152-Part-2-Do-CSP-enforcement-check.-Load-ab.patch

Hi Olli,

I've moved the CSP enforcement code to nsCSPService::ShouldLoad and it still works fine as expected. Could you have a review again? Thanks!

        Attachment #8740858 -
        Flags: review?(bugs)

    
    

    
  
Comment on attachment 8740858 [details] [diff] [review]
0002-Bug-1251152-Part-2-Do-CSP-enforcement-check.-Load-ab.patch

It would be good for someone from security team to review this too.

>+static already_AddRefed<nsIDocument>
>+GetDocumentFromContext(nsISupports* aContext)
>+{
>+  nsCOMPtr<nsIDocument> doc;
>+  nsCOMPtr<nsIContent> node = do_QueryInterface(aContext);
>+  if (node) {
>+      doc = node->OwnerDoc();
>+  }
>+  if (!doc) {
>+      doc = do_QueryInterface(aContext);
>+  }
If you QIed to nsINode, you could just check its OwnerDoc().
(nsIContent and nsIDocument both inherit nsINode)
But in fact I think this method isn't needed if the code was moved to be in CSPService::ShouldLoad

>+static bool MaybeCheckEnforcement(uint32_t aContentType,
>+                                  nsISupports *aRequestContext)
>+{
>+  nsCOMPtr<nsIDocument> doc = GetDocumentFromContext(aRequestContext);
>+
>+  // Check if CSP needs to be enforced and handle the error.
>+  if (!nsContentUtils::IsPreloadType(aContentType) &&
>+      ShouldEnforceContentPolicy(doc) && !HasContentPolicy(doc)) {
>+    nsIDocShell* docShell = doc->GetDocShell();
>+    nsCOMPtr<nsIDocumentLoader> docLoader;
>+    if (docShell) {
>+      docLoader = do_QueryInterface(docShell);
>+    }
QI is a null safe call, so this could be
nsCOMPtr<nsIDocumentLoader> docLoader = do_QueryInterface(doc->GetDocShell());


> /* nsIContentPolicy implementation */
> NS_IMETHODIMP
> CSPService::ShouldLoad(uint32_t aContentType,
>                        nsIURI *aContentLocation,
>                        nsIURI *aRequestOrigin,
>                        nsISupports *aRequestContext,
>                        const nsACString &aMimeTypeGuess,
>                        nsISupports *aExtra,
>@@ -130,16 +198,25 @@ CSPService::ShouldLoad(uint32_t aContentType,
>   // or type is *not* subject to CSP.
>   // Please note, the correct way to opt-out of CSP using a custom
>   // protocolHandler is to set one of the nsIProtocolHandler flags
>   // that are whitelistet in subjectToCSP()
>   if (!sCSPEnabled || !subjectToCSP(aContentLocation, aContentType)) {
>     return NS_OK;
>   }
Because of early return from subjectToCSP this patch has in theory a bit different behavior than the previous versions of the patch.
Is that expected?


>+  // Do CSP enforcement check whenever needed.
>+  if (!MaybeCheckEnforcement(aContentType, aRequestContext)) {
>+    nsCString contentURL;
>+    aContentLocation->GetAsciiSpec(contentURL);
>+    printf_stderr("CSP should be enforced! %s\n", contentURL.get());
>+    *aDecision = nsIContentPolicy::REJECT_OTHER;
>+    return NS_ERROR_CSP_NOT_ENFORCED;
>+  }
it is unclear to me why we need this special if here, and then we have normal CSP check 
later "// 2) Apply actual CSP to all loads"
This new 'if' effectively copies part of the 1) and 2) handling: it ends up check whether the load is
isPreload, just like 1) and then access csp, just like 2).
And least the duplicate nsContentUtils::IsPreloadType(aContentType) check could be avoided if this new code was after 1)
But I think this all could probably go to the step 2), so that GetVerifySignedContent check and policyCount > 0 are somehow there.

Who sets GetVerifySignedContent() to be true in remote tab case?

        Attachment #8740858 -
        Flags: review?(bugs) → review-

    
    

    
  
Comment on attachment 8741257 [details] [diff] [review]
WIP: Do CSP enforcement check before <body> appended

Hi Olli,

Besides the patch you have reviewed for CSP enforcement in every subresoruce loading, there still needs another patch to check CSP enforcement in the case that a document has no subresource at all. This patch is definitely a hacky solution but I'd like use it to demonstrate my intention and what I should do in the most correct way.

What I need is to have a CSP enforcement check right before the <body> element is appended. The reason is:

1) CSP is expected to be declared in <meta> tag.
2) Even when a document has no tag at all, <body> element will still be created.

I have totally no idea where is the best place to put do the CSP enforcement check and redirect to an error page when the check failed. What this patch does is just working but absolutely not good since it messes up with the html parser.

Do you know if there's a proper callback or observer for me to do the CSP check? What we'd to check is:

If this document is signed, it has to have CSP or we stop loading it otherwise.

Thanks!

        Attachment #8741257 -
        Flags: feedback?(bugs)

    
    

    
  

      
      
      
      

        Attached file
          
        xdiff-part2 (obsolete)
        — 
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        xdiff-part2.diff (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #8741302 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #72)
> Comment on attachment 8740858 [details] [diff] [review]
> 0002-Bug-1251152-Part-2-Do-CSP-enforcement-check.-Load-ab.patch
> 
> It would be good for someone from security team to review this too.
> 

I will request Christoph to review this as well. Thanks :)

> >+static already_AddRefed<nsIDocument>
> >+GetDocumentFromContext(nsISupports* aContext)
> >+{
> >+  nsCOMPtr<nsIDocument> doc;
> >+  nsCOMPtr<nsIContent> node = do_QueryInterface(aContext);
> >+  if (node) {
> >+      doc = node->OwnerDoc();
> >+  }
> >+  if (!doc) {
> >+      doc = do_QueryInterface(aContext);
> >+  }
> If you QIed to nsINode, you could just check its OwnerDoc().
> (nsIContent and nsIDocument both inherit nsINode)

What did you mean here? Actaully I copied these lines from nsContentPolicy::CheckPolicy. It seems to me that aContext could be |nsIContent| or just |nsIDocument|. Do I miss anything?

> But in fact I think this method isn't needed if the code was moved to be in
> CSPService::ShouldLoad
> 

Did you mean aContext is assured to be able to be QI'ed as nsIDocument or? I don't see CheckPolicy does anything special to |requestingContext| before passing to nsCSPService::ShouldLoad.

> > /* nsIContentPolicy implementation */
> > NS_IMETHODIMP
> > CSPService::ShouldLoad(uint32_t aContentType,
> >                        nsIURI *aContentLocation,
> >                        nsIURI *aRequestOrigin,
> >                        nsISupports *aRequestContext,
> >                        const nsACString &aMimeTypeGuess,
> >                        nsISupports *aExtra,
> >@@ -130,16 +198,25 @@ CSPService::ShouldLoad(uint32_t aContentType,
> >   // or type is *not* subject to CSP.
> >   // Please note, the correct way to opt-out of CSP using a custom
> >   // protocolHandler is to set one of the nsIProtocolHandler flags
> >   // that are whitelistet in subjectToCSP()
> >   if (!sCSPEnabled || !subjectToCSP(aContentLocation, aContentType)) {
> >     return NS_OK;
> >   }
> Because of early return from subjectToCSP this patch has in theory a bit
> different behavior than the previous versions of the patch.
> Is that expected?
> 
> 

Yes, it is. If this document is not subject to CSP, the CSP enforcement is meaningless to it since CSP will not have effect to it.

> >+  // Do CSP enforcement check whenever needed.
> >+  if (!MaybeCheckEnforcement(aContentType, aRequestContext)) {
> >+    nsCString contentURL;
> >+    aContentLocation->GetAsciiSpec(contentURL);
> >+    printf_stderr("CSP should be enforced! %s\n", contentURL.get());
> >+    *aDecision = nsIContentPolicy::REJECT_OTHER;
> >+    return NS_ERROR_CSP_NOT_ENFORCED;
> >+  }
> it is unclear to me why we need this special if here, and then we have
> normal CSP check 
> later "// 2) Apply actual CSP to all loads"
> This new 'if' effectively copies part of the 1) and 2) handling: it ends up
> check whether the load is
> isPreload, just like 1) and then access csp, just like 2).
> And least the duplicate nsContentUtils::IsPreloadType(aContentType) check
> could be avoided if this new code was after 1)
> But I think this all could probably go to the step 2), so that
> GetVerifySignedContent check and policyCount > 0 are somehow there.
> 

Because if a document needs to be signed (i.e. GetVerifySignedContent() is true), we have to ensure that it has CSP policies defined in <meta> tag (no matter what they are). If a document has no CSP, no CSPContext::ShouldLoad will do nothing at all. So, we have to make sure the document does have CSP before actually doing CSPContext::ShouldLoad.

> Who sets GetVerifySignedContent() to be true in remote tab case?

AboutRedirector sets it in [1].

[1] https://dxr.mozilla.org/mozilla-central/source/browser/components/about/AboutRedirector.cpp#175

    
    

    
  
(In reply to Henry Chang [:henry] from comment #77)
> > >+GetDocumentFromContext(nsISupports* aContext)
> > >+{
> > >+  nsCOMPtr<nsIDocument> doc;
> > >+  nsCOMPtr<nsIContent> node = do_QueryInterface(aContext);
> > >+  if (node) {
> > >+      doc = node->OwnerDoc();
> > >+  }
> > >+  if (!doc) {
> > >+      doc = do_QueryInterface(aContext);
> > >+  }
> > If you QIed to nsINode, you could just check its OwnerDoc().
> > (nsIContent and nsIDocument both inherit nsINode)
> 
> What did you mean here? Actaully I copied these lines from
> nsContentPolicy::CheckPolicy. It seems to me that aContext could be
> |nsIContent| or just |nsIDocument|. Do I miss anything?
No need to QI to nsIContent or nsIDocument, when you can just do

nsCOMPtr<nsINode> node = do_QueryInterface(aContext);
return node ? node->OwnerDoc() : nullptr;
(I don't see reason for the method to return already_AddRefed, nsIDocument* should be fine.)


> Did you mean aContext is assured to be able to be QI'ed as nsIDocument or? I
> don't see CheckPolicy does anything special to |requestingContext| before
> passing to nsCSPService::ShouldLoad.
I mean that nsCSPService::ShouldLoad already has code 
nsCOMPtr<nsINode> node(do_QueryInterface(aRequestContext));
and if that node is non-null, node->OwnerDoc(); give the document.

> > it is unclear to me why we need this special if here, and then we have
> > normal CSP check 
> > later "// 2) Apply actual CSP to all loads"
> > This new 'if' effectively copies part of the 1) and 2) handling: it ends up
> > check whether the load is
> > isPreload, just like 1) and then access csp, just like 2).
> > And least the duplicate nsContentUtils::IsPreloadType(aContentType) check
> > could be avoided if this new code was after 1)
> > But I think this all could probably go to the step 2), so that
> > GetVerifySignedContent check and policyCount > 0 are somehow there.
> > 
> 
> Because if a document needs to be signed (i.e. GetVerifySignedContent() is
> true), we have to ensure that it has CSP policies defined in <meta> tag (no
> matter what they are). If a document has no CSP, no CSPContext::ShouldLoad
> will do nothing at all. So, we have to make sure the document does have CSP
> before actually doing CSPContext::ShouldLoad.
But why not do all the checks as part of the existing (2), or certainly after the existing (1)?
(using the numbers from the current source code.)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #78)

> > Did you mean aContext is assured to be able to be QI'ed as nsIDocument or? I
> > don't see CheckPolicy does anything special to |requestingContext| before
> > passing to nsCSPService::ShouldLoad.
> I mean that nsCSPService::ShouldLoad already has code 
> nsCOMPtr<nsINode> node(do_QueryInterface(aRequestContext));
> and if that node is non-null, node->OwnerDoc(); give the document.
> 

I wasn't aware of this! Thanks!
 
> > Because if a document needs to be signed (i.e. GetVerifySignedContent() is
> > true), we have to ensure that it has CSP policies defined in <meta> tag (no
> > matter what they are). If a document has no CSP, no CSPContext::ShouldLoad
> > will do nothing at all. So, we have to make sure the document does have CSP
> > before actually doing CSPContext::ShouldLoad.
> But why not do all the checks as part of the existing (2), or certainly
> after the existing (1)?
> (using the numbers from the current source code.)

The intention of |MaybeCheckCspEnforcement| is not doing any actual CSP check. It just ensures a document does have any CSP policy so that the |ShoudlLoad| below would have policy entries to check. (1) and (2) is normal CSP policy check and is something beyond policy check. Do I clarify your confusions?

Christoph, could you also have a review of patch part 2? It basically remains unchanged from what you have reviewed before. Only some of the enforcement code is moved to nsCSPService::Should. Thanks :)

    
    

    
  
(In reply to Henry Chang [:henry] from comment #79)
> (In reply to Olli Pettay [:smaug] from comment #78)
> 
> > > Did you mean aContext is assured to be able to be QI'ed as nsIDocument or? I
> > > don't see CheckPolicy does anything special to |requestingContext| before
> > > passing to nsCSPService::ShouldLoad.
> > I mean that nsCSPService::ShouldLoad already has code 
> > nsCOMPtr<nsINode> node(do_QueryInterface(aRequestContext));
> > and if that node is non-null, node->OwnerDoc(); give the document.
> > 
> 
> I wasn't aware of this! Thanks!
>  
> > > Because if a document needs to be signed (i.e. GetVerifySignedContent() is
> > > true), we have to ensure that it has CSP policies defined in <meta> tag (no
> > > matter what they are). If a document has no CSP, no CSPContext::ShouldLoad
> > > will do nothing at all. So, we have to make sure the document does have CSP
> > > before actually doing CSPContext::ShouldLoad.
> > But why not do all the checks as part of the existing (2), or certainly
> > after the existing (1)?
> > (using the numbers from the current source code.)
> 
> The intention of |MaybeCheckCspEnforcement| is not doing any actual CSP
> check. It just ensures a document does have any CSP policy so that the
> |ShoudlLoad| below would have policy entries to check. (1) and (2) is normal
> CSP policy check and is something beyond policy check. Do I clarify your
> confusions?
not really. We don't do (1) for the cases the we're adding support in this bug, so why do we need to check 
nsContentUtils::IsPreloadType(aContentType) in two places, when we could simplify the code and check that only once.
Perhaps the new code should be between (1) and (2), and then rename (2) to (3) and explain what the new (2) is  - something that it is a special case for GetVerifySignedContent() stuff.

    
    

    
  
Comment on attachment 8741257 [details] [diff] [review]
WIP: Do CSP enforcement check before <body> appended

What if there isn't body element at all? Say, you're loading an xhtml document without any <body>.
And calling Stop() in middle of a "DocUpdate" is most propably unsafe, or at least might lead to some assertions.


I have to admit, it isn't clear what this patch is about. This really needs feedback from someone who understand better what this bug is supposed to fix :)

        Attachment #8741257 -
        Flags: feedback?(bugs)

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #81)
> Comment on attachment 8741257 [details] [diff] [review]
> WIP: Do CSP enforcement check before <body> appended
> 
> What if there isn't body element at all? Say, you're loading an xhtml
> document without any <body>.

I haven't figured out how it works but I am certain that the <body> element will be created even if a document has only plaintext.

> And calling Stop() in middle of a "DocUpdate" is most propably unsafe, or at
> least might lead to some assertions.
> 

My initial attempt was to call Stop() in HTMLBodyElement::BindToTree and it does lead to assertions. However, after moving the invocation of Stop() to nsHtml5TreeOperation::Append, there's no assertion occurred. Could you suggest what kind and where might the assertion occur so that I can have a check?

> 
> I have to admit, it isn't clear what this patch is about. This really needs
> feedback from someone who understand better what this bug is supposed to fix
> :)

The patch is to deal with the following case:

If a document needs to be signed (e.g. remote-newtab) but it has no CSP defined in the <meta> element, we need to throw an error and redirect to a fallback page (maybe the local newtab). The soonest timing when we are able to decide to stop document load or not is when after </head> is parsed. Considering a document may have no <head> section, I do the check in the <body> construction time. In other words, this patch is to stop the document loading as soon as we can determine it has a CSP enforcement error. (the condition was mentioned in the first sentence of this paragraph.)

NI Frenzikus and Christoph to see if they want to add something I missed.

Thanks :)
Flags: needinfo?(franziskuskiefer)

    
  
Flags: needinfo?(ckerschb)

    
    

    
  
(In reply to Henry Chang [:henry] from comment #82)
> (In reply to Olli Pettay [:smaug] from comment #81)
> > Comment on attachment 8741257 [details] [diff] [review]
> > WIP: Do CSP enforcement check before <body> appended
> > 
> > What if there isn't body element at all? Say, you're loading an xhtml
> > document without any <body>.
> 
> I haven't figured out how it works but I am certain that the <body> element
> will be created even if a document has only plaintext.
plaintext sure, but I was talking about xhtml. As an example:
data:application/xhtml+xml,<html xmlns="http://www.w3.org/1999/xhtml"></html>

> My initial attempt was to call Stop() in HTMLBodyElement::BindToTree and it
> does lead to assertions. However, after moving the invocation of Stop() to
> nsHtml5TreeOperation::Append, there's no assertion occurred. Could you
> suggest what kind and where might the assertion occur so that I can have a
> check?
IIRC Stop() might lead to dispatching events and if we're in an "update" there might be scriptblockers on stack during which dispatching DOM Events will assert. Perhaps call Stop() when the 'update' goes out from stack?

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #80)
> (In reply to Henry Chang [:henry] from comment #79)
> > (In reply to Olli Pettay [:smaug] from comment #78)
> > 
> > > > Did you mean aContext is assured to be able to be QI'ed as nsIDocument or? I
> > > > don't see CheckPolicy does anything special to |requestingContext| before
> > > > passing to nsCSPService::ShouldLoad.
> > > I mean that nsCSPService::ShouldLoad already has code 
> > > nsCOMPtr<nsINode> node(do_QueryInterface(aRequestContext));
> > > and if that node is non-null, node->OwnerDoc(); give the document.
> > > 
> > 
> > I wasn't aware of this! Thanks!
> >  
> > > > Because if a document needs to be signed (i.e. GetVerifySignedContent() is
> > > > true), we have to ensure that it has CSP policies defined in <meta> tag (no
> > > > matter what they are). If a document has no CSP, no CSPContext::ShouldLoad
> > > > will do nothing at all. So, we have to make sure the document does have CSP
> > > > before actually doing CSPContext::ShouldLoad.
> > > But why not do all the checks as part of the existing (2), or certainly
> > > after the existing (1)?
> > > (using the numbers from the current source code.)
> > 
> > The intention of |MaybeCheckCspEnforcement| is not doing any actual CSP
> > check. It just ensures a document does have any CSP policy so that the
> > |ShoudlLoad| below would have policy entries to check. (1) and (2) is normal
> > CSP policy check and is something beyond policy check. Do I clarify your
> > confusions?
> not really. We don't do (1) for the cases the we're adding support in this
> bug, so why do we need to check 
> nsContentUtils::IsPreloadType(aContentType) in two places, when we could
> simplify the code and check that only once.
> Perhaps the new code should be between (1) and (2), and then rename (2) to
> (3) and explain what the new (2) is  - something that it is a special case
> for GetVerifySignedContent() stuff.

What (1) is doing is to check the preload resource against preloadCsp (the preloadCsp is the CSP defined in <meta> tag.) It totally has nothing to do with the CSP enforcement check. From what I can find, the only benefit to put |MaybeCheckCspEnforcement| between (1) and (2) is to save a |IsPreloadType| call. Is it what you want or there's anything I missed?

    
    

    
  
(In reply to Henry Chang [:henry] from comment #84)
> From what I can find, the only benefit to
> put |MaybeCheckCspEnforcement| between (1) and (2) is to save a
> |IsPreloadType| call. Is it what you want or there's anything I missed?

That. Hoping to get the code easier to read by not checking the same condition many times.

    
    

    
  
> If a document needs to be signed (e.g. remote-newtab) but it has no CSP
> defined in the <meta> element, we need to throw an error and redirect to a
> fallback page (maybe the local newtab). The soonest timing when we are able
> to decide to stop document load or not is when after </head> is parsed.
> Considering a document may have no <head> section, I do the check in the
> <body> construction time. In other words, this patch is to stop the document
> loading as soon as we can determine it has a CSP enforcement error. (the
> condition was mentioned in the first sentence of this paragraph.)

right, that's the use case. I'm not sure about xhtml and not having a body.
> data:application/xhtml+xml,<html xmlns="http://www.w3.org/1999/xhtml"></html>
this is a correct example, but also an empty page. Adding text in there requires a body tag afaik. So checking the head element and body should be fine I think.
Flags: needinfo?(franziskuskiefer)

    
    

    
  
There has been a lot of discussion in this bug since I last checked. Before we invest any more time implementing, we should all make sure we agree on the approach.

Basically CSP can be set either through the CSP header or through the meta tag. *If* there is a CSP to enforce on a site we definitely get a call to |ApplySettingsFromCSP()|. Unfortunately if there is no CSP on the site, we don't get that call. What makes implementing this feature hard is that there are no guarantees that a site uses a <head> tag and also no guarantees a site has a body tag either, so we can't check at the end of the <head> that we have a CSP and we also can't check at the beginning of <body> that we have a CSP on the site.

One option would be, that about:newtab needs to enforce CSP deliverd through a header. I can't think of any central point after ::StartDocumentLoad() where we can enforce that all sites have a CSP either through the header or through the meta tag.

I am open for further discussions.
Flags: needinfo?(ckerschb)

    
    

    
  
Olivier, I can't recall if there is a specific need that CSP for about:newtab can be delivered through the meta tag. Is there? Or could we agree that CSP needs to be delivered using the header only, which would simplify things a lot (see also comment 87).
Flags: needinfo?(oyiptong)

    
    

    
  
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #88)
> Olivier, I can't recall if there is a specific need that CSP for
> about:newtab can be delivered through the meta tag. Is there? Or could we
> agree that CSP needs to be delivered using the header only, which would
> simplify things a lot (see also comment 87).

What I learned from Olivier and Frenziskus is, since we don't trust any of the CDN servers as well as the HTTP headers sent from them, we would only accept CSP defined in <meta>. (indicated by Frenziskus.)

    
    

    
  
(In reply to Henry Chang [:henry] from comment #89)
> What I learned from Olivier and Frenziskus is, since we don't trust any of
> the CDN servers as well as the HTTP headers sent from them, we would only
> accept CSP defined in <meta>. (indicated by Frenziskus.)

Ah yes, I forgot about that. In that case I suppose we can agree that remote newtab pages need to ship with a Meta CSP and fall back to a local newpage site if that is not the case.

I briefly chatted with Franziskus and we both think it might be desirable to implement CSP enforcement for about:newtab in the following way:
1) Remote newtab pages need to have a <head> -> Otherwise fall back to use local URI.
2) Remote newtab pages need to have a meta csp within the head -> Otherwise fall back to use local URI.


Now my question for Henri who knows the html5 parser bits inside out: Is it possible to make sure that a site has a head and also a Meta CSP within that head? If so, where might be a code spot in the code where we could add such an enforcement check?
Flags: needinfo?(hsivonen)

    
    

    
  
Supporting something like CSP in <meta> is badness.

What happens with <script> and <link> before the <meta>? What happens to speculative loads made from pre-parsing before the <meta> truly ends up in the DOM? What happens if a script mutates the DOM? What happens with application/xhtml+xml?

If the use case is the new tab page, I think it would make sense to make whatever channel you are loading the HTML from support headers equivalent to HTTP headers in Necko instead of keeping up the fiction that http-equiv in HTML could actually be HTTP-equivalent.
Flags: needinfo?(hsivonen)

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #91)
> Supporting something like CSP in <meta> is badness.

There is definitely some truth in that.

> What happens with <script> and <link> before the <meta>? What happens to
> speculative loads made from pre-parsing before the <meta> truly ends up in
> the DOM? What happens if a script mutates the DOM? What happens with
> application/xhtml+xml?

CSP spec strongly encourages authors to place the meta element as early in the document as possible. If someone includes <script> before placing the meta element in the head then obviously meta csp can't block the load. When implementing meta tag support for CSP (Bug 663570) we already discussed most (if not all of your concerns). E.g. within Gecko we support a speculate CSP which is applied to preloads but might be invalidated if e.g. a document.write() comments out the meta CSP.

[1] https://www.w3.org/TR/2014/WD-CSP2-20140703/#delivery-html-meta-element

> If the use case is the new tab page, I think it would make sense to make
> whatever channel you are loading the HTML from support headers equivalent to
> HTTP headers in Necko instead of keeping up the fiction that http-equiv in
> HTML could actually be HTTP-equivalent.

I suppose the threat model is that we don't trust headers generated by a CDN, but we do trust a signed page what we uploaded to the CDN. So I suppose having Meta CSP within the remote about:newtab page is what we want.
Besides the badness of Meta CSP, do you think there is a place in our codebase where we could make sure that a remote newtab site is actually shipped including a meta CSP?


Btw, I suppose the ni? for Oliver is not needed anymore.
Flags: needinfo?(oyiptong) → needinfo?(hsivonen)

    
    

    
  
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #90)
> (In reply to Henry Chang [:henry] from comment #89)
> > What I learned from Olivier and Frenziskus is, since we don't trust any of
> > the CDN servers as well as the HTTP headers sent from them, we would only
> > accept CSP defined in <meta>. (indicated by Frenziskus.)
> 
> Ah yes, I forgot about that. In that case I suppose we can agree that remote
> newtab pages need to ship with a Meta CSP and fall back to a local newpage
> site if that is not the case.
> 
> I briefly chatted with Franziskus and we both think it might be desirable to
> implement CSP enforcement for about:newtab in the following way:
> 1) Remote newtab pages need to have a <head> -> Otherwise fall back to use
> local URI.

This seems pretty hard to me :( Requires Henri's feedback.

> 2) Remote newtab pages need to have a meta csp within the head -> Otherwise
> fall back to use local URI.
> 

This is semi-hard to me: We could do the CSP enforcement check in HTMLMetaElement::BindToTree. 
However, when and where to request to stop document loading is still not clear to me.
I was trying to do that in |nsHtml5TreeOperation::Append| but :smaug mentioned that it might lead
to a assertion. 

Anyways, I will be figuring out how to stop document loading whenever the CSP enforcement check fails.

    
    

    
  
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #92)
> (In reply to Henri Sivonen (:hsivonen) from comment #91)
> CSP spec strongly encourages authors to place the meta element as early in
> the document as possible. If someone includes <script> before placing the
> meta element in the head then obviously meta csp can't block the load. When
> implementing meta tag support for CSP (Bug 663570) we already discussed most
> (if not all of your concerns). E.g. within Gecko we support a speculate CSP
> which is applied to preloads but might be invalidated if e.g. a
> document.write() comments out the meta CSP.

I didn't realize that we already supported CSP via <meta>. :-(

> [1] https://www.w3.org/TR/2014/WD-CSP2-20140703/#delivery-html-meta-element
> 
> > If the use case is the new tab page, I think it would make sense to make
> > whatever channel you are loading the HTML from support headers equivalent to
> > HTTP headers in Necko instead of keeping up the fiction that http-equiv in
> > HTML could actually be HTTP-equivalent.
> 
> I suppose the threat model is that we don't trust headers generated by a
> CDN, but we do trust a signed page what we uploaded to the CDN. So I suppose
> having Meta CSP within the remote about:newtab page is what we want.

If you check a signature (where's that delivered?), you need some special-casing in Necko to avoid passing the start of the stream to the HTML parser before the whole HTTP resource has been received and the signature validated.

When you need a special case in Necko anyway, why not make Necko add a strict CSP to remote new tab pages? Why take the risk of letting the page itself deliver the CSP via <meta>?

> Besides the badness of Meta CSP, do you think there is a place in our
> codebase where we could make sure that a remote newtab site is actually
> shipped including a meta CSP?

Checking that it included a <meta> CSP isn't enough if it includes scripts before the CSP <meta>, right? Does it matter what the behavior is on failure?

One option, which would add a perf penalty of at least one branch to every parser-created element on every page would be to make nsHtml5TreeOperation::CreateElement() track if it has seen elements other than <html> and <head>. If the page is a remote tab page and the first non-<html>, non-<head> element is a CSP <meta>, mark things as being OK (keeping the flag on aBuilder) and stop checking. If the first non-<html>, non-<head> element is something else, call aBuilder->MarkAsBroken().

This wouldn't cover XHTML, of course.
Flags: needinfo?(hsivonen)

    
    

    
  
What I can answer is:

(In reply to Henri Sivonen (:hsivonen) from comment #94)
> If you check a signature (where's that delivered?), you need some
> special-casing in Necko to avoid passing the start of the stream to the HTML
> parser before the whole HTTP resource has been received and the signature
> validated.
> 

Correct! That has been done in necko.

> When you need a special case in Necko anyway, why not make Necko add a
> strict CSP to remote new tab pages? Why take the risk of letting the page
> itself deliver the CSP via <meta>?
> 

We already have similar enforcement for webapps [1]. In comment 3, it's mentioned that hard-coded CSP isn't desired.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#2871

    
    

    
  
(In reply to Henri Sivonen (:hsivonen) from comment #94)
> When you need a special case in Necko anyway, why not make Necko add a
> strict CSP to remote new tab pages? Why take the risk of letting the page
> itself deliver the CSP via <meta>?

Since I wasn't there when the decision was made (per comment 3), NI related people to see what feedback they'd like to give regarding the "applying default CSP (via pref, maybe) on signed contents".
Flags: needinfo?(oyiptong)
Flags: needinfo?(franziskuskiefer)
Flags: needinfo?(ckerschb)

    
    

    
  
Having the default CSP in a pref is a solution that would work.

We want to keep the policy in something we can change (possibly via add-on updates) so we can run experiments.

We still need to find a good default CSP.
Flags: needinfo?(oyiptong)
Flags: needinfo?(franziskuskiefer)
Flags: needinfo?(ckerschb)

    
    

    
  
(In reply to Olivier Yiptong [:oyiptong] from comment #97)
> Having the default CSP in a pref is a solution that would work.
> 
> We want to keep the policy in something we can change (possibly via add-on
> updates) so we can run experiments.
> 
> We still need to find a good default CSP.

Sweet! I'll provide a new patch for the new solution soon. Thanks!

    
    

    
  


  

        Attachment #8734271 -
        Attachment is obsolete: true

        Attachment #8734273 -
        Attachment is obsolete: true

        Attachment #8740858 -
        Attachment is obsolete: true

        Attachment #8741257 -
        Attachment is obsolete: true

        Attachment #8741304 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Test case (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Test case (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #8745291 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8745290 [details] [diff] [review]
Apply default CSP to signed contents

Hi Christoph, 

Could you have a review for my implementation using default CSP on the signed content? The default CSP for the signed content can be set by pref. The test case is in another patch. Thanks!

        Attachment #8745290 -
        Flags: review?(ckerschb)

    
    

    
  
Comment on attachment 8745292 [details] [diff] [review]
Test case

Hi Frenziskus,

Could you review the test which tests if the default CSP is properly applied to the signed content? Thanks!

        Attachment #8745292 -
        Flags: review?(franziskuskiefer)

    
    

    
  
Comment on attachment 8745292 [details] [diff] [review]
Test case

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

lgtm, but is this the only test? It should be enough in theory but I don't really trust it. So maybe a newtab page that violates the default CSP would be nice. And then check that the load gets blocked and the fallback is loaded.

::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js
@@ +135,5 @@
>              "sanity check: default URL for about:newtab should return the new URL");
>        }
> +
> +      // If the original URL is about:newtab and the will be redirected to
> +      // a well-signed content, the document should have a default CSP applied.

I don't really get this comment :)

        Attachment #8745292 -
        Flags: review?(franziskuskiefer)

    
    

    
  
Comment on attachment 8745290 [details] [diff] [review]
Apply default CSP to signed contents

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

Changes look good, but I am not sure if we should land the default CSP as is. Can you please check with Olivier?

::: dom/base/nsDocument.cpp
@@ +2819,5 @@
>        }
>      }
>    }
>  
> + bool applySignedContentCSP = IsSignedContent(aChannel);

I am fine with inlining the content of IsSignedContent here. No need for a new static function in my opinion.

@@ +2886,5 @@
>      csp->AppendPolicy(appManifestCSP, false, false);
>    }
>  
> +  // ----- if the doc is a signed content, apply the default CSP in the pref.
> +  if (applySignedContentCSP) {

We need to be careful here in the future. Once content signing becomes a standart API (if at all) then we would accidentaly apply the default CSP here which is probably not we want.

::: modules/libpref/init/all.js
@@ +2041,5 @@
>  pref("security.apps.privileged.CSP.default", "default-src * data: blob:; script-src 'self'; object-src 'none'; style-src 'self' 'unsafe-inline'");
>  
> +// Default Content Security Policy to apply to signed contents.
> +pref("security.signed_content.CSP.default", "script-src 'self' 'unsafe-inline'; style-src 'self'");
> +

Do we really need the unsafe-inline here? I briefly looked at [1] and it seems it's not that case to include unsafe-inline. We should check with Olivier.

[1] https://github.com/mozilla/remote-newtab/issues/148

        Attachment #8745290 -
        Flags: review?(ckerschb) → feedback+

    
    

    
  
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #104)
> Comment on attachment 8745292 [details] [diff] [review]
> Test case
> 
> Review of attachment 8745292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm, but is this the only test? It should be enough in theory but I don't
> really trust it. 
  ^^^^^^^^^^^^^^^

Did you mean you don't trust CSP feature itself? If a document's nodePrincipal does have CSP 
and still can be violated, it should be caught by CSP test cases. We don't need to re-write a CSP
test case here, do we? 

> So maybe a newtab page that violates the default CSP would
> be nice. And then check that the load gets blocked and the fallback is
> loaded.

Oop I totally forgot this but I don't have a scalable solution for now. The reason is, there's a variety of ways to violate CSP and we have to override the default CSP violation handler individually :( What I can think of are 

- resource loading violation
- unsafe-inline violation
- unsafe-eval violation
- ...

> 
> ::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js
> @@ +135,5 @@
> >              "sanity check: default URL for about:newtab should return the new URL");
> >        }
> > +
> > +      // If the original URL is about:newtab and the will be redirected to
> > +      // a well-signed content, the document should have a default CSP applied.
> 
> I don't really get this comment :)

Since some of the test cases are not "about:newtab" or the remote newtab is not properly signed, we only check the CSP for those contents which are valid remote newtab.

    
    

    
  
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #105)
> Comment on attachment 8745290 [details] [diff] [review]
> Apply default CSP to signed contents
> 
> Review of attachment 8745290 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changes look good, but I am not sure if we should land the default CSP as
> is. Can you please check with Olivier?

Sure! Or just leave it blank and to be set in firefox.js?

> 
> @@ +2886,5 @@
> >      csp->AppendPolicy(appManifestCSP, false, false);
> >    }
> >  
> > +  // ----- if the doc is a signed content, apply the default CSP in the pref.
> > +  if (applySignedContentCSP) {
> 
> We need to be careful here in the future. Once content signing becomes a
> standart API (if at all) then we would accidentaly apply the default CSP
> here which is probably not we want.
> 

Or, we strictly use something like "remote content" (but not "signed content") and 
enforce the "remote content" to be the subset of "signed content" (which means 
remote contents MUST be signed). Then we define and apply default CSP on "remote content" 
rather than "signed content". What do you think?

    
  
Flags: needinfo?(ckerschb)

    
  
Flags: needinfo?(franziskuskiefer)

    
  
Flags: needinfo?(oyiptong)

    
    

    
  
> Did you mean you don't trust CSP feature itself? If a document's nodePrincipal does have CSP 
> and still can be violated, it should be caught by CSP test cases. We don't need to re-write a CSP
> test case here, do we? 
We trust the CSP, but we don't know what's happening if it fails. Do we simply block? Or de do a fallback? So we need a test case that tests that in case of CSP violation we do the right thing.
Flags: needinfo?(franziskuskiefer)

    
    

    
  
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #108)
> > Did you mean you don't trust CSP feature itself? If a document's nodePrincipal does have CSP 
> > and still can be violated, it should be caught by CSP test cases. We don't need to re-write a CSP
> > test case here, do we? 
> We trust the CSP, but we don't know what's happening if it fails. Do we
> simply block? Or de do a fallback? So we need a test case that tests that in
> case of CSP violation we do the right thing.

Agreed. So what do you think of the CSP violation fallback? At least I can find all possible violation spots and redirect to the fallback. Maybe you or Christopher has better solution?

    
    

    
  
(In reply to Henry Chang [:henry] from comment #107)
> > Changes look good, but I am not sure if we should land the default CSP as
> > is. Can you please check with Olivier?
> 
> Sure! Or just leave it blank and to be set in firefox.js?

Whatever is easiest for the folks writing the remote:newtab site. I am fine either way, but I would like to have 'unsafe-inline' removed if possible.

> Or, we strictly use something like "remote content" (but not "signed
> content") and 
> enforce the "remote content" to be the subset of "signed content" (which
> means 
> remote contents MUST be signed). Then we define and apply default CSP on
> "remote content" 
> rather than "signed content". What do you think?

Yeah, we could do that, but I am not sure it's worth the time. Who knows if content signing ever becomes a standard? I just wanted to point it out, so we keep it in mind.
Flags: needinfo?(ckerschb)

    
    

    
  


  

        Attachment #8745290 -
        Attachment is obsolete: true

        Attachment #8745292 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Part 3: Test case (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #8747029 -
        Flags: review?(ckerschb)

    
  

        Attachment #8747030 -
        Flags: review?(oyiptong)

    
  

        Attachment #8747031 -
        Flags: review?(franziskuskiefer)

    
    

    
  
Hi Christopher, Olivier, Franziskus,

Could you help review the patches with all comments addressed? I moved the actual default CSP definition to an individual patch to be reviewed by Olivier and added a test case which has inline scripts which should be blocked due to the CSP violation. Other than those, most of the changes are the same. Thanks!

    
    

    
  
Comment on attachment 8747030 [details] [diff] [review]
Part 2: Define default signed content CSP in all.js

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

These are good CSP rules to start out with. We can tighten as we go

        Attachment #8747030 -
        Flags: review?(oyiptong) → review+

    
    

    
  
The rules are good right now since they are contained in a pref.

Should we want to tighten the CSP, e.g. disabling unsafe-inline, we'd need to have CSP application follow the same rules as content signature verification.

i.e. the CSP should be applied for production and staging, but not in test, test2 or dev resource URLs:
https://dxr.mozilla.org/mozilla-central/source/browser/components/newtab/NewTabRemoteResources.jsm
Flags: needinfo?(oyiptong)

    
    

    
  
The comment above can be applied later. For now, having those rules applied is definitely better than not having them.

    
    

    
  
Comment on attachment 8747031 [details] [diff] [review]
Part 3: Test case

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

::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js
@@ +81,5 @@
>      STYLESHEET_WITH_SRI_LOADED,
>      SCRIPT_WITHOUT_SRI_BLOCKED,
>      SCRIPT_WITH_SRI_LOADED,
>      ]},
> +  { "aboutURI" : URI_BAD_CSP, "testStrings" : [CSP_TEST_SUCCESS_STRING] },

Maybe I'm missing something but I don't think this tests what you want to test. In this test the CSP should block part of the load, right? So testing for "CSP violation test succeeded." which is always in the result doesn't really help. Shouldn't you check that "CSP violation test failed." is _not_ in there?

@@ +148,5 @@
>              "sanity check: default URL for about:newtab should return the new URL");
>        }
> +
> +      // If the original URL is about:newtab and the will be redirected to
> +      // a well-signed content, the document should have a default CSP applied.

I still don't get this comment :)

        Attachment #8747031 -
        Flags: review?(franziskuskiefer)

    
    

    
  
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #118)
> Comment on attachment 8747031 [details] [diff] [review]
> Part 3: Test case
> 
> Review of attachment 8747031 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/security/test/contentverifier/browser_verify_content_about_newtab.js
> @@ +81,5 @@
> >      STYLESHEET_WITH_SRI_LOADED,
> >      SCRIPT_WITHOUT_SRI_BLOCKED,
> >      SCRIPT_WITH_SRI_LOADED,
> >      ]},
> > +  { "aboutURI" : URI_BAD_CSP, "testStrings" : [CSP_TEST_SUCCESS_STRING] },
> 
> Maybe I'm missing something but I don't think this tests what you want to
> test. In this test the CSP should block part of the load, right? So testing
> for "CSP violation test succeeded." which is always in the result doesn't
> really help. Shouldn't you check that "CSP violation test failed." is _not_
> in there?
> 

If the inline script is blocked, "CSP violation test succeeded." will be there. 
If the inline script is NOT blocked, "CSP violation test succeeded." will be overwritten to "CSP violation test failed."

Checking the existence of "CSP violation test failed." is 100% equivalent to checking "CSP violation test succeeded." Since what we check is the entire document (but not the body content only), we cannot just
check if "CSP violation test failed." exists (it always exists). We have change the comparison string
(like the body content only) OR write something like 

<script>
  document.body.innerHTML = "CSP violation" + "test failed";
</script>

to avoid "CSP violation test failed" being found.

However, I still think checking if "CSP violation test succeeded." is overwritten is already representative and no exception :)

> @@ +148,5 @@
> >              "sanity check: default URL for about:newtab should return the new URL");
> >        }
> > +
> > +      // If the original URL is about:newtab and the will be redirected to
> > +      // a well-signed content, the document should have a default CSP applied.
> 
> I still don't get this comment :)

Do you mean the wording or what I'd like to express? What it's supposed to express is we only check 
those tests which is about:newtab (some of the tests are not about:newtab) and is redirected a valid
remote content. For example, if a about:newtab is redirected to a content with invalid signature,
then it will be redirected to other page, which will not have the default signed content CSP.

Thanks :)
Flags: needinfo?(franziskuskiefer)

    
    

    
  
uh, right it's an innerHTML =, not +=. So all good.

> > > +      // If the original URL is about:newtab and the will be redirected to
> > > +      // a well-signed content, the document should have a default CSP applied.

This is not a correct English sentence and I'm not sure what you want to say with it.

> What it's supposed to express is we only check 
> those tests which is about:newtab (some of the tests are not about:newtab) and is redirected a valid
> remote content. For example, if a about:newtab is redirected to a content with invalid signature,
> then it will be redirected to other page, which will not have the default signed content CSP.

hm, so maybe something like "Every valid newtab page must pass the built in CSP."? We don't check that the page has a CSP but only that the remote newtab page passes the CSP and is displayed correctly.
Flags: needinfo?(franziskuskiefer)

    
    

    
  
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #120)
> uh, right it's an innerHTML =, not +=. So all good.
> 

Sweet :)

> > > > +      // If the original URL is about:newtab and the will be redirected to
> > > > +      // a well-signed content, the document should have a default CSP applied.
> 
> This is not a correct English sentence and I'm not sure what you want to say
> with it.
> 
> > What it's supposed to express is we only check 
> > those tests which is about:newtab (some of the tests are not about:newtab) and is redirected a valid
> > remote content. For example, if a about:newtab is redirected to a content with invalid signature,
> > then it will be redirected to other page, which will not have the default signed content CSP.
> 
> hm, so maybe something like "Every valid newtab page must pass the built in
> CSP."? We don't check that the page has a CSP but only that the remote
> newtab page passes the CSP and is displayed correctly.

So do I just fix the comment and ask for review again? Thanks!
Flags: needinfo?(franziskuskiefer)

    
    

    
  
> So do I just fix the comment and ask for review again? Thanks!

yep :)
Flags: needinfo?(franziskuskiefer)

    
    

    
  
Comment on attachment 8747030 [details] [diff] [review]
Part 2: Define default signed content CSP in all.js

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

::: modules/libpref/init/all.js
@@ +2040,5 @@
>  // Default Content Security Policy to apply to privileged apps.
>  pref("security.apps.privileged.CSP.default", "default-src * data: blob:; script-src 'self'; object-src 'none'; style-src 'self' 'unsafe-inline'");
>  
> +// Default Content Security Policy to apply to signed contents.
> +pref("security.signed_content.CSP.default", "script-src 'self'; style-src 'self'");

Would be nice if we could also define default-src. Olivier, maybe we should sit down at one point and look at the structure of such a remote newpage.

    
    

    
  
Comment on attachment 8747029 [details] [diff] [review]
Part 1: Apply default CSP to signed contents

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

thanks, r=me

        Attachment #8747029 -
        Flags: review?(ckerschb) → review+

    
    

    
  


  

        Attachment #8747031 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #120)
> hm, so maybe something like "Every valid newtab page must pass the built in
> CSP."? We don't check that the page has a CSP but only that the remote
> newtab page passes the CSP and is displayed correctly.

Maybe you understood something here :p

What we check with

"is(browser.contentDocument.nodePrincipal.cspJSON, SIGNED_CONTENT_CSP, ...)"

is that if a page (document) has a CSP in its node principal.

Whether a page would be displayed correctly is NOT this one is intended to test.
(it's tested by finding "CSP violation test succeeded." in the document.innerHTML).

In other words, we check both if the valid remote newtab pages PASS and HAVE CSP :)

    
  

        Attachment #8747819 -
        Flags: review?(franziskuskiefer)

    
  

        Attachment #8747819 -
        Flags: review?(franziskuskiefer) → review+

    
  
Keywords: checkin-needed

    
    

    
  
So glad to see this bug finally being landed!
Thank you Henry and all the reviewers here!

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: