Validate output artifacts' file permission.

Bug: 249984283
Test: m test-art-host-gtest-art_artd_tests
Test: atest ArtGtestsTargetChroot:ArtdTest
Ignore-AOSP-First: ART Services.
Change-Id: I7f46d681969859b143b5c4f1835120a41e455135

3 files changed, 193 insertions, 19 deletions

diff --git a/artd/artd.cc b/artd/artd.cc
index 15ef58be87..459f8d4ce8 100644
--- a/artd/artd.cc
+++ b/artd/artd.cc
@@ -761,6 +761,34 @@ ndk::ScopedAStatus Artd::dexopt(
   FdLogger fd_logger;
 
   const FsPermission& fs_permission = in_outputArtifacts.permissionSettings.fileFsPermission;
+
+  std::unique_ptr<File> dex_file = OR_RETURN_NON_FATAL(OpenFileForReading(in_dexFile));
+  args.Add("--zip-fd=%d", dex_file->Fd()).Add("--zip-location=%s", in_dexFile);
+  fd_logger.Add(*dex_file);
+  struct stat dex_st = OR_RETURN_NON_FATAL(Fstat(*dex_file));
+  if ((dex_st.st_mode & S_IROTH) == 0) {
+    if (fs_permission.isOtherReadable) {
+      return NonFatal(
+          "Outputs cannot be other-readable because the dex file '{}' is not other-readable"_format(
+              dex_file->GetPath()));
+    }
+    // Negative numbers mean no `chown`. 0 means root.
+    // Note: this check is more strict than it needs to be. For example, it doesn't allow the
+    // outputs to belong to a group that is a subset of the dex file's group. This is for
+    // simplicity, and it's okay as we don't have to handle such complicated cases in practice.
+    if ((fs_permission.uid > 0 && static_cast<uid_t>(fs_permission.uid) != dex_st.st_uid) ||
+        (fs_permission.gid > 0 && static_cast<gid_t>(fs_permission.gid) != dex_st.st_uid &&
+         static_cast<gid_t>(fs_permission.gid) != dex_st.st_gid)) {
+      return NonFatal(
+          "Outputs' owner doesn't match the dex file '{}' (outputs: {}:{}, dex file: {}:{})"_format(
+              dex_file->GetPath(),
+              fs_permission.uid,
+              fs_permission.gid,
+              dex_st.st_uid,
+              dex_st.st_gid));
+    }
+  }
+
   std::unique_ptr<NewFile> oat_file = OR_RETURN_NON_FATAL(NewFile::Create(oat_path, fs_permission));
   args.Add("--oat-fd=%d", oat_file->Fd()).Add("--oat-location=%s", oat_path);
   fd_logger.Add(*oat_file);
@@ -792,10 +820,6 @@ ndk::ScopedAStatus Artd::dexopt(
     fd_logger.Add(*swap_file);
   }
 
-  std::unique_ptr<File> dex_file = OR_RETURN_NON_FATAL(OpenFileForReading(in_dexFile));
-  args.Add("--zip-fd=%d", dex_file->Fd()).Add("--zip-location=%s", in_dexFile);
-  fd_logger.Add(*dex_file);
-
   std::vector<std::unique_ptr<File>> context_files;
   if (context != nullptr) {
     std::vector<std::string> flattened_context = context->FlattenDexPaths();
@@ -835,6 +859,13 @@ ndk::ScopedAStatus Artd::dexopt(
     profile_file = OR_RETURN_NON_FATAL(OpenFileForReading(profile_path.value()));
     args.Add("--profile-file-fd=%d", profile_file->Fd());
     fd_logger.Add(*profile_file);
+    struct stat profile_st = OR_RETURN_NON_FATAL(Fstat(*profile_file));
+    if (fs_permission.isOtherReadable && (profile_st.st_mode & S_IROTH) == 0) {
+      return NonFatal(
+          "Outputs cannot be other-readable because the profile '{}' is not other-readable"_format(
+              profile_file->GetPath()));
+    }
+    // TODO(b/260228411): Check uid and gid.
   }
 
   AddBootImageFlags(args);
@@ -1131,10 +1162,10 @@ void Artd::AddPerfConfigFlags(PriorityClass priority_class, /*out*/ CmdlineBuild
              "--compile-individually");
 }
 
-android::base::Result<int> Artd::ExecAndReturnCode(const std::vector<std::string>& args,
-                                                   int timeout_sec,
-                                                   const ExecCallbacks& callbacks,
-                                                   ProcessStat* stat) const {
+Result<int> Artd::ExecAndReturnCode(const std::vector<std::string>& args,
+                                    int timeout_sec,
+                                    const ExecCallbacks& callbacks,
+                                    ProcessStat* stat) const {
   std::string error_msg;
   ExecResult result =
       exec_utils_->ExecAndReturnResult(args, timeout_sec, callbacks, stat, &error_msg);
@@ -1144,5 +1175,13 @@ android::base::Result<int> Artd::ExecAndReturnCode(const std::vector<std::string
   return result.exit_code;
 }
 
+Result<struct stat> Artd::Fstat(const File& file) const {
+  struct stat st;
+  if (fstat_(file.Fd(), &st) != 0) {
+    return Errorf("Unable to fstat file '{}'", file.GetPath());
+  }
+  return st;
+}
+
 }  // namespace artd
 }  // namespace art
diff --git a/artd/artd.h b/artd/artd.h
index 0801317fe8..484c281c5d 100644
--- a/artd/artd.h
+++ b/artd/artd.h
@@ -17,6 +17,7 @@
 #ifndef ART_ARTD_ARTD_H_
 #define ART_ARTD_ARTD_H_
 
+#include <sys/stat.h>
 #include <sys/types.h>
 
 #include <csignal>
@@ -36,6 +37,7 @@
 #include "android-base/result.h"
 #include "android-base/thread_annotations.h"
 #include "android/binder_auto_utils.h"
+#include "base/os.h"
 #include "exec_utils.h"
 #include "oat_file_assistant_context.h"
 #include "tools/cmdline_builder.h"
@@ -70,8 +72,12 @@ class Artd : public aidl::com::android::server::art::BnArtd {
   explicit Artd(std::unique_ptr<art::tools::SystemProperties> props =
                     std::make_unique<art::tools::SystemProperties>(),
                 std::unique_ptr<ExecUtils> exec_utils = std::make_unique<ExecUtils>(),
-                std::function<int(pid_t, int)> kill_func = kill)
-      : props_(std::move(props)), exec_utils_(std::move(exec_utils)), kill_(std::move(kill_func)) {}
+                std::function<int(pid_t, int)> kill_func = kill,
+                std::function<int(int, struct stat*)> fstat_func = fstat)
+      : props_(std::move(props)),
+        exec_utils_(std::move(exec_utils)),
+        kill_(std::move(kill_func)),
+        fstat_(std::move(fstat_func)) {}
 
   ndk::ScopedAStatus isAlive(bool* _aidl_return) override;
 
@@ -198,6 +204,8 @@ class Artd : public aidl::com::android::server::art::BnArtd {
   void AddPerfConfigFlags(aidl::com::android::server::art::PriorityClass priority_class,
                           /*out*/ art::tools::CmdlineBuilder& args);
 
+  android::base::Result<struct stat> Fstat(const art::File& file) const;
+
   std::mutex cache_mu_;
   std::optional<std::vector<std::string>> cached_boot_image_locations_ GUARDED_BY(cache_mu_);
   std::optional<std::vector<std::string>> cached_boot_class_path_ GUARDED_BY(cache_mu_);
@@ -211,6 +219,7 @@ class Artd : public aidl::com::android::server::art::BnArtd {
   const std::unique_ptr<art::tools::SystemProperties> props_;
   const std::unique_ptr<ExecUtils> exec_utils_;
   const std::function<int(pid_t, int)> kill_;
+  const std::function<int(int, struct stat*)> fstat_;
 };
 
 }  // namespace artd
diff --git a/artd/artd_test.cc b/artd/artd_test.cc
index e17f71fbd6..44250e5845 100644
--- a/artd/artd_test.cc
+++ b/artd/artd_test.cc
@@ -17,6 +17,7 @@
 #include "artd.h"
 
 #include <fcntl.h>
+#include <sys/stat.h>
 #include <sys/types.h>
 #include <unistd.h>
 
@@ -82,6 +83,7 @@ using ::android::base::WriteStringToFile;
 using ::testing::_;
 using ::testing::AllOf;
 using ::testing::AnyNumber;
+using ::testing::AnyOf;
 using ::testing::Contains;
 using ::testing::ContainsRegex;
 using ::testing::DoAll;
@@ -92,6 +94,7 @@ using ::testing::IsEmpty;
 using ::testing::Matcher;
 using ::testing::MockFunction;
 using ::testing::Not;
+using ::testing::Property;
 using ::testing::ResultOf;
 using ::testing::Return;
 using ::testing::SetArgPointee;
@@ -116,6 +119,12 @@ void CheckContent(const std::string& path, const std::string& expected_content)
   EXPECT_EQ(actual_content, expected_content);
 }
 
+void CheckOtherReadable(const std::string& path, bool expected_value) {
+  EXPECT_EQ((std::filesystem::status(path).permissions() & std::filesystem::perms::others_read) !=
+                std::filesystem::perms::none,
+            expected_value);
+}
+
 void WriteToFdFlagImpl(const std::vector<std::string>& args,
                        const std::string& flag,
                        const std::string& content,
@@ -168,11 +177,7 @@ MATCHER_P2(ListFlag, flag, matcher, "") {
 
 // Matches an FD of a file whose path matches `matcher`.
 MATCHER_P(FdOf, matcher, "") {
-  int fd;
-  if (!ParseInt(arg, &fd)) {
-    return false;
-  }
-  std::string proc_path = "/proc/self/fd/{}"_format(fd);
+  std::string proc_path = "/proc/self/fd/{}"_format(arg);
   char path[PATH_MAX];
   ssize_t len = readlink(proc_path.c_str(), path, sizeof(path));
   if (len < 0) {
@@ -245,13 +250,17 @@ class ArtdTest : public CommonArtTest {
     EXPECT_CALL(*mock_props_, GetProperty).Times(AnyNumber()).WillRepeatedly(Return(""));
     auto mock_exec_utils = std::make_unique<MockExecUtils>();
     mock_exec_utils_ = mock_exec_utils.get();
-    artd_ = ndk::SharedRefBase::make<Artd>(
-        std::move(mock_props), std::move(mock_exec_utils), mock_kill_.AsStdFunction());
+    artd_ = ndk::SharedRefBase::make<Artd>(std::move(mock_props),
+                                           std::move(mock_exec_utils),
+                                           mock_kill_.AsStdFunction(),
+                                           mock_fstat_.AsStdFunction());
     scratch_dir_ = std::make_unique<ScratchDir>();
     scratch_path_ = scratch_dir_->GetPath();
     // Remove the trailing '/';
     scratch_path_.resize(scratch_path_.length() - 1);
 
+    ON_CALL(mock_fstat_, Call).WillByDefault(fstat);
+
     // Use an arbitrary existing directory as ART root.
     art_root_ = scratch_path_ + "/com.android.art";
     std::filesystem::create_directories(art_root_);
@@ -313,6 +322,14 @@ class ArtdTest : public CommonArtTest {
   void RunDexopt(binder_exception_t expected_status = EX_NONE,
                  Matcher<DexoptResult> aidl_return_matcher = Field(&DexoptResult::cancelled, false),
                  std::shared_ptr<IArtdCancellationSignal> cancellation_signal = nullptr) {
+    RunDexopt(Property(&ndk::ScopedAStatus::getExceptionCode, expected_status),
+              std::move(aidl_return_matcher),
+              cancellation_signal);
+  }
+
+  void RunDexopt(Matcher<ndk::ScopedAStatus> status_matcher,
+                 Matcher<DexoptResult> aidl_return_matcher = Field(&DexoptResult::cancelled, false),
+                 std::shared_ptr<IArtdCancellationSignal> cancellation_signal = nullptr) {
     InitFilesBeforeDexopt();
     if (cancellation_signal == nullptr) {
       ASSERT_TRUE(artd_->createCancellationSignal(&cancellation_signal).isOk());
@@ -330,7 +347,7 @@ class ArtdTest : public CommonArtTest {
                                               dexopt_options_,
                                               cancellation_signal,
                                               &aidl_return);
-    ASSERT_EQ(status.getExceptionCode(), expected_status) << status.getMessage();
+    ASSERT_THAT(status, std::move(status_matcher)) << status.getMessage();
     if (status.isOk()) {
       ASSERT_THAT(aidl_return, std::move(aidl_return_matcher));
     }
@@ -353,6 +370,7 @@ class ArtdTest : public CommonArtTest {
   MockSystemProperties* mock_props_;
   MockExecUtils* mock_exec_utils_;
   MockFunction<int(pid_t, int)> mock_kill_;
+  MockFunction<int(int, struct stat*)> mock_fstat_;
 
   std::string dex_file_;
   std::string isa_;
@@ -367,11 +385,17 @@ class ArtdTest : public CommonArtTest {
   PriorityClass priority_class_ = PriorityClass::BACKGROUND;
   DexoptOptions dexopt_options_;
   std::optional<ProfilePath> profile_path_;
+  bool dex_file_other_readable_ = true;
+  bool profile_other_readable_ = true;
 
  private:
   void InitFilesBeforeDexopt() {
     // Required files.
     CreateFile(dex_file_);
+    std::filesystem::permissions(dex_file_,
+                                 std::filesystem::perms::others_read,
+                                 dex_file_other_readable_ ? std::filesystem::perm_options::add :
+                                                            std::filesystem::perm_options::remove);
 
     // Optional files.
     if (vdex_path_.has_value()) {
@@ -381,7 +405,12 @@ class ArtdTest : public CommonArtTest {
       CreateFile(OR_FATAL(BuildDexMetadataPath(dm_path_.value())));
     }
     if (profile_path_.has_value()) {
-      CreateFile(OR_FATAL(BuildProfileOrDmPath(profile_path_.value())));
+      std::string path = OR_FATAL(BuildProfileOrDmPath(profile_path_.value()));
+      CreateFile(path);
+      std::filesystem::permissions(path,
+                                   std::filesystem::perms::others_read,
+                                   profile_other_readable_ ? std::filesystem::perm_options::add :
+                                                             std::filesystem::perm_options::remove);
     }
 
     // Files to be replaced.
@@ -525,6 +554,8 @@ TEST_F(ArtdTest, dexopt) {
 
   CheckContent(scratch_path_ + "/a/oat/arm64/b.odex", "oat");
   CheckContent(scratch_path_ + "/a/oat/arm64/b.vdex", "vdex");
+  CheckOtherReadable(scratch_path_ + "/a/oat/arm64/b.odex", true);
+  CheckOtherReadable(scratch_path_ + "/a/oat/arm64/b.vdex", true);
 }
 
 TEST_F(ArtdTest, dexoptClassLoaderContext) {
@@ -1021,6 +1052,101 @@ TEST_F(ArtdTest, dexoptCancelledAfterDex2oat) {
   EXPECT_FALSE(std::filesystem::exists(scratch_path_ + "/a/oat/arm64/b.art"));
 }
 
+TEST_F(ArtdTest, dexoptDexFileNotOtherReadable) {
+  dex_file_other_readable_ = false;
+  EXPECT_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).Times(0);
+  RunDexopt(AllOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_SERVICE_SPECIFIC),
+                  Property(&ndk::ScopedAStatus::getMessage,
+                           HasSubstr("Outputs cannot be other-readable because the dex file"))));
+}
+
+TEST_F(ArtdTest, dexoptProfileNotOtherReadable) {
+  profile_other_readable_ = false;
+  EXPECT_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).Times(0);
+  RunDexopt(AllOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_SERVICE_SPECIFIC),
+                  Property(&ndk::ScopedAStatus::getMessage,
+                           HasSubstr("Outputs cannot be other-readable because the profile"))));
+}
+
+TEST_F(ArtdTest, dexoptOutputNotOtherReadable) {
+  output_artifacts_.permissionSettings.fileFsPermission.isOtherReadable = false;
+  dex_file_other_readable_ = false;
+  profile_other_readable_ = false;
+  EXPECT_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).WillOnce(Return(0));
+  RunDexopt();
+  CheckOtherReadable(scratch_path_ + "/a/oat/arm64/b.odex", false);
+  CheckOtherReadable(scratch_path_ + "/a/oat/arm64/b.vdex", false);
+}
+
+TEST_F(ArtdTest, dexoptUidMismatch) {
+  output_artifacts_.permissionSettings.fileFsPermission.uid = 12345;
+  output_artifacts_.permissionSettings.fileFsPermission.isOtherReadable = false;
+  dex_file_other_readable_ = false;
+  EXPECT_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).Times(0);
+  RunDexopt(AllOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_SERVICE_SPECIFIC),
+                  Property(&ndk::ScopedAStatus::getMessage,
+                           HasSubstr("Outputs' owner doesn't match the dex file"))));
+}
+
+TEST_F(ArtdTest, dexoptGidMismatch) {
+  output_artifacts_.permissionSettings.fileFsPermission.gid = 12345;
+  output_artifacts_.permissionSettings.fileFsPermission.isOtherReadable = false;
+  dex_file_other_readable_ = false;
+  EXPECT_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).Times(0);
+  RunDexopt(AllOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_SERVICE_SPECIFIC),
+                  Property(&ndk::ScopedAStatus::getMessage,
+                           HasSubstr("Outputs' owner doesn't match the dex file"))));
+}
+
+TEST_F(ArtdTest, dexoptGidMatchesUid) {
+  output_artifacts_.permissionSettings.fileFsPermission = {
+      .uid = 123, .gid = 123, .isOtherReadable = false};
+  EXPECT_CALL(mock_fstat_, Call(_, _)).WillRepeatedly(fstat);  // For profile.
+  EXPECT_CALL(mock_fstat_, Call(FdOf(dex_file_), _))
+      .WillOnce(DoAll(SetArgPointee<1>((struct stat){
+                          .st_mode = S_IRUSR | S_IRGRP, .st_uid = 123, .st_gid = 456}),
+                      Return(0)));
+  ON_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).WillByDefault(Return(0));
+  // It's okay to fail on chown. This happens when the test is not run as root.
+  RunDexopt(AnyOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_NONE),
+                  AllOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_SERVICE_SPECIFIC),
+                        Property(&ndk::ScopedAStatus::getMessage, HasSubstr("Failed to chown")))));
+}
+
+TEST_F(ArtdTest, dexoptGidMatchesGid) {
+  output_artifacts_.permissionSettings.fileFsPermission = {
+      .uid = 123, .gid = 456, .isOtherReadable = false};
+  EXPECT_CALL(mock_fstat_, Call(_, _)).WillRepeatedly(fstat);  // For profile.
+  EXPECT_CALL(mock_fstat_, Call(FdOf(dex_file_), _))
+      .WillOnce(DoAll(SetArgPointee<1>((struct stat){
+                          .st_mode = S_IRUSR | S_IRGRP, .st_uid = 123, .st_gid = 456}),
+                      Return(0)));
+  ON_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).WillByDefault(Return(0));
+  // It's okay to fail on chown. This happens when the test is not run as root.
+  RunDexopt(AnyOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_NONE),
+                  AllOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_SERVICE_SPECIFIC),
+                        Property(&ndk::ScopedAStatus::getMessage, HasSubstr("Failed to chown")))));
+}
+
+TEST_F(ArtdTest, dexoptUidGidChangeOk) {
+  // The dex file is other-readable, so we don't check uid and gid.
+  output_artifacts_.permissionSettings.fileFsPermission = {
+      .uid = 12345, .gid = 12345, .isOtherReadable = false};
+  ON_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).WillByDefault(Return(0));
+  // It's okay to fail on chown. This happens when the test is not run as root.
+  RunDexopt(AnyOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_NONE),
+                  AllOf(Property(&ndk::ScopedAStatus::getExceptionCode, EX_SERVICE_SPECIFIC),
+                        Property(&ndk::ScopedAStatus::getMessage, HasSubstr("Failed to chown")))));
+}
+
+TEST_F(ArtdTest, dexoptNoUidGidChange) {
+  output_artifacts_.permissionSettings.fileFsPermission = {
+      .uid = -1, .gid = -1, .isOtherReadable = false};
+  dex_file_other_readable_ = false;
+  EXPECT_CALL(*mock_exec_utils_, DoExecAndReturnCode(_, _, _)).WillOnce(Return(0));
+  RunDexopt();
+}
+
 TEST_F(ArtdTest, isProfileUsable) {
   std::string profile_file = OR_FATAL(BuildProfileOrDmPath(profile_path_.value()));
   CreateFile(profile_file);