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

chrome/browser/safe_browsing/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/verifier.h"
+
+#include <windows.h>
+#include <psapi.h>
+
+#include "base/files/file_path.h"
+#include "base/files/memory_mapped_file.h"
+#include "base/win/pe_image.h"
+
+namespace safe_browsing {
+
+namespace {
+
+// The number of bytes made to agree (between memory and disk) when the
+// relocations are accounted for.
+int bytes_corrected_by_reloc = 0;
+
+// This is true if the relocation table contains a reloc of type we don't
+// currently handle.
+bool unknown_reloc_type = false;
+
+bool AddrIsInCodeSection(void* address, BYTE* code_addr, SIZE_T code_size) {
+  return (code_addr <= address && address <= code_addr + code_size);
+}
+
+bool GetCodeAddrsAndSize(const base::win::PEImage mem_peimage,

[grt (UTC plus 2), 2014/07/22 17:39:49]

const base::win::PEImage&

[krstnmnlsn, 2014/07/25 00:15:42]

Done.

+                         base::win::PEImageAsData disk_peimage,
+                         BYTE*& mem_code_addr,

[grt (UTC plus 2), 2014/07/22 17:39:50]

chromium passes out params by pointer rather than by non-const ref

[krstnmnlsn, 2014/07/25 00:15:43]

done.  The page on smartpointers is saying to use a raw pointers or WeakPtr when
you don't own the obj/value.  Is there a preference?

+                         BYTE*& disk_code_addr,
+                         SIZE_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<BYTE*>(
+      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<BYTE*>(disk_peimage.module()) + disk_code_offset;
+  return true;
+}
+
+int CountBytesDiffInPtr(intptr_t num_a, intptr_t num_b) {
+  int num_bytes = 0;
+  for (int i = 0; i < sizeof(num_a); ++i) {
+    if ((num_a & 0xFF) != (num_b & 0xFF))
+      ++num_bytes;
+    num_a >>= 8;
+    num_b >>= 8;
+  }
+  return num_bytes;
+}
+
+bool EnumRelocsCallback(const base::win::PEImage& mem_peimage,
+                        WORD type,
+                        void* address,
+                        void* cookie) {
+  base::win::PEImageAsData disk_peimage =
+      *reinterpret_cast<base::win::PEImageAsData*>(cookie);
+  BYTE* mem_code_addr = NULL;
+  BYTE* disk_code_addr = NULL;
+  SIZE_T code_size = 0;
+  if (!GetCodeAddrsAndSize(
+          mem_peimage, disk_peimage, mem_code_addr, disk_code_addr, code_size))
+    return false;
+
+  switch (type) {
+    case IMAGE_REL_BASED_HIGHLOW: {
+      // If not in the code section return true to continue to the next reloc.
+      if (!AddrIsInCodeSection(address, mem_code_addr, code_size))
+        return true;
+
+      BYTE* preferred_image_base = reinterpret_cast<BYTE*>(
+          disk_peimage.GetNTHeaders()->OptionalHeader.ImageBase);
+      intptr_t delta =
+          preferred_image_base - reinterpret_cast<BYTE*>(mem_peimage.module());
+      BYTE* new_value = (*reinterpret_cast<BYTE**>(address)) + delta;
+      int bytes_corrected =
+          CountBytesDiffInPtr(reinterpret_cast<intptr_t>(new_value),
+                              *reinterpret_cast<intptr_t*>(address));
+
+      // Check that the adding delta did correct the value to agree with disk.
+      BYTE** disk_address = reinterpret_cast<BYTE**>(
+          reinterpret_cast<BYTE*>(address) - mem_code_addr + disk_code_addr);
+      if (new_value == *disk_address) {
+        bytes_corrected_by_reloc += bytes_corrected;
+      }
+      break;
+    }
+    case IMAGE_REL_BASED_ABSOLUTE:
+      break;
+    default: {
+      // TODO(krstnmnlsn): Find a reliable description of the behaviour of the
+      // remaining types of relocation and handle them.
+      unknown_reloc_type = true;
+      break;
+    }
+  }
+  return true;
+}
+
+int CountBytesDiffInMemory(BYTE* disk_code_start,
+                           BYTE* memory_code_start,
+                           SIZE_T code_size) {
+  int counter = 0;
+  for (int i = 0; i < static_cast<int>(code_size); ++i) {
+    if (*(disk_code_start + i) != *(memory_code_start + i))
+      ++counter;
+  }
+  return counter;
+}
+
+}  // namespace
+
+ModuleState VerifyModule(wchar_t* module_name) {

[grt (UTC plus 2), 2014/07/22 17:39:49]

const wchar_t*

[krstnmnlsn, 2014/07/25 00:15:42]

Done.

+  HMODULE module_handle = GetModuleHandle(module_name);

[grt (UTC plus 2), 2014/07/22 17:39:49]

the module could be unloaded immediately after this call, leading to a crash or
other undesired behavior below. do something like this instead:

  HMODULE module_handle = NULL;
  if (!GetModuleHandleEx(0, module_name, &module_handle))
    return MODULE_STATE_UNKNOWN;
  base::ScopedNativeLibrary native_library(module_handle);

so that a reference is held on the module for the lifetime of this function.

[krstnmnlsn, 2014/07/25 00:15:42]

Cool!  I was wondering about this, and thought (/was hoping) maybe the modules
we are interacting with would be important enough (chrome, ntdll.dll) that we'd
be in trouble if they unloaded anyways.  But that might not always be the case. 

+  if (module_handle == NULL)
+    return MODULE_STATE_UNKNOWN;
+
+  WCHAR module_path[MAX_PATH] = {0};

[grt (UTC plus 2), 2014/07/22 17:39:50]

nit: {0} -> {}

[krstnmnlsn, 2014/07/25 00:15:42]

Done.

+  if (GetModuleFileNameEx(

[grt (UTC plus 2), 2014/07/22 17:39:49]

check the return value. if the buffer is too small, arraysize(module_path) is
returned and the string may not be terminated, leading to an assortment of bad
stuff below. depending on how much you care about handling long paths, you could
either return MODULE_STATE_UNKNOWN if arraysize(module_path) is returned, or
switch from using a stack-based array to a heap-based string. for example:

  base::string16 module_path;
  DWORD buffer_length = MAX_PATH;
  do {
    DWORD length = GetModuleFileName(module_handle,
                                     WriteInto(&module_path, buffer_length),
                                     buffer_length);
    if (length < buffer_length) {
      module_path.resize(length);
      break;
    }
    if (buffer_length & 0x80000000)
      return MODULE_STATE_UNKNOWN;
    buffer_length *= 2;
  } while(true);

[grt (UTC plus 2), 2014/07/22 17:39:50]

msdn says: "To retrieve the name of a module in the current process, use the
GetModuleFileName function. This is more efficient and more reliable than
calling GetModuleFileNameEx with a handle to the current process."

[krstnmnlsn, 2014/07/25 00:15:42]

Resolution: trusting in all reasonable cases MAX_PATH will be sufficient, giving
up and returning MODULE_STATE_UNKNOWN on failure (ie. if arraysize(module_path)
is returned).

[krstnmnlsn, 2014/07/25 00:15:43]

Done.

+          GetCurrentProcess(), module_handle, module_path, MAX_PATH) == 0)

[grt (UTC plus 2), 2014/07/22 17:39:49]

MAX_PATH -> arraysize(module_path)

[krstnmnlsn, 2014/07/25 00:15:43]

right, thanks.

+    return MODULE_STATE_UNKNOWN;
+
+  base::MemoryMappedFile mapped_module;
+  mapped_module.Initialize(base::FilePath(module_path));
+  base::win::PEImageAsData disk_peimage(

[grt (UTC plus 2), 2014/07/22 17:39:49]

it would be safer to use safe_browsing::PeImageReader rather than PEImageAsData.
i think the former is more careful when it comes to checking offsets.
PeImageReader may not currently have all of the features you need, but it
shouldn't be too difficult to extend it.

[krstnmnlsn, 2014/07/25 00:15:42]

Continuing to use PEImageAsData for now, I could add a comment about the wish to
add relocs to PeImageReader/merge the two peimage parsers?

+      reinterpret_cast<HMODULE>(const_cast<uint8*>(mapped_module.data())));
+
+  base::win::PEImage mem_peimage(module_handle);
+  if (!mem_peimage.VerifyMagic() || !disk_peimage.VerifyMagic())
+    return MODULE_STATE_UNKNOWN;
+
+  BYTE* mem_code_addr = NULL;
+  BYTE* disk_code_addr = NULL;
+  SIZE_T code_size = 0;
+  if (!GetCodeAddrsAndSize(
+          mem_peimage, disk_peimage, mem_code_addr, disk_code_addr, code_size))
+    return MODULE_STATE_UNKNOWN;
+
+  int num_bytes_different =
+      CountBytesDiffInMemory(disk_code_addr, mem_code_addr, code_size);
+  mem_peimage.EnumRelocs(
+      (base::win::PEImage::EnumRelocsFunction) & EnumRelocsCallback,
+      &disk_peimage);
+  if (unknown_reloc_type)
+    return MODULE_STATE_UNKNOWN;
+
+  if (num_bytes_different - bytes_corrected_by_reloc == 0)

[grt (UTC plus 2), 2014/07/22 17:39:49]

?num_bytes_different == bytes_corrected_by_reloc?

[krstnmnlsn, 2014/07/25 00:15:42]

Done. :P

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