Opened 3 years ago

Last modified 2 weeks ago

#14205 assigned task

Closely review all uses of IsCallerChrome() for e10s

Reported by: mikeperry Owned by: mcs
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-fingerprinting, tbb-e10s, tbb-rebase, ff52-esr, tbb-7.0-must, TorBrowserTeam201712
Cc: brade, arthuredelstein, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

A lot of our fingerprinting patches depend upon the accuracy is nsContentUtils::IsCallerChrome() to determine if it is content window or browser chrome accessing fingerprinting information.

IsCallerChrome() kind of scares me, and has had issues for unexpected contexts like WebWorkers (See #13027).

We should keep a close eye on this as we transition to e10s support post FF38, as who knows that the child/parent context relationship changes may do to various codepaths.

Child Tickets

TicketStatusOwnerSummaryComponent
#16715closedmcsuse ThreadsafeIsCallerChrome() instead of IsCallerChrome()Applications/Tor Browser

Change History (20)

comment:1 Changed 3 years ago by gk

Cc: gk added

comment:2 Changed 3 years ago by gk

We might want to think about false negatives as well, see #13439. They might not be as problematic but still it bothers me that IsCallerChrome() is not reliable in this regard either.

comment:3 Changed 3 years ago by arthuredelstein

I noticed a new call has been added: nsContentUtils::ThreadsafeIsCallerChrome(), which checks if the caller is a Chrome web worker. I think dangers with this approach remain, but at least it appears to be an improvement.

comment:4 in reply to:  3 Changed 3 years ago by arthuredelstein

Replying to arthuredelstein:

I noticed a new call has been added: nsContentUtils::ThreadsafeIsCallerChrome(), which checks if the caller is a Chrome web worker. I think dangers with this approach remain, but at least it appears to be an improvement.

Oops, this is not new. Not sure if it is going to work or not.

comment:5 Changed 2 years ago by mcs

Cc: brade arthuredelstein added
Owner: changed from tbb-team to mcs
Status: newassigned

comment:6 Changed 2 years ago by arthuredelstein

FWIW, I tried to minimize IsCallerChrome() usage in https://hg.mozilla.org/mozilla-central/rev/3abb08512b24, which is a port of #13016, #13025, #5856, #4755, and #2875. In that version, IsCallerChrome() is only being used in dom/events/Event.cpp.

I suppose we could backport this patch to ESR38 and use it to replace the corresponding tor-browser.git patches, if that seems safer. There are probably further uses of IsCallerChrome() in other tor-browser.git patches, though.

comment:7 Changed 2 years ago by mcs

Kathy and I spent some time analyzing our use of nsContentUtils::isCallerChrome() and also thinking about when it makes sense to use it and when it does not.

Currently, we use it in the following Tor Browser patches:

1c671d687504e1886587f86c176248b6367bf7ac (#13016 - Hide CSS -moz-osx-font-smoothing)
1df6eeba14da4e1924e3576ce1103e2c56d786d5 (#6253 - Add canvas image extraction prompt)
797a6165050e97c3cdd700e342aea059e8afe895 (#4755 - Return window coords for mouse screenX/Y)
8d2b33f78f325cc50ebbe1e2a6657254bacdd9fc (#15646 - Prevent keyboard layout fingerprinting)

In each of the above cases, content JavaScript is always involved when we want to block access to fingerprinting vectors, so isCallerChrome() is OK to use. Actually, it would be better to use nsContentUtils::ThreadsafeIsCallerChrome() instead in all cases because that call will do the right thing for web workers.

We have not yet evaluated the situation when electrolysis is enabled.

An alternative to IsCallerChrome() and ThreadsafeIsCallerChrome() is to use calls such as nsDocShell::GetIsContent() and nsPresContext::IsChrome(). Those methods return a value that is not based on who is asking (i.e., not based on what is in the call stack); the value returned is based on the context in which the document was created. The problem with this approach is that if privileged code is manipulating a content document, we may want to allow access to otherwise blocked info... in which case ThreadsafeIsCallerChrome() is a better choice.

For TB 5.0, Kathy and I think we should replace all of our IsCallerChrome() calls with ThreadsafeIsCallerChrome() but otherwise leave things as-is.

comment:8 in reply to:  7 Changed 2 years ago by mcs

Replying to mcs:

For TB 5.0, Kathy and I think we should replace all of our IsCallerChrome() calls with ThreadsafeIsCallerChrome() but otherwise leave things as-is.

I filed #16715 to track this task.

comment:9 Changed 11 months ago by gk

Keywords: ff52-esr added; ff38-esr removed
Severity: Normal

Bringing this on our ESR52 radar for e10s evaluation.

comment:10 Changed 8 months ago by gk

Keywords: tbb-7.0-must added

Adding tickets to our 7.0 ticket list

comment:11 Changed 7 months ago by mcs

Looking at all of the tor-browser patches on our tor-browser-52.1.1esr-7.0-1 branch, the following patches call IsCallerChrome() , ThreadsafeIsCallerChrome(), or LegacyIsCallerChromeOrNativeCode():

commit 2c9aa3b1c278653c27a0339db89675ad89933ec9
Author: Kathy Brade <brade@…>
Date: Thu Sep 26 17:11:19 2013 -0400

Bug 6253: Add canvas image extraction prompt.

commit 331f089d6b6ba62463d8362d7ca01641a4cc92dc
Author: Arthur Edelstein <arthuredelstein@…>
Date: Mon Apr 24 08:18:25 2017 -0700

Bug 10286: Touch API fingerprinting resistance

commit 6b2e06d4dd12679056b0cefb5f57c8dd0e7ea7f1
Author: Arthur Edelstein <arthuredelstein@…>
Date: Sat Jun 13 19:27:43 2015 -0700

Bug 15646: Prevent keyboard layout fingerprinting in KeyboardEvent

commit 58d186df19450f9aef0423c71e78f6eaa17679f8
Author: Arthur Edelstein <arthuredelstein@…>
Date: Thu Apr 27 15:00:14 2017 -0700

Bug 21792: Suppress MediaError.message when privacy.resistFingerprinting = true

comment:12 Changed 7 months ago by mcs

The problem of reviewing the calls to ensure that they are only called from the tab/comtent process when e10s is enabled is definitely a challenge. Look here for an interesting comment:
https://dxr.mozilla.org/mozilla-esr52/source/dom/base/nsContentUtils.h#203

One approach would be for us to put breakpoints in IsCallerChrome() and related calls and see what the stack looks like in e10s mode when the breakpoints are hit while we exercise the code paths we care about. But we might miss something.

The Mozilla developers are working on this issue as well. See https://bugzilla.mozilla.org/show_bug.cgi?id=1316480 ("Get rid of IsCallerChrome and friends") which depends on a bunch of other bugs, many – but not all – of which have been fixed. In many cases, Mozilla wants to hide an API from regular web pages, so they can handle the issue at the webidl level. But in many cases we need to do something more subtle such as return a different, less fingerprintable result to web pages.

comment:13 Changed 7 months ago by gk

Keywords: TorBrowserTeam201706 added

comment:14 Changed 6 months ago by gk

Keywords: TorBrowserTeam201707 added; TorBrowserTeam201706 removed

Moving Tickets to July 2017.

comment:15 Changed 5 months ago by gk

Keywords: TorBrowserTeam201708 added; TorBrowserTeam201707 removed

Moving our Tickets to August.

comment:16 Changed 4 months ago by gk

Keywords: TorBrowserTeam201709 added; TorBrowserTeam201708 removed

Items for September 2017.

comment:17 Changed 3 months ago by gk

Keywords: TorBrowserTeam201710 added; TorBrowserTeam201709 removed

Items for October 2017

comment:18 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201711 added; TorBrowserTeam201710 removed

Moving tickets over to November.

comment:19 Changed 2 weeks ago by gk

Moving tickets to December 2017

comment:20 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201712 added; TorBrowserTeam201711 removed

Moving tickets to December 2017, for realz.

Note: See TracTickets for help on using tickets.