usb_bouncer: Daemonize and cleanup udev commands.
This eliminates some of the cases where usb_bouncer blocks udev for a
relatively long time, and completely removes waiting on syslog or D-Bus
since the waiting needs to happen before entering the jail.
BUG=chromium:1133849, b:170306064
TEST=FEATURES=test emerge-${BOARD} usb_bouncer &&
tast run ${TEST_DEVICE} security.USBBouncer.check_seccomp
Change-Id: I3da38c55d8038f0708a7c4fc461322e49cae3217
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2466610
Tested-by: Allen Webb <allenwebb@google.com>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Commit-Queue: Allen Webb <allenwebb@google.com>
(cherry picked from commit 2a0a5e54c5dba3ee0d193a6e9546ab17e1fdeb59)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2581747
Reviewed-by: Allen Webb <allenwebb@google.com>
Commit-Queue: Henry Sun <henrysun@google.com>
Tested-by: Henry Sun <henrysun@google.com>
diff --git a/usb_bouncer/40-usb-bouncer.rules b/usb_bouncer/40-usb-bouncer.rules
index 19daa63..c76fab5 100644
--- a/usb_bouncer/40-usb-bouncer.rules
+++ b/usb_bouncer/40-usb-bouncer.rules
@@ -1,3 +1,3 @@
# Keep track of USB devices that should be allow-listed at the lock screen.
-ACTION=="add", DRIVER=="usb", RUN+="/usr/sbin/usb_bouncer udev add '%E{DEVPATH}'"
-ACTION=="remove", ENV{DEVTYPE}=="usb_device", RUN+="/usr/sbin/usb_bouncer udev remove '%E{DEVPATH}'"
+ACTION=="add", DRIVER=="usb", RUN+="/usr/sbin/usb_bouncer --fork udev add '%E{DEVPATH}'"
+ACTION=="remove", ENV{DEVTYPE}=="usb_device", RUN+="/usr/sbin/usb_bouncer --fork udev remove '%E{DEVPATH}'"
diff --git a/usb_bouncer/entry_manager.cc b/usb_bouncer/entry_manager.cc
index 1ebfb4b..d87afee 100644
--- a/usb_bouncer/entry_manager.cc
+++ b/usb_bouncer/entry_manager.cc
@@ -31,8 +31,10 @@
} // namespace
-EntryManager* EntryManager::GetInstance() {
- static EntryManager instance;
+EntryManager* EntryManager::GetInstance(
+ DevpathToRuleCallback rule_from_devpath) {
+ static EntryManager instance("/", GetUserDBDir(), IsLockscreenShown(),
+ IsGuestSession(), rule_from_devpath);
if (!instance.global_db_.Valid()) {
LOG(ERROR) << "Failed to open global DB.";
return nullptr;
diff --git a/usb_bouncer/entry_manager.h b/usb_bouncer/entry_manager.h
index 539dd68..3aae235 100644
--- a/usb_bouncer/entry_manager.h
+++ b/usb_bouncer/entry_manager.h
@@ -41,7 +41,7 @@
public:
enum class UdevAction { kAdd = 0, kRemove = 1 };
- static EntryManager* GetInstance();
+ static EntryManager* GetInstance(DevpathToRuleCallback rule_from_devpath);
static bool CreateDefaultGlobalDB();
~EntryManager();
diff --git a/usb_bouncer/seccomp/usb_bouncer-seccomp-amd64.policy b/usb_bouncer/seccomp/usb_bouncer-seccomp-amd64.policy
index 8e8ddcf..8769aa8 100644
--- a/usb_bouncer/seccomp/usb_bouncer-seccomp-amd64.policy
+++ b/usb_bouncer/seccomp/usb_bouncer-seccomp-amd64.policy
@@ -39,12 +39,15 @@
# These were not included by security.USBBouncer.
clock_gettime: 1
+clone: 1
creat: 1
+dup3: 1
fchown: 1
newfstatat: 1
futex: 1
getpid: 1
gettimeofday: 1
+ioctl: arg1 == TCGETS
lstat: 1
mmap: arg2 in ~PROT_EXEC || arg2 in ~PROT_WRITE
mprotect: arg2 in ~PROT_EXEC || arg2 in ~PROT_WRITE
@@ -53,5 +56,6 @@
rt_sigprocmask: 1
set_robust_list: 1
set_tid_address: 1
+setsid: 1
tgkill: 1
unlinkat: 1
diff --git a/usb_bouncer/seccomp/usb_bouncer-seccomp-arm.policy b/usb_bouncer/seccomp/usb_bouncer-seccomp-arm.policy
index 4b29148..99a709b 100644
--- a/usb_bouncer/seccomp/usb_bouncer-seccomp-arm.policy
+++ b/usb_bouncer/seccomp/usb_bouncer-seccomp-arm.policy
@@ -43,8 +43,11 @@
# These were not included by security.USBBouncer.
ARM_set_tls: 1
+clone: 1
+dup3: 1
fstatat64: 1
getpid: 1
+ioctl: arg1 == TCGETS
lstat64: 1
mmap2: arg2 in ~PROT_EXEC || arg2 in ~PROT_WRITE
mprotect: arg2 in ~PROT_EXEC || arg2 in ~PROT_WRITE
@@ -53,6 +56,7 @@
rt_sigprocmask: 1
set_robust_list: 1
set_tid_address: 1
+setsid: 1
tgkill: 1
ugetrlimit: 1
unlinkat: 1
diff --git a/usb_bouncer/seccomp/usb_bouncer-seccomp-arm64.policy b/usb_bouncer/seccomp/usb_bouncer-seccomp-arm64.policy
index 75be97d..2d500cd 100644
--- a/usb_bouncer/seccomp/usb_bouncer-seccomp-arm64.policy
+++ b/usb_bouncer/seccomp/usb_bouncer-seccomp-arm64.policy
@@ -41,6 +41,8 @@
# These were not included by security.USBBouncer.
clock_getres: 1
clock_gettime: 1
+clone: 1
+dup3: 1
getpid: 1
gettimeofday: 1
ioctl: arg1 == TCGETS
@@ -51,5 +53,6 @@
rt_sigprocmask: 1
set_robust_list: 1
set_tid_address: 1
+setsid: 1
tgkill: 1
unlinkat: 1
diff --git a/usb_bouncer/usb_bouncer.cc b/usb_bouncer/usb_bouncer.cc
index 578da33..3068c50 100644
--- a/usb_bouncer/usb_bouncer.cc
+++ b/usb_bouncer/usb_bouncer.cc
@@ -7,7 +7,6 @@
#include <cstdio>
#include <cstdlib>
-#include <base/bind.h>
#include <base/command_line.h>
#include <brillo/syslog_logging.h>
#include <brillo/flag_helper.h>
@@ -17,8 +16,10 @@
#include "usb_bouncer/entry_manager.h"
#include "usb_bouncer/util.h"
+using usb_bouncer::Daemonize;
+using usb_bouncer::DevpathToRuleCallback;
using usb_bouncer::EntryManager;
-using usb_bouncer::ForkAndWaitIfNotReady;
+using usb_bouncer::GetRuleFromDevPath;
using usb_bouncer::kDBusPath;
namespace {
@@ -35,16 +36,27 @@
kDisabled,
};
+enum class ForkConfig {
+ kDoubleFork,
+ kDisabled,
+};
+
+class Configuration {
+ public:
+ const SeccompEnforcement seccomp;
+ const ForkConfig fork_config;
+};
+
static constexpr char kLogPath[] = "/dev/log";
static constexpr char kUMAEventsPath[] = "/var/lib/metrics/uma-events";
-void DropPrivileges(SeccompEnforcement seccomp) {
+void DropPrivileges(const Configuration& config) {
ScopedMinijail j(minijail_new());
minijail_change_user(j.get(), usb_bouncer::kUsbBouncerUser);
minijail_change_group(j.get(), usb_bouncer::kUsbBouncerGroup);
minijail_inherit_usergroups(j.get());
minijail_no_new_privs(j.get());
- if (seccomp == SeccompEnforcement::kEnabled) {
+ if (config.seccomp == SeccompEnforcement::kEnabled) {
minijail_use_seccomp_filter(j.get());
minijail_parse_seccomp_filters(
j.get(), "/usr/share/policy/usb_bouncer-seccomp.policy");
@@ -52,7 +64,13 @@
minijail_namespace_ipc(j.get());
minijail_namespace_net(j.get());
- minijail_namespace_pids(j.get());
+ // If minijail were to run as init, then it would be tracked by udev and
+ // defeat the purpose of daemonizing. If minijail doesn't run as init, the
+ // descendant processes will die when daemonizing because there won't be an
+ // init to keep the pid namespace from closing.
+ if (config.fork_config == ForkConfig::kDisabled) {
+ minijail_namespace_pids(j.get());
+ }
minijail_namespace_uts(j.get());
minijail_namespace_vfs(j.get());
if (minijail_enter_pivot_root(j.get(), "/mnt/empty") != 0) {
@@ -122,19 +140,19 @@
umask(0077);
}
-EntryManager* GetEntryManagerOrDie(SeccompEnforcement seccomp) {
+EntryManager* GetEntryManagerOrDie(const Configuration& config) {
if (!EntryManager::CreateDefaultGlobalDB()) {
LOG(FATAL) << "Unable to create default global DB!";
}
- DropPrivileges(seccomp);
- EntryManager* entry_manager = EntryManager::GetInstance();
+ DropPrivileges(config);
+ EntryManager* entry_manager = EntryManager::GetInstance(GetRuleFromDevPath);
if (!entry_manager) {
LOG(FATAL) << "EntryManager::GetInstance() failed!";
}
return entry_manager;
}
-int HandleAuthorizeAll(SeccompEnforcement seccomp,
+int HandleAuthorizeAll(const Configuration& config,
const std::vector<std::string>& argv) {
if (!argv.empty()) {
LOG(ERROR) << "Invalid options!";
@@ -148,14 +166,14 @@
return EXIT_SUCCESS;
}
-int HandleCleanup(SeccompEnforcement seccomp,
+int HandleCleanup(const Configuration& config,
const std::vector<std::string>& argv) {
if (!argv.empty()) {
LOG(ERROR) << "Invalid options!";
return EXIT_FAILURE;
}
- EntryManager* entry_manager = GetEntryManagerOrDie(seccomp);
+ EntryManager* entry_manager = GetEntryManagerOrDie(config);
if (!entry_manager->GarbageCollect()) {
LOG(ERROR) << "cleanup failed!";
return EXIT_FAILURE;
@@ -163,14 +181,14 @@
return EXIT_SUCCESS;
}
-int HandleGenRules(SeccompEnforcement seccomp,
+int HandleGenRules(const Configuration& config,
const std::vector<std::string>& argv) {
if (!argv.empty()) {
LOG(ERROR) << "Invalid options!";
return EXIT_FAILURE;
}
- EntryManager* entry_manager = GetEntryManagerOrDie(seccomp);
+ EntryManager* entry_manager = GetEntryManagerOrDie(config);
std::string rules = entry_manager->GenerateRules();
if (rules.empty()) {
LOG(ERROR) << "genrules failed!";
@@ -181,7 +199,7 @@
return EXIT_SUCCESS;
}
-int HandleUdev(SeccompEnforcement seccomp,
+int HandleUdev(const Configuration& config,
const std::vector<std::string>& argv) {
if (argv.size() != 2) {
LOG(ERROR) << "Invalid options!";
@@ -198,30 +216,61 @@
return EXIT_FAILURE;
}
- // The return code isn't checked because LOG statements are already present in
- // the function. The return value allows tests to validate the behavior.
- ForkAndWaitIfNotReady(base::BindRepeating([]() -> bool {
- return base::PathExists(base::FilePath(kLogPath)) &&
- base::PathExists(base::FilePath(kDBusPath));
- }),
- "logging or D-Bus isn't ready");
+ // We need to drop privileges prior to reading from sysfs so instead of
+ // calling GetEntryManagerOrDie split it up so the privileges can be dropped
+ // earlier.
+ if (!EntryManager::CreateDefaultGlobalDB()) {
+ LOG(FATAL) << "Unable to create default global DB!";
+ }
+ DropPrivileges(config);
- EntryManager* entry_manager = GetEntryManagerOrDie(seccomp);
- if (!entry_manager->HandleUdev(action, argv[1])) {
+ // Perform sysfs reads before daemonizing to avoid races.
+ const std::string& devpath = argv[1];
+ std::string rule;
+ if (action == EntryManager::UdevAction::kAdd) {
+ rule = GetRuleFromDevPath(devpath);
+ if (rule.empty()) {
+ LOG(ERROR) << "Unable convert devpath to USBGuard allow-list rule.";
+ exit(0);
+ }
+ }
+
+ // All the information needed from udev and sysfs should be obtained prior to
+ // this point. Daemonizing here allows usb_bouncer to wait on other system
+ // services without blocking udev.
+ if (config.fork_config == ForkConfig::kDoubleFork) {
+ Daemonize();
+ }
+
+ // The DevpathToRuleCallback here to forwards the result of the sysfs read
+ // performed before daemonizing.
+ EntryManager* entry_manager = EntryManager::GetInstance(
+ [rule, devpath](const std::string& devpath_) -> const std::string {
+ if (devpath != devpath_) {
+ LOG(ERROR) << "Got devpath: '" << devpath_ << "' expected '"
+ << devpath;
+ return "";
+ }
+ return rule;
+ });
+ if (!entry_manager) {
+ LOG(FATAL) << "EntryManager::GetInstance() failed!";
+ }
+ if (!entry_manager->HandleUdev(action, devpath)) {
LOG(ERROR) << "udev failed!";
return EXIT_FAILURE;
}
return EXIT_SUCCESS;
}
-int HandleUserLogin(SeccompEnforcement seccomp,
+int HandleUserLogin(const Configuration& config,
const std::vector<std::string>& argv) {
if (!argv.empty()) {
LOG(ERROR) << "Invalid options!";
return EXIT_FAILURE;
}
- EntryManager* entry_manager = GetEntryManagerOrDie(seccomp);
+ EntryManager* entry_manager = GetEntryManagerOrDie(config);
if (!entry_manager->HandleUserLogin()) {
LOG(ERROR) << "userlogin failed!";
return EXIT_FAILURE;
@@ -237,6 +286,8 @@
DCHECK_IS_ON()
? "Enable or disable seccomp filtering."
: "Flag is ignored in production, but reported as a crash if false.");
+ DEFINE_bool(fork, false,
+ "Daemonizes udev commands with a double fork if enabled.");
brillo::FlagHelper::Init(argc, argv, kUsageMessage);
base::CommandLine::Init(argc, argv);
@@ -289,10 +340,12 @@
} else {
seccomp = SeccompEnforcement::kEnabled;
}
+ ForkConfig fork_config =
+ FLAGS_fork ? ForkConfig::kDoubleFork : ForkConfig::kDisabled;
const struct {
const std::string command;
- int (*handler)(SeccompEnforcement seccomp,
+ int (*handler)(const Configuration& config,
const std::vector<std::string>& argv);
} command_handlers[] = {
// clang-format off
@@ -307,7 +360,8 @@
for (const auto& command_handler : command_handlers) {
if (command_handler.command == command) {
return command_handler.handler(
- seccomp, std::vector<std::string>(command_args_start, args.end()));
+ {seccomp, fork_config},
+ std::vector<std::string>(command_args_start, args.end()));
}
}
diff --git a/usb_bouncer/util.cc b/usb_bouncer/util.cc
index e7b105a..25fe6f8 100644
--- a/usb_bouncer/util.cc
+++ b/usb_bouncer/util.cc
@@ -601,48 +601,38 @@
return num_removed;
}
-pid_t DoubleFork() {
- if (fork() != 0) {
+void Daemonize() {
+ pid_t result = fork();
+ if (result < 0) {
+ PLOG(FATAL) << "First fork failed";
+ }
+ if (result != 0) {
exit(0);
}
setsid();
- return fork();
-}
-
-bool ForkAndWaitIfNotReady(const base::RepeatingCallback<bool()> ready,
- const std::string message,
- const base::TimeDelta& timeout,
- base::Callback<pid_t()> fork_func) {
- if (ready.Run()) {
- return true;
+ result = fork();
+ if (result < 0) {
+ PLOG(FATAL) << "Second fork failed";
}
-
- // Exit success for the parent to allow udev to continue but fork so the event
- // can be handled once logging is available.
- if (fork_func.Run() != 0) {
+ if (result != 0) {
exit(0);
}
- if (ready.Run()) {
- LOG(INFO) << "Forked because " << message;
- return true;
+ // Since we're demonizing we don't expect to ever read or write from the
+ // standard file descriptors. Also, udev waits for the hangup before
+ // continuing to execute on the same event, so this is necessary to unblock
+ // udev.
+ if (freopen("/dev/null", "a+", stdout) == nullptr) {
+ LOG(FATAL) << "Failed to replace stdout.";
}
-
- const base::Time deadline = base::Time::Now() + timeout;
- const base::TimeDelta check_interval = base::TimeDelta::FromMilliseconds(250);
-
- while (base::Time::Now() < deadline) {
- base::PlatformThread::Sleep(check_interval);
- if (ready.Run()) {
- LOG(INFO) << "Forked because " << message;
- return true;
- }
+ if (freopen("/dev/null", "a+", stderr) == nullptr) {
+ LOG(FATAL) << "Failed to replace stdout.";
}
-
- LOG(ERROR) << "Timed out after forking because " << message;
- return false;
+ if (fclose(stdin) != 0) {
+ LOG(FATAL) << "Failed to close stdin.";
+ }
}
#define TO_STRING_HELPER(x) \
diff --git a/usb_bouncer/util.h b/usb_bouncer/util.h
index 669c282..09b2556 100644
--- a/usb_bouncer/util.h
+++ b/usb_bouncer/util.h
@@ -119,23 +119,13 @@
const std::string& state_file_name,
bool lock);
-// This implements a double fork with setsid for use with ForkAndWaitIfNotReady.
-pid_t DoubleFork();
-
-// Returns true if |ready| returns true. If it returns true immediately, no
-// further action is taken. Otherwise, the process is forked and the parent
-// exits immediately. The child will wait until |ready| returns true or the
-// |timeout| is reached. |message| is printed to the log as the reason for
-// forking the process.
+// Forks (exiting the parent), calls setsid, and returns the result of a second
+// fork.
//
// This is used to avoid blocking udev while waiting on journald to finish
-// setting up logging or D-Bus to be ready. |fork_func| is provided for
-// testability (note that if fork_func returns non-zero, exit(0) is called).
-bool ForkAndWaitIfNotReady(
- const base::RepeatingCallback<bool()> ready,
- const std::string message,
- const base::TimeDelta& timeout = base::TimeDelta::FromSeconds(5),
- base::Callback<pid_t()> fork_func = base::Bind(&DoubleFork));
+// setting up logging, D-Bus to be ready, or D-Bus calls that can take on the
+// order of seconds to complete.
+void Daemonize();
void UpdateTimestamp(Timestamp* timestamp);
size_t RemoveEntriesOlderThan(base::TimeDelta cutoff, EntryMap* map);
diff --git a/usb_bouncer/util_test.cc b/usb_bouncer/util_test.cc
index ed466e9..ec2b2a0 100644
--- a/usb_bouncer/util_test.cc
+++ b/usb_bouncer/util_test.cc
@@ -52,21 +52,6 @@
CheckEnabledFlag(dir.Append(kSysFSAuthorizedDefault));
}
-// Mock for testing ForkAndWaitIfDoesNotExist(...). It sets the |taken| to true,
-// and always returns 0. If |to_create| isn't empty, it creates a directory at
-// that path to make it possible to test the case the path is created after the
-// fork.
-int ForkMock(bool* taken, const base::FilePath& to_create) {
- *taken = true;
-
- if (!to_create.empty()) {
- base::CreateDirectory(to_create);
- }
-
- // Always take the child path, because the parent path exits.
- return 0;
-}
-
} // namespace
TEST(UtilTest, IncludeRuleAtLockscreen) {
@@ -124,51 +109,6 @@
EXPECT_TRUE(CheckDeviceNodeAuthorized(deepdir));
}
-TEST(UtilTest, ForkAndWaitIfNotReady_Ready) {
- base::FilePath empty_path;
- base::ScopedTempDir temp_dir_;
- ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()) << strerror(errno);
- const base::FilePath temp_dir_path = temp_dir_.GetPath();
-
- bool fork_taken = false;
- EXPECT_TRUE(ForkAndWaitIfNotReady(
- base::BindLambdaForTesting(
- [&temp_dir_path]() -> bool { return PathExists(temp_dir_path); }),
- "temp_dir doesn't exist", base::TimeDelta::FromSeconds(0),
- base::Bind(&ForkMock, base::Unretained(&fork_taken), empty_path)));
- EXPECT_FALSE(fork_taken);
-}
-
-TEST(UtilTest, ForkAndWaitIfPathUnavailable_PathDoesntExistTimeout) {
- base::FilePath empty_path;
- base::ScopedTempDir temp_dir_;
- ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()) << strerror(errno);
- base::FilePath sub_dir_ = temp_dir_.GetPath().Append("DoesNotExist");
- LOG(INFO) << "PATH: " << sub_dir_.value();
-
- bool fork_taken = false;
- EXPECT_FALSE(ForkAndWaitIfNotReady(
- base::BindLambdaForTesting(
- [&sub_dir_]() -> bool { return PathExists(sub_dir_); }),
- "sub_dir_ doesn't exist", base::TimeDelta::FromSeconds(0),
- base::Bind(&ForkMock, base::Unretained(&fork_taken), empty_path)));
- EXPECT_TRUE(fork_taken);
-}
-
-TEST(UtilTest, ForkAndWaitIfPathUnavailable_PathDoesntExistSucceed) {
- base::ScopedTempDir temp_dir_;
- ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()) << strerror(errno);
- base::FilePath sub_dir_ = temp_dir_.GetPath().Append("ExistsAfterFork");
-
- bool fork_taken = false;
- EXPECT_TRUE(ForkAndWaitIfNotReady(
- base::BindLambdaForTesting(
- [&sub_dir_]() -> bool { return PathExists(sub_dir_); }),
- "sub_dir_ doesn't exist", base::TimeDelta::FromSeconds(1),
- base::Bind(&ForkMock, base::Unretained(&fork_taken), sub_dir_)));
- EXPECT_TRUE(fork_taken);
-}
-
TEST(UtilTest, GetRuleFromString_Success) {
EXPECT_TRUE(GetRuleFromString("allow id 0000:0000"));
}
