Opened 7 months ago

Closed 2 months ago

#23104 closed defect (fixed)

CSS line-height reveals the platform Tor Browser is running on

Reported by: gk Owned by: igt0
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-fingerprinting-os, TorBrowserTeam201712R
Cc: igt0, mcs, brade, pospeselr, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: gk Sponsor:

Description

Dr. Neal Krawetz reported via HackerOne that it is possible to detect the platform a Tor Browser is running with the CSS line-height attribute: 19px is used on Linux, 19.5167px on macOS, and 19.2px or 20px on Windows.

We could think about adjusting that to 20px independent of the platform Tor Browser is running on.

Child Tickets

Attachments (5)

0001-bug-23104-Add-a-default-line-height-compensation.2.patch (2.8 KB) - added by igt0 5 months ago.
Updated patch - Revision 2
0001-bug-23104-Add-a-default-line-height-compensation.3.patch (4.0 KB) - added by igt0 5 months ago.
Revision 3
0001-bug-23104-Add-a-default-line-height-compensation.patch (4.0 KB) - added by igt0 5 months ago.
Revision 3
0001-bug-23104-Add-a-default-line-height-compensation.4.patch (4.0 KB) - added by igt0 5 months ago.
Revision 4
0001-bug-23104-Add-a-default-line-height-compensation.5.patch (5.5 KB) - added by igt0 3 months ago.
version 5 - Make sure the content is not inside the chrome

Download all attachments as: .zip

Change History (26)

comment:1 Changed 6 months ago by gk

comment:2 Changed 5 months ago by igt0

Owner: changed from tbb-team to igt0
Status: newaccepted

comment:3 Changed 5 months ago by igt0

Cc: igt0 added
Reviewer: gk

Many fonts have issues with their vertical metrics. they
are used to influence the height of ascenders and depth
of descenders. Gecko uses it to calculate the line height
(font height + ascender + descender), however because of
that idiosyncratic behavior across multiple operating
systems, it can be used to identify the user's OS.

The solution proposed in the patch([1]) is to use a default factor
to be multiplied as ascender and descender. This way all operating
systems will have the same line height.

[1] https://trac.torproject.org/projects/tor/attachment/ticket/23104/0001-bug-23104-Add-a-default-line-height-compensation.2.patch

Last edited 5 months ago by igt0 (previous) (diff)

comment:4 Changed 5 months ago by igt0

Status: acceptedneeds_review

comment:5 Changed 5 months ago by gk

Keywords: TorBrowserTeam201709R added

Changed 5 months ago by igt0

Updated patch - Revision 2

comment:7 Changed 5 months ago by gk

Keywords: TorBrowserTeam201709R removed
Status: needs_reviewneeds_revision

Okay, some comments. I just tested Windows and Linux builds and it seems your fix works for me. Nice! Do you have a test which let you check your patch works correctly? If not, how have you verified your patch does indeed fix the problem?

In Tor Browser (and Mozilla in Firefox) we bind our fingerprinting resistance to the privacy.resistFingerprinting preference: only when that preference is set to true all the fingerprinting patches are active. If not then the default Firefox behavior in that regard is visible. That way it is much more easier for Mozilla to upstream our patches.

Could you make sure as well that your code is only active when this preference is set to true and inactive otherwise? Bonus points for a unit test showing that your patch does indeed do what it claims it does.

comment:8 Changed 5 months ago by igt0

Last edited 5 months ago by igt0 (previous) (diff)

comment:9 Changed 5 months ago by igt0

Status: needs_revisionneeds_review

comment:10 Changed 5 months ago by gk

Keywords: TorBrowserTeam201709R added

comment:11 Changed 5 months ago by gk

Cc: mcs brade pospeselr arthuredelstein added

Looks good to me and works on Linux and Windows at least. The test passes as well. I wonder, though, how future-proof the test is assuming the factor 1.2 comes from NORMAL_LINE_HEIGHT_FACTOR? Adding some folks for a second review.

Last edited 5 months ago by gk (previous) (diff)

comment:12 Changed 5 months ago by igt0

Great question, the css2 specification[1] says that the line height factor should be between 1.0 and 1.2. Mozilla uses 1.2 for some time[2] and only if the ascending and descending values are 0. Chrome has its own thing[3].

That said, for the short and medium term, I think the current test makes sense because if the line factor changes, the test will break. I tried to think about a ref test instead of JS test. However the 1.2 value would be hardcoded on it anyway.

[1] https://www.w3.org/TR/CSS2/visudet.html
[2] https://developer.mozilla.org/en-US/docs/Web/CSS/line-height
[3] https://bugs.chromium.org/p/chromium/issues/detail?id=455754

comment:13 Changed 5 months ago by mcs

r=brade, r=mcs
One suggestion for the test would be to add something like:

const NORMAL_LINE_HEIGHT_FACTOR = 1.2;

(and add a comment to refer back to layout/generic/ReflowInput.cpp.

Also, it seems like it is possible that the tests would fail when the anti-fingerprinting pref is turned off, e.g., if the line-height happens to really be 1.2x the font-size.

comment:14 Changed 5 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Taking that one with a fixup for the upcoming alpha (commits 323d2525fcd2de963a2e291eab91d5ca9ea7ac9b and 125ac5c7c4259be3d0bfa9da6fe5a6466ee975cb on tor-browser-52.3.0esr-7.5-2). I think we can adapt the test during upstreaming the patch to Firefox. Thanks.

comment:15 Changed 5 months ago by gk

Resolution: fixed
Status: closedreopened

I think we should take #23701 right away into account and reopen this bug. I think the missing piece we want is making sure this defense is only applied to content but not the browser chrome. I am backing out the patch from our alpha branch (done with commit 5117adf6b06bd2901144d15847b4cc902a94741a).

Changed 3 months ago by igt0

version 5 - Make sure the content is not inside the chrome

comment:16 Changed 3 months ago by igt0

Status: reopenedneeds_review

comment:17 Changed 3 months ago by igt0

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201709R removed

comment:18 Changed 3 months ago by gk

Keywords: TorBrowserTeam201712R added; TorBrowserTeam201712 removed

comment:19 Changed 2 months ago by mcs

The new patch looks good to Kathy and me. We did not build and test a Windows browser to make sure that #23701 was fixed. Igor or Georg, did you do that?

Also, we noticed that a Math.ceil() call was removed from the test. I assume it is not needed?

comment:20 in reply to:  19 Changed 2 months ago by igt0

Replying to mcs:

The new patch looks good to Kathy and me. We did not build and test a Windows browser to make sure that #23701 was fixed. Igor or Georg, did you do that?

I tested in a Windows and Linux machines, however I would love to have a second person giving a try to make sure I didn't miss anything.

Also, we noticed that a Math.ceil() call was removed from the test. I assume it is not needed?

Yep, It was not needed.

comment:21 Changed 2 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good. Applied to tor-browser-52.5.2esr-7.5-2 as commit 89d8bc54cfe6a4cd999ab1f16f434c4fb809b643.

Note: See TracTickets for help on using tickets.