Opened 7 months ago

Closed 6 weeks ago

#30683 closed defect (fixed)

Properties in dom/locales/$lang/chrome/ allow detecting user locale

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-fingerprinting-locale TorBrowserTeam201911R
Cc: mcs, brade Actual Points: 1
Parent ID: Points: 0.25
Reviewer: Sponsor:

Description

z3t reported a bunch of issues on HackerOne regarding detection of user locale with the help of dom/locales/$lang/chrome/ properties. PoCs done by z3t:

dom/dom.properties: https://people.torproject.org/~gk/tests/tor_form_locale_leak.html
layout/xmlparser.properties: https://people.torproject.org/~gk/tests/tor_domparser_locale_leak.html
layout/MediaDocument.properties: https://people.torproject.org/~gk/tests/tor_image_locale_leak.html

Child Tickets

Change History (30)

comment:1 in reply to:  description Changed 6 months ago by gk

Replying to gk:

z3t reported a bunch of issues on HackerOne regarding detection of user locale with the help of dom/locales/$lang/chrome/ properties. PoCs done by z3t:

dom/dom.properties: https://people.torproject.org/~gk/tests/tor_form_locale_leak.html

<input type="email"> is relevant here as well (thanks to mik317 on HackerOne for pointing that out).

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

comment:2 Changed 6 months ago by acat

Keywords: TorBrowserTeam201906R added
Status: newneeds_review

Here is a small patch for review: https://www.github.com/acatarineu/tor-browser/commit/30683. I checked htmlparser.properties just in case, but it seems only being used in view-source:* pages, which should not be accessible from web content.

Do we want all these tbb-fingerprinting-locale fixed just for next esr68 or also for current esr60? I did the patch based on current alpha, but it should not be difficult to adapt for esr68.

comment:3 in reply to:  2 Changed 6 months ago by gk

Replying to acat:

Here is a small patch for review: https://www.github.com/acatarineu/tor-browser/commit/30683. I checked htmlparser.properties just in case, but it seems only being used in view-source:* pages, which should not be accessible from web content.

Do we want all these tbb-fingerprinting-locale fixed just for next esr68 or also for current esr60? I did the patch based on current alpha, but it should not be difficult to adapt for esr68.

We need a patch for esr68 as well be it now or when we rebase, so having one version for esr68 (even though we start testing with an esr60 alpha) seems good. Ideally, while you are at it you'd file a ticket at bugzilla and get the patch upstreamed (and with some luck we can include it that way in the esr68 without needing to ship the patch ourselves).

comment:4 Changed 6 months ago by gk

fwiw: mik317 reminded us to check for error message because of corrupted png images, too (which is available in MediaDocument.properties), thanks!

comment:5 Changed 6 months ago by gk

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201906R removed

No reviews in June 2019 anymore, moving them.

comment:6 Changed 6 months ago by gk

Cc: mcs brade added
Keywords: TorBrowserTeam201907 added; TorBrowserTeam201907R removed
Status: needs_reviewneeds_revision

I think this looks mostly good to me. Could you adapt
// When we spoof English, use en-US default strings in HTML forms. a bit as the code that follows is not only about forms anymore?

Have you checked whether the patch is a good upstreaming idea as-is? I wonder in particular if the en-US strings will always be available in, say fr, builds that don't come with fr lang packs.

mcs, brade could you have a second look?

comment:7 Changed 6 months ago by acat

Have you checked whether the patch is a good upstreaming idea as-is? I wonder in particular if the en-US strings will always be available in, say fr, builds that don't come with fr lang packs.

That's a very nice point, I missed this one. Indeed, Firefox localized builds do not have these, I only tested with language packs... Good that you realized before https://phabricator.services.mozilla.com/D35815 landed, since it was accepted.

I'm not sure what's the best way to make this upstreamable. I think it's not realistic to try to make the localized messages not accessible to web content, so I think making available these en-US locales in localized builds is the only way. I will take a look and try to see how difficult it is to do this, and update https://bugzilla.mozilla.org/show_bug.cgi?id=1561322. The approach is the same, so whatever is accepted in that bug should also apply here.

comment:8 in reply to:  6 Changed 6 months ago by mcs

Replying to gk:

I think this looks mostly good to me. Could you adapt
// When we spoof English, use en-US default strings in HTML forms. a bit as the code that follows is not only about forms anymore?

Maybe something like:
// When we spoof English, use en-US properties in strings that are accessible by content.

Have you checked whether the patch is a good upstreaming idea as-is? I wonder in particular if the en-US strings will always be available in, say fr, builds that don't come with fr lang packs.

mcs, brade could you have a second look?

r=brade,r=mcs
The patch looks good to us. Regarding upstreaming, it seems like the choice is to include the en-US properties files or hard-coded a bunch of strings. Including the en-US files seems like a good solution, but I guess we will see what the Mozilla engineers say.

comment:9 Changed 6 months ago by cypherpunks

hard-coded a bunch of strings

Never do that.

comment:10 Changed 4 months ago by gk

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201907 removed

comment:11 Changed 4 months ago by acat

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201908 removed
Status: needs_revisionneeds_review

Here is a branch with the patch ported to esr68: https://github.com/acatarineu/tor-browser/commits/30683_esr68. I also reverted #24056 and backported the upstreamed to Firefox one, since this patch depends on those changes. So #31298 should be also fixed here.

comment:12 Changed 4 months ago by acat

More than porting, it's actually rewriting the patch similarly to the upstreamed patch for #24056 (https://bugzilla.mozilla.org/show_bug.cgi?id=1561322).

comment:13 in reply to:  12 Changed 4 months ago by gk

Keywords: TorBrowserTeam201908 added; TorBrowserTeam201908R removed
Status: needs_reviewneeds_revision

Replying to acat:

More than porting, it's actually rewriting the patch similarly to the upstreamed patch for #24056 (https://bugzilla.mozilla.org/show_bug.cgi?id=1561322).

It seems my tiny request in comment:6 still applies?

comment:14 Changed 4 months ago by acat

Status: needs_revisionneeds_review

comment:15 Changed 3 months ago by gk

Looks good. I wonder whether the ServiceWorker ReportToAllClients() messages are detectable from web content actually. Have we ruled that out? (while not being enabled in ESR 68 it will be for mobile IIRC).

comment:16 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201908 removed

Moving tickets to September

comment:17 Changed 3 months ago by pili

Points: 0.25

comment:19 in reply to:  15 Changed 3 months ago by acat

Replying to gk:

Looks good. I wonder whether the ServiceWorker ReportToAllClients() messages are detectable from web content actually. Have we ruled that out? (while not being enabled in ESR 68 it will be for mobile IIRC).

I think we are good. LocalizeAndReportToAllClients ends up calling ConsoleUtils.cpp which seems it's only logging to console. I also tested a bit with the different error handlers in Service Workers and I could not get a localized message.

comment:20 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201909 removed

comment:21 Changed 3 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Alright, we are done here. The patch for #24056 got reverted on tor-browser-68.1.0esr-9.0-2 and the patch for this bug landed as commit 262c4677852fd683bd3257427b39fd5accc7b30f with 80f3dafdd420491e23c22a688d075548480f78a6 as the follow-up Android fixup. (Both actually made it into 9.0a6, fwiw.)

comment:22 Changed 3 months ago by acat

Points: 0.251

comment:23 Changed 3 months ago by acat

Actual Points: 1
Points: 10.25

comment:24 Changed 6 weeks ago by acat

Keywords: TorBrowserTeam201909R removed
Resolution: fixed
Status: closedreopened

comment:25 Changed 6 weeks ago by acat

Keywords: TorBrowserTeam201911 added

comment:26 Changed 6 weeks ago by acat

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: reopenedneeds_review

comment:27 in reply to:  26 Changed 6 weeks ago by gk

Replying to acat:

Backported in https://github.com/acatarineu/tor-browser/commits/30683_backport.

Looks mostly good. I am a bit confused why we need the FormatMaybeLocalizedString() version with const nsTArray<nsString>& aParamArray. As far as I can see none of the changed code is using that one. The patch is already large, thus, could you trim it accordingly if I am not being wrong here?

comment:28 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201911R removed
Status: needs_reviewneeds_revision

acat thinks I am right, marking this as needs_revision then.

comment:29 Changed 6 weeks ago by acat

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: needs_revisionneeds_review

comment:30 Changed 6 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks! Merged to tor-browser-68.2.0esr-9.5-1 (commit cb255f2504b37916160e886a181f1a02f85dba7c and 79d87f543224b0e9d69a7520e25d9179d4f2c2bc).

Note: See TracTickets for help on using tickets.