Merge "StatsService allows uids to impersonate themselves"

3 files changed, 92 insertions, 58 deletions

diff --git a/cmds/statsd/src/StatsService.cpp b/cmds/statsd/src/StatsService.cpp
index 04173b217dcb..51a731680a56 100644
--- a/cmds/statsd/src/StatsService.cpp
+++ b/cmds/statsd/src/StatsService.cpp
@@ -466,23 +466,12 @@ status_t StatsService::cmd_trigger_broadcast(int out, Vector<String8>& args) {
         name.assign(args[1].c_str(), args[1].size());
         good = true;
     } else if (argCount == 3) {
-        // If it's a userdebug or eng build, then the shell user can
-        // impersonate other uids.
-        if (mEngBuild) {
-            const char* s = args[1].c_str();
-            if (*s != '\0') {
-                char* end = NULL;
-                uid = strtol(s, &end, 0);
-                if (*end == '\0') {
-                    name.assign(args[2].c_str(), args[2].size());
-                    good = true;
-                }
-            }
-        } else {
-            dprintf(out,
-                    "The metrics can only be dumped for other UIDs on eng or userdebug "
-                    "builds.\n");
+        good = getUidFromArgs(args, 1, uid);
+        if (!good) {
+            dprintf(out, "Invalid UID. Note that the metrics can only be dumped for "
+                         "other UIDs on eng or userdebug builds.\n");
         }
+        name.assign(args[2].c_str(), args[2].size());
     }
     if (!good) {
         print_cmd_help(out);
@@ -518,23 +507,12 @@ status_t StatsService::cmd_config(int in, int out, int err, Vector<String8>& arg
                 name.assign(args[2].c_str(), args[2].size());
                 good = true;
             } else if (argCount == 4) {
-                // If it's a userdebug or eng build, then the shell user can
-                // impersonate other uids.
-                if (mEngBuild) {
-                    const char* s = args[2].c_str();
-                    if (*s != '\0') {
-                        char* end = NULL;
-                        uid = strtol(s, &end, 0);
-                        if (*end == '\0') {
-                            name.assign(args[3].c_str(), args[3].size());
-                            good = true;
-                        }
-                    }
-                } else {
-                    dprintf(err,
-                            "The config can only be set for other UIDs on eng or userdebug "
-                            "builds.\n");
+                good = getUidFromArgs(args, 2, uid);
+                if (!good) {
+                    dprintf(err, "Invalid UID. Note that the config can only be set for "
+                                 "other UIDs on eng or userdebug builds.\n");
                 }
+                name.assign(args[3].c_str(), args[3].size());
             } else if (argCount == 2 && args[1] == "remove") {
                 good = true;
             }
@@ -612,23 +590,12 @@ status_t StatsService::cmd_dump_report(int out, const Vector<String8>& args) {
             name.assign(args[1].c_str(), args[1].size());
             good = true;
         } else if (argCount == 3) {
-            // If it's a userdebug or eng build, then the shell user can
-            // impersonate other uids.
-            if (mEngBuild) {
-                const char* s = args[1].c_str();
-                if (*s != '\0') {
-                    char* end = NULL;
-                    uid = strtol(s, &end, 0);
-                    if (*end == '\0') {
-                        name.assign(args[2].c_str(), args[2].size());
-                        good = true;
-                    }
-                }
-            } else {
-                dprintf(out,
-                        "The metrics can only be dumped for other UIDs on eng or userdebug "
-                        "builds.\n");
+            good = getUidFromArgs(args, 1, uid);
+            if (!good) {
+                dprintf(out, "Invalid UID. Note that the metrics can only be dumped for "
+                             "other UIDs on eng or userdebug builds.\n");
             }
+            name.assign(args[2].c_str(), args[2].size());
         }
         if (good) {
             vector<uint8_t> data;
@@ -714,18 +681,14 @@ status_t StatsService::cmd_log_app_breadcrumb(int out, const Vector<String8>& ar
         state = atoi(args[2].c_str());
         good = true;
     } else if (argCount == 4) {
-        uid = atoi(args[1].c_str());
-        // If it's a userdebug or eng build, then the shell user can impersonate other uids.
-        // Otherwise, the uid must match the actual caller's uid.
-        if (mEngBuild || (uid >= 0 && (uid_t)uid == IPCThreadState::self()->getCallingUid())) {
-            label = atoi(args[2].c_str());
-            state = atoi(args[3].c_str());
-            good = true;
-        } else {
+        good = getUidFromArgs(args, 1, uid);
+        if (!good) {
             dprintf(out,
-                    "Selecting a UID for writing AppBreadcrumb can only be done for other UIDs "
-                    "on eng or userdebug builds.\n");
+                    "Invalid UID. Note that selecting a UID for writing AppBreadcrumb can only be "
+                    "done for other UIDs on eng or userdebug builds.\n");
         }
+        label = atoi(args[2].c_str());
+        state = atoi(args[3].c_str());
     }
     if (good) {
         dprintf(out, "Logging AppBreadcrumbReported(%d, %d, %d) to statslog.\n", uid, label, state);
@@ -792,6 +755,28 @@ status_t StatsService::cmd_print_logs(int out, const Vector<String8>& args) {
     }
 }
 
+bool StatsService::getUidFromArgs(const Vector<String8>& args, size_t uidArgIndex, int32_t& uid) {
+    const char* s = args[uidArgIndex].c_str();
+    if (*s == '\0') {
+        return false;
+    }
+    char* endc = NULL;
+    int64_t longUid = strtol(s, &endc, 0);
+    if (*endc != '\0') {
+        return false;
+    }
+    int32_t goodUid = static_cast<int32_t>(longUid);
+    if (longUid < 0 || static_cast<uint64_t>(longUid) != static_cast<uid_t>(goodUid)) {
+        return false;  // It was not of uid_t type.
+    }
+    uid = goodUid;
+
+    int32_t callingUid = IPCThreadState::self()->getCallingUid();
+    return mEngBuild // UserDebug/EngBuild are allowed to impersonate uids.
+            || (callingUid == goodUid) // Anyone can 'impersonate' themselves.
+            || (callingUid == AID_ROOT && goodUid == AID_SHELL); // ROOT can impersonate SHELL.
+}
+
 Status StatsService::informAllUidData(const vector<int32_t>& uid, const vector<int64_t>& version,
                                       const vector<String16>& version_string,
                                       const vector<String16>& app,
diff --git a/cmds/statsd/src/StatsService.h b/cmds/statsd/src/StatsService.h
index cd4d601a606f..135a3c9cde51 100644
--- a/cmds/statsd/src/StatsService.h
+++ b/cmds/statsd/src/StatsService.h
@@ -291,6 +291,15 @@ private:
     status_t cmd_print_logs(int outFd, const Vector<String8>& args);
 
     /**
+     * Writes the value of args[uidArgIndex] into uid.
+     * Returns whether the uid is reasonable (type uid_t) and whether
+     * 1. it is equal to the calling uid, or
+     * 2. the device is mEngBuild, or
+     * 3. the caller is AID_ROOT and the uid is AID_SHELL (i.e. ROOT can impersonate SHELL).
+     */
+    bool getUidFromArgs(const Vector<String8>& args, size_t uidArgIndex, int32_t& uid);
+
+    /**
      * Adds a configuration after checking permissions and obtaining UID from binder call.
      */
     bool addConfigurationChecked(int uid, int64_t key, const vector<uint8_t>& config);
@@ -340,6 +349,7 @@ private:
     FRIEND_TEST(StatsServiceTest, TestAddConfig_simple);
     FRIEND_TEST(StatsServiceTest, TestAddConfig_empty);
     FRIEND_TEST(StatsServiceTest, TestAddConfig_invalid);
+    FRIEND_TEST(StatsServiceTest, TestGetUidFromArgs);
     FRIEND_TEST(PartialBucketE2eTest, TestCountMetricNoSplitOnNewApp);
     FRIEND_TEST(PartialBucketE2eTest, TestCountMetricSplitOnUpgrade);
     FRIEND_TEST(PartialBucketE2eTest, TestCountMetricSplitOnRemoval);
diff --git a/cmds/statsd/tests/StatsService_test.cpp b/cmds/statsd/tests/StatsService_test.cpp
index a7b413666266..560fb9f02174 100644
--- a/cmds/statsd/tests/StatsService_test.cpp
+++ b/cmds/statsd/tests/StatsService_test.cpp
@@ -58,6 +58,45 @@ TEST(StatsServiceTest, TestAddConfig_invalid) {
             service.addConfigurationChecked(123, 12345, {serialized.begin(), serialized.end()}));
 }
 
+TEST(StatsServiceTest, TestGetUidFromArgs) {
+    Vector<String8> args;
+    args.push(String8("-1"));
+    args.push(String8("0"));
+    args.push(String8("1"));
+    args.push(String8("9999999999999999999999999999999999"));
+    args.push(String8("a1"));
+    args.push(String8(""));
+
+    int32_t uid;
+
+    StatsService service(nullptr);
+    service.mEngBuild = true;
+
+    // "-1"
+    EXPECT_FALSE(service.getUidFromArgs(args, 0, uid));
+
+    // "0"
+    EXPECT_TRUE(service.getUidFromArgs(args, 1, uid));
+    EXPECT_EQ(0, uid);
+
+    // "1"
+    EXPECT_TRUE(service.getUidFromArgs(args, 2, uid));
+    EXPECT_EQ(1, uid);
+
+    // "999999999999999999"
+    EXPECT_FALSE(service.getUidFromArgs(args, 3, uid));
+
+    // "a1"
+    EXPECT_FALSE(service.getUidFromArgs(args, 4, uid));
+
+    // ""
+    EXPECT_FALSE(service.getUidFromArgs(args, 5, uid));
+
+    // For a non-userdebug, uid "1" cannot be impersonated.
+    service.mEngBuild = false;
+    EXPECT_FALSE(service.getUidFromArgs(args, 2, uid));
+}
+
 #else
 GTEST_LOG_(INFO) << "This test does nothing.\n";
 #endif