diff options
author | Alex Buynytskyy <alexbuy@google.com> | 2022-08-12 15:14:41 -0700 |
---|---|---|
committer | Alex Buynytskyy <alexbuy@google.com> | 2022-08-12 15:28:31 -0700 |
commit | d04631f4a9e3ddf02e446926b817a245074a804a (patch) | |
tree | 9e58b5b68d024b2fe18897b209ffc162b2a3c0b3 | |
parent | 0bdfe03d68d30c251cd2a76a2926d6a4cb2e4943 (diff) | |
download | native-d04631f4a9e3ddf02e446926b817a245074a804a.tar.gz |
Fix for: copySystemProfile can cause arbitrary file read and write
Bug: 216113220
Fixes: 216113220
Test: atest installd_dexopt_test
Change-Id: Iff44fb23c351ceacf4ce5d8a2394495e7afc84c8
-rw-r--r-- | cmds/installd/InstalldNativeService.cpp | 33 | ||||
-rw-r--r-- | cmds/installd/tests/installd_dexopt_test.cpp | 35 |
2 files changed, 68 insertions, 0 deletions
diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp index ee3a67e5fd..a2de471807 100644 --- a/cmds/installd/InstalldNativeService.cpp +++ b/cmds/installd/InstalldNativeService.cpp @@ -230,6 +230,19 @@ binder::Status checkArgumentPath(const std::optional<std::string>& path) { } } +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 @@ binder::Status checkArgumentPath(const std::optional<std::string>& path) { } \ } +#define CHECK_ARGUMENT_FILE_NAME(path) \ + { \ + binder::Status status = checkArgumentFileName((path)); \ + if (!status.isOk()) { \ + return status; \ + } \ + } + #ifdef GRANULAR_LOCKS /** @@ -2974,7 +2995,19 @@ binder::Status InstalldNativeService::copySystemProfile(const std::string& syste 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(); diff --git a/cmds/installd/tests/installd_dexopt_test.cpp b/cmds/installd/tests/installd_dexopt_test.cpp index 800e141d92..aadf6dd086 100644 --- a/cmds/installd/tests/installd_dexopt_test.cpp +++ b/cmds/installd/tests/installd_dexopt_test.cpp @@ -1293,6 +1293,41 @@ TEST_F(ProfileTest, ProfilePrepareFailProfileChangedUid) { 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: |