[CodeHealth] Use SafeBaseName for TorConfig

The use of `base::FilePath` in `TorConfig` could be a serious source for a vulnerability if someone got control of the browser process. This change employs the use of `SafeBaseName`, which only allows for the base component of a path. With this browser process and the tor launcher do agree over a certain definition of how the files are discovered. Additionally, this change removes the setting of executable bit when extracting the tor binary, as this has been correct upstream since[1]. [1] https://crrev.com/c/3379289 Resolves brave/brave-browser#41834
brave · Oct 24, 2024 · a1c6336 · a1c6336
1 parent e2ded45
commit a1c6336
Show file tree

Hide file tree

Showing 14 changed files with 252 additions and 169 deletions.
diff --git a/browser/tor/test/brave_tor_browsertest.cc b/browser/tor/test/brave_tor_browsertest.cc
@@ -4,6 +4,7 @@
  * You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 #include "base/feature_list.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
@@ -123,6 +124,17 @@ void DownloadTorComponent(const std::string& component_id) {
 
   ASSERT_TRUE(base::CopyDirectory(
       component_dir, user_data_dir.AppendASCII(component_id), true));
+
+#if BUILDFLAG(IS_POSIX)
+  base::FileEnumerator traversal(
+      user_data_dir.AppendASCII(component_id).AppendASCII("1.0.0"), false,
+      base::FileEnumerator::FILES, "tor-0.*");
+  for (base::FilePath file = traversal.Next(); !file.empty();
+       file = traversal.Next()) {
+    // LOG(INFO) << "Setting permissions for " << file.BaseName().value();
+    ASSERT_TRUE(base::SetPosixFilePermissions(file, 0755));
+  }
+#endif  // BUILDFLAG(IS_POSIX)
 }
 
 bool CheckComponentExists(const std::string& component_id) {

diff --git a/components/services/tor/BUILD.gn b/components/services/tor/BUILD.gn
@@ -13,6 +13,7 @@ static_library("tor") {
     "public/interfaces",
     "//base",
     "//brave/components/child_process_monitor",
+    "//brave/components/tor:constants",
     "//mojo/public/cpp/bindings",
   ]
 }
diff --git a/components/services/tor/public/interfaces/tor.mojom b/components/services/tor/public/interfaces/tor.mojom
@@ -3,14 +3,17 @@
 // You can obtain one at http://mozilla.org/MPL/2.0/.
 module tor.mojom;
 
-import "mojo/public/mojom/base/file_path.mojom";
+import "mojo/public/mojom/base/safe_base_name.mojom";
 import "sandbox/policy/mojom/sandbox.mojom";
 
 struct TorConfig {
-  mojo_base.mojom.FilePath binary_path;
-  mojo_base.mojom.FilePath torrc_path;
-  mojo_base.mojom.FilePath tor_data_path;
-  mojo_base.mojom.FilePath tor_watch_path;
+  // The install directory provided by the component updater for the Tor
+  // client.
+  mojo_base.mojom.SafeBaseName install_dir;
+
+  // The filename for the tor executable. The executable is expected to be in
+  // the install directory.
+  mojo_base.mojom.SafeBaseName executable;
 };
 
 [ServiceSandbox=sandbox.mojom.Sandbox.kNoSandbox]

diff --git a/components/services/tor/tor_launcher_impl.cc b/components/services/tor/tor_launcher_impl.cc
@@ -11,10 +11,24 @@
 #include "base/files/file_util.h"
 #include "base/process/launch.h"
 #include "base/strings/string_number_conversions.h"
+#include "brave/components/tor/constants.h"
 #include "build/build_config.h"
 
 namespace tor {
 
+namespace {
+
+// A utiltiy function to create the missing directories that are used by the
+// launcher.
+base::FilePath CreateIfNotExists(const base::FilePath& path) {
+  if (!base::DirectoryExists(path)) {
+    CHECK(base::CreateDirectory(path));
+  }
+  return path;
+}
+
+}  // namespace
+
 TorLauncherImpl::TorLauncherImpl(
     mojo::PendingReceiver<mojom::TorLauncher> receiver)
     : child_monitor_(std::make_unique<brave::ChildProcessMonitor>()),
@@ -53,32 +67,27 @@ void TorLauncherImpl::Launch(mojom::TorConfigPtr config,
       std::move(callback).Run(false, -1);
     return;
   }
-  base::CommandLine args(config->binary_path);
+  base::CommandLine args(
+      GetClientExecutablePath(config->install_dir, config->executable));
   args.AppendArg("--ignore-missing-torrc");
+
+  auto torrc_path = GetTorRcPath(config->install_dir);
   args.AppendArg("-f");
-  args.AppendArgPath(config->torrc_path);
+  args.AppendArgPath(torrc_path);
   args.AppendArg("--defaults-torrc");
-  args.AppendArgPath(config->torrc_path);
-  base::FilePath tor_data_path = config->tor_data_path;
-  if (!tor_data_path.empty()) {
-    if (!base::DirectoryExists(tor_data_path))
-      base::CreateDirectory(tor_data_path);
-    args.AppendArg("--DataDirectory");
-    args.AppendArgPath(tor_data_path);
-  }
+  args.AppendArgPath(torrc_path);
+
+  args.AppendArg("--DataDirectory");
+  args.AppendArgPath(CreateIfNotExists(GetTorDataPath()));
   args.AppendArg("--__OwningControllerProcess");
   args.AppendArg(base::NumberToString(base::Process::Current().Pid()));
-  tor_watch_path_ = config->tor_watch_path;
-  if (!tor_watch_path_.empty()) {
-    if (!base::DirectoryExists(tor_watch_path_))
-      base::CreateDirectory(tor_watch_path_);
-    args.AppendArg("--pidfile");
-    args.AppendArgPath(tor_watch_path_.AppendASCII("tor.pid"));
-    args.AppendArg("--controlportwritetofile");
-    args.AppendArgPath(tor_watch_path_.AppendASCII("controlport"));
-    args.AppendArg("--cookieauthfile");
-    args.AppendArgPath(tor_watch_path_.AppendASCII("control_auth_cookie"));
-  }
+  tor_watch_path_ = CreateIfNotExists(GetTorWatchPath());
+  args.AppendArg("--pidfile");
+  args.AppendArgPath(tor_watch_path_.AppendASCII("tor.pid"));
+  args.AppendArg("--controlportwritetofile");
+  args.AppendArgPath(tor_watch_path_.AppendASCII("controlport"));
+  args.AppendArg("--cookieauthfile");
+  args.AppendArgPath(tor_watch_path_.AppendASCII("control_auth_cookie"));
 
   base::LaunchOptions launchopts;
 #if BUILDFLAG(IS_LINUX)
@@ -87,13 +96,11 @@ void TorLauncherImpl::Launch(mojom::TorConfigPtr config,
 #if BUILDFLAG(IS_WIN)
   launchopts.start_hidden = true;
 #endif
-  launchopts.current_directory = config->binary_path.DirName();
   base::Process tor_process = base::LaunchProcess(args, launchopts);
 
-  bool result = tor_process.IsValid();
-
-  if (callback)
-    std::move(callback).Run(result, tor_process.Pid());
+  if (callback) {
+    std::move(callback).Run(tor_process.IsValid(), tor_process.Pid());
+  }
 
   child_monitor_->Start(std::move(tor_process),
                         base::BindOnce(&TorLauncherImpl::OnChildCrash,

diff --git a/components/tor/BUILD.gn b/components/tor/BUILD.gn
@@ -3,6 +3,18 @@ import("//chrome/common/features.gni")
 
 assert(enable_tor)
 
+source_set("constants") {
+  sources = [
+    "constants.cc",
+    "constants.h",
+  ]
+
+  deps = [
+    "//base",
+    "//components/component_updater:component_updater_paths",
+  ]
+}
+
 static_library("tor") {
   public_deps = [
     ":common",
@@ -45,6 +57,7 @@ static_library("tor") {
   ]
 
   deps += [
+    ":constants",
     "//brave/components/brave_component_updater/browser",
     "//brave/components/resources:strings",
     "//brave/components/services/tor/public/interfaces",

diff --git a/components/tor/brave_tor_client_updater.cc b/components/tor/brave_tor_client_updater.cc
@@ -16,9 +16,9 @@
 #include "base/logging.h"
 #include "base/task/task_runner.h"
 #include "base/task/thread_pool.h"
+#include "brave/components/tor/constants.h"
 #include "brave/components/tor/pref_names.h"
 #include "brave/components/tor/tor_switches.h"
-#include "build/build_config.h"
 #include "components/prefs/pref_service.h"
 #include "third_party/re2/src/re2/re2.h"
 
@@ -28,10 +28,8 @@ namespace tor {
 
 namespace {
 
-std::pair<base::FilePath, base::FilePath> InitTorPath(
-    const base::FilePath& install_dir) {
+base::FilePath InitTorPath(const base::FilePath& install_dir) {
   base::FilePath executable_path;
-  base::FilePath torrc_path;
   base::FileEnumerator traversal(install_dir, false,
                                  base::FileEnumerator::FILES,
                                  FILE_PATH_LITERAL("tor-*"));
@@ -42,83 +40,24 @@ std::pair<base::FilePath, base::FilePath> InitTorPath(
             file_info.GetName().MaybeAsASCII(),
             "tor-\\d+\\.\\d+\\.\\d+\\.\\d+-\\w+(-\\w+)?-brave-\\d+")) {
       executable_path = current;
-    } else if (file_info.GetName().MaybeAsASCII() == "tor-torrc") {
-      torrc_path = current;
     }
 
-    if (!executable_path.empty() && !torrc_path.empty())
+    if (!executable_path.empty()) {
       break;
+    }
   }
 
-  if (executable_path.empty() || torrc_path.empty()) {
-    LOG(ERROR) << "Failed to locate Tor client executable or torrc in "
+  if (executable_path.empty()) {
+    LOG(ERROR) << "Failed to locate the Tor client executable in "
                << install_dir.value().c_str();
-    return std::make_pair(base::FilePath(), base::FilePath());
-  }
-
-#if BUILDFLAG(IS_POSIX)
-  // Ensure that Tor client executable has appropriate file
-  // permissions, as CRX unzipping does not preserve them.
-  // See https://crbug.com/555011
-  if (!base::SetPosixFilePermissions(executable_path, 0755)) {
-    LOG(ERROR) << "Failed to set executable permission on "
-               << executable_path.value().c_str();
-    return std::make_pair(base::FilePath(), base::FilePath());
+    return {};
   }
-#endif  // BUILDFLAG(IS_POSIX)
 
-  return std::make_pair(executable_path, torrc_path);
+  return executable_path;
 }
 
 }  // namespace
 
-#if BUILDFLAG(IS_WIN)
-constexpr char kTorClientComponentName[] = "Brave Tor Client Updater (Windows)";
-constexpr char kTorClientComponentId[] = "cpoalefficncklhjfpglfiplenlpccdb";
-constexpr char kTorClientComponentBase64PublicKey[] =
-    "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA1AYAsmR/VoRwkZCsjRpD"
-    "58xjrgngW5y17H6BqQ7/CeNSpmXlcMXy6bJs2D/yeS96rhZSrQSHTzS9h/ieo/NZ"
-    "F5PIwcv07YsG5sRd6zF5a6m92aWCQa1OkbL6jpcpL2Tbc4mCqNxhKMErT7EtIIWL"
-    "9cW+mtFUjUjvV3rJLQ3Vy9u6fEi77Y8b25kGnTJoVt3uETAIHBnyNpL7ac2f8Iq+"
-    "4Qa6VFmuoBhup54tTZvMv+ikoKKaQkHzkkjTa4hV5AzdnFDKO8C9qJb3T/Ef0+MO"
-    "IuZjyySVzGNcOfASeHkhxhlwMQSQuhCN5mdFW5YBnVZ/5QWx8WzbhqBny/ZynS4e"
-    "rQIDAQAB";
-#elif BUILDFLAG(IS_MAC)
-constexpr char kTorClientComponentName[] = "Brave Tor Client Updater (Mac)";
-constexpr char kTorClientComponentId[] = "cldoidikboihgcjfkhdeidbpclkineef";
-constexpr char kTorClientComponentBase64PublicKey[] =
-    "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAw2QUXSbVuRxYpItYApZ8"
-    "Ly/fGeUD3A+vb3J7Ot62CF32wTfWweANWyyB+EBGfbtNDAuRlAbNk0QYeCQEttuf"
-    "jLh3Kd5KR5fSyyNNd2cAzAckQ8p7JdiFYjvqZLGC5vlnHgqq4O8xACX5EPwHLNFD"
-    "iSpsthNmz3GCUrHrzPHjHVfy+IuucQXygnRv2fwIaAIxJmTbYm4fqsGKpfolWdMe"
-    "jKVAy1hc9mApZSyt4oGvUu4SJZnxlYMrY4Ze+OWbDesi2JGy+6dA1ddL9IdnwCb3"
-    "9CBOMNjaHeCVz0MKxdCWGPieQM0R7S1KvDCVqAkss6NAbLB6AVM0JulqxC9b+hr/"
-    "xwIDAQAB";
-#elif BUILDFLAG(IS_LINUX)
-constexpr char kTorClientComponentName[] = "Brave Tor Client Updater (Linux)";
-#if defined(ARCH_CPU_ARM64)
-constexpr char kTorClientComponentId[] = "monolafkoghdlanndjfeebmdfkbklejg";
-constexpr char kTorClientComponentBase64PublicKey[] =
-    "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAzqb14fggDpbjZtv3HKmR"
-    "UTnvfDTcqVbVZo0DdCHQi6SwxDlRweGwsvsHuy9U37VBr41ha/neemQGf+5qkWgY"
-    "y+mzzAkb5ZtrHkBSOOsZdyO9WEj7GwXuAx9FvcxG2zPpA/CvagnC14VhMyUFLL8v"
-    "XdfHYPmQOtIVdW3eR0G/4JP/mTbnAEkipQfxrDMtDVpX+FDB+Zy5yEMGKWHRLcdH"
-    "bHUgb/VhB9ppt0LKRjM44KSpyPDlYquXNcn3WFmxHoVm7PZ3LTAn3eSNZrT4ptmo"
-    "KveT4LgWtObrHoZtrg+/LnHAi1GYf8PHrRc+o/FptobOWoUN5lt8NvhLjv85ERBt"
-    "rQIDAQAB";
-#else
-constexpr char kTorClientComponentId[] = "biahpgbdmdkfgndcmfiipgcebobojjkp";
-constexpr char kTorClientComponentBase64PublicKey[] =
-    "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAseuq8dXKawkZC7RSE7xb"
-    "lRwh6DD+oPEGEjZWKh596/42IrWNQw60gRIR6s7x0YHh5geFnBRkx9bisEXOrFkq"
-    "oArVY7eD0gMkjpor9CneD5CnCxc9/2uIPajtXfAmmLAHtN6Wk7yW30SkRf/WvLWX"
-    "/H+PqskQBN7I5MO7sveYxSrRMSj7prrFHEiFmXTgG/DwjpzrA7KV6vmzz/ReD51o"
-    "+UuLHE7cxPhnsNd/52uY3Lod3GhxvDoXKYx9kWlzBjxB53A2eLBCDIwwCpqS4/Ib"
-    "RSJhvF33KQT8YM+7V1MitwB49klP4aEWPXwOlFHmn9Dkmlx2RbO7S0tRcH9UH4LK"
-    "2QIDAQAB";
-#endif
-#endif
-
 BraveTorClientUpdater::BraveTorClientUpdater(
     BraveComponent::Delegate* component_delegate,
     PrefService* local_state,
@@ -173,40 +112,19 @@ void BraveTorClientUpdater::RemoveObsoleteFiles() {
   task_runner_->PostTask(FROM_HERE, base::GetDeleteFileCallback(tor_log));
 }
 
-void BraveTorClientUpdater::SetTorPath(
-    const std::pair<base::FilePath, base::FilePath>& paths) {
-  executable_path_ = paths.first;
-  torrc_path_ = paths.second;
+void BraveTorClientUpdater::OnExecutablePathFound(base::FilePath path) {
+  executable_ = std::move(path);
   for (Observer& observer : observers_)
-    observer.OnExecutableReady(paths.second);
-}
-
-base::FilePath BraveTorClientUpdater::GetExecutablePath() const {
-  return executable_path_;
-}
-
-base::FilePath BraveTorClientUpdater::GetTorrcPath() const {
-  return torrc_path_;
-}
-
-base::FilePath BraveTorClientUpdater::GetTorDataPath() const {
-  DCHECK(!user_data_dir_.empty());
-  return user_data_dir_.Append(FILE_PATH_LITERAL("tor"))
-      .Append(FILE_PATH_LITERAL("data"));
-}
-
-base::FilePath BraveTorClientUpdater::GetTorWatchPath() const {
-  DCHECK(!user_data_dir_.empty());
-  return user_data_dir_.Append(FILE_PATH_LITERAL("tor"))
-      .Append(FILE_PATH_LITERAL("watch"));
+    observer.OnExecutableReady(executable_);
 }
 
 void BraveTorClientUpdater::OnComponentReady(const std::string& component_id,
                                              const base::FilePath& install_dir,
                                              const std::string& manifest) {
+  install_dir_ = install_dir;
   GetTaskRunner()->PostTaskAndReplyWithResult(
       FROM_HERE, base::BindOnce(&InitTorPath, install_dir),
-      base::BindOnce(&BraveTorClientUpdater::SetTorPath,
+      base::BindOnce(&BraveTorClientUpdater::OnExecutablePathFound,
                      weak_ptr_factory_.GetWeakPtr()));
 }
 

diff --git a/components/tor/brave_tor_client_updater.h b/components/tor/brave_tor_client_updater.h
@@ -8,7 +8,6 @@
 
 #include <memory>
 #include <string>
-#include <utility>
 
 #include "base/files/file_path.h"
 #include "base/memory/raw_ptr.h"
@@ -47,14 +46,13 @@ class BraveTorClientUpdater : public BraveComponent {
   void Register();
   void Unregister();
   void Cleanup();
-  base::FilePath GetExecutablePath() const;
-  base::FilePath GetTorrcPath() const;
-  base::FilePath GetTorDataPath() const;
-  base::FilePath GetTorWatchPath() const;
   scoped_refptr<base::SequencedTaskRunner> GetTaskRunner() {
     return task_runner_;
   }
 
+  const base::FilePath& install_dir() const { return install_dir_; }
+  const base::FilePath& executable() const { return executable_; }
+
   void AddObserver(Observer* observer);
   void RemoveObserver(Observer* observer);
 
@@ -67,13 +65,18 @@ class BraveTorClientUpdater : public BraveComponent {
  private:
   void RemoveObsoleteFiles();
 
-  // <tor executable, torrc>
-  void SetTorPath(const std::pair<base::FilePath, base::FilePath>&);
+  // Called with a response for the search for the executable path.
+  void OnExecutablePathFound(base::FilePath path);
 
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
   bool registered_;
-  base::FilePath executable_path_;
-  base::FilePath torrc_path_;
+
+  // The path where the binary has been ultimately installed.
+  base::FilePath install_dir_;
+
+  // The path for the tor executable.
+  base::FilePath executable_;
+
   base::ObserverList<Observer> observers_;
   raw_ptr<PrefService> local_state_ = nullptr;
   base::FilePath user_data_dir_;