Skip to content

Commit

Permalink
[chrome_elf] Removing blacklist finch for dynamic dll changes.
Browse files Browse the repository at this point in the history 
Part of chrome_elf cleanup.  This was never used in the wild, and
we won't need it in the foreseeable future.

Leaving beacon and emergency disable finch switch for safety.

Tests:
1) chrome_elf_unittests, chrome_elf_util_unittest.cc: BlacklistTest*
2) unit_tests, chrome_elf_init_unittest_win.cc: ChromeBlacklistTrialTest*
[email protected]
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
BUG=631771

Review-Url: https://codereview.chromium.org/2163803003
Cr-Commit-Position: refs/heads/master@{#408269}
  • Loading branch information
pennymac authored and Commit bot committed Jul 27, 2016
1 parent 770b48e commit c4da848
Show file tree
Hide file tree
Showing 12 changed files with 14 additions and 231 deletions.
13 changes: 0 additions & 13 deletions chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,18 +234,6 @@ void RecordLspFeature(ClientIncidentReport_EnvironmentData_Process* process) {
}
}

void CollectDllBlacklistData(
ClientIncidentReport_EnvironmentData_Process* process) {
PathSanitizer path_sanitizer;
base::win::RegistryValueIterator iter(HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath);
for (; iter.Valid(); ++iter) {
base::FilePath dll_name(iter.Value());
path_sanitizer.StripHomeDirectory(&dll_name);
process->add_blacklisted_dll(dll_name.AsUTF8Unsafe());
}
}

void CollectModuleVerificationData(
const wchar_t* const modules_to_verify[],
size_t num_modules_to_verify,
Expand Down Expand Up @@ -311,7 +299,6 @@ void CollectPlatformProcessData(
ClientIncidentReport_EnvironmentData_Process* process) {
CollectDlls(process);
RecordLspFeature(process);
CollectDllBlacklistData(process);
CollectModuleVerificationData(
kModulesToVerify, arraysize(kModulesToVerify), process);
}
Expand Down
49 changes: 0 additions & 49 deletions chrome/browser/safe_browsing/incident_reporting/environment_data_collection_win_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ namespace safe_browsing {

namespace {

const wchar_t test_dll[] = L"test_name.dll";

// Returns true if a dll with filename |dll_name| is found in |process_report|,
// providing a copy of it in |result|.
bool GetProcessReportDll(
Expand Down Expand Up @@ -125,53 +123,6 @@ TEST(SafeBrowsingEnvironmentDataCollectionWinTest, RecordLspFeature) {
FAIL() << "No LSP feature found for " << lsp;
}

TEST(SafeBrowsingEnvironmentDataCollectionWinTest, CollectDllBlacklistData) {
// Ensure that CollectDllBlacklistData correctly adds the set of sanitized dll
// names currently stored in the registry to the report.
registry_util::RegistryOverrideManager override_manager;
override_manager.OverrideRegistry(HKEY_CURRENT_USER);

base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath,
KEY_QUERY_VALUE | KEY_SET_VALUE);

// Check that with an empty registry the blacklisted dlls field is left empty.
ClientIncidentReport_EnvironmentData_Process process_report;
CollectDllBlacklistData(&process_report);
EXPECT_EQ(0, process_report.blacklisted_dll_size());

// Check that after adding exactly one dll to the registry it appears in the
// process report.
blacklist_registry_key.WriteValue(test_dll, test_dll);
CollectDllBlacklistData(&process_report);
ASSERT_EQ(1, process_report.blacklisted_dll_size());

base::string16 process_report_dll =
base::UTF8ToWide(process_report.blacklisted_dll(0));
EXPECT_EQ(base::string16(test_dll), process_report_dll);

// Check that if the registry contains the full path to a dll it is properly
// sanitized before being reported.
blacklist_registry_key.DeleteValue(test_dll);
process_report.clear_blacklisted_dll();

base::FilePath path;
ASSERT_TRUE(PathService::Get(base::DIR_HOME, &path));
base::string16 input_path =
path.Append(FILE_PATH_LITERAL("test_path.dll")).value();

std::string path_expected = base::FilePath(FILE_PATH_LITERAL("~"))
.Append(FILE_PATH_LITERAL("test_path.dll"))
.AsUTF8Unsafe();

blacklist_registry_key.WriteValue(input_path.c_str(), input_path.c_str());
CollectDllBlacklistData(&process_report);

ASSERT_EQ(1, process_report.blacklisted_dll_size());
std::string process_report_path = process_report.blacklisted_dll(0);
EXPECT_EQ(path_expected, process_report_path);
}

#if !defined(_WIN64)
TEST(SafeBrowsingEnvironmentDataCollectionWinTest, VerifyLoadedModules) {
// Load the test modules.
Expand Down
37 changes: 0 additions & 37 deletions chrome/browser/win/chrome_elf_init.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ void InitializeChromeElf() {
base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER);
blacklist_registry_key.DeleteKey(blacklist::kRegistryBeaconPath);
} else {
AddFinchBlacklistToRegistry();
BrowserBlacklistBeaconSetup();
}

Expand All @@ -112,42 +111,6 @@ void InitializeChromeElf() {
base::TimeDelta::FromSeconds(kBlacklistReportingDelaySec));
}

// Note that running multiple chrome instances with distinct user data
// directories could lead to deletion (and/or replacement) of the finch
// blacklist registry data in one instance before the second has a chance to
// read those values.
void AddFinchBlacklistToRegistry() {
base::win::RegKey finch_blacklist_registry_key(
HKEY_CURRENT_USER, blacklist::kRegistryFinchListPath, KEY_SET_VALUE);

// No point in trying to continue if the registry key isn't valid.
if (!finch_blacklist_registry_key.Valid())
return;

// Delete and recreate the key to clear the registry.
finch_blacklist_registry_key.DeleteKey(L"");
finch_blacklist_registry_key.Create(
HKEY_CURRENT_USER, blacklist::kRegistryFinchListPath, KEY_SET_VALUE);

std::map<std::string, std::string> params;
std::string value = variations::GetVariationParamValue(
kBrowserBlacklistTrialName, blacklist::kRegistryFinchListValueNameStr);
if (value.empty())
return;
base::string16 value_wcs = base::UTF8ToWide(value);

// The dll names are comma-separated in this param value. We need to turn
// this into REG_MULTI_SZ format (double-null terminates).
// Note that the strings are wide character in registry.
value_wcs.push_back(L'\0');
value_wcs.push_back(L'\0');
std::replace(value_wcs.begin(), value_wcs.end(), L',', L'\0');

finch_blacklist_registry_key.WriteValue(
blacklist::kRegistryFinchListValueName, value_wcs.data(),
(value_wcs.size() * sizeof(wchar_t)), REG_MULTI_SZ);
}

void BrowserBlacklistBeaconSetup() {
base::win::RegKey blacklist_registry_key(HKEY_CURRENT_USER,
blacklist::kRegistryBeaconPath,
Expand Down
3 changes: 0 additions & 3 deletions chrome/browser/win/chrome_elf_init.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ extern const char kBrowserBlacklistTrialDisabledGroupName[];
// only affect future runs since Chrome Elf is already setup by this point).
void InitializeChromeElf();

// Add the blacklist from the finch configs in the registry.
void AddFinchBlacklistToRegistry();

// Set the required state for an enabled browser blacklist.
void BrowserBlacklistBeaconSetup();

Expand Down
43 changes: 0 additions & 43 deletions chrome/browser/win/chrome_elf_init_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@

namespace {

const char kBrowserBlacklistTrialEnabledGroupName[] = "Enabled";

class ChromeBlacklistTrialTest : public testing::Test {
protected:
ChromeBlacklistTrialTest() {}
Expand Down Expand Up @@ -156,45 +154,4 @@ TEST_F(ChromeBlacklistTrialTest, VersionChanged) {
ASSERT_EQ(static_cast<DWORD>(0), attempt_count);
}

TEST_F(ChromeBlacklistTrialTest, AddFinchBlacklistToRegistry) {
// Create the field trial with the blacklist enabled group.
base::FieldTrialList field_trial_list(
new metrics::SHA1EntropyProvider("test"));

scoped_refptr<base::FieldTrial> trial(base::FieldTrialList::CreateFieldTrial(
kBrowserBlacklistTrialName, kBrowserBlacklistTrialEnabledGroupName));

// Set up the trial with the desired parameters.
std::map<std::string, std::string> desired_params;

desired_params[blacklist::kRegistryFinchListValueNameStr] =
"TestDll1.dll,TestDll2.dll";

variations::AssociateVariationParams(
kBrowserBlacklistTrialName,
kBrowserBlacklistTrialEnabledGroupName,
desired_params);

// This should add the dlls in those parameters to the registry.
AddFinchBlacklistToRegistry();

// Check that all the dll names in desired_params were added to the registry.
std::vector<std::wstring> dlls;

base::win::RegKey finch_blacklist_registry_key(
HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath,
KEY_QUERY_VALUE | KEY_SET_VALUE);

ASSERT_TRUE(finch_blacklist_registry_key.HasValue(
blacklist::kRegistryFinchListValueName));
ASSERT_EQ(ERROR_SUCCESS, finch_blacklist_registry_key.ReadValues(
blacklist::kRegistryFinchListValueName, &dlls));

ASSERT_EQ((size_t)2,
/* Number of dll names passed in this test. */ dlls.size());
EXPECT_STREQ(L"TestDll1.dll", dlls[0].c_str());
EXPECT_STREQ(L"TestDll2.dll", dlls[1].c_str());
}

} // namespace
11 changes: 10 additions & 1 deletion chrome/installer/setup/uninstall.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -786,8 +786,17 @@ void RemoveBlacklistState() {
InstallUtil::DeleteRegistryKey(HKEY_CURRENT_USER,
blacklist::kRegistryBeaconPath,
0); // wow64_access
// The following key is no longer used (https://crbug.com/631771).
// This cleanup is being left in for a time though.
#if defined(GOOGLE_CHROME_BUILD)
const wchar_t kRegistryFinchListPath[] =
L"SOFTWARE\\Google\\Chrome\\BLFinchList";
#else
const wchar_t kRegistryFinchListPath[] =
L"SOFTWARE\\Chromium\\BLFinchList";
#endif
InstallUtil::DeleteRegistryKey(HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath,
kRegistryFinchListPath,
0); // wow64_access
}

Expand Down
18 changes: 0 additions & 18 deletions chrome_elf/blacklist/blacklist.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -347,25 +347,7 @@ bool Initialize(bool force) {
VirtualProtect(&g_thunk_storage, sizeof(g_thunk_storage),
PAGE_EXECUTE_READ, &old_protect);

AddDllsFromRegistryToBlacklist();

return NT_SUCCESS(ret) && page_executable;
}

void AddDllsFromRegistryToBlacklist() {
std::vector<std::wstring> dlls;

if (!nt::QueryRegValueMULTISZ(nt::HKCU, kRegistryFinchListPath,
kRegistryFinchListValueName, &dlls) ||
dlls.empty())
return;

// Add each DLL to the BL in memory
for (auto name : dlls) {
AddDllToBlacklist(name.c_str());
}

return;
}

} // namespace blacklist
4 changes: 0 additions & 4 deletions chrome_elf/blacklist/blacklist.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,6 @@ extern "C" bool RemoveDllFromBlacklist(const wchar_t* dll_name);
// is only exposed in tests (and should stay that way).
extern "C" void SuccessfullyBlocked(const wchar_t** blocked_dlls, int* size);

// Add the dlls, originally passed in through finch, from the registry to the
// blacklist so that they will be blocked identically to those hard coded in.
extern "C" void AddDllsFromRegistryToBlacklist();

// Record that the dll at the given index was blocked.
extern "C" void BlockedDll(size_t blocked_index);

Expand Down
51 changes: 4 additions & 47 deletions chrome_elf/blacklist/test/blacklist_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ extern const wchar_t* kEnvVars[];
namespace {

// Functions we need from blacklist_test_main_dll.dll
typedef void (*TestDll_AddDllsFromRegistryToBlacklistFunction)();
typedef bool (*TestDll_AddDllToBlacklistFunction)(const wchar_t* dll_name);
typedef int (*TestDll_BlacklistSizeFunction)();
typedef void (*TestDll_BlockedDllFunction)(size_t blocked_index);
Expand All @@ -49,8 +48,6 @@ typedef bool (*TestDll_SuccessfullyBlockedFunction)(
int* size);
typedef void (*InitTestDllFunction)();

TestDll_AddDllsFromRegistryToBlacklistFunction
TestDll_AddDllsFromRegistryToBlacklist = nullptr;
TestDll_AddDllToBlacklistFunction TestDll_AddDllToBlacklist = nullptr;
TestDll_BlacklistSizeFunction TestDll_BlacklistSize = nullptr;
TestDll_BlockedDllFunction TestDll_BlockedDll = nullptr;
Expand Down Expand Up @@ -160,9 +157,6 @@ class BlacklistTest : public testing::Test {
dll = ::LoadLibraryW(L"blacklist_test_main_dll.dll");
if (!dll)
return;
TestDll_AddDllsFromRegistryToBlacklist =
reinterpret_cast<TestDll_AddDllsFromRegistryToBlacklistFunction>(
::GetProcAddress(dll, "TestDll_AddDllsFromRegistryToBlacklist"));
TestDll_AddDllToBlacklist =
reinterpret_cast<TestDll_AddDllToBlacklistFunction>(
::GetProcAddress(dll, "TestDll_AddDllToBlacklist"));
Expand All @@ -184,11 +178,10 @@ class BlacklistTest : public testing::Test {
::GetProcAddress(dll, "TestDll_SuccessfullyBlocked"));
InitTestDll = reinterpret_cast<InitTestDllFunction>(
::GetProcAddress(dll, "InitTestDll"));
if (!TestDll_AddDllsFromRegistryToBlacklist || !TestDll_AddDllToBlacklist ||
!TestDll_BlacklistSize || !TestDll_BlockedDll ||
!TestDll_GetBlacklistIndex || !TestDll_IsBlacklistInitialized ||
!TestDll_RemoveDllFromBlacklist || !TestDll_SuccessfullyBlocked ||
!InitTestDll)
if (!TestDll_AddDllToBlacklist || !TestDll_BlacklistSize ||
!TestDll_BlockedDll || !TestDll_GetBlacklistIndex ||
!TestDll_IsBlacklistInitialized || !TestDll_RemoveDllFromBlacklist ||
!TestDll_SuccessfullyBlocked || !InitTestDll)
return;

// We have to call this exported function every time this test setup runs.
Expand Down Expand Up @@ -316,42 +309,6 @@ TEST_F(BlacklistTest, LoadBlacklistedLibrary) {
CheckBlacklistedDllsNotLoaded();
}

TEST_F(BlacklistTest, AddDllsFromRegistryToBlacklist) {
// Ensure that the blacklist is loaded.
ASSERT_TRUE(TestDll_IsBlacklistInitialized());

// Delete the finch registry key to clear its values.
base::win::RegKey key(HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath,
KEY_QUERY_VALUE | KEY_SET_VALUE);
key.DeleteKey(L"");

// Add the test dlls to the registry.
// (REG_MULTI_SZ: eos separated, double null terminated.)
base::win::RegKey finch_blacklist_registry_key(
HKEY_CURRENT_USER,
blacklist::kRegistryFinchListPath,
KEY_QUERY_VALUE | KEY_SET_VALUE);

std::vector<wchar_t>(reg_buffer);
for (size_t i = 0; i < arraysize(test_data); ++i) {
if (reg_buffer.size() > 0)
reg_buffer.push_back(L'\0');
const wchar_t* dll = test_data[i].dll_name;
// Append the name, not including terminator.
reg_buffer.insert(reg_buffer.end(), dll, dll + ::wcslen(dll));
}
reg_buffer.push_back(L'\0');
reg_buffer.push_back(L'\0');

finch_blacklist_registry_key.WriteValue(
blacklist::kRegistryFinchListValueName, reg_buffer.data(),
(DWORD)(reg_buffer.size() * sizeof(wchar_t)), REG_MULTI_SZ);

TestDll_AddDllsFromRegistryToBlacklist();
CheckBlacklistedDllsNotLoaded();
}

void TestResetBeacon(std::unique_ptr<base::win::RegKey>& key,
DWORD input_state,
DWORD expected_output_state) {
Expand Down
1 change: 0 additions & 1 deletion chrome_elf/blacklist/test/blacklist_test_main_dll.def
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
LIBRARY "blacklist_test_main_dll.dll"

EXPORTS
TestDll_AddDllsFromRegistryToBlacklist=AddDllsFromRegistryToBlacklist
TestDll_AddDllToBlacklist=AddDllToBlacklist
TestDll_BlacklistSize=BlacklistSize
TestDll_BlockedDll=BlockedDll
Expand Down
5 changes: 0 additions & 5 deletions chrome_elf/chrome_elf_constants.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@ namespace blacklist {

const wchar_t kRegistryBeaconPath[] =
L"SOFTWARE\\" PRODUCT_STRING_PATH L"\\BLBeacon";
const wchar_t kRegistryFinchListPath[] =
L"SOFTWARE\\" PRODUCT_STRING_PATH L"\\BLFinchList";
const char kRegistryFinchListValueNameStr[] = "BLDlls";
const wchar_t kRegistryFinchListValueName[] = L"BLDlls";
const wchar_t kBeaconVersion[] = L"version";
const wchar_t kBeaconState[] = L"state";
const wchar_t kBeaconAttemptCount[] = L"failed_count";
Expand All @@ -35,7 +31,6 @@ namespace elf_sec {

const wchar_t kRegSecurityFinchPath[] =
L"SOFTWARE\\" PRODUCT_STRING_PATH L"\\BrowserSboxFinch";

const wchar_t kRegSecurityPath[] =
L"SOFTWARE\\" PRODUCT_STRING_PATH L"\\BrowserSec";

Expand Down
10 changes: 0 additions & 10 deletions chrome_elf/chrome_elf_constants.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,6 @@ namespace blacklist {
// The registry path of the blacklist beacon.
extern const wchar_t kRegistryBeaconPath[];

// The registry path of the finch blacklist dlls.
extern const wchar_t kRegistryFinchListPath[];

// The registry value name for the REG_MULTI_SZ list of blacklist dlls.
// Note the char version is handy for use as the param name when
// appending dll names to the base::FieldTrial. Can be removed
// if no longer used.
extern const char kRegistryFinchListValueNameStr[];
extern const wchar_t kRegistryFinchListValueName[];

// The properties for the blacklist beacon.
extern const wchar_t kBeaconVersion[];
extern const wchar_t kBeaconState[];
Expand Down

0 comments on commit c4da848

Please sign in to comment.