diff options
author | Josh Gao <jmgao@google.com> | 2016-07-14 16:43:12 -0700 |
---|---|---|
committer | gitbuildkicker <android-build@google.com> | 2016-07-21 17:35:58 -0700 |
commit | c764dcd6977a04e272e44b0381c13ac4aed8a280 (patch) | |
tree | 08ebba7ece5aa22e9d8d6f6aea23325e67aedb45 /debuggerd/debuggerd.cpp | |
parent | f483354e592c9edd836acbc0fffcedfb6958c889 (diff) | |
download | system_core-c764dcd6977a04e272e44b0381c13ac4aed8a280.zip system_core-c764dcd6977a04e272e44b0381c13ac4aed8a280.tar.gz system_core-c764dcd6977a04e272e44b0381c13ac4aed8a280.tar.bz2 |
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: I6c6efcf82a49bca140d761b2d1de04215ba4d252
Diffstat (limited to 'debuggerd/debuggerd.cpp')
-rw-r--r-- | debuggerd/debuggerd.cpp | 33 |
1 files changed, 27 insertions, 6 deletions
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); |