1330467 - When "privacy.firstparty.isolate" is true, double-key permissions to origin + firstPartyDomain

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Arthur Edelstein [:arthur]]

Permissions settings can be a tracking vector. When we are isolating by first party, we should double-key permissions to the origin and the first-party (and possibly other origin attribute fields)?

Comment 1

[Arthur Edelstein [:arthur]]

Here is Tor Browser's current patch:
https://torpat.ch/21569

Comment 2

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-19273-Avoid-JavaScript-patching-of-the-external-.patch-WIP

Comment 3

[Arthur Edelstein [:arthur]]

Attachment: 0002-Bug-1330467-Isolate-permissions-by-first-party-domai.patch

Comment 4

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1330467-Change-SitePermissions-to-use-principal.patch

Comment 5

[Arthur Edelstein [:arthur]]

I've added a couple of WIP patches. Johann, I wonder if you might have time to have a look and let me know if this approach would be acceptable? (I'm still working on fixing some unit tests.)

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8981570 [details] [diff] [review]
0001-Bug-1330467-Change-SitePermissions-to-use-principal.patch

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

Hi Arthur, this looks like a great approach to me! Switching nsIPermissionManager to use only principals is in fact already tracked in bug 1402957 and since the permission manager stores (stripped) origins anyway I don't think we're in danger of losing anything by not using the URI interfaces.

There's one thing I wonder. We could have SitePermissions.jsm functions accept EITHER an nsIPrincipal or an nsIURI (which would be converted to a principal then), right? That would provide us with a few advantages:

- Your patch would touch way less code, you could just leave all the e.g. test usage with nsIURI in place
- We don't have to fear accidentally missing to fix up any consumer of SitePermissions.jsm
- It's a little more ergonomic for front-end developers

I guess creating the principal on the fly is a little expensive, so we might want to document that the nsIURI part is mostly intended to be used in tests. I think that in most real world code it should be possible to get the content principal without taking it from the URI.

I'm happy to do a more thorough review of this whenever you think it's ready :)

Thanks!

::: browser/components/preferences/sitePermissions.js
@@ +314,4 @@
>  
>      for (let p of this._permissionsToChange.values()) {
>        let uri = Services.io.newURI(p.origin);
> +      let principal = Services.scriptSecurityManager.createCodebasePrincipal(uri, {});

Hm, is this stripping OA on purpose? Otherwise this could be simplified to Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(p.origin).

::: browser/modules/webrtcUI.jsm
@@ +364,4 @@
>      uri = Services.io.newURI(aRequest.documentURI);
>    }
>  
> +  let principal = Services.scriptSecurityManager.createCodebasePrincipalFromOrigin(uri);

I don't think that works with an nsIURI, you likely want to pass in aRequest.origin. But it's even easier than that, you can just use browser.contentPrincipal here.

Comment 7

[Marion Daly [:mdaly]]

Status on this?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Marion Daly [:mdaly] from comment #7)
> Status on this?

302 to Arthur who is working on this bug. Arthur, are there any updates here?

Comment 9

[Arthur Edelstein [:arthur]]

Hi all -- no updates at the moment, but I hope to work on this in the next couple of weeks.

Comment 10

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1330467-part-1.-Don-t-strip-first-party-domain-f.patch

Comment 11

[Arthur Edelstein [:arthur]]

Attachment: 0002-Bug-1330467-part-2.-Add-SitePermissions-APIs-functio.patch

Comment 12

[Arthur Edelstein [:arthur]]

Attachment: 0003-Bug-1330467-part-3.-Use-principal-for-permissions-in.patch

Comment 13

[Arthur Edelstein [:arthur]]

Attachment: 0004-Bug-1330467-part-4.-Isolate-Temporary-and-Global-per.patch

Comment 14

[Arthur Edelstein [:arthur]]

Attachment: 0005-Bug-1330467-part-5.-Access-permissions-by-principal-.patch

Comment 15

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-1330467-part-1.-Don-t-strip-first-party-domain-f.patch

Comment 16

[Arthur Edelstein [:arthur]]

Attachment: 0002-Bug-1330467-part-2.-Add-SitePermissions-APIs-functio.patch

Comment 17

[Arthur Edelstein [:arthur]]

Attachment: 0003-Bug-1330467-part-3.-Use-principal-for-permissions-in.patch

Comment 18

[Arthur Edelstein [:arthur]]

Attachment: 0004-Bug-1330467-part-4.-Isolate-Temporary-and-Global-per.patch

Comment 19

[Arthur Edelstein [:arthur]]

Attachment: 0005-Bug-1330467-part-5.-Access-permissions-by-principal-.patch

Comment 20

[Arthur Edelstein [:arthur]]

Attachment: 0006-Bug-1330467-part-6.-Use-principal-for-permissions-in.patch

Comment 21

[Arthur Edelstein [:arthur]]

Attachment: 0007-Bug-1330467-part-7.-Confirm-FPI-in-permission-manage.patch

Comment 22

[Arthur Edelstein [:arthur]]

Attachment: 0008-Bug-1330467-part-8.-Make-canvas-permission-respect-F.patch

Comment 23

[Arthur Edelstein [:arthur]]

Attachment: 0009-Bug-1330467-part-9.-Use-principal-with-mobile-permis.patch

Comment 24

[Arthur Edelstein [:arthur]]

Attachment: 0010-Bug-1330467-part-10.-Use-principal-for-permissions-i.patch

Comment 25

[Arthur Edelstein [:arthur]]

Attachment: 0011-Bug-1330467-part-11.-Use-principal-for-permissions-i.patch

Comment 26

[Arthur Edelstein [:arthur]]

Attachment: 0012-Bug-1330467-part-12.-FPI-isolation-of-translation-pe.patch

Comment 27

[Arthur Edelstein [:arthur]]

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69f9fb7cba27&selectedJob=212120566

Comment 28

[Gian-Carlo Pascutto [:gcp]]

edit: nvm, misinterpreted description.

Comment 29

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 1. Don't strip first party domain from permissions key;

Comment 30

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 2. Add SitePermissions APIs functions to accept principal;

Comment 31

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 3. Use principal for permissions in pageinfo/permissions.js;

Comment 32

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 4. Isolate Temporary and Global permissions by firstPartyDomain;

Comment 33

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 5. Access permissions by principal in PermissionUI.jsm;

Comment 34

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 6. Use principal for permissions in sitePermissions.js;

Comment 35

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 7. Confirm FPI in permission manager tests;

Comment 36

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 8. Make canvas permission respect FPI;

Comment 37

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 9. Use principal with mobile permissions;

Comment 38

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 10. Use principal for permissions in browser site-identity;

Comment 39

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 11. Use principal for permissions in webrtc UI;

Comment 40

[Gary Chen [:xeonchen]]

Attachment: Bug 1330467 - part 12. FPI isolation of translation permissions;

Comment 41

[Gary Chen [:xeonchen]]

rebased try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fbcb1b9495cca4ab4f56b15fb4d5b1f7e2f7b33

Comment 42

[Gary Chen [:xeonchen]]

green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2b6193c2f228ceacd804df8bfda1981d9b545085

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d48bdbb4035
part 1. Don't strip first party domain from permissions key; r=johannh,Ehsan
https://hg.mozilla.org/integration/autoland/rev/0ae215d91758
part 2. Add SitePermissions APIs functions to accept principal; r=johannh
https://hg.mozilla.org/integration/autoland/rev/f939c61e051f
part 3. Use principal for permissions in pageinfo/permissions.js; r=johannh
https://hg.mozilla.org/integration/autoland/rev/6ac44130d2bb
part 5. Access permissions by principal in PermissionUI.jsm; r=johannh
https://hg.mozilla.org/integration/autoland/rev/35d96a4ff659
part 6. Use principal for permissions in sitePermissions.js; r=johannh
https://hg.mozilla.org/integration/autoland/rev/b43fa07d5756
part 7. Confirm FPI in permission manager tests; r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/4686eebd8962
part 8. Make canvas permission respect FPI; r=johannh,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/2648f5bb1804
part 9. Use principal with mobile permissions; r=johannh,snorp
https://hg.mozilla.org/integration/autoland/rev/2cd09bae2bdf
part 10. Use principal for permissions in browser site-identity; r=johannh
https://hg.mozilla.org/integration/autoland/rev/2f2308fe5747
part 11. Use principal for permissions in webrtc UI; r=johannh
https://hg.mozilla.org/integration/autoland/rev/0229d5353d50
part 12. FPI isolation of translation permissions; r=johannh

Comment 44

[Johann Hofmann [:johannh]]

whooo fingers crossed

Comment 45

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/1d48bdbb4035
https://hg.mozilla.org/mozilla-central/rev/0ae215d91758
https://hg.mozilla.org/mozilla-central/rev/f939c61e051f
https://hg.mozilla.org/mozilla-central/rev/6ac44130d2bb
https://hg.mozilla.org/mozilla-central/rev/35d96a4ff659
https://hg.mozilla.org/mozilla-central/rev/b43fa07d5756
https://hg.mozilla.org/mozilla-central/rev/4686eebd8962
https://hg.mozilla.org/mozilla-central/rev/2648f5bb1804
https://hg.mozilla.org/mozilla-central/rev/2cd09bae2bdf
https://hg.mozilla.org/mozilla-central/rev/2f2308fe5747
https://hg.mozilla.org/mozilla-central/rev/0229d5353d50

Comment 46

[Ethan Tseng [:ethan]]

Good job. Well done!

Comment 47

[Alexandru Michis [:malexandru]]

Backed out 11 changesets (Bug 1330467) as requested by xeonchen on IRC.

Backout: https://hg.mozilla.org/integration/autoland/rev/3866561a7bae97a6c9ec3c18174747ec36bba090

Comment 48

[Gary Chen [:xeonchen]]

(In reply to Mihai Alexandru Michis [:malexandru] from comment #47)

Backed out 11 changesets (Bug 1330467) as requested by xeonchen on IRC.

Backout: https://hg.mozilla.org/integration/autoland/rev/3866561a7bae97a6c9ec3c18174747ec36bba090

Thanks!

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38e1751e35df
part 1. Don't strip first party domain from permissions key; r=johannh,Ehsan
https://hg.mozilla.org/integration/autoland/rev/0ba364f5941f
part 2. Add SitePermissions APIs functions to accept principal; r=johannh
https://hg.mozilla.org/integration/autoland/rev/51c96b44ff30
part 3. Use principal for permissions in pageinfo/permissions.js; r=johannh
https://hg.mozilla.org/integration/autoland/rev/ebf3f2f197f6
part 5. Access permissions by principal in PermissionUI.jsm; r=johannh
https://hg.mozilla.org/integration/autoland/rev/1bb35c7f8236
part 6. Use principal for permissions in sitePermissions.js; r=johannh
https://hg.mozilla.org/integration/autoland/rev/974de551ec76
part 7. Confirm FPI in permission manager tests; r=Ehsan
https://hg.mozilla.org/integration/autoland/rev/66b30aa15bcd
part 8. Make canvas permission respect FPI; r=johannh,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/51414d51854c
part 9. Use principal with mobile permissions; r=johannh,snorp
https://hg.mozilla.org/integration/autoland/rev/0f0888b33389
part 10. Use principal for permissions in browser site-identity; r=johannh
https://hg.mozilla.org/integration/autoland/rev/299bbef55ad6
part 11. Use principal for permissions in webrtc UI; r=johannh
https://hg.mozilla.org/integration/autoland/rev/446a73750a48
part 12. FPI isolation of translation permissions; r=johannh

Comment 50

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/38e1751e35df
https://hg.mozilla.org/mozilla-central/rev/0ba364f5941f
https://hg.mozilla.org/mozilla-central/rev/51c96b44ff30
https://hg.mozilla.org/mozilla-central/rev/ebf3f2f197f6
https://hg.mozilla.org/mozilla-central/rev/1bb35c7f8236
https://hg.mozilla.org/mozilla-central/rev/974de551ec76
https://hg.mozilla.org/mozilla-central/rev/66b30aa15bcd
https://hg.mozilla.org/mozilla-central/rev/51414d51854c
https://hg.mozilla.org/mozilla-central/rev/0f0888b33389
https://hg.mozilla.org/mozilla-central/rev/299bbef55ad6
https://hg.mozilla.org/mozilla-central/rev/446a73750a48

Comment 51

[Tom Ritter [:tjr]]

[Tracking Requested - why for this release]: After this has spun in nightly for a bit; we would ideally uplift it to beta since this is for Tor and we want it to be in the ESR if at all possible.

Comment 52

[Julien Cristau [:jcristau]]

I doubt this would be suitable for beta uplift...

Comment 53

[vtol]

Regression bug 1554805