logd: clientHasLogCredentials false negatives

Vote three times in /proc/pid/status to look for AID_LOG group

If not, we may default to the callers UID, and the net result is
to perform the task related to that UID. For adb logcat and
shell logcat, the UID is AID_SHELL which typically has no logs,
leaving no net action taken.

Bug: 23711431
Change-Id: I2b5900a2d37173bd995eb308ee9ecafa20602b62

1 files changed, 68 insertions, 51 deletions

diff --git a/logd/LogCommand.cpp b/logd/LogCommand.cpp
index 06d865cd5..6d0e92e4e 100644
--- a/logd/LogCommand.cpp
+++ b/logd/LogCommand.cpp
@@ -42,7 +42,6 @@ LogCommand::LogCommand(const char *cmd) : FrameworkCommand(cmd) {
 static bool groupIsLog(char *buf) {
     char *ptr;
     static const char ws[] = " \n";
-    bool ret = false;
 
     for (buf = strtok_r(buf, ws, &ptr); buf; buf = strtok_r(NULL, ws, &ptr)) {
         errno = 0;
@@ -51,10 +50,10 @@ static bool groupIsLog(char *buf) {
             return false;
         }
         if (Gid == AID_LOG) {
-            ret = true;
+            return true;
         }
     }
-    return ret;
+    return false;
 }
 
 bool clientHasLogCredentials(SocketClient * cli) {
@@ -69,61 +68,79 @@ bool clientHasLogCredentials(SocketClient * cli) {
     }
 
     // FYI We will typically be here for 'adb logcat'
-    bool ret = false;
-
-    char filename[1024];
-    snprintf(filename, sizeof(filename), "/proc/%d/status", cli->getPid());
-
-    FILE *file = fopen(filename, "r");
-    if (!file) {
-        return ret;
-    }
+    char filename[256];
+    snprintf(filename, sizeof(filename), "/proc/%u/status", cli->getPid());
 
+    bool ret;
+    bool foundLog = false;
     bool foundGid = false;
     bool foundUid = false;
 
-    char line[1024];
-    while (fgets(line, sizeof(line), file)) {
-        static const char groups_string[] = "Groups:\t";
-        static const char uid_string[] = "Uid:\t";
-        static const char gid_string[] = "Gid:\t";
-
-        if (strncmp(groups_string, line, strlen(groups_string)) == 0) {
-            ret = groupIsLog(line + strlen(groups_string));
-            if (!ret) {
-                break;
-            }
-        } else if (strncmp(uid_string, line, strlen(uid_string)) == 0) {
-            uid_t u[4] = { (uid_t) -1, (uid_t) -1, (uid_t) -1, (uid_t) -1};
-
-            sscanf(line + strlen(uid_string), "%u\t%u\t%u\t%u",
-                   &u[0], &u[1], &u[2], &u[3]);
-
-            // Protect against PID reuse by checking that the UID is the same
-            if ((uid != u[0]) || (uid != u[1]) || (uid != u[2]) || (uid != u[3])) {
-                ret = false;
-                break;
-            }
-            foundUid = true;
-        } else if (strncmp(gid_string, line, strlen(gid_string)) == 0) {
-            gid_t g[4] = { (gid_t) -1, (gid_t) -1, (gid_t) -1, (gid_t) -1};
-
-            sscanf(line + strlen(gid_string), "%u\t%u\t%u\t%u",
-                   &g[0], &g[1], &g[2], &g[3]);
+    //
+    // Reading /proc/<pid>/status is rife with race conditions. All of /proc
+    // suffers from this and its use should be minimized. However, we have no
+    // choice.
+    //
+    // Notably the content from one 4KB page to the next 4KB page can be from a
+    // change in shape even if we are gracious enough to attempt to read
+    // atomically. getline can not even guarantee a page read is not split up
+    // and in effect can read from different vintages of the content.
+    //
+    // We are finding out in the field that a 'logcat -c' via adb occasionally
+    // is returned with permission denied when we did only one pass and thus
+    // breaking scripts. For security we still err on denying access if in
+    // doubt, but we expect the falses  should be reduced significantly as
+    // three times is a charm.
+    //
+    for (int retry = 3;
+            !(ret = foundGid && foundUid && foundLog) && retry;
+            --retry) {
+        FILE *file = fopen(filename, "r");
+        if (!file) {
+            continue;
+        }
 
-            // Protect against PID reuse by checking that the GID is the same
-            if ((gid != g[0]) || (gid != g[1]) || (gid != g[2]) || (gid != g[3])) {
-                ret = false;
-                break;
+        char *line = NULL;
+        size_t len = 0;
+        while (getline(&line, &len, file) > 0) {
+            static const char groups_string[] = "Groups:\t";
+            static const char uid_string[] = "Uid:\t";
+            static const char gid_string[] = "Gid:\t";
+
+            if (strncmp(groups_string, line, sizeof(groups_string) - 1) == 0) {
+                if (groupIsLog(line + sizeof(groups_string) - 1)) {
+                    foundLog = true;
+                }
+            } else if (strncmp(uid_string, line, sizeof(uid_string) - 1) == 0) {
+                uid_t u[4] = { (uid_t) -1, (uid_t) -1, (uid_t) -1, (uid_t) -1};
+
+                sscanf(line + sizeof(uid_string) - 1, "%u\t%u\t%u\t%u",
+                       &u[0], &u[1], &u[2], &u[3]);
+
+                // Protect against PID reuse by checking that UID is the same
+                if ((uid == u[0])
+                        && (uid == u[1])
+                        && (uid == u[2])
+                        && (uid == u[3])) {
+                    foundUid = true;
+                }
+            } else if (strncmp(gid_string, line, sizeof(gid_string) - 1) == 0) {
+                gid_t g[4] = { (gid_t) -1, (gid_t) -1, (gid_t) -1, (gid_t) -1};
+
+                sscanf(line + sizeof(gid_string) - 1, "%u\t%u\t%u\t%u",
+                       &g[0], &g[1], &g[2], &g[3]);
+
+                // Protect against PID reuse by checking that GID is the same
+                if ((gid == g[0])
+                        && (gid == g[1])
+                        && (gid == g[2])
+                        && (gid == g[3])) {
+                    foundGid = true;
+                }
             }
-            foundGid = true;
         }
-    }
-
-    fclose(file);
-
-    if (!foundGid || !foundUid) {
-        ret = false;
+        free(line);
+        fclose(file);
     }
 
     return ret;