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

added TorBrowserTeam201712R in Legacy / Trac component::applications/tor browser in Legacy / Trac owner::igt0 in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac reviewer::gk in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tbb-fingerprinting-os in Legacy / Trac type::defect in Legacy / Trac labels

FWIW: this is https://bugzilla.mozilla.org/show_bug.cgi?id=1397994 on Mozilla's side.

Trac:
Owner: tbb-team to igt0
Status: new to accepted

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

Trac:
Reviewer: N/A to gk
Cc: N/A to igt0

Trac:
Status: accepted to needs_review

Trac:
Keywords: N/A deleted, TorBrowserTeam201709R added

Trac:
0001-bug-23104-Add-a-default-line-height-compensation.2.patch

Updated patch - Revision 2

Updated patch(accidentally I submitted an old version):

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

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.

Trac:
Keywords: TorBrowserTeam201709R deleted, N/A added
Status: needs_review to needs_revision

Trac:
0001-bug-23104-Add-a-default-line-height-compensation.3.patch

Revision 3

Code updated to use privacy.resistFingerprinting and I added a tbb test:

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

Trac:
Status: needs_revision to needs_review

Trac:
0001-bug-23104-Add-a-default-line-height-compensation.patch

Revision 3

Trac:
0001-bug-23104-Add-a-default-line-height-compensation.4.patch

Revision 4

Trac:
Keywords: N/A deleted, TorBrowserTeam201709R 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.

Trac:
Cc: igt0 to igt0, mcs, brade, pospeselr, arthuredelstein

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

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.

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

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

I think we should take legacy/trac#23701 (moved) 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 5117adf6).

Trac:
Resolution: fixed to N/A
Status: closed to reopened

Trac:
0001-bug-23104-Add-a-default-line-height-compensation.5.patch

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

Trac:
Status: reopened to needs_review

Trac:
Keywords: TorBrowserTeam201709R deleted, TorBrowserTeam201712 added

Trac:
Keywords: TorBrowserTeam201712 deleted, TorBrowserTeam201712R added

The new patch looks good to Kathy and me. We did not build and test a Windows browser to make sure that legacy/trac#23701 (moved) 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?

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 legacy/trac#23701 (moved) 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.