Adding the VerifyModule function (and helpers) to safe browsing.

chrome/browser/safe_browsing/module_integrity_verifier_win.cc

Index: chrome/browser/safe_browsing/module_integrity_verifier_win.cc
diff --git a/chrome/browser/safe_browsing/module_integrity_verifier_win.cc b/chrome/browser/safe_browsing/module_integrity_verifier_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0505723c5bfe97eb091b25fa12a2851071782f42
--- /dev/null
+++ b/chrome/browser/safe_browsing/module_integrity_verifier_win.cc
@@ -0,0 +1,296 @@
+// Copyright 2014 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/browser/safe_browsing/module_integrity_verifier_win.h"
+
+#include <set>

[grt (UTC plus 2), 2014/08/05 15:51:55]

#include "base/containers/hash_tables.h"

[krstnmnlsn, 2014/08/05 19:17:48]

Done.

+
+#include "base/files/file_path.h"
+#include "base/files/memory_mapped_file.h"
+#include "base/metrics/histogram.h"
+#include "base/scoped_native_library.h"
+#include "base/win/pe_image.h"
+
+namespace safe_browsing {
+
+struct ModuleVerificationState {
+  explicit ModuleVerificationState(HMODULE hModule);
+  ~ModuleVerificationState();
+
+  base::win::PEImageAsData disk_peimage;
+
+  // The module's preferred base address minus the base address it actually
+  // loaded at.
+  uintptr_t image_base_delta;
+
+  // The location of the disk_peimage module's code section minus that of the
+  // mem_peimage module's code section.
+  uintptr_t code_section_delta;
+
+  // The bytes corrected by relocs.
+  base::hash_set<uintptr_t> reloc_addr;
+
+  // Set true if the relocation table contains a reloc of type that we don't
+  // currently handle.
+  bool unknown_reloc_type;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ModuleVerificationState);
+};
+
+ModuleVerificationState::ModuleVerificationState(HMODULE hModule)
+    : disk_peimage(hModule),
+      image_base_delta(0),
+      code_section_delta(0),
+      reloc_addr(),
+      unknown_reloc_type(false) {
+}
+
+ModuleVerificationState::~ModuleVerificationState() {
+}
+
+namespace {
+
+struct Export {
+  Export(void* addr, std::string name);

[grt (UTC plus 2), 2014/08/05 15:51:56]

const std::string&

[krstnmnlsn, 2014/08/05 19:17:48]

Done.

+  ~Export();
+
+  bool operator<(const Export& other) const;
+
+  void* addr;
+  std::string name;
+};
+
+Export::Export(void* addr, std::string name) : addr(addr), name(name) {
+}
+
+Export::~Export() {
+}
+
+bool Export::operator<(const Export& other) const {
+  return addr < other.addr;
+}
+
+bool ByteAccountedForByReloc(uint8_t* byte_addr,
+                             ModuleVerificationState* state) {
+  return ((state->reloc_addr.count(reinterpret_cast<uintptr_t>(byte_addr))) >
+          0);
+}
+
+// Checks each byte in the module's code section again the corresponding byte on

[grt (UTC plus 2), 2014/08/05 15:51:55]

please add to the doc comment that |exports| must be sorted

[krstnmnlsn, 2014/08/05 19:17:48]

Done.

+// disk.  Returns a list of the functions who may have been modified.
+int ExamineBytesDiffInMemory(uint8_t* disk_code_start,
+                             uint8_t* mem_code_start,
+                             uint32_t code_size,
+                             std::vector<Export> exports,

[grt (UTC plus 2), 2014/08/05 15:51:55]

const std::vector<Export>&

[krstnmnlsn, 2014/08/05 19:17:48]

Done.

+                             ModuleVerificationState* state,
+                             std::set<std::string>* modified_exports) {
+  int bytes_different = 0;
+  std::vector<Export>::iterator export_it = exports.begin();

[grt (UTC plus 2), 2014/08/05 15:51:55]

const_iterator

[krstnmnlsn, 2014/08/05 19:17:48]

Done.

+
+  for (uint8_t* end = mem_code_start + code_size; mem_code_start != end;
+       ++mem_code_start) {
+    if ((*disk_code_start++ != *mem_code_start) &&
+        !ByteAccountedForByReloc(mem_code_start, state)) {
+      // We get the largest export address still smaller than |addr|.  It is
+      // possible that |addr| belongs to some nonexported function located
+      // between this export and the following one.
+      Export addr =

[grt (UTC plus 2), 2014/08/05 15:51:56]

      Export addr(reinterpret_cast<void*>(mem_code_start), std::string());

[krstnmnlsn, 2014/08/05 19:17:49]

Done.

+          Export(reinterpret_cast<void*>(mem_code_start), std::string());
+      std::vector<Export>::iterator modified_export_it =
+          std::upper_bound(export_it, exports.end(), addr);
+
+      if (modified_export_it != exports.begin())
+        modified_exports->insert((modified_export_it - 1)->name);
+      ++bytes_different;
+
+      // No later byte can belong to an earlier export.
+      export_it = modified_export_it;
+    }
+  }
+  return bytes_different;
+}
+
+// Adds to |state->reloc_addr| the bytes of the pointer at |address| that are
+// corrected by adding |image_base_delta|.
+void AddBytesCorrectedByReloc(ModuleVerificationState* state,

[grt (UTC plus 2), 2014/08/05 15:51:55]

swap args since |state| is modified

[krstnmnlsn, 2014/08/05 19:17:49]

Done.

+                              uintptr_t address) {
+  uintptr_t orig_mem_value = *reinterpret_cast<uintptr_t*>(address);
+  uintptr_t fixed_mem_value = orig_mem_value + state->image_base_delta;
+  uintptr_t disk_value =
+      *reinterpret_cast<uintptr_t*>(address + state->code_section_delta);
+
+  uintptr_t diff_before = orig_mem_value ^ disk_value;
+  uintptr_t shared_after = ~(fixed_mem_value ^ disk_value);
+  int i = 0;
+  for (uintptr_t fixed = diff_before & shared_after; fixed; fixed >>= 8) {
+    if (fixed & 0xFF) {

[grt (UTC plus 2), 2014/08/05 15:51:56]

nit: no braces

[krstnmnlsn, 2014/08/05 19:17:48]

Done.

+      state->reloc_addr.insert(address + i);

[grt (UTC plus 2), 2014/08/05 15:51:55]

this depends on the endianness of the machine, doesn't it? do you care?

[krstnmnlsn, 2014/08/05 19:17:49]

Oh hmm.  I do care if windows ever stores big endian addresses.  What is the
normal way of handling that in chrome?  I found some macros people seem to be
using (see above).

[grt (UTC plus 2), 2014/08/06 01:52:49]

maybe something like:
#if defined(ARCH_CPU_LITTLE_ENDIAN)
...
#else
#error Big-endian architectures are unsupported.
#endif

somewhere in this function

+    }
+    ++i;

[grt (UTC plus 2), 2014/08/05 15:51:55]

put this in the for loop (fixed >>= 8, ++i)? i think it'll just fit without
wrapping:

  for (uintptr_t fixed = diff_before & shared_after; fixed; fixed >>= 8, ++i) {
    if (fixed & 0xFF)
      state->reloc_addr.insert(address + i);
  }

[krstnmnlsn, 2014/08/05 19:17:48]

With (one) space to spare!

+  }
+}
+
+bool AddrIsInCodeSection(void* address,
+                         uint8_t* code_addr,
+                         uint32_t code_size) {
+  return (code_addr <= address && address < code_addr + code_size);
+}
+
+bool EnumRelocsCallback(const base::win::PEImage& mem_peimage,
+                        WORD type,
+                        void* address,
+                        void* cookie) {
+  ModuleVerificationState* state =
+      reinterpret_cast<ModuleVerificationState*>(cookie);
+
+  uint8_t* mem_code_addr = NULL;
+  uint8_t* disk_code_addr = NULL;
+  uint32_t code_size = 0;
+  if (!GetCodeAddrsAndSize(mem_peimage,
+                           state->disk_peimage,
+                           &mem_code_addr,
+                           &disk_code_addr,
+                           &code_size))
+    return false;
+
+  // If not in the code section return true to continue to the next reloc.
+  if (!AddrIsInCodeSection(address, mem_code_addr, code_size))
+    return true;
+
+  switch (type) {
+    case IMAGE_REL_BASED_HIGHLOW: {
+      AddBytesCorrectedByReloc(state, reinterpret_cast<uintptr_t>(address));
+      break;
+    }
+    case IMAGE_REL_BASED_ABSOLUTE:
+      // Absolute type relocations are a noop, sometimes used to pad a section
+      // of relocations.
+      break;
+    default: {
+      // TODO(krstnmnlsn): Find a reliable description of the behaviour of the
+      // remaining types of relocation and handle them.
+      UMA_HISTOGRAM_ENUMERATION(

[grt (UTC plus 2), 2014/08/05 15:51:56]

if this is done on line 161, then you'll be able to see how often these
unhandled types occur relative to the handled ones. is that valuable?

[krstnmnlsn, 2014/08/05 19:17:48]

That would probably give a better picture yes.

+          "SafeBrowsing.ModuleBaseRelocation", type, IMAGE_REL_BASED_DIR64 + 1);

[Alexei Svitkine (slow), 2014/08/05 14:51:21]

How do you know |type| will always be in range?

Perhaps in this case, it's better to use a sparse histogram (see
UMA_HISTOGRAM_SPARSE_SLOWLY), which doesn't require specifying a max. That is,
if you're OK with the overhead of it using a map (i.e. which may involve an
allocation).

[grt (UTC plus 2), 2014/08/05 15:51:55]

If it's out of range, the PE image is malformed in some way. The NT loader may
not even know what to do with it.
Do values that exceed the max end up in an overflow bucket? Does
UMA_HISTOGRAM_ENUMERATION keep an overflow bucket? Can it be used with one? It
might be enough to know that unknown values are seen (and how often) without
knowing their exact values.

[krstnmnlsn, 2014/08/05 19:17:48]

If it doesn't have an overflow bucket I could add one manually?  I agree that
larger values shouldn't in theory appear...

[Alexei Svitkine (slow), 2014/08/05 19:27:07]

Yes, there will be an overflow bucket by default. My point is that it's not
clear to me what ensures |type| is in range. What stops a newer version of
Windows and MS dev tools from introducing a new reloc type or for there to exist
some undocumented reloc types that Microsoft uses internally?

If you're OK keeping as is, please add a comment explaining why
IMAGE_REL_BASED_DIR64 + 1 is the correct value to use here (e.g. by pointing at
a msdn URL or something).

+      state->unknown_reloc_type = true;
+      break;
+    }
+  }
+  return true;
+}
+
+bool EnumExportsCallback(const base::win::PEImage& mem_peimage,
+                         DWORD ordinal,
+                         DWORD hint,
+                         LPCSTR name,
+                         PVOID function_addr,
+                         LPCSTR forward,
+                         PVOID cookie) {
+  std::vector<Export>* exports = reinterpret_cast<std::vector<Export>*>(cookie);
+  if (name) {

[grt (UTC plus 2), 2014/08/05 15:51:56]

nit: no braces

[krstnmnlsn, 2014/08/05 19:17:48]

Done.

+    exports->push_back(Export(function_addr, std::string(name)));
+  }

[grt (UTC plus 2), 2014/08/05 15:51:55]

do you care about handling exports-by-ordinal?

[krstnmnlsn, 2014/08/05 19:17:48]

I'm not sure what you mean?  Currently, the code is only interested in
determining which export a modified byte in memory could belong to. I didn't
think the ordinals could help with that, since (I thought) ordinal's aren't
guaranteed to be ordered as in memory?

[grt (UTC plus 2), 2014/08/06 01:52:49]

Functions can be exported/imported by ordinal rather than by name; see
http://msdn.microsoft.com/library/e7tsx612.aspx. By name is useful as-is, so
please put a TODO in the code to add support for ordinals. The Microsoft
Portable Executable and Common Object File Format Specification has gory
details.

+  return true;
+}
+
+}  // namespace
+
+bool GetCodeAddrsAndSize(const base::win::PEImage& mem_peimage,
+                         const base::win::PEImageAsData& disk_peimage,
+                         uint8_t** mem_code_addr,
+                         uint8_t** disk_code_addr,
+                         uint32_t* code_size) {
+  DWORD base_of_code = mem_peimage.GetNTHeaders()->OptionalHeader.BaseOfCode;
+
+  // Get the address and size of the code section in the loaded module image.
+  PIMAGE_SECTION_HEADER mem_code_header =
+      mem_peimage.GetImageSectionFromAddr(mem_peimage.RVAToAddr(base_of_code));
+  if (mem_code_header == NULL)
+    return false;
+  *mem_code_addr = reinterpret_cast<uint8_t*>(
+      mem_peimage.RVAToAddr(mem_code_header->VirtualAddress));
+  // If the section is padded with zeros when mapped then |VirtualSize| can be
+  // larger.  Alternatively, |SizeOfRawData| can be rounded up to align
+  // according to OptionalHeader.FileAlignment.
+  *code_size = std::min(mem_code_header->Misc.VirtualSize,
+                        mem_code_header->SizeOfRawData);
+
+  // Get the address of the code section in the module mapped as data from disk.
+  DWORD disk_code_offset = 0;
+  if (!mem_peimage.ImageAddrToOnDiskOffset(
+          reinterpret_cast<void*>(*mem_code_addr), &disk_code_offset))
+    return false;
+  *disk_code_addr =
+      reinterpret_cast<uint8_t*>(disk_peimage.module()) + disk_code_offset;
+  return true;
+}
+
+ModuleState VerifyModule(const wchar_t* module_name,
+                         std::set<std::string>* modified_exports) {
+  // Get module handle, load a copy from disk as data and create PEImages.
+  HMODULE module_handle = NULL;
+  if (!GetModuleHandleEx(0, module_name, &module_handle))
+    return MODULE_STATE_UNKNOWN;
+  base::ScopedNativeLibrary native_library(module_handle);
+
+  WCHAR module_path[MAX_PATH] = {};
+  DWORD length =
+      GetModuleFileName(module_handle, module_path, arraysize(module_path));
+  if (!length || length == arraysize(module_path))
+    return MODULE_STATE_UNKNOWN;
+
+  base::MemoryMappedFile mapped_module;
+  if (!mapped_module.Initialize(base::FilePath(module_path)))
+    return MODULE_STATE_UNKNOWN;
+  ModuleVerificationState state(
+      reinterpret_cast<HMODULE>(const_cast<uint8*>(mapped_module.data())));
+
+  base::win::PEImage mem_peimage(module_handle);
+  if (!mem_peimage.VerifyMagic() || !state.disk_peimage.VerifyMagic())
+    return MODULE_STATE_UNKNOWN;
+
+  // Get the list of exports.
+  std::vector<Export> exports;
+  mem_peimage.EnumExports(EnumExportsCallback, &exports);
+  std::sort(exports.begin(), exports.end());
+
+  // Get the addresses of the code sections then calculate |code_section_delta|
+  // and |image_base_delta|.
+  uint8_t* mem_code_addr = NULL;
+  uint8_t* disk_code_addr = NULL;
+  uint32_t code_size = 0;
+  if (!GetCodeAddrsAndSize(mem_peimage,
+                           state.disk_peimage,
+                           &mem_code_addr,
+                           &disk_code_addr,
+                           &code_size))
+    return MODULE_STATE_UNKNOWN;
+
+  state.code_section_delta = disk_code_addr - mem_code_addr;
+
+  uint8_t* preferred_image_base = reinterpret_cast<uint8_t*>(
+      state.disk_peimage.GetNTHeaders()->OptionalHeader.ImageBase);
+  state.image_base_delta =
+      preferred_image_base - reinterpret_cast<uint8_t*>(mem_peimage.module());
+
+  // Get the relocations.
+  mem_peimage.EnumRelocs(EnumRelocsCallback, &state);
+  if (state.unknown_reloc_type)
+    return MODULE_STATE_UNKNOWN;
+
+  // Count the modified bytes (after accounting for relocs) and get the set of
+  // modified functions.
+  int num_bytes_different = ExamineBytesDiffInMemory(disk_code_addr,
+                                                     mem_code_addr,
+                                                     code_size,
+                                                     exports,
+                                                     &state,
+                                                     modified_exports);
+
+  if (num_bytes_different == 0)

[grt (UTC plus 2), 2014/08/05 15:51:55]

  return num_bytes_different ? MODULE_STATE_MODIFIED : MODULE_STATE_UNMODIFIED;

[krstnmnlsn, 2014/08/05 19:17:48]

Done.

+    return MODULE_STATE_UNMODIFIED;
+  return MODULE_STATE_MODIFIED;
+}
+
+}  // namespace safe_browsing