From a1ce6f5f120b1ce61d567cbd1f89d66cd5cf36a2 Mon Sep 17 00:00:00 2001 From: topjohnwu Date: Mon, 6 Mar 2023 03:25:59 -0800 Subject: [PATCH] Fix race condition when switching root manager Before this change, the root manager package name is only written into the database after the repackaged APK is installed. In the time between the repackaged APK being installed and the package name being written into the database, if some operation calls `get_manager`, the Magisk daemon will cache this result and ignore the repackaged APK, even if the package name is set afterwards, because the cache won't be invalidated. The result is that the repackaged manager APK will not be recognized as the root manager, breaking the hide manager feature. This race condition is more likely to happen when Zygisk is enabled, because `get_manager` is called with a very high frequency in that case. To fix the issue, we have to set the new package name into the database BEFORE installing the repackaged APK. We also stop pruning the database if the repackaged manager is not found, moving this logic into the Magisk app. By doing so, we can guarantee that the instant after the repackaged manager APK is installed, the Magisk daemon will immediately pick it up and treat it as the root manager. Another small optimization: when the requester is root, simply bypass the whole database + manager package check. Since the Magisk app hiding APK installation proces will call `su` several times to run `pm` under different UIDs, doing this opimization will reduce the amount of unnecessary database query + filesystem traversals. --- .../topjohnwu/magisk/core/tasks/HideAPK.kt | 3 ++- .../com/topjohnwu/magisk/ui/SplashActivity.kt | 2 +- native/src/core/package.cpp | 26 +++++++++---------- native/src/su/su_daemon.cpp | 11 +++++--- 4 files changed, 23 insertions(+), 19 deletions(-) diff --git a/app/src/main/java/com/topjohnwu/magisk/core/tasks/HideAPK.kt b/app/src/main/java/com/topjohnwu/magisk/core/tasks/HideAPK.kt index 500d5fb57..d7c23a609 100644 --- a/app/src/main/java/com/topjohnwu/magisk/core/tasks/HideAPK.kt +++ b/app/src/main/java/com/topjohnwu/magisk/core/tasks/HideAPK.kt @@ -160,7 +160,6 @@ object HideAPK { private fun launchApp(activity: Activity, pkg: String) { val intent = activity.packageManager.getLaunchIntentForPackage(pkg) ?: return - Config.suManager = if (pkg == APPLICATION_ID) "" else pkg val self = activity.packageName val flag = Intent.FLAG_GRANT_READ_URI_PERMISSION activity.grantUriPermission(pkg, Provider.preferencesUri(self), flag) @@ -191,6 +190,7 @@ object HideAPK { launchApp(activity, pkg) } + Config.suManager = pkg val cmd = "adb_pm_install $repack $pkg" if (Shell.cmd(cmd).exec().isSuccess) return true @@ -239,6 +239,7 @@ object HideAPK { launchApp(activity, APPLICATION_ID) dialog.dismiss() } + Config.suManager = "" val cmd = "adb_pm_install $apk $APPLICATION_ID" if (Shell.cmd(cmd).await().isSuccess) return val success = withContext(Dispatchers.IO) { diff --git a/app/src/main/java/com/topjohnwu/magisk/ui/SplashActivity.kt b/app/src/main/java/com/topjohnwu/magisk/ui/SplashActivity.kt index 97932f378..71f2bdef5 100644 --- a/app/src/main/java/com/topjohnwu/magisk/ui/SplashActivity.kt +++ b/app/src/main/java/com/topjohnwu/magisk/ui/SplashActivity.kt @@ -134,7 +134,7 @@ abstract class SplashActivity : NavigationActivity/dev/null 2>&1").exec() } } else { - if (!Const.Version.atLeast_25_0() && Config.suManager.isNotEmpty()) + if (Config.suManager.isNotEmpty()) Config.suManager = "" pkg ?: return Shell.cmd("(pm uninstall $pkg)& >/dev/null 2>&1").exec() diff --git a/native/src/core/package.cpp b/native/src/core/package.cpp index 0617429e0..26a84e373 100644 --- a/native/src/core/package.cpp +++ b/native/src/core/package.cpp @@ -126,7 +126,7 @@ int get_manager(int user_id, string *pkg, bool install) { if (access(app_path, F_OK) == 0) { // Always check dyn signature for repackaged app if (!mgr_pkg->empty() && !check_dyn(user_id)) - goto not_found; + goto ignore; if (pkg) *pkg = name; return user_id * AID_USER_OFFSET + mgr_app_id; } else { @@ -163,7 +163,7 @@ int get_manager(int user_id, string *pkg, bool install) { // Check the repackaged package name bool invalid = false; - auto check_pkg = [&](int u) -> bool { + auto check_stub_apk = [&](int u) -> bool { ssprintf(app_path, sizeof(app_path), "%s/%d/%s", APP_DATA_DIR, u, str[SU_MANAGER].data()); if (stat(app_path, &st) == 0) { @@ -194,16 +194,16 @@ int get_manager(int user_id, string *pkg, bool install) { return false; }; - if (check_pkg(user_id)) { + if (check_stub_apk(user_id)) { if (!check_dyn(user_id)) - goto not_found; + goto ignore; if (pkg) *pkg = *mgr_pkg; return st.st_uid; } if (!invalid) { collect_users(); for (int u : users) { - if (check_pkg(u)) { + if (check_stub_apk(u)) { // Found repackaged app, but not installed in the requested user goto not_found; } @@ -212,16 +212,13 @@ int get_manager(int user_id, string *pkg, bool install) { } } - // Repackaged app not found, remove package from db - rm_db_strings(SU_MANAGER); - - // Fallthrough + // Repackaged app not found, fall through } // Check the original package name bool invalid = false; - auto check_pkg = [&](int u) -> bool { + auto check_apk = [&](int u) -> bool { ssprintf(app_path, sizeof(app_path), "%s/%d/" JAVA_PACKAGE_NAME, APP_DATA_DIR, u); if (stat(app_path, &st) == 0) { #if ENFORCE_SIGNATURE @@ -246,14 +243,14 @@ int get_manager(int user_id, string *pkg, bool install) { return false; }; - if (check_pkg(user_id)) { + if (check_apk(user_id)) { if (pkg) *pkg = JAVA_PACKAGE_NAME; return st.st_uid; } if (!invalid) { collect_users(); for (int u : users) { - if (check_pkg(u)) { + if (check_apk(u)) { // Found app, but not installed in the requested user goto not_found; } @@ -271,8 +268,9 @@ int get_manager(int user_id, string *pkg, bool install) { install_stub(); not_found: - const char *name = mgr_pkg->empty() ? JAVA_PACKAGE_NAME : mgr_pkg->data(); - LOGW("pkg: cannot find %s for user=[%d]\n", name, user_id); + LOGW("pkg: cannot find %s for user=[%d]\n", + mgr_pkg->empty() ? JAVA_PACKAGE_NAME : mgr_pkg->data(), user_id); +ignore: if (pkg) pkg->clear(); return -1; } diff --git a/native/src/su/su_daemon.cpp b/native/src/su/su_daemon.cpp index 659782e11..4edaf4321 100644 --- a/native/src/su/su_daemon.cpp +++ b/native/src/su/su_daemon.cpp @@ -167,8 +167,13 @@ void prune_su_access() { static shared_ptr get_su_info(unsigned uid) { LOGD("su: request from uid=[%d]\n", uid); - shared_ptr info; + if (uid == AID_ROOT) { + auto info = make_shared(uid); + info->access = SILENT_SU_ACCESS; + return info; + } + shared_ptr info; { mutex_guard lock(cache_lock); if (!cached || cached->uid != uid || !cached->is_fresh()) @@ -183,8 +188,8 @@ static shared_ptr get_su_info(unsigned uid) { // Not cached, get data from database info->check_db(); - // If it's root or the manager, allow it silently - if (info->uid == AID_ROOT || to_app_id(info->uid) == to_app_id(info->mgr_uid)) { + // If it's the manager, allow it silently + if (to_app_id(info->uid) == to_app_id(info->mgr_uid)) { info->access = SILENT_SU_ACCESS; return info; }