Commit 5fbf3bbb authored by Joe Mason's avatar Joe Mason Committed by Commit Bot

Enable quarantine feature in chrome_cleaner

R=csharp@chromium.org

Bug: 830892
Change-Id: I6c3a17c2761545f58e18d9058bf2dedc31acf39e
Reviewed-on: https://chromium-review.googlesource.com/c/1338566Reviewed-by: default avatarChris Sharp <csharp@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/master@{#608862}
parent 1b94510d
......@@ -163,6 +163,7 @@ void AppendFolderInformation(const FolderInformation& folder,
void AppendMatchedFile(const MatchedFile& file, MessageBuilder* builder) {
AppendFileInformation(file.file_information(), builder);
builder->Add(L", removal_status = ", file.removal_status());
builder->Add(L", quarantine_status = ", file.quarantine_status());
}
void AppendMatchedRegistryEntry(const MatchedRegistryEntry& registry,
......@@ -834,11 +835,17 @@ void CleanerLoggingService::UpdateFileRemovalStatuses() {
auto folder_it = matched_folders_.find(sanitized_path);
if (file_it != matched_files_.end()) {
for (MatchedFile* matched_file : file_it->second)
for (MatchedFile* matched_file : file_it->second) {
matched_file->set_removal_status(status.removal_status);
matched_file->set_quarantine_status(status.quarantine_status);
}
} else if (folder_it != matched_folders_.end()) {
for (MatchedFolder* matched_folder : folder_it->second)
for (MatchedFolder* matched_folder : folder_it->second) {
matched_folder->set_removal_status(status.removal_status);
// We don't quarantine folders. So the quarantine status should be
// |QUARANTINE_STATUS_UNSPECIFIED| and we don't need to record it.
DCHECK(status.quarantine_status == QUARANTINE_STATUS_UNSPECIFIED);
}
} else {
known_matched_file = false;
}
......@@ -879,6 +886,7 @@ void CleanerLoggingService::UpdateFileRemovalStatuses() {
file->mutable_file_information()->set_path(sanitized_path);
}
file->set_removal_status(status.removal_status);
file->set_quarantine_status(status.quarantine_status);
}
}
}
......
......@@ -88,7 +88,7 @@ const wchar_t kMatchedFileToStringExpectedString[] =
L"product_short_name = 'PNShort', internal_name = 'Internal Name', "
L"original_filename = 'Something_Original.tmp', file_description = 'Very "
L"descriptive', file_version = '42.1.2', active_file = '0', removal_status "
L"= 0";
L"= 0, quarantine_status = 0";
const wchar_t kMatchedRegistryEntryKey[] = L"123";
const wchar_t kMatchedRegistryEntryValueName[] = L"Value Name";
......@@ -1189,6 +1189,19 @@ void ExpectRemovalStatus(const ChromeCleanerReport& report,
}
}
// Checks that all files for all UwS entries in |report| have quarantine status
// |expected_status|.
void ExpectQuarantineStatus(const ChromeCleanerReport& report,
QuarantineStatus expected_status) {
for (const UwS& uws : report.detected_uws()) {
for (const MatchedFile& file : uws.files())
EXPECT_EQ(expected_status, file.quarantine_status());
// We should not quarantine any folder.
EXPECT_TRUE(uws.folders().empty());
}
}
// Checks that the given path appears in the unknown UwS field with the expected
// removal status.
void ExpectUnknownRemovalStatus(const ChromeCleanerReport& report,
......@@ -1293,6 +1306,52 @@ TEST_P(CleanerLoggingServiceTest, UpdateRemovalStatus) {
}
}
TEST_P(CleanerLoggingServiceTest, UpdateQuarantineStatus) {
const base::FilePath kFile1(L"C:\\Program Files\\My Dear UwS\\File1.exe");
// Creates a vector of all QuarantineStatus enum values to improve readability
// of loops in this test and ensure that all QuarantineStatus enumerators are
// checked.
std::vector<QuarantineStatus> all_quarantine_status;
for (int i = QuarantineStatus_MIN; i <= QuarantineStatus_MAX; ++i) {
// Status cannot be set to QUARANTINE_STATUS_UNSPECIFIED - this is guarded
// by an assert.
QuarantineStatus status = static_cast<QuarantineStatus>(i);
if (QuarantineStatus_IsValid(status) &&
status != QUARANTINE_STATUS_UNSPECIFIED)
all_quarantine_status.push_back(status);
}
FileRemovalStatusUpdater* removal_status_updater =
FileRemovalStatusUpdater::GetInstance();
for (QuarantineStatus status : all_quarantine_status) {
logging_service_->EnableUploads(true, registry_logger_.get());
UwS uws;
uws.set_id(1);
AddFileToUwS(kFile1, &uws);
logging_service_->AddDetectedUwS(uws);
ChromeCleanerReport report;
// The default quarantine status should be |QUARANTINE_STATUS_UNSPECIFIED|.
EXPECT_TRUE(report.ParseFromString(logging_service_->RawReportContent()));
ExpectQuarantineStatus(report, QUARANTINE_STATUS_UNSPECIFIED);
// Removal status has to be updated with a valid status.
removal_status_updater->UpdateRemovalStatus(kFile1,
REMOVAL_STATUS_MATCHED_ONLY);
removal_status_updater->UpdateQuarantineStatus(kFile1, status);
// It should always succeed to override |QUARANTINE_STATUS_UNSPECIFIED|.
EXPECT_TRUE(report.ParseFromString(logging_service_->RawReportContent()));
ExpectQuarantineStatus(report, status);
// Reset the logging service, so one the current test doesn't interfere with
// the next one.
logging_service_->Terminate();
logging_service_->Initialize(registry_logger_.get());
}
}
TEST_P(CleanerLoggingServiceTest, UpdateRemovalStatus_UwSAdded) {
constexpr wchar_t kFile1[] = L"C:\\Program Files\\My Dear UwS\\File1.exe";
......
......@@ -103,14 +103,19 @@ source_set("cleaner_os") {
":common_os",
"//base:base",
"//chrome/chrome_cleaner/constants:common_strings",
"//chrome/chrome_cleaner/constants:quarantine_constants",
"//chrome/chrome_cleaner/constants:version_header",
"//chrome/chrome_cleaner/interfaces:zip_archiver_interface",
"//chrome/chrome_cleaner/logging/proto:removal_status_proto",
"//chrome/chrome_cleaner/proto:shared_pup_enums_proto",
"//chrome/chrome_cleaner/zip_archiver:common",
"//components/chrome_cleaner/public/constants:constants",
"//sandbox/win:sandbox",
]
public_deps = [
":file_remover_api",
"//chrome/chrome_cleaner/logging/proto:removal_status_proto",
]
}
......@@ -146,7 +151,9 @@ source_set("unittest_sources") {
"//base/test:test_config",
"//base/test:test_support",
"//chrome/chrome_cleaner/constants:common_strings",
"//chrome/chrome_cleaner/constants:quarantine_constants",
"//chrome/chrome_cleaner/constants:version_header",
"//chrome/chrome_cleaner/ipc:mojo_task_runner",
"//chrome/chrome_cleaner/logging/proto:removal_status_proto",
"//chrome/chrome_cleaner/proto:shared_pup_enums_proto",
"//chrome/chrome_cleaner/strings",
......@@ -156,7 +163,11 @@ source_set("unittest_sources") {
"//chrome/chrome_cleaner/test:test_strings",
"//chrome/chrome_cleaner/test:test_util",
"//chrome/chrome_cleaner/test/resources:test_resources",
"//chrome/chrome_cleaner/zip_archiver:common",
"//chrome/chrome_cleaner/zip_archiver/broker:common",
"//chrome/chrome_cleaner/zip_archiver/target:common",
"//components/chrome_cleaner/public/constants:constants",
"//sandbox/win:sandbox",
"//testing/gmock",
"//testing/gtest",
]
......
......@@ -215,10 +215,14 @@ void FileRemovalStatusUpdater::Clear() {
void FileRemovalStatusUpdater::UpdateRemovalStatus(const base::FilePath& path,
RemovalStatus status) {
// Force update of RemovalStatusCanBeOverriddenBy() if RemovalStatus enum
// changes. REMOVAL_STATUS_UNSPECIFIED should never be set.
// Compare against the highest known removal status, not RemovalStatus_MAX.
// That way if the RemovalStatus enum changes, a unit test that iterates up
// to RemovalStatus_MAX will fail on this DCHECK. This is a reminder to add
// the new RemovalStatus to RemovalStatusCanBeOverriddenBy().
DCHECK(status > REMOVAL_STATUS_UNSPECIFIED &&
status <= REMOVAL_STATUS_ERROR_IN_ARCHIVER);
status <= REMOVAL_STATUS_ERROR_IN_ARCHIVER)
<< "Unknown RemovalStatus: need to update "
"RemovalStatusCanBeOverriddenBy()?";
const base::string16 sanitized_path = SanitizePath(path);
......@@ -229,6 +233,7 @@ void FileRemovalStatusUpdater::UpdateRemovalStatus(const base::FilePath& path,
FileRemovalStatus new_status;
new_status.path = path;
new_status.removal_status = status;
new_status.quarantine_status = QUARANTINE_STATUS_UNSPECIFIED;
removal_statuses_.emplace(sanitized_path, new_status);
} else {
// Only update the entry if the new status is allowed to override the
......@@ -254,6 +259,44 @@ RemovalStatus FileRemovalStatusUpdater::GetRemovalStatusOfSanitizedPath(
: it->second.removal_status;
}
void FileRemovalStatusUpdater::UpdateQuarantineStatus(
const base::FilePath& path,
QuarantineStatus status) {
// QUARANTINE_STATUS_UNSPECIFIED should never be set.
DCHECK(status > QUARANTINE_STATUS_UNSPECIFIED &&
status <= QuarantineStatus_MAX);
const base::string16 sanitized_path = SanitizePath(path);
base::AutoLock lock(removal_status_lock_);
auto it = removal_statuses_.find(sanitized_path);
// If the |sanitized_path| is not found, it will initialize the removal status
// with |REMOVAL_STATUS_UNSPECIFIED|, which should be updated with other valid
// statuses later.
if (it == removal_statuses_.end()) {
FileRemovalStatus new_status;
new_status.path = path;
new_status.removal_status = REMOVAL_STATUS_UNSPECIFIED;
new_status.quarantine_status = status;
removal_statuses_.emplace(sanitized_path, new_status);
} else {
it->second.path = path;
it->second.quarantine_status = status;
}
}
QuarantineStatus FileRemovalStatusUpdater::GetQuarantineStatus(
const base::FilePath& path) const {
const base::string16 sanitized_path = SanitizePath(path);
base::AutoLock lock(removal_status_lock_);
const auto it = removal_statuses_.find(sanitized_path);
return it == removal_statuses_.end() ? QUARANTINE_STATUS_UNSPECIFIED
: it->second.quarantine_status;
}
FileRemovalStatusUpdater::SanitizedPathToRemovalStatusMap
FileRemovalStatusUpdater::GetAllRemovalStatuses() const {
base::AutoLock lock(removal_status_lock_);
......
......@@ -50,20 +50,23 @@ GetRemovalStatusOverridePermissionMap();
} // namespace internal
// This class manages a map of RemovalStatus values for all files and folders
// This class manages a map of remove statuses for all files and folders
// encountered during cleaning, keyed by path. It does not distinguish whether
// the path refers to a file or a folder.
class FileRemovalStatusUpdater {
public:
struct FileRemovalStatus {
// The full path that was passed to UpdateRemovalStatus. This is needed
// because when a file removal status is logged,
// GetFileInformationProtoObject can be called, which needs a full path
// that can be resolved.
// The full path that was passed to UpdateRemovalStatus or
// UpdateQuarantineStatus. This is needed because when a file removal status
// is logged, GetFileInformationProtoObject can be called, which needs a
// full path that can be resolved.
base::FilePath path;
// The status of the last attempted file removal at the above path.
// The removal status of the last attempted update at the above path.
RemovalStatus removal_status = REMOVAL_STATUS_UNSPECIFIED;
// The quarantine status of the last attempted update at the above path.
QuarantineStatus quarantine_status = QUARANTINE_STATUS_UNSPECIFIED;
};
typedef std::unordered_map<base::string16, FileRemovalStatus>
......@@ -82,19 +85,30 @@ class FileRemovalStatusUpdater {
void UpdateRemovalStatus(const base::FilePath& path, RemovalStatus status);
// Returns the removal status of |path|, or REMOVAL_STATUS_UNSPECIFIED if
// UpdateRemovalStatus has never been called for that path.
// the removal status have never been updated for that path.
RemovalStatus GetRemovalStatus(const base::FilePath& path) const;
// Returns the removal status of |sanitized_path|, or
// REMOVAL_STATUS_UNSPECIFIED if UpdateRemovalStatus has never been called
// for an unsanitized form of that path.
// REMOVAL_STATUS_UNSPECIFIED if the removal status have never
// been updated for an unsanitized form of that path.
RemovalStatus GetRemovalStatusOfSanitizedPath(
const base::string16& sanitized_path) const;
// Updates quarantine status for a file given by |path|.
// Note: UpdateRemovalStatus should be called for |path| at some point as
// well, because it is invalid to quarantine a file that doesn't have some
// removal status.
void UpdateQuarantineStatus(const base::FilePath& path,
QuarantineStatus status);
// Returns the quarantine status of |path|, or QUARANTINE_STATUS_UNSPECIFIED
// if the quarantine status have never been updated for that path.
QuarantineStatus GetQuarantineStatus(const base::FilePath& path) const;
// Returns all saved removal statuses, keyed by sanitized path. Each
// sanitized path is mapped to a single FileRemovalStatus which holds the
// path and status values from the most recent call to UpdateRemovalStatus
// that had an effect.
// path and status values from the most recent call to UpdateRemovalStatus or
// UpdateQuarantineStatus that had an effect.
SanitizedPathToRemovalStatusMap GetAllRemovalStatuses() const;
private:
......
......@@ -4,11 +4,12 @@
#include "chrome/chrome_cleaner/os/file_removal_status_updater.h"
#include <vector>
#include "base/base_paths.h"
#include "base/path_service.h"
#include "base/strings/string_util.h"
#include "chrome/chrome_cleaner/os/file_path_sanitization.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace chrome_cleaner {
......@@ -129,6 +130,60 @@ TEST_F(FileRemovalStatusUpdaterTest, UpdateRemovalStatus) {
}
}
TEST_F(FileRemovalStatusUpdaterTest, UpdateQuarantineStatus) {
// Creates a vector of all QuarantineStatus enum values to improve readability
// of loops in this test and ensure that all QuarantineStatus enumerators are
// checked.
std::vector<QuarantineStatus> all_quarantine_status;
for (int i = QuarantineStatus_MIN; i <= QuarantineStatus_MAX; ++i) {
// Status cannot be set to QUARANTINE_STATUS_UNSPECIFIED - this is guarded
// by an assert.
QuarantineStatus status = static_cast<QuarantineStatus>(i);
if (QuarantineStatus_IsValid(status) &&
status != QUARANTINE_STATUS_UNSPECIFIED)
all_quarantine_status.push_back(status);
}
for (QuarantineStatus status : all_quarantine_status) {
for (QuarantineStatus new_status : all_quarantine_status) {
// Empty the map for the next test.
instance_->Clear();
// Quarantine status should be |QUARANTINE_STATUS_UNSPECIFIED| if the file
// is not in the map.
EXPECT_EQ(QUARANTINE_STATUS_UNSPECIFIED,
instance_->GetQuarantineStatus(file_1_));
instance_->UpdateQuarantineStatus(file_1_, status);
// It should always succeed to override |QUARANTINE_STATUS_UNSPECIFIED|.
EXPECT_EQ(status, instance_->GetQuarantineStatus(file_1_));
instance_->UpdateQuarantineStatus(file_1_, new_status);
// It should always succeed to override the old quarantine status.
EXPECT_EQ(new_status, instance_->GetQuarantineStatus(file_1_));
}
}
}
TEST_F(FileRemovalStatusUpdaterTest, MixedRemovalQuarantineUpdate) {
instance_->UpdateRemovalStatus(file_1_, REMOVAL_STATUS_REMOVED);
// The initial quarantine status should be |QUARANTINE_STATUS_UNSPECIFIED|.
EXPECT_EQ(QUARANTINE_STATUS_UNSPECIFIED,
instance_->GetQuarantineStatus(file_1_));
instance_->UpdateQuarantineStatus(file_1_, QUARANTINE_STATUS_QUARANTINED);
// |UpdateQuarantineStatus| should not change removal status.
EXPECT_EQ(REMOVAL_STATUS_REMOVED, instance_->GetRemovalStatus(file_1_));
instance_->UpdateQuarantineStatus(file_2_, QUARANTINE_STATUS_SKIPPED);
// The initial removal status should be |REMOVAL_STATUS_UNSPECIFIED|.
EXPECT_EQ(REMOVAL_STATUS_UNSPECIFIED, instance_->GetRemovalStatus(file_2_));
instance_->UpdateRemovalStatus(file_2_, REMOVAL_STATUS_SCHEDULED_FOR_REMOVAL);
// |UpdateRemovalStatus| should not change quarantine status.
EXPECT_EQ(QUARANTINE_STATUS_SKIPPED, instance_->GetQuarantineStatus(file_2_));
}
TEST_F(FileRemovalStatusUpdaterTest, PathSanitization) {
base::FilePath home_dir;
ASSERT_TRUE(base::PathService::Get(base::DIR_HOME, &home_dir));
......
......@@ -9,9 +9,12 @@
#include <unordered_set>
#include <utility>
#include "base/bind.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/path_service.h"
#include "base/synchronization/waitable_event.h"
#include "chrome/chrome_cleaner/interfaces/zip_archiver.mojom.h"
#include "chrome/chrome_cleaner/logging/proto/removal_status.pb.h"
#include "chrome/chrome_cleaner/os/disk_util.h"
#include "chrome/chrome_cleaner/os/file_path_sanitization.h"
......@@ -77,13 +80,33 @@ bool IsSafeNameForDeletion(const base::FilePath& path) {
return false;
}
void OnArchiveDone(FileRemover::QuarantineResultCallback archival_done_callback,
mojom::ZipArchiverResultCode result_code) {
switch (result_code) {
// If the archive exists, |path| has already been quarantined.
case mojom::ZipArchiverResultCode::kSuccess:
case mojom::ZipArchiverResultCode::kZipFileExists:
std::move(archival_done_callback).Run(QUARANTINE_STATUS_QUARANTINED);
return;
case mojom::ZipArchiverResultCode::kIgnoredSourceFile:
std::move(archival_done_callback).Run(QUARANTINE_STATUS_SKIPPED);
return;
default:
LOG(ERROR) << "ZipArchiver returned an error code: " << result_code;
break;
}
std::move(archival_done_callback).Run(QUARANTINE_STATUS_ERROR);
}
} // namespace
FileRemover::FileRemover(std::shared_ptr<DigestVerifier> digest_verifier,
std::unique_ptr<SandboxedZipArchiver> archiver,
const LayeredServiceProviderAPI& lsp,
const FilePathSet& deletion_allowed_paths,
base::RepeatingClosure reboot_needed_callback)
: digest_verifier_(digest_verifier),
archiver_(std::move(archiver)),
deletion_allowed_paths_(deletion_allowed_paths),
reboot_needed_callback_(reboot_needed_callback) {
LSPPathToGUIDs providers;
......@@ -96,18 +119,21 @@ FileRemover::FileRemover(std::shared_ptr<DigestVerifier> digest_verifier,
FileRemover::~FileRemover() = default;
bool FileRemover::RemoveNow(const base::FilePath& path) const {
void FileRemover::RemoveNow(const base::FilePath& path,
DoneCallback callback) const {
FileRemovalStatusUpdater* removal_status_updater =
FileRemovalStatusUpdater::GetInstance();
switch (CanRemove(path)) {
case DeletionValidationStatus::FORBIDDEN:
removal_status_updater->UpdateRemovalStatus(
path, REMOVAL_STATUS_BLACKLISTED_FOR_REMOVAL);
return false;
std::move(callback).Run(false);
return;
case DeletionValidationStatus::INACTIVE:
removal_status_updater->UpdateRemovalStatus(
path, REMOVAL_STATUS_NOT_REMOVED_INACTIVE_EXTENSION);
return true;
std::move(callback).Run(true);
return;
case DeletionValidationStatus::ALLOWED:
// No-op. Proceed to removal.
break;
......@@ -116,45 +142,30 @@ bool FileRemover::RemoveNow(const base::FilePath& path) const {
chrome_cleaner::ScopedDisableWow64Redirection disable_wow64_redirection;
if (!base::PathExists(path)) {
removal_status_updater->UpdateRemovalStatus(path, REMOVAL_STATUS_NOT_FOUND);
return true;
}
if (!base::DeleteFile(path, /*recursive=*/false)) {
// If the attempt to delete the file fails, propagate the failure as
// normal so that the engine knows about it and can try a backup action,
// but also register the file for post-reboot removal in case the engine
// doesn't have any effective backup action.
//
// A potential downside to this implementation is that if the file is
// later successfully deleted, we might ask users to reboot when no
// reboot is needed.
if (RegisterFileForPostRebootRemoval(path)) {
reboot_needed_callback_.Run();
removal_status_updater->UpdateRemovalStatus(
path, REMOVAL_STATUS_SCHEDULED_FOR_REMOVAL_FALLBACK);
} else {
removal_status_updater->UpdateRemovalStatus(
path, REMOVAL_STATUS_FAILED_TO_REMOVE);
std::move(callback).Run(true);
return;
}
return false;
}
removal_status_updater->UpdateRemovalStatus(path, REMOVAL_STATUS_REMOVED);
DeleteEmptyDirectories(path.DirName());
return true;
TryToQuarantine(
path, base::BindOnce(&FileRemover::RemoveFile, base::Unretained(this),
path, base::Passed(&callback)));
}
bool FileRemover::RegisterPostRebootRemoval(
const base::FilePath& file_path) const {
void FileRemover::RegisterPostRebootRemoval(const base::FilePath& file_path,
DoneCallback callback) const {
FileRemovalStatusUpdater* removal_status_updater =
FileRemovalStatusUpdater::GetInstance();
switch (CanRemove(file_path)) {
case DeletionValidationStatus::FORBIDDEN:
removal_status_updater->UpdateRemovalStatus(
file_path, REMOVAL_STATUS_BLACKLISTED_FOR_REMOVAL);
return false;
std::move(callback).Run(false);
return;
case DeletionValidationStatus::INACTIVE:
removal_status_updater->UpdateRemovalStatus(
file_path, REMOVAL_STATUS_NOT_REMOVED_INACTIVE_EXTENSION);
return true;
std::move(callback).Run(true);
return;
case DeletionValidationStatus::ALLOWED:
// No-op. Proceed to removal.
break;
......@@ -164,19 +175,13 @@ bool FileRemover::RegisterPostRebootRemoval(
if (!base::PathExists(file_path)) {
removal_status_updater->UpdateRemovalStatus(file_path,
REMOVAL_STATUS_NOT_FOUND);
return true;
std::move(callback).Run(true);
return;
}
if (!RegisterFileForPostRebootRemoval(file_path)) {
PLOG(ERROR) << "Failed to schedule delete file: "
<< chrome_cleaner::SanitizePath(file_path);
removal_status_updater->UpdateRemovalStatus(
file_path, REMOVAL_STATUS_FAILED_TO_SCHEDULE_FOR_REMOVAL);
return false;
}
reboot_needed_callback_.Run();
removal_status_updater->UpdateRemovalStatus(
file_path, REMOVAL_STATUS_SCHEDULED_FOR_REMOVAL);
return true;
TryToQuarantine(file_path, base::BindOnce(&FileRemover::ScheduleRemoval,
base::Unretained(this), file_path,
base::Passed(&callback)));
}
FileRemoverAPI::DeletionValidationStatus FileRemover::CanRemove(
......@@ -233,4 +238,81 @@ FileRemoverAPI::DeletionValidationStatus FileRemover::IsFileRemovalAllowed(
return DeletionValidationStatus::INACTIVE;
}
void FileRemover::TryToQuarantine(const base::FilePath& path,
QuarantineResultCallback callback) const {
// The quarantine feature is disabled.
if (archiver_ == nullptr) {
std::move(callback).Run(QUARANTINE_STATUS_DISABLED);
return;
}
archiver_->Archive(path, base::BindOnce(&OnArchiveDone, std::move(callback)));
}
void FileRemover::RemoveFile(const base::FilePath& path,
DoneCallback removal_done_callback,
QuarantineStatus quarantine_status) const {
FileRemovalStatusUpdater* removal_status_updater =
FileRemovalStatusUpdater::GetInstance();
removal_status_updater->UpdateQuarantineStatus(path, quarantine_status);
if (quarantine_status == QUARANTINE_STATUS_ERROR) {
removal_status_updater->UpdateRemovalStatus(
path, REMOVAL_STATUS_ERROR_IN_ARCHIVER);
std::move(removal_done_callback).Run(false);
return;
}
if (!base::DeleteFile(path, /*recursive=*/false)) {
// If the attempt to delete the file fails, propagate the failure as
// normal so that the engine knows about it and can try a backup action,
// but also register the file for post-reboot removal in case the engine
// doesn't have any effective backup action.
//
// A potential downside to this implementation is that if the file is
// later successfully deleted, we might ask users to reboot when no
// reboot is needed. See b/66944160 for more details.
if (RegisterFileForPostRebootRemoval(path)) {
reboot_needed_callback_.Run();
removal_status_updater->UpdateRemovalStatus(
path, REMOVAL_STATUS_SCHEDULED_FOR_REMOVAL_FALLBACK);
} else {
removal_status_updater->UpdateRemovalStatus(
path, REMOVAL_STATUS_FAILED_TO_REMOVE);
}
std::move(removal_done_callback).Run(false);
return;
}
removal_status_updater->UpdateRemovalStatus(path, REMOVAL_STATUS_REMOVED);
DeleteEmptyDirectories(path.DirName());
std::move(removal_done_callback).Run(true);
}
void FileRemover::ScheduleRemoval(
const base::FilePath& file_path,
FileRemover::DoneCallback removal_done_callback,
QuarantineStatus quarantine_status) const {
FileRemovalStatusUpdater* removal_status_updater =
FileRemovalStatusUpdater::GetInstance();
removal_status_updater->UpdateQuarantineStatus(file_path, quarantine_status);
if (quarantine_status == QUARANTINE_STATUS_ERROR) {
removal_status_updater->UpdateRemovalStatus(
file_path, REMOVAL_STATUS_ERROR_IN_ARCHIVER);
std::move(removal_done_callback).Run(false);
return;
}
if (!RegisterFileForPostRebootRemoval(file_path)) {
PLOG(ERROR) << "Failed to schedule delete file: "
<< chrome_cleaner::SanitizePath(file_path);
removal_status_updater->UpdateRemovalStatus(
file_path, REMOVAL_STATUS_FAILED_TO_SCHEDULE_FOR_REMOVAL);
std::move(removal_done_callback).Run(false);
return;
}
reboot_needed_callback_.Run();
removal_status_updater->UpdateRemovalStatus(
file_path, REMOVAL_STATUS_SCHEDULED_FOR_REMOVAL);
std::move(removal_done_callback).Run(true);
}
} // namespace chrome_cleaner
......@@ -12,16 +12,20 @@
#include "base/callback.h"
#include "base/strings/string16.h"
#include "chrome/chrome_cleaner/logging/proto/removal_status.pb.h"
#include "chrome/chrome_cleaner/os/digest_verifier.h"
#include "chrome/chrome_cleaner/os/file_path_set.h"
#include "chrome/chrome_cleaner/os/file_remover_api.h"
#include "chrome/chrome_cleaner/os/layered_service_provider_api.h"
#include "chrome/chrome_cleaner/zip_archiver/sandboxed_zip_archiver.h"
namespace chrome_cleaner {
// This class implements the |FileRemoverAPI| for production code.
class FileRemover : public FileRemoverAPI {
public:
typedef base::OnceCallback<void(QuarantineStatus)> QuarantineResultCallback;
// Checks whether deletion of the file at |path| is allowed. Files at paths in
// |fordbid_deletion| are never allowed to be deleted. Files at paths in
// |allow_deletion| are allowed to be deleted even if they do not appear to be
......@@ -35,6 +39,7 @@ class FileRemover : public FileRemoverAPI {
// If it is an instance of DigestVerifier, any files known to the
// DigestVerifier will not be removed.
FileRemover(std::shared_ptr<DigestVerifier> digest_verifier,
std::unique_ptr<SandboxedZipArchiver> archiver,
const LayeredServiceProviderAPI& lsp,
const FilePathSet& deletion_allowed_paths,
base::RepeatingClosure reboot_needed_callback);
......@@ -42,15 +47,27 @@ class FileRemover : public FileRemoverAPI {
~FileRemover() override;
// FileRemoverAPI implementation.
bool RemoveNow(const base::FilePath& path) const override;
bool RegisterPostRebootRemoval(
const base::FilePath& file_path) const override;
void RemoveNow(const base::FilePath& path,
DoneCallback callback) const override;
void RegisterPostRebootRemoval(const base::FilePath& file_path,
DoneCallback callback) const override;
// Checks if the file is active and can be deleted.
DeletionValidationStatus CanRemove(const base::FilePath& file) const override;
private:
void TryToQuarantine(const base::FilePath& path,
QuarantineResultCallback callback) const;
void RemoveFile(const base::FilePath& path,
DoneCallback removal_done_callback,
QuarantineStatus result) const;
void ScheduleRemoval(const base::FilePath& file_path,
DoneCallback removal_done_callback,
QuarantineStatus quarantine_status) const;
std::shared_ptr<DigestVerifier> digest_verifier_;
std::unique_ptr<SandboxedZipArchiver> archiver_;
FilePathSet deletion_forbidden_paths_;
FilePathSet deletion_allowed_paths_;
base::RepeatingClosure reboot_needed_callback_;
......
......@@ -7,6 +7,7 @@
#include <vector>
#include "base/callback.h"
#include "base/files/file_path.h"
#include "chrome/chrome_cleaner/os/file_path_set.h"
......@@ -21,16 +22,20 @@ class FileRemoverAPI {
FORBIDDEN,
INACTIVE,
};
// Callback used for the asynchronous versions of RemoveNow
// and RegisterPostRebootRemoval.
typedef base::OnceCallback<void(bool)> DoneCallback;
virtual ~FileRemoverAPI() {}
// Remove file at |path| from the user's disk. Return false on failure.
virtual bool RemoveNow(const base::FilePath& path) const = 0;
virtual void RemoveNow(const base::FilePath& path,
DoneCallback callback) const = 0;
// Register the file in |file_path| to be deleted after a machine reboot.
// Return false on failure.
virtual bool RegisterPostRebootRemoval(
const base::FilePath& file_path) const = 0;
virtual void RegisterPostRebootRemoval(const base::FilePath& file_path,
DoneCallback callback) const = 0;
// Check if file at |path| is not whitelisted and can be deleted.
virtual DeletionValidationStatus CanRemove(
......
......@@ -5,16 +5,24 @@
#include "chrome/chrome_cleaner/os/file_remover.h"
#include <stdint.h>
#include <memory>
#include <utility>
#include "base/base_paths.h"
#include "base/bind.h"
#include "base/bind_helpers.h"
#include "base/command_line.h"
#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/memory/scoped_refptr.h"
#include "base/message_loop/message_loop.h"
#include "base/path_service.h"
#include "base/strings/strcat.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/multiprocess_test.h"
#include "base/test/scoped_path_override.h"
#include "chrome/chrome_cleaner/ipc/mojo_task_runner.h"
#include "chrome/chrome_cleaner/logging/proto/removal_status.pb.h"
#include "chrome/chrome_cleaner/os/disk_util.h"
#include "chrome/chrome_cleaner/os/file_removal_status_updater.h"
......@@ -22,13 +30,21 @@
#include "chrome/chrome_cleaner/os/pre_fetched_paths.h"
#include "chrome/chrome_cleaner/os/system_util.h"
#include "chrome/chrome_cleaner/os/whitelisted_directory.h"
#include "chrome/chrome_cleaner/test/file_remover_test_util.h"
#include "chrome/chrome_cleaner/test/reboot_deletion_helper.h"
#include "chrome/chrome_cleaner/test/resources/grit/test_resources.h"
#include "chrome/chrome_cleaner/test/test_file_util.h"
#include "chrome/chrome_cleaner/test/test_layered_service_provider.h"
#include "chrome/chrome_cleaner/test/test_name_helper.h"
#include "chrome/chrome_cleaner/test/test_strings.h"
#include "chrome/chrome_cleaner/test/test_util.h"
#include "chrome/chrome_cleaner/zip_archiver/broker/sandbox_setup.h"
#include "chrome/chrome_cleaner/zip_archiver/sandboxed_zip_archiver.h"
#include "chrome/chrome_cleaner/zip_archiver/target/sandbox_setup.h"
#include "sandbox/win/src/sandbox.h"
#include "sandbox/win/src/sandbox_factory.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "testing/multiprocess_func_list.h"
namespace chrome_cleaner {
......@@ -47,7 +63,8 @@ class FileRemoverTest : public ::testing::Test {
protected:
FileRemoverTest()
: default_file_remover_(
nullptr,
/*digest_verifier=*/nullptr,
/*archiver=*/nullptr,
LayeredServiceProviderWrapper(),
/*deletion_allowed_paths=*/{},
base::BindRepeating(&FileRemoverTest::RebootRequired,
......@@ -66,12 +83,12 @@ class FileRemoverTest : public ::testing::Test {
FileRemovalStatusUpdater* removal_status_updater =
FileRemovalStatusUpdater::GetInstance();
EXPECT_FALSE(remover->RemoveNow(path));
VerifyRemoveNowFailure(path, remover);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(path),
REMOVAL_STATUS_BLACKLISTED_FOR_REMOVAL);
removal_status_updater->Clear();
EXPECT_FALSE(remover->RegisterPostRebootRemoval(path));
VerifyRegisterPostRebootRemovalFailure(path, remover);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(path),
REMOVAL_STATUS_BLACKLISTED_FOR_REMOVAL);
......@@ -80,6 +97,7 @@ class FileRemoverTest : public ::testing::Test {
}
FileRemover default_file_remover_;
base::MessageLoop message_loop_;
bool reboot_required_ = false;
};
......@@ -94,7 +112,7 @@ TEST_F(FileRemoverTest, RemoveNowValidFile) {
EXPECT_TRUE(CreateEmptyFile(file_path));
// Removing it must succeed.
EXPECT_TRUE(default_file_remover_.RemoveNow(file_path));
VerifyRemoveNowSuccess(file_path, &default_file_remover_);
EXPECT_FALSE(base::PathExists(file_path));
EXPECT_EQ(
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(file_path),
......@@ -113,14 +131,14 @@ TEST_F(FileRemoverTest, RemoveNowAbsentFile) {
EXPECT_FALSE(base::PathExists(file_path));
// Removing it must not generate an error.
EXPECT_TRUE(default_file_remover_.RemoveNow(file_path));
VerifyRemoveNowSuccess(file_path, &default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(file_path),
REMOVAL_STATUS_NOT_FOUND);
// Ensure the non-existant files with non-existant parents don't generate an
// error.
base::FilePath file_path_deeper = file_path.Append(kRemoveFile2);
EXPECT_TRUE(default_file_remover_.RemoveNow(file_path_deeper));
VerifyRemoveNowSuccess(file_path_deeper, &default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(file_path_deeper),
REMOVAL_STATUS_NOT_FOUND);
}
......@@ -131,8 +149,8 @@ TEST_F(FileRemoverTest, NoKnownFileRemoval) {
FileRemover remover(
DigestVerifier::CreateFromResource(IDS_TEST_SAMPLE_DLL_DIGEST),
LayeredServiceProviderWrapper(), /*deletion_allowed_paths=*/{},
base::DoNothing::Repeatedly());
/*archiver=*/nullptr, LayeredServiceProviderWrapper(),
/*deletion_allowed_paths=*/{}, base::DoNothing::Repeatedly());
// Copy the sample DLL to the temp folder.
base::FilePath dll_path = GetSampleDLLPath();
......@@ -171,7 +189,8 @@ TEST_F(FileRemoverTest, NoLSPRemoval) {
TestLayeredServiceProvider lsp;
lsp.AddProvider(kGUID1, provider_path);
FileRemover remover(nullptr, lsp, /*deletion_allowed_paths=*/{},
FileRemover remover(/*digest_verifier=*/nullptr, /*archiver=*/nullptr, lsp,
/*deletion_allowed_paths=*/{},
base::DoNothing::Repeatedly());
TestBlacklistedRemoval(&remover, provider_path);
......@@ -226,7 +245,7 @@ TEST_F(FileRemoverTest, RemoveNowDoesNotDeleteFolders) {
ASSERT_TRUE(CreateEmptyFile(file_path1));
// The folder should not be removed.
EXPECT_FALSE(default_file_remover_.RemoveNow(subfolder_path));
VerifyRemoveNowFailure(subfolder_path, &default_file_remover_);
EXPECT_EQ(
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(subfolder_path),
REMOVAL_STATUS_BLACKLISTED_FOR_REMOVAL);
......@@ -252,7 +271,7 @@ TEST_F(FileRemoverTest, RemoveNowDeletesEmptyFolders) {
FileRemovalStatusUpdater::GetInstance();
// Delete a file in a folder with other stuff, so the folder isn't deleted.
EXPECT_TRUE(default_file_remover_.RemoveNow(file_path1));
VerifyRemoveNowSuccess(file_path1, &default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(file_path1),
REMOVAL_STATUS_REMOVED);
EXPECT_TRUE(base::PathExists(subfolder_path));
......@@ -262,7 +281,7 @@ TEST_F(FileRemoverTest, RemoveNowDeletesEmptyFolders) {
// Delete the file and ensure the two parent folders are deleted since they
// are now empty.
EXPECT_TRUE(default_file_remover_.RemoveNow(file_path2));
VerifyRemoveNowSuccess(file_path2, &default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(file_path2),
REMOVAL_STATUS_REMOVED);
EXPECT_FALSE(base::PathExists(subfolder_path));
......@@ -281,7 +300,7 @@ TEST_F(FileRemoverTest, RemoveNowDeletesEmptyFoldersNotTemp) {
ASSERT_TRUE(CreateEmptyFile(file_path));
// Delete the file and ensure Temp isn't deleted since it is whitelisted.
EXPECT_TRUE(default_file_remover_.RemoveNow(file_path));
VerifyRemoveNowSuccess(file_path, &default_file_remover_);
EXPECT_EQ(
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(file_path),
REMOVAL_STATUS_REMOVED);
......@@ -298,7 +317,7 @@ TEST_F(FileRemoverTest, RegisterPostRebootRemoval) {
// When trying to delete a whitelisted file, we should fail to register the
// file for removal, and no reboot should be required.
base::FilePath exe_path = PreFetchedPaths::GetInstance()->GetExecutablePath();
EXPECT_FALSE(default_file_remover_.RegisterPostRebootRemoval(exe_path));
VerifyRegisterPostRebootRemovalFailure(exe_path, &default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(exe_path),
REMOVAL_STATUS_BLACKLISTED_FOR_REMOVAL);
EXPECT_FALSE(reboot_required_);
......@@ -309,7 +328,7 @@ TEST_F(FileRemoverTest, RegisterPostRebootRemoval) {
// When trying to delete an non-existant file, we should return success, but
// not require a reboot.
EXPECT_TRUE(default_file_remover_.RegisterPostRebootRemoval(file_path));
VerifyRegisterPostRebootRemovalSuccess(file_path, &default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(file_path),
REMOVAL_STATUS_NOT_FOUND);
EXPECT_FALSE(reboot_required_);
......@@ -317,7 +336,7 @@ TEST_F(FileRemoverTest, RegisterPostRebootRemoval) {
// When trying to delete a real file, we should return success and require a
// reboot.
ASSERT_TRUE(CreateEmptyFile(file_path));
EXPECT_TRUE(default_file_remover_.RegisterPostRebootRemoval(file_path));
VerifyRegisterPostRebootRemovalSuccess(file_path, &default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(file_path),
REMOVAL_STATUS_SCHEDULED_FOR_REMOVAL);
EXPECT_TRUE(reboot_required_);
......@@ -336,7 +355,8 @@ TEST_F(FileRemoverTest, RegisterPostRebootRemoval_Directories) {
FileRemovalStatusUpdater::GetInstance();
// Directories shouldn't be registered for deletion.
EXPECT_FALSE(default_file_remover_.RegisterPostRebootRemoval(subfolder_path));
VerifyRegisterPostRebootRemovalFailure(subfolder_path,
&default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(subfolder_path),
REMOVAL_STATUS_BLACKLISTED_FOR_REMOVAL);
......@@ -345,7 +365,8 @@ TEST_F(FileRemoverTest, RegisterPostRebootRemoval_Directories) {
removal_status_updater->Clear();
base::FilePath file_path1 = subfolder_path.Append(kRemoveFile1);
ASSERT_TRUE(CreateEmptyFile(file_path1));
EXPECT_FALSE(default_file_remover_.RegisterPostRebootRemoval(subfolder_path));
VerifyRegisterPostRebootRemovalFailure(subfolder_path,
&default_file_remover_);
EXPECT_EQ(removal_status_updater->GetRemovalStatus(subfolder_path),
REMOVAL_STATUS_BLACKLISTED_FOR_REMOVAL);
}
......@@ -359,7 +380,7 @@ TEST_F(FileRemoverTest, NotActiveFileType) {
EXPECT_EQ(ValidationStatus::INACTIVE,
FileRemover::IsFileRemovalAllowed(path, {}, {}));
EXPECT_TRUE(default_file_remover_.RemoveNow(path));
VerifyRemoveNowSuccess(path, &default_file_remover_);
EXPECT_EQ(REMOVAL_STATUS_NOT_REMOVED_INACTIVE_EXTENSION,
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(path));
EXPECT_TRUE(base::PathExists(path));
......@@ -372,12 +393,13 @@ TEST_F(FileRemoverTest, NotActiveFileAllowed) {
ASSERT_TRUE(
CreateFileInFolder(path.DirName(), path.BaseName().value().c_str()));
FileRemover remover(nullptr, LayeredServiceProviderWrapper(),
{path.value().c_str()}, base::DoNothing::Repeatedly());
FileRemover remover(/*digest_verifier=*/nullptr, /*archiver=*/nullptr,
LayeredServiceProviderWrapper(), {path.value().c_str()},
base::DoNothing::Repeatedly());
EXPECT_EQ(ValidationStatus::ALLOWED, FileRemover::IsFileRemovalAllowed(
path, {path.value().c_str()}, {}));
EXPECT_TRUE(remover.RemoveNow(path));
VerifyRemoveNowSuccess(path, &remover);
EXPECT_EQ(REMOVAL_STATUS_REMOVED,
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(path));
EXPECT_FALSE(base::PathExists(path));
......@@ -391,15 +413,210 @@ TEST_F(FileRemoverTest, DosMzExecutable) {
CreateFileWithContent(path, kMzExecutable, sizeof(kMzExecutable));
ASSERT_TRUE(base::PathExists(path));
FileRemover remover(nullptr, LayeredServiceProviderWrapper(),
{path.value().c_str()}, base::DoNothing::Repeatedly());
FileRemover remover(/*digest_verifier=*/nullptr, /*archiver=*/nullptr,
LayeredServiceProviderWrapper(), {path.value().c_str()},
base::DoNothing::Repeatedly());
EXPECT_EQ(ValidationStatus::ALLOWED,
FileRemover::IsFileRemovalAllowed(path, {}, {}));
EXPECT_TRUE(remover.RemoveNow(path));
VerifyRemoveNowSuccess(path, &remover);
EXPECT_EQ(REMOVAL_STATUS_REMOVED,
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(path));
EXPECT_FALSE(base::PathExists(path));
}
namespace {
constexpr char kTestPassword[] = "1234";
constexpr char kTestContent[] = "Hello World";
constexpr wchar_t kTestFileName[] = L"temp_file.exe";
constexpr wchar_t kTestExpectArchiveName[] =
L"temp_file.exe_"
L"A591A6D40BF420404A011733CFB7B190D62C65BF0BCDA32B57B277D9AD9F146E.zip";
class FileRemoverQuarantineTest : public base::MultiProcessTest,
public ::testing::WithParamInterface<bool> {
public:
void SetUp() override {
use_reboot_removal_ = GetParam();
scoped_refptr<MojoTaskRunner> mojo_task_runner = MojoTaskRunner::Create();
ZipArchiverSandboxSetupHooks setup_hooks(
mojo_task_runner.get(), base::BindOnce([] {
FAIL() << "ZipArchiver sandbox connection error";
}));
ASSERT_EQ(RESULT_CODE_SUCCESS,
StartSandboxTarget(MakeCmdLine("FileRemoverQuarantineTargetMain"),
&setup_hooks, SandboxType::kTest));
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
auto zip_archiver = std::make_unique<SandboxedZipArchiver>(
mojo_task_runner, setup_hooks.TakeZipArchiverPtr(), temp_dir_.GetPath(),
kTestPassword);
file_remover_ = std::make_unique<FileRemover>(
/*digest_verifier=*/nullptr, std::move(zip_archiver),
LayeredServiceProviderWrapper(), FilePathSet(),
base::DoNothing::Repeatedly());
}
protected:
// Do removal corresponding to |use_reboot_removal_|. Also do corresponding
// check for file existence and removal status.
void DoAndExpectCorrespondingRemoval(const base::FilePath& path) {
if (use_reboot_removal_) {
VerifyRegisterPostRebootRemovalSuccess(path, file_remover_.get());
EXPECT_EQ(
REMOVAL_STATUS_SCHEDULED_FOR_REMOVAL,
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(path));
EXPECT_TRUE(base::PathExists(path));
} else {
VerifyRemoveNowSuccess(path, file_remover_.get());
EXPECT_EQ(
REMOVAL_STATUS_REMOVED,
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(path));
EXPECT_FALSE(base::PathExists(path));
}
return;
}
bool use_reboot_removal_ = false;
base::MessageLoop message_loop_;
base::ScopedTempDir temp_dir_;
std::unique_ptr<FileRemover> file_remover_;
};
} // namespace
MULTIPROCESS_TEST_MAIN(FileRemoverQuarantineTargetMain) {
sandbox::TargetServices* sandbox_target_services =
sandbox::SandboxFactory::GetTargetServices();
DCHECK(sandbox_target_services);
// |RunZipArchiverSandboxTarget| won't return. The mojo error handler will
// abort this process when the connection is broken.
RunZipArchiverSandboxTarget(*base::CommandLine::ForCurrentProcess(),
sandbox_target_services);
return 0;
}
TEST_P(FileRemoverQuarantineTest, QuarantineFile) {
const base::FilePath path = temp_dir_.GetPath().Append(kTestFileName);
CreateFileWithContent(path, kTestContent, strlen(kTestContent));
DoAndExpectCorrespondingRemoval(path);
EXPECT_EQ(QUARANTINE_STATUS_QUARANTINED,
FileRemovalStatusUpdater::GetInstance()->GetQuarantineStatus(path));
const base::FilePath expected_archive_path =
temp_dir_.GetPath().Append(kTestExpectArchiveName);
EXPECT_TRUE(base::PathExists(expected_archive_path));
}
TEST_P(FileRemoverQuarantineTest, FailToQuarantine) {
const base::FilePath path = temp_dir_.GetPath().Append(kTestFileName);
// Acquire exclusive read access to the file, so the archiver can't open the
// file.
base::File file(path, base::File::FLAG_CREATE | base::File::FLAG_READ |
base::File::FLAG_EXCLUSIVE_READ);
ASSERT_TRUE(file.IsValid());
(use_reboot_removal_)
? VerifyRegisterPostRebootRemovalFailure(path, file_remover_.get())
: VerifyRemoveNowFailure(path, file_remover_.get());
EXPECT_EQ(REMOVAL_STATUS_ERROR_IN_ARCHIVER,
FileRemovalStatusUpdater::GetInstance()->GetRemovalStatus(path));
EXPECT_EQ(QUARANTINE_STATUS_ERROR,
FileRemovalStatusUpdater::GetInstance()->GetQuarantineStatus(path));
EXPECT_TRUE(base::PathExists(path));
}
TEST_P(FileRemoverQuarantineTest, DuplicatedFile) {
const base::FilePath path = temp_dir_.GetPath().Append(kTestFileName);
const base::FilePath expected_archive_path =
temp_dir_.GetPath().Append(kTestExpectArchiveName);
CreateFileWithContent(path, kTestContent, strlen(kTestContent));
DoAndExpectCorrespondingRemoval(path);
EXPECT_EQ(QUARANTINE_STATUS_QUARANTINED,
FileRemovalStatusUpdater::GetInstance()->GetQuarantineStatus(path));
EXPECT_TRUE(base::PathExists(expected_archive_path));
// The file should not be archived twice if there is already one with the same
// name and content in the quarantine. So the modified time of the archive
// should not be updated.
base::File::Info old_info;
ASSERT_TRUE(base::GetFileInfo(expected_archive_path, &old_info));
// Recreate the source file and remove it again.
CreateFileWithContent(path, kTestContent, strlen(kTestContent));
DoAndExpectCorrespondingRemoval(path);
// Although the file won't be archived again, it still has a backup in the
// quarantine. So the status should be |QUARANTINE_STATUS_QUARANTINED|.
EXPECT_EQ(QUARANTINE_STATUS_QUARANTINED,
FileRemovalStatusUpdater::GetInstance()->GetQuarantineStatus(path));
EXPECT_TRUE(base::PathExists(expected_archive_path));
base::File::Info new_info;
ASSERT_TRUE(base::GetFileInfo(expected_archive_path, &new_info));
EXPECT_EQ(old_info.last_modified, new_info.last_modified);
}
TEST_P(FileRemoverQuarantineTest, DoNotQuarantineSymbolicLink) {
const base::FilePath path = temp_dir_.GetPath().Append(L"source_temp_file");
CreateFileWithContent(path, kTestContent, strlen(kTestContent));
const base::FilePath sym_path = temp_dir_.GetPath().Append(kTestFileName);
ASSERT_NE(0, ::CreateSymbolicLink(sym_path.AsUTF16Unsafe().c_str(),
path.AsUTF16Unsafe().c_str(), 0));
DoAndExpectCorrespondingRemoval(sym_path);
EXPECT_EQ(
QUARANTINE_STATUS_SKIPPED,
FileRemovalStatusUpdater::GetInstance()->GetQuarantineStatus(sym_path));
const base::FilePath expected_archive_path =
temp_dir_.GetPath().Append(kTestExpectArchiveName);
EXPECT_FALSE(base::PathExists(expected_archive_path));
// The original file should exist.
EXPECT_TRUE(base::PathExists(path));
}
TEST_P(FileRemoverQuarantineTest, QuarantineDefaultFileStream) {
const base::FilePath path = temp_dir_.GetPath().Append(kTestFileName);
CreateFileWithContent(path, kTestContent, strlen(kTestContent));
base::FilePath stream_path(base::StrCat({path.AsUTF16Unsafe(), L"::$data"}));
CreateFileWithContent(stream_path, kTestContent, strlen(kTestContent));
DoAndExpectCorrespondingRemoval(stream_path);
EXPECT_EQ(QUARANTINE_STATUS_QUARANTINED,
FileRemovalStatusUpdater::GetInstance()->GetQuarantineStatus(
stream_path));
const base::FilePath expected_archive_path =
temp_dir_.GetPath().Append(kTestExpectArchiveName);
EXPECT_TRUE(base::PathExists(expected_archive_path));
}
TEST_P(FileRemoverQuarantineTest, DoNotQuarantineNonDefaultFileStream) {
const base::FilePath path = temp_dir_.GetPath().Append(kTestFileName);
CreateFileWithContent(path, kTestContent, strlen(kTestContent));
base::FilePath stream_path(
base::StrCat({path.AsUTF16Unsafe(), L":stream:$data"}));
CreateFileWithContent(stream_path, kTestContent, strlen(kTestContent));
DoAndExpectCorrespondingRemoval(stream_path);
EXPECT_EQ(QUARANTINE_STATUS_SKIPPED,
FileRemovalStatusUpdater::GetInstance()->GetQuarantineStatus(
stream_path));
}
INSTANTIATE_TEST_CASE_P(All,
FileRemoverQuarantineTest,
testing::Bool(),
GetParamNameForTest());
} // namespace chrome_cleaner
......@@ -29,6 +29,7 @@ const char* kSwitchesToPropagate[]{
kChromeVersionSwitch, kDumpRawLogsSwitch, kEnableCrashReportingSwitch,
kEngineSwitch, kExecutionModeSwitch, kLogUploadRetryIntervalSwitch,
kNoSelfDeleteSwitch, kTestingSwitch, kUmaUserSwitch,
kQuarantineSwitch,
};
// The name of the task to run post reboot.
......
......@@ -6,6 +6,7 @@
#include <windows.h>
#include <accctrl.h>
#include <aclapi.h>
#include <lmcons.h>
#include <shellapi.h>
......@@ -14,6 +15,8 @@
#include <utility>
#include <vector>
#include "base/command_line.h"
#include "base/files/file_util.h"
#include "base/logging.h"
#include "base/process/process_info.h"
#include "base/process/process_iterator.h"
......@@ -24,8 +27,13 @@
#include "base/win/scoped_com_initializer.h"
#include "base/win/scoped_handle.h"
#include "base/win/windows_version.h"
#include "chrome/chrome_cleaner/constants/chrome_cleaner_switches.h"
#include "chrome/chrome_cleaner/constants/quarantine_constants.h"
#include "chrome/chrome_cleaner/os/disk_util.h"
#include "chrome/chrome_cleaner/os/scoped_service_handle.h"
#include "chrome/chrome_cleaner/os/system_util.h"
#include "sandbox/win/src/acl.h"
#include "sandbox/win/src/sid.h"
namespace chrome_cleaner {
......@@ -231,6 +239,23 @@ bool SendStopToService(const wchar_t* service_name) {
return true;
}
bool GetQuarantineFolderPath(base::FilePath* output_path) {
DCHECK(output_path);
base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
if (command_line->HasSwitch(kQuarantineDirSwitch)) {
*output_path = command_line->GetSwitchValuePath(kQuarantineDirSwitch);
} else {
base::FilePath product_path;
if (!GetAppDataProductDirectory(&product_path)) {
LOG(ERROR) << "Failed to get AppData product directory.";
return false;
}
*output_path = product_path.Append(kQuarantineFolder);
}
return true;
}
} // namespace
bool AcquireDebugRightsPrivileges() {
......@@ -474,4 +499,49 @@ base::Process LaunchElevatedProcessWithAssociatedWindow(
return runner.GetProcess();
}
// Only allow administrator group to access the quarantine folder.
bool InitializeQuarantineFolder(base::FilePath* output_quarantine_path) {
DCHECK(output_quarantine_path);
base::FilePath quarantine_path;
if (!GetQuarantineFolderPath(&quarantine_path)) {
LOG(ERROR) << "Failed to get quarantine folder path.";
return false;
}
if (!base::DirectoryExists(quarantine_path) &&
!base::CreateDirectory(quarantine_path)) {
LOG(ERROR) << "Failed to create quarantine folder.";
return false;
}
sandbox::Sid admin_sid(WinBuiltinAdministratorsSid);
if (!admin_sid.IsValid()) {
LOG(ERROR) << "Failed to get administrator sid.";
return false;
}
PACL dacl;
if (!sandbox::AddSidToDacl(admin_sid, /*old_dacl=*/nullptr, SET_ACCESS,
GENERIC_ALL, &dacl)) {
LOG(ERROR) << "Failed to create ACLs.";
return false;
}
DWORD result_code = ERROR_SUCCESS;
// |PROTECTED_DACL_SECURITY_INFORMATION| will remove inherited ACLs.
result_code = ::SetNamedSecurityInfo(
const_cast<wchar_t*>(quarantine_path.value().c_str()), SE_FILE_OBJECT,
OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION |
PROTECTED_DACL_SECURITY_INFORMATION,
admin_sid.GetPSID(), /*psidGroup=*/nullptr, dacl, /*pSacl=*/nullptr);
::LocalFree(dacl);
if (result_code != ERROR_SUCCESS) {
LOG(ERROR) << "Failed to set ACLs to quarantine folder.";
return false;
}
*output_quarantine_path = quarantine_path;
return true;
}
} // namespace chrome_cleaner
......@@ -75,6 +75,8 @@ base::Process LaunchElevatedProcessWithAssociatedWindow(
const base::CommandLine& command_line,
HWND hwnd);
bool InitializeQuarantineFolder(base::FilePath* output_quarantine_path);
} // namespace chrome_cleaner
#endif // CHROME_CHROME_CLEANER_OS_SYSTEM_UTIL_CLEANER_H_
......@@ -6,6 +6,8 @@
#include <windows.h>
#include <aclapi.h>
#include <shlobj.h>
#include <shlwapi.h>
#include <stdint.h>
#include <wincrypt.h>
......@@ -14,6 +16,7 @@
#include <string>
#include <vector>
#include "base/base_paths_win.h"
#include "base/command_line.h"
#include "base/files/file.h"
#include "base/files/file_util.h"
......@@ -22,10 +25,15 @@
#include "base/process/launch.h"
#include "base/strings/string16.h"
#include "base/strings/string_util.h"
#include "base/test/scoped_command_line.h"
#include "base/test/scoped_path_override.h"
#include "base/test/test_shortcut_win.h"
#include "base/test/test_timeouts.h"
#include "base/win/shortcut.h"
#include "chrome/chrome_cleaner/constants/chrome_cleaner_switches.h"
#include "chrome/chrome_cleaner/constants/quarantine_constants.h"
#include "chrome/chrome_cleaner/os/disk_util.h"
#include "chrome/chrome_cleaner/os/file_path_sanitization.h"
#include "chrome/chrome_cleaner/os/layered_service_provider_api.h"
#include "chrome/chrome_cleaner/os/layered_service_provider_wrapper.h"
#include "chrome/chrome_cleaner/strings/string_util.h"
......@@ -33,6 +41,7 @@
#include "chrome/chrome_cleaner/test/test_scoped_service_handle.h"
#include "chrome/chrome_cleaner/test/test_strings.h"
#include "chrome/chrome_cleaner/test/test_util.h"
#include "sandbox/win/src/sid.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -127,4 +136,81 @@ TEST_F(ServiceUtilCleanerTest, DISABLED_DeleteRunningService) {
EXPECT_FALSE(IsProcessRunning(kTestServiceExecutableName));
}
TEST_F(ServiceUtilCleanerTest, QuarantineFolderPermission) {
base::ScopedPathOverride local_app_data_override(
CsidlToPathServiceKey(CSIDL_LOCAL_APPDATA));
base::FilePath quarantine_path;
EXPECT_TRUE(InitializeQuarantineFolder(&quarantine_path));
PSID owner_sid;
PACL dacl;
PSECURITY_DESCRIPTOR security_descriptor;
// Get the ownership and ACL of the quarantine folder and check the values.
ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS),
::GetNamedSecurityInfo(
quarantine_path.AsUTF16Unsafe().c_str(), SE_FILE_OBJECT,
OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION,
&owner_sid, /*psidGroup=*/nullptr, &dacl,
/*pSacl=*/nullptr, &security_descriptor));
sandbox::Sid admin_sid(WinBuiltinAdministratorsSid);
ASSERT_TRUE(admin_sid.IsValid());
// Check that the administrator group is the owner.
EXPECT_TRUE(::EqualSid(owner_sid, admin_sid.GetPSID()));
EXPLICIT_ACCESS* explicit_access;
ULONG entry_count;
ASSERT_EQ(static_cast<DWORD>(ERROR_SUCCESS),
::GetExplicitEntriesFromAcl(dacl, &entry_count, &explicit_access));
// ACL should only contains one rule.
ASSERT_EQ(1UL, entry_count);
// Administrator group should have full access.
EXPECT_EQ(static_cast<DWORD>(FILE_ALL_ACCESS),
FILE_ALL_ACCESS & explicit_access[0].grfAccessPermissions);
EXPECT_EQ(static_cast<DWORD>(NO_INHERITANCE),
explicit_access[0].grfInheritance);
EXPECT_EQ(TRUSTEE_IS_SID, explicit_access[0].Trustee.TrusteeForm);
// The trustee of the rule should be administrator group.
EXPECT_TRUE(
::EqualSid(explicit_access[0].Trustee.ptstrName, admin_sid.GetPSID()));
::LocalFree(explicit_access);
::LocalFree(security_descriptor);
}
TEST_F(ServiceUtilCleanerTest, DefaultQuarantineFolderPath) {
base::ScopedPathOverride local_app_data_override(
CsidlToPathServiceKey(CSIDL_LOCAL_APPDATA));
base::FilePath quarantine_path;
EXPECT_TRUE(InitializeQuarantineFolder(&quarantine_path));
base::FilePath product_path;
ASSERT_TRUE(GetAppDataProductDirectory(&product_path));
const base::FilePath default_path = product_path.Append(kQuarantineFolder);
EXPECT_EQ(quarantine_path, default_path);
}
TEST_F(ServiceUtilCleanerTest, SpecifiedQuarantineFolderPath) {
// Override the default path of local appdata, so if we fail to redirect the
// quarantine folder, the test won't drop any file in the real directory.
base::ScopedPathOverride local_app_data_override(
CsidlToPathServiceKey(CSIDL_LOCAL_APPDATA));
base::ScopedTempDir temp_dir;
ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
base::test::ScopedCommandLine scoped_command_line;
scoped_command_line.GetProcessCommandLine()->AppendSwitchPath(
kQuarantineDirSwitch, temp_dir.GetPath());
base::FilePath quarantine_path;
EXPECT_TRUE(InitializeQuarantineFolder(&quarantine_path));
EXPECT_EQ(quarantine_path, temp_dir.GetPath());
}
} // namespace chrome_cleaner
......@@ -134,6 +134,8 @@ source_set("test_util") {
testonly = true
sources = [
"file_remover_test_util.cc",
"file_remover_test_util.h",
"reboot_deletion_helper.cc",
"reboot_deletion_helper.h",
"test_file_util.cc",
......
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#include "chrome/chrome_cleaner/test/file_remover_test_util.h"
#include <utility>
#include "base/bind.h"
#include "base/run_loop.h"
#include "testing/gtest/include/gtest/gtest.h"
namespace chrome_cleaner {
namespace {
void SaveBoolValueCallback(base::OnceClosure run_loop_closure,
bool* result_storage,
bool result) {
*result_storage = result;
std::move(run_loop_closure).Run();
}
void VerifyRemoveNow(const base::FilePath& path,
FileRemoverAPI* remover,
bool expected_result) {
bool returned_result = !expected_result;
base::RunLoop run_loop;
remover->RemoveNow(
path, base::BindOnce(&SaveBoolValueCallback, run_loop.QuitClosure(),
&returned_result));
run_loop.Run();
EXPECT_EQ(expected_result, returned_result);
}
void VerifyRegisterPostRebootRemoval(const base::FilePath& path,
FileRemoverAPI* remover,
bool expected_result) {
bool returned_result = !expected_result;
base::RunLoop run_loop;
remover->RegisterPostRebootRemoval(
path, base::BindOnce(&SaveBoolValueCallback, run_loop.QuitClosure(),
&returned_result));
run_loop.Run();
EXPECT_EQ(expected_result, returned_result);
}
} // namespace
void VerifyRemoveNowSuccess(const base::FilePath& path,
FileRemoverAPI* remover) {
VerifyRemoveNow(path, remover, /*expected_result=*/true);
}
void VerifyRemoveNowFailure(const base::FilePath& path,
FileRemoverAPI* remover) {
VerifyRemoveNow(path, remover, /*expected_result=*/false);
}
void VerifyRegisterPostRebootRemovalSuccess(const base::FilePath& path,
FileRemoverAPI* remover) {
VerifyRegisterPostRebootRemoval(path, remover, /*expected_result=*/true);
}
void VerifyRegisterPostRebootRemovalFailure(const base::FilePath& path,
FileRemoverAPI* remover) {
VerifyRegisterPostRebootRemoval(path, remover, /*expected_result=*/false);
}
} // namespace chrome_cleaner
// Copyright 2018 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
#ifndef CHROME_CHROME_CLEANER_TEST_FILE_REMOVER_TEST_UTIL_H_
#define CHROME_CHROME_CLEANER_TEST_FILE_REMOVER_TEST_UTIL_H_
#include "base/files/file_path.h"
#include "chrome/chrome_cleaner/os/file_remover_api.h"
namespace chrome_cleaner {
void VerifyRemoveNowSuccess(const base::FilePath& path,
FileRemoverAPI* remover);
void VerifyRemoveNowFailure(const base::FilePath& path,
FileRemoverAPI* remover);
void VerifyRegisterPostRebootRemovalSuccess(const base::FilePath& path,
FileRemoverAPI* remover);
void VerifyRegisterPostRebootRemovalFailure(const base::FilePath& path,
FileRemoverAPI* remover);
} // namespace chrome_cleaner
#endif // CHROME_CHROME_CLEANER_TEST_FILE_REMOVER_TEST_UTIL_H_
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment