Merge "DO NOT MERGE: debuggerd: verify that traced threads belong to the right...

Merge "DO NOT MERGE: debuggerd: verify that traced threads belong to the right process." into klp-dev

debuggerd/backtrace.c

@@ -62,7 +62,7 @@ static void dump_process_footer(log_t* log, pid_t pid) {
     _LOG(log, SCOPE_AT_FAULT, "\n----- end %d -----\n", pid);
 }
 
-static void dump_thread(log_t* log, pid_t tid, ptrace_context_t* context, bool attached,
+static void dump_thread(log_t* log, pid_t pid, pid_t tid, ptrace_context_t* context, bool attached,
         bool* detach_failed, int* total_sleep_time_usec) {
     char path[PATH_MAX];
     char threadnamebuf[1024];
@@ -84,7 +84,7 @@ static void dump_thread(log_t* log, pid_t tid, ptrace_context_t* context, bool a
     _LOG(log, SCOPE_AT_FAULT, "\n\"%s\" sysTid=%d\n",
             threadname ? threadname : "<unknown>", tid);
 
-    if (!attached && ptrace(PTRACE_ATTACH, tid, 0, 0) < 0) {
+    if (!attached && !ptrace_attach_thread(pid, tid)) {
         _LOG(log, SCOPE_AT_FAULT, "Could not attach to thread: %s\n", strerror(errno));
         return;
     }
@@ -122,7 +122,7 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed,
 
     ptrace_context_t* context = load_ptrace_context(tid);
     dump_process_header(&log, pid);
-    dump_thread(&log, tid, context, true, detach_failed, total_sleep_time_usec);
+    dump_thread(&log, pid, tid, context, true, detach_failed, total_sleep_time_usec);
 
     char task_path[64];
     snprintf(task_path, sizeof(task_path), "/proc/%d/task", pid);
@@ -140,7 +140,7 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed,
                 continue;
             }
 
-            dump_thread(&log, new_tid, context, false, detach_failed, total_sleep_time_usec);
+            dump_thread(&log, pid, new_tid, context, false, detach_failed, total_sleep_time_usec);
         }
         closedir(d);
     }

debuggerd/debuggerd.c

@@ -233,13 +233,11 @@ static int read_request(int fd, debugger_request_t* out_request) {
 
     if (msg.action == DEBUGGER_ACTION_CRASH) {
         /* Ensure that the tid reported by the crashing process is valid. */
-        char buf[64];
-        struct stat s;
-        snprintf(buf, sizeof buf, "/proc/%d/task/%d", out_request->pid, out_request->tid);
-        if(stat(buf, &s)) {
-            LOG("tid %d does not exist in pid %d. ignoring debug request\n",
-                    out_request->tid, out_request->pid);
-            return -1;
+        // This check needs to happen again after ptracing the requested thread to prevent a race.
+        if (!pid_contains_tid(out_request->pid, out_request->tid)) {
+          XLOG("tid %d does not exist in pid %d. ignoring debug request\n", out_request->tid,
+               out_request->pid);
+          return -1;
         }
     } else if (cr.uid == 0
             || (cr.uid == AID_SYSTEM && msg.action == DEBUGGER_ACTION_DUMP_BACKTRACE)) {
@@ -290,9 +288,32 @@ static void handle_request(int fd) {
          * See details in bionic/libc/linker/debugger.c, in function
          * debugger_signal_handler().
          */
-        if (ptrace(PTRACE_ATTACH, request.tid, 0, 0)) {
+        if (!ptrace_attach_thread(request.pid, request.tid)) {
             LOG("ptrace attach failed: %s\n", strerror(errno));
         } else {
+            // DEBUGGER_ACTION_CRASH requests can come from arbitrary processes and the tid field in
+            // the request is sent from the other side. If an attacker can cause a process to be
+            // spawned with the pid of their process, they could trick debuggerd into dumping that
+            // process by exiting after sending the request. Validate the trusted request.uid/gid
+            // to defend against this.
+            if (request.action == DEBUGGER_ACTION_CRASH) {
+              pid_t pid;
+              uid_t uid;
+              gid_t gid;
+              if (get_process_info(request.tid, &pid, &uid, &gid) != 0) {
+                XLOG("debuggerd: failed to get process info for tid '%d'", request.tid);
+                exit(1);
+              }
+
+              if (pid != request.pid || uid != request.uid || gid != request.gid) {
+                XLOG(
+                  "debuggerd: attached task %d does not match request: "
+                  "expected pid=%d,uid=%d,gid=%d, actual pid=%d,uid=%d,gid=%d",
+                  request.tid, request.pid, request.uid, request.gid, pid, uid, gid);
+                exit(1);
+              }
+            }
+
             bool detach_failed = false;
             bool attach_gdb = should_attach_gdb(&request);
             if (TEMP_FAILURE_RETRY(write(fd, "\0", 1)) != 1) {

debuggerd/tombstone.c

@@ -459,7 +459,7 @@ static bool dump_sibling_thread_report(const ptrace_context_t* context,
         }
 
         /* Skip this thread if cannot ptrace it */
-        if (ptrace(PTRACE_ATTACH, new_tid, 0, 0) < 0) {
+        if (!ptrace_attach_thread(pid, new_tid)) {
             continue;
         }
 

debuggerd/utility.c

@@ -18,6 +18,7 @@
 #include <stddef.h>
 #include <stdbool.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
 #include <errno.h>
 #include <unistd.h>
@@ -128,3 +129,31 @@ void wait_for_stop(pid_t tid, int* total_sleep_time_usec) {
         *total_sleep_time_usec += sleep_time_usec;
     }
 }
+
+bool pid_contains_tid(pid_t pid, pid_t tid) {
+  char task_path[PATH_MAX];
+  if (snprintf(task_path, PATH_MAX, "/proc/%d/task/%d", pid, tid) >= PATH_MAX) {
+    XLOG("debuggerd: task path overflow (pid = %d, tid = %d)\n", pid, tid);
+    exit(1);
+  }
+
+  return access(task_path, F_OK) == 0;
+}
+
+// Attach to a thread, and verify that it's still a member of the given process
+bool ptrace_attach_thread(pid_t pid, pid_t tid) {
+  if (ptrace(PTRACE_ATTACH, tid, 0, 0) != 0) {
+    return false;
+  }
+
+  // Make sure that the task we attached to is actually part of the pid we're dumping.
+  if (!pid_contains_tid(pid, tid)) {
+    if (ptrace(PTRACE_DETACH, tid, 0, 0) != 0) {
+      XLOG("debuggerd: failed to detach from thread '%d'", tid);
+      exit(1);
+    }
+    return false;
+  }
+
+  return true;
+}

debuggerd/utility.h

@@ -63,4 +63,9 @@ void _LOG(log_t* log, int scopeFlags, const char *fmt, ...)
 int wait_for_signal(pid_t tid, int* total_sleep_time_usec);
 void wait_for_stop(pid_t tid, int* total_sleep_time_usec);
 
+bool pid_contains_tid(pid_t pid, pid_t tid);
+
+// Attach to a thread, and verify that it's still a member of the given process
+bool ptrace_attach_thread(pid_t pid, pid_t tid);
+
 #endif // _DEBUGGERD_UTILITY_H