From f1e3bfff40560c311c00474e640f59fc950acf5c Mon Sep 17 00:00:00 2001
From: Nikita Ioffe <ioffe@google.com>
Date: Thu, 22 Dec 2022 16:05:40 +0000
Subject: [PATCH] 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
---
 init/host_init_verifier.cpp | 84 +++++++++++++++++++++++++++++++++++++
 init/service.h              |  2 +
 2 files changed, 86 insertions(+)

diff --git a/init/host_init_verifier.cpp b/init/host_init_verifier.cpp
index db127d3f2..d015ae9c8 100644
--- a/init/host_init_verifier.cpp
+++ b/init/host_init_verifier.cpp
@@ -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;
 }
 
diff --git a/init/service.h b/init/service.h
index f9749d207..9cc292093 100644
--- a/init/service.h
+++ b/init/service.h
@@ -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);