From d2d95688f81fcce492b46b8e260bdf4f6e0b679b Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 14 Jul 2016 16:43:12 -0700 Subject: 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//task/ check and the PTRACE_ATTACH. 2. Sibling threads could exit between listing /proc//task and the PTRACE_ATTACH. Backport of NYC change I4dfe1ea30e2c211d2389321bd66e3684dd757591 Bug: http://b/29555636 Change-Id: I6c6efcf82a49bca140d761b2d1de04215ba4d252 --- debuggerd/backtrace.cpp | 10 +++++----- debuggerd/debuggerd.cpp | 33 +++++++++++++++++++++++++++------ debuggerd/utility.cpp | 29 +++++++++++++++++++++++++++++ debuggerd/utility.h | 5 +++++ 4 files changed, 66 insertions(+), 11 deletions(-) diff --git a/debuggerd/backtrace.cpp b/debuggerd/backtrace.cpp index b8084c5..ad6a6ee 100644 --- a/debuggerd/backtrace.cpp +++ b/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 : "", 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); } diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index 9c8a41e..2ab716d 100644 --- a/debuggerd/debuggerd.cpp +++ b/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); diff --git a/debuggerd/utility.cpp b/debuggerd/utility.cpp index 9f340a8..236d667 100644 --- a/debuggerd/utility.cpp +++ b/debuggerd/utility.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -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; +} diff --git a/debuggerd/utility.h b/debuggerd/utility.h index 263374d..e8ec7ef 100644 --- a/debuggerd/utility.h +++ b/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 -- cgit v1.1