Preference to disable video statistics

RESOLVED FIXED in mozilla25

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
3 months ago

People

(Reporter: curtisk, Assigned: leonard.beck)

Tracking

(Blocks: 1 bug, {privacy})

unspecified
mozilla25
privacy
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tor] [fingerprinting])

Attachments

(1 attachment, 5 obsolete attachments)

Comment hidden (empty)
Need a pref in about:config to disable video stats to reduce fingerprinting threat
Assignee: nobody → chris
Blocks: 580531
Keywords: privacy
The framerate and other information contains information about the user's machine's capabilities; users who want to be more anonymous will want to turn off entropy sources like this (and so will privacy-enhancing add-ons like torbutton) -- adding an about:config preference to turn it off enables this.
Summary: Preferance to disable video statistics → Preference to disable video statistics
Can you tell me more about how this is an actual fingerprinting threat?

The data we have for statistics is basically frames parsed/decoded/presented/painted. Frames aren't painted if the video is not visible. But then the person who wanted to fingerprint could just use the Page Visibility API. Are we blocking that too?
Flags: needinfo?(sstamm)
(Assignee)

Comment 4

4 years ago
Created attachment 760941 [details] [diff] [review]
Not finalized! Pref added, 0 returned when enabled, display TODO

Problem in /toolkit/content/widgets/videocontrols.xml for reading the pref value.
Attachment #760941 - Flags: review?(paul)
Attachment #760941 - Flags: review?(jaws)
Comment on attachment 760941 [details] [diff] [review]
Not finalized! Pref added, 0 returned when enabled, display TODO

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

::: content/html/content/src/HTMLVideoElement.cpp
@@ +160,5 @@
>  
>  uint32_t HTMLVideoElement::MozParsedFrames() const
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "Should be on main thread.");
> +  if (Preferences::GetBool("media.video_stats.disabled", false)) {

Checking this pref on each request will result in pretty poor performance. It would be better to cache the value of the pref.
Attachment #760941 - Flags: review?(jaws) → review-
I should note that I don't think this bug is a worthwhile pursuit until I hear more from Sid or Curtis about why this is an actual threat.
(In reply to Jared Wein [:jaws] from comment #3)
> Can you tell me more about how this is an actual fingerprinting threat?

To expand upon comment 2:  The stats in themselves are not a fingerprint, but they contribute to a fingerprint.  As we deploy new features, we should also enable add-ons and privacy-conscious users to reduce the number of features that can be used as a constellation of entropy sources to fingerprint them.

> But then the person who wanted to fingerprint could just use the
> Page Visibility API. Are we blocking that too?

I'm not asking for us to "block" video stats.  We should just create a pref for entropy-minimizing add-ons like Torbutton (and privacy-conscious individuals) to disable this source of entropy if they want.  We should probably have some (perhaps hidden) pref control the page visibility API too.

Mainly, the goal here is to reinforce our User Control privacy principle:
https://wiki.mozilla.org/Privacy/Principles#User_Control
Flags: needinfo?(sstamm)
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #7)
> (In reply to Jared Wein [:jaws] from comment #3)
> > Can you tell me more about how this is an actual fingerprinting threat?
> 
> To expand upon comment 2:  The stats in themselves are not a fingerprint,
> but they contribute to a fingerprint.  As we deploy new features, we should
> also enable add-ons and privacy-conscious users to reduce the number of
> features that can be used as a constellation of entropy sources to
> fingerprint them.
> 
> > But then the person who wanted to fingerprint could just use the
> > Page Visibility API. Are we blocking that too?
> 
> I'm not asking for us to "block" video stats.  We should just create a pref
> for entropy-minimizing add-ons like Torbutton (and privacy-conscious
> individuals) to disable this source of entropy if they want.  We should
> probably have some (perhaps hidden) pref control the page visibility API too.
> 
> Mainly, the goal here is to reinforce our User Control privacy principle:
> https://wiki.mozilla.org/Privacy/Principles#User_Control

I understand the goal here, but we have to be practical about it too.

There are probably many other APIs that we have that are getting more usage of in fingerprinting circles and we could getter better outcomes out of.
Also, we should not be striving to make Firefox the next Tor. In fact, Torbutton is deprecated and they recommend using their patched version of Firefox.

Would it be better to submit a patch to the Tor repo that completely disables these statistics?
(In reply to Jared Wein [:jaws] from comment #8)
> I understand the goal here, but we have to be practical about it too.

Sure.  What are the trade-offs?  How much engineering time do we anticipate spending on implementing this?  What's the expected perf overhead of a cached pref?  If it's seemingly low-impact and low-effort, lets spend the energy on implementation and not discussion.

> There are probably many other APIs that we have that are getting more usage
> of in fingerprinting circles and we could getter better outcomes out of.

Probably, and we should enable controls for those too.

re: tor... I agree, but I would like to see us help them by implementing these controls in the main line.  Tor is not the only consumer of privacy settings...
Should it be a separate pref for each type of privacy control, or a single pref that the user can flip?

If we go for separate prefs, then a user can get more fine-grained control. If we go for a single pref, then add-ons won't need to be updated when we want to guard another API using the same preference.
I like the idea of a one-size-fits-all pref, but due to the variety of entropy sources it's probably not actually one-size-fits all; I can imagine one add-on wanting a subset disabled but not all of 'em.

My opinion is that the onus is on the privacy tool implementation developers to decide which features they want and which they don't; if we do it for 'em, we will likely mess it up.

So perhaps the best way forward is to go for separate prefs now and then revisit as a longer-term effort to distill sets of prefs into one.
Ok, then let's go with separate prefs. But please update the patch to cache the pref value.
(Assignee)

Comment 14

4 years ago
We're on it.

Patch with cache attached in a couple of hours.

Our other question still remains: we can't read the pref value in videocontrols.xml (error: permission denied to access property 'classes'). Any idea? (See comment #241 in the current patch.)
You won't be able to read preferences from within videocontrols.xml since it runs with the same privileges as page content. Showing 0 here should be sufficient.
(Assignee)

Comment 16

4 years ago
Created attachment 761726 [details] [diff] [review]
Pref added, 0 returned if enabled, pref value cached
Attachment #760941 - Attachment is obsolete: true
Attachment #760941 - Flags: review?(paul)
Attachment #761726 - Flags: review?(paul)
Attachment #761726 - Flags: review?(jaws)
Comment on attachment 761726 [details] [diff] [review]
Pref added, 0 returned if enabled, pref value cached

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

Make sure to change the commit message to something like:

Bug xxxxxx - Add a preference to disable media statistics. r=reviewers

::: content/html/content/src/HTMLVideoElement.cpp
@@ +39,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  
> +static bool mVideoStatsDisabled;

|mVideoStatsDisabled| (with a "m" prefix) is how we call class members.

Please call this |sVideoStatsDisabled| instead.

@@ +75,5 @@
>  HTMLVideoElement::HTMLVideoElement(already_AddRefed<nsINodeInfo> aNodeInfo)
>    : HTMLMediaElement(aNodeInfo)
>  {
>    SetIsDOMBinding();
> +  Preferences::AddBoolVarCache(&mVideoStatsDisabled, "media.video_stats.disabled");

Actually, we want this done only once. Create a static HTMLVideoElement::Init static method, in which you register the pref.

Then call the HTMLVideoElement::Init method from LayoutStatics.cpp.

@@ +164,5 @@
>  uint32_t HTMLVideoElement::MozParsedFrames() const
>  {
>    MOZ_ASSERT(NS_IsMainThread(), "Should be on main thread.");
> +  if (mVideoStatsDisabled)
> +    return 0;

Braces even on single if statement, please, and other occurrences below.

::: content/media/test/test_bug654550.html
@@ +15,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +  /* Test for Bug 654550 */
> +  

Remove trailing spaces in this file.

@@ +18,5 @@
> +  /* Test for Bug 654550 */
> +  
> +  var manager = new MediaTestManager;
> +
> +  function statsDisabled(video) {

Maybe this could be called |checkStatsDisabled|.

@@ +30,5 @@
> +  }
> +  
> +  function onplay(event) {
> +    var v = event.target;
> +    SpecialPowers.setBoolPref("media.video.stats_disabled", false);

You should make sure that time has actually advanced before checking that, so that we had the chance to paint/decode/etc. at least one frame. One way would be to do something along the lines of: 

>  function ontimeupate(e) {
>   var t = e.target;
>   if (t.currentTime == 0) {
>     return;
>   }
>   statsDisabled(t);
>   //disable pref, and do the other stuff.
>   document.removeEventListener("timeupdate", ontimeupdate);
> });
>
> document.addEventListener("timeupdate", ontimeupdate);
Attachment #761726 - Flags: review?(paul) → feedback+
(In reply to Paul Adenot (:padenot) from comment #17)
> @@ +30,5 @@
> > +  }
> > +  
> > +  function onplay(event) {
> > +    var v = event.target;
> > +    SpecialPowers.setBoolPref("media.video.stats_disabled", false);
> 
> You should make sure that time has actually advanced before checking that,
> so that we had the chance to paint/decode/etc. at least one frame. One way
> would be to do something along the lines of: 

Using the MozAfterPaint event might be better.
Assignee: cpearce → leonard.beck
Status: NEW → ASSIGNED
Comment on attachment 761726 [details] [diff] [review]
Pref added, 0 returned if enabled, pref value cached

Clearing review until updated patch is posted.
Attachment #761726 - Flags: review?(jaws)
(Assignee)

Comment 20

4 years ago
Created attachment 762072 [details] [diff] [review]
Patch updated: cache, test, coding style

Wi did'nt manage to use |MozAfterPaint| but it seems to work with |timeupdate|.
Test only on video format supporting video stats (.ogv and .webm).
Attachment #761726 - Attachment is obsolete: true
Attachment #762072 - Flags: review?(paul)
Attachment #762072 - Flags: review?(jaws)
Comment on attachment 762072 [details] [diff] [review]
Patch updated: cache, test, coding style

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

Looks real good so far.

::: content/media/test/test_bug654550.html
@@ +18,5 @@
> +  /* Test for Bug 654550 */
> +
> +  var manager = new MediaTestManager;
> +
> +  //SpecialPowers.setBoolPref("dom.send_after_paint_to_content", true);

This line can be deleted.

@@ +24,5 @@
> +  function checkStatsDisabled(video) {
> +    if ((video.mozParsedFrames == 0)
> +        && (video.mozDecodedFrames == 0)
> +        && (video.mozPresentedFrames == 0)
> +        && (video.mozPaintedFrames == 0)) {

Replace these with calls to ok() such that if one of them fails we will know which one failed.

You can do it like so:
function checkStats(aVideo, aShouldBeEnabled) {
  is(!!video.mozParsedFrames, aShouldBeEnabled, "mozParsedFrames should be 0 if it is disabled");
  ...
}

@@ +36,5 @@
> +    if (SpecialPowers.getBoolPref("media.video_stats.disabled")) {
> +      ok(checkStatsDisabled(v), "All stats should be equal to 0");
> +      SpecialPowers.setBoolPref("media.video_stats.disabled", false);
> +    } else {
> +      var v = event.target;

This line shouldn't be necessary.

::: modules/libpref/src/init/all.js
@@ +222,5 @@
>  // MediaDecoderReader's mVideoQueue.
>  pref("media.video-queue.default-size", 10);
>  
> +// Whether to disable the video stats to prevent fingerprinting
> +pref("media.video_stats.disabled", false);

I would prefer the pref name be in the affirmative as it's usually easier to read negations of it. Please change it to:
pref("media.video_stats.enabled", true);
Attachment #762072 - Flags: review?(jaws) → feedback+
(Assignee)

Comment 22

4 years ago
Created attachment 762168 [details] [diff] [review]
Patch updated: pref cached, test modified, coding style

Test slightly modified to catch the value of the interesting stats.
Ancient version running on test servers.
Attachment #762072 - Attachment is obsolete: true
Attachment #762072 - Flags: review?(paul)
Attachment #762168 - Flags: review?
Comment on attachment 762168 [details] [diff] [review]
Patch updated: pref cached, test modified, coding style

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

Forwarding the review to Paul.

::: content/media/test/test_bug654550.html
@@ +33,5 @@
> +        "mozPresentedFrames should be 0 if stats are disabled");
> +      ok(!v.mozPaintedFrames,
> +        "mozPaintedFrames should be 0 if stats are disabled");
> +    }
> +      

nit: please remove the trailing whitespace here and on line 28.
Attachment #762168 - Flags: review?(paul)
Attachment #762168 - Flags: review?
Attachment #762168 - Flags: feedback+
(Assignee)

Comment 24

4 years ago
Created attachment 762221 [details] [diff] [review]
Update: coding style
Attachment #762168 - Attachment is obsolete: true
Attachment #762168 - Flags: review?(paul)
Attachment #762221 - Flags: review?(paul)
Attachment #762221 - Flags: review?(jaws)
Comment on attachment 762221 [details] [diff] [review]
Update: coding style

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

Looks fine to me, just make the one change below. No need to re-request review from me.

::: content/media/test/test_bug654550.html
@@ +22,5 @@
> +  function checkStats(v, aShouldBeEnabled) {
> +    if (aShouldBeEnabled) {
> +      ok(v.mozParsedFrames + v.mozDecodedFrames +
> +         v.mozPresentedFrames + v.mozPaintedFrames != 0,
> +         "At least one value should be different from 0 if stats are enabled");

These values are all related. A frame can't be painted if it's not presented. A frame can't be presented if it's not decoded. A frame can't be decoded if it's not parsed. Thus, you should only need to check mozParsedFrames. See http://blog.pearce.org.nz/2011/03/html5-video-painting-performance.html for more background on this.
Attachment #762221 - Flags: review?(jaws) → review+

Comment 26

4 years ago
Created attachment 762336 [details] [diff] [review]
Patch updated

Patch corrected
Attachment #762336 - Flags: review?(paul)

Updated

4 years ago
Attachment #762221 - Attachment is obsolete: true
Attachment #762221 - Flags: review?(paul)

Updated

4 years ago
Attachment #762336 - Flags: review?(paul) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8710fcb40a32
https://hg.mozilla.org/mozilla-central/rev/8710fcb40a32
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

3 months ago
Blocks: 1329996
Whiteboard: [tor] [fingerprinting]

Updated

3 months ago
See Also: → bug 1362186
You need to log in before you can comment on or make changes to this bug.