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

REOPENED
Assigned to

Status

()

defect
P2
normal
REOPENED
2 years ago
9 days ago

People

(Reporter: arthur, Assigned: xeonchen)

Tracking

(Blocks 4 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor 21569][domsecurity-backlog2])

Attachments

(23 attachments, 9 obsolete attachments)

8.50 KB, patch
Details | Diff | Splinter Review
18.68 KB, patch
Details | Diff | Splinter Review
4.05 KB, patch
Details | Diff | Splinter Review
6.91 KB, patch
Details | Diff | Splinter Review
4.55 KB, patch
Details | Diff | Splinter Review
1.53 KB, patch
Details | Diff | Splinter Review
11.58 KB, patch
Details | Diff | Splinter Review
9.51 KB, patch
Details | Diff | Splinter Review
18.49 KB, patch
Details | Diff | Splinter Review
3.33 KB, patch
Details | Diff | Splinter Review
11.31 KB, patch
Details | Diff | Splinter Review
4.47 KB, patch
Details | Diff | Splinter Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
Reporter

Description

2 years ago
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)?
Priority: -- → P2
Whiteboard: [tor 20317] → [tor 20317][domsecurity-backlog]
Priority: P2 → P3
Whiteboard: [tor 20317][domsecurity-backlog] → [tor 20317][domsecurity-backlog2]
Reporter

Updated

2 years ago
Whiteboard: [tor 20317][domsecurity-backlog2] → [tor 21569][domsecurity-backlog2]
Reporter

Comment 1

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

Updated

2 years ago
See Also: → 1422056
Reporter

Updated

a year ago
Priority: P3 → P1
Reporter

Comment 5

a year ago
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.)
Flags: needinfo?(jhofmann)
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.
Attachment #8981570 - Flags: feedback+
Blocks: 1402957
Flags: needinfo?(jhofmann)

Comment 7

11 months ago
Status on this?
Flags: needinfo?(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?
Flags: needinfo?(ckerschb) → needinfo?(arthuredelstein)
Reporter

Comment 9

11 months ago
Hi all -- no updates at the moment, but I hope to work on this in the next couple of weeks.
Flags: needinfo?(arthuredelstein)
Assignee: nobody → arthuredelstein
Status: NEW → ASSIGNED
Priority: P1 → P2
Blocks: 1494327
Reporter

Comment 10

6 months ago
Attachment #8981569 - Attachment is obsolete: true
Attachment #8981570 - Attachment is obsolete: true
Comment hidden (obsolete)
Assignee: arthur → xeonchen
Blocks: 1422056
See Also: 1422056
Attachment #9044147 - Attachment is obsolete: true

Comment 43

16 days ago
Pushed by xeonchen@gmail.com:
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

whooo fingers crossed

Good job. Well done!

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

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

Status: RESOLVED → REOPENED
Flags: needinfo?(xeonchen)
Resolution: FIXED → ---
Target Milestone: mozilla68 → ---

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

Flags: needinfo?(xeonchen)
You need to log in before you can comment on or make changes to this bug.