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

chrome/browser/safe_browsing/module_integrity_verifier.cc

+// 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.h"
+
+#include "base/files/file_path.h"
+#include "base/files/memory_mapped_file.h"
+#include "base/scoped_native_library.h"
+#include "base/win/pe_image.h"
+
+namespace safe_browsing {
+
+namespace {
+
+struct RelocEnumerationState {
+  explicit RelocEnumerationState(HMODULE hModule);
+  ~RelocEnumerationState();
+
+  base::win::PEImageAsData disk_peimage;
+
+  // The number of bytes made equivalent between memory and disk due to
+  // relocations.
+  int bytes_corrected_by_reloc;
+
+  // Set true if the relocation table contains a reloc of type that we don't
+  // currently handle.
+  bool unknown_reloc_type;
+

[grt (UTC plus 2), 2014/07/31 17:03:33]

 private:

[krstnmnlsn, 2014/08/04 15:18:15]

Done.

+  DISALLOW_COPY_AND_ASSIGN(RelocEnumerationState);
+};
+
+RelocEnumerationState::RelocEnumerationState(HMODULE hModule)
+    : disk_peimage(hModule),
+      bytes_corrected_by_reloc(0),
+      unknown_reloc_type(false) {
+}
+
+RelocEnumerationState::~RelocEnumerationState() {
+}
+
+int CountBytesDiffInMemory(uint8_t* disk_code_start,
+                           uint8_t* memory_code_start,
+                           uint32_t code_size) {
+  int counter = 0;
+  for (int i = 0; i < static_cast<int>(code_size); ++i) {

[grt (UTC plus 2), 2014/07/31 17:03:33]

unsigned -> signed is implementation defined if the original value is too big to
fit in the target type, so this cast is dangerous. how about:

  for (uint8_t* end = disk_code_start + code_size; disk_code_start != end; ) {
    if (*disk_code_start++ != *memory_code_start++)
      ++counter;
  }

[krstnmnlsn, 2014/08/04 15:18:14]

Right, had tried to fix this in the other branch with uint32_t, using an actual
pointer seems more semantically correct though.  Done.

+    if (*(disk_code_start + i) != *(memory_code_start + i))
+      ++counter;
+  }
+  return counter;
+}
+
+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) {
+  RelocEnumerationState* reloc_enum_state =
+      reinterpret_cast<RelocEnumerationState*>(cookie);
+  const base::win::PEImageAsData* disk_peimage_ptr =
+      &reloc_enum_state->disk_peimage;
+  uint8_t* mem_code_addr = NULL;
+  uint8_t* disk_code_addr = NULL;
+  uint32_t code_size = 0;
+  if (!GetCodeAddrsAndSize(mem_peimage,
+                           *disk_peimage_ptr,
+                           &mem_code_addr,
+                           &disk_code_addr,
+                           &code_size))

[grt (UTC plus 2), 2014/07/31 17:03:34]

i thought chromium style called for braces around this since the "if" spans
multiple lines, but if "git cl format" allows it, then i guess i'm wrong. okay.

[krstnmnlsn, 2014/08/04 15:18:15]

Don't see anything.  They do say we can add them if we want them however.  Would
you like some braces? 

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

Naw, if cl-format is happy, I'm happy.

+    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: {
+      uint8_t* preferred_image_base = reinterpret_cast<uint8_t*>(
+          disk_peimage_ptr->GetNTHeaders()->OptionalHeader.ImageBase);
+      uintptr_t delta = preferred_image_base -
+                        reinterpret_cast<uint8_t*>(mem_peimage.module());

[grt (UTC plus 2), 2014/07/31 17:03:33]

why cast to uint8_t* and then assign to uintptr_t?

[krstnmnlsn, 2014/08/04 15:18:15]

The original plan was to store pointers as pointers, but delta is not
technically an address so I thought storing it as an actual pointer would be
bad?  Compromised on uintptr_t, which is still misleading... 
I could change everything to uintptr_t? 

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

Ah, I misread this before. Deltas are often signed (e.g., ptrdiff_t is signed
while size_t is unsigned) since they ordinarily can be positive or negative. If
that's the case here, then intptr_t is probably the right choice. Does this
sound safe to you?

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

Right, that sounds perfect to me.  This code has been heavily reshuffled so I
think this change now amounts to making image_base_delta and code_section_delta
intptr_t's in ModuleVerificationState.

+      uint8_t* new_value = (*reinterpret_cast<uint8_t**>(address)) + delta;
+      int bytes_corrected =
+          CountBytesDiffInPtr(reinterpret_cast<uintptr_t>(new_value),
+                              *reinterpret_cast<uintptr_t*>(address));
+
+      // Check that the adding delta corrected the value to agree with disk.
+      uint8_t** disk_address = reinterpret_cast<uint8_t**>(
+          reinterpret_cast<uint8_t*>(address) - mem_code_addr + disk_code_addr);
+      if (new_value == *disk_address) {

[grt (UTC plus 2), 2014/07/31 17:03:34]

nit: remove braces

[krstnmnlsn, 2014/08/04 15:18:14]

Done.

+        reloc_enum_state->bytes_corrected_by_reloc += bytes_corrected;
+      }
+      break;
+    }
+    case IMAGE_REL_BASED_ABSOLUTE:
+      // Absolute type relocations are a noop, sometimes used to pad a section
+      // of relocations.
+      break;
+    default: {

[grt (UTC plus 2), 2014/07/31 17:03:33]

nit: remove braces

[krstnmnlsn, 2014/08/04 15:18:15]

Done.

+      // TODO(krstnmnlsn): Find a reliable description of the behaviour of the
+      // remaining types of relocation and handle them.
+      reloc_enum_state->unknown_reloc_type = true;

[grt (UTC plus 2), 2014/07/31 17:03:33]

consider putting:
  UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.ModuleBaseRelocation",
                            type, IMAGE_REL_BASED_DIR64+1);
up around line 80 so that you can see if these unhandled cases are happening in
the field. you'll need a corresponding change to
src/tools/metrics/histograms/histograms.xml to add the histogram and its enum
type.

[krstnmnlsn, 2014/08/04 15:18:14]

I like this idea!  Added a histogram, I made the value-name pairs of the enum
agree with the winnt.h based relocation types.  Should we try to handle machine
specific differences instead?

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

Can you clarify the question? What are the machine-specific differences?

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

So in my enum I have things like 
"<int value="5" label="IMAGE_REL_BASED_MACHINE_SPECIFIC_5"/>"
Because winnt.h defines IMAGE_REL_BASED_MACHINE_SPECIFIC_5 as 5.  However they
also define IMAGE_REL_BASED_MIPS_JMPADDR and IMAGE_REL_BASED_MIPS_JMPADDR16 as
5.  So receiving a 5 could mean either of those two (depending on the machine). 
Do you think it's worth differentiating right now?

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

No, just storing the values is fine. We have CPU info elsewhere in the protobuf,
so we can figure out which symbolic name it is if needed.

+      break;
+    }
+  }
+  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));
+  *code_size = mem_code_header->Misc.VirtualSize;
+
+  // Get the address of the code section in the module mapped as data from disk.
+  // Section size will be the same as in the loaded module.
+  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;
+}
+
+int CountBytesDiffInPtr(uintptr_t num_a, uintptr_t num_b) {
+  int num_bytes = 0;
+  for (int i = 0; i < sizeof(num_a); ++i) {

[grt (UTC plus 2), 2014/07/31 17:03:33]

is this equivalent?
  for (uintptr_t delta = num_a ^ num_b; delta; delta >>= 8) {
    if (delta & 0xFF)
      ++num_bytes;
  }

if i'm not mistaken, this will terminate once low-order byte differences have
been found (and not loop at all when the pointers are identical).

[krstnmnlsn, 2014/08/04 15:18:14]

right, why not take the difference right away.  thanks!

+    if ((num_a & 0xFF) != (num_b & 0xFF))
+      ++num_bytes;
+    num_a >>= 8;
+    num_b >>= 8;
+  }
+  return num_bytes;
+}
+
+ModuleState VerifyModule(const wchar_t* module_name) {
+  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;
+  RelocEnumerationState reloc_enum_state(
+      reinterpret_cast<HMODULE>(const_cast<uint8*>(mapped_module.data())));
+
+  base::win::PEImage mem_peimage(module_handle);
+  if (!mem_peimage.VerifyMagic() ||
+      !reloc_enum_state.disk_peimage.VerifyMagic())
+    return MODULE_STATE_UNKNOWN;
+
+  uint8_t* mem_code_addr = NULL;
+  uint8_t* disk_code_addr = NULL;
+  uint32_t code_size = 0;
+  if (!GetCodeAddrsAndSize(mem_peimage,
+                           reloc_enum_state.disk_peimage,
+                           &mem_code_addr,
+                           &disk_code_addr,
+                           &code_size))
+    return MODULE_STATE_UNKNOWN;
+
+  mem_peimage.EnumRelocs(EnumRelocsCallback, &reloc_enum_state);
+  if (reloc_enum_state.unknown_reloc_type)
+    return MODULE_STATE_UNKNOWN;
+
+  int num_bytes_different =
+      CountBytesDiffInMemory(disk_code_addr, mem_code_addr, code_size);
+  if (num_bytes_different == reloc_enum_state.bytes_corrected_by_reloc)
+    return MODULE_STATE_UNMODIFIED;
+  return MODULE_STATE_MODIFIED;
+}
+
+}  // namespace safe_browsing