1337161 - Disable navigator.getGamepads() when privacy.resistFingerprinting = true

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Chris Peterson [:cpeterson]]

The navigator.getGamepads() API [1] can reveal unique hardware IDs [2] for connected game controllers. We should disable the "dom.gamepad.enabled" pref when the "privacy.resistFingerprinting" pref is enabled.

[1] https://developer.mozilla.org/en-US/docs/Web/API/Navigator/getGamepads
[2] https://developer.mozilla.org/en-US/docs/Web/API/Gamepad/id

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Per discussion with Ethan, SecEng team will take this. Mirroring the priority of the meta bug, i.e. P2

Comment 2

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-Hide-information-of-connected-gamepads-from-content-.patch

I'm trying to hide information of connected gamepads from content when the pref privacy.resistFingerprinting is true.
As I understand,
1. navigator.getGamepads() should not expose any information, so returns an empty array
2. gamepadconnected and gamepaddisconnected should not be fired
3. conditions related to the pref dom.gamepad.enabled should also check privacy.resistFingerprinting
Does the solution seem reasonable?

The most serious concern is, will the change break any web compatibility?
We need your expertise to help analyze the risk.
Thanks.

Comment 3

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

well, pages requiring use of gamepads won't work. Most users probably don't have gamepads anyhow.

I guess we still want to expose the interfaces to the web pages so that APIs look the same as without
privacy.resistFingerprinting, so the approach the patch has taken looks reasonable.

Comment 4

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true

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

Comment 5

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1337161 - Add test case

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

Comment 6

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

Comment on attachment 8878287 [details]
Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true

https://reviewboard.mozilla.org/r/149612/#review155036

Comment 7

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

Comment on attachment 8878966 [details]
Bug 1337161 - Add test case

https://reviewboard.mozilla.org/r/150214/#review155038

rs+

Comment 8

[Arthur Edelstein [:arthur]]

Comment on attachment 8878287 [details]
Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true

https://reviewboard.mozilla.org/r/149612/#review156516

Comment 9

[Arthur Edelstein [:arthur]]

Comment on attachment 8878966 [details]
Bug 1337161 - Add test case

https://reviewboard.mozilla.org/r/150214/#review156518

Comment 10

[Chung-Sheng Fu [:cfu]]

Attachment: 0001-GamepadPlatformService-singleton-cannot-be-released.patch

My test causes a leak issue, which is not related to this bug and is reproducible without the patch.

The leak can be observed by setting dom.gamepad.enabled = false and dom.gamepad.test.enabled = true, and call navigator.requestGamepadServiceTest().addGamepad.

The symptom is that the GamepadPlatformService singleton (http://searchfox.org/mozilla-central/source/dom/gamepad/GamepadPlatformService.cpp#27) cannot be released, which means the function GamepadPlatformService::MaybeShutdown (http://searchfox.org/mozilla-central/source/dom/gamepad/GamepadPlatformService.cpp#259) is not called.
It should be called for each GamepadManager::mChannelChildren (http://searchfox.org/mozilla-central/source/dom/gamepad/GamepadManager.cpp#110).
Elements are appended to GamepadManager::mChannelChildren in GamepadManager::ActorCreated, which is called by GamepadManager::AddListener (http://searchfox.org/mozilla-central/source/dom/gamepad/GamepadManager.cpp#159), but when dom.gamepad.enabled = false, GamepadManager::AddListener will never reach the logic creating IPDL child, so GamepadManager::mChannelChildren is always an empty array and there is no chance to release the GamepadPlatformService singleton.

The attachment is how I try to solve the problem.
In GamepadManager::AddListener, the logic creating IPDL child seems not related to the listeners so I moved it to the beginning of the function.
Because there is always a IPDL child, GamepadManager::Update will be called if gamepad state changes, like calling navigator.requestGamepadServiceTest().addGamepad.
But when dom.gamepad.enabled = false, GamepadManager::mListeners is always an empty array and assertion (http://searchfox.org/mozilla-central/source/dom/gamepad/GamepadManager.cpp#524) will be triggered.
So I also modified the condition at the beginning of GamepadManager::Update to avoid the assertion.

Does the solution look fine?
Should I also apply the patch for this bug?

Comment 11

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

I think qdot has looked at the gamepad implementation recently.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Looks ok to me, but forwarding to :lenzak who's done more work on this lately than I have.

Comment 13

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8880299 [details] [diff] [review]
0001-GamepadPlatformService-singleton-cannot-be-released.patch

Review of attachment 8880299 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, too.

Comment 14

[Chung-Sheng Fu [:cfu]]

Thank you all, I will apply the patch with this bug.

Comment 15

[Chung-Sheng Fu [:cfu]]

Attachment: Bug 1337161 - Fix leak of GamepadPlatformService

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

Comment 16

[Chung-Sheng Fu [:cfu]]

I have to send a review request again through mozreview.
Sorry for your inconvenience.

Comment 17

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8878287 [details]
Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true

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

Comment 18

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8878966 [details]
Bug 1337161 - Add test case

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

Comment 19

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8881554 [details]
Bug 1337161 - Fix leak of GamepadPlatformService

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

Comment 20

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8881554 [details]
Bug 1337161 - Fix leak of GamepadPlatformService

https://reviewboard.mozilla.org/r/152732/#review157948

LGTM

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until it meets the MozReview review requirements.
http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/autoland.html#landing-commits

Comment 22

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8881554 [details]
Bug 1337161 - Fix leak of GamepadPlatformService

https://reviewboard.mozilla.org/r/152732/#review158212

Comment 23

[Jonathan Hao (inactive) [:jhao]]

Comment on attachment 8881554 [details]
Bug 1337161 - Fix leak of GamepadPlatformService

https://reviewboard.mozilla.org/r/152732/#review158214

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/41a148dc837d
Hide information of connected gamepads from content when privacy.resistFingerprinting is true r=arthuredelstein,smaug
https://hg.mozilla.org/integration/autoland/rev/5f871d332791
Add test case r=arthuredelstein,smaug
https://hg.mozilla.org/integration/autoland/rev/97043840f1ed
Fix leak of GamepadPlatformService r=jhao,Lenzak

Comment 25

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 69b392d4c0fa -d 7e56bf782a5c: rebasing 404437:69b392d4c0fa "Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true r=arthuredelstein,smaug"
merging dom/gamepad/GamepadManager.cpp
warning: conflicts while merging dom/gamepad/GamepadManager.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 26

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8878287 [details]
Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true

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

Comment 27

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8878966 [details]
Bug 1337161 - Add test case

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

Comment 28

[Chung-Sheng Fu [:cfu]]

Comment on attachment 8881554 [details]
Bug 1337161 - Fix leak of GamepadPlatformService

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

Comment 29

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fa0132948f98 -d cf4987e9de05: rebasing 404443:fa0132948f98 "Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true r=arthuredelstein,smaug"
merging dom/gamepad/GamepadManager.cpp
warning: conflicts while merging dom/gamepad/GamepadManager.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 30

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fa0132948f98 -d ddcdefe1d0b4: rebasing 404551:fa0132948f98 "Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true r=arthuredelstein,smaug"
merging dom/gamepad/GamepadManager.cpp
warning: conflicts while merging dom/gamepad/GamepadManager.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 31

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s fa0132948f98 -d 6f02ce7cabf5: rebasing 404562:fa0132948f98 "Bug 1337161 - Hide information of connected gamepads from content when privacy.resistFingerprinting is true r=arthuredelstein,smaug"
merging dom/gamepad/GamepadManager.cpp
warning: conflicts while merging dom/gamepad/GamepadManager.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/41a148dc837d
https://hg.mozilla.org/mozilla-central/rev/5f871d332791
https://hg.mozilla.org/mozilla-central/rev/97043840f1ed

Comment 33

[Ethan Tseng [:ethan]]

Glad to see another fingerprint bug being landed.
Good job!

Comment 34

[Ciprian Georgiu, Desktop QA]

Hi :cfu,

We want to test this bug for the anti-fingerprinting pre-Release sign off. At this moment we have 2 gamepads available (Xbox 360 and a F310 Logitech).

If you can suggest us steps to reproduce/expected results here in order to manual test this, or other scenarios that we can cover around this fix, would be very helpful.

Comment 35

[Chung-Sheng Fu [:cfu]]

(In reply to Ciprian Georgiu, QA [:ciprian_georgiu] from comment #34)
> Hi :cfu,
> 
> We want to test this bug for the anti-fingerprinting pre-Release sign off.
> At this moment we have 2 gamepads available (Xbox 360 and a F310 Logitech).
> 
> If you can suggest us steps to reproduce/expected results here in order to
> manual test this, or other scenarios that we can cover around this fix,
> would be very helpful.

Thank you very much for helping with this. Here are some steps that I may suggest:

1. Set privacy.resistFingerprinting to false for some preparation.

2. Register gamepadconnected and gamepaddisconnected events. You can try the following scripts

     window.addEventListener("gamepadconnected", () => { console.log("gamepadconnected") });
     window.addEventListener("gamepaddisconnected", () => { console.log("gamepaddisconnected") });

3. You may want to check the normal behavior for comparison, so
   a. plug gamepad, and gamepadconnected event should be fired
      if you ran the scripts above, console shows "gamepadconnected"
   b. call navigator.getGamepads(), and it should return an non-empty array
   c. remove gamepad, and gamepaddisconnected event should be fired
      if you ran the scripts above, console shows "gamepaddisconnected"

4. Set privacy.resistFingerprinting to true.

5. Now the gamepad APIs should not work
   a. plug gamepad, and gamepadconnected event should not be fired
   b. navigator.getGamepads() should always return empty array
   c. remove gamepad, and gamepaddisconnected event should not be fired

Please ni? me if there is still something unclear, thanks!