Skip to content

Commit

Permalink
[CodeHealth] Use SafeBaseName for TorConfig
Browse files Browse the repository at this point in the history 
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.

Resolves brave/brave-browser#41834
  • Loading branch information
@cdesouza-chromium
cdesouza-chromium committed Oct 24, 2024
1 parent be3fe14 commit 29b9178
Show file tree
Hide file tree
Showing 14 changed files with 283 additions and 177 deletions.
38 changes: 19 additions & 19 deletions browser/tor/test/brave_tor_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,18 +142,18 @@ void NonBlockingDelay(base::TimeDelta delay) {

} // namespace

class BraveTorTest : public InProcessBrowserTest {
class BraveTorBrowserTest : public InProcessBrowserTest {
public:
struct TorInfo {
raw_ptr<Profile> tor_profile = nullptr;
int tor_pid = 0;
};

BraveTorTest() {
BraveTorBrowserTest() {
BraveSettingsUI::ShouldExposeElementsForTesting() = true;
}

~BraveTorTest() override {
~BraveTorBrowserTest() override {
BraveSettingsUI::ShouldExposeElementsForTesting() = false;
}

Expand Down Expand Up @@ -222,7 +222,7 @@ class BraveTorTest : public InProcessBrowserTest {
}
};

IN_PROC_BROWSER_TEST_F(BraveTorTest, OpenCloseDisableTorWindow) {
IN_PROC_BROWSER_TEST_F(BraveTorBrowserTest, OpenCloseDisableTorWindow) {
EXPECT_FALSE(TorProfileServiceFactory::IsTorDisabled(browser()->profile()));
DownloadTorClient();

Expand Down Expand Up @@ -254,7 +254,7 @@ IN_PROC_BROWSER_TEST_F(BraveTorTest, OpenCloseDisableTorWindow) {
}
}

class BraveTorTestWithCustomProfile : public BraveTorTest {
class BraveTorWithCustomProfileBrowserTest : public BraveTorBrowserTest {
private:
void SetUpCommandLine(base::CommandLine* command_line) override {
InProcessBrowserTest::SetUpCommandLine(command_line);
Expand All @@ -273,7 +273,7 @@ class BraveTorTestWithCustomProfile : public BraveTorTest {
}
};

IN_PROC_BROWSER_TEST_F(BraveTorTestWithCustomProfile, PRE_SetupBridges) {
IN_PROC_BROWSER_TEST_F(BraveTorWithCustomProfileBrowserTest, PRE_SetupBridges) {
EXPECT_FALSE(TorProfileServiceFactory::IsTorDisabled(browser()->profile()));
DownloadTorClient();

Expand Down Expand Up @@ -331,7 +331,7 @@ IN_PROC_BROWSER_TEST_F(BraveTorTestWithCustomProfile, PRE_SetupBridges) {
g_brave_browser_process->tor_pluggable_transport_updater());
}

IN_PROC_BROWSER_TEST_F(BraveTorTestWithCustomProfile, SetupBridges) {
IN_PROC_BROWSER_TEST_F(BraveTorWithCustomProfileBrowserTest, SetupBridges) {
// Tor is disabled in PRE, check pluggable transports are removed.
EXPECT_FALSE(CheckComponentExists(tor::kTorPluggableTransportComponentId));

Expand All @@ -345,7 +345,7 @@ IN_PROC_BROWSER_TEST_F(BraveTorTestWithCustomProfile, SetupBridges) {
nullptr));
}

IN_PROC_BROWSER_TEST_F(BraveTorTestWithCustomProfile, Incognito) {
IN_PROC_BROWSER_TEST_F(BraveTorWithCustomProfileBrowserTest, Incognito) {
EXPECT_FALSE(TorProfileServiceFactory::IsTorDisabled(browser()->profile()));
EXPECT_FALSE(TorProfileServiceFactory::IsTorManaged(browser()->profile()));

Expand Down Expand Up @@ -405,7 +405,7 @@ IN_PROC_BROWSER_TEST_F(BraveTorTestWithCustomProfile, Incognito) {
EXPECT_TRUE(is_element_enabled("torSnowflake"));
}

IN_PROC_BROWSER_TEST_F(BraveTorTestWithCustomProfile, Autofill) {
IN_PROC_BROWSER_TEST_F(BraveTorWithCustomProfileBrowserTest, Autofill) {
GURL fake_url("http://brave.com/");
// Disable autofill in private windows.
browser()->profile()->GetPrefs()->SetBoolean(kBraveAutofillPrivateWindows,
Expand All @@ -426,7 +426,7 @@ IN_PROC_BROWSER_TEST_F(BraveTorTestWithCustomProfile, Autofill) {
TestAutofillInWindow(web_contents, fake_url, true);
}

IN_PROC_BROWSER_TEST_F(BraveTorTest, PRE_ResetBridges) {
IN_PROC_BROWSER_TEST_F(BraveTorBrowserTest, PRE_ResetBridges) {
EXPECT_FALSE(TorProfileServiceFactory::IsTorDisabled(browser()->profile()));
DownloadTorClient();
DownloadTorPluggableTransports();
Expand All @@ -451,14 +451,14 @@ IN_PROC_BROWSER_TEST_F(BraveTorTest, PRE_ResetBridges) {
WaitProcessExit(tor::kSnowflakeExecutableName);
}

IN_PROC_BROWSER_TEST_F(BraveTorTest, ResetBridges) {
IN_PROC_BROWSER_TEST_F(BraveTorBrowserTest, ResetBridges) {
// Tor is enabled and bridges are disabled check pluggable transports are
// removed.
EXPECT_TRUE(CheckComponentExists(tor::kTorClientComponentId));
EXPECT_FALSE(CheckComponentExists(tor::kTorPluggableTransportComponentId));
}

IN_PROC_BROWSER_TEST_F(BraveTorTest, HttpAllowlistIsolation) {
IN_PROC_BROWSER_TEST_F(BraveTorBrowserTest, HttpAllowlistIsolation) {
// Normal window
Profile* main_profile = browser()->profile();
auto* main_storage_partition = main_profile->GetDefaultStoragePartition();
Expand Down Expand Up @@ -511,11 +511,11 @@ IN_PROC_BROWSER_TEST_F(BraveTorTest, HttpAllowlistIsolation) {
EXPECT_TRUE(tor_state->IsHttpAllowedForHost(host3, tor_storage_partition));
}

class BraveTorTest_EnableTorHttpsOnlyFlag
: public BraveTorTest,
class BraveTorBrowserTest_EnableTorHttpsOnlyFlag
: public BraveTorBrowserTest,
public ::testing::WithParamInterface<bool> {
public:
BraveTorTest_EnableTorHttpsOnlyFlag() {
BraveTorBrowserTest_EnableTorHttpsOnlyFlag() {
if (IsBraveHttpsByDefaultEnabled()) {
std::vector<base::test::FeatureRef> enabled_features{
net::features::kBraveTorWindowsHttpsOnly};
Expand All @@ -530,15 +530,15 @@ class BraveTorTest_EnableTorHttpsOnlyFlag
}
}

~BraveTorTest_EnableTorHttpsOnlyFlag() override = default;
~BraveTorBrowserTest_EnableTorHttpsOnlyFlag() override = default;

bool IsBraveHttpsByDefaultEnabled() { return GetParam(); }

protected:
base::test::ScopedFeatureList scoped_feature_list_;
};

IN_PROC_BROWSER_TEST_P(BraveTorTest_EnableTorHttpsOnlyFlag,
IN_PROC_BROWSER_TEST_P(BraveTorBrowserTest_EnableTorHttpsOnlyFlag,
TorWindowHttpsOnly) {
EXPECT_FALSE(TorProfileServiceFactory::IsTorDisabled(browser()->profile()));
DownloadTorClient();
Expand All @@ -549,6 +549,6 @@ IN_PROC_BROWSER_TEST_P(BraveTorTest_EnableTorHttpsOnlyFlag,
EXPECT_TRUE(prefs->GetBoolean(prefs::kHttpsOnlyModeEnabled));
}

INSTANTIATE_TEST_SUITE_P(BraveTorTest_EnableTorHttpsOnlyFlag,
BraveTorTest_EnableTorHttpsOnlyFlag,
INSTANTIATE_TEST_SUITE_P(BraveTorBrowserTest_EnableTorHttpsOnlyFlag,
BraveTorBrowserTest_EnableTorHttpsOnlyFlag,
::testing::Bool());
1 change: 1 addition & 0 deletions components/services/tor/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ static_library("tor") {
"public/interfaces",
"//base",
"//brave/components/child_process_monitor",
"//brave/components/tor:constants",
"//mojo/public/cpp/bindings",
]
}
13 changes: 8 additions & 5 deletions components/services/tor/public/interfaces/tor.mojom
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
62 changes: 36 additions & 26 deletions components/services/tor/tor_launcher_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>()),
Expand Down Expand Up @@ -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)
Expand All @@ -87,13 +96,14 @@ void TorLauncherImpl::Launch(mojom::TorConfigPtr config,
#if BUILDFLAG(IS_WIN)
launchopts.start_hidden = true;
#endif
launchopts.current_directory = config->binary_path.DirName();
// This line is necessary as the paths for tor_snowflake and tor_obfs4 are set
// up relative to this binary.
launchopts.current_directory = args.GetProgram().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,
Expand Down
13 changes: 13 additions & 0 deletions components/tor/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down
Loading

0 comments on commit 29b9178

Please sign in to comment.