Make chaps database permissions more robust.
We have seen instances where chaps database permissions / ownership are not
right and there is no workaround short of removing the account. Although the
root cause is not known, this CL checks all permissions and ownership before
loading a token and makes any necessary corrections, logging warnings if
corrections are necessary. If the permissions or ownership cannot be fixed,
the token will not be loaded.
BUG=chromium:268974
TEST=unit, manual
Change-Id: Id2936753339da51ae905d044863aafa3af4ab083
Reviewed-on: https://chromium-review.googlesource.com/169750
Reviewed-by: Darren Krahn <dkrahn@chromium.org>
Commit-Queue: Darren Krahn <dkrahn@chromium.org>
Tested-by: Darren Krahn <dkrahn@chromium.org>
diff --git a/mock_platform.h b/mock_platform.h
index 56c580a..2ccff48 100644
--- a/mock_platform.h
+++ b/mock_platform.h
@@ -23,6 +23,8 @@
MockFileEnumerator() {
ON_CALL(*this, Next())
.WillByDefault(Invoke(this, &MockFileEnumerator::MockNext));
+ ON_CALL(*this, GetFindInfo(_))
+ .WillByDefault(Invoke(this, &MockFileEnumerator::MockGetFindInfo));
}
virtual ~MockFileEnumerator() {}
@@ -30,6 +32,7 @@
MOCK_METHOD1(GetFindInfo, void(FindInfo* info));
std::vector<std::string> entries_;
+ std::vector<FindInfo> find_info_;
protected:
virtual std::string MockNext() {
if (entries_.empty())
@@ -38,6 +41,12 @@
entries_.erase(entries_.begin(), entries_.begin()+1);
return entry;
}
+ virtual void MockGetFindInfo(FindInfo* info) {
+ if (find_info_.empty())
+ return;
+ *info = find_info_.at(0);
+ find_info_.erase(find_info_.begin(), find_info_.begin()+1);
+ }
};
diff --git a/mount.cc b/mount.cc
index d618abc..04794c4 100644
--- a/mount.cc
+++ b/mount.cc
@@ -1245,92 +1245,74 @@
}
bool Mount::CheckChapsDirectory(const std::string& dir,
- const std::string& legacy_dir,
- bool* permissions_status) {
- // 0750: u + rwx, g + rx
- const mode_t chaps_permissions = S_IRWXU | S_IRGRP | S_IXGRP;
- const mode_t permissions_mask = S_IRWXU | S_IRWXG | S_IRWXO;
+ const std::string& legacy_dir) {
+ const Platform::Permissions kChapsDirPermissions = {
+ chaps_user_, // chaps
+ default_access_group_, // chronos-access
+ S_IRWXU | S_IRGRP | S_IXGRP // 0750
+ };
+ const Platform::Permissions kChapsFilePermissions = {
+ chaps_user_, // chaps
+ default_access_group_, // chronos-access
+ S_IRUSR | S_IWUSR | S_IRGRP // 0640
+ };
+ const Platform::Permissions kChapsSaltPermissions = {
+ 0, // root
+ 0, // root
+ S_IRUSR | S_IWUSR // 0600
+ };
- DCHECK(permissions_status);
- *permissions_status = true;
// If the Chaps database directory does not exist, create it.
if (!platform_->DirectoryExists(dir)) {
if (platform_->DirectoryExists(legacy_dir)) {
LOG(INFO) << "Moving chaps directory from " << legacy_dir << " to "
<< dir;
if (!platform_->CopyWithPermissions(legacy_dir, dir)) {
- *permissions_status = false;
return false;
}
if (!platform_->DeleteFile(legacy_dir, true)) {
PLOG(WARNING) << "Failed to clean up " << legacy_dir;
- *permissions_status = false;
return false;
}
} else {
if (!platform_->CreateDirectory(dir)) {
LOG(ERROR) << "Failed to create " << dir;
- *permissions_status = false;
return false;
}
- if (!platform_->SetOwnership(dir, chaps_user_, default_access_group_)) {
+ if (!platform_->SetOwnership(dir,
+ kChapsDirPermissions.user,
+ kChapsDirPermissions.group)) {
LOG(ERROR) << "Couldn't set file ownership for " << dir;
- *permissions_status = false;
return false;
}
- if (!platform_->SetPermissions(dir, chaps_permissions)) {
+ if (!platform_->SetPermissions(dir, kChapsDirPermissions.mode)) {
LOG(ERROR) << "Couldn't set permissions for " << dir;
- *permissions_status = false;
return false;
}
}
return true;
}
// Directory already exists so check permissions and log a warning
- // if not as expected.
- mode_t current_permissions;
- if (!platform_->GetPermissions(dir, ¤t_permissions)) {
- LOG(ERROR) << "Couldn't get permissions for " << dir;
- *permissions_status = false;
+ // if not as expected then attempt to apply correct permissions.
+ std::map<std::string, Platform::Permissions> special_cases;
+ special_cases[dir + "/auth_data_salt"] = kChapsSaltPermissions;
+ if (!platform_->ApplyPermissionsRecursive(dir,
+ kChapsFilePermissions,
+ kChapsDirPermissions,
+ special_cases)) {
+ LOG(ERROR) << "Chaps permissions failure.";
return false;
}
- if ((current_permissions & permissions_mask) != chaps_permissions) {
- LOG(WARNING) << "Chaps directory had incorrect permissions: "
- << StringPrintf("Expected: %o Found: %o",
- chaps_permissions, current_permissions);
- *permissions_status = false;
- }
- uid_t current_user;
- gid_t current_group;
- if (!platform_->GetOwnership(dir, ¤t_user, ¤t_group)) {
- LOG(ERROR) << "Couldn't get ownership for " << dir;
- *permissions_status = false;
- return false;
- }
- if (current_user != chaps_user_) {
- LOG(WARNING) << "Chaps directory had incorrect owner: "
- << StringPrintf("Expected: %u Found: %u",
- chaps_user_, current_user);
- *permissions_status = false;
- }
- if (current_group != default_access_group_) {
- LOG(WARNING) << "Chaps directory had incorrect group: "
- << StringPrintf("Expected: %u Found: %u",
- default_access_group_, current_group);
- *permissions_status = false;
- }
return true;
}
bool Mount::InsertPkcs11Token() {
- bool permissions_status = false;
std::string username = current_user_->username();
FilePath token_dir = homedirs_->GetChapsTokenDir(username);
FilePath legacy_token_dir = homedirs_->GetLegacyChapsTokenDir(username);
if (!CheckChapsDirectory(token_dir.value(),
- legacy_token_dir.value(),
- &permissions_status))
+ legacy_token_dir.value()))
return false;
// We may create a salt file and, if so, we want to restrict access to it.
ScopedUmask scoped_umask(platform_, kDefaultUmask);
@@ -1366,7 +1348,7 @@
LOG(ERROR) << "Failed to load PKCS #11 token.";
}
pkcs11_passkey_.clear_contents();
- return permissions_status;
+ return true;
}
void Mount::RemovePkcs11Token() {
diff --git a/mount.h b/mount.h
index 779b010..2f6fcd8 100644
--- a/mount.h
+++ b/mount.h
@@ -279,6 +279,7 @@
friend class MakeTests;
friend class MountTest;
friend class EphemeralTest;
+ friend class ChapsDirectoryTest;
private:
// A class which scopes a mount point. i.e. The mount point is unmounted on
@@ -503,7 +504,7 @@
// Checks Chaps Directory and makes sure that it has the correct
// permissions, owner uid and gid. If any of these values are
- // incorrect, permissions_check is set to false. If the directory
+ // incorrect, the correct values are set. If the directory
// does not exist, it is created and initialzed with the correct
// values. If the directory or its attributes cannot be checked,
// set or created, a fatal error has occured and the function
@@ -513,10 +514,8 @@
// Parameters
// dir - directory to check
// legacy_dir - legacy directory location
- // permissions_check - set to false if permissions, uid or gid is incorrect
bool CheckChapsDirectory(const std::string& dir,
- const std::string& legacy_dir,
- bool* permissions_check);
+ const std::string& legacy_dir);
// Ensures that the device policy is loaded.
//
@@ -773,14 +772,6 @@
FRIEND_TEST(MountTest, UserActivityTimestampUpdated);
FRIEND_TEST(MountTest, GoodReDecryptTest);
FRIEND_TEST(MountTest, MountCryptohomeNoChange);
- FRIEND_TEST(MountTest, CheckChapsDirectoryCalledWithExistingDirWithBadPerms);
- FRIEND_TEST(MountTest, CheckChapsDirectoryCalledWithExistingDirWithBadUID);
- FRIEND_TEST(MountTest, CheckChapsDirectoryCalledWithExistingDirWithBadGID);
- FRIEND_TEST(MountTest,
- CheckChapsDirectoryCalledWithNonexistingDirWithFatalError);
- FRIEND_TEST(MountTest,
- CheckChapsDirectoryCalledWithExistingDirWithFatalError);
- FRIEND_TEST(MountTest, CheckChapsDirectoryCalledWithExistingDir);
FRIEND_TEST(MountTest, CheckChapsDirectoryMigration);
DISALLOW_COPY_AND_ASSIGN(Mount);
diff --git a/mount_unittest.cc b/mount_unittest.cc
index df7ba4c..8b41847 100644
--- a/mount_unittest.cc
+++ b/mount_unittest.cc
@@ -274,209 +274,176 @@
ASSERT_FALSE(mount->AreValid(up));
}
-TEST_F(MountTest, CheckChapsDirectoryCalledWithExistingDir) {
- scoped_refptr<Mount> mount = new Mount();
- NiceMock<MockPlatform> platform;
- NiceMock<MockTpm> tpm;
- mount->crypto()->set_tpm(&tpm);
- mount->crypto()->set_platform(&platform);
- mount->set_shadow_root(kImageDir);
- mount->set_skel_source(kSkelDir);
- mount->set_use_tpm(false);
- mount->set_platform(&platform);
+// A fixture for testing chaps directory checks.
+class ChapsDirectoryTest : public ::testing::Test {
+ public:
+ ChapsDirectoryTest()
+ : kBaseDir("/base_chaps_dir"),
+ kSaltFile("/base_chaps_dir/auth_data_salt"),
+ kDatabaseDir("/base_chaps_dir/database"),
+ kDatabaseFile("/base_chaps_dir/database/file"),
+ kLegacyDir("/legacy"),
+ kRootUID(0), kRootGID(0), kChapsUID(1), kSharedGID(2),
+ mount_(new Mount()) {
+ mount_->crypto()->set_platform(&platform_);
+ mount_->set_platform(&platform_);
+ mount_->chaps_user_ = kChapsUID;
+ mount_->default_access_group_ = kSharedGID;
+ // By default, set stats to the expected values.
+ InitStat(&base_stat_, 040750, kChapsUID, kSharedGID);
+ InitStat(&salt_stat_, 0600, kRootUID, kRootGID);
+ InitStat(&database_dir_stat_, 040750, kChapsUID, kSharedGID);
+ InitStat(&database_file_stat_, 0640, kChapsUID, kSharedGID);
+ }
- NiceMock<MockHomeDirs> mock_homedirs;
- mount->set_homedirs(&mock_homedirs);
- EXPECT_CALL(mock_homedirs, Init())
- .WillOnce(Return(true));
+ virtual ~ChapsDirectoryTest() {}
- chaps_uid_ = 101010;
- shared_gid_ = 101010;
- EXPECT_TRUE(DoMountInit(mount.get()));
- EXPECT_CALL(platform, DirectoryExists("/fake"))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(platform, GetOwnership("/fake", _, _))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(chaps_uid_),
- SetArgumentPointee<2>(shared_gid_),
- Return(true)));
- bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_TRUE(permissions_status);
+ void SetupFakeChapsDirectory() {
+ // Configure the base directory.
+ EXPECT_CALL(platform_, DirectoryExists(kBaseDir))
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(platform_, Stat(kBaseDir, _))
+ .WillRepeatedly(DoAll(SetArgumentPointee<1>(base_stat_), Return(true)));
+
+ // Configure a fake enumerator.
+ MockFileEnumerator* enumerator = platform_.mock_enumerator();
+ enumerator->entries_.push_back(kBaseDir);
+ enumerator->entries_.push_back(kSaltFile);
+ enumerator->entries_.push_back(kDatabaseDir);
+ enumerator->entries_.push_back(kDatabaseFile);
+ FileEnumerator::FindInfo find_info1 = {base_stat_, kBaseDir};
+ FileEnumerator::FindInfo find_info2 = {salt_stat_, kSaltFile};
+ FileEnumerator::FindInfo find_info3 = {database_dir_stat_, kDatabaseDir};
+ FileEnumerator::FindInfo find_info4 = {database_file_stat_, kDatabaseFile};
+ enumerator->find_info_.push_back(find_info1);
+ enumerator->find_info_.push_back(find_info2);
+ enumerator->find_info_.push_back(find_info3);
+ enumerator->find_info_.push_back(find_info4);
+ }
+
+ bool RunCheck() {
+ return mount_->CheckChapsDirectory(kBaseDir, kLegacyDir);
+ }
+
+ protected:
+ const std::string kBaseDir;
+ const std::string kSaltFile;
+ const std::string kDatabaseDir;
+ const std::string kDatabaseFile;
+ const std::string kLegacyDir;
+ const uid_t kRootUID;
+ const gid_t kRootGID;
+ const uid_t kChapsUID;
+ const gid_t kSharedGID;
+
+ struct stat base_stat_;
+ struct stat salt_stat_;
+ struct stat database_dir_stat_;
+ struct stat database_file_stat_;
+
+ scoped_refptr<Mount> mount_;
+ NiceMock<MockPlatform> platform_;
+
+ private:
+ void InitStat(struct stat* s, mode_t mode, uid_t uid, gid_t gid) {
+ memset(s, 0, sizeof(struct stat));
+ s->st_mode = mode;
+ s->st_uid = uid;
+ s->st_gid = gid;
+ }
+
+ DISALLOW_COPY_AND_ASSIGN(ChapsDirectoryTest);
+};
+
+TEST_F(ChapsDirectoryTest, DirectoryOK) {
+ SetupFakeChapsDirectory();
+ ASSERT_TRUE(RunCheck());
}
-TEST_F(MountTest, CheckChapsDirectoryCalledWithExistingDirWithBadPerms) {
- scoped_refptr<Mount> mount = new Mount();
- NiceMock<MockPlatform> platform;
- mode_t bad_perms = S_IXGRP;
- NiceMock<MockTpm> tpm;
- mount->crypto()->set_tpm(&tpm);
- mount->crypto()->set_platform(&platform);
- mount->set_shadow_root(kImageDir);
- mount->set_skel_source(kSkelDir);
- mount->set_use_tpm(false);
- mount->set_platform(&platform);
-
- NiceMock<MockHomeDirs> mock_homedirs;
- mount->set_homedirs(&mock_homedirs);
- EXPECT_CALL(mock_homedirs, Init())
- .WillOnce(Return(true));
-
- EXPECT_TRUE(DoMountInit(mount.get()));
- EXPECT_CALL(platform, DirectoryExists("/fake"))
+TEST_F(ChapsDirectoryTest, DirectoryDoesNotExist) {
+ // Specify directory does not exist.
+ EXPECT_CALL(platform_, DirectoryExists(kBaseDir))
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(platform_, DirectoryExists(kLegacyDir))
+ .WillRepeatedly(Return(false));
+ // Expect basic setup.
+ EXPECT_CALL(platform_, CreateDirectory(kBaseDir))
.WillRepeatedly(Return(true));
- EXPECT_CALL(platform, GetPermissions("/fake", _))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(bad_perms), Return(true)));
- bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_FALSE(permissions_status);
+ EXPECT_CALL(platform_, SetPermissions(kBaseDir, 0750))
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(platform_, SetOwnership(kBaseDir, kChapsUID, kSharedGID))
+ .WillRepeatedly(Return(true));
+ ASSERT_TRUE(RunCheck());
}
-TEST_F(MountTest, CheckChapsDirectoryCalledWithExistingDirWithBadUID) {
- scoped_refptr<Mount> mount = new Mount();
- NiceMock<MockPlatform> platform;
- NiceMock<MockTpm> tpm;
- mount->crypto()->set_tpm(&tpm);
- mount->crypto()->set_platform(&platform);
- mount->set_shadow_root(kImageDir);
- mount->set_skel_source(kSkelDir);
- mount->set_use_tpm(false);
- mount->set_platform(&platform);
- uid_t bad_uid = 0;
-
- NiceMock<MockHomeDirs> mock_homedirs;
- mount->set_homedirs(&mock_homedirs);
- EXPECT_CALL(mock_homedirs, Init())
- .WillOnce(Return(true));
-
- // Populate the chaps uid and gid.
- EXPECT_TRUE(DoMountInit(mount.get()));
-
- EXPECT_CALL(platform, DirectoryExists(_))
- .WillRepeatedly(Return(true));
- mode_t expected_perms = S_IRWXU | S_IRGRP | S_IXGRP;
- EXPECT_CALL(platform, GetPermissions("/fake", _))
- .WillOnce(DoAll(SetArgumentPointee<1>(expected_perms), Return(true)));
- EXPECT_CALL(platform, GetOwnership(_, _, _))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(bad_uid),
- SetArgumentPointee<2>(shared_gid_),
- Return(true)));
-
- bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_FALSE(permissions_status);
+TEST_F(ChapsDirectoryTest, CreateFailure) {
+ // Specify directory does not exist.
+ EXPECT_CALL(platform_, DirectoryExists(kBaseDir))
+ .WillRepeatedly(Return(false));
+ EXPECT_CALL(platform_, DirectoryExists(kLegacyDir))
+ .WillRepeatedly(Return(false));
+ // Expect basic setup but fail.
+ EXPECT_CALL(platform_, CreateDirectory(kBaseDir))
+ .WillRepeatedly(Return(false));
+ ASSERT_FALSE(RunCheck());
}
-TEST_F(MountTest, CheckChapsDirectoryCalledWithExistingDirWithBadGID) {
- scoped_refptr<Mount> mount = new Mount();
- NiceMock<MockPlatform> platform;
- NiceMock<MockTpm> tpm;
- mount->crypto()->set_tpm(&tpm);
- mount->crypto()->set_platform(&platform);
- mount->set_shadow_root(kImageDir);
- mount->set_skel_source(kSkelDir);
- mount->set_use_tpm(false);
- mount->set_platform(&platform);
-
- NiceMock<MockHomeDirs> mock_homedirs;
- mount->set_homedirs(&mock_homedirs);
- EXPECT_CALL(mock_homedirs, Init())
- .WillOnce(Return(true));
-
- chaps_uid_ = 101010;
- shared_gid_ = 101011;
- gid_t bad_gid = 0;
- EXPECT_TRUE(DoMountInit(mount.get()));
- EXPECT_CALL(platform, DirectoryExists("/fake"))
+TEST_F(ChapsDirectoryTest, FixBadPerms) {
+ // Specify some bad perms.
+ base_stat_.st_mode = 040700;
+ salt_stat_.st_mode = 0640;
+ database_dir_stat_.st_mode = 040755;
+ database_file_stat_.st_mode = 0666;
+ SetupFakeChapsDirectory();
+ // Expect corrections.
+ EXPECT_CALL(platform_, SetPermissions(kBaseDir, 0750))
.WillRepeatedly(Return(true));
- EXPECT_CALL(platform, GetOwnership("/fake", _, _))
- .WillRepeatedly(DoAll(SetArgumentPointee<1>(chaps_uid_),
- SetArgumentPointee<2>(bad_gid),
- Return(true)));
- bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_FALSE(permissions_status);
+ EXPECT_CALL(platform_, SetPermissions(kSaltFile, 0600))
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(platform_, SetPermissions(kDatabaseDir, 0750))
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(platform_, SetPermissions(kDatabaseFile, 0640))
+ .WillRepeatedly(Return(true));
+ ASSERT_TRUE(RunCheck());
}
-TEST_F(MountTest, CheckChapsDirectoryCalledWithNonexistingDirWithFatalError) {
- scoped_refptr<Mount> mount = new Mount();
- NiceMock<MockPlatform> platform;
- NiceMock<MockTpm> tpm;
- mount->crypto()->set_tpm(&tpm);
- mount->crypto()->set_platform(&platform);
- mount->set_shadow_root(kImageDir);
- mount->set_skel_source(kSkelDir);
- mount->set_use_tpm(false);
- mount->set_platform(&platform);
-
- NiceMock<MockHomeDirs> mock_homedirs;
- mount->set_homedirs(&mock_homedirs);
- EXPECT_CALL(mock_homedirs, Init())
- .WillOnce(Return(true));
-
- helper_.InjectSystemSalt(&platform, kImageSaltFile);
- EXPECT_TRUE(mount->Init());
- EXPECT_CALL(platform, DirectoryExists("/fake"))
- .WillRepeatedly(Return(false));
- EXPECT_CALL(platform, DirectoryExists("/fake_legacy"))
- .WillRepeatedly(Return(false));
- EXPECT_CALL(platform, CreateDirectory("/fake"))
- .WillOnce(Return(false))
+TEST_F(ChapsDirectoryTest, FixBadOwnership) {
+ // Specify bad ownership.
+ base_stat_.st_uid = kRootUID;
+ salt_stat_.st_gid = kChapsUID;
+ database_dir_stat_.st_gid = kChapsUID;
+ database_file_stat_.st_uid = kSharedGID;
+ SetupFakeChapsDirectory();
+ // Expect corrections.
+ EXPECT_CALL(platform_, SetOwnership(kBaseDir, kChapsUID, kSharedGID))
.WillRepeatedly(Return(true));
- EXPECT_CALL(platform, SetOwnership("/fake", _, _))
- .WillOnce(Return(false))
+ EXPECT_CALL(platform_, SetOwnership(kSaltFile, kRootUID, kRootGID))
.WillRepeatedly(Return(true));
- EXPECT_CALL(platform, SetPermissions("/fake", _))
- .WillRepeatedly(Return(false));
- EXPECT_CALL(platform, GetPermissions("/fake", _))
- .WillRepeatedly(Return(false));
- bool permissions_status = false;
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_FALSE(permissions_status);
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_FALSE(permissions_status);
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_FALSE(permissions_status);
+ EXPECT_CALL(platform_, SetOwnership(kDatabaseDir, kChapsUID, kSharedGID))
+ .WillRepeatedly(Return(true));
+ EXPECT_CALL(platform_, SetOwnership(kDatabaseFile, kChapsUID, kSharedGID))
+ .WillRepeatedly(Return(true));
+ ASSERT_TRUE(RunCheck());
}
-TEST_F(MountTest, CheckChapsDirectoryCalledWithExistingDirWithFatalError) {
- scoped_refptr<Mount> mount = new Mount();
- NiceMock<MockPlatform> platform;
- NiceMock<MockTpm> tpm;
- mount->crypto()->set_tpm(&tpm);
- mount->crypto()->set_platform(&platform);
- mount->set_shadow_root(kImageDir);
- mount->set_skel_source(kSkelDir);
- mount->set_use_tpm(false);
- mount->set_platform(&platform);
-
- NiceMock<MockHomeDirs> mock_homedirs;
- mount->set_homedirs(&mock_homedirs);
- EXPECT_CALL(mock_homedirs, Init())
- .WillOnce(Return(true));
-
- helper_.InjectSystemSalt(&platform, kImageSaltFile);
- EXPECT_TRUE(mount->Init());
- EXPECT_CALL(platform, DirectoryExists("/fake"))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(platform, GetPermissions("/fake", _))
- .WillOnce(Return(false))
- .WillRepeatedly(Return(true));
- EXPECT_CALL(platform, GetOwnership("/fake", _, _))
+TEST_F(ChapsDirectoryTest, FixBadPermsFailure) {
+ // Specify some bad perms.
+ base_stat_.st_mode = 040700;
+ SetupFakeChapsDirectory();
+ // Expect corrections but fail to apply.
+ EXPECT_CALL(platform_, SetPermissions(_, _))
.WillRepeatedly(Return(false));
- bool permissions_status = false;
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_FALSE(permissions_status);
- EXPECT_FALSE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_FALSE(permissions_status);
+ ASSERT_FALSE(RunCheck());
+}
+
+TEST_F(ChapsDirectoryTest, FixBadOwnershipFailure) {
+ // Specify bad ownership.
+ base_stat_.st_uid = kRootUID;
+ SetupFakeChapsDirectory();
+ // Expect corrections but fail to apply.
+ EXPECT_CALL(platform_, SetOwnership(_, _, _))
+ .WillRepeatedly(Return(false));
+ ASSERT_FALSE(RunCheck());
}
TEST_F(MountTest, CheckChapsDirectoryMigration) {
@@ -520,9 +487,8 @@
find_info2.stat.st_mode = 0777;
find_info2.stat.st_uid = 5;
find_info2.stat.st_gid = 6;
- EXPECT_CALL(*enumerator, GetFindInfo(_))
- .WillOnce(SetArgumentPointee<0>(find_info1))
- .WillOnce(SetArgumentPointee<0>(find_info2));
+ enumerator->find_info_.push_back(find_info1);
+ enumerator->find_info_.push_back(find_info2);
// These expectations will ensure the ownership and permissions are being
// correctly applied after the directory has been moved.
@@ -533,10 +499,7 @@
EXPECT_CALL(platform, SetOwnership("/fake", 1, 2)).Times(1);
EXPECT_CALL(platform, SetPermissions("/fake", 0123)).Times(1);
- bool permissions_status = false;
- EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy",
- &permissions_status));
- EXPECT_TRUE(permissions_status);
+ EXPECT_TRUE(mount->CheckChapsDirectory("/fake", "/fake_legacy"));
}
TEST_F(MountTest, CreateCryptohomeTest) {
diff --git a/platform.cc b/platform.cc
index 6be35f0..8480184 100644
--- a/platform.cc
+++ b/platform.cc
@@ -24,6 +24,8 @@
#include <unistd.h>
#include <base/basictypes.h>
+#include <base/bind.h>
+#include <base/callback.h>
#include <base/file_util.h>
#include <base/posix/eintr_wrapper.h>
#include <base/string_number_conversions.h>
@@ -566,31 +568,43 @@
return file_util::CopyDirectory(from_path, to_path, true);
}
-bool Platform::CopyWithPermissions(const std::string& from_path,
- const std::string& to_path) {
- // Save stat for every file.
- std::vector<FileEnumerator::FindInfo> entry_list;
- FileEnumerator::FindInfo base_entry_info;
- base_entry_info.filename = from_path;
- if (!Stat(from_path, &base_entry_info.stat)) {
- PLOG(ERROR) << "Failed to stat " << from_path;
- return false;
- }
- entry_list.push_back(base_entry_info);
- if (FileEnumerator::IsDirectory(base_entry_info)) {
- int file_types = FileEnumerator::FILES | FileEnumerator::DIRECTORIES;
- scoped_ptr<FileEnumerator> file_enumerator(
- GetFileEnumerator(from_path, true, file_types));
- string entry_path;
- while (!(entry_path = file_enumerator->Next()).empty()) {
- FileEnumerator::FindInfo entry_info;
- file_enumerator->GetFindInfo(&entry_info);
- // Keep the full path.
- entry_info.filename = entry_path;
- entry_list.push_back(entry_info);
+bool Platform::CopyPermissionsCallback(
+ const std::string& old_base,
+ const std::string& new_base,
+ const FileEnumerator::FindInfo& file_info) {
+ const FilePath old_base_path(old_base);
+ const FilePath new_base_path(new_base);
+ // Find the new path that corresponds with the old path given by file_info.
+ FilePath old_path(file_info.filename);
+ FilePath new_path(new_base_path);
+ if (old_path != old_base_path) {
+ if (old_path.IsAbsolute()) {
+ if (!old_base_path.AppendRelativePath(old_path, &new_path)) {
+ LOG(ERROR) << "AppendRelativePath failed: parent="
+ << old_base_path.value() << ", child=" << old_path.value();
+ return false;
+ }
+ } else {
+ new_path = new_base_path.Append(old_path);
}
}
+ if (!SetOwnership(new_path.value(),
+ file_info.stat.st_uid,
+ file_info.stat.st_gid)) {
+ PLOG(ERROR) << "Failed to set ownership for " << new_path.value();
+ return false;
+ }
+ const mode_t permissions_mask = 07777;
+ if (!SetPermissions(new_path.value(),
+ file_info.stat.st_mode & permissions_mask)) {
+ PLOG(ERROR) << "Failed to set permissions for " << new_path.value();
+ return false;
+ }
+ return true;
+}
+bool Platform::CopyWithPermissions(const std::string& from_path,
+ const std::string& to_path) {
if (!Copy(from_path, to_path)) {
PLOG(ERROR) << "Failed to copy " << from_path;
return false;
@@ -601,40 +615,73 @@
// Unfortunately, ownership and permissions are not always retained.
// Apply the old ownership / permissions on a per-file basis.
- const FilePath new_base(to_path);
- const FilePath old_base(from_path);
- for (size_t i = 0; i < entry_list.size(); ++i) {
- FilePath old_path(entry_list[i].filename);
- FilePath new_path(new_base);
- if (old_path != old_base) {
- if (old_path.IsAbsolute()) {
- if (!old_base.AppendRelativePath(old_path, &new_path)) {
- LOG(ERROR) << "AppendRelativePath failed: parent=" << old_base.value()
- << ", child=" << old_path.value();
- return false;
- }
- } else {
- new_path = new_base.Append(old_path);
- }
- }
- if (!SetOwnership(new_path.value(),
- entry_list[i].stat.st_uid,
- entry_list[i].stat.st_gid)) {
- PLOG(ERROR) << "Failed to set ownership for " << new_path.value();
- return false;
- }
- const mode_t permissions_mask = 07777;
- if (!SetPermissions(new_path.value(),
- entry_list[i].stat.st_mode & permissions_mask)) {
- PLOG(ERROR) << "Failed to set permissions for " << new_path.value();
- return false;
- }
- }
+ FileEnumeratorCallback callback = base::Bind(
+ &Platform::CopyPermissionsCallback,
+ base::Unretained(this),
+ from_path,
+ to_path);
+ if (!WalkPath(from_path, callback))
+ return false;
+
// The copy is done, keep the new path.
scoped_new_path.release();
return true;
}
+bool Platform::ApplyPermissionsCallback(
+ const Permissions& default_file_info,
+ const Permissions& default_dir_info,
+ const std::map<std::string, Permissions>& special_cases,
+ const FileEnumerator::FindInfo& file_info) {
+ Permissions expected;
+ std::map<std::string, Permissions>::const_iterator it =
+ special_cases.find(file_info.filename);
+ if (it != special_cases.end()) {
+ expected = it->second;
+ } else if (FileEnumerator::IsDirectory(file_info)) {
+ expected = default_dir_info;
+ } else {
+ expected = default_file_info;
+ }
+ if (expected.user != file_info.stat.st_uid ||
+ expected.group != file_info.stat.st_gid) {
+ LOG(WARNING) << "Unexpected user/group for " << file_info.filename;
+ if (!SetOwnership(file_info.filename,
+ expected.user,
+ expected.group)) {
+ PLOG(ERROR) << "Failed to fix user/group for "
+ << file_info.filename;
+ return false;
+ }
+ }
+ const mode_t permissions_mask = 07777;
+ if ((expected.mode & permissions_mask) !=
+ (file_info.stat.st_mode & permissions_mask)) {
+ LOG(WARNING) << "Unexpected permissions for " << file_info.filename;
+ if (!SetPermissions(file_info.filename,
+ expected.mode & permissions_mask)) {
+ PLOG(ERROR) << "Failed to set permissions for "
+ << file_info.filename;
+ return false;
+ }
+ }
+ return true;
+}
+
+bool Platform::ApplyPermissionsRecursive(
+ const std::string& path,
+ const Permissions& default_file_info,
+ const Permissions& default_dir_info,
+ const std::map<std::string, Permissions>& special_cases) {
+ FileEnumeratorCallback callback = base::Bind(
+ &Platform::ApplyPermissionsCallback,
+ base::Unretained(this),
+ default_file_info,
+ default_dir_info,
+ special_cases);
+ return WalkPath(path, callback);
+}
+
bool Platform::StatVFS(const std::string& path, struct statvfs* vfs) {
return statvfs(path.c_str(), vfs) == 0;
}
@@ -777,7 +824,32 @@
return new FileEnumerator(root_path, recursive, file_type);
}
-
+bool Platform::WalkPath(const std::string& path,
+ const FileEnumeratorCallback& callback) {
+ FileEnumerator::FindInfo base_entry_info;
+ base_entry_info.filename = path;
+ if (!Stat(path, &base_entry_info.stat)) {
+ PLOG(ERROR) << "Failed to stat " << path;
+ return false;
+ }
+ if (!callback.Run(base_entry_info))
+ return false;
+ if (FileEnumerator::IsDirectory(base_entry_info)) {
+ int file_types = FileEnumerator::FILES | FileEnumerator::DIRECTORIES;
+ scoped_ptr<FileEnumerator> file_enumerator(
+ GetFileEnumerator(path, true, file_types));
+ std::string entry_path;
+ while (!(entry_path = file_enumerator->Next()).empty()) {
+ FileEnumerator::FindInfo entry_info;
+ file_enumerator->GetFindInfo(&entry_info);
+ // Keep the full path.
+ entry_info.filename = entry_path;
+ if (!callback.Run(entry_info))
+ return false;
+ }
+ }
+ return true;
+}
FileEnumerator::FileEnumerator(const std::string& root_path,
bool recursive,
@@ -838,4 +910,5 @@
*(reinterpret_cast<const file_util::FileEnumerator::FindInfo*>(&info)));
}
+
} // namespace cryptohome
diff --git a/platform.h b/platform.h
index 36f987e..a5858f6 100644
--- a/platform.h
+++ b/platform.h
@@ -9,6 +9,7 @@
#include <sys/statvfs.h>
#include <base/basictypes.h>
+#include <base/callback_forward.h>
#include <chromeos/secure_blob.h>
#include <chromeos/utility.h>
#include <map>
@@ -25,12 +26,67 @@
extern const int kDefaultUmask;
class ProcessInformation;
-class FileEnumerator;
+
+// A class for enumerating the files in a provided path. The order of the
+// results is not guaranteed.
+//
+// DO NOT USE FROM THE MAIN THREAD of your application unless it is a test
+// program where latency does not matter. This class is blocking.
+//
+// See file_util::FileEnumerator for details. This is merely a mockable
+// wrapper.
+class FileEnumerator {
+ public:
+ typedef struct {
+ struct stat stat;
+ std::string filename;
+ } FindInfo;
+ enum FileType {
+ FILES = 1 << 0,
+ DIRECTORIES = 1 << 1,
+ INCLUDE_DOT_DOT = 1 << 2,
+ SHOW_SYM_LINKS = 1 << 4,
+ };
+
+ FileEnumerator(const std::string& root_path,
+ bool recursive,
+ int file_type);
+ FileEnumerator(const std::string& root_path,
+ bool recursive,
+ int file_type,
+ const std::string& pattern);
+ // Meant for testing only.
+ FileEnumerator() : enumerator_(NULL) { }
+ virtual ~FileEnumerator();
+
+ // Returns an empty string if there are no more results.
+ virtual std::string Next();
+
+ // Write the file info into |info|.
+ virtual void GetFindInfo(FindInfo* info);
+
+ // The static methods are exclusively helpers for interpreting FindInfo.
+ static bool IsDirectory(const FindInfo& info);
+ static std::string GetFilename(const FindInfo& find_info);
+ static int64 GetFilesize(const FindInfo& find_info);
+ static base::Time GetLastModifiedTime(const FindInfo& find_info);
+ private:
+ file_util::FileEnumerator* enumerator_;
+};
// Platform specific routines abstraction layer.
// Also helps us to be able to mock them in tests.
class Platform {
public:
+ struct Permissions {
+ uid_t user;
+ gid_t group;
+ mode_t mode;
+ };
+
+ typedef base::Callback<bool(const FileEnumerator::FindInfo&)>
+ FileEnumeratorCallback;
+
Platform();
virtual ~Platform();
@@ -140,6 +196,20 @@
gid_t group_id,
mode_t group_mode) const;
+ // Applies ownership and permissions recursively if they do not already match.
+ // Logs a warning each time ownership or permissions need to be set.
+ //
+ // Parameters
+ // path - The base path.
+ // default_file_info - Default ownership / perms for files.
+ // default_dir_info - Default ownership / perms for directories.
+ // special_cases - A map of absolute path to ownership / perms.
+ virtual bool ApplyPermissionsRecursive(
+ const std::string& path,
+ const Permissions& default_file_info,
+ const Permissions& default_dir_info,
+ const std::map<std::string, Permissions>& special_cases);
+
// Sets the current umask, returning the old mask
//
// Parameters
@@ -390,6 +460,33 @@
// Calls fsync() on |path|. Returns true on success.
bool SyncPath(const std::string& path);
+ // Calls |callback| with |path| and, if |path| is a directory, with every
+ // entry recursively. Order is not guaranteed, see base::FileEnumerator. If
+ // |path| is an absolute path, then the file names sent to |callback| will
+ // also be absolute. Returns true if all invocations of |callback| succeed.
+ // If an invocation fails, the walk terminates and false is returned.
+ bool WalkPath(const std::string& path,
+ const FileEnumeratorCallback& callback);
+
+ // Copies permissions from a file specified by |file_info| to another file
+ // with the same name but a child of |new_base|, not |old_base|.
+ bool CopyPermissionsCallback(const std::string& old_base,
+ const std::string& new_base,
+ const FileEnumerator::FindInfo& file_info);
+
+ // Applies ownership and permissions to a single file or directory.
+ //
+ // Parameters
+ // default_file_info - Default ownership / perms for files.
+ // default_dir_info - Default ownership / perms for directories.
+ // special_cases - A map of absolute path to ownership / perms.
+ // file_info - Info about the file or directory.
+ bool ApplyPermissionsCallback(
+ const Permissions& default_file_info,
+ const Permissions& default_dir_info,
+ const std::map<std::string, Permissions>& special_cases,
+ const FileEnumerator::FindInfo& file_info);
+
std::string mtab_path_;
DISALLOW_COPY_AND_ASSIGN(Platform);
@@ -463,53 +560,6 @@
int process_id_;
};
-// A class for enumerating the files in a provided path. The order of the
-// results is not guaranteed.
-//
-// DO NOT USE FROM THE MAIN THREAD of your application unless it is a test
-// program where latency does not matter. This class is blocking.
-//
-// See file_util::FileEnumerator for details. This is merely a mockable
-// wrapper.
-class FileEnumerator {
- public:
- typedef struct {
- struct stat stat;
- std::string filename;
- } FindInfo;
- enum FileType {
- FILES = 1 << 0,
- DIRECTORIES = 1 << 1,
- INCLUDE_DOT_DOT = 1 << 2,
- SHOW_SYM_LINKS = 1 << 4,
- };
-
- FileEnumerator(const std::string& root_path,
- bool recursive,
- int file_type);
- FileEnumerator(const std::string& root_path,
- bool recursive,
- int file_type,
- const std::string& pattern);
- // Meant for testing only.
- FileEnumerator() : enumerator_(NULL) { }
- virtual ~FileEnumerator();
-
- // Returns an empty string if there are no more results.
- virtual std::string Next();
-
- // Write the file info into |info|.
- virtual void GetFindInfo(FindInfo* info);
-
- // The static methods are exclusively helpers for interpreting FindInfo.
- static bool IsDirectory(const FindInfo& info);
- static std::string GetFilename(const FindInfo& find_info);
- static int64 GetFilesize(const FindInfo& find_info);
- static base::Time GetLastModifiedTime(const FindInfo& find_info);
- private:
- file_util::FileEnumerator* enumerator_;
-};
-
} // namespace cryptohome
#endif // CRYPTOHOME_PLATFORM_H_
