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

RESOLVED FIXED in Firefox 57

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: arthur, Assigned: cfu)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [tor 22165][fingerprinting][fp:m3])

Attachments

(2 attachments)

Reporter

Description

2 years ago
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
Priority: -- → P2
Whiteboard: [tor 22165] → [tor 22165][fingerprinting][fp:m3]
Assignee

Updated

2 years ago
Assignee: nobody → cfu
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8891876 - Flags: review?(hurley)
Assignee

Comment 3

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

Updated

2 years ago
Attachment #8891876 - Flags: review?(arthuredelstein)

Comment 4

2 years ago
mozreview-review
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 :)
Attachment #8891876 - Flags: review?(hurley)
Assignee

Comment 5

2 years ago
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

2 years ago
(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.
Reporter

Comment 7

2 years ago
(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).
Reporter

Comment 8

2 years ago
mozreview-review
Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

https://reviewboard.mozilla.org/r/162898/#review171360
Attachment #8891876 - Flags: review?(arthuredelstein)
Comment hidden (mozreview-request)
Assignee

Comment 10

2 years ago
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

2 years ago
mozreview-review
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()
Attachment #8891876 - Flags: review?(bugs) → review+
Reporter

Comment 12

2 years ago
mozreview-review
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.
Attachment #8891876 - Flags: review?(arthuredelstein)
Reporter

Comment 13

2 years ago
mozreview-review
Comment on attachment 8891876 [details]
Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true

https://reviewboard.mozilla.org/r/162898/#review174106
Attachment #8891876 - Flags: review+
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8898709 - Flags: review?(bugs)
Attachment #8898709 - Flags: review?(arthuredelstein)
Assignee

Comment 16

2 years ago
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

2 years ago
mozreview-review
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.
Attachment #8898709 - Flags: review?(bugs) → review+
Reporter

Comment 18

2 years ago
mozreview-review
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]);
Attachment #8898709 - Flags: review?(arthuredelstein)
Assignee

Comment 19

2 years ago
(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?
Flags: needinfo?(arthuredelstein)
Reporter

Comment 20

2 years ago
(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?
Flags: needinfo?(arthuredelstein)
Assignee

Comment 21

2 years ago
(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.
Reporter

Comment 22

2 years ago
(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?
Assignee

Comment 23

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Attachment #8898709 - Flags: review?(arthuredelstein)
Attachment #8898709 - Flags: review+
Assignee

Comment 26

2 years ago
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.
Attachment #8898709 - Flags: review?(bugs)
Attachment #8898709 - Flags: review?(arthuredelstein)
Attachment #8898709 - Flags: review?(bugs) → review?(schien)
Comment on attachment 8898709 [details]
Bug 1382533 - Add tests for Presentation API fingerprinting resistance.

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

lgtm
Attachment #8898709 - Flags: review?(schien) → review+
Reporter

Comment 28

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

Sorry for the delay. Looks good to me as well.
Attachment #8898709 - Flags: review?(arthuredelstein) → review+
Assignee

Updated

2 years ago
Keywords: checkin-needed

Comment 29

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/aad2ab1d510c
https://hg.mozilla.org/mozilla-central/rev/92659c8ed9f2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.