Merge \\"DO NOT MERGE: debuggerd: verify that traced threads belong to the right process.\\" into mnc-dev am: 73922b82ee

am: 8a9e4369f6

Change-Id: I7aa9984853f5114aea36d51ea2903b729b60d5e8

debuggerd/backtrace.cpp

@@ -67,8 +67,8 @@ static void dump_process_footer(log_t* log, pid_t pid) {
   _LOG(log, logtype::BACKTRACE, "\n----- end %d -----\n", pid);
 }
 
-static void dump_thread(
-    log_t* log, pid_t tid, bool attached, bool* detach_failed, int* total_sleep_time_usec) {
+static void dump_thread(log_t* log, pid_t pid, pid_t tid, bool attached,
+                        bool* detach_failed, int* total_sleep_time_usec) {
   char path[PATH_MAX];
   char threadnamebuf[1024];
   char* threadname = NULL;
@@ -88,7 +88,7 @@ static void dump_thread(
 
   _LOG(log, logtype::BACKTRACE, "\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, logtype::BACKTRACE, "Could not attach to thread: %s\n", strerror(errno));
     return;
   }
@@ -117,7 +117,7 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed,
   log.amfd = amfd;
 
   dump_process_header(&log, pid);
-  dump_thread(&log, tid, true, detach_failed, total_sleep_time_usec);
+  dump_thread(&log, pid, tid, true, detach_failed, total_sleep_time_usec);
 
   char task_path[64];
   snprintf(task_path, sizeof(task_path), "/proc/%d/task", pid);
@@ -135,7 +135,7 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed,
         continue;
       }
 
-      dump_thread(&log, new_tid, false, detach_failed, total_sleep_time_usec);
+      dump_thread(&log, pid, new_tid, false, detach_failed, total_sleep_time_usec);
     }
     closedir(d);
   }

debuggerd/debuggerd.cpp

@@ -225,12 +225,10 @@ 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)) {
+    // 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)) {
       ALOGE("tid %d does not exist in pid %d. ignoring debug request\n",
-          out_request->tid, out_request->pid);
+            out_request->tid, out_request->pid);
       return -1;
     }
   } else if (cr.uid == 0
@@ -380,9 +378,32 @@ static void handle_request(int fd) {
     // ensure that it can run as soon as we call PTRACE_CONT below.
     // 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)) {
       ALOGE("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) {
+          ALOGE("debuggerd: failed to get process info for tid '%d'", request.tid);
+          exit(1);
+        }
+
+        if (pid != request.pid || uid != request.uid || gid != request.gid) {
+          ALOGE(
+            "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 tid_unresponsive = false;
       bool attach_gdb = should_attach_gdb(&request);

debuggerd/utility.cpp

@@ -20,6 +20,7 @@
 
 #include <errno.h>
 #include <signal.h>
+#include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
 #include <sys/ptrace.h>
@@ -207,3 +208,31 @@ void dump_memory(log_t* log, Backtrace* backtrace, uintptr_t addr, const char* f
     _LOG(log, logtype::MEMORY, "%s  %s\n", logline.c_str(), ascii.c_str());
   }
 }
+
+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) {
+    ALOGE("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) {
+      ALOGE("debuggerd: failed to detach from thread '%d'", tid);
+      exit(1);
+    }
+    return false;
+  }
+
+  return true;
+}

debuggerd/utility.h

@@ -79,4 +79,9 @@ int wait_for_sigstop(pid_t, int*, bool*);
 
 void dump_memory(log_t* log, Backtrace* backtrace, uintptr_t addr, const char* fmt, ...);
 
+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