From f5d22ef7cd31cc7120e9b724b040a06cd36ab870 Mon Sep 17 00:00:00 2001 From: Steven Moreland Date: Mon, 3 Apr 2023 23:29:22 +0000 Subject: [PATCH] init: log when 'user' is unspecified NOTE: in master, but should be submitted in AOSP. Waiting to hear from security folks. Also might need cleanup. Not currently done. Seems errorprone. Bug: 276813155 Test: boot, check logs Change-Id: I7cbc39b282889dd582f06a8eedc38ae637c8edec --- init/service.cpp | 36 ++++++++++++++++++------------------ init/service.h | 4 ++-- init/service_parser.cpp | 7 ++++++- init/service_utils.cpp | 4 ++-- init/service_utils.h | 4 +++- 5 files changed, 31 insertions(+), 24 deletions(-) diff --git a/init/service.cpp b/init/service.cpp index 35beaad33..c1520818a 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -140,9 +140,10 @@ bool Service::is_exec_service_running_ = false; Service::Service(const std::string& name, Subcontext* subcontext_for_restart_commands, const std::string& filename, const std::vector& args) - : Service(name, 0, 0, 0, {}, 0, "", subcontext_for_restart_commands, filename, args) {} + : Service(name, 0, std::nullopt, 0, {}, 0, "", subcontext_for_restart_commands, filename, + args) {} -Service::Service(const std::string& name, unsigned flags, uid_t uid, gid_t gid, +Service::Service(const std::string& name, unsigned flags, std::optional uid, gid_t gid, const std::vector& supp_gids, int namespace_flags, const std::string& seclabel, Subcontext* subcontext_for_restart_commands, const std::string& filename, const std::vector& args) @@ -153,7 +154,7 @@ Service::Service(const std::string& name, unsigned flags, uid_t uid, gid_t gid, crash_count_(0), proc_attr_{.ioprio_class = IoSchedClass_NONE, .ioprio_pri = 0, - .uid = uid, + .parsed_uid = uid, .gid = gid, .supp_gids = supp_gids, .priority = 0}, @@ -205,9 +206,9 @@ void Service::KillProcessGroup(int signal, bool report_oneshot) { int max_processes = 0; int r; if (signal == SIGTERM) { - r = killProcessGroupOnce(proc_attr_.uid, pid_, signal, &max_processes); + r = killProcessGroupOnce(uid(), pid_, signal, &max_processes); } else { - r = killProcessGroup(proc_attr_.uid, pid_, signal, &max_processes); + r = killProcessGroup(uid(), pid_, signal, &max_processes); } if (report_oneshot && max_processes > 0) { @@ -228,7 +229,7 @@ void Service::KillProcessGroup(int signal, bool report_oneshot) { void Service::SetProcessAttributesAndCaps(InterprocessFifo setsid_finished) { // Keep capabilites on uid change. - if (capabilities_ && proc_attr_.uid) { + if (capabilities_ && uid()) { // If Android is running in a container, some securebits might already // be locked, so don't change those. unsigned long securebits = prctl(PR_GET_SECUREBITS); @@ -255,7 +256,7 @@ void Service::SetProcessAttributesAndCaps(InterprocessFifo setsid_finished) { if (!SetCapsForExec(*capabilities_)) { LOG(FATAL) << "cannot set capabilities for " << name_; } - } else if (proc_attr_.uid) { + } else if (uid()) { // Inheritable caps can be non-zero when running in a container. if (!DropInheritableCaps()) { LOG(FATAL) << "cannot drop inheritable caps for " << name_; @@ -434,8 +435,8 @@ Result Service::ExecStart() { flags_ |= SVC_EXEC; is_exec_service_running_ = true; - LOG(INFO) << "SVC_EXEC service '" << name_ << "' pid " << pid_ << " (uid " << proc_attr_.uid - << " gid " << proc_attr_.gid << "+" << proc_attr_.supp_gids.size() << " context " + LOG(INFO) << "SVC_EXEC service '" << name_ << "' pid " << pid_ << " (uid " << uid() << " gid " + << proc_attr_.gid << "+" << proc_attr_.supp_gids.size() << " context " << (!seclabel_.empty() ? seclabel_ : "default") << ") started; waiting..."; reboot_on_failure.Disable(); @@ -475,13 +476,13 @@ Result Service::CheckConsole() { // Configures the memory cgroup properties for the service. void Service::ConfigureMemcg() { if (swappiness_ != -1) { - if (!setProcessGroupSwappiness(proc_attr_.uid, pid_, swappiness_)) { + if (!setProcessGroupSwappiness(uid(), pid_, swappiness_)) { PLOG(ERROR) << "setProcessGroupSwappiness failed"; } } if (soft_limit_in_bytes_ != -1) { - if (!setProcessGroupSoftLimit(proc_attr_.uid, pid_, soft_limit_in_bytes_)) { + if (!setProcessGroupSoftLimit(uid(), pid_, soft_limit_in_bytes_)) { PLOG(ERROR) << "setProcessGroupSoftLimit failed"; } } @@ -508,7 +509,7 @@ void Service::ConfigureMemcg() { } if (computed_limit_in_bytes != size_t(-1)) { - if (!setProcessGroupLimit(proc_attr_.uid, pid_, computed_limit_in_bytes)) { + if (!setProcessGroupLimit(uid(), pid_, computed_limit_in_bytes)) { PLOG(ERROR) << "setProcessGroupLimit failed"; } } @@ -705,21 +706,20 @@ Result Service::Start() { if (CgroupsAvailable()) { bool use_memcg = swappiness_ != -1 || soft_limit_in_bytes_ != -1 || limit_in_bytes_ != -1 || limit_percent_ != -1 || !limit_property_.empty(); - errno = -createProcessGroup(proc_attr_.uid, pid_, use_memcg); + errno = -createProcessGroup(uid(), pid_, use_memcg); if (errno != 0) { Result result = cgroups_activated.Write(kActivatingCgroupsFailed); if (!result.ok()) { return Error() << "Sending notification failed: " << result.error(); } - return Error() << "createProcessGroup(" << proc_attr_.uid << ", " << pid_ << ", " - << use_memcg << ") failed for service '" << name_ - << "': " << strerror(errno); + return Error() << "createProcessGroup(" << uid() << ", " << pid_ << ", " << use_memcg + << ") failed for service '" << name_ << "': " << strerror(errno); } // When the blkio controller is mounted in the v1 hierarchy, NormalIoPriority is // the default (/dev/blkio). When the blkio controller is mounted in the v2 hierarchy, the // NormalIoPriority profile has to be applied explicitly. - SetProcessProfiles(proc_attr_.uid, pid_, {"NormalIoPriority"}); + SetProcessProfiles(uid(), pid_, {"NormalIoPriority"}); if (use_memcg) { ConfigureMemcg(); @@ -727,7 +727,7 @@ Result Service::Start() { } if (oom_score_adjust_ != DEFAULT_OOM_SCORE_ADJUST) { - LmkdRegister(name_, proc_attr_.uid, pid_, oom_score_adjust_); + LmkdRegister(name_, uid(), pid_, oom_score_adjust_); } if (Result result = cgroups_activated.Write(kCgroupsActivated); !result.ok()) { diff --git a/init/service.h b/init/service.h index 3ef890299..ce7c0daf2 100644 --- a/init/service.h +++ b/init/service.h @@ -71,7 +71,7 @@ class Service { Service(const std::string& name, Subcontext* subcontext_for_restart_commands, const std::string& filename, const std::vector& args); - Service(const std::string& name, unsigned flags, uid_t uid, gid_t gid, + Service(const std::string& name, unsigned flags, std::optional uid, gid_t gid, const std::vector& supp_gids, int namespace_flags, const std::string& seclabel, Subcontext* subcontext_for_restart_commands, const std::string& filename, const std::vector& args); @@ -115,7 +115,7 @@ class Service { pid_t pid() const { return pid_; } android::base::boot_clock::time_point time_started() const { return time_started_; } int crash_count() const { return crash_count_; } - uid_t uid() const { return proc_attr_.uid; } + uid_t uid() const { return proc_attr_.uid(); } gid_t gid() const { return proc_attr_.gid; } int namespace_flags() const { return namespaces_.flags; } const std::vector& supp_gids() const { return proc_attr_.supp_gids; } diff --git a/init/service_parser.cpp b/init/service_parser.cpp index 356308423..d89664c5b 100644 --- a/init/service_parser.cpp +++ b/init/service_parser.cpp @@ -534,7 +534,7 @@ Result ServiceParser::ParseUser(std::vector&& args) { if (!uid.ok()) { return Error() << "Unable to find UID for '" << args[1] << "': " << uid.error(); } - service_->proc_attr_.uid = *uid; + service_->proc_attr_.parsed_uid = *uid; return {}; } @@ -677,6 +677,11 @@ Result ServiceParser::EndSection() { return {}; } + if (service_->proc_attr_.parsed_uid == std::nullopt) { + LOG(WARNING) << "No user specified for service '" << service_->name() + << "'. Defaults to root."; + } + if (interface_inheritance_hierarchy_) { if (const auto& check_hierarchy_result = CheckInterfaceInheritanceHierarchy( service_->interfaces(), *interface_inheritance_hierarchy_); diff --git a/init/service_utils.cpp b/init/service_utils.cpp index 7004d8dc9..0e19bcc58 100644 --- a/init/service_utils.cpp +++ b/init/service_utils.cpp @@ -271,8 +271,8 @@ Result SetProcessAttributes(const ProcessAttributes& attr, InterprocessFif if (setgroups(attr.supp_gids.size(), const_cast(&attr.supp_gids[0])) != 0) { return ErrnoError() << "setgroups failed"; } - if (attr.uid) { - if (setuid(attr.uid) != 0) { + if (attr.uid()) { + if (setuid(attr.uid()) != 0) { return ErrnoError() << "setuid failed"; } } diff --git a/init/service_utils.h b/init/service_utils.h index d4143aa0b..27ec450e0 100644 --- a/init/service_utils.h +++ b/init/service_utils.h @@ -91,11 +91,13 @@ struct ProcessAttributes { IoSchedClass ioprio_class; int ioprio_pri; std::vector> rlimits; - uid_t uid; + std::optional parsed_uid; gid_t gid; std::vector supp_gids; int priority; bool stdio_to_kmsg; + + uid_t uid() const { return parsed_uid.value_or(0); } }; inline bool RequiresConsole(const ProcessAttributes& attr) {