Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit a50e8ffa authored by Josh Gao's avatar Josh Gao
Browse files

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
parent 72bad95d

debuggerd/backtrace.cpp

+5 −5
Original line number Original line Diff line number Diff line
@@ -67,8 +67,8 @@ static void dump_process_footer(log_t* log, pid_t pid) { 
  _LOG(log, logtype::BACKTRACE, "\n----- end %d -----\n", pid);

  _LOG(log, logtype::BACKTRACE, "\n----- end %d -----\n", pid);

}

}





static void dump_thread(

static void dump_thread(log_t* log, pid_t pid, pid_t tid, bool attached,

    log_t* log, pid_t tid, bool attached, bool* detach_failed, int* total_sleep_time_usec) {

                        bool* detach_failed, int* total_sleep_time_usec) {

  char path[PATH_MAX];

  char path[PATH_MAX];

  char threadnamebuf[1024];

  char threadnamebuf[1024];

  char* threadname = NULL;

  char* threadname = NULL;
@@ -88,7 +88,7 @@ static void dump_thread( 




  _LOG(log, logtype::BACKTRACE, "\n\"%s\" sysTid=%d\n", threadname ? threadname : "<unknown>", tid);

  _LOG(log, logtype::BACKTRACE, "\n\"%s\" sysTid=%d\n", threadname ? threadname : "<unknown>", 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));

    _LOG(log, logtype::BACKTRACE, "Could not attach to thread: %s\n", strerror(errno));

    return;

    return;

  }

  }
@@ -117,7 +117,7 @@ void dump_backtrace(int fd, int amfd, pid_t pid, pid_t tid, bool* detach_failed, 
  log.amfd = amfd;

  log.amfd = amfd;





  dump_process_header(&log, pid);

  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];

  char task_path[64];

  snprintf(task_path, sizeof(task_path), "/proc/%d/task", pid);

  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;

        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);

    closedir(d);

  }

  }

debuggerd/debuggerd.cpp

+27 −6
Original line number Original line Diff line number Diff line
@@ -225,10 +225,8 @@ static int read_request(int fd, debugger_request_t* out_request) { 




  if (msg.action == DEBUGGER_ACTION_CRASH) {

  if (msg.action == DEBUGGER_ACTION_CRASH) {

    // Ensure that the tid reported by the crashing process is valid.

    // Ensure that the tid reported by the crashing process is valid.

    char buf[64];

    // This check needs to happen again after ptracing the requested thread to prevent a race.

    struct stat s;

    if (!pid_contains_tid(out_request->pid, out_request->tid)) {

    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",

      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;

      return -1;
@@ -380,9 +378,32 @@ static void handle_request(int fd) { 
    // ensure that it can run as soon as we call PTRACE_CONT below.

    // ensure that it can run as soon as we call PTRACE_CONT below.

    // See details in bionic/libc/linker/debugger.c, in function

    // See details in bionic/libc/linker/debugger.c, in function

    // debugger_signal_handler().

    // 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));

      ALOGE("ptrace attach failed: %s\n", strerror(errno));

    } else {

    } 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 detach_failed = false;

      bool tid_unresponsive = false;

      bool tid_unresponsive = false;

      bool attach_gdb = should_attach_gdb(&request);

      bool attach_gdb = should_attach_gdb(&request);

debuggerd/utility.cpp

+29 −0
Original line number Original line Diff line number Diff line
@@ -20,6 +20,7 @@ 




#include <errno.h>

#include <errno.h>

#include <signal.h>

#include <signal.h>

#include <stdlib.h>

#include <string.h>

#include <string.h>

#include <unistd.h>

#include <unistd.h>

#include <sys/ptrace.h>

#include <sys/ptrace.h>
@@ -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());

    _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;

}

debuggerd/utility.h

+5 −0
Original line number Original line Diff line number Diff line
@@ -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, ...);

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
 
#endif // _DEBUGGERD_UTILITY_H