1382545 - Animation API exposes high-res time stamp

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P2)

Description

[Arthur Edelstein [:arthur]]

Here's a patch from Tor Browser for possible uplift:
https://torpat.ch/16337
The original ticket is at
https://trac.torproject.org/16337

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Oh right.
Should we reuse Performance::RoundTime() [1] somehow?

[1] https://hg.mozilla.org/mozilla-central/file/eb1d92b2b6a4/dom/performance/Performance.cpp#l264

Comment 2

[Arthur Edelstein [:arthur]]

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

Comment 3

[Tim Huang[:timhuang]]

Attachment: Bug 1382545 - Part 1: Rounding the time of Animation API to 100ms when 'privacy.resistFingerprinting'is true.

Adopt from Tor #16337.

This patch makes Animation API to report a rounded time when 'privacy.resistFingerprinting'
is true. The Animation API uses AnimationUtils::TimeDurationToDouble() to convert
its time duration into a double value and reports it when someone tries to query
time through Animation API. So, we use nsRFPService::ReduceTimePrecisionAsMSecs()
inside this method to round the time in the scope of the millisecond.

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

Comment 4

[Tim Huang[:timhuang]]

Attachment: 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.

Adopt from Tor #16337

This patch adds a test case for testing that Animation API doesn't expose a high
resolution time when 'privacy.resistFingerprinting' is true. The test case will
play an animation and wait a certain amount of time, one second, and then verify
the animation.startTime, animation.currentTime, animation.timeline.currentTime
and document.timeline.currentTime are all rounded with 100ms.

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

Comment 5

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

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.

Comment 6

[Brian Birtles (:birtles)]

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.

Comment 7

[Brian Birtles (:birtles)]

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.

Comment 8

[Arthur Edelstein [:arthur]]

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

Comment 9

[Arthur Edelstein [:arthur]]

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.

Comment 10

[Tim Huang[:timhuang]]

(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 11

[Tim Huang[:timhuang]]

Comment on attachment 8899307 [details]
Bug 1382545 - Part 1: Rounding the time of Animation API to 100ms when 'privacy.resistFingerprinting'is true.

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

Comment 12

[Tim Huang[:timhuang]]

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.

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

Comment 13

[Tim Huang[:timhuang]]

Comment on attachment 8899307 [details]
Bug 1382545 - Part 1: Rounding the time of Animation API to 100ms when 'privacy.resistFingerprinting'is true.

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

Comment 14

[Tim Huang[:timhuang]]

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.

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

Comment 15

[Tim Huang[:timhuang]]

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.

Comment 16

[Tim Huang[:timhuang]]

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.

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

Comment 17

[Tim Huang[:timhuang]]

Try looks fine.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb557db98cee

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e2213726a3d3
https://hg.mozilla.org/mozilla-central/rev/ad3bc2177e89