1382533 - When resisting fingerprinting, don't expose local IP Addresses via mDNS

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Arthur Edelstein [:arthur]]

Tor Browser has a patch being privacy.resistFingerprinting that stops information about a user's local network from being exposed by the Presentation API. Here it is:
https://torpat.ch/22165

We would like to propose uplifting this patch.

The original ticket is at https://trac.torproject.org/22165

Comment 1

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

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

Comment 2

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162898/diff/1-2/

Comment 3

[Chung-Sheng Fu [:cfu]]

This patch simply avoids all implementations of mozilla::net::DoListAddresses from reporting information about local network when privacy.resistFingerprinting = true.

Comment 4

[u408661]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

https://reviewboard.mozilla.org/r/162898/#review168456

So... I'm not sure how I actually feel about this solution. I see a couple potential issues:

(1) I can easily see (at some point in the future) wanting to use these within necko, regardless of the resist fingerprinting setting. Having this code where it is would require reworking this at some point down the line.
(2) This doesn't do anything on android. If you look at PresentationControllingInfo::GetAddress(), nsINetworkInfoService isn't used there at all, the native android method of getting addresses is used. So, if you want to have this pref be useful on android (and I don't know why you wouldn't), then this is totally the wrong place to stop the addresses from being used for fingerprinting.

Now, all that said, the code is (in isolation) fine. So, if you can convince me that my two concerns are invalid, I'll happily r+ the patch :)

Comment 5

[Chung-Sheng Fu [:cfu]]

Hi Arthur,
Should we move the fingerprinting resisting code to PresentationControllingInfo::GetAddress, in order not to affect the network library?
Besides, since PresentationControllingInfo::GetAddress is the top level entry point of collecting local network information, I think we can also cover the android part.

Comment 6

[Tom Ritter [:tjr]]

(In reply to Chung-Sheng Fu [:cfu] from comment #5)
> Hi Arthur,
> Should we move the fingerprinting resisting code to
> PresentationControllingInfo::GetAddress, in order not to affect the network
> library?
> Besides, since PresentationControllingInfo::GetAddress is the top level
> entry point of collecting local network information, I think we can also
> cover the android part.

We definitely want to cover Android, so moving the patch so it does so is necessary. If that also solves the Necko problem, that sounds great too.

Comment 7

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #5)
> Hi Arthur,
> Should we move the fingerprinting resisting code to
> PresentationControllingInfo::GetAddress, in order not to affect the network
> library?
> Besides, since PresentationControllingInfo::GetAddress is the top level
> entry point of collecting local network information, I think we can also
> cover the android part.

Hi Chung-Sheng, that seems like a good start to me. But I worry that the number of Presentation devices or other characteristics of those devices may be exposed by other methods of the Presentation API.

I can think of a few possible solutions:
  1. Disable the Presentation API altogether when privacy.resistFingerprinting = true (same effect as dom.presentation.enabled = false).
  2. Ensure that no information is divulged until after permission is granted by the user.
  3. Spoof a single presentation object as always present to cover the most common case.

My feeling is (1) is simplest and would be at least a good stopgap until we can implement (2) or (3).

Comment 8

[Arthur Edelstein [:arthur]]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

https://reviewboard.mozilla.org/r/162898/#review171360

Comment 9

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162898/diff/2-3/

Comment 10

[Chung-Sheng Fu [:cfu]]

This patch disables Presentation API when privacy.resistFingerprinting = true.

I modify the interfaces according to this document https://www.w3.org/TR/presentation-api/#api

General rules are:

- methods and attribute getters returning promises:
  reject the promise with NS_ERROR_DOM_SECURITY_ERR.

- attribute getters:
  return spoofed values.

- attribute setters:
  do nothing.

- methods:
  do nothing.

- event handlers:
  let them update internal states, but do not fire DOM events.
  if the event is associated with any promise, do not resolve it.

I'm still studying how to write tests for Presentation API, but I think we can review this approach first.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

https://reviewboard.mozilla.org/r/162898/#review172750

Perhaps we could do something simpler, but this should be fine too.

::: dom/presentation/PresentationConnection.cpp:161
(Diff revision 3)
>  
>  void
>  PresentationConnection::GetId(nsAString& aId) const
>  {
> +  if (nsContentUtils::ShouldResistFingerprinting()) {
> +    aId = NS_LITERAL_STRING("");

EmptyString()

::: dom/presentation/PresentationConnection.cpp:172
(Diff revision 3)
>  
>  void
>  PresentationConnection::GetUrl(nsAString& aUrl) const
>  {
> +  if (nsContentUtils::ShouldResistFingerprinting()) {
> +    aUrl = NS_LITERAL_STRING("");

EmptyString()

Comment 12

[Arthur Edelstein [:arthur]]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

https://reviewboard.mozilla.org/r/162898/#review174102

I think this approach looks reasonable. But I would be inclined to wait for a unit test patch before checking it in, unless we are too short of time.

Comment 13

[Arthur Edelstein [:arthur]]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

https://reviewboard.mozilla.org/r/162898/#review174106

Comment 14

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1382533 - Add tests for Presentation API fingerprinting resistance.

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

Comment 15

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162898/diff/3-4/

Comment 16

[Chung-Sheng Fu [:cfu]]

This tests if Presentation API is disabled when privacy.resistFingerprinting = true.

Some interfaces are not instantiatable.  Is it better to leave them commented?  Or just remove them.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8898709 [details]
Bug 1382533 - Add tests for Presentation API fingerprinting resistance.

https://reviewboard.mozilla.org/r/170110/#review175238

I think leaving the interfaces commented out is fine.

::: dom/presentation/tests/mochitest/test_presentation_fingerprinting_resistance.html:59
(Diff revision 1)
> +
> +  // let connectionList = new PresentationConnectionList();
> +  // connectionList.onconnectionavailable = testHandler("PresentationConnectionList.onconnectionavailable");
> +  // SimpleTest.is(connectionList.connections.length, 0, "PresentationConnectionList.connections.length");
> +
> +  // Wait for events to be fired.

Ah, I see, need to wait that events _don't_ fire. Want to clarify the comment.

Comment 18

[Arthur Edelstein [:arthur]]

Comment on attachment 8898709 [details]
Bug 1382533 - Add tests for Presentation API fingerprinting resistance.

https://reviewboard.mozilla.org/r/170110/#review177124

::: dom/presentation/tests/mochitest/test_presentation_fingerprinting_resistance.html:36
(Diff revision 1)
> +  request.onconnectionavailable = testHandler("PresentationRequest.onconnectionavailable");
> +  await testPromise(request.start(), "PresentationRequest.start");
> +  await testPromise(request.reconnect("foo"), "PresentationRequest.reconnect");
> +  await testPromise(request.getAvailability(), "Presentation.getAvailability");
> +
> +  // These interfaces are not directly instantiatable.

Is it possible to generate these interfaces as they would be used by a content script? Or are content scripts fully blocked by using these interfaces when privacy.resistFingerprinting = true?

::: dom/presentation/tests/mochitest/test_presentation_fingerprinting_resistance.html:60
(Diff revision 1)
> +  // let connectionList = new PresentationConnectionList();
> +  // connectionList.onconnectionavailable = testHandler("PresentationConnectionList.onconnectionavailable");
> +  // SimpleTest.is(connectionList.connections.length, 0, "PresentationConnectionList.connections.length");
> +
> +  // Wait for events to be fired.
> +  setTimeout(() => SimpleTest.finish(), 3000);

I wonder if you need to use a timeout like this? On other APIs, I believe any triggered event will be added to the end of the JS execution queue. So on previous mochitests I have used:

exerciseAPI();
triggerDummyEvent();
let result = await Promise.race([apiEvent, dummyEvent]);

Comment 19

[Chung-Sheng Fu [:cfu]]

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #18)
> Comment on attachment 8898709 [details]
> Bug 1382533 - Add test for Presentation API fingerprinting resistance
> 
> https://reviewboard.mozilla.org/r/170110/#review177124
> 
> :::
> dom/presentation/tests/mochitest/test_presentation_fingerprinting_resistance.
> html:36
> (Diff revision 1)
> > +  request.onconnectionavailable = testHandler("PresentationRequest.onconnectionavailable");
> > +  await testPromise(request.start(), "PresentationRequest.start");
> > +  await testPromise(request.reconnect("foo"), "PresentationRequest.reconnect");
> > +  await testPromise(request.getAvailability(), "Presentation.getAvailability");
> > +
> > +  // These interfaces are not directly instantiatable.
> 
> Is it possible to generate these interfaces as they would be used by a
> content script? Or are content scripts fully blocked by using these
> interfaces when privacy.resistFingerprinting = true?

After further reading of the specification, the standard doesn't define constructors for these interfaces.
(e.g., https://w3c.github.io/presentation-api/#interface-presentationconnection)

That is, content script can't explicitly create instances of them, regardless of privacy.resistFingerprinting.

In practice, if we write

    var connection = new PresentationConnection();

an exception will be thrown

    TypeError: Illegal constructor.

> :::
> dom/presentation/tests/mochitest/test_presentation_fingerprinting_resistance.
> html:60
> (Diff revision 1)
> > +  // let connectionList = new PresentationConnectionList();
> > +  // connectionList.onconnectionavailable = testHandler("PresentationConnectionList.onconnectionavailable");
> > +  // SimpleTest.is(connectionList.connections.length, 0, "PresentationConnectionList.connections.length");
> > +
> > +  // Wait for events to be fired.
> > +  setTimeout(() => SimpleTest.finish(), 3000);
> 
> I wonder if you need to use a timeout like this? On other APIs, I believe
> any triggered event will be added to the end of the JS execution queue. So
> on previous mochitests I have used:
> 
> exerciseAPI();
> triggerDummyEvent();
> let result = await Promise.race([apiEvent, dummyEvent]);

I'm sorry the comment is quite confusing.

I'm not really going to fire events but make sure the listener doesn't receive any event.

I will change the comment to

    // Wait for a while to guarantee there is no events fired.

Does it look more reasonable?

Comment 20

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #19)
> In practice, if we write
> 
>     var connection = new PresentationConnection();
> 
> an exception will be thrown
> 
>     TypeError: Illegal constructor.

You are right that content scripts can't call the constructor. But I am concerned that a content script can acquire an instance of a PresentationConnection (for example) by other means. I'm not too familiar with the APIs, but here is an example I found (that is likely blocked by your patch):

const request = new PresentationRequest(receiverUrl);
const connection = await request.start();
const myConnectionId = connection.id;

I haven't studied it carefully, but I think there may be other ways for content scripts to obtain PresentationConnection instances as well. So my question is whether your patch blocks all ways for content scripts to acquire a PresentationConnection instance. And does it block access to instances of the other interfaces as well? If so, great -- I think it would be helpful to explain that in your comments.

> > I wonder if you need to use a timeout like this? On other APIs, I believe
> > any triggered event will be added to the end of the JS execution queue. So
> > on previous mochitests I have used:
> > 
> > exerciseAPI();
> > triggerDummyEvent();
> > let result = await Promise.race([apiEvent, dummyEvent]);
> 
> I'm sorry the comment is quite confusing.
> 
> I'm not really going to fire events but make sure the listener doesn't
> receive any event.
> 
> I will change the comment to
> 
>     // Wait for a while to guarantee there is no events fired.
> 
> Does it look more reasonable?

That is an improvement to the comment, thanks! :) But what I meant is, I think I see a way to avoid using a timeout altogether. A timeout is probably OK, though.

One more question: if you disable privacy.resistFingerprinting, does your script detect the events?

Comment 21

[Chung-Sheng Fu [:cfu]]

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #20)
> (In reply to Chung-Sheng Fu [:cfu] from comment #19)
> 
> You are right that content scripts can't call the constructor. But I am
> concerned that a content script can acquire an instance of a
> PresentationConnection (for example) by other means. I'm not too familiar
> with the APIs, but here is an example I found (that is likely blocked by
> your patch):
> 
> const request = new PresentationRequest(receiverUrl);
> const connection = await request.start();
> const myConnectionId = connection.id;
> 
> I haven't studied it carefully, but I think there may be other ways for
> content scripts to obtain PresentationConnection instances as well. So my
> question is whether your patch blocks all ways for content scripts to
> acquire a PresentationConnection instance. And does it block access to
> instances of the other interfaces as well? If so, great -- I think it would
> be helpful to explain that in your comments.

Yes they can still be obtained by some other ways.

PresentationAvailability:
- PresentationRequest.getAvailability().then()
  The promise will always be rejected when privacy.resistFingerprinting = true

PresentationConnection:
- PresentationRequest.start().then()
  The promise will always be rejected when privacy.resistFingerprinting = true
- PresentationRequest.reconnect().then()
  The promise will always be rejected when privacy.resistFingerprinting = true
- PresentationRequest.onconnectionavailable
  The event will never fire when privacy.resistFingerprinting = true
- PresentationReceiver.connectionList
  The array will always be empty when privacy.resistFingerprinting = true
- PresentationConnectionList.onconnectionavailable
  The event will never fire when privacy.resistFingerprinting = true

PresentationReceiver:
- navigator.presentation.receiver
  The attribute will always be null when privacy.resistFingerprinting = true

PresentationConnectionList:
- PresentationReceiver.connectionList
  PresentationReceiver can only be obtained from navigator.presentation.receiver, which will always be null when privacy.resistFingerprinting = true

So I think the patch covers all possibilities.

> That is an improvement to the comment, thanks! :) But what I meant is, I
> think I see a way to avoid using a timeout altogether. A timeout is probably
> OK, though.

Thanks for the suggestion!  I will try it.

> One more question: if you disable privacy.resistFingerprinting, does your
> script detect the events?

I'm afraid that it won't because there is no receiver at the requested URL.

Comment 22

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #21)
> (In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from
> comment #20)
> [...]
> Yes they can still be obtained by some other ways.
> [...] 
> So I think the patch covers all possibilities.

Great! Thank you for the list. I wonder if it might be practical to confirm these facts via tests in the mochitest.

> I'm afraid that it won't because there is no receiver at the requested URL.

That does seem to be a concern. We want to be sure that any future regression in your fingerprinting resistance patch is caught by your tests. For example, if the code is someday replaced by a Rust implementation.

I notice that some of the existing Presentation API mochitests are loading a script to simulate a receiver device. Here's one:
https://dxr.mozilla.org/mozilla-central/rev/892c8916ba32b7733e06bfbfdd4083ffae3ca028/dom/presentation/tests/mochitest/test_presentation_1ua_connection_wentaway.js#10

I wonder if that script could be used similarly in your test patch?

Comment 23

[Chung-Sheng Fu [:cfu]]

The Presentation API folk suggested we should just test if the controller interfaces are disabled as our expectation without enabling receivers.

I will ask him again how to create receiving browsing context in our test.

Comment 24

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/162898/diff/4-5/

Comment 25

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8898709 [details]
Bug 1382533 - Add tests for Presentation API fingerprinting resistance.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170110/diff/1-2/

Comment 26

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8898709 [details]
Bug 1382533 - Add tests for Presentation API fingerprinting resistance.

The test is totally renewed, please take a look again.

Now an available connection is established with privacy.resistFingerprinting = false.  After toggling the pref, the interfaces of the request and the receiver will be disabled.

Comment 27

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8898709 [details]
Bug 1382533 - Add tests for Presentation API fingerprinting resistance.

https://reviewboard.mozilla.org/r/170110/#review180432

lgtm

Comment 28

[Arthur Edelstein [:arthur]]

Comment on attachment 8898709 [details]
Bug 1382533 - Add tests for Presentation API fingerprinting resistance.

Sorry for the delay. Looks good to me as well.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aad2ab1d510c
Disable Presentation API when privacy.resistFingerprinting = true r=arthuredelstein,smaug
https://hg.mozilla.org/integration/autoland/rev/92659c8ed9f2
Add tests for Presentation API fingerprinting resistance. r=schien,smaug

Comment 30

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/aad2ab1d510c
https://hg.mozilla.org/mozilla-central/rev/92659c8ed9f2