1062920 - WorkerNavigator strings should honor general.*.override prefs

Status: VERIFIED FIXED

(Core :: DOM: Workers, defect)

Description

[acargunes]

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/37.0.2062.94 Chrome/37.0.2062.94 Safari/537.36

Steps to reproduce:

1. Add a pref to override navigator properties, such as general.platform.override=Win64
2. Check the navigator.platform value in a Web Worker, or visit this site: https://securehomes.esat.kuleuven.be/~gacar/dev/worker_test.html


Actual results:

Web Worker script ignores the general.platform.override pref (Win64) and gets access to original value (e.g. Linux i686). 


Expected results:

The WorkerNavigator should honor general.*.override prefs.

Comment 1

[acargunes]

We came across this while testing Tor Browser's fingerprinting defenses in worker context:
https://trac.torproject.org/projects/tor/ticket/13027

It turned out, worker scripts ignore general.*.override prefs since workers are treated as chrome callers in NS_GetNavigator* methods. See, https://trac.torproject.org/projects/tor/ticket/13027#comment:6

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Andrea, want to look at this?

Comment 3

[Andrew McCreight [:mccr8]]

This sounds pretty bad...

Comment 4

[Daniel Veditz [:dveditz]]

This is likely sec-critical if this means you can XHR() on the user's harddisk. Is the Worker really chrome powered or is it just the NS_Navigator methods detecting that incorrectly? (freddyb was going to work up a quick test.)

Comment 5

[Daniel Veditz [:dveditz]]

The Version field here says it's in "32 Branch", but mike perry sent mail saying it was found in ESR-31. I assume you did not have this problem in the ESR-24-based TBB. When did we break this?

Comment 6

[Andrew McCreight [:mccr8]]

XHR itself uses a separate implementation for the main thread and for workers.  The worker implementation does not seem to use nsContentUtils::IsCallerChrome(), but instead calls IsChromeWorker() on the worker private.  Offhand, I would guess the biggest danger is going to be in formerly-mainthread-only code that we've started calling from workers.  I don't think older worker code is going to call IsCallerChrome().

Comment 7

[Andrew McCreight [:mccr8]]

Do you have any thoughts on this, Bobby?  I know you've poked around IsCallerChrome in the past.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I don't think we should have a bug that combines both the easily-fixable navigator strings and the much more architectural IsCallerChrome problem...

Comment 9

[Andrew McCreight [:mccr8]]

That's a good point.  Plus it sounds like fixing IsCallerChrome() won't even fix the other problem because it is trying to access preferences off the main thread.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> because it is trying to access preferences off the main thread.

No, the RuntimeService caches all these strings when the first worker is created when it's guaranteed to be on the main thread.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(IsCallerChrome blows up if it's ever called off the main thread)

Comment 12

[acargunes]

(In reply to Daniel Veditz [:dveditz] from comment #5)
> The Version field here says it's in "32 Branch", but mike perry sent mail
> saying it was found in ESR-31. I assume you did not have this problem in the
> ESR-24-based TBB. When did we break this?

The bug is present on the following:
* ESR 24.0.8 based TBBs
* FF ESR31
* plain FF V32 

TBB 3.0a1 based on ESR 17.0.6 doesn't have the mismatch between Navigator and WorkerNavigator - the test linked above passes. 

I guess it's because it uses IsCallerTrustedForRead instead of IsCallerChrome:
https://mxr.mozilla.org/mozilla-esr17/source/dom/base/Navigator.cpp#1491

Comment 13

[Frederik Braun [:freddy]]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> This is likely sec-critical if this means you can XHR() on the user's
> harddisk. Is the Worker really chrome powered or is it just the NS_Navigator
> methods detecting that incorrectly? (freddyb was going to work up a quick
> test.)

I didn't manage to come up with a working test.
The impact of the stated bug is still unclear to me, because I couldn't find any evidence of the XHR object in Workers having higher privileges.

Comment 14

[acargunes]

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #10)
> No, the RuntimeService caches all these strings when the first worker is
> created when it's guaranteed to be on the main thread.

With this comment and going through the code again, now I think this may be a partially false alarm.

What we know is, when loading (and caching) worker navigator strings, RuntimeService:::RegisterWorker calls NS_Navigator getters as chrome:
https://mxr.mozilla.org/mozilla-esr31/source/dom/workers/RuntimeService.cpp#1350

But successive calls (e.g. from the worker content) to WorkerNavigator properties do not call NS_Navigator methods, just return the cached strings.

So the problem may be limited to worker initialization part.

(In reply to Daniel Veditz [:dveditz] from comment #5)
> When did we break this?
Assuming breaking means "ignoring general.*.override prefs for workernavigator", it's FF 19:
https://hg.mozilla.org/mozilla-central/rev/5b6d7523e355

Versions I tested:
ESR 18: good
FF 18: good
FF 19: bad
ESR 24: bad

Comment 15

[Bobby Holley (:bholley)]

See comment 8. I also commented in the security-group thread.

Comment 16

[Andrew McCreight [:mccr8]]

It sounds like this is not a critical issue.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: overwrite.patch

Comment 18

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Can we unhide this?

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8491496 [details] [diff] [review]
overwrite.patch

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

I don't really think live-updating this based on pref changes is worth the effort. Can't we just keep the get-it-once-at-startup behavior and make sure we collect both the chrome and non-chrome versions? Then expose whichever based on UsesSystemPrincipal()?

Comment 20

[Andrea Marchesini [:baku]]

> I don't really think live-updating this based on pref changes is worth the
> effort. Can't we just keep the get-it-once-at-startup behavior and make sure
> we collect both the chrome and non-chrome versions? Then expose whichever
> based on UsesSystemPrincipal()?

Many no-restart addon can play with these preferences. I think we should support this life-updating feature. This patch doesn't update the values for existing workers but just for new ones.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: overwrite.patch

A better approach.

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8491552 [details] [diff] [review]
overwrite.patch

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

::: dom/workers/test/test_bug1062920.html
@@ +4,5 @@
> +-->
> +<!DOCTYPE HTML>
> +<html>
> +<head>
> +  <title>Test for URLSearchParams object in workers</title>

this title is already changed. I don't want to upload a new patch just for this.

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8491552 [details] [diff] [review]
overwrite.patch

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

::: dom/workers/RuntimeService.cpp
@@ +1054,5 @@
> +  const nsAdoptingString& override =
> +    mozilla::Preferences::GetString(PREF_GENERAL_APPNAME_OVERRIDE);
> +
> +  RuntimeService* rts = RuntimeService::GetService();
> +  if (rts) {

match the existing code and name this "runtime"

Also we should probably just assert that we have the service here, but that can happen in another bug.

Comment 24

[Andrea Marchesini [:baku]]

https://tbpl.mozilla.org/?tree=Try&rev=623b6854ca64

Comment 25

[Andrew McCreight [:mccr8]]

I'm just going to mark this sec-other, as it seems like there's no big security issue here.

Comment 26

[Andrea Marchesini [:baku]]

Attachment: overwrite.patch

Comment 27

[Andrea Marchesini [:baku]]

Comment on attachment 8493580 [details] [diff] [review]
overwrite.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? There is no crash in this issue. But, yes, it easy to reproduce the problem.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? yes

Which older supported branches are affected by this flaw? I don't know.

If not all supported branches, which bug introduced the flaw? bug 925849

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

How likely is this patch to cause regressions; how much testing does it need?

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8493580 [details] [diff] [review]
overwrite.patch

Clearing sec-approval. As a "sec-other" rated issue, this doesn't need approval for checkin on mozilla-central.

Comment 29

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b831a03d9c

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b0b831a03d9c

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Are we going to want to uplift this to any branches?

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8493580 [details] [diff] [review]
overwrite.patch

Approval Request Comment
[Feature/regressing bug #]: 925847
[User impact if declined]: Any overridden prefs for platform, appName and appVersion will be ignored in workers.
[Describe test coverage new/current, TBPL]: https://tbpl.mozilla.org/?tree=Try&rev=623b6854ca64
[Risks and why]: The patch is not complex and it has good mochitests.
[String/UUID change made/needed]: none

Comment 33

[Ryan VanderMeulen [:RyanVM]]

I attempted to rebase this and bug 1069401 for aurora/beta uplift, but am hitting mochitest-4 leaks :(

Aurora: https://tbpl.mozilla.org/?tree=Try&rev=c67f19a993c0
Beta:   https://tbpl.mozilla.org/?tree=Try&rev=2a045fbb9820

Looks like you'll have to take it from here, Andrea.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Bug 1060621 is the reason for the leaks. Try is green with that applied as well.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17a1be541497
https://hg.mozilla.org/releases/mozilla-beta/rev/6d53cfba12f0

Comment 36

[Georg Koppen]

This is still not fixed. Steps to reproduce:

1) Take the latest nightly on a Linux system.
2) Start it with a new profile.
3) If getting ask whether to enable e10s mode, choose "Yes". (Enabling e10s is the crucial step here. Without it the bug is not visible.)
4) After restart open about:config and create a new pref "general.platform.override" with a value of "Win32"
5) Visit https://securehomes.esat.kuleuven.be/~gacar/dev/worker_test.html.
6) Result: The test fails as the scripts detects the Linux platform.

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

e10s isn't officially supported yet.  File a new bug please.

Comment 38

[Andrea Marchesini [:baku]]

and assign that bug to me please.

Comment 39

[Georg Koppen]

Done. This is bug 1078163.

Comment 40

[Daniel Veditz [:dveditz]]

don't think this needs to be hidden if the Tor tickets in comment 1 are public

Comment 41

[Kamil Jozwiak [:kjozwiak]]

Used the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-08-03-02-02-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-08-00-40-04-mozilla-aurora/

OSX 10.9.5 (attempted on both m-c and aurora)

- PASS - - navigator.platform: Win64 (loading the page right after changing the pref)
- FAIL - navigator.platform: MacIntel != Win64 (after the browser is closed/re-opened)

Win 8.1 x64 VM: (attempted on both m-c and aurora)

- PASS - navigator.platform: MacIntel (loading the page right after changing the pref)
- FAIL - navigator.platform: Win32 != MacIntel (after the browser is closed/re-opened)

Before going through the other channels and OS's, is this still an issue? Should the preference be honoured even after fx as been closed/re-opened?

Comment 42

[Andrea Marchesini [:baku]]

I think this issue has been fixed bug bug 1078163. Can you try again when that patch is landed to m-c?

Comment 43

[Kamil Jozwiak [:kjozwiak]]

Great, once that patch lands I'll give it another go and post the results here. Thanks Andrea!

Comment 44

[Kamil Jozwiak [:kjozwiak]]

Used the following build for verification:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-09-03-02-01-mozilla-central/

For the following test cases, I ensured that the new "general.platform.override" value is kept when:

- closing/re-opening fx
- opening the website from comment #0 under regular/private and e10s windows/tabs
- with e10s enabled via about:preferences
- value still correctly being displayed under about:config

I also reproduced the original issue using the following build:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-09-09-03-02-07-mozilla-central/

Once reproduced, I updated fx and ensured that the above test cases worked without any issues.

Tested on the following OS's:

- OSX 10.9.5 x64 - PASSED
- Win 8.1 x64 (VM) - PASSED
- Ubuntu 14.0.4.1 x64 (VM) - PASSED

Comment 45

[Kamil Jozwiak [:kjozwiak]]

This still doesn't work under fx34 and fx33 and appears to rely on the fix from bug #1078163 (waiting for a reply in that bug to confirm). I'm not sure what the correct process for the flags are as this looks like it's not making it into fx33.

Comment 46

[Lawrence Mandel [:lmandel] (use needinfo)]

If this issue is still present in Firefox 34, we should change the status flag to affected and figure out why the uplift didn't fix the issue.

Comment 47

[Andrea Marchesini [:baku]]

I asked to have bug 1078163 in aurora and beta. This should fix the issue in ff33/34.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Restoring the status flags. Bug 1078163 is a separate issue that can be tracked on its own.

Comment 49

[Matt Wobensmith [:mwobensmith][:matt:]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48)
> Restoring the status flags. Bug 1078163 is a separate issue that can be
> tracked on its own.

Bug 1078163 is marked "wontfix" for Fx33.

However, this bug here is not fixed in Fx33 as of comment 45. Would it make more sense to mark the status flag here for Fx33 as "wontfix" also?

Comment 50

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
- https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-13-00-40-02-mozilla-aurora/

I went through the same test cases that are listed under comment #44 without any issues. I also verified bug #1078163 using the test cases from bug #1078163 comment #8.

Lawrence, as per comment #49, should fx33 be marked as "wontfix"??

Comment 51

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Matt Wobensmith from comment #49)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48)
> > Restoring the status flags. Bug 1078163 is a separate issue that can be
> > tracked on its own.
> 
> Bug 1078163 is marked "wontfix" for Fx33.
> 
> However, this bug here is not fixed in Fx33 as of comment 45. Would it make
> more sense to mark the status flag here for Fx33 as "wontfix" also?

Even though we did land a fix for this bug on 33, given that the issue has not been resolved in 33, I do think it makes sense to mark 33 as wontfix.