Fixed memory safety of prop hider

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);

module/jni/modules.cpp

@@ -1,4 +1,5 @@
 #include <string>
+#include <cstring>
 #include <string_view>
 #include <set>
 #include <atomic>
@@ -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<prop_info *>(pi), pi->value, length);
-                }
+                __system_property_update(const_cast<prop_info *>(pi), buffer, length);
+                resetCount++;
             }
         },
         nullptr);

module/jni/utils.cpp

@@ -1,3 +1,4 @@
+#include <string.h>
 #include <string>
 #include <functional>
 #include <format>
@@ -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())