From 905e2e85346bd003703c4fc32ee300cbde8c8d0c Mon Sep 17 00:00:00 2001
From: Steven Moreland <smoreland@google.com>
Date: Wed, 17 Jul 2019 11:05:45 -0700
Subject: servicemanager: restrict service name characters

This is in order to, in the future, potentially have special policy
like:
vendor$vendor_service u:object_r:...:s0
universal$hal_foo_service u:object_r:...:s0

Currently, there are no special characters that could be used for this
purpose.

How safe is this? In all context files in our internal tree, only the
following characters are used in service names:
-._12aAbBcCdDeEfFgGhHiIjkKlmMnNoOpPqQrsStTuUvwWxyz

'.' appears everywhere to mean '\.'.

Test: servicemanager_test
Bug: 136027762
Change-Id: Idd6698a68bbbe825dd5a25d92baf81fae473ea2a
---
 cmds/servicemanager/ServiceManager.cpp | 19 +++++++++++++++++--
 cmds/servicemanager/test_sm.cpp        |  6 ++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/cmds/servicemanager/ServiceManager.cpp b/cmds/servicemanager/ServiceManager.cpp
index b88b67d138..6cfcf40970 100644
--- a/cmds/servicemanager/ServiceManager.cpp
+++ b/cmds/servicemanager/ServiceManager.cpp
@@ -63,6 +63,21 @@ Status ServiceManager::checkService(const std::string& name, sp<IBinder>* outBin
     return Status::ok();
 }
 
+bool isValidServiceName(const std::string& name) {
+    if (name.size() == 0) return false;
+    if (name.size() > 127) return false;
+
+    for (char c : name) {
+        if (c == '_' || c == '-' || c == '.') continue;
+        if (c >= 'a' && c <= 'z') continue;
+        if (c >= 'A' && c <= 'Z') continue;
+        if (c >= '0' && c <= '9') continue;
+        return false;
+    }
+
+    return true;
+}
+
 Status ServiceManager::addService(const std::string& name, const sp<IBinder>& binder, bool allowIsolated, int32_t dumpPriority) {
     auto ctx = mAccess->getCallingContext(name);
 
@@ -79,8 +94,8 @@ Status ServiceManager::addService(const std::string& name, const sp<IBinder>& bi
         return Status::fromExceptionCode(Status::EX_ILLEGAL_ARGUMENT);
     }
 
-    // match legacy rules
-    if (name.size() == 0 || name.size() > 127) {
+    if (!isValidServiceName(name)) {
+        LOG(ERROR) << "Invalid service name: " << name;
         return Status::fromExceptionCode(Status::EX_ILLEGAL_ARGUMENT);
     }
 
diff --git a/cmds/servicemanager/test_sm.cpp b/cmds/servicemanager/test_sm.cpp
index 812d5cacd5..25d2aa8bfa 100644
--- a/cmds/servicemanager/test_sm.cpp
+++ b/cmds/servicemanager/test_sm.cpp
@@ -82,6 +82,12 @@ TEST(AddService, TooLongNameDisallowed) {
         IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());
 }
 
+TEST(AddService, WeirdCharactersDisallowed) {
+    auto sm = getPermissiveServiceManager();
+    EXPECT_FALSE(sm->addService("happy$foo$foo", getBinder(), false /*allowIsolated*/,
+        IServiceManager::DUMP_FLAG_PRIORITY_DEFAULT).isOk());
+}
+
 TEST(AddService, AddNullServiceDisallowed) {
     auto sm = getPermissiveServiceManager();
     EXPECT_FALSE(sm->addService("foo", nullptr, false /*allowIsolated*/,
-- 
cgit v1.2.3-59-g8ed1b