Merge "Only open a path that is a regular file" into main am: eee1cf5f26

Original change: https://android-review.googlesource.com/c/platform/frameworks/native/+/2911497

Change-Id: Ie27618e5481d4fd860dbafc05e1a6009125bcdef
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

2 files changed, 50 insertions, 12 deletions

diff --git a/cmds/installd/InstalldNativeService.cpp b/cmds/installd/InstalldNativeService.cpp
index 7478f291dd..4486bd6d3e 100644
--- a/cmds/installd/InstalldNativeService.cpp
+++ b/cmds/installd/InstalldNativeService.cpp
@@ -4017,24 +4017,37 @@ binder::Status InstalldNativeService::enableFsverity(const sp<IFsveritySetupAuth
         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));
+    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 (fstat(rfd.get(), &stFromPath) < 0) {
-        *_aidl_return = errno;
+    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 f2b578a8d2..023491fc10 100644
--- a/cmds/installd/tests/installd_service_test.cpp
+++ b/cmds/installd/tests/installd_service_test.cpp
@@ -194,6 +194,12 @@ static bool exists_renamed_deleted_dir(const std::string& rootDirectory) {
     });
 }
 
+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 @@ protected:
 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) {
 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_nullAuthToken) {
 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 @@ TEST_F(FsverityTest, enableFsverity_differentFile) {
     // 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 @@ TEST_F(FsverityTest, createFsveritySetupAuthToken_UnownedFile) {
     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;