From 843a0f97717a006fd21cd89fd229b064506e5d05 Mon Sep 17 00:00:00 2001 From: Jason Molenda Date: Tue, 4 Feb 2020 19:43:33 -0800 Subject: [PATCH] Enhance debugserver's err reporting on attach fails Explicitly check for a request to attach to a pid that doesn't exist, to attach to a pid that is already being debugged, unify the SIP process check, and an attempt at checking if developer mode is enabled on the system (which isn't working in debugserver, for some reason; I can't get the authorization record which should be an unprivileged operation and works in a standalone program I wrote). I'll debug the developer mode check later, but I wanted to land it along with everything else; right now it will claim that developer mode is always enabled so it's harmless to include as-is. --- lldb/tools/debugserver/source/DNB.cpp | 5 +- lldb/tools/debugserver/source/DNB.h | 1 + lldb/tools/debugserver/source/RNBRemote.cpp | 199 +++++++++++++++++++++----- lldb/tools/debugserver/source/RNBServices.cpp | 6 +- 4 files changed, 169 insertions(+), 42 deletions(-) diff --git a/lldb/tools/debugserver/source/DNB.cpp b/lldb/tools/debugserver/source/DNB.cpp index 8d9c691..d0dd8fd 100644 --- a/lldb/tools/debugserver/source/DNB.cpp +++ b/lldb/tools/debugserver/source/DNB.cpp @@ -57,7 +57,6 @@ typedef std::map ProcessMap; typedef ProcessMap::iterator ProcessMapIter; typedef ProcessMap::const_iterator ProcessMapConstIter; -size_t GetAllInfos(std::vector &proc_infos); static size_t GetAllInfosMatchingName(const char *process_name, std::vector &matching_proc_infos); @@ -520,7 +519,7 @@ nub_process_t DNBProcessAttach(nub_process_t attach_pid, return INVALID_NUB_PROCESS; } -size_t GetAllInfos(std::vector &proc_infos) { +size_t DNBGetAllInfos(std::vector &proc_infos) { size_t size = 0; int name[] = {CTL_KERN, KERN_PROC, KERN_PROC_ALL}; u_int namelen = sizeof(name) / sizeof(int); @@ -573,7 +572,7 @@ GetAllInfosMatchingName(const char *full_process_name, const size_t process_name_len = strlen(process_name); std::vector proc_infos; - const size_t num_proc_infos = GetAllInfos(proc_infos); + const size_t num_proc_infos = DNBGetAllInfos(proc_infos); if (num_proc_infos > 0) { uint32_t i; for (i = 0; i < num_proc_infos; i++) { diff --git a/lldb/tools/debugserver/source/DNB.h b/lldb/tools/debugserver/source/DNB.h index e29fa0f..ec04a1a 100644 --- a/lldb/tools/debugserver/source/DNB.h +++ b/lldb/tools/debugserver/source/DNB.h @@ -150,6 +150,7 @@ nub_size_t DNBProcessGetAvailableProfileData(nub_process_t pid, char *buf, nub_size_t buf_size) DNB_EXPORT; nub_size_t DNBProcessGetStopCount(nub_process_t pid) DNB_EXPORT; uint32_t DNBProcessGetCPUType(nub_process_t pid) DNB_EXPORT; +size_t DNBGetAllInfos(std::vector &proc_infos); // Process executable and arguments const char *DNBProcessGetExecutablePath(nub_process_t pid); diff --git a/lldb/tools/debugserver/source/RNBRemote.cpp b/lldb/tools/debugserver/source/RNBRemote.cpp index 6f2c735..9d6ca99 100644 --- a/lldb/tools/debugserver/source/RNBRemote.cpp +++ b/lldb/tools/debugserver/source/RNBRemote.cpp @@ -13,8 +13,10 @@ #include "RNBRemote.h" #include +#include #include #include +#include #include #include #include @@ -49,6 +51,9 @@ #include #include +#include +#include + // constants static const std::string OS_LOG_EVENTS_KEY_NAME("events"); @@ -3644,6 +3649,132 @@ rnb_err_t RNBRemote::HandlePacket_qSupported(const char *p) { return SendPacket(buf); } +static bool process_does_not_exist (nub_process_t pid) { + std::vector proc_infos; + DNBGetAllInfos (proc_infos); + const size_t infos_size = proc_infos.size(); + for (size_t i = 0; i < infos_size; i++) + if (proc_infos[i].kp_proc.p_pid == pid) + return false; + + return true; // process does not exist +} + +static bool attach_failed_due_to_sip (nub_process_t pid) { + bool retval = false; +#if defined(__APPLE__) && \ + (__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 101000) + + // csr_check(CSR_ALLOW_TASK_FOR_PID) will be nonzero if System Integrity + // Protection is in effect. + if (csr_check(CSR_ALLOW_TASK_FOR_PID) == 0) + return false; + + if (rootless_allows_task_for_pid(pid) == 0) + retval = true; + + int csops_flags = 0; + int csops_ret = ::csops(pid, CS_OPS_STATUS, &csops_flags, + sizeof(csops_flags)); + if (csops_ret != -1 && (csops_flags & CS_RESTRICT)) { + retval = true; + } +#endif + + return retval; +} + +static bool process_is_already_being_debugged (nub_process_t pid) { + struct kinfo_proc kinfo; + int mib[] = {CTL_KERN, KERN_PROC, KERN_PROC_PID, pid}; + size_t len = sizeof(struct kinfo_proc); + if (sysctl(mib, sizeof(mib) / sizeof(mib[0]), &kinfo, &len, NULL, 0) != 0) { + return false; // pid doesn't exist? well, it's not being debugged... + } + if (kinfo.kp_proc.p_flag & P_TRACED) + return true; // is being debugged already + else + return false; +} + +// Checking for +// +// { +// 'class' : 'rule', +// 'comment' : 'For use by Apple. WARNING: administrators are advised +// not to modify this right.', +// 'k-of-n' : '1', +// 'rule' : [ +// 'is-admin', +// 'is-developer', +// 'authenticate-developer' +// ] +// } +// +// $ security authorizationdb read system.privilege.taskport.debug + +static bool developer_mode_enabled () { + CFDictionaryRef currentRightDict = NULL; + const char *debug_right = "system.privilege.taskport.debug"; + // caller must free dictionary initialized by the following + OSStatus status = AuthorizationRightGet(debug_right, ¤tRightDict); + if (status != errAuthorizationSuccess) { + // could not check authorization + return true; + } + + bool devmode_enabled = true; + + if (!CFDictionaryContainsKey(currentRightDict, CFSTR("k-of-n"))) { + devmode_enabled = false; + } else { + CFNumberRef item = (CFNumberRef) CFDictionaryGetValue(currentRightDict, CFSTR("k-of-n")); + if (item && CFGetTypeID(item) == CFNumberGetTypeID()) { + int64_t num = 0; + ::CFNumberGetValue(item, kCFNumberSInt64Type, &num); + if (num != 1) { + devmode_enabled = false; + } + } else { + devmode_enabled = false; + } + } + + if (!CFDictionaryContainsKey(currentRightDict, CFSTR("class"))) { + devmode_enabled = false; + } else { + CFStringRef item = (CFStringRef) CFDictionaryGetValue(currentRightDict, CFSTR("class")); + if (item && CFGetTypeID(item) == CFStringGetTypeID()) { + if (strcmp (CFStringGetCStringPtr (item, ::CFStringGetSystemEncoding()), "rule") != 0) { + devmode_enabled = false; + } + } else { + devmode_enabled = false; + } + } + + if (!CFDictionaryContainsKey(currentRightDict, CFSTR("rule"))) { + devmode_enabled = false; + } else { + CFArrayRef item = (CFArrayRef) CFDictionaryGetValue(currentRightDict, CFSTR("rule")); + if (item && CFGetTypeID(item) == CFArrayGetTypeID()) { + int count = ::CFArrayGetCount(item); + CFRange range = CFRangeMake (0, count); + if (!::CFArrayContainsValue (item, range, CFSTR("is-admin"))) + devmode_enabled = false; + if (!::CFArrayContainsValue (item, range, CFSTR("is-developer"))) + devmode_enabled = false; + if (!::CFArrayContainsValue (item, range, CFSTR("authenticate-developer"))) + devmode_enabled = false; + } else { + devmode_enabled = false; + } + } + ::CFRelease(currentRightDict); + + return devmode_enabled; +} + /* vAttach;pid @@ -3802,48 +3933,46 @@ rnb_err_t RNBRemote::HandlePacket_v(const char *p) { else m_ctx.LaunchStatus().SetErrorString("attach failed"); -#if defined(__APPLE__) && \ - (__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__ >= 101000) if (pid_attaching_to == INVALID_NUB_PROCESS && !attach_name.empty()) { pid_attaching_to = DNBProcessGetPIDByName(attach_name.c_str()); } - if (pid_attaching_to != INVALID_NUB_PROCESS && - strcmp(err_str, "No such process") != 0) { - // csr_check(CSR_ALLOW_TASK_FOR_PID) will be nonzero if System Integrity - // Protection is in effect. - if (csr_check(CSR_ALLOW_TASK_FOR_PID) != 0) { - bool attach_failed_due_to_sip = false; - - if (rootless_allows_task_for_pid(pid_attaching_to) == 0) { - attach_failed_due_to_sip = true; - } - if (!attach_failed_due_to_sip) { - int csops_flags = 0; - int retval = ::csops(pid_attaching_to, CS_OPS_STATUS, &csops_flags, - sizeof(csops_flags)); - if (retval != -1 && (csops_flags & CS_RESTRICT)) { - attach_failed_due_to_sip = true; - } - } - if (attach_failed_due_to_sip) { - std::string return_message = "E96;"; - return_message += cstring_to_asciihex_string( - "Process attach denied, possibly because " - "System Integrity Protection is enabled and " - "process does not allow attaching."); - - SendPacket(return_message.c_str()); - DNBLogError("Attach failed because process does not allow " - "attaching: \"%s\".", - err_str); - return rnb_err; - } + // attach_pid is INVALID_NUB_PROCESS - we did not succeed in attaching + // if the original request, pid_attaching_to, is available, see if we + // can figure out why we couldn't attach. Return an informative error + // string to lldb. + + if (pid_attaching_to != INVALID_NUB_PROCESS) { + if (process_does_not_exist (pid_attaching_to)) { + DNBLogError("Tried to attach to pid that doesn't exist"); + std::string return_message = "E96;"; + return_message += cstring_to_asciihex_string("no such process."); + return SendPacket(return_message.c_str()); + } + if (process_is_already_being_debugged (pid_attaching_to)) { + DNBLogError("Tried to attach to process already being debugged"); + std::string return_message = "E96;"; + return_message += cstring_to_asciihex_string("tried to attach to " + "process already being debugged"); + return SendPacket(return_message.c_str()); + } + if (!developer_mode_enabled()) { + DNBLogError("Developer mode is not enabled"); + std::string return_message = "E96;"; + return_message += cstring_to_asciihex_string("developer mode is not " + "enabled on this machine. " + "sudo DevToolsSecurity --enable"); + return SendPacket(return_message.c_str()); + } + if (attach_failed_due_to_sip (pid_attaching_to)) { + DNBLogError("Attach failed because of SIP protection."); + std::string return_message = "E96;"; + return_message += cstring_to_asciihex_string("cannot attach " + "to process due to System Integrity Protection"); + return SendPacket(return_message.c_str()); } } -#endif - SendPacket("E01"); // E01 is our magic error value for attach failed. DNBLogError("Attach failed: \"%s\".", err_str); return rnb_err; diff --git a/lldb/tools/debugserver/source/RNBServices.cpp b/lldb/tools/debugserver/source/RNBServices.cpp index 7b2ab7c..6e4b55e 100644 --- a/lldb/tools/debugserver/source/RNBServices.cpp +++ b/lldb/tools/debugserver/source/RNBServices.cpp @@ -12,6 +12,7 @@ #include "RNBServices.h" +#include "DNB.h" #include "CFString.h" #include "DNBLog.h" #include "MacOSX/CFUtils.h" @@ -28,16 +29,13 @@ #include #endif -// From DNB.cpp -size_t GetAllInfos(std::vector &proc_infos); - int GetProcesses(CFMutableArrayRef plistMutableArray, bool all_users) { if (plistMutableArray == NULL) return -1; // Running as root, get all processes std::vector proc_infos; - const size_t num_proc_infos = GetAllInfos(proc_infos); + const size_t num_proc_infos = DNBGetAllInfos(proc_infos); if (num_proc_infos > 0) { const pid_t our_pid = getpid(); const uid_t our_uid = getuid(); -- 2.7.4