418986 - window.screen and CSS media queries provide a large amount of identifiable information (Tor 2875)

Status: RESOLVED FIXED

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

Description

[Mike Perry]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.12) Gecko/20080201 Firefox/2.0.0.12

The window.screen object can be used to build an identifier that can be used to track users independent of IP. Based on my rough calculations, there should be at least 29 bits of extractable state from desktop geometry, toolbar geometry, window size, etc.

Currently, there is no way to obscure this information without injecting javascript into the contentWindow to hook the screen object. It would be nice if the browser provided a pref to say "only report my content window dimensions as inner and outer height." This pref would cause the screen object to behave as if the contentWindow was the size of the entire desktop, with no chrome or desktop toolbar size information.

Or, alternatively, if window.screen was implemented as an XPCOM contract component that could be reimplemented by extensions without having to revert to injecting javascript into pages.

Reproducible: Always

Comment 1

[Mike Connor [:mconnor]]

This would be Core, not Firefox

Comment 2

[Mike Perry]

CSS media queries are another way to get this information. Given this and potential perf issues, perhaps it is best implemented as a pref rather than a component.

Note that Peter Eckersley has better quantified the amount of identifying information coming from resolution information in the panopticlick project. However, he did not use current window resolution or window widget or decoration size information at all, so there are likely more bits of identifying info here than he reports. http://panopticlick.eff.org/

Comment 3

[Asa Dotzler [:asa]]

This was just fixed in Webkit https://bugs.webkit.org/show_bug.cgi?id=41802

Comment 4

[JonDonym]

I am not sure whether this is allowed here (if not, just delete this post): 

We would pay 300 Euro for someone fixing this bug and putting it in the official Mozilla code. And another 300 Euro for fixing it with CSS media queries. Just contact me for further questions and discussions about the implementation.

Comment 5

[Mike Perry]

Due to CSS Media Queries, this should be handled at a lower level. Here's the Tor Project's ticket for creating a patch for a lower-level way to handle this: https://trac.torproject.org/projects/tor/ticket/2875

I think this bug should probably be either re-titled or closed.

Comment 6

[Teo]

(In reply to Mike Perry from comment #5)
> Due to CSS Media Queries, this should be handled at a lower level. Here's
> the Tor Project's ticket for creating a patch for a lower-level way to
> handle this: https://trac.torproject.org/projects/tor/ticket/2875
> 
> I think this bug should probably be either re-titled or closed.

You can re-title the bug by editing the summary.

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: Limit the information revealed by css device (media)

This adds a preference to replace the information retrieved by css media queries from the device iformation to the window information on non-chrome contexts.

I dont like the name of the preference so I am accepting suggestions.

Comment 8

[Camilo Viecco (:cviecco)]

Attachment: test cases

Test cases to preference to limit the css media preference

Comment 9

[Camilo Viecco (:cviecco)]

Attachment: test cases

Fixes issues with names of test. Now actually passes mochitest when running in detached screen mode.

Comment 10

[Camilo Viecco (:cviecco)]

Attachment: a bitroted patch (for external contributors)

Comment 11

[Tomas Touceda]

I've been working on this issue for a bit, and I have a couple of thoughts I would like to discuss:

This bug has two parts:
1) window.screen.* gives a lot of information regarding sizes which helps an attacker fingerprint a user
2) even if we fix 1) there are still css media queries that can help do the same as 1)

I've rebased Camilo's patch to the latest mozilla-central, but I'm not entirely sure it's the best approach to the matter. In my laptop with a screen res of 1366x768, window.screen.width/height/availWidth/availHeight are set to 1920x1080. Which is good to solve 1), but is it needed? 1366x768 isn't a rare resolution, so may be there is no real gain of rounding to the next most common resolution. If every value is the same, or it's just a standard res, then we reduce fingerprinting a lot already.
Either way, I've changed the test to check that the window.screen.* values are normalized (regardless of the method) after setting the corresponding config value.

Regarding 2), I don't think the test is valid, since the query detects my screen resolution correctly (so the test fails), but it still doesn't said much about me as one can say about window.screen.width-window.screen.availWidth.
I'm not a CSS superhacker, so I'm more than likely doing it wrong, but I couldn't find a way to get more than just my standard screen resolution. So is there anything to normalize in this case?

Comment 12

[Camilo Viecco (:cviecco)]

Tomas:

(In reply to Tomas Touceda from comment #11)
> I've been working on this issue for a bit, and I have a couple of thoughts I
> would like to discuss:
> 
> This bug has two parts:
> 1) window.screen.* gives a lot of information regarding sizes which helps an
> attacker fingerprint a user
> 2) even if we fix 1) there are still css media queries that can help do the
> same as 1)
> 
> I've rebased Camilo's patch to the latest mozilla-central, but I'm not
> entirely sure it's the best approach to the matter. In my laptop with a
> screen res of 1366x768, window.screen.width/height/availWidth/availHeight
> are set to 1920x1080. Which is good to solve 1), but is it needed? 1366x768
> isn't a rare resolution, so may be there is no real gain of rounding to the
> next most common resolution. If every value is the same, or it's just a
> standard res, then we reduce fingerprinting a lot already.

The goal of the patch is to reduce the fingerprint. By having a very small set of buckets there is minimal information a fingerprinting agent can gather (no more than 2 bits). As compared with the average 4.89 bits on pantoptclick. (For example, given than your resolution is 1366x768 and you are a coder I am pretty sure you are not using a mac laptop)*

> Either way, I've changed the test to check that the window.screen.* values
> are normalized (regardless of the method) after setting the corresponding
> config value.
Excellent.
> 
> Regarding 2), I don't think the test is valid, since the query detects my
> screen resolution correctly (so the test fails), but it still doesn't said
> much about me as one can say about
> window.screen.width-window.screen.availWidth.
> I'm not a CSS superhacker, so I'm more than likely doing it wrong, but I
> couldn't find a way to get more than just my standard screen resolution. So
> is there anything to normalize in this case?
The code originally was able to do this, but there was a change and it is not working as expected any more. This because I was modifying the functions that were called by CSS AND js calls.


*Only 11' macbook airs have that resolution and those boxes are pretty weak for ff devel.

Comment 13

[Tomas Touceda]

Attachment: Add preference to normalize window.* values

Comment 14

[Tomas Touceda]

I've attached an alternative approach to this issue. It only handles window.screen.* and certain window.* values, not CSS queries, since I think they should be handle similarly to this but in a different bug. For the same reason I've changed the config var name, since the one from the previous patch doesn't really apply anymore.

The screen size is rounded to the next >= resolution value based on the real one that is a multiple of 200x100. This multiple is the same used in Tor Browser, so I thought it was better to keep both of them similar in this sense.

Comment 15

[Georg Koppen]

(In reply to Tomas Touceda from comment #14)
> This multiple is the same used in Tor
> Browser, so I thought it was better to keep both of them similar in this
> sense.
Why? Just using it because the Tor Browser uses it seems not a good argument to me. The Tor folks could easily drop their patches and use the solution in this bug if it is good (they would be glad, I think). Thus, there is no need to harmonize the patching unless resizing the window to a multiple of 200x100 is the best approach. I am not sure about that yet. See: https://trac.torproject.org/projects/tor/ticket/7256
That said, how do you cope with fullscreen mode? See the above ticket as well or https://trac.torproject.org/projects/tor/ticket/7222#comment:3. I fear 90% of the users are using fullscreen mode out of a habit and all of your rounding efforts are in vain.

Comment 16

[Georg Koppen]

(In reply to Georg Koppen from comment #15)
> (In reply to Tomas Touceda from comment #14)
> That said, how do you cope with fullscreen mode?
Err, I meant maximizing the window.

Comment 17

[Georg Koppen]

(In reply to Georg Koppen from comment #15)
> (In reply to Tomas Touceda from comment #14)
> I fear 90% of the users are using fullscreen mode out of a habit and all of
> your rounding efforts are in vain.
That should be "I fear 90% of the users are maximizing their windows out of a habit and all of
your rounding efforts are in vain."

Comment 18

[Tomas Touceda]

(In reply to Georg Koppen from comment #15)
> (In reply to Tomas Touceda from comment #14)
> > This multiple is the same used in Tor
> > Browser, so I thought it was better to keep both of them similar in this
> > sense.
> Why? Just using it because the Tor Browser uses it seems not a good argument
> to me. The Tor folks could easily drop their patches and use the solution in
> this bug if it is good (they would be glad, I think). Thus, there is no need
> to harmonize the patching unless resizing the window to a multiple of
> 200x100 is the best approach. 

True, but it seems 200x100 works quite good based on what I've seen in terms of fingerprinting. I don't really care about the numbers, as long as they are good.

> I am not sure about that yet. See:
> https://trac.torproject.org/projects/tor/ticket/7256
> That said, how do you cope with fullscreen mode? See the above ticket as
> well or https://trac.torproject.org/projects/tor/ticket/7222#comment:3. I
> fear 90% of the users are using fullscreen mode out of a habit and all of
> your rounding efforts are in vain.

I don't see any issues with a maximized browser and this patch. If you change the zoom, the size changes but they are still a multiple of 200x100. Panopticlick results are quite good too.
Could you provide a more concrete example of this problem?

Comment 19

[Georg Koppen]

(In reply to Tomas Touceda from comment #18)
> (In reply to Georg Koppen from comment #15)
> > (In reply to Tomas Touceda from comment #14)
> > > This multiple is the same used in Tor
> > > Browser, so I thought it was better to keep both of them similar in this
> > > sense.
> > Why? Just using it because the Tor Browser uses it seems not a good argument
> > to me. The Tor folks could easily drop their patches and use the solution in
> > this bug if it is good (they would be glad, I think). Thus, there is no need
> > to harmonize the patching unless resizing the window to a multiple of
> > 200x100 is the best approach. 
> 
> True, but it seems 200x100 works quite good based on what I've seen in terms
> of fingerprinting. I don't really care about the numbers, as long as they
> are good.
> 
> > I am not sure about that yet. See:
> > https://trac.torproject.org/projects/tor/ticket/7256
> > That said, how do you cope with fullscreen mode? See the above ticket as
> > well or https://trac.torproject.org/projects/tor/ticket/7222#comment:3. I
> > fear 90% of the users are using fullscreen mode out of a habit and all of
> > your rounding efforts are in vain.
> 
> I don't see any issues with a maximized browser and this patch.
Ah, yes. I did not look closely enough at the code, sorry.
> If you change the zoom, the size changes but they are still a multiple of 200x100.
Not for me. Have a look at the Screen and Browser window rows on http://ip-check.info after you applied zoom and reloaded the test.

What I am wondering as well: If I open a new profile and do not resize the window and start the test on http://ip-check.info I get 994x838 pixels, 994x716 pixels (inner size), Zoom: 100% in the Browser window row (the difference is due to the notification bar that complains about missing plugins which might be a separate issue, dunno). But on the screen row I get 1600 x 900 pixels which is, rather unintuitive. Shouldn't it be more like 1000 x 900 pixels?

Comment 20

[Tomas Touceda]

(In reply to Georg Koppen from comment #19)
> Not for me. Have a look at the Screen and Browser window rows on
> http://ip-check.info after you applied zoom and reloaded the test.
> 
> What I am wondering as well: If I open a new profile and do not resize the
> window and start the test on http://ip-check.info I get 994x838 pixels,
> 994x716 pixels (inner size), Zoom: 100% in the Browser window row (the
> difference is due to the notification bar that complains about missing
> plugins which might be a separate issue, dunno). But on the screen row I get
> 1600 x 900 pixels which is, rather unintuitive. Shouldn't it be more like
> 1000 x 900 pixels?

Why 1000x900 is better than 1600x900? The way the patch sets sizes is meant to keep aspect ratio the same for every case.

I'm not sure how ip-check.info gets those values, may be css media queries? I checked http://eb0b428b.byethost7.com/screen.html and it isn't able to get the same values as ip-check. It would be good to understand what it does to get those values.

Comment 21

[Georg Koppen]

(In reply to Georg Koppen from comment #19)
> (In reply to Tomas Touceda from comment #18)
> > (In reply to Georg Koppen from comment #15)
> > > (In reply to Tomas Touceda from comment #14)
> > If you change the zoom, the size changes but they are still a multiple of 200x100.
> Not for me. Have a look at the Screen and Browser window rows on
> http://ip-check.info after you applied zoom and reloaded the test.
> 
> What I am wondering as well: If I open a new profile and do not resize the
> window and start the test on http://ip-check.info I get 994x838 pixels,
> 994x716 pixels (inner size), Zoom: 100% in the Browser window row (the
> difference is due to the notification bar that complains about missing
> plugins which might be a separate issue, dunno). But on the screen row I get
> 1600 x 900 pixels which is, rather unintuitive. Shouldn't it be more like
> 1000 x 900 pixels?
Forget that. This was another error on my side, *sigh*

Comment 22

[Georg Koppen]

(In reply to Tomas Touceda from comment #20)
> (In reply to Georg Koppen from comment #19)
> > Not for me. Have a look at the Screen and Browser window rows on
> > http://ip-check.info after you applied zoom and reloaded the test.
> > 
> > What I am wondering as well: If I open a new profile and do not resize the
> > window and start the test on http://ip-check.info I get 994x838 pixels,
> > 994x716 pixels (inner size), Zoom: 100% in the Browser window row (the
> > difference is due to the notification bar that complains about missing
> > plugins which might be a separate issue, dunno). But on the screen row I get
> > 1600 x 900 pixels which is, rather unintuitive. Shouldn't it be more like
> > 1000 x 900 pixels?
> 
> Why 1000x900 is better than 1600x900? The way the patch sets sizes is meant
> to keep aspect ratio the same for every case.
Because if the faked screen size is not tied to the current window size somehow an attacker might make correct conclusions about a user having their browser maximized or not. Taking my above values an attacker can be pretty sure that I had not had my browser window maximized (which is correct). It would be good IMO to hide this information as well.
> I'm not sure how ip-check.info gets those values, may be css media queries?
It has a CSS media queries test, yes. But that comes only into play if JavaScript is deactivated IIRC.
> I checked http://eb0b428b.byethost7.com/screen.html and it isn't able to get
> the same values as ip-check. It would be good to understand what it does to
> get those values.
I'll look at that on Monday.

Comment 23

[Georg Koppen]

Another thing I am thinking about is whether replacing outerWidth and outerHeight with innerWidth and innerHeight without rounding them as well is the best strategy in the long run (it is better than the current status quo, though). These faked window values could still be used to help separate users as not much people are probably surfing with, say 994x838 as innerWidth and innerHeight.

The alternatives here are (there a more, of course, e.g. just rounding outer and inner window values and not replacing one group with the other):

1) Current status: an attacker knows the outer and inner window values (which cold be quite uncommon itself)
2) Your idea: an attacker would only know the inner window values (which could be quite uncommon itself)
3) Your idea + rounding: an attacker would neither know the inner nor the outer values AND users with, say 994x838, would have a much larger chance to be in a bucket with other users.

Comment 24

[Tomas Touceda]

(In reply to Georg Koppen from comment #22)
> (In reply to Tomas Touceda from comment #20)
> > (In reply to Georg Koppen from comment #19)
> > > Not for me. Have a look at the Screen and Browser window rows on
> > > http://ip-check.info after you applied zoom and reloaded the test.
> > > 
> > > What I am wondering as well: If I open a new profile and do not resize the
> > > window and start the test on http://ip-check.info I get 994x838 pixels,
> > > 994x716 pixels (inner size), Zoom: 100% in the Browser window row (the
> > > difference is due to the notification bar that complains about missing
> > > plugins which might be a separate issue, dunno). But on the screen row I get
> > > 1600 x 900 pixels which is, rather unintuitive. Shouldn't it be more like
> > > 1000 x 900 pixels?
> > 
> > Why 1000x900 is better than 1600x900? The way the patch sets sizes is meant
> > to keep aspect ratio the same for every case.
> Because if the faked screen size is not tied to the current window size
> somehow an attacker might make correct conclusions about a user having their
> browser maximized or not. Taking my above values an attacker can be pretty
> sure that I had not had my browser window maximized (which is correct). It
> would be good IMO to hide this information as well.

I see your point, but with this patch, it would appear that the user has her browser *always* maximized, which is as good I think. But as I said about the 200x100 multiple, I don't care about the numbers as long as they are good. Since I think both cases are as good, I will wait until there is more consensus on yours before actually changing it.

> > I'm not sure how ip-check.info gets those values, may be css media queries?
> It has a CSS media queries test, yes. But that comes only into play if
> JavaScript is deactivated IIRC.

Right, I think we should bear in mind that there a lot of fingerprintable parts, and we are tackling one at a time. I'm sure this works for window.screen.* and certain window.* values, if there is another way, we should analyze it separately unless it involves the same code components as this case.

> > I checked http://eb0b428b.byethost7.com/screen.html and it isn't able to get
> > the same values as ip-check. It would be good to understand what it does to
> > get those values.
> I'll look at that on Monday.

Great!

Comment 25

[Tomas Touceda]

(In reply to Georg Koppen from comment #23)
> Another thing I am thinking about is whether replacing outerWidth and
> outerHeight with innerWidth and innerHeight without rounding them as well is
> the best strategy in the long run (it is better than the current status quo,
> though). These faked window values could still be used to help separate
> users as not much people are probably surfing with, say 994x838 as
> innerWidth and innerHeight.
> 

Have you seen http://eb0b428b.byethost7.com/screen.html or tried to print the values that this patch is trying to normalize? I'm replacing inner with outer, not the other way around. Outer equals to screen values, which are rounded to a 200x100 multiple.
I think the 994x838 that we are all getting from ip-check is something other than what we are trying to normalize in here. I'm not saying that we shouldn't fix that too, but before I understand what it's doing, I can't judge whether that belongs here or in a different bug and a different code component.

Comment 26

[Georg Koppen]

(In reply to Tomas Touceda from comment #24)
> > I'll look at that on Monday.
> 
> Great!

Research is ongoing.

Comment 27

[Camilo Viecco (:cviecco)]

Tomas:

I think the real name of the bug should be: it is possible to get the screen dimension information from firefox. Here is my opinion:
1. The patch should address both css and js leaks.
2. The patch should not modify the window information (that is another bug).
3. Whatever resolution is 'faked' to the requesting entity it should match well known screen dimensions (increasing the anonymity set). (the tor method I do not like, not only the sizes are not existent (even if it was flawless, it can only mix with other tor users), but there are no real screens with 2:1 ratio).
4. The patch must also protect from full screen detection (window.fullScreen).
5. Maybe disable fullscreen mode? (F11)

Why not address the window size now?
1. It can be easily computed from the elements in the page.
2. There are legitimate uses of window.* such as webpages designed to maximize the use of the space (http://snapp2.bldc.grnoc.iu.edu/futuregrid/ ).

Here is my opinion on the patch.
1. The proposed patch tries to prevent sreen leakage and window leakage, conflating two different issues.
2. Does not cover all the screen information leaks.
3. breaks sites that use the window size (not the screen size).
4. Does not solve the css leaks.

I have to pages to demo the some of the issues:
https://people.mozilla.com/~cviecco/tests/check_screen_size.html -> checks the js issues.
https://people.mozilla.com/~cviecco/tests/screen_test_css.html  -> checks the css issues.

Thanks for your patch, but I think the approach is inappropriate.

(also) using panoptclick as an oracle for the completeness of any approach is not such a good idea.

Comment 28

[Jesse Ruderman]

You can't realistically censor the viewport dimensions (e.g. innerWidth).  In addition to CSS media queries, there are numerous tricks available to scripts: computed styles, scroll heights, scroll positions (after forcing various things to be scrolled into view), and watching cursor events over a period of time.

If you want viewport-dimension privacy, you have to actually use a common viewport dimension.

Censoring screen dimensions (along with the other information inexplicably available to http://howtallaremytoolbars.com/) is more feasible.  You'll want to censor the screenX property of mouse events, preferably by making it equal to clientX.

Comment 29

[Jesse Ruderman]

Fixing CSS and JS should probably be separate patches, because they will need separate review[er]s and separate tests.  A single bug can be used for a queue of patches, though.

Comment 30

[Tomas Touceda]

(In reply to Camilo Viecco (:cviecco) from comment #27)
> Tomas:
> 
> I think the real name of the bug should be: it is possible to get the screen
> dimension information from firefox. Here is my opinion:
> 1. The patch should address both css and js leaks.
> 2. The patch should not modify the window information (that is another bug).
> 3. Whatever resolution is 'faked' to the requesting entity it should match
> well known screen dimensions (increasing the anonymity set). (the tor method
> I do not like, not only the sizes are not existent (even if it was flawless,
> it can only mix with other tor users), but there are no real screens with
> 2:1 ratio).

Ok, I'm guessing the values you used are the ones you prefer, so I'll use those.

> 4. The patch must also protect from full screen detection
> (window.fullScreen).
> 5. Maybe disable fullscreen mode? (F11)

So the patch should take care of fullscreen but nothing else? otherwise, it contradicts point 2.

> 
> Why not address the window size now?
> 1. It can be easily computed from the elements in the page.
> 2. There are legitimate uses of window.* such as webpages designed to
> maximize the use of the space (http://snapp2.bldc.grnoc.iu.edu/futuregrid/ ).

Ok. Although, the values provided by window.{inner,outer}{Width,Height} and that combined with window.screen give a lot of information. But I think if we fake the screen size properly, then we are just left with window.innerHeight - window.outerHeight giving information about the window layout, which gives things like toolbar/menu/etc sizes and can help fingerprint. Now I understand why the TorBrowser sets outer{Width,Height} to inner{Width,Height} and not the other way around.

I'll open another ticket for this.

> 
> Here is my opinion on the patch.
> 1. The proposed patch tries to prevent sreen leakage and window leakage,
> conflating two different issues.
> 2. Does not cover all the screen information leaks.
> 3. breaks sites that use the window size (not the screen size).
> 4. Does not solve the css leaks.

Are 2 and 4 the same? If no, could you give me an example of 2?

> 
> I have to pages to demo the some of the issues:
> https://people.mozilla.com/~cviecco/tests/check_screen_size.html -> checks
> the js issues.
> https://people.mozilla.com/~cviecco/tests/screen_test_css.html  -> checks
> the css issues.
> 
> Thanks for your patch, but I think the approach is inappropriate.
> 

Ok, thanks for the thorough review, I'll improve my patch and re post it.

> (also) using panoptclick as an oracle for the completeness of any approach
> is not such a good idea.

Agreed, I used panopticlick to see how it classified the resolution according to what it has seen, and the results were pretty good (although I agree with the 2:1 ratio being weird, may be it was just luck, not sure).

Comment 31

[Georg Koppen]

(In reply to Georg Koppen from comment #26)
> (In reply to Tomas Touceda from comment #24)
> > > I'll look at that on Monday.
> > 
> > Great!
> 
> Research is ongoing.

The result is the CSS fallback if the test detects something weird (like innerWidth + outerWidth being the same size and innerHeight + outerHeight being the same size).

Comment 32

[Camilo Viecco (:cviecco)]

(In reply to Tomas Touceda from comment #30)
> (In reply to Camilo Viecco (:cviecco) from comment #27)
> > Tomas:
> > 
> Ok, I'm guessing the values you used are the ones you prefer, so I'll use
> those.
Or any set (small) that matches real world screens. 
> 
> > 4. The patch must also protect from full screen detection
> > (window.fullScreen).
> > 5. Maybe disable fullscreen mode? (F11)
> 
> So the patch should take care of fullscreen but nothing else? otherwise, it
> contradicts point 2.
I have not given a lot of tought to fullscreen mode. But having it (and being able to detect it) would undercut many of the things we have done. I do not have currently a good answer to this.
Currently my view (from less than a few hours of tought) is that if we are going to hide the screen size we must do is prevent going into fullscreenmode.

 

> 
> > 
> > Here is my opinion on the patch.
> > 1. The proposed patch tries to prevent sreen leakage and window leakage,
> > conflating two different issues.
> > 2. Does not cover all the screen information leaks.
> > 3. breaks sites that use the window size (not the screen size).
> > 4. Does not solve the css leaks.
> 
> Are 2 and 4 the same? If no, could you give me an example of 2?
No, example for 2:  the patch did not addressed the screenX and screenY event properties.

Comment 33

[Camilo Viecco (:cviecco)]

Attachment: another revision. Covers events, css and window.screen..

Still need to check where I am getting my pref is on the main thread (there are no actual race conditions).
CSS is getting the window size, not the faked screen size, which is a bummer.

Comment 34

[Camilo Viecco (:cviecco)]

Attachment: another revision. Covers events, css and window.screen..

this actually compiles (forgot to qrefresh).
Covers: window.screen events and css:
issues: css reports the window not the faked screen
still misses window.screen*

Comment 35

[Mike Perry]

Attachment: Tor-Browser-resolution-fingerprinting-fixes.diff

This is the combination of two patches in Tor Browser git repository. Together, they cause the device resolution to be reported as the content window, remove window offset information, and report all absolute mouse events relative to the content window instead of to the desktop. It does not handle touch events, as those are not yet supported in the 24ESR series.

This patch is against Firefox 24ESR, and was primarily written by Kathleen Brade and Mark Smith.

Comment 36

[Sid Stamm [:geekboy or :sstamm]]

Mike: any chance you have a newer patch than the 24ESR patch mentioned in comment 35?

Comment 37

[Georg Koppen]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #36)
> Mike: any chance you have a newer patch than the 24ESR patch mentioned in
> comment 35?

We can easily attach essentially the same patch against ESR 31. Is that the thing you had in mind when you wrote "newer"? Or would something else be (even) more helpful?

Comment 38

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Georg Koppen from comment #37)
> We can easily attach essentially the same patch against ESR 31.

How different is it?  Is it a trivial merge?  If it's not trivial, please upload a new patch here so we have "fresh" code.

Comment 39

[Arthur Edelstein [:arthur]]

The latest version of the patches from Tor Browser (ESR31-based) are here:

TBESR31 #5856: Do not expose physical screen info via window & window.screen.
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.0-1&id=89d06c9ea0cbfc683e9e69e9d6d191ecd4f82321

Regression tests for #5856: Do not expose physical screen info via window & window.screen.
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.0-1&id=8f59d6422835d62b339dc6b8a089c9ca36b487dd

TBESR31 #2875: Limit device and system specific CSS Media Queries.
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.0-1&id=4b136a7f7576aede5a46226e132027ea8b9f7067

Regression tests for #2875: Limit device and system specific CSS Media Queries.
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.0-1&id=1810a7a00ae918e69b7e1f50df37cf35758e6506

Also, closely related (in terms of leaking absolute device coordinates)

TBESR31 #4755: Return client window coordinates for mouse event screenX/Y (for dragend, 0,0 is returned).
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.0-1&id=eadce6b069ebbe52af47b59e42db55a15593f4f6

Regression tests for #4755: Return client window coordinates for mouse event screenX/Y (for dragend, 0,0 is returned).
https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-31.4.0esr-4.0-1&id=3840f153c7702f369594fb5ad51c369f2ef56356

These patches are not yet bound to a pref.

Comment 40

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

Comment 41

[Arthur Edelstein [:arthur]]

(In reply to Arthur Edelstein from comment #40)
> Created attachment 8572189 [details] [diff] [review]
> 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

This patch includes Tor Browser's three original patches with regression tests, modified so that the feature is controlled by a "privacy.resistFingerprinting" pref. Updated for mozilla-central.

Comment 42

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

I've corrected an error and refactored the patch a little.

Try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=adb1394b533a

(I'm not sure who is the right person to ask for a review -- please feel free to re-assign the request to someone else.)

Comment 43

[Sid Stamm [:geekboy or :sstamm]]

Arthur: Sorry for the delay here.  I'm happy to take a first pass before finding others to review the code.  We can work out some kinks before review.

Are *all* of the patches in this bug relevant/required for a fix, or just the one you flagged for review?

Comment 44

[Arthur Edelstein [:arthur]]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #43)
> Arthur: Sorry for the delay here.  I'm happy to take a first pass before
> finding others to review the code.  We can work out some kinks before review.
> 
> Are *all* of the patches in this bug relevant/required for a fix, or just
> the one you flagged for review?

Sid: Thanks! Only the patch I flagged for review is required for the fix.

Comment 45

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8572938 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

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

Steve, can you take a look at this and see what we can do to adopt an approach like this?

Comment 46

[Arthur Edelstein [:arthur]]

Sid, Steve: Is there anything I can do to help with this patch at this point?

Comment 47

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8572938 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

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

Arthur - apologies on the delay here. I've added some really basic cleanup comments, and I'm going to forward the review to mrbkap, since he is more of an expert in this code than I.

mrbkap, can you take a look and see if it's ok to land this patch? The changes should all be behind a pref.

::: dom/base/nsGlobalWindow.cpp
@@ +5168,5 @@
>  {
>    FORWARD_TO_OUTER_OR_THROW(GetMozInnerScreenX, (aError), aError, 0);
>  
> +  // When resisting fingerprinting, always return 0.
> +  if (nsContentUtils::ShouldResistFingerprinting()) return 0.0;

Style nit: Please wrap in braces and put on a new line.

if () {
  return 0.0;
}

::: dom/events/Event.cpp
@@ +889,5 @@
> +    // causes GetClientX() to return zero).  Since dragend is for the drag
> +    // originator and not for the receiver, it is probably not widely used
> +    // (receivers get a drop event).  Therefore, returning 0 should not break
> +    // many web pages.  Also, a few years ago Firefox returned 0.
> +    // See:  https://bugzilla.mozilla.org/show_bug.cgi?id=466379

Can you review this comment, please? It looks like it could be more concise.

Also, the last part - "a few years ago Firefox returned 0" - if you mention this, I think you should also explain why we decided not to return 0 and why it's ok to return it now. There are many things we have stopped doing because they're bugs and they break things ;) We don't unfix them and say it's ok because we used to do it :) Just a bit more explanation is needed.

Comment 48

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

Steve -- thanks for the useful review comments. I have cleaned up those two areas, and rebased the patch on top of mozilla-central.

Comment 49

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8588216 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

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

You'll have to get another reviewer for the layout/style bits. I have a few questions that need addressing before I can r+ this.

::: dom/base/nsGlobalWindow.cpp
@@ +5171,5 @@
>  {
>    FORWARD_TO_OUTER_OR_THROW(GetMozInnerScreenY, (aError), aError, 0);
>  
> +  // Return 0 to prevent fingerprinting.
> +  if (nsContentUtils::ShouldResistFingerprinting()) return 0.0;

Nit: please use the full

if (...) {
  return 0.0;
}

style here.

::: dom/base/nsGlobalWindow.h
@@ +601,4 @@
>      return mIsChrome;
>    }
>  
> +  bool IsResistingFingerprinting() const;

This doesn't appear to be implemented anywhere.

::: dom/base/nsScreen.cpp
@@ +67,5 @@
>  int32_t
>  nsScreen::GetPixelDepth(ErrorResult& aRv)
>  {
> +  // Return 24 to prevent fingerprinting.
> +  if (nsContentUtils::ShouldResistFingerprinting()) return 24;

Same nits here and below: please use the full style with braces for these if statements.

@@ +399,5 @@
> +  return NS_OK;
> +}
> +
> +nsresult
> +nsScreen::GetWindowInnerRect(nsRect& aRect)

I would think that you could write this as:

nsPIDOMWindow *win = GetOwner();
if (!owner) ...

auto *globalWin = static_cast<nsGlobalWindow *>(owner);
owner->GetInnerSize(...);
// convert and return as nsRect.

Am I missing something?

::: dom/base/nsScreen.h
@@ +130,4 @@
>    nsDeviceContext* GetDeviceContext();
>    nsresult GetRect(nsRect& aRect);
>    nsresult GetAvailRect(nsRect& aRect);
> +  nsresult GetDOMWindow(nsIDOMWindow **aResult);

Is there any reason to have this in a function? It looks like it's only used once.

::: dom/base/test/test_bug418986-1.html
@@ +38,5 @@
> +    {'set': [["privacy.resistFingerprinting", true]]},
> +    function () {
> +      // Check each of the pairs.
> +      pairs.map(checkPair);
> +      SpecialPowers.popPrefEnv(function () { SimpleTest.finish(); });

No need to call popPrefEnv yourself. The harness takes care of that for you. So just call SimpleTest.finish() directly.

::: dom/events/test/test_bug418986-3.html
@@ +47,5 @@
> +    }
> +  }
> +
> +  function test1 () {
> +    SimpleTest.waitForExplicitFinish();

Does this work? I'd expect onload to be fired, causing the test to finish before we call this. I think this needs to be called at top-level before onload.

That also means that you need a SimpleTest.finish() at the end of test2.

::: layout/style/test/test_bug418986-2.html
@@ +73,5 @@
> +      windows_versions.map(function (val) {
> +        foundOSVersion = foundOSVersion || keyValMatches(["-moz-os-version", val]);
> +      });
> +      ok(!foundOSVersion, "-moz-os-version should have no match");
> +      SpecialPowers.popPrefEnv(function () { SimpleTest.finish(); });

Same comment as in the other test here (explicit popPrefEnv not needed).

Comment 50

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #49)
> Comment on attachment 8588216 [details] [diff] [review]
> 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch
> 
> Review of attachment 8588216 [details] [diff] [review]:

Thanks for the review!
 
> That also means that you need a SimpleTest.finish() at the end of test2.

The SimpleTest.finish() call is in the final callback of test2.

Apart from that, I believe I've followed all of your other recommendations.

I have also added couple of extra lines to spoof the devicePixelRatio to 1, when the pref is enabled.

Comment 51

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

(fixing typo)

Comment 52

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8590077 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

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

The DOM parts look good. Thanks.

Comment 53

[Arthur Edelstein [:arthur]]

(Review request to :heycam is for the layout/style/* parts.)

Comment 54

[Cameron McCormack (:heycam)]

Comment on attachment 8590077 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

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

Sorry for the review delay.  I'd like to see another version of the patch with the comments below addressed.

Can you please add a few more tests:

1. variants of the tests that you already have that ensure that spoofed values aren't used when we're in a chrome document
2. some tests for the media features as used in an @media rule (in addition to the window.matchMedia calls you have), in both content and chrome documents

I think that nsContentUtils::IsCallerChrome will only do what you want here if you have JS on the stack.  When @media rules are being matched, it's going to always return true (or false? not sure).  So you probably want to check the pres context's IsChrome() somewhere to make sure that, for example, the @media rules within chrome style sheets still work to correctly style the browser chrome.

::: layout/style/nsMediaFeatures.cpp
@@ +319,5 @@
>  static nsresult
>  GetSystemMetric(nsPresContext* aPresContext, const nsMediaFeature* aFeature,
>                  nsCSSValue& aResult)
>  {
> +    aResult.Reset();

Can you add a comment to say that we do not expose any system metric backed media feature when fingerprint resisting is enabled (as opposed to supporting them but with a fixed value)?

@@ +337,4 @@
>  {
>      aResult.Reset();
>  #ifdef XP_WIN
> +    if (!nsContentUtils::ShouldResistFingerprinting()) {

Just use an early return like you do in GetOperatinSystemVersion rather than sticking all of this inside the if statement.  Or, make GetOperatinSystemVersion follow this function's style.  Either way.

@@ +363,4 @@
>                           nsCSSValue& aResult)
>  {
>      aResult.Reset();
> +    if (nsContentUtils::ShouldResistFingerprinting()) return NS_OK;

return statement on a new line and within braces, please.

Comment 55

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

Here's a new version with the requested test variants and corrections as suggested in the previous comment. I have also added similar test variants to the DOM parts, and I made some changes to that part to make it more robust to future code changes, so I'll be requesting another review for that part as well.

Comment 56

[Arthur Edelstein [:arthur]]

Comment on attachment 8606011 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

I'm requesting a review again, since I added some tests and made some changes to the already-approved dom/* code.

Comment 57

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8606011 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

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

I have a question, but if we decide to add a pref cache, that can happen in a followup bug, no need to redo the patch here.

::: dom/base/nsContentUtils.cpp
@@ +1997,5 @@
> +  if (!docShell) {
> +    return false;
> +  }
> +  bool isChrome = nsContentUtils::IsChromeDoc(docShell->GetDocument());
> +  return !isChrome && Preferences::GetBool("privacy.resistFingerprinting", false);

Sorry for not asking earlier, but is there any reason not to add a pref cache here (as we do for e.g. dom.url.encode_decode_hash)?

Comment 58

[Cameron McCormack (:heycam)]

Comment on attachment 8606011 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

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

r=me on the layout/ parts and the nsContentUtils change, with comments below addressed.

::: dom/base/nsContentUtils.h
@@ +198,5 @@
>                                    JS::Handle<jsid> aId,
>                                    JS::MutableHandle<JSPropertyDescriptor> aDesc);
>  
> +  // Check whether we should avoid leaking distinguishing information to JS/CSS.
> +  static bool ShouldResistFingerprinting(nsIDocShell* docShell);

aDocShell

::: layout/style/nsComputedDOMStyle.cpp
@@ +1443,5 @@
> +  if (!mPresShell ||
> +      !mPresShell->GetPresContext() ||
> +      nsContentUtils::ShouldResistFingerprinting(
> +        mPresShell->GetPresContext()->GetDocShell()))
> +    return nullptr;

mPresShell will be non-null in here (as well as its pres context), since we will never call property getter functions in GetPropertyCSSValue if UpdateCurrentStyleSources failed due lack of a pres shell/context.  Let's change this to just |if (nsContentUtils::ShouldResist...|.

::: layout/style/nsMediaFeatures.cpp
@@ +347,4 @@
>  {
>      aResult.Reset();
>  #ifdef XP_WIN
> +    if (!ShouldResistFingerprinting(aPresContext)) {

I think you missed my comment on the earlier patch to just make this an early return, rather than indenting the majority of the function (to match GetOperatinSystemVersion).

::: layout/style/test/chrome/bug418986-2.js
@@ +94,5 @@
> +  if (val === null) {
> +    return;
> +  } else if (Array.isArray(val)) {
> +    ok(keyValMatches("min-" + key, val[0]) && keyValMatches("max-" + key, val[1]),
> +       "Expected approximately " + key + ":" + (val[0] + val[1])/2);

Is (val[0] + val[1])/2 going to give you a sensible string, when you pass in min/max values with units (like dppx) on them?  I think it'll end up being "NaN".  Just list the min/max values?

@@ +103,5 @@
> +
> +// __testToggles(resisting)__.
> +// Test whether we are able to match the "toggle" media queries.
> +let testToggles = function (resisting) {
> +  suppressed_toggles.map(

"forEach" is more idiomatic than "map", since the latter is generally used for producing a new array.

Comment 59

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

I have made all changes as requested by reviews in Comment 57 and Comment 58. Here are the try server results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9e8c3e8058c

Comment 60

[Arthur Edelstein [:arthur]]

Comment on attachment 8616257 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

(Unexpected bug found on try server. I'll fix and submit again.)

Comment 61

[Arthur Edelstein [:arthur]]

Attachment: 0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

OK, unit test issue fixed. Here's the new try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26370be300b8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Comment 62

[Arthur Edelstein [:arthur]]

(Here are same try results with nothing hidden:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26370be300b8)

Comment 63

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8616476 [details] [diff] [review]
0001-Bug-418986-Resist-fingerprinting-by-preventing-expos.patch

This needs review before it can be landed. Also, please use the checkin-needed bug keyword rather than setting checkin? on the attachment. Thanks!

Comment 64

[Ryan VanderMeulen [:RyanVM]]

Sorry, I missed that this was carrying over reviews from previous patches (got a bit lost in all the obsolete ones).

Comment 65

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3abb08512b24

Comment 66

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3abb08512b24

Comment 67

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So we use these methods internally in a bunch of places ... and we'll lie to ourselves about the numbers when this preference is enabled ... do we care?

Comment 68

[Arthur Edelstein [:arthur]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #67)
> So we use these methods internally in a bunch of places ... and we'll lie to
> ourselves about the numbers when this preference is enabled ... do we care?

This patch was designed to lie only to content scripts but not to chrome: see https://hg.mozilla.org/mozilla-central/rev/3abb08512b24#l1.51. (I confirmed this by adding unit tests as requested in comment 54.) But if there are example(s) of where chrome code is getting the spoofed result, maybe open a ticket? I would be happy to try to fix any such problems.

Comment 69

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Arthur Edelstein from comment #68)
> (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #67)
> > So we use these methods internally in a bunch of places ... and we'll lie to
> > ourselves about the numbers when this preference is enabled ... do we care?
> 
> This patch was designed to lie only to content scripts but not to chrome:
> see https://hg.mozilla.org/mozilla-central/rev/3abb08512b24#l1.51. (I
> confirmed this by adding unit tests as requested in comment 54.) But if
> there are example(s) of where chrome code is getting the spoofed result,
> maybe open a ticket? I would be happy to try to fix any such problems.

That code has us telling the truth when someone asks for these properties on chrome documents (which only chrome JS can see). It doesn't tell the truth to chrome script calling methods on a content document, or to C++ callers calling methods on a content document. The latter happens in various places, two examples are http://hg.mozilla.org/mozilla-central/annotate/f7b746b4e913/dom/media/webrtc/MediaEngineTabVideoSource.cpp#l204 and http://hg.mozilla.org/mozilla-central/annotate/f7b746b4e913/dom/camera/DOMCameraControl.cpp#l366.

Comment 70

[Arthur Edelstein [:arthur]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #69)
> (In reply to Arthur Edelstein from comment #68)
> > (In reply to Kyle Huey [:khuey] ([e-mail]) from comment #67)
> > > So we use these methods internally in a bunch of places ... and we'll lie to
> > > ourselves about the numbers when this preference is enabled ... do we care?
> > 
> > This patch was designed to lie only to content scripts but not to chrome:
> > see https://hg.mozilla.org/mozilla-central/rev/3abb08512b24#l1.51. (I
> > confirmed this by adding unit tests as requested in comment 54.) But if
> > there are example(s) of where chrome code is getting the spoofed result,
> > maybe open a ticket? I would be happy to try to fix any such problems.
> 
> That code has us telling the truth when someone asks for these properties on
> chrome documents (which only chrome JS can see). It doesn't tell the truth
> to chrome script calling methods on a content document, or to C++ callers
> calling methods on a content document. The latter happens in various places,
> two examples are
> http://hg.mozilla.org/mozilla-central/annotate/f7b746b4e913/dom/media/webrtc/
> MediaEngineTabVideoSource.cpp#l204 and
> http://hg.mozilla.org/mozilla-central/annotate/f7b746b4e913/dom/camera/
> DOMCameraControl.cpp#l366.

I opened bug 1216800 to look into this problem further.