Closed Bug 1281949 Opened 4 years ago Closed 4 years ago

screen.orientation should be spoofed when privacy.resistFingerprinting is enabled (Tor 18958)

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: arthur, Assigned: arthur)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tor][fingerprinting] btpp-active)

Attachments

(1 file, 1 obsolete file)

From https://trac.torproject.org/18958:

Firefox 43 introduced a new orientation API that includes the unprefixed screen.orientation property and possibly support for onchange events. See:

    ​https://developer.mozilla.org/en-US/docs/Web/API/Screen/orientationhttps://w3c.github.io/screen-orientation/

The implementation of the new API did not carry forward the concept of respecting the privacy.resistFingerprinting pref, which was implemented in bug 418986.
Here's the patch, rebased from the Tor Browser patch, that spoofs 'screen.orientation', blocks 'orientationchange' and 'mozorientationchange' events, and blocks the use of 'screen.mozLockOrientation' and 'screen.mozUnlockOrientation' when the pref 'privacy.resistFingerprinting' is true.

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c28500ad90e&selectedJob=22827546
Attachment #8764817 - Flags: review?(mrbkap)
Comment on attachment 8764817 [details] [diff] [review]
0001-Bug-1281949-When-resisting-fingerprinting-spoof-scre.patch

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

Nits only.

::: dom/base/ScreenOrientation.cpp
@@ +589,5 @@
> +ScreenOrientation::ShouldResistFingerprinting() const
> +{
> +  bool resist = false;
> +  nsCOMPtr<nsPIDOMWindowInner> owner = GetOwner();
> +  if (owner) {

I'm a fan of the

if (nsCOMPtr<nsPIDOMWindowInner> owner = GetOwner()) {
  ...owner...
}

style.

::: dom/base/nsGlobalWindow.cpp
@@ +13243,5 @@
>  nsGlobalWindow::EnableOrientationChangeListener()
>  {
>    MOZ_ASSERT(IsInnerWindow());
> +  if (!nsContentUtils::ShouldResistFingerprinting(mDocShell)
> +      && !mOrientationChangeObserver) {

Nit: In DOM, we tend to do:

if (aReallyLongCondition &&
    !bSuperDuperLongCondition) {
  ...
}
Attachment #8764817 - Flags: review?(mrbkap) → review+
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #2)

> Nits only.

Thank you for the quick review! Here's the new version with nits fixed.
Attachment #8764817 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [tor] → [tor] btpp-active
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a493fd7137
When resisting fingerprinting, spoof screen.orientation
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/26a493fd7137
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: meta_tor
Summary: screen.orientation should be spoofed when privacy.resistFingerprinting is enabled → screen.orientation should be spoofed when privacy.resistFingerprinting is enabled (Tor 18958)
Whiteboard: [tor] btpp-active → [tor][fingerprinting] btpp-active
Does
 nsGlobalWindow::EnableOrientationChangeListener()
 {
   MOZ_ASSERT(IsInnerWindow());
-
-  if (!mOrientationChangeObserver) {
+  if (!nsContentUtils::ShouldResistFingerprinting(mDocShell) &&
+      !mOrientationChangeObserver) {
actually work?
Is mDocShell set there? Isn't that set only on outer windows.
Flags: needinfo?(arthuredelstein)
(In reply to Olli Pettay [:smaug] from comment #6)
> Does
>  nsGlobalWindow::EnableOrientationChangeListener()
>  {
>    MOZ_ASSERT(IsInnerWindow());
> -
> -  if (!mOrientationChangeObserver) {
> +  if (!nsContentUtils::ShouldResistFingerprinting(mDocShell) &&
> +      !mOrientationChangeObserver) {
> actually work?
> Is mDocShell set there? Isn't that set only on outer windows.

Thanks for pointing this out. I opened bug 1433815 to work on fixing this.
Flags: needinfo?(arthuredelstein)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.