Closed Bug 1382533 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: arthur, Assigned: cfu)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

Bug 1382533 - Disable Presentation API when privacy.resistFingerprinting = true
59 bytes, text/x-review-board-request
smaug
: review+
arthur
: review+
Details
Bug 1382533 - Add tests for Presentation API fingerprinting resistance.
59 bytes, text/x-review-board-request
arthur
: review+
schien
: review+
Details 
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: nobody → cfu
Attachment #8891876 - Flags: review?(hurley)
This patch simply avoids all implementations of mozilla::net::DoListAddresses from reporting information about local network when privacy.resistFingerprinting = true.
Attachment #8891876 - Flags: review?(arthuredelstein)
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)
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.
(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.
(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 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)
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 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+
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)
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+
Attachment #8898709 - Flags: review?(bugs)
Attachment #8898709 - Flags: review?(arthuredelstein)
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 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+
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)
(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)
(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)
(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.
(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?
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.
Attachment #8898709 - Flags: review?(arthuredelstein)
Attachment #8898709 - Flags: review+
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+
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+
Keywords: checkin-needed
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: