Date.toLocaleFormat exposes OS locale (Tor 13019)

RESOLVED FIXED in Firefox 46

Status

()

--
minor
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jruderman, Assigned: arthur)

Tracking

(Blocks: 1 bug, {dev-doc-needed, privacy})

Trunk
mozilla46
x86_64
Windows 7
dev-doc-needed, privacy
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [fingerprinting][tor])

Attachments

(1 attachment, 4 obsolete attachments)

Comment hidden (empty)
Assignee: general → nobody
(Assignee)

Updated

4 years ago
Whiteboard: [fingerprinting] → [fingerprinting][tor]
(Assignee)

Comment 1

4 years ago
Here's an anti-fingerprinting patch from Tor Browser (rebased to the latest mozilla-central) that exposes a pref, 'javascript.use_us_english_locale', to force the browser to use C/en-US date formatting by default, regardless of the OS locale. This fix covers both Date.toLocaleFormat and locale exposed by Intl.*.
Attachment #8617620 - Flags: review?(sphink)
Comment on attachment 8617620 [details] [diff] [review]
0001-Pref-allows-JS-locale-to-be-set-to-English-C.patch

I'm not sure who the appropriate reviewer is for this, nor am I familiar with the policies for changes like this. I'll punt it over to Jeff, who is at least a little closer to being correct, but perhaps he'll want to send it over to bsmedberg or somebody?
Attachment #8617620 - Flags: review?(sphink) → review?(jwalden+bmo)
Comment on attachment 8617620 [details] [diff] [review]
0001-Pref-allows-JS-locale-to-be-set-to-English-C.patch

Review of attachment 8617620 [details] [diff] [review]:
-----------------------------------------------------------------

There are a bunch of style nits to fix, but it seems not worth mentioning them when there are substantive issues in the way first.

::: browser/base/content/test/general/test_bug_jsdefaultlocale.html
@@ +19,5 @@
> +    originalPrefValue = null,
> +    date = new Date();
> +// Read the current pref value.
> +try {
> +  originalPrefValue = SpecialPowers.getBoolPref(prefName);

There's some sort of pushPrefEnvironment method in tests these days, that lets you do a test with a modified preference that's auto-restored on test completion -- including on test failure.  If we indeed allow modifying the locale via the pref at runtime, I definitely will require that the test be rewritten to use that.

@@ +27,5 @@
> +// Test that we are getting en-US locale everywhere it is exposed in JavaScript
> +is(Intl.Collator().resolvedOptions().locale, "en-US", "content JS locale from Intl.Collator");
> +is(Intl.DateTimeFormat().resolvedOptions().locale, "en-US", "content JS locale Intl.DateTimeFormat");
> +is(Intl.NumberFormat().resolvedOptions().locale, "en-US", "content JS locale from Intl.NumberFormat");
> +is(date.toLocaleString(), date.toLocaleString("en-US"), "Date formatted by JS locale");

So this ensures the locale is correct with the pref set, but it may well be the case that without it set, we *still* get en-US, by dint of our (possibly) only doing tests in en-US builds.  I probably also will want this test to figure out, and properly test, that the override actually overrides a non-"en-US" prior locale.  As it is it's not really proving anything to me by demonstration of a semantic change with the pref set.

::: js/src/jsapi.cpp
@@ +5453,5 @@
> +JS_PUBLIC_API(const char*)
> +JS_GetDefaultLocale(JSRuntime *rt)
> +{
> +    AssertHeapIsIdle(rt);
> +    return rt->getDefaultLocale();

This returns a string owned by the JSRuntime, which is...rather scary, actually, because if someone else decided to change the locale on you, this pointer would turn to garbage.  Instead return a string to be owned by the caller.  Instead do:

    if (const char* locale = rt->getDefaultLocale())
        return JS_strdup(rt, locale);
    return nullptr;

I acknowledge this is probably inconsistent with some other JSAPI methods.  But we gotta start doing it right sometime, and I have little interest in holding out for everyone else to do the right thing first.

::: js/src/jsapi.h
@@ +4481,5 @@
>  JS_SetDefaultLocale(JSRuntime* rt, const char* locale);
>  
>  /*
> + * Returns the default locale for the ECMAScript Internationalization API
> + * (Intl.Collator, Intl.NumberFormat, Intl.DateTimeFormat).

This is somewhat over-specified, because this also affects the behavior of toLocaleString, toLocaleDateString, and toLocaleTimeString.  Let's go with:

Returns the locale used by locale-sensitive JS operations when no locale is specified.  This includes the ECMAScript Internationalization API, as well as those methods whose operation is specified in terms of that API (Date objects' toLocaleString, toDateString, and toTimeString functions).

And in accord with comments on the implementation, also tack on

The returned string is owned by, and must ultimately be JS_free'd by, the caller.

::: xpcom/build/XPCOMInit.cpp
@@ +479,5 @@
> +// The current default locale can be detected in JavaScript by calling
> +// `Intl.DateTimeFormat().resolvedOptions().locale`
> +namespace {
> +
> +#define USE_US_ENGLISH_LOCALE_PREF "javascript.use_us_english_locale"

static const char USE_US_ENGLISH_LOCALE_PREF[] = "...";

@@ +501,5 @@
> +static
> +void UseUSEnglishLocalePrefChangedCallback(const char* /* pref */, void* /* closure */) {
> +  // Get a pointer to the default JS Runtime.
> +  JSRuntime* rt = GetRuntime();
> +  if (rt) {

Needing "a" JSRuntime is a tell.  There are multiple runtimes: one per thread running JS code.  Setting a runtime's default locale only affects code on one thread.  You're fixing the main thread but leaving workers broken.  Worse, the C locale stuff *is* a set-once (not per-thread) thing.  And setlocale isn't even threadsafe!

Solving this all -- particularly scaling across all threads, given the preference service is main-thread-only! -- seems rather horrific.  I think we should abandon having a live preference and just determine the locale and do a setlocale() at startup, then consistently apply it to all runtimes.

So:

1. In XPCOM startup code:
* save a copy of the system locale
* query the preference
* if the preference says to use "en-US", setlocale() it and set the saved locale
2. In xpc_LocalizeRuntime, in js/xpconnect/src/XPCLocale.cpp:
* set the default locale on each runtime, on the basis of the saved locale
3. In XPCOM shutdown code:
* free the saved-aside locale

...wait.  Oh gag.  xpc_LocalizeRuntime is called only on the main thread.  We don't even specify the default locale on workers, so everything goes to pieces.  La la la la la.  Filed as bug 1174386.  You may well want to wait til that's landed to continue here.  :-\  Or perhaps take it yourself, although I concede this is a bit yak-shavy for what probably seemed like a "simple" change to you.
Attachment #8617620 - Flags: review?(jwalden+bmo) → review-
(Assignee)

Updated

4 years ago
Depends on: 1174386
(Assignee)

Comment 4

4 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Comment on attachment 8617620 [details] [diff] [review]
> 0001-Pref-allows-JS-locale-to-be-set-to-English-C.patch
> 
> Review of attachment 8617620 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the excellent review.

> There are a bunch of style nits to fix, but it seems not worth mentioning
> them when there are substantive issues in the way first.
> 
> ::: browser/base/content/test/general/test_bug_jsdefaultlocale.html
> @@ +19,5 @@
> > +    originalPrefValue = null,
> > +    date = new Date();
> > +// Read the current pref value.
> > +try {
> > +  originalPrefValue = SpecialPowers.getBoolPref(prefName);
> 
> There's some sort of pushPrefEnvironment method in tests these days, that
> lets you do a test with a modified preference that's auto-restored on test
> completion -- including on test failure.  If we indeed allow modifying the
> locale via the pref at runtime, I definitely will require that the test be
> rewritten to use that.

Makes sense -- I've used SpecialPowers.pushPref before and I'll be happy to change it to that.

> @@ +27,5 @@
> > +// Test that we are getting en-US locale everywhere it is exposed in JavaScript
> > +is(Intl.Collator().resolvedOptions().locale, "en-US", "content JS locale from Intl.Collator");
> > +is(Intl.DateTimeFormat().resolvedOptions().locale, "en-US", "content JS locale Intl.DateTimeFormat");
> > +is(Intl.NumberFormat().resolvedOptions().locale, "en-US", "content JS locale from Intl.NumberFormat");
> > +is(date.toLocaleString(), date.toLocaleString("en-US"), "Date formatted by JS locale");
> 
> So this ensures the locale is correct with the pref set, but it may well be
> the case that without it set, we *still* get en-US, by dint of our
> (possibly) only doing tests in en-US builds.  I probably also will want this
> test to figure out, and properly test, that the override actually overrides
> a non-"en-US" prior locale.  As it is it's not really proving anything to me
> by demonstration of a semantic change with the pref set.

Good point. How do we do this, given that test machines are en-US? Is there any way to set environment variables before startup?
> 
> ::: js/src/jsapi.cpp
> @@ +5453,5 @@
> > +JS_PUBLIC_API(const char*)
> > +JS_GetDefaultLocale(JSRuntime *rt)
> > +{
> > +    AssertHeapIsIdle(rt);
> > +    return rt->getDefaultLocale();
> 
> This returns a string owned by the JSRuntime, which is...rather scary,
> actually, because if someone else decided to change the locale on you, this
> pointer would turn to garbage.  Instead return a string to be owned by the
> caller.  Instead do:
> 
>     if (const char* locale = rt->getDefaultLocale())
>         return JS_strdup(rt, locale);
>     return nullptr;
> 
> I acknowledge this is probably inconsistent with some other JSAPI methods. 
> But we gotta start doing it right sometime, and I have little interest in
> holding out for everyone else to do the right thing first.

Absolutely.

> ::: js/src/jsapi.h
> @@ +4481,5 @@
> >  JS_SetDefaultLocale(JSRuntime* rt, const char* locale);
> >  
> >  /*
> > + * Returns the default locale for the ECMAScript Internationalization API
> > + * (Intl.Collator, Intl.NumberFormat, Intl.DateTimeFormat).
> 
> This is somewhat over-specified, because this also affects the behavior of
> toLocaleString, toLocaleDateString, and toLocaleTimeString.  Let's go with:
> 
> Returns the locale used by locale-sensitive JS operations when no locale is
> specified.  This includes the ECMAScript Internationalization API, as well
> as those methods whose operation is specified in terms of that API (Date
> objects' toLocaleString, toDateString, and toTimeString functions).

In fact, toLocaleString, toDateString, toTimeString and toLocaleFormat locales are independent of JS_[Get/Set]DefaultLocale. Instead the format those calls use must be get and set using the setlocale() function. That's why I use both JS_GetDefaultLocale and setlocale in, for example, StartWatchingUseUSEnglishLocalePref()

> And in accord with comments on the implementation, also tack on
> 
> The returned string is owned by, and must ultimately be JS_free'd by, the
> caller.
> 
> ::: xpcom/build/XPCOMInit.cpp
> @@ +479,5 @@
> > +// The current default locale can be detected in JavaScript by calling
> > +// `Intl.DateTimeFormat().resolvedOptions().locale`
> > +namespace {
> > +
> > +#define USE_US_ENGLISH_LOCALE_PREF "javascript.use_us_english_locale"
> 
> static const char USE_US_ENGLISH_LOCALE_PREF[] = "...";
> 
> @@ +501,5 @@
> > +static
> > +void UseUSEnglishLocalePrefChangedCallback(const char* /* pref */, void* /* closure */) {
> > +  // Get a pointer to the default JS Runtime.
> > +  JSRuntime* rt = GetRuntime();
> > +  if (rt) {
> 
> Needing "a" JSRuntime is a tell.  There are multiple runtimes: one per
> thread running JS code.  Setting a runtime's default locale only affects
> code on one thread.  You're fixing the main thread but leaving workers
> broken.  Worse, the C locale stuff *is* a set-once (not per-thread) thing. 
> And setlocale isn't even threadsafe!

Very good point.

> Solving this all -- particularly scaling across all threads, given the
> preference service is main-thread-only! -- seems rather horrific.  I think
> we should abandon having a live preference and just determine the locale and
> do a setlocale() at startup, then consistently apply it to all runtimes.

That sounds like a good plan to me.

> So:
> 
> 1. In XPCOM startup code:
> * save a copy of the system locale
> * query the preference
> * if the preference says to use "en-US", setlocale() it and set the saved
> locale
> 2. In xpc_LocalizeRuntime, in js/xpconnect/src/XPCLocale.cpp:
> * set the default locale on each runtime, on the basis of the saved locale
> 3. In XPCOM shutdown code:
> * free the saved-aside locale
> 
> ...wait.  Oh gag.  xpc_LocalizeRuntime is called only on the main thread. 
> We don't even specify the default locale on workers, so everything goes to
> pieces.  La la la la la.  Filed as bug 1174386.  You may well want to wait
> til that's landed to continue here.  :-\  Or perhaps take it yourself,
> although I concede this is a bit yak-shavy for what probably seemed like a
> "simple" change to you.

Nah, I know even easy things are hard. :) I'll have a look at Bug 1174386 when I have a chance.

(Marking as needsinfo for testing question above.)
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 5

4 years ago
For reference, here's a link that tracks the latest version of the Tor Browser patch:
https://torpat.ch/5926
(Assignee)

Comment 6

3 years ago
Here's a version of the patch with a static pref (which applies at startup). It's much simpler than the previous patch, which is good. Unfortunately, I don't see a good way to write a unit test for this pref, given that the system locale on test machines is likely to already be "C" or "en-US".
Attachment #8617620 - Attachment is obsolete: true
Attachment #8685669 - Flags: review?(jwalden+bmo)
Comment on attachment 8685669 [details] [diff] [review]
0001-Bug-867501-Pref-allows-JS-locale-to-be-set-to-US-Eng.patch

Review of attachment 8685669 [details] [diff] [review]:
-----------------------------------------------------------------

Seems plausible to me, but I don't necessarily know this code super-well, so let's have someone XPCOMish look, too.
Attachment #8685669 - Flags: review?(khuey)
Attachment #8685669 - Flags: review?(jwalden+bmo)
Attachment #8685669 - Flags: review+
(In reply to Arthur Edelstein from comment #4)
> Good point. How do we do this, given that test machines are en-US? Is there
> any way to set environment variables before startup?

If we had fixed this in the JS engine itself, js/src/jsapi-tests would stand ready to address this.  But  (maybe even correctly) we didn't, so we don't have that option, exactly.

But we do have Mozilla's C++ tests system available, to do somewhat similar things in.  Take a look at the C++ files in xpcom/tests/ for how to go about this.  I think that should give you enough rope to test this, if you do the setenv *before* setting up XPCOM.

> In fact, toLocaleString, toDateString, toTimeString and toLocaleFormat
> locales are independent of JS_[Get/Set]DefaultLocale. Instead the format
> those calls use must be get and set using the setlocale() function.

I believe it depends on how you build.  If you build with --with-intl-api (the default everywhere except Firefox for Android), the various to*String methods use Intl behavior under the hood.  See js/src/builtin/Date.js, for example.  toLocaleFormat is just the nonstandard red-headed stepchild that does its own thing.
Flags: needinfo?(jwalden+bmo)
Your code to check this preference is executed before we load the user's profile-specific preferences. Meaning that it will only ever see the value that is shipped in the built in preferences. If that is acceptable, why not just use a compile-time define here?
Flags: needinfo?(arthuredelstein)
Comment on attachment 8685669 [details] [diff] [review]
0001-Bug-867501-Pref-allows-JS-locale-to-be-set-to-US-Eng.patch

r-, on the premise that the patch is going to change after my question is answered.
Attachment #8685669 - Flags: review?(khuey) → review-
(Assignee)

Comment 11

3 years ago
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) (Away 11/25 - early december) from comment #9)
> Your code to check this preference is executed before we load the user's
> profile-specific preferences. Meaning that it will only ever see the value
> that is shipped in the built in preferences. If that is acceptable, why not
> just use a compile-time define here?

Thank you for noticing that mistake. Here's a revised patch where the code in question is executed after user prefs are loaded.

try server results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cd70ce581dc
Attachment #8685669 - Attachment is obsolete: true
Flags: needinfo?(arthuredelstein)
Comment on attachment 8693346 [details] [diff] [review]
0001-Bug-867501-Pref-allows-JS-locale-to-be-set-to-US-Eng.patch

Review of attachment 8693346 [details] [diff] [review]:
-----------------------------------------------------------------

With the caveat that I haven't tested this, r=me.
Attachment #8693346 - Flags: review+
(Assignee)

Comment 13

3 years ago
Turns out this patch is broken when e10s is activated. So I need to work on it some more.
(Assignee)

Comment 14

3 years ago
Here's a new version of the patch, where, if the pref is activated, a default "C" locale is applied to both parent (chrome) and child (content) processes.

try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebd4a515e678
Attachment #8693346 - Attachment is obsolete: true
Attachment #8699697 - Flags: review?(khuey)
(Assignee)

Comment 15

3 years ago
Thanks for the review!
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
No longer depends on: 1174386
(Assignee)

Comment 18

3 years ago
Sorry for the bustage. I forgot to include a header file, locale.h, and some platforms consequently failed to build.

I'm attaching a revised patch with the requisite header file.

Here's the try result testing all platforms after I added the missing #include:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=41325eff419c
Attachment #8699697 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Wondering if we want (or have) a 'privacy-hardening' doc on MDN. Adding dev-doc-needed here to decide this when this is landing.
Keywords: dev-doc-needed

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/52d635f2b33d
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Updated

3 years ago
Assignee: nobody → arthuredelstein
(Assignee)

Updated

3 years ago
Summary: Date.toLocaleFormat exposes OS locale → Date.toLocaleFormat exposes OS locale (Tor 13019)
Blocks: 1260929
You need to log in before you can comment on or make changes to this bug.