From 0528829b7378034e79820ac2d99396999bb4dd40 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 14 Jul 2016 16:05:10 -0700 Subject: [PATCH] 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: I320f47216b21018d3f613cfbbaaff40b3548ef36 --- debuggerd/backtrace.cpp | 10 +++++----- debuggerd/debuggerd.cpp | 35 ++++++++++++++++++++++++++++------- debuggerd/tombstone.cpp | 2 +- debuggerd/utility.cpp | 29 +++++++++++++++++++++++++++++ debuggerd/utility.h | 5 +++++ 5 files changed, 68 insertions(+), 13 deletions(-) diff --git a/debuggerd/backtrace.cpp b/debuggerd/backtrace.cpp index e49ef9b2c..9765b2877 100644 --- 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 : "", 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); } diff --git a/debuggerd/debuggerd.cpp b/debuggerd/debuggerd.cpp index 61805c932..95ded9951 100644 --- 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) { diff --git a/debuggerd/tombstone.cpp b/debuggerd/tombstone.cpp index 8bf3a50e8..3ff3ae3bc 100644 --- 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; } diff --git a/debuggerd/utility.cpp b/debuggerd/utility.cpp index 9a30fe3de..554d2f28a 100644 --- a/debuggerd/utility.cpp +++ b/debuggerd/utility.cpp @@ -20,6 +20,7 @@ #include #include +#include #include #include #include @@ -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; +} diff --git a/debuggerd/utility.h b/debuggerd/utility.h index 82413b876..0d0e5acf0 100644 --- 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