WorkerNavigator strings should honor general.*.override prefs

VERIFIED FIXED in Firefox 34

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: acargunes, Assigned: baku)

Tracking

({privacy, sec-other})

32 Branch
mozilla35
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 wontfix, firefox33 wontfix, firefox34 verified, firefox35 verified, firefox-esr31 wontfix, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.2 unaffected)

Details

(Whiteboard: [adv-main34-])

Attachments

(1 attachment, 2 obsolete attachments)

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.
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
Andrea, want to look at this?
Flags: needinfo?(amarchesini)
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
OS: Linux → All
Hardware: x86 → All
This sounds pretty bad...
Group: core-security
Summary: WebWorker content scripts treated as chrome callers → IsCallerChrome() returns true in worker content scripts
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.)
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?
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().
Do you have any thoughts on this, Bobby?  I know you've poked around IsCallerChrome in the past.
Flags: needinfo?(bobbyholley)
I don't think we should have a bug that combines both the easily-fixable navigator strings and the much more architectural IsCallerChrome problem...
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.
(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.
(IsCallerChrome blows up if it's ever called off the main thread)
(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
(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.
(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
See comment 8. I also commented in the security-group thread.
Flags: needinfo?(bobbyholley)
Summary: IsCallerChrome() returns true in worker content scripts → WorkerNavigator strings should honor general.*.override prefs
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: sec-critical
It sounds like this is not a critical issue.
Posted patch overwrite.patch (obsolete) — Splinter Review
Attachment #8491496 - Flags: review?(khuey)
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()?
> 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.
Posted patch overwrite.patch (obsolete) — Splinter Review
A better approach.
Attachment #8491496 - Attachment is obsolete: true
Attachment #8491496 - Flags: review?(khuey)
Attachment #8491552 - Flags: review?(khuey)
Blocks: 1069401
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 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.
Attachment #8491552 - Flags: review?(khuey) → review+
I'm just going to mark this sec-other, as it seems like there's no big security issue here.
Attachment #8491552 - Attachment is obsolete: true
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?
Attachment #8493580 - Flags: sec-approval?
Keywords: checkin-needed
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.
Attachment #8493580 - Flags: sec-approval?
https://hg.mozilla.org/mozilla-central/rev/b0b831a03d9c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Are we going to want to uplift this to any branches?
Flags: needinfo?(amarchesini)
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
Attachment #8493580 - Flags: approval-mozilla-beta?
Attachment #8493580 - Flags: approval-mozilla-aurora?
Flags: needinfo?(amarchesini)
Attachment #8493580 - Flags: approval-mozilla-beta?
Attachment #8493580 - Flags: approval-mozilla-beta+
Attachment #8493580 - Flags: approval-mozilla-aurora?
Attachment #8493580 - Flags: approval-mozilla-aurora+
Flags: in-testsuite+
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.
Bug 1060621 is the reason for the leaks. Try is green with that applied as well.
Flags: needinfo?(amarchesini)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
e10s isn't officially supported yet.  File a new bug please.
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
and assign that bug to me please.
Done. This is bug 1078163.
don't think this needs to be hidden if the Tor tickets in comment 1 are public
Group: core-security
Keywords: privacy
See Also: → 1078163
Whiteboard: [adv-main33-]
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?
Flags: needinfo?(amarchesini)
I think this issue has been fixed bug bug 1078163. Can you try again when that patch is landed to m-c?
Flags: needinfo?(amarchesini)
Great, once that patch lands I'll give it another go and post the results here. Thanks Andrea!
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
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.
Flags: needinfo?(lmandel)
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.
Flags: needinfo?(lmandel)
Flags: needinfo?(kamiljoz)
Flags: needinfo?(amarchesini)
I asked to have bug 1078163 in aurora and beta. This should fix the issue in ff33/34.
Flags: needinfo?(amarchesini)
Restoring the status flags. Bug 1078163 is a separate issue that can be tracked on its own.
(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?
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"??
Flags: needinfo?(lmandel)
(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.
Flags: needinfo?(lmandel)
Status: RESOLVED → VERIFIED
Whiteboard: [adv-main33-] → [adv-main34-]
You need to log in before you can comment on or make changes to this bug.