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
diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp
index 06c16f8..7be64f1 100644
--- a/debuggerd/debuggerd.cpp
+++ b/debuggerd/debuggerd.cpp
@@ -168,12 +168,10 @@
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 @@
// 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);