1770498 - Refactor fine-grained ShouldRFP checks

Status: RESOLVED FIXED

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

Description

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

While working on Bug 1737829 I had this nagging feeling questioning why the ShouldRFP(Principal, OriginAttributes) function needed both arguments - shouldn't a Principal contain OriginAttributes? Can't I just pass the principal?

Looking at the code using my new function - yes, the originattributes comes from the principal.

Now we have an existing ShouldRFP(Principal) function that is not fine-grained. My proposal is to delete that one while dropping the OA parameter from the new one, which in one step will cut over the uses of the old function to the new, fine-grained function.

---

Edit: This comment is out of date, see Comment 9

Comment 1

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Simplify Principal-based ShouldRFP check r?ckerschb

Comment 2

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

When trying to confirm that loadInfo->GetLoadingPrincipal()->GetOriginAttributes() == loadInfo->GetOriginAttributes() I hit a very complex nest of 'principals to inherit' and so forth that did not leave me 100% confident they were always equal...

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9bf6a5eb2ed5
Simplify Principal-based ShouldRFP check r=ckerschb,freddyb

Comment 4

[Cristina Cozmuta (:CrissCozmuta)]

Backed out for causing mochitest failures on browser_navigator.js. CLOSED TREE

Backout link : https://hg.mozilla.org/integration/autoland/rev/e4d76b11feef20a6ec98bcc3e96d11f17e46769e

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception%2Crunnable&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cmochitests%2Cwith%2Csoftware%2Cwebrender%2Cwith%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fdebug-mochitest-browser-chrome-swr-fis-e10s%2Cbc12&revision=9bf6a5eb2ed55b506b6239abdf7bee5c46537145&selectedTaskRun=D-ExiRopQXWJ1wvViRw2zQ.0

Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=380047112&repo=autoland&lineNumber=7997

Failure line :
TEST-UNEXPECTED-FAIL | browser/components/resistfingerprinting/test/browser/browser_navigator.js | Checking spoofed navigator.userAgent. - Got "Mozilla/5.0 (Windows NT 10.0; rv:103.0) Gecko/20100101 Firefox/103.0", expected "Mozilla/5.0 (X11; Linux x86_64; rv:103.0) Gecko/20100101 Firefox/103.0"

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tjr, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #5)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.

It was backed out - Tom is on it - clearing my ni?...

Comment 7

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

This bug is being expanded to a more comprehensive overhaul of how Resist Fingerprinting Exemptions should work. In particular before we took a very simplistic view to domain-based RFP exemptions - if the loading principal said an exempted domain, you were exempted.

This did not intelligently take into account iframes. In particular, if a non-exempted domain framed an exempted domain, the exempted domain's iframe would be exempted. It.... probably should not. This is debatable, but this is my attitude towards it, and I'll have Tor review it. In particular, my suggestions for behavior are:

Top-Level Document is on an Exempted Domain
- RFP is disabled.

Top-Level Document on an Exempted Domain embedding a non-exempted cross-origin iframe
- RFP in the iframe is enabled (NOT exempted).

Top-Level Document on an Exempted Domain embedding an exempted cross-origin iframe
- RFP in the iframe is disabled (exempted).

Top-Level Document on a Non-Exempted Domain4
- RFP is enabled (NOT exempted).

Top-Level Document on a Non-Exempted Domain embeds an exempted cross-origin iframe
- RFP in the iframe is enabled (NOT exempted).

Exempted Document (top-level or iframe) contacts any cross-origin domain (exempted or non-exempted)
- RFP is disabled (exempted) for the request

Non-Exempted Document (top-level or iframe) contacts any cross-origin domain (exempted or non-exempted)
- RFP is enabled (NOT exempted) for the request

This patchset will refactor the ShouldRFP tests to behave this way. Bug 1737829 (which I intend to land concurrently) will create a test for the behavior explicitly.

Comment 8

[richard (Tor Project)]

Will exemptions be ephemeral if set while in private browsing mode?

Comment 9

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Right now the mechanism for how exemptions are set is "Put them in this preference value". I had expected that eventually we would switch the mechanism over to a site permission (which would then respect PBM) but I haven't done any of that plumbing.

On irc Richard raised this point:

23:24:21 <richard> out of curiosity, what happens in the case of iframes within iframes within iframes etc
23:35:15 <tjr> If the top-level document is not-exempted then nothing below will be exempted.
23:35:45 <tjr> But the way it's written now, if we have exempted -> not exempted -> exempted then the innermost would be exempted I believe.
23:36:26 <tjr> This would be a problem if the innermost was communicating with its embedder and giving it data that wound up to be fingerprintable.
23:37:00 <tjr> I'm not sure if this would be a problem in practice, but conceptually this should be fixed.  I'm just not sure _how_....

Comment 10

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Correct a call to ShouldRFP r?smaug

Instead of trying to get the ScriptObjectPrincipal
of a window; use the window's Document, which will
take into account a lot more context.

Comment 11

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Abstract the Document check so it can be used in more places r?ckerschb

In rare cases, we will need to see if we ShouldRFP using
only a URI and OA. (This is already the case for a
top-level load.)

Abstract this check into its own function, but at the
same time explicitly label it as dangerous and require
the developer to provide a justification why they need
to use it to hopefully prevent misuse.

Depends on D146945

Comment 12

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Use the final channel URI for SubDocument types also r?ckerschb

Testing indicated that for a SubDocument, the loading principal
is that of the parent document, but the final channel uri is
the uri of the subdocument. In this situation, use the final
channel uri just as we do for a Document.

Depends on D150585

Comment 13

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Add RFP to CookieJarSettings r?timhuang

Before we took a very simplistic view to domain-based RFP
exemptions - if the loading principal said an exempted
domain, you were exempted.

This did not intelligently take into account iframes. In
particular, if a non-exempted domain framed an exempted
domain, the exempted domain's iframe would be exempted.

It probably should not. This is debatable, but at this
point in time after consultation with Tor we are going
to take the approach described in the comments in
CookieJarSettings.h

Notably this is creating a gap when it comes to nested
iframes. If an exempted domain iframes a non-exempted
domain, which in turn iframes an exempted domain - the
innermost iframe will be exempted even though its parent
is not. This is not ideal, once a document is non-
exempted, all children should be non-exempted. But this
is especially difficult to implement with the facilities
easily available to us, so we leave this corner case for
another day.

Depends on D150586

Comment 14

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Populate the RFP member of CookieJar Settings r?timhuang

CookieJarSettings frequently gets populated in a place
where we have ready access to the Document/Channel it
is being constructed for. This lets us populate the boolean
and pass it into CookieJarSetting's constructor easily.

When it is created for LoadInfo, we need to plumb the URI
through by adding it to LoadInfo::CreateForDocument.

Depends on D150587

Comment 15

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Use the CookieJarSetting's ShouldRFP Member in nsContentUtils::ShouldRFP r?timhuang

Now that the ShouldRFP member is correctly populated (we hope)
in CookieJarSettings, we can use it in our ShouldRFP methods.

There are two general scenarios:
An exempted domain frames a non-exempted domain.
The inner frame should be non-exempted.
A non-exempted domain frames an exempted domain
The inner frame should be non-exempted.

In the first scenario, CookieJarSettings.shouldRFP member
will be false (exempted.) We clearly cannot blindly rely
on the member, it would be wrong here.

In the second scenario, CookieJarSettings.shouldRFP member
will be true (non-exempted). This is the scenario we really
need CookieJarSettings for.

---

We take the opportunity to add an additional check for the
'exempted domain framed by a non-exempted domain' scenario.
This additional check takes advantage of the first party
isolation key (for FPI) or partition key (for dFPI).

Depends on D150588

Comment 16

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Add shouldResistFingerprinting to RemoteWorkerData r?asuth

Previously, we initialized WorkerLoadInfo's shouldRFP member
using the Worker's principal at time of construction. It is
better to populate it from the RemoteWorkerData structure.

The RemoteWorkerData's shouldRFP member can be initialized
in a more TKTKTKTK TODO UPDATE THIS

Depends on D150589

Comment 17

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Create ShouldRFP(nsILoadInfo) and make ShouldRFP(nsIPrincipal) explicitly dangerous r?timhuang

• Move ShouldRFP(char*), ShouldRFP(docshell), ShouldRFP(Document)
  below some utility code.

• Now that we know we should check the CookieJarSettings, using
  ShouldRFP(nsIPrincipal) is dangerous. We mark it as dangerous
  and annotate the existing uses of it.

• At the same time, an nsILoadInfo has the CookieJarSettings we
  want to check, so create a ShouldRFP(nsILoadInfo) that checks
  it and cascades to the (marked-dangerous-but-not-dangerous-for-
  this-call) principal function.

• We also correct a situation where WorkerLoadInfo does not
  initialize the shouldRFP member.

Depends on D150590

Comment 18

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Update the browser_canvas_rfp_exclusion.js test r?timhuang

Previously, this test would open a page and then enable/disable
RFP while the tab was open, writing and reading the canvas as
it went.

Now that CookieJarSettings gets initialized when a document loads
and stays constant for the lifetime of the document, we cannot enable
and disable the prefs and expect them to continue to behave correctly.

We do a 'pre-test' to populate a canvas and then get the true image
data for it. Then we set the preferences prior to loading future
documents and compare the extracted canvas data to the known-correct
data to see if we are correctly spoofing it or not.

Depends on D150591

Comment 19

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: Correct a call to ShouldRFP r?smaug

Instead of trying to get the ScriptObjectPrincipal
of a window; use the window's Document, which will
take into account a lot more context.

Comment 20

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: Bug 1770498: De-duplicate the CookieJar check into a local function r?timhuang

Depends on D150591

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2159bdd42b61
Correct a call to ShouldRFP r=smaug
https://hg.mozilla.org/integration/autoland/rev/5eb081a32599
Simplify Principal-based ShouldRFP check r=ckerschb,freddyb
https://hg.mozilla.org/integration/autoland/rev/9f28ff2d9806
Abstract the Document check so it can be used in more places r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/8f6e60b4c221
Use the final channel URI for SubDocument types also r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/ecac59080420
Add RFP to CookieJarSettings r=timhuang
https://hg.mozilla.org/integration/autoland/rev/844060e1fd8d
Populate the RFP member of CookieJar Settings r=timhuang,geckoview-reviewers,owlish
https://hg.mozilla.org/integration/autoland/rev/47b0626647f4
Use the CookieJarSetting's ShouldRFP Member in nsContentUtils::ShouldRFP r=timhuang
https://hg.mozilla.org/integration/autoland/rev/c0c4a265c1b4
Add shouldResistFingerprinting to RemoteWorkerData r=asuth
https://hg.mozilla.org/integration/autoland/rev/2a94f16a8b47
Create ShouldRFP(nsILoadInfo) and make ShouldRFP(nsIPrincipal) explicitly dangerous r=timhuang
https://hg.mozilla.org/integration/autoland/rev/f3ce63ace6ef
De-duplicate the CookieJar check into a local function r=timhuang
https://hg.mozilla.org/integration/autoland/rev/20d198e82c50
Update the browser_canvas_rfp_exclusion.js test r=timhuang

Comment 22

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/2159bdd42b61
https://hg.mozilla.org/mozilla-central/rev/5eb081a32599
https://hg.mozilla.org/mozilla-central/rev/9f28ff2d9806
https://hg.mozilla.org/mozilla-central/rev/8f6e60b4c221
https://hg.mozilla.org/mozilla-central/rev/ecac59080420
https://hg.mozilla.org/mozilla-central/rev/844060e1fd8d
https://hg.mozilla.org/mozilla-central/rev/47b0626647f4
https://hg.mozilla.org/mozilla-central/rev/c0c4a265c1b4
https://hg.mozilla.org/mozilla-central/rev/2a94f16a8b47
https://hg.mozilla.org/mozilla-central/rev/f3ce63ace6ef
https://hg.mozilla.org/mozilla-central/rev/20d198e82c50