From f2059d3fd959ce6f0d8a106ad4483c733aab3f66 Mon Sep 17 00:00:00 2001 From: Sergio Giro Date: Tue, 28 Jun 2016 18:02:29 +0100 Subject: libutils/Unicode.cpp: Correct length computation and add checks for utf16->utf8 Inconsistent behaviour between utf16_to_utf8 and utf16_to_utf8_length is causing a heap overflow. Correcting the length computation and adding bound checks to the conversion functions. Test: ran libutils_tests Bug: 29250543 Change-Id: I6115e3357141ed245c63c6eb25fc0fd0a9a7a2bb (cherry picked from commit c4966a363e46d2e1074d1a365e232af0dcedd6a1) --- include/utils/Unicode.h | 4 ++-- libutils/String8.cpp | 25 +++++++++++++------------ libutils/Unicode.cpp | 15 +++++++++++---- libutils/tests/String8_test.cpp | 19 +++++++++++++++++++ 4 files changed, 45 insertions(+), 18 deletions(-) diff --git a/include/utils/Unicode.h b/include/utils/Unicode.h index b76a5e2..4e17cc3 100644 --- a/include/utils/Unicode.h +++ b/include/utils/Unicode.h @@ -87,7 +87,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len); * "dst" becomes \xE3\x81\x82\xE3\x81\x84 * (note that "dst" is NOT null-terminated, like strncpy) */ -void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst); +void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst, size_t dst_len); /** * Returns the unicode value at "index". @@ -109,7 +109,7 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len); * enough to fit the UTF-16 as measured by utf16_to_utf8_length with an added * NULL terminator. */ -void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst); +void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst, size_t dst_len); /** * Returns the length of "src" when "src" is valid UTF-8 string. diff --git a/libutils/String8.cpp b/libutils/String8.cpp index ad65fdb..75dfa29 100644 --- a/libutils/String8.cpp +++ b/libutils/String8.cpp @@ -102,20 +102,21 @@ static char* allocFromUTF16(const char16_t* in, size_t len) { if (len == 0) return getEmptyString(); - const ssize_t bytes = utf16_to_utf8_length(in, len); - if (bytes < 0) { + // Allow for closing '\0' + const ssize_t resultStrLen = utf16_to_utf8_length(in, len) + 1; + if (resultStrLen < 1) { return getEmptyString(); } - SharedBuffer* buf = SharedBuffer::alloc(bytes+1); + SharedBuffer* buf = SharedBuffer::alloc(resultStrLen); ALOG_ASSERT(buf, "Unable to allocate shared buffer"); if (!buf) { return getEmptyString(); } - char* str = (char*)buf->data(); - utf16_to_utf8(in, len, str); - return str; + char* resultStr = (char*)buf->data(); + utf16_to_utf8(in, len, resultStr, resultStrLen); + return resultStr; } static char* allocFromUTF32(const char32_t* in, size_t len) @@ -124,21 +125,21 @@ static char* allocFromUTF32(const char32_t* in, size_t len) return getEmptyString(); } - const ssize_t bytes = utf32_to_utf8_length(in, len); - if (bytes < 0) { + const ssize_t resultStrLen = utf32_to_utf8_length(in, len) + 1; + if (resultStrLen < 1) { return getEmptyString(); } - SharedBuffer* buf = SharedBuffer::alloc(bytes+1); + SharedBuffer* buf = SharedBuffer::alloc(resultStrLen); ALOG_ASSERT(buf, "Unable to allocate shared buffer"); if (!buf) { return getEmptyString(); } - char* str = (char*) buf->data(); - utf32_to_utf8(in, len, str); + char* resultStr = (char*) buf->data(); + utf32_to_utf8(in, len, resultStr, resultStrLen); - return str; + return resultStr; } // --------------------------------------------------------------------------- diff --git a/libutils/Unicode.cpp b/libutils/Unicode.cpp index fb876c9..2b5293e 100644 --- a/libutils/Unicode.cpp +++ b/libutils/Unicode.cpp @@ -14,6 +14,7 @@ * limitations under the License. */ +#include #include #include @@ -182,7 +183,7 @@ ssize_t utf32_to_utf8_length(const char32_t *src, size_t src_len) return ret; } -void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst) +void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst, size_t dst_len) { if (src == NULL || src_len == 0 || dst == NULL) { return; @@ -193,9 +194,12 @@ void utf32_to_utf8(const char32_t* src, size_t src_len, char* dst) char *cur = dst; while (cur_utf32 < end_utf32) { size_t len = utf32_codepoint_utf8_length(*cur_utf32); + LOG_ALWAYS_FATAL_IF(dst_len < len, "%zu < %zu", dst_len, len); utf32_codepoint_to_utf8((uint8_t *)cur, *cur_utf32++, len); cur += len; + dst_len -= len; } + LOG_ALWAYS_FATAL_IF(dst_len < 1, "dst_len < 1: %zu < 1", dst_len); *cur = '\0'; } @@ -324,7 +328,7 @@ int strzcmp16_h_n(const char16_t *s1H, size_t n1, const char16_t *s2N, size_t n2 : 0); } -void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst) +void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst, size_t dst_len) { if (src == NULL || src_len == 0 || dst == NULL) { return; @@ -345,9 +349,12 @@ void utf16_to_utf8(const char16_t* src, size_t src_len, char* dst) utf32 = (char32_t) *cur_utf16++; } const size_t len = utf32_codepoint_utf8_length(utf32); + LOG_ALWAYS_FATAL_IF(dst_len < len, "%zu < %zu", dst_len, len); utf32_codepoint_to_utf8((uint8_t*)cur, utf32, len); cur += len; + dst_len -= len; } + LOG_ALWAYS_FATAL_IF(dst_len < 1, "%zu < 1", dst_len); *cur = '\0'; } @@ -408,10 +415,10 @@ ssize_t utf16_to_utf8_length(const char16_t *src, size_t src_len) const char16_t* const end = src + src_len; while (src < end) { if ((*src & 0xFC00) == 0xD800 && (src + 1) < end - && (*++src & 0xFC00) == 0xDC00) { + && (*(src + 1) & 0xFC00) == 0xDC00) { // surrogate pairs are always 4 bytes. ret += 4; - src++; + src += 2; } else { ret += utf32_codepoint_utf8_length((char32_t) *src++); } diff --git a/libutils/tests/String8_test.cpp b/libutils/tests/String8_test.cpp index c42c68d..7cd67d3 100644 --- a/libutils/tests/String8_test.cpp +++ b/libutils/tests/String8_test.cpp @@ -17,6 +17,7 @@ #define LOG_TAG "String8_test" #include #include +#include #include @@ -72,4 +73,22 @@ TEST_F(String8Test, OperatorPlusEquals) { EXPECT_STREQ(src3, " Verify me."); } +// http://b/29250543 +TEST_F(String8Test, CorrectInvalidSurrogate) { + // d841d8 is an invalid start for a surrogate pair. Make sure this is handled by ignoring the + // first character in the pair and handling the rest correctly. + String16 string16(u"\xd841\xd841\xdc41\x0000"); + String8 string8(string16); + + EXPECT_EQ(4U, string8.length()); +} + +TEST_F(String8Test, CheckUtf32Conversion) { + // Since bound checks were added, check the conversion can be done without fatal errors. + // The utf8 lengths of these are chars are 1 + 2 + 3 + 4 = 10. + const char32_t string32[] = U"\x0000007f\x000007ff\x0000911\x0010fffe"; + String8 string8(string32); + EXPECT_EQ(10U, string8.length()); +} + } -- cgit v1.1 From 11440395e379f35069e8e39a6e3ccfa7a7bfc72f Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 17 May 2016 19:23:39 -0700 Subject: adb: switch the socket list mutex to a recursive_mutex. sockets.cpp was branching on whether a socket close function was local_socket_close in order to avoid a potential deadlock if the socket list lock was held while closing a peer socket. Bug: http://b/28347842 Change-Id: I5e56f17fa54275284787f0f1dc150d1960256ab3 (functionally a cherrypick of 903b749f + 9b587dec, with windows disabled) --- adb/Android.mk | 3 ++ adb/mutex_list.h | 1 - adb/sockets.cpp | 78 +++++++++++++--------------- adb/sysdeps/mutex.h | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 180 insertions(+), 45 deletions(-) create mode 100644 adb/sysdeps/mutex.h diff --git a/adb/Android.mk b/adb/Android.mk index 425bf9b..d4adbfb 100644 --- a/adb/Android.mk +++ b/adb/Android.mk @@ -200,7 +200,10 @@ endif # will violate ODR LOCAL_SHARED_LIBRARIES := +# Don't build the host adb on Windows (this branch should only be used for security updates.) +ifneq ($(HOST_OS),windows) include $(BUILD_HOST_EXECUTABLE) +endif $(call dist-for-goals,dist_files sdk,$(LOCAL_BUILT_MODULE)) diff --git a/adb/mutex_list.h b/adb/mutex_list.h index ff72751..15e383c 100644 --- a/adb/mutex_list.h +++ b/adb/mutex_list.h @@ -6,7 +6,6 @@ #ifndef ADB_MUTEX #error ADB_MUTEX not defined when including this file #endif -ADB_MUTEX(socket_list_lock) ADB_MUTEX(transport_lock) #if ADB_HOST ADB_MUTEX(local_transports_lock) diff --git a/adb/sockets.cpp b/adb/sockets.cpp index 32ca17d..31b5443 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -25,18 +25,27 @@ #include #include +#include +#include +#include +#include + #if !ADB_HOST #include "cutils/properties.h" #endif #include "adb.h" #include "adb_io.h" +#include "sysdeps/mutex.h" #include "transport.h" -ADB_MUTEX_DEFINE( socket_list_lock ); +#if !defined(__BIONIC__) +using std::recursive_mutex; +#endif -static void local_socket_close_locked(asocket *s); +static void local_socket_close(asocket* s); +static recursive_mutex& local_socket_list_lock = *new recursive_mutex(); static unsigned local_socket_next_id = 1; static asocket local_socket_list = { @@ -61,7 +70,7 @@ asocket *find_local_socket(unsigned local_id, unsigned peer_id) asocket *s; asocket *result = NULL; - adb_mutex_lock(&socket_list_lock); + std::lock_guard lock(local_socket_list_lock); for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { if (s->id != local_id) continue; @@ -70,7 +79,6 @@ asocket *find_local_socket(unsigned local_id, unsigned peer_id) } break; } - adb_mutex_unlock(&socket_list_lock); return result; } @@ -84,20 +92,17 @@ insert_local_socket(asocket* s, asocket* list) s->next->prev = s; } - -void install_local_socket(asocket *s) -{ - adb_mutex_lock(&socket_list_lock); +void install_local_socket(asocket* s) { + std::lock_guard lock(local_socket_list_lock); s->id = local_socket_next_id++; // Socket ids should never be 0. - if (local_socket_next_id == 0) - local_socket_next_id = 1; + if (local_socket_next_id == 0) { + fatal("local socket id overflow"); + } insert_local_socket(s, &local_socket_list); - - adb_mutex_unlock(&socket_list_lock); } void remove_socket(asocket *s) @@ -116,19 +121,17 @@ void remove_socket(asocket *s) void close_all_sockets(atransport *t) { asocket *s; - - /* this is a little gross, but since s->close() *will* modify - ** the list out from under you, your options are limited. - */ - adb_mutex_lock(&socket_list_lock); + /* this is a little gross, but since s->close() *will* modify + ** the list out from under you, your options are limited. + */ + std::lock_guard lock(local_socket_list_lock); restart: - for(s = local_socket_list.next; s != &local_socket_list; s = s->next){ - if(s->transport == t || (s->peer && s->peer->transport == t)) { - local_socket_close_locked(s); + for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { + if (s->transport == t || (s->peer && s->peer->transport == t)) { + local_socket_close(s); goto restart; } } - adb_mutex_unlock(&socket_list_lock); } static int local_socket_enqueue(asocket *s, apacket *p) @@ -191,13 +194,6 @@ static void local_socket_ready(asocket *s) fdevent_add(&s->fde, FDE_READ); } -static void local_socket_close(asocket *s) -{ - adb_mutex_lock(&socket_list_lock); - local_socket_close_locked(s); - adb_mutex_unlock(&socket_list_lock); -} - // be sure to hold the socket list lock when calling this static void local_socket_destroy(asocket *s) { @@ -226,27 +222,21 @@ static void local_socket_destroy(asocket *s) } } - -static void local_socket_close_locked(asocket *s) -{ - D("entered local_socket_close_locked. LS(%d) fd=%d\n", s->id, s->fd); - if(s->peer) { - D("LS(%d): closing peer. peer->id=%d peer->fd=%d\n", - s->id, s->peer->id, s->peer->fd); +static void local_socket_close(asocket* s) { + D("entered local_socket_close. LS(%d) fd=%d", s->id, s->fd); + std::lock_guard lock(local_socket_list_lock); + if (s->peer) { + D("LS(%d): closing peer. peer->id=%d peer->fd=%d", s->id, s->peer->id, s->peer->fd); /* Note: it's important to call shutdown before disconnecting from * the peer, this ensures that remote sockets can still get the id * of the local socket they're connected to, to send a CLOSE() * protocol event. */ - if (s->peer->shutdown) - s->peer->shutdown(s->peer); - s->peer->peer = 0; - // tweak to avoid deadlock - if (s->peer->close == local_socket_close) { - local_socket_close_locked(s->peer); - } else { - s->peer->close(s->peer); + if (s->peer->shutdown) { + s->peer->shutdown(s->peer); } - s->peer = 0; + s->peer->peer = nullptr; + s->peer->close(s->peer); + s->peer = nullptr; } /* If we are already closing, or if there are no diff --git a/adb/sysdeps/mutex.h b/adb/sysdeps/mutex.h new file mode 100644 index 0000000..ef5d9b1 --- /dev/null +++ b/adb/sysdeps/mutex.h @@ -0,0 +1,143 @@ +#pragma once + +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#if defined(_WIN32) + +#include + +#include + +#include "adb.h" + +// The prebuilt version of mingw we use doesn't support mutex or recursive_mutex. +// Therefore, implement our own using the Windows primitives. +// Put them directly into the std namespace, so that when they're actually available, the build +// breaks until they're removed. + +#include +namespace std { + +// CRITICAL_SECTION is recursive, so just wrap it in a Mutex-compatible class. +class recursive_mutex { + public: + recursive_mutex() { + InitializeCriticalSection(&mutex_); + } + + ~recursive_mutex() { + DeleteCriticalSection(&mutex_); + } + + void lock() { + EnterCriticalSection(&mutex_); + } + + bool try_lock() { + return TryEnterCriticalSection(&mutex_); + } + + void unlock() { + LeaveCriticalSection(&mutex_); + } + + private: + CRITICAL_SECTION mutex_; + + DISALLOW_COPY_AND_ASSIGN(recursive_mutex); +}; + +class mutex { + public: + mutex() { + } + + ~mutex() { + } + + void lock() { + mutex_.lock(); + if (++lock_count_ != 1) { + fatal("non-recursive mutex locked reentrantly"); + } + } + + void unlock() { + if (--lock_count_ != 0) { + fatal("non-recursive mutex unlock resulted in unexpected lock count: %d", lock_count_); + } + mutex_.unlock(); + } + + bool try_lock() { + if (!mutex_.try_lock()) { + return false; + } + + if (lock_count_ != 0) { + mutex_.unlock(); + return false; + } + + ++lock_count_; + return true; + } + + private: + recursive_mutex mutex_; + size_t lock_count_ = 0; +}; + +} + +#elif defined(__BIONIC__) + +// On M, the recovery image uses parts of adb that depends on recursive_mutex, and uses libstdc++, +// which lacks it. + +#include +#include + +#include + +class recursive_mutex { + public: + recursive_mutex() { + } + + ~recursive_mutex() { + } + + void lock() { + pthread_mutex_lock(&mutex_); + } + + bool try_lock() { + return pthread_mutex_trylock(&mutex_); + } + + void unlock() { + pthread_mutex_unlock(&mutex_); + } + + private: + pthread_mutex_t mutex_ = PTHREAD_RECURSIVE_MUTEX_INITIALIZER_NP; + + DISALLOW_COPY_AND_ASSIGN(recursive_mutex); +}; + +#endif -- cgit v1.1 From 3b268646f802d72f9b53800ac00540201223cf2e Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Wed, 18 May 2016 10:39:48 -0700 Subject: adb: use asocket's close function when closing. close_all_sockets was assuming that all registered local sockets used local_socket_close as their close function. However, this is not true for JDWP sockets. Bug: http://b/28347842 Change-Id: I40a1174845cd33f15f30ce70828a7081cd5a087e (cherry picked from commit 53eb31d87cb84a4212f4850bf745646e1fb12814) (cherry picked from commit 014b01706cc64dc9c2ad94a96f62e07c058d0b5d) --- adb/sockets.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/adb/sockets.cpp b/adb/sockets.cpp index 31b5443..3919147 100644 --- a/adb/sockets.cpp +++ b/adb/sockets.cpp @@ -43,8 +43,6 @@ using std::recursive_mutex; #endif -static void local_socket_close(asocket* s); - static recursive_mutex& local_socket_list_lock = *new recursive_mutex(); static unsigned local_socket_next_id = 1; @@ -128,7 +126,7 @@ void close_all_sockets(atransport *t) restart: for (s = local_socket_list.next; s != &local_socket_list; s = s->next) { if (s->transport == t || (s->peer && s->peer->transport == t)) { - local_socket_close(s); + s->close(s); goto restart; } } -- cgit v1.1 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 From e86d0e14970166fa38f16187260679431cdd4119 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Tue, 9 Aug 2016 15:29:58 -0700 Subject: debuggerd: fix missed use of ptrace(PTRACE_ATTACH). Bug: http://b/29555636 Change-Id: Ibd8a2e2b619b74aac667555b7085d6f28e367c07 --- debuggerd/tombstone.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/debuggerd/tombstone.cpp b/debuggerd/tombstone.cpp index aeffc66..d04b721 100644 --- a/debuggerd/tombstone.cpp +++ b/debuggerd/tombstone.cpp @@ -447,7 +447,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; } -- cgit v1.1 From e30579f3359eba93019171861f32bbf4c72313d1 Mon Sep 17 00:00:00 2001 From: Connor O'Brien Date: Fri, 12 Aug 2016 11:52:46 -0700 Subject: Fix vold vulnerability in FrameworkListener Modify FrameworkListener to ignore commands that exceed the maximum buffer length and send an error message. Bug: 29831647 Change-Id: I9e57d1648d55af2ca0191bb47868e375ecc26950 Signed-off-by: Connor O'Brien (cherry picked from commit baa126dc158a40bc83c17c6d428c760e5b93fb1a) (cherry picked from commit 470484d2a25ad432190a01d1c763b4b36db33c7e) --- include/sysutils/FrameworkListener.h | 1 + libsysutils/src/FrameworkListener.cpp | 17 ++++++++++++++--- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/include/sysutils/FrameworkListener.h b/include/sysutils/FrameworkListener.h index 18049cd..2137069 100644 --- a/include/sysutils/FrameworkListener.h +++ b/include/sysutils/FrameworkListener.h @@ -32,6 +32,7 @@ private: int mCommandCount; bool mWithSeq; FrameworkCommandCollection *mCommands; + bool mSkipToNextNullByte; public: FrameworkListener(const char *socketName); diff --git a/libsysutils/src/FrameworkListener.cpp b/libsysutils/src/FrameworkListener.cpp index e7b3dd6..579ead9 100644 --- a/libsysutils/src/FrameworkListener.cpp +++ b/libsysutils/src/FrameworkListener.cpp @@ -49,6 +49,7 @@ void FrameworkListener::init(const char *socketName UNUSED, bool withSeq) { errorRate = 0; mCommandCount = 0; mWithSeq = withSeq; + mSkipToNextNullByte = false; } bool FrameworkListener::onDataAvailable(SocketClient *c) { @@ -59,10 +60,15 @@ bool FrameworkListener::onDataAvailable(SocketClient *c) { if (len < 0) { SLOGE("read() failed (%s)", strerror(errno)); return false; - } else if (!len) + } else if (!len) { return false; - if(buffer[len-1] != '\0') + } else if (buffer[len-1] != '\0') { SLOGW("String is not zero-terminated"); + android_errorWriteLog(0x534e4554, "29831647"); + c->sendMsg(500, "Command too large for buffer", false); + mSkipToNextNullByte = true; + return false; + } int offset = 0; int i; @@ -70,11 +76,16 @@ bool FrameworkListener::onDataAvailable(SocketClient *c) { for (i = 0; i < len; i++) { if (buffer[i] == '\0') { /* IMPORTANT: dispatchCommand() expects a zero-terminated string */ - dispatchCommand(c, buffer + offset); + if (mSkipToNextNullByte) { + mSkipToNextNullByte = false; + } else { + dispatchCommand(c, buffer + offset); + } offset = i + 1; } } + mSkipToNextNullByte = false; return true; } -- cgit v1.1