diff --git a/debuggerd/backtrace.c b/debuggerd/backtrace.c index f42f24c4206f..d5086d4dd40e 100644 --- a/debuggerd/backtrace.c +++ b/debuggerd/backtrace.c @@ -62,7 +62,7 @@ static void dump_process_footer(log_t* log, pid_t pid) { _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 @@ static void dump_thread(log_t* log, pid_t tid, ptrace_context_t* context, bool a _LOG(log, SCOPE_AT_FAULT, "\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, SCOPE_AT_FAULT, "Could not attach to thread: %s\n", strerror(errno)); return; } @@ -122,7 +122,7 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed, 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 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed, 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 756f7bb102ce..54e77d0f3d73 100644 --- a/debuggerd/debuggerd.c +++ b/debuggerd/debuggerd.c @@ -233,13 +233,11 @@ 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)) { - 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 @@ static void handle_request(int fd) { * 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 2921350584af..faac6dede956 100644 --- a/debuggerd/tombstone.c +++ b/debuggerd/tombstone.c @@ -459,7 +459,7 @@ static bool dump_sibling_thread_report(const ptrace_context_t* context, } /* 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 41be9821efe0..a8f0cca93244 100644 --- a/debuggerd/utility.c +++ b/debuggerd/utility.c @@ -18,6 +18,7 @@ #include #include #include +#include #include #include #include @@ -128,3 +129,31 @@ void wait_for_stop(pid_t tid, int* total_sleep_time_usec) { *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 1f006ed5d231..ed5f0a19c6ca 100644 --- a/debuggerd/utility.h +++ b/debuggerd/utility.h @@ -63,4 +63,9 @@ void _LOG(log_t* log, int scopeFlags, const char *fmt, ...) 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