Animation API exposes high-res time stamp

RESOLVED FIXED in Firefox 57

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: arthur, Assigned: timhuang)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

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

Attachments

(2 attachments)

Here's a patch from Tor Browser for possible uplift:
https://torpat.ch/16337
The original ticket is at
https://trac.torproject.org/16337
Oh right.
Should we reuse Performance::RoundTime() [1] somehow?

[1] https://hg.mozilla.org/mozilla-central/file/eb1d92b2b6a4/dom/performance/Performance.cpp#l264
I think we should extend the approach in Bug 1217238 to cover the Animation API.
The implementation is https://hg.mozilla.org/mozilla-central/rev/1a7356fac9ba
and regression tests are https://hg.mozilla.org/mozilla-central/rev/e967714a341a
Depends on: 1217238
Priority: -- → P2
Whiteboard: [tor 16337] → [tor 16337][fingerprinting][fp:m3]
Assignee: nobody → tihuang
Status: NEW → ASSIGNED
Comment on attachment 8899307 [details]
Bug 1382545 - Part 1: Rounding the time of Animation API to 100ms when 'privacy.resistFingerprinting'is true.

I think birtles should review this.
Attachment #8899307 - Flags: review?(bugs) → review?(bbirtles)
Attachment #8899308 - Flags: review?(bugs) → review?(bbirtles)
Comment on attachment 8899307 [details]
Bug 1382545 - Part 1: Rounding the time of Animation API to 100ms when 'privacy.resistFingerprinting'is true.

https://reviewboard.mozilla.org/r/170530/#review176174

There's a general invariant that when an animation is in play:

  current time = (timeline time - start time) × playback rate

It seems to me that with this patch that relationship might not hold and content that relies on that holding could suffer minor glitches.

Likewise, anything using those values for synchronization will not be able to do a very good job.

Then there's the fact that setting any of these values to a non-rounded value and then getting it back will give a different result which is a little counter-intuitive, could break content (unlikely) and makes it very obvious you've flipped this pref (not a problem I expect).

I'm not aware of the broader picture of fingerprinting here, what sort of attacks are foreseen, and what sort content breakage is acceptable, but I trust you are and assuming this is ok with you the code changes seem fine to me (except for the one minor comment below).

::: dom/animation/AnimationUtils.h:36
(Diff revision 1)
>    {
>      dom::Nullable<double> result;
>  
>      if (!aTime.IsNull()) {
> -      result.SetValue(aTime.Value().ToMilliseconds());
> +      result.SetValue(
> +        nsRFPService::ReduceTimePrecisionAsMSecs(aTime.Value().ToMilliseconds())

Any chance you could re-factor ReduceTimePrecisionAsMSecs so that the !IsResistFingerprintingEnabled() check is inline (so there's minimal overhead in the 99.9% case where that setting is not in-effect).

And the method should probably be renamed MaybeReduceTimePrecisionAsMSecs too.
Attachment #8899307 - Flags: review?(bbirtles) → review+
Comment on attachment 8899308 [details]
Bug 1382545 - Part 2: Add a test case for making sure that Animation API doesn't expose a high resolution time when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/170532/#review176176

::: browser/components/resistfingerprinting/test/mochitest/test_animation_api.html:17
(Diff revision 1)
> +
> +  /** Test for Bug 1382545 **/
> +  SimpleTest.waitForExplicitFinish();
> +
> +  window.onload = () => {
> +    SimpleTest.waitForFocus(() => {

Curious, any need to wait for focus? Or for 'load' for that matter?)

::: browser/components/resistfingerprinting/test/mochitest/test_animation_api.html:21
(Diff revision 1)
> +  window.onload = () => {
> +    SimpleTest.waitForFocus(() => {
> +      SpecialPowers.pushPrefEnv({"set":
> +        [
> +          ["privacy.resistFingerprinting", true],
> +          ["dom.animations-api.core.enabled", true]

I'm not totally sure this works. I think for prefs that affect the Element interface, we need to tweak them before creating any Element instances (see bug 1159743 comment 4). Previously we've had to work around this using a separate opener file although as of bug 1328830 you can set the pref in the mochitest.ini but only for all tests in manifest (which probably doesn't make sense here).

::: browser/components/resistfingerprinting/test/mochitest/test_animation_api.html:28
(Diff revision 1)
> +    let isRounded = x => (Math.floor(x/100)*100) === x;
> +    let testDiv = document.getElementById("testDiv");
> +    let animation = testDiv.animate({ opacity: [0,1] }, 100000);

(nit: s/let/const/)

::: browser/components/resistfingerprinting/test/mochitest/test_animation_api.html:33
(Diff revision 1)
> +    SimpleTest.waitForCondition(
> +      () => animation.currentTime > 1000,
> +        function () {

1s seems like a long time to wait. It means this test adds 1s to every test run ever. Can we make this a little shorter?

::: browser/components/resistfingerprinting/test/mochitest/test_animation_api.html:54
(Diff revision 1)
> +  }
> +
> +  </script>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=">Mozilla Bug </a>

Not sure you need this line, but if you do you probably want to add some bug numbers to it.
Attachment #8899308 - Flags: review?(bbirtles) → review+
Comment on attachment 8899307 [details]
Bug 1382545 - Part 1: Rounding the time of Animation API to 100ms when 'privacy.resistFingerprinting'is true.

https://reviewboard.mozilla.org/r/170530/#review177136
Attachment #8899307 - Flags: review?(arthuredelstein) → review+
Comment on attachment 8899308 [details]
Bug 1382545 - Part 2: Add a test case for making sure that Animation API doesn't expose a high resolution time when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/170532/#review177140

r+, pending the issues identified by Brian.
Attachment #8899308 - Flags: review?(arthuredelstein) → review+
(In reply to Brian Birtles (:birtles) from comment #6)
> 
> Any chance you could re-factor ReduceTimePrecisionAsMSecs so that the
> !IsResistFingerprintingEnabled() check is inline (so there's minimal
> overhead in the 99.9% case where that setting is not in-effect).
> 
> And the method should probably be renamed MaybeReduceTimePrecisionAsMSecs
> too.

Sure, I will open a follow-up bug for this, thanks.
Comment on attachment 8899308 [details]
Bug 1382545 - Part 2: Add a test case for making sure that Animation API doesn't expose a high resolution time when 'privacy.resistFingerprinting' is true.

https://reviewboard.mozilla.org/r/170532/#review176176

> Curious, any need to wait for focus? Or for 'load' for that matter?)

It is not really necessary here, so I remove this.
See Also: → 1393662
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e2213726a3d3
Part 1: Rounding the time of Animation API to 100ms when 'privacy.resistFingerprinting'is true. r=arthuredelstein,birtles
https://hg.mozilla.org/integration/autoland/rev/ad3bc2177e89
Part 2: Add a test case for making sure that Animation API doesn't expose a high resolution time when 'privacy.resistFingerprinting' is true. r=arthuredelstein,birtles
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e2213726a3d3
https://hg.mozilla.org/mozilla-central/rev/ad3bc2177e89
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1424341
You need to log in before you can comment on or make changes to this bug.