Opened 13 months ago

Closed 3 months ago

#24056 closed defect (fixed)

UI locale is detectable by button width

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: tbb-fingerprinting, TorBrowserTeam201808R
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We got a HackerOne report by xiaoyinl outlining steps to detect the UI locale of the browser despite the user not changing the Accept header:

If one specifies a button <input type="submit"> without a "value" property, the default text of the button depends on the UI language. And the resulting width of the button can be measured.

We should not only focus on that particular button but should look closer whether other UI parts are suffering from the same problem.

Child Tickets

Change History (12)

comment:1 Changed 13 months ago by tom

I doubt these would be detectable, but I know javascript's prompt() and confirm() functions would do the same. And then there's the default text on the button in alert().

comment:2 Changed 4 months ago by arthuredelstein

Here's list of strings that are displayed by default in elements of HTML forms:
https://dxr.mozilla.org/mozilla-central/source/dom/locales/en-US/chrome/layout/HtmlForm.properties

comment:3 Changed 3 months ago by arthuredelstein

Keywords: TorBrowserTeam201808R added
Status: newneeds_review

Here is a patch for review. It spoofs all HTML form elements as US English when "privacy.spoof_english" == 2.

https://github.com/arthuredelstein/tor-browser/commit/24056

comment:4 Changed 3 months ago by pospeselr

Looking at the code, there doesn't *seem* to be any reason why gPropertiesFile can't be:

static const char* gPropertiesFile[nsContentUtils::PropertiesFile[COUNT]

The CreateBundle method each of those strings is passed to expects a const char* with no hard-coded expectation of length. It's static so the symbol can't be resolved outside this cpp. Also, fwiw the new max length string in that array is 75, not 78 (including null-terminator).

Why is the setting to enable 2 rather than 1?

Apart from that it looks fine to me.

Last edited 3 months ago by pospeselr (previous) (diff)

comment:5 Changed 3 months ago by pospeselr

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201808R removed
Status: needs_reviewneeds_revision

comment:6 in reply to:  4 ; Changed 3 months ago by arthuredelstein

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201808 removed
Status: needs_revisionneeds_review

Replying to pospeselr:

Thanks for the review!

Looking at the code, there doesn't *seem* to be any reason why gPropertiesFile can't be:

static const char* gPropertiesFile[nsContentUtils::PropertiesFile[COUNT]

The CreateBundle method each of those strings is passed to expects a const char* with no hard-coded expectation of length. It's static so the symbol can't be resolved outside this cpp.

Good point. Following your suggestion I changed the line to

static const char* gPropertiesFiles[nsContentUtils::PropertiesFile_COUNT] = {

Why is the setting to enable 2 rather than 1?

From firefox.js:

// If Accept-Language should be spoofed by en-US
// 0 - will prompt
// 1 - don't spoof
// 2 - spoof
pref("privacy.spoof_english", 0);

Apart from that it looks fine to me.

Here's the revised patch:
https://github.com/arthuredelstein/tor-browser/commit/24056+1

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

Replying to arthuredelstein:

Here's the revised patch:
https://github.com/arthuredelstein/tor-browser/commit/24056+1

It seems we don't need to potentially adjust aFile in

nsresult nsContentUtils::FormatLocalizedString(
                                          PropertiesFile aFile,
                                          const char* aKey,
                                          const nsTArray<nsString>& aParamArray,
                                          nsAString& aResult)

as it does not get touched in it but rather gets passed on.

I guess we are safe regarding GetLocalizedEllipsis()? At least I hope so, assuming the localization folks adhered to Mozilla's advice to use \u2026 for it...

comment:8 Changed 3 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201808R removed
Status: needs_reviewneeds_revision

comment:9 in reply to:  7 Changed 3 months ago by arthuredelstein

Status: needs_revisionneeds_review

Replying to gk:

Replying to arthuredelstein:

Here's the revised patch:
https://github.com/arthuredelstein/tor-browser/commit/24056+1

It seems we don't need to potentially adjust aFile in

nsresult nsContentUtils::FormatLocalizedString(
                                          PropertiesFile aFile,
                                          const char* aKey,
                                          const nsTArray<nsString>& aParamArray,
                                          nsAString& aResult)

as it does not get touched in it but rather gets passed on.

You're right. I removed the inserted lines there.

I guess we are safe regarding GetLocalizedEllipsis()? At least I hope so, assuming the localization folks adhered to Mozilla's advice to use \u2026 for it...

Good point. I think it's safer to patch this. I have done so in this latest version:

https://github.com/arthuredelstein/tor-browser/commit/24056+2

comment:10 Changed 3 months ago by arthuredelstein

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201808 removed

comment:11 Changed 3 months ago by pospeselr

Latest version looks good to me!

comment:12 Changed 3 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Great, cherry-picked to tor-browser-60.1.0esr-8.0-1 (commit cbb04b72c68272c2de42f157d40cd7d29a6b7b55).

Note: See TracTickets for help on using tickets.