1385306 - Make Activity Stream unprivileged: ensure about:newtab document runs with null principal

Status: RESOLVED WONTFIX

(Firefox :: New Tab Page, enhancement, P1)

Description

[Ursula Sarracini (:ursula)]

After giving Activity Stream URI_SAFE_FOR_UNSTRUSTED_CONTENT in Bug 1021667, to make Activity Stream fully unprivileged we need to set the principal to be null.

Comment 1

[Ursula Sarracini (:ursula)]

Christoph, perhaps you could shed some light on the best/easiest way to do this?

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ursula Sarracini (:ursula) from comment #1)
> Christoph, perhaps you could shed some light on the best/easiest way to do
> this?

I can help with that. First question: do you want to set it to null or to a NullPrincipal? Where do you want that principal? And what principal - The load/triggering/principalToInherit in the loadInfo?

Comment 3

[:Gijs (he/him)]

What's the current state of this bug? Do we still intend to do this "soon"?

Comment 4

[Ursula Sarracini (:ursula)]

It got de-prioritized recently because we were focusing on some critical things for 57, but yes we still plan on doing this. I think we may actually need this in order for bug 1184701 to work, and the work for bug 1184701 was intended to start soon, so we should start the work on this soon as well.

Comment 5

[Ursula Sarracini (:ursula)]

Attachment: Bug 1385306 - Make Activity Stream unprivileged: ensure about:newtab document runs with null principal

Review commit: https://reviewboard.mozilla.org/r/219896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219896/

Comment 6

[:Gijs (he/him)]

Comment on attachment 8950621 [details]
Bug 1385306 - Make Activity Stream unprivileged: ensure about:newtab document runs with null principal

https://reviewboard.mozilla.org/r/219896/#review225736

Great, r=me.

It might be worth having a followup bug and discussing with Christoph Kerschbaumer if we can add a CSP to about:newtab/about:home-with-activity-stream and use them to constrain attack vectors further.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #6)
> It might be worth having a followup bug and discussing with Christoph
> Kerschbaumer if we can add a CSP to
> about:newtab/about:home-with-activity-stream and use them to constrain
> attack vectors further.

Yes, that would be great. Happy to help find the right CSP. You can take a look at Bug 1436808 where we started to add a CSP to content privileged about pages.

Comment 8

[Ursula Sarracini (:ursula)]

So after pushing the original patch to try, there were some failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=757ea71cf9f7b4114261ec0fd8a09303b4bca5fc&group_state=expanded, mainly about not hiding the url in the url bar when the principal was null. 
I thought that was bizarre but then I saw this: https://dxr.mozilla.org/mozilla-central/rev/6d8f470b2579e7570f14e3db557264dc075dd654/browser/base/content/browser.js#6965 which we use to help determine (based on the browser's content principal) if we should set the urlbar value to "" later one: https://dxr.mozilla.org/mozilla-central/rev/6d8f470b2579e7570f14e3db557264dc075dd654/browser/base/content/browser.js#2712. 
There's already some special casing going on for about:blank having a null principal, so adding some special casing for about:newtab/about:home there too did the trick. 
I checked and there aren't any about: pages that need url bar hiding and *also* have null principal aside from these 3 so I'm thinking special case is fine. Gijs, what do you think?

Comment 9

[:Gijs (he/him)]

(In reply to Ursula Sarracini (:ursula) from comment #8)
> I checked and there aren't any about: pages that need url bar hiding and
> *also* have null principal aside from these 3 so I'm thinking special case
> is fine. Gijs, what do you think?

Yep, just adding a special case for about:newtab having the null principal here is fine. Happy to review a patch to do that in more detail if that's helpful.

Comment 10

[Ursula Sarracini (:ursula)]

After lots of back and forth with Gijs and ckerschb, we decided this will be a WONTFIX. As it turns out we can't access indexedDB on documents with a null principal, and as a result snippets won't work on newtab/home if we drop their principals to null. So we're going to leave their principals to codebase but we should really get review on our CSP and figure out if it'll break snippets/onboarding and sort that out so we can at least add the CSP (right now it's on report-only).