Only open a path that is a regular file
... and return a fixed errno before the file path is authenticated.
To avoid a malicious client blocking installd with a path to FIFO,
check the file type before opening it. A corresponding test isn't added
because it's hard to test that the FIFO has never been opened by path.
Bug: 320682896
Bug: 319284987
Test: atest FsVerityTest installd_service_test
Change-Id: Icd30cca809a45447444842ec277b2ac536f26d88
diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp
index 71a8740..9399c73 100644
--- a/cmds/installd/InstalldNativeService.cpp
+++ b/cmds/installd/InstalldNativeService.cpp
@@ -4029,24 +4029,37 @@
return exception(binder::Status::EX_ILLEGAL_ARGUMENT, "Received a null auth token");
}
- // Authenticate to check the targeting file is the same inode as the authFd.
+ // Authenticate to check the targeting file is the same inode as the authFd. With O_PATH, we
+ // prevent a malicious client from blocking installd by providing a path to FIFO. After the
+ // authentication, the actual open is safe.
sp<IBinder> authTokenBinder = IInterface::asBinder(authToken)->localBinder();
if (authTokenBinder == nullptr) {
return exception(binder::Status::EX_SECURITY, "Received a non-local auth token");
}
- auto authTokenInstance = sp<FsveritySetupAuthToken>::cast(authTokenBinder);
- unique_fd rfd(open(filePath.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW));
- struct stat stFromPath;
- if (fstat(rfd.get(), &stFromPath) < 0) {
- *_aidl_return = errno;
+ unique_fd pathFd(open(filePath.c_str(), O_RDONLY | O_CLOEXEC | O_NOFOLLOW | O_PATH));
+ // Returns a constant errno to avoid one app probing file existence of the others, before the
+ // authentication is done.
+ const int kFixedErrno = EPERM;
+ if (pathFd.get() < 0) {
+ PLOG(DEBUG) << "Failed to open the path";
+ *_aidl_return = kFixedErrno;
return ok();
}
+ std::string procFdPath(StringPrintf("/proc/self/fd/%d", pathFd.get()));
+ struct stat stFromPath;
+ if (stat(procFdPath.c_str(), &stFromPath) < 0) {
+ PLOG(DEBUG) << "Failed to stat proc fd " << pathFd.get() << " -> " << filePath;
+ *_aidl_return = kFixedErrno;
+ return ok();
+ }
+ auto authTokenInstance = sp<FsveritySetupAuthToken>::cast(authTokenBinder);
if (!authTokenInstance->isSameStat(stFromPath)) {
LOG(DEBUG) << "FD authentication failed";
- *_aidl_return = EPERM;
+ *_aidl_return = kFixedErrno;
return ok();
}
+ unique_fd rfd(open(procFdPath.c_str(), O_RDONLY | O_CLOEXEC));
fsverity_enable_arg arg = {};
arg.version = 1;
arg.hash_algorithm = FS_VERITY_HASH_ALG_SHA256;
diff --git a/cmds/installd/tests/installd_service_test.cpp b/cmds/installd/tests/installd_service_test.cpp
index f2b578a..023491f 100644
--- a/cmds/installd/tests/installd_service_test.cpp
+++ b/cmds/installd/tests/installd_service_test.cpp
@@ -194,6 +194,12 @@
});
}
+static void unlink_path(const std::string& path) {
+ if (unlink(path.c_str()) < 0) {
+ PLOG(DEBUG) << "Failed to unlink " + path;
+ }
+}
+
class ServiceTest : public testing::Test {
protected:
InstalldNativeService* service;
@@ -555,7 +561,7 @@
TEST_F(FsverityTest, enableFsverity) {
const std::string path = kTestPath + "/foo";
create_with_content(path, kTestAppUid, kTestAppUid, 0600, "content");
- UniqueFile raii(/*fd=*/-1, path, [](const std::string& path) { unlink(path.c_str()); });
+ UniqueFile raii(/*fd=*/-1, path, &unlink_path);
// Expect to fs-verity setup to succeed
sp<IFsveritySetupAuthToken> authToken;
@@ -573,7 +579,7 @@
TEST_F(FsverityTest, enableFsverity_nullAuthToken) {
const std::string path = kTestPath + "/foo";
create_with_content(path, kTestAppUid, kTestAppUid, 0600, "content");
- UniqueFile raii(/*fd=*/-1, path, [](const std::string& path) { unlink(path.c_str()); });
+ UniqueFile raii(/*fd=*/-1, path, &unlink_path);
// Verity null auth token fails
sp<IFsveritySetupAuthToken> authToken;
@@ -586,7 +592,7 @@
TEST_F(FsverityTest, enableFsverity_differentFile) {
const std::string path = kTestPath + "/foo";
create_with_content(path, kTestAppUid, kTestAppUid, 0600, "content");
- UniqueFile raii(/*fd=*/-1, path, [](const std::string& path) { unlink(path.c_str()); });
+ UniqueFile raii(/*fd=*/-1, path, &unlink_path);
// Expect to fs-verity setup to succeed
sp<IFsveritySetupAuthToken> authToken;
@@ -597,17 +603,36 @@
// Verity auth token does not work for a different file
const std::string anotherPath = kTestPath + "/bar";
ASSERT_TRUE(android::base::WriteStringToFile("content", anotherPath));
- UniqueFile raii2(/*fd=*/-1, anotherPath, [](const std::string& path) { unlink(path.c_str()); });
+ UniqueFile raii2(/*fd=*/-1, anotherPath, &unlink_path);
int32_t errno_local;
status = service->enableFsverity(authToken, anotherPath, "fake.package.name", &errno_local);
EXPECT_TRUE(status.isOk());
EXPECT_NE(errno_local, 0);
}
+TEST_F(FsverityTest, enableFsverity_errnoBeforeAuthenticated) {
+ const std::string path = kTestPath + "/foo";
+ create_with_content(path, kTestAppUid, kTestAppUid, 0600, "content");
+ UniqueFile raii(/*fd=*/-1, path, &unlink_path);
+
+ // Expect to fs-verity setup to succeed
+ sp<IFsveritySetupAuthToken> authToken;
+ binder::Status status = createFsveritySetupAuthToken(path, O_RDWR, &authToken);
+ EXPECT_TRUE(status.isOk());
+ EXPECT_TRUE(authToken != nullptr);
+
+ // Verity errno before the fd authentication is constant (EPERM)
+ int32_t errno_local;
+ status = service->enableFsverity(authToken, path + "-non-exist", "fake.package.name",
+ &errno_local);
+ EXPECT_TRUE(status.isOk());
+ EXPECT_EQ(errno_local, EPERM);
+}
+
TEST_F(FsverityTest, createFsveritySetupAuthToken_ReadonlyFdDoesNotAuthenticate) {
const std::string path = kTestPath + "/foo";
create_with_content(path, kTestAppUid, kTestAppUid, 0600, "content");
- UniqueFile raii(/*fd=*/-1, path, [](const std::string& path) { unlink(path.c_str()); });
+ UniqueFile raii(/*fd=*/-1, path, &unlink_path);
// Expect the fs-verity setup to fail
sp<IFsveritySetupAuthToken> authToken;
@@ -619,7 +644,7 @@
const std::string path = kTestPath + "/foo";
// Simulate world-writable file owned by another app
create_with_content(path, kTestAppUid + 1, kTestAppUid + 1, 0666, "content");
- UniqueFile raii(/*fd=*/-1, path, [](const std::string& path) { unlink(path.c_str()); });
+ UniqueFile raii(/*fd=*/-1, path, &unlink_path);
// Expect the fs-verity setup to fail
sp<IFsveritySetupAuthToken> authToken;