Closed Bug 1449845 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 1 obsolete file)

bug_1449845_assert_about_page_has_csp.patch
(deleted), patch
smaug
: review+
Gijs
: review+
Details | Diff | Splinter Review
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.
Blocks: 1430748
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P1
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
Attached patch bug_1449845_assert_about_page_has_csp.patch (obsolete) (deleted) — — Splinter Review
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).
Attachment #8963509 - Flags: review?(gijskruitbosch+bugs)
Attachment #8963509 - Flags: review?(bugs)
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...
Attachment #8963509 - Flags: review?(gijskruitbosch+bugs)
(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 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.
Attachment #8963509 - Flags: review?(bugs)
(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?
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?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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!
Attachment #8963509 - Attachment is obsolete: true
Attachment #8964560 - Flags: review?(gijskruitbosch+bugs)
Attachment #8964560 - Flags: review?(bugs)
The typical way you'd compile-time check whether this is a desktop Firefox build is by checking if MOZ_BUILD_APP == 'browser'.
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.
Attachment #8964560 - Flags: review?(gijskruitbosch+bugs) → review+
(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 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 &
Attachment #8964560 - Flags: review?(bugs) → review+
Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/42ac00d4125c Add assertion that all new about: pages ship with a CSP. r=smaug,gijs
https://hg.mozilla.org/mozilla-central/rev/42ac00d4125c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

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.

(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?

Flags: needinfo?(MattN+bmo)

(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.

Flags: needinfo?(MattN+bmo)
Depends on: 1537685

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.

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

Attachment

General

Creator:
Created:
Updated:
Size: