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: I93f6423e6de38e2bc6c75d8d33052da4cd2daa8a
diff --git a/debuggerd/backtrace.c b/debuggerd/backtrace.c
index f42f24c..d5086d4 100644
--- a/debuggerd/backtrace.c
+++ b/debuggerd/backtrace.c
@@ -62,7 +62,7 @@
_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 @@
_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 @@
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 @@
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);
}
diff --git a/debuggerd/debuggerd.c b/debuggerd/debuggerd.c
index 756f7bb..54e77d0 100644
--- a/debuggerd/debuggerd.c
+++ b/debuggerd/debuggerd.c
@@ -233,13 +233,11 @@
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 @@
* 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) {
diff --git a/debuggerd/tombstone.c b/debuggerd/tombstone.c
index 2921350..faac6de 100644
--- a/debuggerd/tombstone.c
+++ b/debuggerd/tombstone.c
@@ -459,7 +459,7 @@
}
/* Skip this thread if cannot ptrace it */
- if (ptrace(PTRACE_ATTACH, new_tid, 0, 0) < 0) {
+ if (!ptrace_attach_thread(pid, new_tid)) {
continue;
}
diff --git a/debuggerd/utility.c b/debuggerd/utility.c
index 41be982..a8f0cca 100644
--- a/debuggerd/utility.c
+++ b/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 @@
*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;
+}
diff --git a/debuggerd/utility.h b/debuggerd/utility.h
index 1f006ed..ed5f0a1 100644
--- a/debuggerd/utility.h
+++ b/debuggerd/utility.h
@@ -63,4 +63,9 @@
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