Opened 5 years ago

Closed 5 years ago

#13027 closed defect (fixed)

Make WebWorkers use spoofed navigator.* useragent values

Reported by: mikeperry Owned by: gk
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: ff31-esr, tbb-easy, tbb-fingerprinting, TorBrowserTeam201410Easy, MikePerry201410R
Cc: boklm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We spoof the navigator values through various general.useragent.override prefs. However, this object is now exposed to WebWorkers too, which may or may not be listening to these new prefs (because WebWorkers are special threads and have restricted access to much of XPCOM).

https://bugzilla.mozilla.org/show_bug.cgi?id=925847

Child Tickets

Attachments (2)

worker_test.html (917 bytes) - added by gacar 5 years ago.
worker_test.js (348 bytes) - added by gacar 5 years ago.

Download all attachments as: .zip

Change History (33)

comment:1 Changed 5 years ago by mikeperry

Keywords: tbb-easy tbb-testcase added

This should be easy to test using some JS that inspects these values inside a JS WebWorker thread.

comment:2 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201409Easy added; TorBrowserTeam201409 removed

comment:3 Changed 5 years ago by gacar

I'd like to work on this. I'll be writing the simple test that Mike has mentioned.

comment:4 Changed 5 years ago by gacar

It seems there's a problem here: WebWorker side has access to unspoofed values of appVersion and platform. I tested with the attached html + js on both 32bit and 64bit Linux.

platform (32bit Linux) = Linux i686 (should be Win32)
platform (64bit Linux) = Linux x86_64 (should be Win32)
appVersion = 5.0 (X11) - (should be 5.0 (Windows))

There are only four properties available in navigator object on the worker side and two of them (userAgent and appName) match the spoofed values.

It suspect the sweetspot is Create method in dom/workers/Navigator.cpp(1), which calls STRING_TO_JSVAL(2) to populate navigator properties:
1: https://mxr.mozilla.org/mozilla-esr24/source/dom/workers/Navigator.cpp#78
2: https://mxr.mozilla.org/mozilla-esr24/source/js/public/Value.h#1778

Changed 5 years ago by gacar

Attachment: worker_test.html added

Changed 5 years ago by gacar

Attachment: worker_test.js added

comment:5 Changed 5 years ago by mikeperry

Keywords: tbb-testcase removed
Summary: Ensure WebWorkers see spoofed navigator.* valuesMake WebWorkers use spoofed navigator.* useragent values

comment:6 Changed 5 years ago by gacar

It seems worker calls doesn't listen to prefs because they are treated as chrome calls.

I debugged NS_GetNavigator* methods that are used to initialize WorkerNavigator and confirmed that IsCallerChrome() returns true for worker scripts (check esp. 1, 2).

This also explains why useragent matches between worker and global context: there's no
IsCallerChrome() check in nsHttpHandler.cpp where useragent value comes from.

comment:7 Changed 5 years ago by gacar

Also I think this is a genuine FF bug since webworker spec says:

"navigator[...] must return an instance of the WorkerNavigator interface, which represents the identity and state of the user agent (the client)"

In Chromium, device emulation works in the same way for worker and global scripts (they both read the same spoofed values).

comment:8 in reply to:  7 Changed 5 years ago by gk

Replying to gacar:

Also I think this is a genuine FF bug since webworker spec says:

Do you mind filing one and adding me?

comment:10 Changed 5 years ago by boklm

Cc: boklm added

comment:11 Changed 5 years ago by gacar

Probably this bug is not scary as we thought, since IsCallerChrome() returns true when WorkerNavigator strings are initialized and cached - before the worker starts running.

So this may be limited to initialization step (RuntimeService::RegisterWorker), which is hopefully easier to fix and less scary than workers running with chrome privileges. I updated the relevant Mozilla bug.

comment:12 Changed 5 years ago by gacar

comment:13 Changed 5 years ago by gk

gacar: Do you want to backport the patch and write a test? Or should I put that on my plate (as I am doing both already atm)?

comment:14 in reply to:  13 ; Changed 5 years ago by gacar

Replying to gk:

gacar: Do you want to backport the patch and write a test? Or should I put that on my plate (as I am doing both already atm)?

Please go ahead.

comment:15 in reply to:  14 Changed 5 years ago by gk

Replying to gacar:

Replying to gk:

gacar: Do you want to backport the patch and write a test? Or should I put that on my plate (as I am doing both already atm)?

Please go ahead.

Just to be clear "as I am doing both already atm" was not referring to this bug but to #13186 and #13024. I have not started working on this bug yet. :)

comment:16 Changed 5 years ago by gacar

Ah, now I see :)

I won't be having any time until late next week. I'll be happy to give a try after that.

Actually, that patch seemed to cause some crashes at Try, maybe we should wait until it gets fixed first.

comment:17 in reply to:  16 ; Changed 5 years ago by gacar

Actually, that patch seemed to cause some crashes at Try, maybe we should wait until it gets fixed first.

It seems we need apply an additional patch (along with the one from 1062920):
https://bugzilla.mozilla.org/show_bug.cgi?id=1060621

See, https://bugzilla.mozilla.org/show_bug.cgi?id=1062920#c34

comment:18 in reply to:  17 Changed 5 years ago by gk

Owner: changed from tbb-team to gk
Status: newassigned

Replying to gacar:

Actually, that patch seemed to cause some crashes at Try, maybe we should wait until it gets fixed first.

It seems we need apply an additional patch (along with the one from 1062920):
https://bugzilla.mozilla.org/show_bug.cgi?id=1060621

See, https://bugzilla.mozilla.org/show_bug.cgi?id=1062920#c34

Yes, I did that and the result is https://gitweb.torproject.org/user/gk/tor-browser.git/commit/d1f812c14245eb008ed4aeb4221417ad41ef1d0a and https://gitweb.torproject.org/user/gk/tor-browser.git/commit/f7ab149ce1b0ab2e4c5eca98460704bf59781536 it is untested yet and I still need to write a test.

comment:19 Changed 5 years ago by gk

While my fixup gets the code compiled it does not solve this bug. Thus, we need a better backport here first.

comment:20 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201410 added

comment:21 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201410Easy added; TorBrowserTeam201409Easy removed

comment:22 Changed 5 years ago by mikeperry

Keywords: TorBrowserTeam201410 removed

comment:23 Changed 5 years ago by gk

Keywords: MikePerry201410R added

bug_13027 in my tor-browser repo has the backported patch. I am working on the test right now.

comment:24 Changed 5 years ago by gk

Status: assignedneeds_review

comment:25 Changed 5 years ago by mikeperry

It looks like the unit tests in that branch have an unresolved conflict still in them, in dom/workers/test/mochitest.ini.

The rest of it looks ok at a glance, but I will review it deeper in a bit.

comment:26 in reply to:  25 Changed 5 years ago by gk

Replying to mikeperry:

It looks like the unit tests in that branch have an unresolved conflict still in them, in dom/workers/test/mochitest.ini.

Oops, that's embarrassing. See bug_13027_v2 in my tor-browser repo which should have this fixed.

comment:27 Changed 5 years ago by gk

bug_13027_fixup in my tor-browser repo has the fixup.

comment:28 Changed 5 years ago by gk

Seems the bug is triggerable by a Mozmill test without Workers being involved at all:

    "use strict";
     
    Cu.import("resource://gre/modules/Services.jsm");
     
    const TEST_URL = "https://www.mediawiki.org/wiki/MediaWiki";
     
    var setupModule = function(aModule) {
      aModule.controller = mozmill.getBrowserController();
    }
     
    function load_page(controller, url) {
      var retry = 4;
      while (retry-- > 0) {
        var success = true;
        controller.open(url);
        try {
          controller.waitForPageLoad(10000);
        } catch(e) {
          success = false;
        }
        if (success) {
          return true;
        }
      }
      controller.open(url);
      return controller.waitForPageLoad(50000);
    }
     
    var testNavigatorPlatform = function() {
      Services.prefs.setCharPref("general.platform.override", "Win32");
      load_page(controller, TEST_URL);
      let nav = controller.tabs.activeTab.defaultView.navigator;
      for (var prop in nav) {
        if (prop === "platform")
          dump("Test result for " + prop + " is: " + nav[prop]);
      }
    } 

A Tor Browser reports on my 64 bit Linux "Linux x86_64". The same holds for the current Firefox release (32.0.3) and the vanilla ESR 31 but not for Firefox nightly/aurora/beta. That's pretty weird but might help us writing a proper test verifying it is indeed fixed.

comment:29 in reply to:  28 Changed 5 years ago by gk

Replying to gk:

Seems the bug is triggerable by a Mozmill test without Workers being involved at all:

Yes, and the Mozmill issues got fixed by bug 1062920. This, however, was *not* the case for the branch backports and is one of the reasons we needed to adapt the code for the ESR 31 backport. What a mess.

comment:30 Changed 5 years ago by gk

The test I have is broken due to 5d2a421005dac5b4ea02181a8aced550980032d7 which means one of our non-default preference values interferes here.

comment:31 in reply to:  30 Changed 5 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to gk:

The test I have is broken due to 5d2a421005dac5b4ea02181a8aced550980032d7 which means one of our non-default preference values interferes here.

I can't reproduce anymore. :( But still the Mozmill issue is still one with our custom backport. The test needs to wait for ESR 38 (if no one else is stepping up with one meanwhile). Which is #13496.

Note: See TracTickets for help on using tickets.