1449845 - Add assertion that all new about: pages ship with a CSP

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P1)

Description

[Christoph Kerschbaumer [:ckerschb]]

Moving forward with our model that all about: pages should end up with a CSP we should add an assertion that all about: pages ship with a CSP. We started to apply CSPs to content-privileged about: pages (see Bug  1436808) and we will continue to add CSPs to already existing about: pages.

Initially we have to whitelist already existing about: pages, but the assertion can trigger for new about: pages that are going to be added. Over time that whitelist will be smaller and smaller and we end up with an assertion to ensure that all about: pages have a CSP.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1449845_assert_about_page_has_csp.patch

Ultimately we would like to ensure that new about: pages can not be deployed without a CSP. I am not entirely sure if EndLoad() is the right place for it and I am happy to move it somewhere else, but please note that about: pages will ship a CSP using the Meta Tag, so I am not sure what other place to put the assertion.

Additonal note: We started to add CSP to some like 7 content privileged about pages (see Bug 1436808) and we will continue to add CSP to more content-priviled about pages. Ultimately we would also like to shop system privileged about pages with a CSP, but in order to do that we have to get the CSP off the Principal first (see Bug 965637).

Comment 2

[:Gijs (he/him)]

Comment on attachment 8963509 [details] [diff] [review]
bug_1449845_assert_about_page_has_csp.patch

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

We can't add a CSP to the system-principal'd pages, so there's no point checking for one. So I think this should use the nsIAboutModule to check if the about: page has URI_SAFE_FOR_UNTRUSTED_CONTENT, and only check for CSP on pages that have that flag.

For pages that don't, perhaps we want a separate list of those, and a separate error message if people add new ones. Perhaps we're OK with the fact that some pages are going to be painful to run as non-system-principal'd things anyway so we can just bail out for pages that don't get URI_SAFE_FOR_UNTRUSTED_CONTENT (but then, will that encourage people to use system principal even when they don't need to, which would be a shame...). I don't know. Either way, that would considerably shrink the existing giant list of listed documents.

Note that mobile has different about: pages than desktop, and Thunderbird/SeaMonkey might have their own, so I'm not sure how easy it'll be to maintain this list in dom/ anyway...

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #2)
> We can't add a CSP to the system-principal'd pages, so there's no point
> checking for one. So I think this should use the nsIAboutModule to check if
> the about: page has URI_SAFE_FOR_UNTRUSTED_CONTENT, and only check for CSP
> on pages that have that flag.

Yeah, I thought about that as well. I am fine with relying on URI_SAFE_FOR_UNTRUSTED_CONTENT for now and then expand that list once we got the CSP off the principal (see Bug 965637) which allows *all* about pages to ship with a CSP. Or we could just ship the entire list right away. To me it doens't make a big difference.
 
> Note that mobile has different about: pages than desktop, and
> Thunderbird/SeaMonkey might have their own, so I'm not sure how easy it'll
> be to maintain this list in dom/ anyway...

That is true. Probably we should not just use |#ifdef DEBUG| but can define our own which is only set for Desktop debug builds.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8963509 [details] [diff] [review]
bug_1449845_assert_about_page_has_csp.patch

Gijs had very valid concerns.

about:srcodc ?

In order to make this a tad more flexible, would it be horrible to have a
list of non-system-principal about pages without csp as a pref.
csp.non_system_principal_about_uris_without_csp = "about,addons,blank.."
the pref could be debug only.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] (only webcomponents and event handling reviews, please) from comment #4)
> In order to make this a tad more flexible, would it be horrible to have a
> list of non-system-principal about pages without csp as a pref.
> csp.non_system_principal_about_uris_without_csp = "about,addons,blank.."
> the pref could be debug only.

Yeah, I am fine with that. The other question is: is this the right place to add the assertion, or rather somewhere else?

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Hey Smaug, what is in your opinion the best way to enforce the assertion only for Desktop builds? Do you happen to know if there is some ifdef we could use?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

There is of course ifdef ANDROID or some such, but I wonder if there is also some ifdef FIREFOX.
I guess #developers would know better.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1449845_assert_about_page_has_csp.patch

I am loading the list using a pref now which only asserts on desktop debug builds. Thanks for your help in getting this assertion landed!

Comment 9

[:Gijs (he/him)]

The typical way you'd compile-time check whether this is a desktop Firefox build is by checking if MOZ_BUILD_APP == 'browser'.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8964560 [details] [diff] [review]
bug_1449845_assert_about_page_has_csp.patch

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

Tentative r=me on approach. Ideally I think about:blank and about:srcdoc shouldn't need to be in the list, but I don't know if there's some reasonable other way to deal with that, and I guess it doesn't hurt for now. I'm not sure they'll ever get a codebase principal with those URIs though... I think normally about:blank gets either a codebase principal for some non-about: URI, or a null principal. Less sure about about:srcdoc.

::: dom/base/nsDocument.cpp
@@ +5311,5 @@
>  
>    UnblockOnload(true);
>  }
>  
> +#if defined(DEBUG) && !defined(ANDROID)

Sorry, I raced with your patch, see my earlier comment.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #10)
> > +#if defined(DEBUG) && !defined(ANDROID)
> 
> Sorry, I raced with your patch, see my earlier comment.

I just asked on #developes; folks suggested:
#if defined(DEBUG) && !defined(ANDROID)

but I am fine either way.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8964560 [details] [diff] [review]
bug_1449845_assert_about_page_has_csp.patch

>+  // Potentially init the legacy whitelist of about URIs without a CSP.
>+  static StaticAutoPtr<nsTArray<nsCString> > sLegacyAboutPagesWithNoCSP;
extra space between > and >

>+  // Check if the about URI is whitelisted
>+  nsAutoCString aboutSpec;
>+  aDocumentURI->GetSpec(aboutSpec);
>+  for (auto & legacyPageEntry : *sLegacyAboutPagesWithNoCSP) {
extra space before &

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ac00d4125c
Add assertion that all new about: pages ship with a CSP. r=smaug,gijs

Comment 14

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/42ac00d4125c

Comment 15

[Matthew N. [:MattN]]

Note that this assertion doesn't actually stop shipping… it only stops debug builds from using these about: pages :) about:compat was added and fails this assertion but since nothing tests that page in debug builds (not sure if there are any tests at all), this wasn't caught.

Comment 16

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #15)

Note that this assertion doesn't actually stop shipping… it only stops debug builds from using these about: pages :) about:compat was added and fails this assertion but since nothing tests that page in debug builds (not sure if there are any tests at all), this wasn't caught.

Is there anything we can reasonably do about this? :-(

I guess we could add a debug-only test that loads all about: pages, or something?

Comment 17

[Matthew N. [:MattN]]

(In reply to :Gijs (he/him) from comment #16)

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #15)

Note that this assertion doesn't actually stop shipping… it only stops debug builds from using these about: pages :) about:compat was added and fails this assertion but since nothing tests that page in debug builds (not sure if there are any tests at all), this wasn't caught.

Is there anything we can reasonably do about this? :-(

I guess we could add a debug-only test that loads all about: pages, or something?

Right, that's what I was thinking. I was mostly pointing out that the commit didn't really achieve what the summary said.

Or don't allow new about: pages to be added without their own tests :) It's sad that there are no tests for about:compat from what I can tell.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

FWIW, I filed bug 1537685 to check if it's possible to write a test that iterates all about: pages and ensures all of them have a valid csp.