Commit: 0528829b7378034e79820ac2d99396999bb4dd40 - system-coremmmm (git)

system/corennnnn

Commit MetaInfo

• Révision: 0528829b7378034e79820ac2d99396999bb4dd40 (tree)
• l'heure: 2016-07-15 08:47:10
• Auteur: Josh Gao <jmgao@goog...>
• Commiter: Josh Gao

Message de Log

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

Fix two races in debuggerd's PTRACE_ATTACH logic:

1. The target thread in a crash dump request could exit between the
   /proc/<pid>/task/<tid> check and the PTRACE_ATTACH.
   
2. Sibling threads could exit between listing /proc/<pid>/task and the
   PTRACE_ATTACH.
   

Backport of NYC change I4dfe1ea30e2c211d2389321bd66e3684dd757591
Bug: http://b/29555636
Change-Id: I320f47216b21018d3f613cfbbaaff40b3548ef36

Change Summary

• modified: debuggerd/backtrace.cpp
• modified: debuggerd/debuggerd.cpp
• modified: debuggerd/tombstone.cpp
• modified: debuggerd/utility.cpp
• modified: debuggerd/utility.h

Modification

--- a/debuggerd/backtrace.cpp
+++ b/debuggerd/backtrace.cpp
@@ -62,8 +62,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;
@@ -83,7 +83,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;
   }
@@ -108,7 +108,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);
@@ -126,7 +126,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);
   }

--- a/debuggerd/debuggerd.cpp
+++ b/debuggerd/debuggerd.cpp
@@ -168,12 +168,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)) {
-      ALOGE("tid %d does not exist in pid %d. ignoring debug request\n",
-          out_request->tid, out_request->pid);
+    // 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);
       return -1;
     }
   } else if (cr.uid == 0
@@ -223,9 +221,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 attach_gdb = should_attach_gdb(&request);
       if (TEMP_FAILURE_RETRY(write(fd, "\0", 1)) != 1) {

--- a/debuggerd/tombstone.cpp
+++ b/debuggerd/tombstone.cpp
@@ -416,7 +416,7 @@ static bool dump_sibling_thread_report(
     }
 
     // Skip this thread if cannot ptrace it
-    if (ptrace(PTRACE_ATTACH, new_tid, 0, 0) < 0) {
+    if (!ptrace_attach_thread(pid, new_tid)) {
       _LOG(log, logtype::ERROR, "ptrace attach to %d failed: %s\n", new_tid, strerror(errno));
       continue;
     }

--- a/debuggerd/utility.cpp
+++ b/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>
@@ -208,3 +209,31 @@ void dump_memory(log_t* log, pid_t tid, uintptr_t addr) {
         _LOG(log, logtype::MEMORY, "    %s %s\n", code_buffer, ascii_buffer);
     }
 }
+
+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;
+}

--- a/debuggerd/utility.h
+++ b/debuggerd/utility.h
@@ -76,4 +76,9 @@ void wait_for_stop(pid_t tid, int* total_sleep_time_usec);
 
 void dump_memory(log_t* log, pid_t tid, uintptr_t addr);
 
+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