commit d04631f4a9e3ddf02e446926b817a245074a804a
author Alex Buynytskyy <alexbuy@google.com> Fri Aug 12 15:14:41 2022 -0700
committer Alex Buynytskyy <alexbuy@google.com> Fri Aug 12 15:28:31 2022 -0700
tree 9e58b5b68d024b2fe18897b209ffc162b2a3c0b3
parent 0bdfe03d68d30c251cd2a76a2926d6a4cb2e4943

Fix for: copySystemProfile can cause arbitrary file read and write

Bug: 216113220
Fixes: 216113220
Test: atest installd_dexopt_test
Change-Id: Iff44fb23c351ceacf4ce5d8a2394495e7afc84c8

cmds/installd/InstalldNativeService.cpp

diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp
index ee3a67e..a2de471 100644
--- a/cmds/installd/InstalldNativeService.cpp
+++ b/cmds/installd/InstalldNativeService.cpp
@@ -230,6 +230,19 @@
     }
 }
 
+binder::Status checkArgumentFileName(const std::string& path) {
+    if (path.empty()) {
+        return exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing name");
+    }
+    for (const char& c : path) {
+        if (c == '\0' || c == '\n' || c == '/') {
+            return exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+                             StringPrintf("Name %s is malformed", path.c_str()));
+        }
+    }
+    return ok();
+}
+
 #define ENFORCE_UID(uid) {                                  \
     binder::Status status = checkUid((uid));                \
     if (!status.isOk()) {                                   \
@@ -266,6 +279,14 @@
     }                                                       \
 }
 
+#define CHECK_ARGUMENT_FILE_NAME(path)                         \
+    {                                                          \
+        binder::Status status = checkArgumentFileName((path)); \
+        if (!status.isOk()) {                                  \
+            return status;                                     \
+        }                                                      \
+    }
+
 #ifdef GRANULAR_LOCKS
 
 /**
@@ -2974,7 +2995,19 @@
         int32_t packageUid, const std::string& packageName, const std::string& profileName,
         bool* _aidl_return) {
     ENFORCE_UID(AID_SYSTEM);
+    CHECK_ARGUMENT_PATH(systemProfile);
+    if (!base::EndsWith(systemProfile, ".prof")) {
+        return exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+                         StringPrintf("System profile path %s does not end with .prof",
+                                      systemProfile.c_str()));
+    }
     CHECK_ARGUMENT_PACKAGE_NAME(packageName);
+    CHECK_ARGUMENT_FILE_NAME(profileName);
+    if (!base::EndsWith(profileName, ".prof")) {
+        return exception(binder::Status::EX_ILLEGAL_ARGUMENT,
+                         StringPrintf("Profile name %s does not end with .prof",
+                                      profileName.c_str()));
+    }
     LOCK_PACKAGE();
     *_aidl_return = copy_system_profile(systemProfile, packageUid, packageName, profileName);
     return ok();

cmds/installd/tests/installd_dexopt_test.cpp

diff --git a/cmds/installd/tests/installd_dexopt_test.cpp b/cmds/installd/tests/installd_dexopt_test.cpp
index 800e141..aadf6dd 100644
--- a/cmds/installd/tests/installd_dexopt_test.cpp
+++ b/cmds/installd/tests/installd_dexopt_test.cpp
@@ -1293,6 +1293,41 @@
     preparePackageProfile(package_name_, "primary.prof", /*expected_result*/ false);
 }
 
+TEST_F(ProfileTest, CopySystemProfileOk) {
+    LOG(INFO) << "CopySystemProfileOk";
+
+    bool result;
+    ASSERT_BINDER_SUCCESS(
+            service_->copySystemProfile("/data/app/random.string/package.name.random/base.apk.prof",
+                                        kTestAppUid, package_name_, "primary.prof", &result));
+}
+
+TEST_F(ProfileTest, CopySystemProfileFailWrongSystemProfilePath) {
+    LOG(INFO) << "CopySystemProfileFailWrongSystemProfilePath";
+
+    bool result;
+    ASSERT_BINDER_FAIL(service_->copySystemProfile("../../secret.dat", kTestAppUid, package_name_,
+                                                   "primary.prof", &result));
+    ASSERT_BINDER_FAIL(service_->copySystemProfile("/data/user/package.name/secret.data",
+                                                   kTestAppUid, package_name_, "primary.prof",
+                                                   &result));
+}
+
+TEST_F(ProfileTest, CopySystemProfileFailWrongProfileName) {
+    LOG(INFO) << "CopySystemProfileFailWrongProfileName";
+
+    bool result;
+    ASSERT_BINDER_FAIL(
+            service_->copySystemProfile("/data/app/random.string/package.name.random/base.apk.prof",
+                                        kTestAppUid, package_name_,
+                                        "../../../../dalvik-cache/arm64/test.vdex", &result));
+    ASSERT_BINDER_FAIL(
+            service_->copySystemProfile("/data/app/random.string/package.name.random/base.apk.prof",
+                                        kTestAppUid, package_name_, "/test.prof", &result));
+    ASSERT_BINDER_FAIL(
+            service_->copySystemProfile("/data/app/random.string/package.name.random/base.apk.prof",
+                                        kTestAppUid, package_name_, "base.apk", &result));
+}
 
 class BootProfileTest : public ProfileTest {
   public: