From 79a1c39b3041da65b5cadf34653ad8f033c53564 Mon Sep 17 00:00:00 2001 From: topjohnwu Date: Thu, 28 Sep 2023 20:38:16 -0700 Subject: [PATCH] Simplify fd sanitization --- native/src/zygisk/hook.cpp | 101 +++++++++++++++++++------------------ 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/native/src/zygisk/hook.cpp b/native/src/zygisk/hook.cpp index 0401e6ac6..0587c9d3a 100644 --- a/native/src/zygisk/hook.cpp +++ b/native/src/zygisk/hook.cpp @@ -45,6 +45,8 @@ enum { FLAG_MAX }; +#define MAX_FD_SIZE 1024 + // Global variables vector> *plt_hook_list; map, StringCmp> *jni_hook_list; @@ -53,6 +55,7 @@ bool should_unmap_zygisk = false; // Current context HookContext *g_ctx; +bitset *g_allowed_fds = nullptr; const JNINativeInterface *old_functions = nullptr; JNINativeInterface *new_functions = nullptr; @@ -60,8 +63,6 @@ JNINativeInterface *new_functions = nullptr; void name##_pre(); \ void name##_post(); -#define MAX_FD_SIZE 1024 - struct HookContext { JNIEnv *env; union { @@ -77,7 +78,6 @@ struct HookContext { int pid; bitset flags; uint32_t info_flags; - bitset allowed_fds; vector exempted_fds; struct RegisterInfo { @@ -121,6 +121,7 @@ struct HookContext { void sanitize_fds(); bool exempt_fd(int fd); bool is_child() const { return pid <= 0; } + bool can_exempt_fd() const { return flags[APP_FORK_AND_SPECIALIZE] && args.app->fds_to_ignore; } // Compatibility shim void plt_hook_register(const char *regex, const char *symbol, void *fn, void **backup); @@ -169,19 +170,13 @@ DCL_HOOK_FUNC(int, unshare, int flags) { return res; } -// Close logd_fd if necessary to prevent crashing -// For more info, check comments in zygisk_log_write +// Sanitize file descriptors to prevent crashing DCL_HOOK_FUNC(void, android_log_close) { if (g_ctx == nullptr) { // Happens during un-managed fork like nativeForkApp, nativeForkUsap get_magiskd().close_log_pipe(); - } else if (!g_ctx->flags[SKIP_FD_SANITIZATION]) { - g_ctx->magiskd.close_log_pipe(); - if (g_ctx->is_child()) { - // Switch to plain old android logging because we cannot talk - // to magiskd to fetch our log pipe afterwards anyways. - android_logging(); - } + } else { + g_ctx->sanitize_fds(); } old_android_log_close(); } @@ -412,26 +407,32 @@ int sigmask(int how, int signum) { } void HookContext::fork_pre() { + if (g_allowed_fds == nullptr) { + default_new(g_allowed_fds); + + auto &allowed_fds = *g_allowed_fds; + // Record all open fds + auto dir = xopen_dir("/proc/self/fd"); + for (dirent *entry; (entry = xreaddir(dir.get()));) { + int fd = parse_int(entry->d_name); + if (fd < 0 || fd >= MAX_FD_SIZE) { + close(fd); + continue; + } + allowed_fds[fd] = true; + } + // The dirfd will be closed once out of scope + allowed_fds[dirfd(dir.get())] = false; + // logd_fd should be handled separately + if (int logd_fd = magiskd.get_log_pipe(); logd_fd >= 0) { + allowed_fds[logd_fd] = false; + } + } + // Do our own fork before loading any 3rd party code // First block SIGCHLD, unblock after original fork is done sigmask(SIG_BLOCK, SIGCHLD); pid = old_fork(); - - if (pid != 0 || flags[SKIP_FD_SANITIZATION]) - return; - - // Record all open fds - auto dir = xopen_dir("/proc/self/fd"); - for (dirent *entry; (entry = xreaddir(dir.get()));) { - int fd = parse_int(entry->d_name); - if (fd < 0 || fd >= MAX_FD_SIZE) { - close(fd); - continue; - } - allowed_fds[fd] = true; - } - // The dirfd should not be allowed - allowed_fds[dirfd(dir.get())] = false; } void HookContext::fork_post() { @@ -443,17 +444,27 @@ void HookContext::sanitize_fds() { if (flags[SKIP_FD_SANITIZATION]) return; - if (flags[APP_FORK_AND_SPECIALIZE] && args.app->fds_to_ignore) { - auto update_fd_array = [&](int off) -> jintArray { + if (!is_child() || g_allowed_fds == nullptr) { + magiskd.close_log_pipe(); + return; + } + + auto &allowed_fds = *g_allowed_fds; + if (can_exempt_fd()) { + if (int logd_fd = magiskd.get_log_pipe(); logd_fd >= 0) { + exempted_fds.push_back(logd_fd); + } + + auto update_fd_array = [&](int old_len) -> jintArray { if (exempted_fds.empty()) return nullptr; - jintArray array = env->NewIntArray(static_cast(off + exempted_fds.size())); + jintArray array = env->NewIntArray(static_cast(old_len + exempted_fds.size())); if (array == nullptr) return nullptr; - env->SetIntArrayRegion(array, off, static_cast(exempted_fds.size()), - exempted_fds.data()); + env->SetIntArrayRegion( + array, old_len, static_cast(exempted_fds.size()), exempted_fds.data()); for (int fd : exempted_fds) { if (fd >= 0 && fd < MAX_FD_SIZE) { allowed_fds[fd] = true; @@ -480,6 +491,11 @@ void HookContext::sanitize_fds() { } else { update_fd_array(0); } + } else { + magiskd.close_log_pipe(); + // Switch to plain old android logging because we cannot talk + // to magiskd to fetch our log pipe afterwards anyways. + android_logging(); } // Close all forbidden fds to prevent crashing @@ -487,8 +503,7 @@ void HookContext::sanitize_fds() { int dfd = dirfd(dir.get()); for (dirent *entry; (entry = xreaddir(dir.get()));) { int fd = parse_int(entry->d_name); - int logd_fd = magiskd.get_log_pipe(); - if ((fd < 0 || fd >= MAX_FD_SIZE || !allowed_fds[fd]) && fd != dfd && fd != logd_fd) { + if ((fd < 0 || fd >= MAX_FD_SIZE || !allowed_fds[fd]) && fd != dfd) { close(fd); } } @@ -645,7 +660,7 @@ HookContext::~HookContext() { bool HookContext::exempt_fd(int fd) { if (flags[POST_SPECIALIZE] || flags[SKIP_FD_SANITIZATION]) return true; - if (!flags[APP_FORK_AND_SPECIALIZE]) + if (!can_exempt_fd()) return false; exempted_fds.push_back(fd); return true; @@ -673,7 +688,6 @@ void HookContext::nativeForkSystemServer_pre() { fork_pre(); if (is_child()) { server_specialize_pre(); - sanitize_fds(); } } @@ -688,24 +702,11 @@ void HookContext::nativeForkSystemServer_post() { void HookContext::nativeForkAndSpecialize_pre() { process = env->GetStringUTFChars(args.app->nice_name, nullptr); ZLOGV("pre forkAndSpecialize [%s]\n", process); - flags[APP_FORK_AND_SPECIALIZE] = true; - if (args.app->fds_to_ignore == nullptr) { - // if fds_to_ignore does not exist, we do not have a good way to determine - // whether keeping fd open during fork is allowed, as needed symbols may be - // inlined. Better be safe than sorry. - flags[SKIP_FD_SANITIZATION] = false; - } else { - int logd_fd = magiskd.get_log_pipe(); - if (logd_fd >= 0) { - exempted_fds.push_back(logd_fd); - } - } fork_pre(); if (is_child()) { app_specialize_pre(); - sanitize_fds(); } }