Closed Bug 1337161 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: cpeterson, Assigned: cfu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor][fingerprinting][fp:m2])

Attachments

(3 files, 2 obsolete files)

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
Per discussion with Ethan, SecEng team will take this. Mirroring the priority of the meta bug, i.e. P2
Priority: -- → P2
Whiteboard: [tor][fingerprinting]
Priority: P2 → P1
Whiteboard: [tor][fingerprinting] → [tor][fingerprinting][fp:m1]
Whiteboard: [tor][fingerprinting][fp:m1] → [tor][fingerprinting][fp:m2]
Assignee: nobody → cfu
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.
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
Attachment #8878287 - Flags: review?(bugs)
Attachment #8878287 - Flags: review?(arthuredelstein)
Attachment #8878966 - Flags: review?(bugs)
Attachment #8878966 - Flags: review?(arthuredelstein)
Attachment #8877958 - Attachment is obsolete: true
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
Attachment #8878287 - Flags: review?(bugs) → review+
Comment on attachment 8878966 [details]
Bug 1337161 - Add test case

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

rs+
Attachment #8878966 - Flags: review?(bugs) → review+
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
Attachment #8878287 - Flags: review?(arthuredelstein) → review+
Comment on attachment 8878966 [details]
Bug 1337161 - Add test case

https://reviewboard.mozilla.org/r/150214/#review156518
Attachment #8878966 - Flags: review?(arthuredelstein) → review+
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?
Flags: needinfo?(bugs)
I think qdot has looked at the gamepad implementation recently.
Flags: needinfo?(bugs) → needinfo?(kyle)
Looks ok to me, but forwarding to :lenzak who's done more work on this lately than I have.
Flags: needinfo?(kyle) → needinfo?(cleu)
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.
Attachment #8880299 - Flags: review+
Flags: needinfo?(cleu)
Thank you all, I will apply the patch with this bug.
Attachment #8881554 - Flags: review?(cleu)
I have to send a review request again through mozreview.
Sorry for your inconvenience.
Attachment #8880299 - Attachment is obsolete: true
Comment on attachment 8881554 [details]
Bug 1337161 - Fix leak of GamepadPlatformService

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

LGTM
Attachment #8881554 - Flags: review?(cleu) → review+
Keywords: checkin-needed
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
Keywords: checkin-needed
Attachment #8881554 - Flags: review?(jhao)
Comment on attachment 8881554 [details]
Bug 1337161 - Fix leak of GamepadPlatformService

https://reviewboard.mozilla.org/r/152732/#review158214
Attachment #8881554 - Flags: review?(jhao) → review+
Keywords: checkin-needed
Pushed by jhao@mozilla.com:
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
Keywords: checkin-needed
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)
Keywords: checkin-needed
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)
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)
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)
Glad to see another fingerprint bug being landed.
Good job!
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.
Flags: needinfo?(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!
Flags: needinfo?(cfu)
You need to log in before you can comment on or make changes to this bug.