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
diff --git a/artd/artd.cc b/artd/artd.cc
index 15ef58b..459f8d4 100644
--- a/artd/artd.cc
+++ b/artd/artd.cc
@@ -761,6 +761,34 @@
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 @@
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 @@
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 @@
"--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 @@
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 0801317..484c281 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 @@
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 @@
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 @@
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 e17f71f..44250e5 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 ::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::Matcher;
using ::testing::MockFunction;
using ::testing::Not;
+using ::testing::Property;
using ::testing::ResultOf;
using ::testing::Return;
using ::testing::SetArgPointee;
@@ -116,6 +119,12 @@
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 @@
// 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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);