Skip to content

Draft: Bug 41165: Onion rewrite static pref causes a crash in debug builds

guest475646844 requested to merge guest475646844/tor-browser:tor-browser-115.2.0esr-13.0-1 into tor-browser-115.2.0esr-13.0-1

Merge Info

Related Issues

Backporting

Timeline

  • Immediate: patchset needed as soon as possible
  • Next Minor Stable Release: patchset that needs to be verified in nightly before backport
  • Eventually: patchset that needs to be verified in alpha before backport
  • No Backport (preferred): patchset for the next major stable

(Optional) Justification

  • Emergency security update: patchset fixes CVEs, 0-days, etc
  • Censorship event: patchset enables censorship circumvention
  • Critical bug-fix: patchset fixes a bug in core-functionality
  • Consistency: patchset which would make development easier if it were in both the alpha and release branches; developer tools, build system changes, etc
  • Sponsor required: patchset required for sponsor
  • Other: please explain

Merging

  • Merge to tor-browser - !fixups to tor-browser-specific commits, new features, security backports
  • Merge to base-browser - !fixups to base-browser-specific commits, new features to be shared with mullvad-browser, and security backports
    • NOTE: if your changeset includes patches to both base-browser and tor-browser please clearly label in the change description which commits should be cherry-picked to base-browser after merging

Issue Tracking

Review

Request Reviewer

  • Request review from an applications developer depending on modified system:
    • NOTE: if the MR modifies multiple areas, please /cc all the relevant reviewers (since gitlab only allows 1 reviewer)
    • android : dan
    • misc/other : pierov

Change Description

This is a merge request to fix a bug that manifests in debug builds. I performed only a basic unit test on Android platform with old Tor Browser version 12.0.2 based on Firefox 102.7.0esr, but it's still relevant for version 115.2.0esr, though some names are different [12][13][14]. Essentially it's a patch for MR "Bug 40458: Implement .tor.onion aliases" !262 (merged) .

The logic works as follow:

A. In main process string "browser.urlbar" is added to sParentOnlyPrefBranchList[] [1].

B. Preference "browser.urlbar.onionRewrites.enabled" isn't added to gSharedMap in Preferences::EnsureSnapshot() [2] because ShouldSanitizePreference_Impl() finds its left part in sParentOnlyPrefBranchList[] [3].

C. In gpu process (GPUProcessImpl::Init() [4]) pref_SharedLookup() doesn't find value of preference "browser.urlbar.onionRewrites.enabled" in gSharedMap [5] which leads to signal SIGSEGV in macro MOZ_ASSERT() with message "Failed accessing browser.urlbar.onionRewrites.enabled" [6]. Call stack:

pref_SharedLookup() [5]
Internals::GetSharedPrefValue() [7]
macro ALWAYS_PREF() (also calls macro MOZ_ASSERT()) [6]
StaticPrefList_browser.h (generated from StaticPrefList.yaml) [8][9]
InitStaticPrefsFromShared() [6]

One of possible solutions, that's proposed in this MR, can be moving preference "browser.urlbar.onionRewrites.enabled" from StaticPrefList.yaml to 001-base-profile.js where other preferences, starting with "browser.urlbar", reside [10], and replacing its reference StaticPrefs::browser_urlbar_onionRewrites_enabled() in OnionAliasService::GetOnionAlias() [11] with Preferences::GetBool("browser.urlbar.onionRewrites.enabled").

Code snippets (line numbers in snippets 1-11 are from git tag tor-browser-102.7.0esr-12.0-1-build1, and 13-14 from tor-browser-115.2.0esr-13.0-1-build2): 
1. modules/libpref/Preferences.cpp:5774
// These prefs are not useful in child processes.
static const PrefListEntry sParentOnlyPrefBranchList[] = {
    // Remove prefs with user data
...
    PREF_LIST_ENTRY("browser.urlbar"),

2. modules/libpref/Preferences.cpp:3690
FileDescriptor Preferences::EnsureSnapshot(size_t* aSize) {
...
  if (!gSharedMap) {
    SharedPrefMapBuilder builder;
    nsTArray<Pref*> toRepopulate;
    NameArena* newPrefNameArena = new NameArena();
    for (auto iter = HashTable()->modIter(); !iter.done(); iter.next()) {
      if (!ShouldSanitizePreference(iter.get().get(), true)) {
        iter.get()->AddToMap(builder);
      } else {
        Pref* pref = iter.getMutable().release();
        pref->RelocateName(newPrefNameArena);
        toRepopulate.AppendElement(pref);
      }
    }

3. modules/libpref/Preferences.cpp:5840
static bool ShouldSanitizePreference_Impl(const T& aPref,
                                          bool aIsDestWebContentProcess) {
...
    // First check against the denylist, the denylist is used for
    // all subprocesses to reduce IPC traffic.
    for (const auto& entry : sParentOnlyPrefBranchList) {
      if (strncmp(entry.mPrefBranch, prefName, entry.mLen) == 0) {
        return true;

4. gfx/ipc/GPUProcessImpl.cpp:28
bool GPUProcessImpl::Init(int aArgc, char* aArgv[]) {

5. modules/libpref/Preferences.cpp:1605
Maybe<PrefWrapper> pref_SharedLookup(const char* aPrefName) {
  MOZ_DIAGNOSTIC_ASSERT(gSharedMap, "gSharedMap must be initialized");
  if (Maybe<SharedPrefMap::Pref> pref = gSharedMap->Get(aPrefName)) {
    return Some(*pref);
  }
  return Nothing();
}

6. modules/libpref/Preferences.cpp:5670
static void InitStaticPrefsFromShared() {
...
#define ALWAYS_PREF(name, base_id, full_id, cpp_type, default_value)           \
  {                                                                            \
    StripAtomic<cpp_type> val;                                                 \
    if (!XRE_IsParentProcess() && IsString<cpp_type>::value &&                 \
        sCrashOnBlocklistedPref) {                                             \
      MOZ_DIAGNOSTIC_ASSERT(                                                   \
          !ShouldSanitizePreference(name, XRE_IsContentProcess()),             \
          "Should not access the preference '" name "' in Content Processes"); \
    }                                                                          \
    DebugOnly<nsresult> rv = Internals::GetSharedPrefValue(name, &val);        \
    MOZ_ASSERT(NS_SUCCEEDED(rv), "Failed accessing " name);                    \
    StaticPrefs::sMirror_##full_id = val;                                      \
  }
...
#include "mozilla/StaticPrefListAll.h"

7. modules/libpref/Preferences.cpp:4589
  static nsresult GetSharedPrefValue(const char* aName, T* aResult) {
    nsresult rv = NS_ERROR_UNEXPECTED;
    if (Maybe<PrefWrapper> pref = pref_SharedLookup(aName)) {
      rv = pref->GetValue(PrefValueKind::User, aResult);
...
    }
    return rv;
  }

8. obj-aarch64-linux-android/dist/include/mozilla/StaticPrefList_browser.h
Line 1
// This file was generated by generate_static_pref_list.py from modules/libpref/init/StaticPrefList.yaml. DO NOT EDIT.
...
Line 604
ALWAYS_PREF(
  "browser.urlbar.onionRewrites.enabled",
   browser_urlbar_onionRewrites_enabled,
   browser_urlbar_onionRewrites_enabled,
  RelaxedAtomicBool, true
)

9. modules/libpref/init/StaticPrefList.yaml:1541
- name: browser.urlbar.onionRewrites.enabled
  type: RelaxedAtomicBool
  value: true
  mirror: always

10. browser/app/profile/001-base-profile.js:99
pref("browser.urlbar.suggest.searches", false);
pref("browser.urlbar.suggest.quicksuggest.nonsponsored", false);
pref("browser.urlbar.suggest.quicksuggest.sponsored", false);

11. netwerk/dns/OnionAliasService.cpp:78
OnionAliasService::GetOnionAlias(const nsACString& aShortHostname,
                                 nsACString& aLongHostname) {
  aLongHostname = aShortHostname;
  if (mozilla::StaticPrefs::browser_urlbar_onionRewrites_enabled() &&
      StringEndsWith(aShortHostname, ".tor.onion"_ns)) {
    nsAutoCString* alias = nullptr;

12. sRestrictFromWebContentProcesses[] instead of sParentOnlyPrefBranchList[], and ShouldSanitizePreference() instead of ShouldSanitizePreference_Impl().

13. modules/libpref/Preferences.cpp:6035
// A preference is 'sanitized' (i.e. not sent to web content processes) if
// one of two criteria are met:
//   1. The pref name matches one of the prefixes in the following list
//   2. The pref is dynamically named (i.e. not specified in all.js or
//      StaticPrefList.yml), a string pref, and it is NOT exempted in
//      sDynamicPrefOverrideList
//
// This behavior is codified in ShouldSanitizePreference() below
static const PrefListEntry sRestrictFromWebContentProcesses[] = {
    // Remove prefs with user data
...
    PREF_LIST_ENTRY("browser.urlbar"),

14. modules/libpref/Preferences.cpp:6165
static bool ShouldSanitizePreference(const Pref* const aPref) {
  // In the parent process, we use a heuristic to decide if a pref
  // value should be sanitized before sending to subprocesses.
...
  // First check against the denylist.
  // The services pref is an annoying one - it's much easier to blocklist
  // the whole branch and then add this one check to let this one annoying
  // pref through.
  for (const auto& entry : sRestrictFromWebContentProcesses) {
    if (strncmp(entry.mPrefBranch, prefName, entry.mLen) == 0) {
      const auto* p = prefName;  // This avoids clang-format doing ugly things.
      return !(strncmp("services.settings.clock_skew_seconds", p, 36) == 0 ||
               strncmp("services.settings.last_update_seconds", p, 37) == 0 ||
               strncmp("services.settings.server", p, 24) == 0);
    }
  }

/cc @pierov @dan @tjr

Edited by guest475646844

Merge request reports