From 131365a3e560224edd48f73ecf45bcc57cad4c6d Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Tue, 24 Mar 2020 23:49:02 -0700 Subject: [PATCH] [vold] Add argument verification to IncFS methods + Get rid of an extra string copy in path validation function Bug: 152349257 Test: atest vold_tests Change-Id: I03a8cab0dd6abd7d5c9dcbbc2acb651e818e6cd8 --- Android.bp | 1 + VoldNativeService.cpp | 158 +++++++-------------- VoldNativeServiceValidation.cpp | 109 ++++++++++++++ VoldNativeServiceValidation.h | 37 +++++ tests/Android.bp | 2 + tests/VoldNativeServiceValidation_test.cpp | 51 +++++++ 6 files changed, 248 insertions(+), 110 deletions(-) create mode 100644 VoldNativeServiceValidation.cpp create mode 100644 VoldNativeServiceValidation.h create mode 100644 tests/VoldNativeServiceValidation_test.cpp diff --git a/Android.bp b/Android.bp index c2f8936..03dde1e 100644 --- a/Android.bp +++ b/Android.bp @@ -131,6 +131,7 @@ cc_library_static { "ScryptParameters.cpp", "Utils.cpp", "VoldNativeService.cpp", + "VoldNativeServiceValidation.cpp", "VoldUtil.cpp", "VolumeManager.cpp", "cryptfs.cpp", diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 57cee23..5a77a83 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -26,6 +26,7 @@ #include "MetadataCrypt.h" #include "MoveStorage.h" #include "Process.h" +#include "VoldNativeServiceValidation.h" #include "VoldUtil.h" #include "VolumeManager.h" #include "cryptfs.h" @@ -45,6 +46,7 @@ using android::base::StringPrintf; using std::endl; +using namespace std::literals; namespace android { namespace vold { @@ -53,14 +55,6 @@ namespace { constexpr const char* kDump = "android.permission.DUMP"; -static binder::Status ok() { - return binder::Status::ok(); -} - -static binder::Status exception(uint32_t code, const std::string& msg) { - return binder::Status::fromExceptionCode(code, String8(msg.c_str())); -} - static binder::Status error(const std::string& msg) { PLOG(ERROR) << msg; return binder::Status::fromServiceSpecificError(errno, String8(msg.c_str())); @@ -82,77 +76,9 @@ static binder::Status translateBool(bool status) { } } -binder::Status checkPermission(const char* permission) { - pid_t pid; - uid_t uid; - - if (checkCallingPermission(String16(permission), reinterpret_cast(&pid), - reinterpret_cast(&uid))) { - return ok(); - } else { - return exception(binder::Status::EX_SECURITY, - StringPrintf("UID %d / PID %d lacks permission %s", uid, pid, permission)); - } -} - -binder::Status checkUidOrRoot(uid_t expectedUid) { - uid_t uid = IPCThreadState::self()->getCallingUid(); - if (uid == expectedUid || uid == AID_ROOT) { - return ok(); - } else { - return exception(binder::Status::EX_SECURITY, - StringPrintf("UID %d is not expected UID %d", uid, expectedUid)); - } -} - -binder::Status checkArgumentId(const std::string& id) { - if (id.empty()) { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing ID"); - } - for (const char& c : id) { - if (!std::isalnum(c) && c != ':' && c != ',' && c != ';') { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, - StringPrintf("ID %s is malformed", id.c_str())); - } - } - return ok(); -} - -binder::Status checkArgumentPath(const std::string& path) { - if (path.empty()) { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing path"); - } - if (path[0] != '/') { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, - StringPrintf("Path %s is relative", path.c_str())); - } - if ((path + '/').find("/../") != std::string::npos) { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, - StringPrintf("Path %s is shady", path.c_str())); - } - for (const char& c : path) { - if (c == '\0' || c == '\n') { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, - StringPrintf("Path %s is malformed", path.c_str())); - } - } - return ok(); -} - -binder::Status checkArgumentHex(const std::string& hex) { - // Empty hex strings are allowed - for (const char& c : hex) { - if (!std::isxdigit(c) && c != ':' && c != '-') { - return exception(binder::Status::EX_ILLEGAL_ARGUMENT, - StringPrintf("Hex %s is malformed", hex.c_str())); - } - } - return ok(); -} - #define ENFORCE_SYSTEM_OR_ROOT \ { \ - binder::Status status = checkUidOrRoot(AID_SYSTEM); \ + binder::Status status = CheckUidOrRoot(AID_SYSTEM); \ if (!status.isOk()) { \ return status; \ } \ @@ -160,7 +86,7 @@ binder::Status checkArgumentHex(const std::string& hex) { #define CHECK_ARGUMENT_ID(id) \ { \ - binder::Status status = checkArgumentId((id)); \ + binder::Status status = CheckArgumentId((id)); \ if (!status.isOk()) { \ return status; \ } \ @@ -168,7 +94,7 @@ binder::Status checkArgumentHex(const std::string& hex) { #define CHECK_ARGUMENT_PATH(path) \ { \ - binder::Status status = checkArgumentPath((path)); \ + binder::Status status = CheckArgumentPath((path)); \ if (!status.isOk()) { \ return status; \ } \ @@ -176,7 +102,7 @@ binder::Status checkArgumentHex(const std::string& hex) { #define CHECK_ARGUMENT_HEX(hex) \ { \ - binder::Status status = checkArgumentHex((hex)); \ + binder::Status status = CheckArgumentHex((hex)); \ if (!status.isOk()) { \ return status; \ } \ @@ -206,7 +132,7 @@ status_t VoldNativeService::start() { status_t VoldNativeService::dump(int fd, const Vector& /* args */) { auto out = std::fstream(StringPrintf("/proc/self/fd/%d", fd)); - const binder::Status dump_permission = checkPermission(kDump); + const binder::Status dump_permission = CheckPermission(kDump); if (!dump_permission.isOk()) { out << dump_permission.toString8() << endl; return PERMISSION_DENIED; @@ -214,7 +140,6 @@ status_t VoldNativeService::dump(int fd, const Vector& /* args */) { ACQUIRE_LOCK; out << "vold is happy!" << endl; - out.flush(); return NO_ERROR; } @@ -224,7 +149,7 @@ binder::Status VoldNativeService::setListener( ACQUIRE_LOCK; VolumeManager::Instance()->setListener(listener); - return ok(); + return Ok(); } binder::Status VoldNativeService::monitor() { @@ -234,7 +159,7 @@ binder::Status VoldNativeService::monitor() { { ACQUIRE_LOCK; } { ACQUIRE_CRYPT_LOCK; } - return ok(); + return Ok(); } binder::Status VoldNativeService::reset() { @@ -281,12 +206,12 @@ binder::Status VoldNativeService::onUserStopped(int32_t userId) { binder::Status VoldNativeService::addAppIds(const std::vector& packageNames, const std::vector& appIds) { - return ok(); + return Ok(); } binder::Status VoldNativeService::addSandboxIds(const std::vector& appIds, const std::vector& sandboxIds) { - return ok(); + return Ok(); } binder::Status VoldNativeService::onSecureKeyguardStateChanged(bool isShowing) { @@ -403,7 +328,7 @@ static binder::Status pathForVolId(const std::string& volId, std::string* path) return error("Volume " + volId + " missing path"); } } - return ok(); + return Ok(); } binder::Status VoldNativeService::benchmark( @@ -417,7 +342,7 @@ binder::Status VoldNativeService::benchmark( if (!status.isOk()) return status; std::thread([=]() { android::vold::Benchmark(path, listener); }).detach(); - return ok(); + return Ok(); } binder::Status VoldNativeService::checkEncryption(const std::string& volId) { @@ -448,7 +373,7 @@ binder::Status VoldNativeService::moveStorage( } std::thread([=]() { android::vold::MoveStorage(fromVol, toVol, listener); }).detach(); - return ok(); + return Ok(); } binder::Status VoldNativeService::remountUid(int32_t uid, int32_t remountMode) { @@ -534,7 +459,7 @@ binder::Status VoldNativeService::fstrim( ACQUIRE_LOCK; std::thread([=]() { android::vold::Trim(listener); }).detach(); - return ok(); + return Ok(); } binder::Status VoldNativeService::runIdleMaint( @@ -543,7 +468,7 @@ binder::Status VoldNativeService::runIdleMaint( ACQUIRE_LOCK; std::thread([=]() { android::vold::RunIdleMaint(listener); }).detach(); - return ok(); + return Ok(); } binder::Status VoldNativeService::abortIdleMaint( @@ -552,7 +477,7 @@ binder::Status VoldNativeService::abortIdleMaint( ACQUIRE_LOCK; std::thread([=]() { android::vold::AbortIdleMaint(listener); }).detach(); - return ok(); + return Ok(); } binder::Status VoldNativeService::mountAppFuse(int32_t uid, int32_t mountId, @@ -584,7 +509,7 @@ binder::Status VoldNativeService::openAppFuseFile(int32_t uid, int32_t mountId, } *_aidl_return = android::base::unique_fd(fd); - return ok(); + return Ok(); } binder::Status VoldNativeService::fdeCheckPassword(const std::string& password) { @@ -601,7 +526,7 @@ binder::Status VoldNativeService::fdeRestart() { // Spawn as thread so init can issue commands back to vold without // causing deadlock, usually as a result of prep_data_fs. std::thread(&cryptfs_restart).detach(); - return ok(); + return Ok(); } binder::Status VoldNativeService::fdeComplete(int32_t* _aidl_return) { @@ -609,7 +534,7 @@ binder::Status VoldNativeService::fdeComplete(int32_t* _aidl_return) { ACQUIRE_CRYPT_LOCK; *_aidl_return = cryptfs_crypto_complete(); - return ok(); + return Ok(); } static int fdeEnableInternal(int32_t passwordType, const std::string& password, @@ -649,7 +574,7 @@ binder::Status VoldNativeService::fdeEnable(int32_t passwordType, const std::str // Spawn as thread so init can issue commands back to vold without // causing deadlock, usually as a result of prep_data_fs. std::thread(&fdeEnableInternal, passwordType, password, encryptionFlags).detach(); - return ok(); + return Ok(); } binder::Status VoldNativeService::fdeChangePassword(int32_t passwordType, @@ -676,7 +601,7 @@ binder::Status VoldNativeService::fdeGetField(const std::string& key, std::strin return error(StringPrintf("Failed to read field %s", key.c_str())); } else { *_aidl_return = buf; - return ok(); + return Ok(); } } @@ -692,7 +617,7 @@ binder::Status VoldNativeService::fdeGetPasswordType(int32_t* _aidl_return) { ACQUIRE_CRYPT_LOCK; *_aidl_return = cryptfs_get_password_type(); - return ok(); + return Ok(); } binder::Status VoldNativeService::fdeGetPassword(std::string* _aidl_return) { @@ -703,7 +628,7 @@ binder::Status VoldNativeService::fdeGetPassword(std::string* _aidl_return) { if (res != nullptr) { *_aidl_return = res; } - return ok(); + return Ok(); } binder::Status VoldNativeService::fdeClearPassword() { @@ -711,7 +636,7 @@ binder::Status VoldNativeService::fdeClearPassword() { ACQUIRE_CRYPT_LOCK; cryptfs_clear_password(); - return ok(); + return Ok(); } binder::Status VoldNativeService::fbeEnable() { @@ -730,7 +655,7 @@ binder::Status VoldNativeService::mountDefaultEncrypted() { // causing deadlock, usually as a result of prep_data_fs. std::thread(&cryptfs_mount_default_encrypted).detach(); } - return ok(); + return Ok(); } binder::Status VoldNativeService::initUser0() { @@ -745,7 +670,7 @@ binder::Status VoldNativeService::isConvertibleToFbe(bool* _aidl_return) { ACQUIRE_CRYPT_LOCK; *_aidl_return = cryptfs_isConvertibleToFBE() != 0; - return ok(); + return Ok(); } binder::Status VoldNativeService::mountFstab(const std::string& blkDevice, @@ -845,13 +770,13 @@ binder::Status VoldNativeService::destroyUserStorage(const std::unique_ptr= 0) { _aidl_return->log.reset(unique_fd(result.logs)); } - return ok(); + return Ok(); } binder::Status VoldNativeService::unmountIncFs(const std::string& dir) { + ENFORCE_SYSTEM_OR_ROOT; + CHECK_ARGUMENT_PATH(dir); + return translate(IncFs_Unmount(dir.c_str())); } binder::Status VoldNativeService::bindMount(const std::string& sourceDir, const std::string& targetDir) { + ENFORCE_SYSTEM_OR_ROOT; + CHECK_ARGUMENT_PATH(sourceDir); + CHECK_ARGUMENT_PATH(targetDir); + return translate(IncFs_BindMount(sourceDir.c_str(), targetDir.c_str())); } diff --git a/VoldNativeServiceValidation.cpp b/VoldNativeServiceValidation.cpp new file mode 100644 index 0000000..2e21ace --- /dev/null +++ b/VoldNativeServiceValidation.cpp @@ -0,0 +1,109 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "VoldNativeServiceValidation.h" + +#include +#include +#include +#include +#include + +#include +#include + +using android::base::StringPrintf; +using namespace std::literals; + +namespace android::vold { + +binder::Status Ok() { + return binder::Status::ok(); +} + +binder::Status Exception(uint32_t code, const std::string& msg) { + return binder::Status::fromExceptionCode(code, String8(msg.c_str())); +} + +binder::Status CheckPermission(const char* permission) { + pid_t pid; + uid_t uid; + + if (checkCallingPermission(String16(permission), reinterpret_cast(&pid), + reinterpret_cast(&uid))) { + return Ok(); + } else { + return Exception(binder::Status::EX_SECURITY, + StringPrintf("UID %d / PID %d lacks permission %s", uid, pid, permission)); + } +} + +binder::Status CheckUidOrRoot(uid_t expectedUid) { + uid_t uid = IPCThreadState::self()->getCallingUid(); + if (uid == expectedUid || uid == AID_ROOT) { + return Ok(); + } else { + return Exception(binder::Status::EX_SECURITY, + StringPrintf("UID %d is not expected UID %d", uid, expectedUid)); + } +} + +binder::Status CheckArgumentId(const std::string& id) { + if (id.empty()) { + return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing ID"); + } + for (const char& c : id) { + if (!std::isalnum(c) && c != ':' && c != ',' && c != ';') { + return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, + StringPrintf("ID %s is malformed", id.c_str())); + } + } + return Ok(); +} + +binder::Status CheckArgumentPath(const std::string& path) { + if (path.empty()) { + return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Missing path"); + } + if (path[0] != '/') { + return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, + StringPrintf("Path %s is relative", path.c_str())); + } + if (path.find("/../"sv) != path.npos || android::base::EndsWith(path, "/.."sv)) { + return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, + StringPrintf("Path %s is shady", path.c_str())); + } + for (const char& c : path) { + if (c == '\0' || c == '\n') { + return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, + StringPrintf("Path %s is malformed", path.c_str())); + } + } + return Ok(); +} + +binder::Status CheckArgumentHex(const std::string& hex) { + // Empty hex strings are allowed + for (const char& c : hex) { + if (!std::isxdigit(c) && c != ':' && c != '-') { + return Exception(binder::Status::EX_ILLEGAL_ARGUMENT, + StringPrintf("Hex %s is malformed", hex.c_str())); + } + } + return Ok(); +} + +} // namespace android::vold diff --git a/VoldNativeServiceValidation.h b/VoldNativeServiceValidation.h new file mode 100644 index 0000000..d2fc9e0 --- /dev/null +++ b/VoldNativeServiceValidation.h @@ -0,0 +1,37 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +#include + +#include +#include + +namespace android::vold { + +binder::Status Ok(); +binder::Status Exception(uint32_t code, const std::string& msg); + +binder::Status CheckPermission(const char* permission); +binder::Status CheckUidOrRoot(uid_t expectedUid); +binder::Status CheckArgumentId(const std::string& id); +binder::Status CheckArgumentPath(const std::string& path); +binder::Status CheckArgumentHex(const std::string& hex); + +} // namespace android::vold diff --git a/tests/Android.bp b/tests/Android.bp index 4731d0a..b90de3a 100644 --- a/tests/Android.bp +++ b/tests/Android.bp @@ -7,7 +7,9 @@ cc_test { srcs: [ "Utils_test.cpp", + "VoldNativeServiceValidation_test.cpp", "cryptfs_test.cpp", ], static_libs: ["libvold"], + shared_libs: ["libbinder"] } diff --git a/tests/VoldNativeServiceValidation_test.cpp b/tests/VoldNativeServiceValidation_test.cpp new file mode 100644 index 0000000..0f87937 --- /dev/null +++ b/tests/VoldNativeServiceValidation_test.cpp @@ -0,0 +1,51 @@ +/* + * Copyright (C) 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "../VoldNativeServiceValidation.h" + +#include + +#include + +using namespace std::literals; + +namespace android::vold { + +class VoldServiceValidationTest : public testing::Test {}; + +TEST_F(VoldServiceValidationTest, CheckArgumentPathTest) { + EXPECT_TRUE(CheckArgumentPath("/").isOk()); + EXPECT_TRUE(CheckArgumentPath("/1/2").isOk()); + EXPECT_TRUE(CheckArgumentPath("/1/2/").isOk()); + EXPECT_TRUE(CheckArgumentPath("/1/2/./3").isOk()); + EXPECT_TRUE(CheckArgumentPath("/1/2/./3/.").isOk()); + EXPECT_TRUE(CheckArgumentPath( + "/very long path with some/ spaces and quite/ uncommon names /in\1 it/") + .isOk()); + + EXPECT_FALSE(CheckArgumentPath("").isOk()); + EXPECT_FALSE(CheckArgumentPath("relative/path").isOk()); + EXPECT_FALSE(CheckArgumentPath("../data/..").isOk()); + EXPECT_FALSE(CheckArgumentPath("/../data/..").isOk()); + EXPECT_FALSE(CheckArgumentPath("/data/../system").isOk()); + EXPECT_FALSE(CheckArgumentPath("/data/..trick/../system").isOk()); + EXPECT_FALSE(CheckArgumentPath("/data/..").isOk()); + EXPECT_FALSE(CheckArgumentPath("/data/././../apex").isOk()); + EXPECT_FALSE(CheckArgumentPath(std::string("/data/strange\0one"sv)).isOk()); + EXPECT_FALSE(CheckArgumentPath(std::string("/data/strange\ntwo"sv)).isOk()); +} + +} // namespace android::vold