Opened 6 weeks ago

Closed 6 days ago

Last modified 6 days ago

#31740 closed defect (fixed)

Review RemoteSettings usages in esr68

Reported by: acat Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff68-esr, tbb-9.0-must-alpha, TorBrowserTeam201910R
Cc: Actual Points: 1.5
Parent ID: Points: 0.5
Reviewer: Sponsor:

Description

RemoteSettings (https://firefox-source-docs.mozilla.org/main/68.0/services/common/services/RemoteSettings.html) is now used for several features as a way to synchronize some local state (collections, sets of records) with some remote state that Mozilla controls. I did not review the protocol, but the way I understand it is that there are periodical polls to the server with an etag header (last_timestamp, returned by the server in a previous poll) which return the list of changes since that timestamp. I think this etag may be quite close to a user identifier, so Mozilla's servers could probably link together all the RemoteSettings request can get is the sequence of all 'RemoteSettings polls' for a user. Not sure how much you can guess about the user with that, though.

In any case, opening this to decide/review which ones of the usages of RemoteSettings we need or not. The RemoteSettings one is probably a subset of the requests happening in background, but I guess it's a start. I'm listing files where RemoteSettings are used. My understanding is that just calling RemoteSettings('acollectionname') will start the syncing mechanism, meaning that there will be requests polling in background 'from time to time'.

  • browser/components/newtab/{lib/ASRouter.jsm, lib/FaviconFeed.jsm, lib/PersonalityProvider.jsm, lib/SiteClassifier.jsm}
    • These should not be run if we disable Activity Stream (#31575).
  • toolkit/components/normandy/lib/RecipeRunner.jsm
    • I think this will not be run, since the RemoteSettings is created lazily when gRemoteSettingsClient is accessed, and that should not happen since we have datareporting.healthreport.uploadEnabled = false.
  • toolkit/components/search/SearchService.jsm
    • Used to update the "ignore list settings" which can be used to remotely "ignore" search engines? I think we can remove this one.
  • browser/components/preferences/browserLanguages.js
    • There is a dump for this one in services/settings/dumps/main/language-dictionaries.json. It seems to be a mapping of locale to dictionary ids. We probably have to keep this one if we want to allow changing UI language in about:preferences, via Mozilla's language packs.
  • netwerk/url-classifier/UrlClassifierSkipListService.jsm
    • If I understand it correctly, this is used for content blocking, to be able to skip some steps ("features") for specific urls. I guess to be able to fix antitracking breakage remotely? We should not need this one if we don't plan to make it possible to enable content blocking/mozilla's antitracking.
  • security/manager/ssl/RemoteSecuritySettings.jsm
    • Intermediate certificate preloading. There is #30682 to decide what to do with this.

Finally some blocklists, I think some of them we already have in esr60, but not all? Not completely sure, but I would say it's better to keep these?

  • services/common/blocklist-clients.js
    • onecrl (already in esr60).
    • pinning blocklist (related to HPKP? not sure if it's in esr60).
  • toolkit/mozapps/extensions/Blocklist.jsm (these were already in esr60)
    • extensions blocklist
    • plugins blocklist
    • gfx blocklist

Child Tickets

Change History (9)

comment:1 Changed 5 weeks ago by pili

Points: 0.5

comment:2 Changed 3 weeks ago by gk

Keywords: tbb-9.0-must-alpha added

comment:3 Changed 3 weeks ago by gk

While looking over closed bugs I stumbled over https://bugzilla.mozilla.org/show_bug.cgi?id=1440022, which might be relevant here.

comment:4 Changed 3 weeks ago by pili

Keywords: TorBrowserTeam201910 added

comment:5 Changed 3 weeks ago by pili

Keywords: TorBrowserTeam201909 removed

comment:6 in reply to:  3 Changed 9 days ago by acat

Replying to gk:

While looking over closed bugs I stumbled over https://bugzilla.mozilla.org/show_bug.cgi?id=1440022, which might be relevant here.

The "global broadcast"/real time updates API, should not be enabled as long as dom.push.enabled=false. This is because PushService is only initialized in one place: https://searchfox.org/mozilla-esr68/rev/4fc15df791ad4d3ceaf1a958af2bfc1252433ca8/dom/push/PushComponents.jsm#22. And https://searchfox.org/mozilla-esr68/rev/4fc15df791ad4d3ceaf1a958af2bfc1252433ca8/dom/push/PushBroadcastService.jsm uses PushService but it does not initialize it, just adds some listeners which will never be used.

dom.push.enabled is false in esr68 desktop, but not on mobile AFAIK. We do have pref("dom.push.serverURL", ""); which should do the trick, but we might want to have also pref("dom.push.enabled", false) just in case.

comment:7 Changed 7 days ago by acat

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed
Status: newneeds_review

To give an idea about what these remote-settings requests look like. Periodically, or responding to some events (e.g. creating a RemoteSettings(...) client for a new collection), there will be a GET request to https://firefox.settings.services.mozilla.com/v1/buckets/monitor/collections/changes/records, used to obtain a list of available "collections" and metadata about them (https://searchfox.org/mozilla-esr68/rev/4fc15df791ad4d3ceaf1a958af2bfc1252433ca8/services/settings/Utils.jsm#57). Each entry looks like:

id: 8da7db1e-dffb-18c9-2efe-0e9d7459a0f4
last_modified: 1571184016986
bucket: main
collection: normandy-recipes
host: firefox.settings.services.mozilla.com

For each collection (distinct RemoteSettings("...") call), depending on the corresponding last_modified value returned in the previous request another request might be performed to retrieve the records of that collection (possibly only the ones that changed since some timestamp). For example, for onecrl collection https://firefox.settings.services.mozilla.com/v1/buckets/security-state/collections/onecrl?_expected=1568310941289 would be fetched (with possibly different parameters).

One concern is that the different parameters (etag, timestamps...) might be leaking enough info about the user state that it allows linking together requests done over time as belonging to the same user. In principle, the request parameters depend on the values returned in previous responses, and these seem not to change very often. I did not do a deep analysis, but I feel like we would not lose too much by doing the same requests without parameters (as if there was no previous state in the browser). I don't see the responses being so big, nor the requests done so often. But this would probably require a bit more time to make sure the changes are not breaking the RemoteSettings functionality. Perhaps on a different ticket?

For now, I think we can disable the RemoteSettings("...") calls that we do not need, if only for sparing some unnecessary requests. The currently active RemoteSettings instances are:

  • toolkit/components/search/SearchService.jsm (hijack-blocklists)
    • I think we don't want this one, it allows mozilla to blacklist search extensions.
  • browser/components/preferences/browserLanguages.js (language-dictionaries):
    • I think we need this one if we allow changing language via about:preferences.
  • netwerk/url-classifier/UrlClassifierSkipListService.jsm (url-classifier-skip-urls):
    • We don't need this one until we enable enhanced tracking protection (content blocking).
  • services/common/blocklist-clients.js (onecrl, pins):
    • OneCRl and certificate pinning blocklist? I think we want to have these.

Note that the previously mentioned RemoteSettings instances in toolkit/mozapps/extensions/Blocklist.jsm are actually currently disabled because of extensions.blocklist.useXML = true, see https://trac.torproject.org/projects/tor/ticket/16931#comment:8.

So here is a patch disabling hijack-blocklists and url-classifier-skip-urls: https://github.com/acatarineu/tor-browser/commit/31740. If it's ok to disable the latter, we could update #30939, as we will need to reenable if we decide to enable Firefox Enhanced Tracking Protection.

comment:8 in reply to:  7 Changed 6 days ago by gk

Resolution: fixed
Status: needs_reviewclosed

Replying to acat:

Thanks for the detailed analysis, really appreciated!

To give an idea about what these remote-settings requests look like. Periodically, or responding to some events (e.g. creating a RemoteSettings(...) client for a new collection), there will be a GET request to https://firefox.settings.services.mozilla.com/v1/buckets/monitor/collections/changes/records, used to obtain a list of available "collections" and metadata about them (https://searchfox.org/mozilla-esr68/rev/4fc15df791ad4d3ceaf1a958af2bfc1252433ca8/services/settings/Utils.jsm#57). Each entry looks like:

id: 8da7db1e-dffb-18c9-2efe-0e9d7459a0f4
last_modified: 1571184016986
bucket: main
collection: normandy-recipes
host: firefox.settings.services.mozilla.com

For each collection (distinct RemoteSettings("...") call), depending on the corresponding last_modified value returned in the previous request another request might be performed to retrieve the records of that collection (possibly only the ones that changed since some timestamp). For example, for onecrl collection https://firefox.settings.services.mozilla.com/v1/buckets/security-state/collections/onecrl?_expected=1568310941289 would be fetched (with possibly different parameters).

One concern is that the different parameters (etag, timestamps...) might be leaking enough info about the user state that it allows linking together requests done over time as belonging to the same user. In principle, the request parameters depend on the values returned in previous responses, and these seem not to change very often. I did not do a deep analysis, but I feel like we would not lose too much by doing the same requests without parameters (as if there was no previous state in the browser). I don't see the responses being so big, nor the requests done so often. But this would probably require a bit more time to make sure the changes are not breaking the RemoteSettings functionality. Perhaps on a different ticket?

Sounds good, please file one.

For now, I think we can disable the RemoteSettings("...") calls that we do not need, if only for sparing some unnecessary requests. The currently active RemoteSettings instances are:

  • toolkit/components/search/SearchService.jsm (hijack-blocklists)
    • I think we don't want this one, it allows mozilla to blacklist search extensions.
  • browser/components/preferences/browserLanguages.js (language-dictionaries):
    • I think we need this one if we allow changing language via about:preferences.
  • netwerk/url-classifier/UrlClassifierSkipListService.jsm (url-classifier-skip-urls):
    • We don't need this one until we enable enhanced tracking protection (content blocking).
  • services/common/blocklist-clients.js (onecrl, pins):
    • OneCRl and certificate pinning blocklist? I think we want to have these.

Note that the previously mentioned RemoteSettings instances in toolkit/mozapps/extensions/Blocklist.jsm are actually currently disabled because of extensions.blocklist.useXML = true, see https://trac.torproject.org/projects/tor/ticket/16931#comment:8.

So here is a patch disabling hijack-blocklists and url-classifier-skip-urls: https://github.com/acatarineu/tor-browser/commit/31740. If it's ok to disable the latter, we could update #30939, as we will need to reenable if we decide to enable Firefox Enhanced Tracking Protection.

Looks good, thanks! I cherry-picked the commit to tor-browser-68.1.0esr-9.0-3 (commit e1e1f70187dc106628e8e8c58ecbe23976b31131). Making a note on #30939 would be good. Maybe we don't want to use that protocol when working on ETP but we should have it on our radar that we disabled it.

comment:9 Changed 6 days ago by acat

Actual Points: 1.5
Note: See TracTickets for help on using tickets.