1372073 - Neutralize the threat of fingerprinting of media devices API when 'privacy.resistFingerprinting' is true

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement, P3)

Description

[Tim Huang[:timhuang]]

The navigator.mediaDevices API can reveal one's hardware about the source of media data, which is a fingerprinting vector. So, we want to neutralize this fingerprinting issue when 'privacy.resistFingerprinting' is on.

Maybe we can throw exceptions when accessing these APIs entries when 'privacy.resistFingerprinting' is on. Or we can spoof these APIs and block the ondevicechange event.

Comment 1

[Tim Huang[:timhuang]]

Do you have a good suggestion regarding how to do this, Arthur and jib?

Comment 2

[Tom Ritter [:tjr] (OOTO until mid-August)]

What is the most common usage pattern of these APIs?  Is it '.enumerateDevices()' and then '.getUserMedia()' (where getUserMedia() causes the prompt?)

Can we hardcode a response to enumerateDevices (maybe lying about hardware present, maybe not, but certainly providing only fixed data about what we return) so that the call to getUserMedia will cause the prompt after which we would stop lying? Can we make .enumerateDevices() cause the prompt?

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The purpose of the enumerateDevices API is twofold:

 1) Let the site know whether user has both cam+mic, cam only, mic only, or neither, to know whether to bother with gUM at all.

 2) For users with more than one cam or mic, get deviceIds from last visit, to remember user preference.

For #2 the default behavior of enumerateDevices in Firefox is to not persist deviceIds from one run of the browser to the next, if the user has never granted gUM (or has cleared cookies since they granted gUM). IOW it returns new ids instead.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Note that deviceIds are unique to the origin.

Also, in private browsing mode, we use a separate set of deviceIds which are never persisted across runs of the browser, and are reset when the last pb browsing session from the origin is closed.

Comment 5

[Arthur Edelstein [:arthur]]

I did a manual test and confirmed that deviceIds seem to not persist across first party domain.

So the remaining privacy concern seems to be number and type of devices and any other information that might be leaked by this API.

I agree with Tom that the best approach seems to be to give a fixed response. Perhaps the best would be to claim the presence of a single camera and a single microphone.

Then if the content script requests the use of the claimed camera (for example) and the camera is not actually present, then perhaps a "permission denied" spoof could be returned after a random interval.

Comment 6

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

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

Comment 7

[Chung-Sheng Fu [:cfu]]

navigator.mediaDevices.enumerateDevices will always return a single camera and a single microphone.  If the client has more than one camera/microphone, the first one will be reported.  If the client has no camera/microphone, a fake one will be reported, using MediaEngineDefaultAudioSource/MediaEngineDefaultVideoSource like media.navigator.streams.fake does.

The patch doesn't change navigator.mediaDevices.getUserMedia because when the content script requests the fake device like:

    navigator.mediaDevices.getUserMedia({audio: {deviceId: 'the-fake-microphone'}}).then(...).catch(...)

a NotFoundError will be thrown.  Can we just keep this behavior, instead of throwing NotAllowedError?

Comment 8

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #7)

> The patch doesn't change navigator.mediaDevices.getUserMedia because when
> the content script requests the fake device like:
> 
>     navigator.mediaDevices.getUserMedia({audio: {deviceId:
> 'the-fake-microphone'}}).then(...).catch(...)
> 
> a NotFoundError will be thrown.  Can we just keep this behavior, instead of
> throwing NotAllowedError?

Hi Chung-Sheng,

I think we should throw NotAllowedError so that a fake device and a user blocking use of the single camera are indistinguishable.

On another subject: I looked into getUserMedia further I found that it is possible for a content page script in Firefox to silently detect the dimensions of the user's cameras, using the getUserMedia() call. Here's a demo:

https://arthuredelstein.github.io/tordemos/mediaDevices.html

I tried this on Mac, Linux, and Windows laptops. (Oddly, the dimensions I get from getUserMedia() seem to return incorrect but fingerprintable values on all platforms.) I think it's probably also possible to extract more properties from cameras or microphones. So I think it's probably good if we can ignore most of the constraints passed to getUserMedia().

Also I wonder if navigator.mediaDevices.getSupportedConstraints() can be fingerprintable? (I haven't investigated this.) And I think we will also need to suppress "devicechange" events.

Comment 9

[Arthur Edelstein [:arthur]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review182586

Postponing review while we look at further things that need to be suppressed. (Also it would be good to have a regression test.)

Comment 10

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

What do you think if we only take care about deviceId, ignoring other constraints in getUserMedia when privacy.resistFingerprinting = true?  Also getSupportedConstraints may return like

    {
      deviceId: true,
      width: false,
      height: false,
      ...
    }

Will this change break something?

Comment 11

[Arthur Edelstein [:arthur]]

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #8)

> https://arthuredelstein.github.io/tordemos/mediaDevices.html
> 
> I tried this on Mac, Linux, and Windows laptops. (Oddly, the dimensions I
> get from getUserMedia() seem to return incorrect but fingerprintable values
> on all platforms.) 

It turns out each camera offers multiple available widths and heights. I updated the demo to list them.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #11)
> (In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from
> comment #8)
> 
> > https://arthuredelstein.github.io/tordemos/mediaDevices.html
> > 
> > I tried this on Mac, Linux, and Windows laptops. (Oddly, the dimensions I
> > get from getUserMedia() seem to return incorrect but fingerprintable values
> > on all platforms.) 
> 
> It turns out each camera offers multiple available widths and heights. I
> updated the demo to list them.

This will in fact be HW (camera) dependent, and is as expected.  Note that since gUM is a discovery API, you would not only need to set Enumerate to only show a standard list of resolutions (320x240, 640x480, 1280x720, 1920x1080), but also you'd need to internally select a same-or-larger resolution and then rescale the camera output to that.  In cases where the camera can't provide the resolution you may need to upscale.  Also, real-world cameras may not be able to provide high resolutions at 30fps; I advice limiting to 15fps max for 1920x1080, or perhaps even eliminating it.   Effectively you need to implement a pseudo-camera wrapping the real camera so far as the system is concerned. This code would live in CamerasParent.cpp most likely, and since that drives the rest of the system, it would believe that you had such a pseudo camera and all constraints/etc would work.

Note that old, cheap USB cameras might max out at 640x480 or even 320x240.

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review182798

Hi, can you point me to a doc explaining the expectations wrt getUserMedia() in this fingerprint-conservative mode?

From looking at this patch, "privacy.resistFingerprinting" looks identical to "media.navigator.streams.fake", except it only modifies enumerateDevices, not getUserMedia, so gUM still produces real cameras and mics. Fallout from this difference is the two APIs no longer speak the same IDs. Specifically:

This will BREAK https://webrtc.github.io/samples/src/content/devices/input-output/ by not playing any camera or mic output at all.

It will break all use of getUserMedia with deviceId basically, e.g.: https://jsfiddle.net/jib1/0njx3n3h/  

    let devices = await navigator.mediaDevices.enumerateDevices();
    let video = {deviceId: {exact: devices.find(d => d.kind == "videoinput").deviceId}};
    let audio = {deviceId: {exact: devices.find(d => d.kind == "audioinput").deviceId}};
    element.srcObject = await navigator.mediaDevices.getUserMedia({video, audio});

...which violates the spec, I believe. To solve this, we'd need to change getUserMedia to accept the "fake" ids, but unlike "media.navigator.streams.fake", produce the real cam and mic (A sufficient implementation may be to simply ignore deviceIds in this mode).

Otherwise patch looks like it works, just more code than necessary I think, so I've requested a simplification below. Also, we should add a mochitest, including the above use-case (to the extent possible, maybe testing turning off "media.navigator.streams.fake" during enumerateDevices, and turning it back on before calling gUM, since we can't hit real hardware in automated tests).

::: dom/media/MediaManager.cpp:1770
(Diff revision 1)
> -    if (hasVideo) {
> -      nsTArray<RefPtr<VideoDevice>> videos;
> -      GetSources(fakeCams? fakeBackend : realBackend, aVideoType,
> -                 &MediaEngine::EnumerateVideoDevices, videos,
> -                 videoLoopDev.get());
> -      for (auto& source : videos) {
> -        result->AppendElement(source);
> -      }
> -    }
> +    GetMediaDevices<VideoDevice, MediaEngineDefaultVideoSource>(
> +      fakeCams ? fakeBackend : realBackend,
> +      aVideoType,
> +      &MediaEngine::EnumerateVideoDevices,
> +      videoLoopDev.get(),
> +      hasVideo,
> +      aResistFingerprinting,
> +      result
> +    );

Doesn't this new code boil down to just:

    if (hasVideo) {
      nsTArray<RefPtr<VideoDevice>> videos;
      GetSources((fakeCams || aResistFingerprinting)? fakeBackend : realBackend,
                 aVideoType,
                 &MediaEngine::EnumerateVideoDevices, videos,
                 videoLoopDev.get());
      for (auto& source : videos) {
        result->AppendElement(source);
      }

?

Note fakeBackend delivers a MediaEngineDefaultVideoSource already [1]. Same for fakeMics.

[1] https://dxr.mozilla.org/mozilla-central/rev/37b95547f0d27565452136d16b2df2857be840f6/dom/media/webrtc/MediaEngineDefault.cpp#538,560-561

::: dom/media/MediaManager.cpp:2495
(Diff revision 1)
>    bool askPermission =
>        (!privileged || Preferences::GetBool("media.navigator.permission.force")) &&
>        (!fake || Preferences::GetBool("media.navigator.permission.fake"));
>  
>    RefPtr<PledgeSourceSet> p = EnumerateDevicesImpl(windowID, videoType,
> -                                                   audioType, fake);
> +                                                   audioType, fake, false);

As you no doubt see from the code here, getUserMedia enumerates devices as part of all requests, and a false here means getUserMedia can still be used for fingerprint matching against an earlier obtained id using OverconstrainedError (without prompt):

    await navigator.mediaDevices.getUserMedia({audio: {deviceId: {exact: id}}});

or, once bug 1398180 is fixed, without an earlier obtained id (but behind a prompt):

    track.getSettings().deviceId;

Just checking expectations.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review182874

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review182798

> As you no doubt see from the code here, getUserMedia enumerates devices as part of all requests, and a false here means getUserMedia can still be used for fingerprint matching against an earlier obtained id using OverconstrainedError (without prompt):
> 
>     await navigator.mediaDevices.getUserMedia({audio: {deviceId: {exact: id}}});
> 
> or, once bug 1398180 is fixed, without an earlier obtained id (but behind a prompt):
> 
>     track.getSettings().deviceId;
> 
> Just checking expectations.

I guess I'm asking here whether fingerprinting-protection goals extend to when users agree to sharing camera and mic. If so, then there's more work TBD, since track.getSettings() and track.applyConstraints() expose quite a bit of surface, even without bug 1398180.

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

You might also want to block the ondevicechange event here. Not much use for it, and it can be a time correlated across domains. It currently requires permission, but see bug 1397977 and bug 1397978.

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Chung-Sheng Fu [:cfu] from comment #7)
> If the client has more than one camera/microphone, the first one will be reported.
> If the client has no camera/microphone, a fake one will be reported,

Sorry, I just saw this comment, and I misread your patch slightly then. But this seems to leak information, in that you'll get a different id suddenly if someone inserts or removes a USB cam or mic. So I think my advice is the same, always return a fake deviceId, and ignore it in getUserMedia instead.

We could still throw OverconstrainedError on *other* ids, if we're concerned about sites detecting this mode.

> a NotFoundError will be thrown.  Can we just keep this behavior, instead of
> throwing NotAllowedError?

NotFoundError is bits about a user's system (no camera, or no mic, or both).
NotAllowedError is bits about a user's current preference.

What to return depends on the illusion desired I think. If we want to protect these bits we should fix both APIs to project the same illusion, either a user with one camera and one mic always (fake if need be), or a user with neither. My bias would be to try to get real camera and mic working in this mode.

Comment 18

[Chung-Sheng Fu [:cfu]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #17)
> (In reply to Chung-Sheng Fu [:cfu] from comment #7)
> > If the client has more than one camera/microphone, the first one will be reported.
> > If the client has no camera/microphone, a fake one will be reported,
> 
> Sorry, I just saw this comment, and I misread your patch slightly then. But
> this seems to leak information, in that you'll get a different id suddenly
> if someone inserts or removes a USB cam or mic. So I think my advice is the
> same, always return a fake deviceId, and ignore it in getUserMedia instead.

I didn't notice this scenario.  Thank you for pointing it out.

I was also wondering if we can treat "privacy.resistFingerprinting" as "media.navigator.streams.fake" in enumerateDevices.  But I guess Arthur's idea in comment 5 is to keep the APIs as compatible as possible so that there will still be available devices for web sites to select.


Hi Arthur,

What do you think if enumerateDevices always reports a fake camera and a fame mic?

> We could still throw OverconstrainedError on *other* ids, if we're concerned
> about sites detecting this mode.
> 
> > a NotFoundError will be thrown.  Can we just keep this behavior, instead of
> > throwing NotAllowedError?
> 
> NotFoundError is bits about a user's system (no camera, or no mic, or both).
> NotAllowedError is bits about a user's current preference.
> 
> What to return depends on the illusion desired I think. If we want to
> protect these bits we should fix both APIs to project the same illusion,
> either a user with one camera and one mic always (fake if need be), or a
> user with neither. My bias would be to try to get real camera and mic
> working in this mode.

For getUserMedia, I'm planning to make it throw a NotAllowedError when it is going to report a fake device when "privacy.resistFingerprinting" is true.  If this idea is applicable, it will indicate that once we treat "privacy.resistFingerprinting" as "media.navigator.streams.fake", getUserMedia will always throw NotAllowedError because it will always report fake devices.  Is this behavior reasonable?

Besides, I guess we will not have to take care about the constraints if we treat "privacy.resistFingerprinting" as "media.navigator.streams.fake" because we will be using only fake devices.  Also, it seems that the function getSupportedConstraints itself is not fingerprintable because it always returns the same values.
http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/dom/media/MediaDevices.h#38

Thank you very much.

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

What I meant is return deviceIds based on fake devices from enumerateDevices, and ignore deviceIds in getUserMedia.

The net result should be deviceIds different from regular (without pref) runs, yet default camera and mic should still work.

Comment 20

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #18)

> What do you think if enumerateDevices always reports a fake camera and a
> fame mic?

Hi Chung-Sheng. How about the following behavior?

1. navigator.mediaDevices.enumerateDevices() reports two fake device Ids: [fakeCameraDeviceId, fakeMicDeviceId].
2. Web page script calls `let mediaPromise = getUserMedia({video : { deviceID : { exact : fakeCameraDeviceId } [ plus other constraints ] } });`.
3. User is shown a prompt: "Will you allow 'github.io' to use your camera?" and a dropdown menu allowing the user to choose "Camera to share".
4. If the user selects a real camera, and clicks "Allow", then mediaPromise resolves to a MediaStream from that camera, regardless of deviceId or other constraints in the getUserMedia(...) call in step 2.
5. If the user clicks "Don't Allow" then mediaPromise is rejected with the same error, regardless of constraints in step 2.

(Maybe the above behavior is what you are already proposing; I'm just trying to confirm that I understand.)

This behavior implies that by choosing to allow the use of a read (hardware) camera, the user unavoidably leaks details about their camera (such as dimensions and framerate). But if the user does not allow a camera to be used, then no real information about hardware (its existence or characteristics) is passed to content.

Comment 21

[Chung-Sheng Fu [:cfu]]

This approach looks great, and I think it also applies Jan-Ivar's suggestion.  Thank you for clarifying it!  I will implement it very soon.

Comment 22

[Bulk Bug Changes for mreavy's org]

Mass change P2->P3 to align with new Mozilla triage process.

Comment 23

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1372073 - Ignore constraints in getUserMedia.

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

Comment 24

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1372073 - Suppress devicechange event.

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

Comment 25

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

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

Comment 26

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

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

Comment 27

[Chung-Sheng Fu [:cfu]]

For enumerateDevices, just make it work like "media.navigator.streams.fake" is true to report a single camera and a single microphone with fake IDs.

For getUserMedia, only take care about if user is requesting audio or video stream, ignoring all constraints.  For example, if the passed constraint looks like

    {
      video: {
        deviceId: "foo",
        width: {min: 1024, ideal: 1280, max: 1920}
      }
    }

it will be regarded as

    {
      video: true
    }

If there is no available device, it implies that content script is requesting a fake device, and throws NotAllowedError instead of NotFoundError.

Also the devicechange is suppressed when "privacy.resistFingerprinting" is true.

The test works by the following steps:
1. Set "media.video_loopback_dev" and "media.audio_loopback_dev" so that real devices will not be enumerated because we don't know what the testing environment exactly is.
2. enumerateDevices(), and there should be no devices reported.
3. Set "privacy.resistFingerprinting".
4. enumerateDevices(), and it should report a fake camera and a fake microphone.
5. getUserMedia() with constraints [{audio: true}, {video: true}, {audio: true, video: true}], and all of them should be rejected with NotAllowedError.

Comment 28

[Arthur Edelstein [:arthur]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review187300

::: dom/media/MediaManager.cpp:2777
(Diff revision 2)
>    // Create an inactive SourceListener to act as a placeholder, so the
>    // window listener doesn't clean itself up until we're done.
>    RefPtr<SourceListener> sourceListener = new SourceListener();
>    windowListener->Register(sourceListener);
>  
> -  bool fake = Preferences::GetBool("media.navigator.streams.fake");
> +  bool fake = Preferences::GetBool("media.navigator.streams.fake") ||

I think here and in the other patches we should only resist fingerprinting in a content document; chrome code should be able to obtain the true values. Maybe the one-argument version of nsContentUtils::ShouldResistFingerprinting(...) can be used?

Comment 29

[Arthur Edelstein [:arthur]]

Comment on attachment 8909666 [details]
Bug 1372073 - Suppress devicechange event.

https://reviewboard.mozilla.org/r/181182/#review187306

::: dom/media/MediaManager.cpp:2074
(Diff revision 1)
>        return NS_OK;
>      }
> +
> +    // When privacy.resistFingerprinting = true, don't fire event but still
> +    // update internal states.
> +    if (!nsContentUtils::ShouldResistFingerprinting()) {

Again check that we're not in chrome.

Comment 30

[Arthur Edelstein [:arthur]]

Comment on attachment 8909667 [details]
Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

https://reviewboard.mozilla.org/r/181184/#review187310

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:17
(Diff revision 1)
> +      ["media.video_loopback_dev", "bar"]
> +    ]
> +  });
> +}
> +
> +let checkDevices = () => new Promise((resolve) => {

nit: I think it's more readable to use await/async and try/catch than Promise...then...catch, so I would suggest using that instead in each of your test functions.

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:63
(Diff revision 1)
> +    {audio: true}, {video: true}, {audio: true, video: true}
> +  ];
> +  return Promise.all(constraints.map((constraint) => new Promise((resolve) => {
> +    navigator.mediaDevices.getUserMedia(constraint)
> +        .then(() => {
> +          SimpleTest.ok(false, "Should not get media stream");

Is it possible to produce a simulated media device in mochitests? (I haven't checked.) It would be ideal if we could show that a device can be obtained even though we have used a fake id.

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:78
(Diff revision 1)
> +
> +(async () => {
> +  SimpleTest.waitForExplicitFinish();
> +  await setup();
> +  await checkDevices();
> +  await enableResistFingerprinting();

I think it would be nice to run the same tests both with and without "privacy.resistFingerprinting". Otherwise we don't know if it's having the expected effect.

Comment 31

[Arthur Edelstein [:arthur]]

Comment on attachment 8909665 [details]
Bug 1372073 - Ignore constraints in getUserMedia.

https://reviewboard.mozilla.org/r/181180/#review187302

::: dom/media/MediaManager.cpp:2245
(Diff revision 1)
>    }
>  
> +  // When privacy.resistFingerprinting = true, we just take care about if
> +  // content script is requesting video/audio device, ignoring all constraints.
> +  const bool resistFingerprinting = nsContentUtils::ShouldResistFingerprinting();
> +  if (resistFingerprinting) {

Again only resist fingerprinting if we're not in chrome.

Comment 32

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review187806

::: dom/media/MediaManager.cpp:2777
(Diff revision 2)
>    // Create an inactive SourceListener to act as a placeholder, so the
>    // window listener doesn't clean itself up until we're done.
>    RefPtr<SourceListener> sourceListener = new SourceListener();
>    windowListener->Register(sourceListener);
>  
> -  bool fake = Preferences::GetBool("media.navigator.streams.fake");
> +  bool fake = Preferences::GetBool("media.navigator.streams.fake") ||

Agree. This is important, especially because enumerateDevices is used by the preview code inside  Firefox's own permission prompt for screensharing.

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8909665 [details]
Bug 1372073 - Ignore constraints in getUserMedia.

https://reviewboard.mozilla.org/r/181180/#review187808

r- for treating screenshare request as camera request

::: dom/media/MediaManager.cpp:2245
(Diff revision 1)
>    }
>  
> +  // When privacy.resistFingerprinting = true, we just take care about if
> +  // content script is requesting video/audio device, ignoring all constraints.
> +  const bool resistFingerprinting = nsContentUtils::ShouldResistFingerprinting();
> +  if (resistFingerprinting) {

Agree again.

::: dom/media/MediaManager.cpp:2246
(Diff revision 1)
> +    if (IsOn(c.mVideo)) {
> +      c.mVideo.SetAsBoolean() = true;
> +    }

Note that this will blow out the mediaSource constraint, which in Firefox determines whether camera or screensharing is requested. See examples [1]. If the goal is to disable screensharing, it should be with an error, not treating it as a request for camera. 

[1] http://mozilla.github.io/webrtc-landing/gum_test.html

::: dom/media/MediaManager.cpp:2249
(Diff revision 1)
> +    if (IsOn(c.mAudio)) {
> +      c.mAudio.SetAsBoolean() = true;
> +    }

mediaSource exists for audio as well: "microphone" and "audiocapture", though the latter is chrome only for now.

Comment 34

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8909667 [details]
Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

https://reviewboard.mozilla.org/r/181184/#review187822

Great to see tests!

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:4
(Diff revision 1)
> +<!DOCTYPE html>
> +<meta charset="utf-8">
> +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> +<script>

Please include:

    createHTML({title: "your title", bug: "1372073"});

or equivalent.

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:10
(Diff revision 1)
> +      // Disallow enumerating real devices.
> +      ["media.audio_loopback_dev", "foo"],
> +      ["media.video_loopback_dev", "bar"]

Maybe add a comment that the intent is to test that absence of devices can't be detected?

In addition to this, it might be useful to still test enumerateDevices without these prefs as well, to make sure it returns one audioinput and one videoinput successfully (yes I know that's our default fake behavior in mochitests, but still, it's desired behavior with this feature and important to test).

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:17
(Diff revision 1)
> +let checkDevices = () => new Promise((resolve) => {
> +  navigator.mediaDevices.enumerateDevices()
> +      .then((devices) => {
> +        SimpleTest.is(devices.length, 0, "Client has no device");
> +        resolve();
> +      })

Please avoid the promise constructor anti-pattern [1] as it will swallow even programming errors inside .then()s.

Using an async function like Arthur suggests, is a good idea. Then we can use plain old try{}catch(){}

Applies throughout.

[1] https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:63
(Diff revision 1)
> +    {audio: true}, {video: true}, {audio: true, video: true}
> +  ];
> +  return Promise.all(constraints.map((constraint) => new Promise((resolve) => {
> +    navigator.mediaDevices.getUserMedia(constraint)
> +        .then(() => {
> +          SimpleTest.ok(false, "Should not get media stream");

Yes, getUserMedia() in mochitests use fake devices already. The reason it fails here is because of setup().

I agree it would be useful to test that getUserMedia still succeeds with resistFingerprinting.

Comment 35

[Chung-Sheng Fu [:cfu]]

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #33)
> Comment on attachment 8909665 [details]
> Bug 1372073 - Ignore constraints in getUserMedia.
> 
> https://reviewboard.mozilla.org/r/181180/#review187808
> 
> r- for treating screenshare request as camera request
> 
> ::: dom/media/MediaManager.cpp:2245
> (Diff revision 1)
> >    }
> >  
> > +  // When privacy.resistFingerprinting = true, we just take care about if
> > +  // content script is requesting video/audio device, ignoring all constraints.
> > +  const bool resistFingerprinting = nsContentUtils::ShouldResistFingerprinting();
> > +  if (resistFingerprinting) {
> 
> Agree again.
> 
> ::: dom/media/MediaManager.cpp:2246
> (Diff revision 1)
> > +    if (IsOn(c.mVideo)) {
> > +      c.mVideo.SetAsBoolean() = true;
> > +    }
> 
> Note that this will blow out the mediaSource constraint, which in Firefox
> determines whether camera or screensharing is requested. See examples [1].
> If the goal is to disable screensharing, it should be with an error, not
> treating it as a request for camera.

Thanks!  I really learned a lot :)

I will keep the mediaSource constraint, and I think I can still ignore other constraints describing the device capability?


Hi Arthur,

Do you think we should disable screen sharing?  The streams itself seems to be the same as a camera source, and it also requires user permission, so I guess we can allow user to use it.

> [1] http://mozilla.github.io/webrtc-landing/gum_test.html
> 
> ::: dom/media/MediaManager.cpp:2249
> (Diff revision 1)
> > +    if (IsOn(c.mAudio)) {
> > +      c.mAudio.SetAsBoolean() = true;
> > +    }
> 
> mediaSource exists for audio as well: "microphone" and "audiocapture",
> though the latter is chrome only for now.

Comment 36

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Comment on attachment 8909666 [details]
Bug 1372073 - Suppress devicechange event.

https://reviewboard.mozilla.org/r/181182/#review188304

r+ with the nits Arthur pointed out

Comment 37

[Chung-Sheng Fu [:cfu]]

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #29)
> Comment on attachment 8909666 [details]
> Bug 1372073 - Suppress devicechange event.
> 
> https://reviewboard.mozilla.org/r/181182/#review187306
> 
> ::: dom/media/MediaManager.cpp:2074
> (Diff revision 1)
> >        return NS_OK;
> >      }
> > +
> > +    // When privacy.resistFingerprinting = true, don't fire event but still
> > +    // update internal states.
> > +    if (!nsContentUtils::ShouldResistFingerprinting()) {
> 
> Again check that we're not in chrome.

After second tracing, I think the better place to do this is MediaDevices::OnDeviceChange.  Content script adds listeners (by navigator.mediaDevices.ondevicechange or navigator.mediaDevices.addEventListener("devicechange")) to MediaDevices, and then MediaDevices adds itself to MediaManager, so MediaDevices is just one of the listeners of MediaManager.  If we avoid the self->DeviceChangeCallback::OnDeviceChange() call in MediaManager::OnDeviceChange, some other listeners, not only the JS listeners, will be suppressed.  Because our target is to prevent content script from getting such information, stopping firing event if MediaDevices seems more reasonable.

Besides, since ondevicechange is triggered by system, I guess we don't have to (and we can't?) consider the context principal or caller type?

Comment 38

[Chung-Sheng Fu [:cfu]]

(In reply to Chung-Sheng Fu [:cfu] from comment #37)
> After second tracing, I think the better place to do this is
> MediaDevices::OnDeviceChange.  Content script adds listeners (by
> navigator.mediaDevices.ondevicechange or
> navigator.mediaDevices.addEventListener("devicechange")) to MediaDevices,
> and then MediaDevices adds itself to MediaManager, so MediaDevices is just
> one of the listeners of MediaManager.  If we avoid the
> self->DeviceChangeCallback::OnDeviceChange() call in
> MediaManager::OnDeviceChange, some other listeners, not only the JS
> listeners, will be suppressed.  Because our target is to prevent content
> script from getting such information, stopping firing event if MediaDevices
> seems more reasonable.

typo: stopping firing event in MediaDevices

Comment 39

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #35)

> Hi Arthur,
> 
> Do you think we should disable screen sharing?  The streams itself seems to
> be the same as a camera source, and it also requires user permission, so I
> guess we can allow user to use it.

Hi Chung-Sheng,

Yes, I think allowing screen sharing is OK, provided that user permission is required, and there is nothing that reveals the screen size, number of screens, screen type, etc.

Comment 40

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

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

Comment 41

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909665 [details]
Bug 1372073 - Ignore constraints in getUserMedia.

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

Comment 42

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909666 [details]
Bug 1372073 - Suppress devicechange event.

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

Comment 43

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909667 [details]
Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

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

Comment 44

[Chung-Sheng Fu [:cfu]]

enumerateDevices:
Added NeedsCallerType in webidl, in order to distinguish calls from content script or chrome code.

getUserMedia:
Keep mediaSource in audio/video constraint, and ignore others.

ondevicechange:
Suppress it in MediaDevices, instead of MediaManager.

Test:
Now it works as follows
1. Set "media.navigator.streams.fake" and "privacy.resistFingerprinting" to true.
2. enumerateDevices.  Should return a camera and a microphone.
3. getUserMedia.  Can get streams.
4. Reset pref.
5. Set "media.audio_loopback_dev" and "media.video_loopback_dev", in order to disable real devices.
6. enumerateDevices.  Should list nothing.
7. Set "privacy.resistFingerprinting" to true.
8. enumerateDevices.  Should return a camera and a microphone.
9. getUserMedia.  Should be rejected by NotAllowedError.

Comment 45

[Tom Ritter [:tjr] (OOTO until mid-August)]

Attachment: GUM-helloworld.html

While fiddling around in this area I discovered the 'id' and 'label' attributes on MediaStreamTrack (returned from getVideoTracks and getAudioTracks). I've attached a getUserMedia hello world that illustrates the problem. (I'm just starting to explore this API, so there may be more scary things here.)



I have not figured out the source of the 'id' attribute, and if it reveals system information, but the 'label' attribute contains the make/model of my webcam.  We should standardize this property.

The id property should be investigated, and I don't believe it is the same as the deviceid talked about above. The track id should be random, I think.

Separately, I opened Bug 1405842 about Device IDs being the same the a single origin, and places that would be bad.

Comment 46

[Arthur Edelstein [:arthur]]

It turns out there is also a currentTime property on LocalMediaStream that reveals time with 17 digits of precision. We should round that as we have other sources of timing. And MediaStreamTrack and LocalMediaStream each have id properties that are UUIDs. We should confirm these are random and not somehow usable for tracking across first-party domains or containers.

Comment 47

[Chung-Sheng Fu [:cfu]]

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #46)
> It turns out there is also a currentTime property on LocalMediaStream that
> reveals time with 17 digits of precision. We should round that as we have
> other sources of timing. And MediaStreamTrack and LocalMediaStream each have
> id properties that are UUIDs. We should confirm these are random and not
> somehow usable for tracking across first-party domains or containers.

According to MDN, LocalMediaStream has been removed from the specification.
https://developer.mozilla.org/en-US/docs/Web/API/LocalMediaStream
It seems that getUserMedia used to return LocalMediaStream, while it now returns MediaStream.  So I guess we can ignore this interface?

Comment 48

[Tom Ritter [:tjr] (OOTO until mid-August)]

(In reply to Tom Ritter [:tjr] from comment #45)
> The id property should be investigated, and I don't believe it is the same
> as the deviceid talked about above. The track id should be random, I think.

It can either come remotely in the case of WebRTC or locally it's a real UUID: http://searchfox.org/mozilla-central/source/dom/media/MediaStreamTrack.cpp#148

Comment 49

[Chung-Sheng Fu [:cfu]]

If I understand correctly, MediaStreamTrack.id is random and we only have to consider MediaStreamTrack.label.

Is it a good idea to spoof it with fake device name?
http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/webrtc/MediaEngineDefault.cpp#343
http://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/webrtc/MediaEngineDefault.cpp#62

Comment 50

[Tom Ritter [:tjr] (OOTO until mid-August)]

(In reply to Chung-Sheng Fu [:cfu] from comment #49)
> If I understand correctly, MediaStreamTrack.id is random and we only have to
> consider MediaStreamTrack.label.
> 
> Is it a good idea to spoof it with fake device name?
> http://searchfox.org/mozilla-central/rev/
> 1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/webrtc/MediaEngineDefault.
> cpp#343
> http://searchfox.org/mozilla-central/rev/
> 1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/webrtc/MediaEngineDefault.
> cpp#62

Yes, spoofing it with a fake device name is the right approach. I suppose it would be best to make the value whatever is the most common value seen on the Internet, but I don't know what that would be.

Comment 51

[Chung-Sheng Fu [:cfu]]

(In reply to Tom Ritter [:tjr] (PTO 10/12-1016) from comment #50)
> Yes, spoofing it with a fake device name is the right approach. I suppose it
> would be best to make the value whatever is the most common value seen on
> the Internet, but I don't know what that would be.

Example in MSDN: "External USB Microphone"/"External USB Camera".
Example in MDN: "internal microphone"/"internal camera".
But I guess the names in our fake device implementation is the most neutral ones?  "Default Audio Device"/"Default Video Device".

Comment 52

[Tom Ritter [:tjr] (OOTO until mid-August)]

(In reply to Chung-Sheng Fu [:cfu] from comment #51)
> (In reply to Tom Ritter [:tjr] (PTO 10/12-1016) from comment #50)
> > Yes, spoofing it with a fake device name is the right approach. I suppose it
> > would be best to make the value whatever is the most common value seen on
> > the Internet, but I don't know what that would be.
> 
> Example in MSDN: "External USB Microphone"/"External USB Camera".
> Example in MDN: "internal microphone"/"internal camera".
> But I guess the names in our fake device implementation is the most neutral
> ones?  "Default Audio Device"/"Default Video Device".

My mac has an 'Internal Microphone" (with that capitalization) but a 'Facetime HD Camera'.  I guess 'Internal Microphone' and 'Internal Camera' are probably best...

Comment 53

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1372073 - Spoof MediaStreamTrack.

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

Comment 54

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

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

Comment 55

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909665 [details]
Bug 1372073 - Ignore constraints in getUserMedia.

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

Comment 56

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909666 [details]
Bug 1372073 - Suppress devicechange event.

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

Comment 57

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909667 [details]
Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

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

Comment 58

[Chung-Sheng Fu [:cfu]]

I added a patch that spoofs MediaStreamTrack.label by "Internal Microphone" or "Internal Camera" according to its kind.

I also updated the test so that when getUserMedia resolves, it checks all tracks of the stream if theirs labels are spoofed (because in the test, getUserMedia only resolves when both "media.navigator.streams.fake" and "privacy.resistFingerprinting" are true).

Comment 59

[Tom Ritter [:tjr] (OOTO until mid-August)]

Are settings on a device, after we've granted permission, important enough to hardcode? channelCount, noiseSuppression, frameRate, height, width....

Label seems like a needless leak that would have low compat problems, the other settings are more useful and only appear after the user has granted permission.

Comment 60

[Chung-Sheng Fu [:cfu]]

(In reply to Tom Ritter [:tjr] from comment #59)
> Are settings on a device, after we've granted permission, important enough
> to hardcode? channelCount, noiseSuppression, frameRate, height, width....
> 
> Label seems like a needless leak that would have low compat problems, the
> other settings are more useful and only appear after the user has granted
> permission.

This implementation depends on user's determination so that once permission is granted, some details will be unavoidably leaked, as Arthur mentioned in comment 20.

Hi Arthur,
Do you think we should spoof track settings after the user has granted permission?

Comment 61

[Arthur Edelstein [:arthur]]

Comment on attachment 8918163 [details]
Bug 1372073 - Spoof MediaStreamTrack.

https://reviewboard.mozilla.org/r/189020/#review195974

Comment 62

[Arthur Edelstein [:arthur]]

Comment on attachment 8909667 [details]
Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

https://reviewboard.mozilla.org/r/181184/#review195990

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:20
(Diff revision 3)
> +  }
> +  if (noDevices) {
> +    SimpleTest.is(devices.length, 0, "testEnumerateDevices: No devices");
> +    return;
> +  }
> +  let count = devices.reduce((count, device) => {

I find this use of reduce to be hard to read. Maybe just do something like:
let count = {audioinput: 0, videoinput: 0};
for (device of devices) {
    if (device.kind in count) {
      ++count[device.kind];
    }
}

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:35
(Diff revision 3)
> +async function testGetUserMedia(noDevices) {
> +  const constraints = [
> +    {audio: true}, {video: true}, {audio: true, video: true}
> +  ];
> +  for (let i = 0; i < constraints.length; ++i) {
> +    let constraint = constraints[i];

You can use
for (let constraint of constraints) {

Comment 63

[Arthur Edelstein [:arthur]]

(In reply to Chung-Sheng Fu [:cfu] from comment #60)
> Hi Arthur,
> Do you think we should spoof track settings after the user has granted
> permission?

Hi Tom and Chung-Sheng -- thanks for noticing this problem. I guess it is probably best to spoof only those settings that we can actually hide. I'm looking at the properties in https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings and I came up with three categories:

1. The following IDs probably don't need to be spoofed if they are random per first-party: deviceId, groupID.

2. Here are the properties I think we can spoof now:
* autoGainControl: Spoof as false
* echoCancellation: Spoof as false
* facingMode: Spoof to "user"
* latency: Spoof as 0.0
* noiseSuppression: Spoof as false
* volume: Spoof to 1.0

3. Then there are some properties I think we could spoof in the future, but it is very difficult, so we should open a second ticket:
* Video properties we should spoof only if we crop/rescale the video frames and resample the video frame rate: aspectRatio, frameRate, height, width.
* Audio properties we should spoof only if we resample the audio to a standard sample rate and sample size, and combine all tracks to mono: channelCount, sampleRate, sampleSize.

Do you agree with my assessments?

Comment 64

[Arthur Edelstein [:arthur]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review193266

This looks good to me. Further patches will of course be needed to

Comment 65

[Tom Ritter [:tjr] (OOTO until mid-August)]

(In reply to Arthur Edelstein (Tor Browser dev) [:arthuredelstein] from comment #63)
> (In reply to Chung-Sheng Fu [:cfu] from comment #60)
> > Hi Arthur,
> > Do you think we should spoof track settings after the user has granted
> > permission?
> 
> Hi Tom and Chung-Sheng -- thanks for noticing this problem. I guess it is
> probably best to spoof only those settings that we can actually hide. I'm
> looking at the properties in
> https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings and I
> came up with three categories:
> 
> 1. The following IDs probably don't need to be spoofed if they are random
> per first-party: deviceId, groupID.

Agreed

> 2. Here are the properties I think we can spoof now:
> * autoGainControl: Spoof as false
> * echoCancellation: Spoof as false
> * facingMode: Spoof to "user"
> * latency: Spoof as 0.0
> * noiseSuppression: Spoof as false
> * volume: Spoof to 1.0

I've talked a little bit with :jib about getUSerMedia prefs and fingerprinting vectors. 

echoCancellation, noiseSuppression, and autoGainControl are software features that have defaults in the browser (he referenced https://blog.mozilla.org/webrtc/fiddle-of-the-week-audio-constraints/ as a demo). We don't expose hardware support (or lack of support) via these items, so we can leave them alone.

facingMode, latency, and volume seem like they should be spoofed.


> 3. Then there are some properties I think we could spoof in the future, but
> it is very difficult, so we should open a second ticket:
> * Video properties we should spoof only if we crop/rescale the video frames
> and resample the video frame rate: aspectRatio, frameRate, height, width.
> * Audio properties we should spoof only if we resample the audio to a
> standard sample rate and sample size, and combine all tracks to mono:
> channelCount, sampleRate, sampleSize.
> 
> Do you agree with my assessments?

Is the reasoning behind this that if we lie about the numbers, one can simply look at the video (or audio) stream and say something like "Well, they report it's 200px wide in this javascript property, but it's clearly 1080px wide"?  If so, that makes sense to me. We don't have any mechanism to downscale video: Bug 1286945 tracks downscaling and decimating framerate for video. And resampling the audio is probably not a feature Firefox is going to pursue, if I was going to guess.

Plus: Once the user has accepted a permission prompt to share their camera they probably realize that, hey, this could fingerprint me. Cause they're probably going to see my face. 

Also: applying constraints to screensharing WILL result in a downscaling and decimating of frame rate, unlike video. "Unconstrained, you get the natural dimensions and frame rate of the screen/window." Exposing that data would be a fingerprinting vector.  BUT, exposing _anything_ via screen sharing (even if we lied about dimensions and frame rate) seems like a complete fingerprinting vector too, just like the camera. 

Because the items in Category 3 are all behind permission prompts, and the thing you're granting permission to is the biggest fingerprint vector of all, I think lying about the properties thus exposed can be a WONTFIX. If we wanted to, we could add an additional scary line in the permission prompts (when privacy.resistFingerprinting is enabled) that says something like "Granting this permission will allow [origin] to uniquely identify you."

Comment 66

[Arthur Edelstein [:arthur]]

(In reply to Tom Ritter [:tjr] from comment #65)
 
> Is the reasoning behind this that if we lie about the numbers, one can
> simply look at the video (or audio) stream and say something like "Well,
> they report it's 200px wide in this javascript property, but it's clearly
> 1080px wide"? 

Yes, that's what I was thinking.

> We don't have any mechanism to
> downscale video: Bug 1286945 tracks downscaling and decimating framerate for
> video. And resampling the audio is probably not a feature Firefox is going
> to pursue, if I was going to guess.

I guess it depends on how hard it is. Can we use AudioContext? Maybe the Tor Project will do it one day. :)
 
> Plus: Once the user has accepted a permission prompt to share their camera
> they probably realize that, hey, this could fingerprint me. Cause they're
> probably going to see my face. 

It depends, I think. If the user is using a rear-facing camera (away from user's face), then maybe not.

> Also: applying constraints to screensharing WILL result in a downscaling and
> decimating of frame rate, unlike video. "Unconstrained, you get the natural
> dimensions and frame rate of the screen/window." Exposing that data would be
> a fingerprinting vector.  BUT, exposing _anything_ via screen sharing (even
> if we lied about dimensions and frame rate) seems like a complete
> fingerprinting vector too, just like the camera. 

I think that also depends. What about sharing a fullscreen slide show?

> Because the items in Category 3 are all behind permission prompts, and the
> thing you're granting permission to is the biggest fingerprint vector of
> all, I think lying about the properties thus exposed can be a WONTFIX. If we
> wanted to, we could add an additional scary line in the permission prompts
> (when privacy.resistFingerprinting is enabled) that says something like
> "Granting this permission will allow [origin] to uniquely identify you."

I agree these are all difficult fixes and not high priority at this stage. And I also agree an additional scary line would be very valuable.

Comment 67

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1372073 - Spoof MediaTrackSettings.

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

Comment 68

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

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

Comment 69

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909665 [details]
Bug 1372073 - Ignore constraints in getUserMedia.

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

Comment 70

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909666 [details]
Bug 1372073 - Suppress devicechange event.

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

Comment 71

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8918163 [details]
Bug 1372073 - Spoof MediaStreamTrack.

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

Comment 72

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909667 [details]
Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

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

Comment 73

[Chung-Sheng Fu [:cfu]]

The new patch spoofs MediaTrackSettings attributes.  According to the discussion, facingMode, latency, and volume should be spoofed.  But I found an interesting fact that Firefox doesn't support latency and volume, so actually the patch only handles facingMode :p
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/latency#Browser_compatibility
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/volume#Browser_compatibility

Besides, if I understand correctly, we are not going to spoof audio and video properties in this phase, right?

(If the review gets granted, I will merge it to the "Bug 1372073 - Spoof MediaStreamTrack.label." patch and rename it to "Bug 1372073 - Spoof MediaStreamTrack.".)

Comment 74

[Arthur Edelstein [:arthur]]

Comment on attachment 8920459 [details]
Bug 1372073 - Spoof MediaTrackSettings.

https://reviewboard.mozilla.org/r/191452/#review197486

Comment 75

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review203444

Comment 76

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8909665 [details]
Bug 1372073 - Ignore constraints in getUserMedia.

https://reviewboard.mozilla.org/r/181180/#review203946

::: dom/media/MediaManager.cpp:2166
(Diff revision 4)
> +  // mediaSource is not set, regard it as boolean value.
> +  if (c.mMediaSource.EqualsLiteral("")) {

mediaSource defaults to "camera" [1] so it is always set, so this test is unnecessary and can be removed (the rare case that someone passes in "" doesn't require a special case, I don't think).

[1] https://searchfox.org/mozilla-central/rev/7fb4cc447c06f14fe3b5c6b0c9d103a860937250/dom/webidl/MediaStreamTrack.webidl#49

Comment 77

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8918163 [details]
Bug 1372073 - Spoof MediaStreamTrack.

https://reviewboard.mozilla.org/r/189020/#review203948

::: dom/media/VideoStreamTrack.cpp:39
(Diff revision 2)
>  
> +void
> +VideoStreamTrack::GetLabel(nsAString& aLabel, CallerType aCallerType)
> +{
> +  if (nsContentUtils::ResistFingerprinting(aCallerType)) {
> +    aLabel.AssignLiteral("Internal Camera");

Is it a requirement that these strings be common? From personal experience, "Internal Microphone" is the name of the built-in Macbook microphone, but I've never seen "Internal Camera" anywhere (for instance OSX says "FaceTime HD Camera" in contrast).

Comment 78

[Tom Ritter [:tjr] (OOTO until mid-August)]

(In reply to Jan-Ivar Bruaroey [:jib] (PTO Nov 9-12) from comment #77)
> Comment on attachment 8918163 [details]
> Bug 1372073 - Spoof MediaStreamTrack.label.
> 
> https://reviewboard.mozilla.org/r/189020/#review203948
> 
> ::: dom/media/VideoStreamTrack.cpp:39
> (Diff revision 2)
> >  
> > +void
> > +VideoStreamTrack::GetLabel(nsAString& aLabel, CallerType aCallerType)
> > +{
> > +  if (nsContentUtils::ResistFingerprinting(aCallerType)) {
> > +    aLabel.AssignLiteral("Internal Camera");
> 
> Is it a requirement that these strings be common? From personal experience,
> "Internal Microphone" is the name of the built-in Macbook microphone, but
> I've never seen "Internal Camera" anywhere (for instance OSX says "FaceTime
> HD Camera" in contrast).

I suggested 'Internal Camera' because it felt weird arbitrarily saying "We'll use an OSX default". (But maybe 'Internal Microphone' is also OSX only?)  Ideally we'd pick the most common name on the Internet.  But we probably have no way of knowing what that is, so unless anyone has a better idea yea I think switching it to 'FaceTime HD Camera' is good.

Comment 79

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8909667 [details]
Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

https://reviewboard.mozilla.org/r/181184/#review203950

Tests lgtm. Misc nits.

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:3
(Diff revision 4)
> +<!DOCTYPE html>
> +<meta charset="utf-8">
> +<title>Bug 1372073 - Neutralize the threat of fingerprinting of media devices API when 'privacy.resistFingerprinting' is true</title>

Nit: Is this to avoid including head.js? Unless there's a pressing reason, I'd prefer for maintenance reasons to use createHTML() for consistency with other tests here [1].

[1] https://searchfox.org/mozilla-central/rev/7fb4cc447c06f14fe3b5c6b0c9d103a860937250/dom/media/tests/mochitest/test_enumerateDevices.html#9

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:8
(Diff revision 4)
> +<title>Bug 1372073 - Neutralize the threat of fingerprinting of media devices API when 'privacy.resistFingerprinting' is true</title>
> +<script src="/tests/SimpleTest/SimpleTest.js"></script>
> +<script>
> +/* global SimpleTest SpecialPowers */
> +
> +async function testEnumerateDevices(noDevices) {

Nit: Negated booleans can be confusing. Maybe s/noDevices/expectDevices/ ?

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:12
(Diff revision 4)
> +  } catch (e) {
> +    SimpleTest.ok(false, "enumerateDevices failed: " + e);
> +    return;
> +  }

Nit: this catch replaces an exception with a return.

enumerateDevices()) failing is exceptional IMHO. I'd prefer a try/catch-free success-path here, letting exceptions be caught higher up instead.

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:20
(Diff revision 4)
> +  let count = devices.reduce((count, device) => {
> +    if (device.kind in count) {
> +      ++count[device.kind];
> +    }
> +    return count;
> +  }, {audioinput: 0, videoinput: 0});
> +  SimpleTest.ok((count.audioinput === 1) && (count.videoinput === 1),

I find this complicated and hard to read. How about:

    let cams = devices.filter(d => d.kind == "videoinput");
    let mics = devices.filter(d => d.kind == "audioinput");
    
    SimpleTest.ok(cams.length == 1 && mics.length == 1, "...");

?

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:26
(Diff revision 4)
> +    if (device.kind in count) {
> +      ++count[device.kind];
> +    }
> +    return count;
> +  }, {audioinput: 0, videoinput: 0});
> +  SimpleTest.ok((count.audioinput === 1) && (count.videoinput === 1),

In JavaScript, == is preferred to ===. [1]

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Operators

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:34
(Diff revision 4)
> +  for (let i = 0; i < constraints.length; ++i) {
> +    let constraint = constraints[i];

Prefer

    for (let constraint of constraints) {

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:46
(Diff revision 4)
> +        continue;
> +      }
> +
> +      // We only do testGetUserMedia(false) when privacy.resistFingerprinting
> +      // is true, test if MediaStreamTrack.label is spoofed.
> +      stream.getTracks().forEach((track) => {

We should avoid forEach in async functions because they work poorly with await (not a bug here but a maintenance hazard nonetheless). They're also hard to read, changing the semantics of embedded return statements. Use:

    for (let track of stream.getTracks()) {

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:49
(Diff revision 4)
> +      // We only do testGetUserMedia(false) when privacy.resistFingerprinting
> +      // is true, test if MediaStreamTrack.label is spoofed.
> +      stream.getTracks().forEach((track) => {
> +        switch (track.kind) {
> +          case "audio":
> +            return SimpleTest.is(track.label, "Internal Microphone", "AudioStreamTrack.label");

Don't forget to replace returns with breaks to avoid cutting tests short.

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:95
(Diff revision 4)
> +(async () => {
> +  SimpleTest.waitForExplicitFinish();
> +
> +  // Make sure enumerateDevices and getUserMedia work when
> +  // privacy.resistFingerprinting is true.
> +  await testDevices();
> +
> +  // Test that absence of devices can't be detected.
> +  await testNoDevices();
> +
> +  SimpleTest.finish();
> +})();

Since this isn't using runTest() or similar infrastructure, we should try/catch around all code in this outer-most async function, to avoid swallowing any future programming errors (Firefox isn't good at reporting unhandled rejections).

::: dom/media/tests/mochitest/test_fingerprinting_resistance.html:108
(Diff revision 4)
> +  await testNoDevices();
> +
> +  SimpleTest.finish();
> +})();
> +</script>
> +<a href="https://bugzilla.mozilla.org/show_bug.cgi?id=1372073">Bug 1372073 - Neutralize the threat of fingerprinting of media devices API when 'privacy.resistFingerprinting' is true</a>

Remove if you use createHTML()

Comment 80

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8920459 [details]
Bug 1372073 - Spoof MediaTrackSettings.

https://reviewboard.mozilla.org/r/191452/#review203970

Lgtm. You'll need a DOM reviewer on the webidl or this will bounce.

Comment 81

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

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

https://reviewboard.mozilla.org/r/177244/#review203974

Comment 82

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

Comment on attachment 8918163 [details]
Bug 1372073 - Spoof MediaStreamTrack.

https://reviewboard.mozilla.org/r/189020/#review203976

Comment 83

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

Comment on attachment 8920459 [details]
Bug 1372073 - Spoof MediaTrackSettings.

https://reviewboard.mozilla.org/r/191452/#review203980

Comment 84

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8905428 [details]
Bug 1372073 - Spoof navigator.mediaDevices.enumerateDevices.

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

Comment 85

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909665 [details]
Bug 1372073 - Ignore constraints in getUserMedia.

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

Comment 86

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909666 [details]
Bug 1372073 - Suppress devicechange event.

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

Comment 87

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8918163 [details]
Bug 1372073 - Spoof MediaStreamTrack.

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

Comment 88

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8909667 [details]
Bug 1372073 - Add tests for Media Capture and Streams fingerprinting resistance.

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

Comment 89

[Chung-Sheng Fu [:cfu]]

Thank you very much for your help :)

Comment 90

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/018f84af573b
Spoof navigator.mediaDevices.enumerateDevices. r=arthuredelstein,jib,smaug
https://hg.mozilla.org/integration/autoland/rev/f995ab0441e9
Ignore constraints in getUserMedia. r=arthuredelstein,jib
https://hg.mozilla.org/integration/autoland/rev/2133a8484ef0
Suppress devicechange event. r=arthuredelstein,mchiang
https://hg.mozilla.org/integration/autoland/rev/e04a7ec9c152
Spoof MediaStreamTrack. r=arthuredelstein,jib,smaug
https://hg.mozilla.org/integration/autoland/rev/3f811aa66cdb
Add tests for Media Capture and Streams fingerprinting resistance. r=arthuredelstein,jib

Comment 91

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/018f84af573b
https://hg.mozilla.org/mozilla-central/rev/f995ab0441e9
https://hg.mozilla.org/mozilla-central/rev/2133a8484ef0
https://hg.mozilla.org/mozilla-central/rev/e04a7ec9c152
https://hg.mozilla.org/mozilla-central/rev/3f811aa66cdb

Comment 92

[Ethan Tseng [:ethan]]

Another huge bug landed!  Good job, CS.
Thank you and all the reviewers.  :)