From 02aae1bbd7c39207bd9901e7586b09918ceea4c3 Mon Sep 17 00:00:00 2001 From: snake-4 <18491360+snake-4@users.noreply.github.com> Date: Fri, 3 May 2024 19:54:40 +0200 Subject: [PATCH] Fixed memory safety of prop hider --- module/jni/include/utils.hpp | 22 ++++++++---- module/jni/modules.cpp | 65 +++++++++++++++++++----------------- module/jni/utils.cpp | 15 +++++++++ 3 files changed, 65 insertions(+), 37 deletions(-) diff --git a/module/jni/include/utils.hpp b/module/jni/include/utils.hpp index 3fb3c3e..6999271 100644 --- a/module/jni/include/utils.hpp +++ b/module/jni/include/utils.hpp @@ -9,21 +9,29 @@ ret (*old_##func)(__VA_ARGS__) = nullptr; \ ret new_##func(__VA_ARGS__) -#define ASSERT_LOG(tag, expr) \ - if (!(expr)) \ - { \ +#define ASSERT_LOG(tag, expr) \ + if (!(expr)) \ + { \ LOGE("%s:%d Assertion %s failed. %d:%s", #tag, __LINE__, #expr, errno, std::strerror(errno)); \ } -#define ASSERT_DO(tag, expr, ret) \ - if (!(expr)) \ - { \ +#define ASSERT_DO(tag, expr, ret) \ + if (!(expr)) \ + { \ LOGE("%s:%d Assertion %s failed. %d:%s", #tag, __LINE__, #expr, errno, std::strerror(errno)); \ - ret; \ + ret; \ } namespace Utils { + /* + * Always null terminates dest if dest_size is at least 1. + * Writes at most dest_size bytes to dest including null terminator. + * Reads at most dest_size bytes from src. + * Returns strlen(dest) + */ + size_t safeStringCopy(char *dest, const char *src, size_t dest_size); + bool switchMountNS(int pid); int isUserAppUID(int uid); bool hookPLTByName(zygisk::Api *api, const std::string &libName, const std::string &symbolName, void *hookFunc, void **origFunc); diff --git a/module/jni/modules.cpp b/module/jni/modules.cpp index 19f1b82..7b098ec 100644 --- a/module/jni/modules.cpp +++ b/module/jni/modules.cpp @@ -1,4 +1,5 @@ #include +#include #include #include #include @@ -168,49 +169,53 @@ void doHideZygisk() } } -void doMrProp() +static bool shouldResetProperty(const prop_info *pi) { - static bool isInitialized = false; - static int resetCount = 0; - if (!isInitialized) + // Read-write properties or properties with long values should not be reset + if (strncmp(pi->name, "ro.", 3) != 0 || pi->is_long()) + return false; + + // Check if the serial indicates that it was modified + auto serial = std::atomic_load_explicit(&pi->serial, std::memory_order_relaxed); + if ((serial & 0xFFFFFF) != 0) + return true; + + // Check if any characters exist beyond the null-terminated string + size_t length = strnlen(pi->value, PROP_VALUE_MAX); + for (size_t i = length; i < PROP_VALUE_MAX; i++) { - isInitialized = __system_properties_init() == 0; + if (pi->value[i] != '\0') + return true; } + return false; +} + +void doMrProp() +{ + static bool isInitialized = false; + static int resetCount = 0; if (!isInitialized) { - LOGE("Could not initialize system_properties!"); - return; + if (__system_properties_init() == -1) + { + LOGE("Could not initialize system_properties!"); + return; + } + isInitialized = true; } int ret = __system_property_foreach( [](const prop_info *pi, void *) { - if (std::string_view(pi->name).starts_with("ro.") && !pi->is_long()) + if (shouldResetProperty(pi)) { - auto serial = std::atomic_load_explicit(&pi->serial, std::memory_order_relaxed); - - // Well this is a bit dangerous - bool shouldReset = (serial & 0xFF) != 0; - auto length = strlen(pi->value); + // Overlapping pointers in strncpy is undefined behavior so make a copy. + char buffer[PROP_VALUE_MAX]; + size_t length = Utils::safeStringCopy(buffer, pi->value, PROP_VALUE_MAX); - if (!shouldReset) - { - for (size_t i = length; i < PROP_VALUE_MAX; i++) - { - if (pi->value[i] != 0) - { - shouldReset = true; - break; - } - } - } - - if (shouldReset) - { - resetCount++; - __system_property_update(const_cast(pi), pi->value, length); - } + __system_property_update(const_cast(pi), buffer, length); + resetCount++; } }, nullptr); diff --git a/module/jni/utils.cpp b/module/jni/utils.cpp index 4924800..8114cc6 100644 --- a/module/jni/utils.cpp +++ b/module/jni/utils.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -14,6 +15,20 @@ using namespace Utils; +size_t Utils::safeStringCopy(char *dest, const char *src, size_t dest_size) +{ + if (dest_size < 1) + return 0; + + *dest = 0; + // strncpy does not null terminate, strlcpy fails on non-terminated src + // strncat is relatively safe but may write count+1 because of the null terminator + strncat(dest, src, dest_size - 1); + + // strlen is safe here because dest is now null-terminated + return strlen(dest); +} + bool Utils::hookPLTByName(zygisk::Api *api, const std::string &libName, const std::string &symbolName, void *hookFunc, void **origFunc) { for (const auto &map : Parsers::parseSelfMaps())