Opened 8 months ago

Closed 3 months ago

Last modified 11 days ago

#30304 closed defect (fixed)

Browser locale can be obtained via DTD strings

Reported by: acat Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff68-esr, tbb-fingerprinting-locale, TorBrowserTeam201909R, BugSmashFund
Cc: Actual Points: 2
Parent ID: Points: 2
Reviewer: Sponsor:

Description

See https://bugzilla.mozilla.org/show_bug.cgi?id=467035.

Works in Tor Browser and Firefox 67 (with a different dtd file as in the bugzilla PoC), probably also next ESR.

Did not do a PoC but it would be easy to get a specific string in all locales, and just compare with value obtained via a hidden iframe that loads an xml with the translated string.

Child Tickets

Change History (22)

comment:1 Changed 8 months ago by gk

Keywords: tbb-fingerprinting added

comment:2 Changed 8 months ago by Thorin

I was going to bring this (GMTA).. seriously in the last few days this has been on my mind, I am not super savy in JS etc, and would appreciate a PoC for TZP ( if its possible via a github.io)

Version 0, edited 8 months ago by Thorin (next)

comment:3 Changed 8 months ago by acat

Here is a patch: https://github.com/acatarineu/tor-browser/commit/30304.

https://github.com/acatarineu/tor-browser/commit/30304#diff-52beed12eac86326f289d0b43ffadb3bL215 should remove the exception that allows all DTDs to be loaded. In practice, removing this I think would only break legacy extensions. For example, it breaks about:tor, because its locale files are not contentaccessible and cannot be loaded anymore.

With that change, contentaccessible=yes dtds can still be loaded from web content, and therefore still leaking browser language. https://github.com/acatarineu/tor-browser/commit/30304#diff-318b0aeb24e87bc216c590860a030b58R524 should fix that, forbidding any chrome://*/locale/* URIs to be loaded from web content, contentaccessible or not.

This change breaks many about:* pages, since they rely on the locales being contentaccessible. The change here: https://github.com/acatarineu/tor-browser/commit/30304#diff-d637999733ba943ba8fc3b01c451df66R866 tries to fix that (and also previously mentioned about:tor breakage), allowing about:* pages to always load chrome://* resources.

I'm making a couple of assumptions I would need to check. First, I'm assuming the only way to load locale DTDs is via chrome://*/locale/*. If there is a way to load them via resource://* or some chrome URIs that do not follow that structure, I think the patch would not work. Second, there is no way to load a resource from web content as about:*. If this was possible we could bypass the protection, because we are adding an exception for about pages.

Then, there is breakage again. Since Firefox relies on several resource:// and chrome:// URIs to load in web content for some features, these changes might break something. I think #8725 and corresponding https://bugzilla.mozilla.org/show_bug.cgi?id=863246 are relevant here. Arthur mentions in bugzilla several features that use these (video controls, image display, text-box resizing, and directory listing). I checked this and they work with this patch in ESR60 (except text-box resizing, not sure which one is that).

So I could not find this breaking in current ESR60. BUT, the same patch breaks several things in Firefox Nightly. First, the browser UI itself (for some reason it's loading chrome://... dtds from moz-nullprincipal origins). Video controls are also broken, displaying xml with errors (because the locale DTD could not be loaded).

I think I'll update the bugzilla ticket with a patch for central (even if it's broken), and ni ckerschb as Tom suggested.

A couple of tests: https://acatarineu.github.io/fp/locale.html https://acatarineu.github.io/fp/locale_nullprincipal.html.

comment:4 Changed 7 months ago by acat

Updated patch: https://github.com/acatarineu/tor-browser/commit/30304+1. Took PrincipalAllowsL10n function from the checks for new localization system (Fluent) https://hg.mozilla.org/mozilla-central/file/c6d806b496845985516cc04342c04988aa1817dd/dom/base/Document.cpp#l1995, and applied to DTD URI loads.

Still, in central applying this will require fixing some things breaking due to some internal UI changes, but in ESR60 I did not see anything break so far. I think it would be ok to apply, but not sure if we want to wait for some progress on the bugzilla before.

comment:5 Changed 7 months ago by acat

Keywords: TorBrowserTeam201905 added
Status: newneeds_review

comment:6 Changed 7 months ago by gk

Keywords: TorBrowserTeam201905R added; TorBrowserTeam201905 removed

comment:7 in reply to:  4 Changed 7 months ago by gk

Keywords: ff68-esr added

Replying to acat:

Updated patch: https://github.com/acatarineu/tor-browser/commit/30304+1. Took PrincipalAllowsL10n function from the checks for new localization system (Fluent) https://hg.mozilla.org/mozilla-central/file/c6d806b496845985516cc04342c04988aa1817dd/dom/base/Document.cpp#l1995, and applied to DTD URI loads.

I think we should not just copy the method but just have one in the tree and make all caller using that one (as you did in your patch for mozilla-central). Apart from that this looks fine to me (although I have not tested the fix yet)

Still, in central applying this will require fixing some things breaking due to some internal UI changes, but in ESR60 I did not see anything break so far. I think it would be ok to apply, but not sure if we want to wait for some progress on the bugzilla before.

So, this fix won't very likely make it into Tor Browser 8.5 and thus in no ESR 60 based stable release. Therefore, I am inclined to just fix it on ESR 68 and get it early on into our Tor Browser alpha builds based on that series (to iron out possible breakage). I am marking this ticket with ff68-esr to get it onto our radar.

comment:8 Changed 7 months ago by gk

Status: needs_reviewneeds_revision

comment:9 Changed 7 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201905R removed

comment:10 Changed 7 months ago by gk

Keywords: tbb-fingerprinting-locale added; tbb-fingerprinting removed

Add a more specific locale fingerprinting keyword.

comment:11 Changed 6 months ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201905 removed

Moving tickets to June

comment:12 Changed 5 months ago by acat

We can backport https://hg.mozilla.org/mozilla-central/rev/3b5afc9aa309 and https://hg.mozilla.org/mozilla-central/rev/9276e88ea3fe for this. Note that the hack for Android/Desktop duplicated translations (https://github.com/acatarineu/tor-browser/commit/6647e87dc1ed59c5b39e8618bf8753d1d8423343) will have to be adapted after this (we need parser.forceEnableDTD();)

comment:13 Changed 4 months ago by pili

Sponsor: Sponsor44-can

Adding Sponsor 44 to ESR68 tickets

comment:14 Changed 3 months ago by acat

Ported patches and fixup for review in https://github.com/acatarineu/tor-browser/commits/30304+2 (last three commits).

comment:15 Changed 3 months ago by acat

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201906 removed
Status: needs_revisionneeds_review

comment:16 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201909R removed
Status: needs_reviewneeds_revision

Looks good to me. I guess we want to have a fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1573276, too? Either way, I cherry-picked what we have at the moment and applied it to tor-browser-68.1.0esr-9.0-2 (commit 93c30885b92760fcdf6bd06b235ddd3c87b09b97, 2fe57c62af9706be45fc2085796a9398c3b10763, and 7baed647f56e5d3e7c92eaffadb1c18c07aabe7f).

comment:17 Changed 3 months ago by acat

Actual Points: 2

Yes, let's see if it gets reviewed/accepted soon.

comment:18 Changed 3 months ago by pili

Points: 2

comment:19 Changed 3 months ago by acat

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed
Status: needs_revisionneeds_review

comment:20 Changed 3 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Looks good, thanks! Merged to tor-browser-68.1.0esr-9.0-2 (commit 9c1877056070c4de048b18e201fcb3dfcb1e1a02).

comment:21 Changed 11 days ago by pili

Keywords: BugSmashFund added

BugSmashFund can be used for the ESR work done so far

comment:22 Changed 11 days ago by pili

Sponsor: Sponsor44-can

Sponsor 44 only covered PM and Team Lead work

Note: See TracTickets for help on using tickets.