[chrome_elf whitelist] Adjust whitelist file support.

- kFileNotFound and kArraySizeZero now treated as kSuccess.
- Updated shared definitions in whitelist_packed_format source set.  New timestamp
to track last load attempt in local cache.  Added relative path and file name too.
- Decision made to check is_sorted on array during init.
The array will be smaller than in original design, so speed hit will be minimal.
- New tests.

BUG=769590
TEST=chrome_elf_unittests.exe, WhitelistFileTest.*

Change-Id: I45c4321ccdf89d8768509c0dad21c9c15f73c4e3
Reviewed-on: https://chromium-review.googlesource.com/794551
Commit-Queue: Penny MacNeil <pennymac@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523544}

chrome_elf/BUILD.gn

@@ -137,6 +137,13 @@ source_set("sha1") {
   ]
 }
 
+source_set("whitelist_packed_format") {
+  sources = [
+    "whitelist/whitelist_packed_format.cc",
+    "whitelist/whitelist_packed_format.h",
+  ]
+}
+
 ##------------------------------------------------------------------------------
 ## chrome_elf sub targets
 ##------------------------------------------------------------------------------
@@ -231,13 +238,13 @@ static_library("whitelist") {
     "whitelist/whitelist.h",
     "whitelist/whitelist_file.cc",
     "whitelist/whitelist_file.h",
-    "whitelist/whitelist_file_format.h",
     "whitelist/whitelist_ime.cc",
     "whitelist/whitelist_ime.h",
   ]
   deps = [
     ":nt_registry",
     ":sha1",
+    ":whitelist_packed_format",
     "//chrome/install_static:install_static_util",
   ]
 }
@@ -270,6 +277,7 @@ test("chrome_elf_unittests") {
     ":security",
     ":sha1",
     ":whitelist",
+    ":whitelist_packed_format",
     "//base",
     "//base/test:test_support",
     "//chrome/common:version_header",

chrome_elf/whitelist/whitelist_file.cc

@@ -14,32 +14,22 @@
 
 #include "chrome/install_static/user_data_dir.h"
 #include "chrome_elf/sha1/sha1.h"
-#include "chrome_elf/whitelist/whitelist_file_format.h"
+#include "chrome_elf/whitelist/whitelist_packed_format.h"
 
 namespace whitelist {
 namespace {
 
-// TODO(pennymac): update subdir and filename with final shared defines.
-constexpr wchar_t kFileSubdir[] =
-    L"\\ThirdPartyModuleList"
-#if defined(_WIN64)
-    L"64";
-#else
-    L"32";
-#endif
-constexpr wchar_t kFileName[] = L"\\dbfile";
-
 // No concern about concurrency control in chrome_elf.
 bool g_initialized = false;
 
-// This will hold a packed whitelist module array, read directly from a
-// component-update file during InitComponent().
-PackedWhitelistModule* g_module_array = nullptr;
-size_t g_module_array_size = 0;
+// This will hold a packed blacklist module array, read directly from a
+// data file during InitComponent().
+PackedWhitelistModule* g_bl_module_array = nullptr;
+size_t g_bl_module_array_size = 0;
 
 // NOTE: this "global" is only initialized once during InitComponent().
-// NOTE: this is wrapped in a function to prevent exit-time dtors.
-std::wstring& GetFilePath() {
+// NOTE: it is wrapped in a function to prevent exit-time dtors.
+std::wstring& GetBlFilePath() {
   static std::wstring* const file_path = new std::wstring();
   return *file_path;
 }
@@ -71,10 +61,12 @@ bool HashBinaryPredicate(const PackedWhitelistModule& lhs,
   return CompareHashes(lhs.basename_hash, rhs.basename_hash) < 0;
 }
 
-// Given a file opened for read, pull in the packed whitelist.
+// Given a file opened for read, pull in the packed list.
 //
 // - Returns kSuccess or kArraySizeZero on success.
-FileStatus ReadInArray(HANDLE file) {
+FileStatus ReadInArray(HANDLE file,
+                       size_t* array_size,
+                       PackedWhitelistModule** array_ptr) {
   PackedWhitelistMetadata metadata;
   DWORD bytes_read = 0;
 
@@ -88,71 +80,98 @@ FileStatus ReadInArray(HANDLE file) {
   if (metadata.version != PackedWhitelistVersion::kCurrent)
     return FileStatus::kInvalidFormatVersion;
 
-  g_module_array_size = metadata.module_count;
+  *array_size = metadata.module_count;
   // Check for size 0.
-  if (!g_module_array_size)
+  if (!*array_size)
     return FileStatus::kArraySizeZero;
 
   // Sanity check the array fits in a DWORD.
-  if (g_module_array_size >
+  if (*array_size >
       (std::numeric_limits<DWORD>::max() / sizeof(PackedWhitelistModule))) {
     assert(false);
     return FileStatus::kArrayTooBig;
   }
 
   DWORD buffer_size =
-      static_cast<DWORD>(g_module_array_size * sizeof(PackedWhitelistModule));
-  g_module_array =
+      static_cast<DWORD>(*array_size * sizeof(PackedWhitelistModule));
+  *array_ptr =
       reinterpret_cast<PackedWhitelistModule*>(new uint8_t[buffer_size]);
 
   // Read in the array.
   // NOTE: Ignore the rest of the file - other data could be stored at the end.
-  if (!::ReadFile(file, g_module_array, buffer_size, &bytes_read, FALSE) ||
+  if (!::ReadFile(file, *array_ptr, buffer_size, &bytes_read, FALSE) ||
       bytes_read != buffer_size) {
-    delete[] g_module_array;
-    g_module_array = nullptr;
-    g_module_array_size = 0;
+    delete[] * array_ptr;
+    *array_ptr = nullptr;
+    *array_size = 0;
     return FileStatus::kArrayReadFail;
   }
-  // TODO(pennymac): calculate cost of is_sorted() call against real database
-  // array. Is it too expensive for startup?  Maybe do a delayed check?
-  if (!std::is_sorted(g_module_array, g_module_array + g_module_array_size,
+
+  // Ensure array is sorted (as expected).
+  if (!std::is_sorted(*array_ptr, *array_ptr + *array_size,
                       HashBinaryPredicate)) {
-    delete[] g_module_array;
-    g_module_array = nullptr;
-    g_module_array_size = 0;
+    delete[] * array_ptr;
+    *array_ptr = nullptr;
+    *array_size = 0;
     return FileStatus::kArrayNotSorted;
   }
 
   return FileStatus::kSuccess;
 }
 
-// Example file location, relative to user data dir.
-// %localappdata% / Google / Chrome SxS / User Data / ThirdPartyModuleList64 /
+// Open a packed data file.
 //
-// TODO(pennymac): Missing or malformed component file shouldn't happen.
-//                 Possible UMA log in future.
-FileStatus InitInternal() {
-  std::wstring& file_path = GetFilePath();
+// - Returns kSuccess or kFileNotFound on success.
+FileStatus OpenDataFile(HANDLE* file_handle) {
+  *file_handle = INVALID_HANDLE_VALUE;
+  std::wstring& file_path = GetBlFilePath();
+
   // The path may have been overridden for testing.
   if (file_path.empty()) {
     if (!install_static::GetUserDataDirectory(&file_path, nullptr))
       return FileStatus::kUserDataDirFail;
     file_path.append(kFileSubdir);
-    file_path.append(kFileName);
+    file_path.append(kBlFileName);
   }
 
   // See if file exists.  INVALID_HANDLE_VALUE alert!
-  HANDLE file =
+  *file_handle =
       ::CreateFileW(file_path.c_str(), FILE_READ_DATA,
                     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
                     nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
-  if (file == INVALID_HANDLE_VALUE)
-    return FileStatus::kFileNotFound;
+  if (*file_handle == INVALID_HANDLE_VALUE) {
+    switch (::GetLastError()) {
+      case ERROR_FILE_NOT_FOUND:
+      case ERROR_PATH_NOT_FOUND:
+        return FileStatus::kFileNotFound;
+      case ERROR_ACCESS_DENIED:
+        return FileStatus::kFileAccessDenied;
+      default:
+        return FileStatus::kFileUnexpectedFailure;
+    }
+  }
 
-  FileStatus status = ReadInArray(file);
-  ::CloseHandle(file);
+  return FileStatus::kSuccess;
+}
+
+// Example file location, relative to user data dir.
+// %localappdata% / Google / Chrome SxS / User Data / ThirdPartyModuleList64 /
+//
+// - NOTE: kFileNotFound and kArraySizeZero are treated as kSuccess.
+FileStatus InitInternal() {
+  // blacklist
+  // ---------
+  HANDLE handle = INVALID_HANDLE_VALUE;
+  FileStatus status = OpenDataFile(&handle);
+  if (status == FileStatus::kFileNotFound)
+    return FileStatus::kSuccess;
+  if (status == FileStatus::kSuccess) {
+    status = ReadInArray(handle, &g_bl_module_array_size, &g_bl_module_array);
+    ::CloseHandle(handle);
+  }
 
+  if (status == FileStatus::kArraySizeZero)
+    return FileStatus::kSuccess;
   return status;
 }
 
@@ -162,11 +181,11 @@ FileStatus InitInternal() {
 // Public defines & functions
 //------------------------------------------------------------------------------
 
-bool IsModuleWhitelisted(const std::string& basename,
-                         DWORD image_size,
-                         DWORD time_date_stamp) {
+bool IsModuleListed(const std::string& basename,
+                    DWORD image_size,
+                    DWORD time_date_stamp) {
   assert(g_initialized);
-  if (!g_module_array_size)
+  if (!g_bl_module_array_size)
     return false;
 
   // Max hex 32-bit value is 8 characters long.  2*8+1.
@@ -181,9 +200,9 @@ bool IsModuleWhitelisted(const std::string& basename,
 
   // Binary search for primary hash (basename).  There can be more than one
   // match.
-  auto pair =
-      std::equal_range(g_module_array, g_module_array + g_module_array_size,
-                       target, HashBinaryPredicate);
+  auto pair = std::equal_range(g_bl_module_array,
+                               g_bl_module_array + g_bl_module_array_size,
+                               target, HashBinaryPredicate);
 
   // Search for secondary hash.
   for (PackedWhitelistModule* i = pair.first; i != pair.second; ++i) {
@@ -195,16 +214,16 @@ bool IsModuleWhitelisted(const std::string& basename,
   return false;
 }
 
-std::wstring GetFilePathUsed() {
-  return GetFilePath();
+std::wstring GetBlFilePathUsed() {
+  return GetBlFilePath();
 }
 
-// Grab the latest whitelist file.
+// Grab the latest whitelist and blacklist.
 FileStatus InitFromFile() {
   // Debug check: InitFromFile should not be called more than once.
   assert(!g_initialized);
 
-  // TODO(pennymac): Should kArraySizeZero be a failure?  Or just UMA?
+  // TODO(pennymac): Possible UMA log for unexpected failures.
   FileStatus status = InitInternal();
 
   if (status == FileStatus::kSuccess)
@@ -213,8 +232,8 @@ FileStatus InitFromFile() {
   return status;
 }
 
-void OverrideFilePathForTesting(const std::wstring& new_path) {
-  GetFilePath().assign(new_path);
+void OverrideFilePathForTesting(const std::wstring& new_bl_path) {
+  GetBlFilePath().assign(new_bl_path);
 }
 
 }  // namespace whitelist

chrome_elf/whitelist/whitelist_file.h

@@ -16,29 +16,31 @@ enum class FileStatus {
   kSuccess = 0,
   kUserDataDirFail = 1,
   kFileNotFound = 2,
-  kMetadataReadFail = 3,
-  kInvalidFormatVersion = 4,
-  kArraySizeZero = 5,
-  kArrayTooBig = 6,
-  kArrayReadFail = 7,
-  kArrayNotSorted = 8,
+  kFileAccessDenied = 3,
+  kFileUnexpectedFailure = 4,
+  kMetadataReadFail = 5,
+  kInvalidFormatVersion = 6,
+  kArraySizeZero = 7,
+  kArrayTooBig = 8,
+  kArrayReadFail = 9,
+  kArrayNotSorted = 10,
   COUNT
 };
 
 // Look up a binary based on the required data points.
-// - Returns true if match found in whitelist.  I.E. Allow module load.
-bool IsModuleWhitelisted(const std::string& basename,
-                         DWORD image_size,
-                         DWORD time_date_stamp);
+// - Returns true if match found in the blacklist.  I.E. Block if true.
+bool IsModuleListed(const std::string& basename,
+                    DWORD image_size,
+                    DWORD time_date_stamp);
 
-// Get the full path of the whitelist file used.
-std::wstring GetFilePathUsed();
+// Get the full path of the blacklist file used.
+std::wstring GetBlFilePathUsed();
 
 // Initialize internal whitelist from file.
 FileStatus InitFromFile();
 
-// Sets the path to the whitelist file for use by tests.
-void OverrideFilePathForTesting(const std::wstring& new_path);
+// Overrides the blacklist path for use by tests.
+void OverrideFilePathForTesting(const std::wstring& new_bl_path);
 
 }  // namespace whitelist
 

chrome_elf/whitelist/whitelist_file_unittest.cc

@@ -17,13 +17,13 @@
 #include "base/win/pe_image.h"
 #include "chrome/install_static/user_data_dir.h"
 #include "chrome_elf/sha1/sha1.h"
-#include "chrome_elf/whitelist/whitelist_file_format.h"
+#include "chrome_elf/whitelist/whitelist_packed_format.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace whitelist {
 namespace {
 
-constexpr wchar_t kFileName[] = L"dbfile";
+constexpr wchar_t kTestBlFileName[] = L"blfile";
 constexpr DWORD kPageSize = 4096;
 
 struct TestModule {
@@ -87,7 +87,7 @@ bool GetTestModules(std::vector<TestModule>* test_modules,
 }
 
 //------------------------------------------------------------------------------
-// WhitelistComponentTest class
+// WhitelistFileTest class
 //------------------------------------------------------------------------------
 
 class WhitelistFileTest : public testing::Test {
@@ -95,74 +95,113 @@ class WhitelistFileTest : public testing::Test {
   WhitelistFileTest() = default;
 
   void SetUp() override {
-    std::vector<PackedWhitelistModule> test_file_array;
-    ASSERT_TRUE(GetTestModules(&test_array_, &test_file_array));
+    ASSERT_TRUE(GetTestModules(&test_array_, &test_packed_array_));
 
-    // Setup test component file.
+    // Setup temp test dir.
     ASSERT_TRUE(scoped_temp_dir_.CreateUniqueTempDir());
+
+    // Store full path to test file (without creating it yet).
     base::FilePath path = scoped_temp_dir_.GetPath();
+    path = path.Append(kTestBlFileName);
+    bl_test_file_path_ = std::move(path.value());
 
-    // Create the component file.  It will be cleaned up with
-    // |scoped_temp_dir_|.
-    path = path.Append(kFileName);
-    base::File file(path, base::File::FLAG_CREATE_ALWAYS |
-                              base::File::FLAG_WRITE |
-                              base::File::FLAG_SHARE_DELETE |
-                              base::File::FLAG_DELETE_ON_CLOSE);
-    ASSERT_TRUE(file.IsValid());
+    // Override the file paths in the live code for testing.
+    OverrideFilePathForTesting(bl_test_file_path_);
+  }
 
-    // Store the path for tests to use.
-    test_file_path_ = std::move(path.value());
+  void CreateTestFile() {
+    base::File file(base::FilePath(bl_test_file_path_),
+                    base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE |
+                        base::File::FLAG_SHARE_DELETE |
+                        base::File::FLAG_DELETE_ON_CLOSE);
+    ASSERT_TRUE(file.IsValid());
 
     // Write content {metadata}{array_of_modules}.
     PackedWhitelistMetadata meta = {
-        kInitialVersion, static_cast<uint32_t>(test_file_array.size())};
-    ASSERT_TRUE(file.Write(0, reinterpret_cast<const char*>(&meta),
-                           sizeof(meta)) == sizeof(meta));
-    int size = static_cast<int>(test_file_array.size() *
+        kInitialVersion, static_cast<uint32_t>(test_packed_array_.size())};
+    ASSERT_EQ(file.Write(0, reinterpret_cast<const char*>(&meta), sizeof(meta)),
+              static_cast<int>(sizeof(meta)));
+    int size = static_cast<int>(test_packed_array_.size() *
                                 sizeof(PackedWhitelistModule));
-    ASSERT_TRUE(
+    ASSERT_EQ(
         file.Write(sizeof(PackedWhitelistMetadata),
-                   reinterpret_cast<const char*>(test_file_array.data()),
-                   size) == size);
+                   reinterpret_cast<const char*>(test_packed_array_.data()),
+                   size),
+        size);
 
     // Leave file handle open for DELETE_ON_CLOSE.
-    file_ = std::move(file);
+    bl_file_ = std::move(file);
   }
 
-  const base::string16& GetTestFilePath() { return test_file_path_; }
+  const base::string16& GetBlTestFilePath() { return bl_test_file_path_; }
+
+  base::File* GetBlFile() { return &bl_file_; }
 
   const std::vector<TestModule>& GetTestArray() { return test_array_; }
 
  private:
   base::ScopedTempDir scoped_temp_dir_;
-  base::File file_;
-  base::string16 test_file_path_;
+  base::File bl_file_;
+  base::string16 bl_test_file_path_;
   std::vector<TestModule> test_array_;
+  std::vector<PackedWhitelistModule> test_packed_array_;
 
   DISALLOW_COPY_AND_ASSIGN(WhitelistFileTest);
 };
 
 //------------------------------------------------------------------------------
-// Whitelist component tests
+// Whitelist file tests
 //------------------------------------------------------------------------------
 
-// Test initialization of the whitelist from file.
-TEST_F(WhitelistFileTest, Init) {
-  // Override the component file path for testing.
-  OverrideFilePathForTesting(GetTestFilePath());
+// Test successful initialization and module lookup.
+TEST_F(WhitelistFileTest, Success) {
+  // Create blacklist data file.
+  CreateTestFile();
 
-  // Init component whitelist!
+  // Init.
   ASSERT_EQ(InitFromFile(), FileStatus::kSuccess);
 
   // Test matching.
   for (const auto& test_module : GetTestArray()) {
-    ASSERT_TRUE(IsModuleWhitelisted(test_module.basename, test_module.imagesize,
-                                    test_module.timedatestamp));
+    EXPECT_TRUE(IsModuleListed(test_module.basename, test_module.imagesize,
+                               test_module.timedatestamp));
   }
 
   // Test a failure to match.
-  ASSERT_FALSE(IsModuleWhitelisted("booya.dll", 1337, 0x12345678));
+  EXPECT_FALSE(IsModuleListed("booya.dll", 1337, 0x12345678));
+}
+
+// Test successful initialization with no packed files.
+TEST_F(WhitelistFileTest, NoFiles) {
+  ASSERT_EQ(InitFromFile(), FileStatus::kSuccess);
+  EXPECT_FALSE(IsModuleListed("booya.dll", 1337, 0x12345678));
+}
+
+TEST_F(WhitelistFileTest, CorruptFile) {
+  CreateTestFile();
+
+  base::File* file = GetBlFile();
+  ASSERT_TRUE(file->IsValid());
+
+  // 1) Not enough data for array size
+  PackedWhitelistMetadata meta = {kCurrent, static_cast<uint32_t>(50)};
+  ASSERT_EQ(file->Write(0, reinterpret_cast<const char*>(&meta), sizeof(meta)),
+            static_cast<int>(sizeof(meta)));
+  EXPECT_EQ(InitFromFile(), FileStatus::kArrayReadFail);
+
+  // 2) Corrupt data or just unsupported metadata version.
+  meta = {kUnsupported, static_cast<uint32_t>(50)};
+  ASSERT_EQ(file->Write(0, reinterpret_cast<const char*>(&meta), sizeof(meta)),
+            static_cast<int>(sizeof(meta)));
+  EXPECT_EQ(InitFromFile(), FileStatus::kInvalidFormatVersion);
+
+  // 3) Not enough data for metadata.
+  meta = {kCurrent, static_cast<uint32_t>(10)};
+  ASSERT_EQ(
+      file->Write(0, reinterpret_cast<const char*>(&meta), sizeof(meta) / 2),
+      static_cast<int>(sizeof(meta) / 2));
+  ASSERT_TRUE(file->SetLength(sizeof(meta) / 2));
+  EXPECT_EQ(InitFromFile(), FileStatus::kMetadataReadFail);
 }
 
 }  // namespace

chrome_elf/whitelist/whitelist_packed_format.cc

+// Copyright 2017 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_elf/whitelist/whitelist_packed_format.h"
+
+namespace whitelist {
+
+// Subdir relative to install_static::GetUserDataDirectory().
+const wchar_t kFileSubdir[] =
+    L"\\ThirdPartyModuleList"
+#if defined(_WIN64)
+    L"64";
+#else
+    L"32";
+#endif
+
+// Packed module data cache file.
+const wchar_t kBlFileName[] = L"\\bldata";
+
+}  // namespace whitelist

chrome_elf/whitelist/whitelist_file_format.h → chrome_elf/whitelist/whitelist_packed_format.h

@@ -14,14 +14,23 @@
 //   second by code_id hash (there can be multiple of the same basename hash).
 // -----------------------------------------------------------------------------
 
-#ifndef CHROME_ELF_WHITELIST_WHITELIST_FILE_FORMAT_H_
-#define CHROME_ELF_WHITELIST_WHITELIST_FILE_FORMAT_H_
+#ifndef CHROME_ELF_WHITELIST_WHITELIST_PACKED_FORMAT_H_
+#define CHROME_ELF_WHITELIST_WHITELIST_PACKED_FORMAT_H_
+
+#include <stdint.h>
 
 namespace whitelist {
 
+// Subdir relative to install_static::GetUserDataDirectory().
+extern const wchar_t kFileSubdir[];
+
+// Packed module data cache file.
+extern const wchar_t kBlFileName[];
+
 enum PackedWhitelistVersion : uint32_t {
   kInitialVersion = 1,
   kCurrent = kInitialVersion,
+  kUnsupported
 };
 
 struct PackedWhitelistMetadata {
@@ -40,6 +49,9 @@ struct PackedWhitelistModule {
   // the FileHeader.TimeDateStamp and OptionalHeader.SizeOfImage with the
   // formatting string %08X%x. Then SHA1 the string.
   uint8_t code_id_hash[20];
+  // A timestamp used for tracking "last attempted load".  Used to manage
+  // lifetime of entries in the local caches.
+  uint32_t time_date_stamp;
 };
 
 // These struct are directly written to a file. Therefore the padding should
@@ -47,10 +59,10 @@ struct PackedWhitelistModule {
 static_assert(sizeof(PackedWhitelistMetadata) == 8,
               "The actual padding of the PackedWhitelistMetadata struct "
               "doesn't match the expected padding");
-static_assert(sizeof(PackedWhitelistModule) == 40,
+static_assert(sizeof(PackedWhitelistModule) == 44,
               "The actual padding of the PackedWhitelistModule struct doesn't "
               "match the expected padding");
 
 }  // namespace whitelist
 
-#endif  // CHROME_ELF_WHITELIST_WHITELIST_FILE_FORMAT_H_
+#endif  // CHROME_ELF_WHITELIST_WHITELIST_PACKED_FORMAT_H_