host_init_verifier: add check for root services and linux capabilities
If a service that runs under root doesn't have the capabilities field in it's definition, then it will inherit all the capabilities that init has. This change adds a linter to detect such services and ask developers to explicitly specify capabilities that their service needs. If service doesn't require any capabilities then empty capabilities fields should be added in the service definition. The actual access control list on what capabilities a process can use is controlled by the SELinux, so inheriting all the init capabilities is not a security issue here. However, asking services to explicitly specify the capabilities they need is a good defense-in-depth mechanism. So far this linter only checks the services on /system partition. All currently offending services are added to the exempt list. I will work on fixing some of them in the follow-up changes. Bug: 249796710 Test: m dist Change-Id: I2db06af165ae320a9c5086756067dceef20cd28d
This commit is contained in:
parent
cc2e7c21a2
commit
f1e3bfff40
2 changed files with 86 additions and 0 deletions
|
@ -22,6 +22,7 @@
|
|||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
|
||||
#include <cstdlib>
|
||||
#include <fstream>
|
||||
#include <iostream>
|
||||
#include <iterator>
|
||||
|
@ -216,6 +217,80 @@ void HandlePropertyContexts(const std::string& filename,
|
|||
}
|
||||
}
|
||||
|
||||
bool CheckServiceCapabilities(const ServiceList& service_list,
|
||||
const std::set<std::string>& system_services) {
|
||||
static const std::set<std::string> kExemptList = {
|
||||
"apexd",
|
||||
"apexd-bootstrap",
|
||||
"apexd-snapshotde",
|
||||
"adbd",
|
||||
"boottrace",
|
||||
"boringssl_self_test32",
|
||||
"boringssl_self_test64",
|
||||
"boringssl_self_test_apex32",
|
||||
"boringssl_self_test_apex64",
|
||||
"bsplogstart",
|
||||
"bugreportd",
|
||||
"charger",
|
||||
"clear-bcb",
|
||||
"composd",
|
||||
"dumpstate",
|
||||
"dumpstatez",
|
||||
"fastbootd",
|
||||
"gsid",
|
||||
"installd",
|
||||
"mmedialogstart",
|
||||
"mobile_log_d",
|
||||
// Yes, it's contorl, not control :(
|
||||
"mobile_log_d_contorl",
|
||||
"mobile_log_d_sublog_config",
|
||||
"odsign",
|
||||
"profcollectd",
|
||||
"recovery",
|
||||
"recovery-console",
|
||||
"servicemanager",
|
||||
"setup-bcb",
|
||||
"snapuserd",
|
||||
"snapuserd_proxy",
|
||||
"sysproxyd",
|
||||
"trace_buf_off",
|
||||
"ueventd",
|
||||
"uncrypt",
|
||||
"update_engine",
|
||||
"update_verifier",
|
||||
"update_verifier_nonencrypted",
|
||||
"usbd",
|
||||
"vold",
|
||||
"zygote",
|
||||
"zygote_secondary",
|
||||
};
|
||||
bool found_error = false;
|
||||
for (const auto& service : service_list) {
|
||||
if (service->uid() != 0) {
|
||||
continue;
|
||||
}
|
||||
// TODO(b/249796710): enable this linter for other partitions as well
|
||||
if (system_services.count(service->name()) == 0) {
|
||||
LOG(DEBUG) << "Skipping capabilities check for '" << service->name()
|
||||
<< "' because it doesn't belong to system partition";
|
||||
continue;
|
||||
}
|
||||
if (!service->capabilities().has_value() && kExemptList.count(service->name()) == 0) {
|
||||
LOG(ERROR) << "Service '" << service->name() << "' (defined in " << service->filename()
|
||||
<< ") runs under 'root' user but does not "
|
||||
<< "specify capabiltiies it needs. This will result in service inheriting "
|
||||
"all the "
|
||||
<< "capabilities that 'init' has. Please explicitly specify the "
|
||||
"capabilities that '"
|
||||
<< service->name()
|
||||
<< "' need. If it doesn't need any capabilities then leave the "
|
||||
"'capabilities' field empty.";
|
||||
found_error = true;
|
||||
}
|
||||
}
|
||||
return !found_error;
|
||||
}
|
||||
|
||||
int main(int argc, char** argv) {
|
||||
android::base::InitLogging(argv, &android::base::StdioLogger);
|
||||
android::base::SetMinimumLogSeverity(android::base::ERROR);
|
||||
|
@ -319,11 +394,17 @@ int main(int argc, char** argv) {
|
|||
parser.AddSectionParser("on", std::make_unique<ActionParser>(&am, GetSubcontext()));
|
||||
parser.AddSectionParser("import", std::make_unique<HostImportParser>());
|
||||
|
||||
std::set<std::string> system_services;
|
||||
if (!partition_map.empty()) {
|
||||
for (const auto& p : partition_search_order) {
|
||||
if (partition_map.find(p) != partition_map.end()) {
|
||||
parser.ParseConfig(partition_map.at(p) + "etc/init");
|
||||
}
|
||||
if (p == "system") {
|
||||
for (const auto& service : ServiceList::GetInstance()) {
|
||||
system_services.insert(service->name());
|
||||
}
|
||||
}
|
||||
}
|
||||
} else {
|
||||
if (!parser.ParseConfigFileInsecure(*argv)) {
|
||||
|
@ -336,6 +417,9 @@ int main(int argc, char** argv) {
|
|||
LOG(ERROR) << "Failed to parse init scripts with " << failures << " error(s).";
|
||||
return EXIT_FAILURE;
|
||||
}
|
||||
if (!CheckServiceCapabilities(sl, system_services)) {
|
||||
return EXIT_FAILURE;
|
||||
}
|
||||
return EXIT_SUCCESS;
|
||||
}
|
||||
|
||||
|
|
|
@ -145,6 +145,8 @@ class Service {
|
|||
const std::string& filename() const { return filename_; }
|
||||
void set_filename(const std::string& name) { filename_ = name; }
|
||||
|
||||
const std::optional<CapSet>& capabilities() const { return capabilities_; }
|
||||
|
||||
private:
|
||||
void NotifyStateChange(const std::string& new_state) const;
|
||||
void StopOrReset(int how);
|
||||
|
|
Loading…
Reference in a new issue