Add ELF header analysis when checking for instruction pointer in code.

src/processor/exploitability_linux.cc

Index: src/processor/exploitability_linux.cc
===================================================================
--- src/processor/exploitability_linux.cc	(revision 1471)
+++ src/processor/exploitability_linux.cc	(working copy)
@@ -36,6 +36,11 @@
 
 #include "processor/exploitability_linux.h"
 
+#include <assert.h>
+#include <elf.h>
+#include <stdlib.h>
+#include <string.h>
+
 #include "google_breakpad/common/minidump_exception_linux.h"
 #include "google_breakpad/processor/call_stack.h"
 #include "google_breakpad/processor/process_state.h"
@@ -119,9 +124,29 @@
     return EXPLOITABILITY_HIGH;
   }
 
+  // There was no strong evidence suggesting exploitability, but the minidump
+  // does not appear totally benign either.
   return EXPLOITABILITY_INTERESTING;
 }
 
+bool ExploitabilityLinux::Architecture32Bit() {
+  // GetContextCPU() should have already been successfully called before

[ahonig, 2015/07/14 19:32:18]

Add a check to make sure this is the case.  If the context is null then you
should either have the program die gracefully or return false and log an error
message.

[liuandrew, 2015/07/15 21:50:14]

Done.

+  // calling this method. Thus the switch statement should not seg fault.
+  switch (dump_->GetException()->GetContext()->GetContextCPU()) {
+    case MD_CONTEXT_ARM:
+    case MD_CONTEXT_X86:
+      return true;
+    case MD_CONTEXT_ARM64:
+    case MD_CONTEXT_AMD64:
+      return false;
+    default:
+      // This should not happen. The four architectures above should be
+      // the only Linux architectures.
+      BPLOG(INFO) << "Unsupported architecture.";

[ivanpe, 2015/07/14 00:23:02]

Instead of bool, you should consider returning an enumeration with a distinct
value for "unsupported architecture".  Otherwise the caller will assume that the
unsupported architecture is 64-bit.

[liuandrew, 2015/07/15 21:50:14]

Done.

+      return false;
+  }
+}
+
 bool ExploitabilityLinux::InstructionPointerInCode(uint64_t instruction_ptr) {
   // Here we get memory mapping. Most minidumps will not contain a memory
   // mapping, so we will commonly resort to checking modules.
@@ -138,13 +163,156 @@
 
   // If the memory mapping retrieval fails, we will check the modules
   // to see if the instruction pointer is inside a module.
-  // TODO(liuandrew): Check if the instruction pointer lies in an executable
-  // region within the module.
   MinidumpModuleList *minidump_module_list = dump_->GetModuleList();
-  return !minidump_module_list ||
-          minidump_module_list->GetModuleForAddress(instruction_ptr);
+  const MinidumpModule *minidump_module =
+      minidump_module_list ?
+      minidump_module_list->GetModuleForAddress(instruction_ptr) : NULL;
+
+  // If the instruction pointer isn't in a module, we can return false.

[ivanpe, 2015/07/14 00:23:02]

Please, don't use "we" in comments. Comments should be written in 3rd person.
e.g:

// If the instruction pointer isn't in a module, return false.

[liuandrew, 2015/07/15 21:50:14]

Done.

+  if (minidump_module == NULL) {
+    return false;
+  }
+
+  // Get ELF header data from the instruction pointer's module.
+  const uint64_t base_address = minidump_module->base_address();
+  MinidumpMemoryList *memory_list = dump_->GetMemoryList();
+  MinidumpMemoryRegion *memory_region =
+      memory_list ?
+      memory_list->GetMemoryRegionForAddress(base_address) : NULL;
+
+  // The minidump does not have the correct memory region.
+  // This returns true because even though there is no memory data available,
+  // the evidence so far suggests that the instruction pointer is not at a
+  // bad location.
+  if (memory_region == NULL) {
+    return true;
+  }
+
+  // Examine ELF headers. Depending on the architecture, the size of the
+  // ELF headers can differ.
+  if (this->Architecture32Bit()) {
+    // Check if the ELF header is within the memory region.
+    if (memory_region->GetSize() < sizeof(Elf32_Phdr)) {
+      return false;
+    }
+    // Set 32-bit ELF header and program header table.

[ivanpe, 2015/07/14 00:23:02]

Instead of "Set" maybe is s better to say "Load" or "Get".

[liuandrew, 2015/07/15 21:50:14]

Done.

+    Elf32_Ehdr *header = this->LoadElf32Header(memory_region, base_address);

[ivanpe, 2015/07/14 00:23:02]

Do we need to worry about ownership transfer here?

[liuandrew, 2015/07/15 21:50:13]

using scoped_ptr

+    assert(header->e_phentsize == sizeof(Elf32_Phdr));

[ahonig, 2015/07/14 19:32:18]

assert is compiled out in non-debug builds.  If this is important make it an if
statement.

[liuandrew, 2015/07/15 21:50:14]

Done.

+    // Check if the program header table is within the memory region.
+    if (memory_region->GetSize() <
+        header->e_phoff + (header->e_phentsize * header->e_phnum)) {
+      return false;
+    }
+
+    Elf32_Phdr *program_headers = this->LoadElf32PHeader(memory_region,
+                                                         base_address,
+                                                         header->e_phoff,
+                                                         header->e_phentsize,
+                                                         header->e_phnum);
+    // Find correct program header that corresponds to the instruction pointer.
+    for (int i = 0; i < header->e_phnum; i++) {
+      Elf32_Phdr program_header = program_headers[i];

[ivanpe, 2015/07/14 00:23:02]

Please, use const Elf32_Phdr& to avoid copy

+      // Check if instruction pointer lies within this program header's region.
+      if (program_header.p_vaddr >= instruction_ptr &&
+          program_header.p_vaddr + program_header.p_memsz < instruction_ptr) {

[ivanpe, 2015/07/14 00:23:02]

This check seems wrong to me.  It can only be true for negative
program_header.p_memsz

[liuandrew, 2015/07/15 21:50:14]

My bad. I flipped the equality signs. Fixed and added a relevant unit test.

+        free(header);

[ivanpe, 2015/07/14 00:23:01]

I noticed that you free header here but you don't free it when returning on line
204.

[ahonig, 2015/07/14 19:32:18]

The pattern of free is pretty complicated and error prone.  Is c++ 11
(std::unique_ptr) allowed in this file?

[liuandrew, 2015/07/15 21:50:14]

Done.

+        free(program_headers);
+        // Return whether this program header region is executable.
+        return program_header.p_flags & 1;

[ivanpe, 2015/07/14 00:23:01]

Instead of the literal 1, isn't there a constant that can be used here.

[liuandrew, 2015/07/15 21:50:14]

Done.

+      }
+    }
+    free(header);
+    free(program_headers);
+  } else {
+    // Check if the ELF header is within the memory region.
+    if (memory_region->GetSize() < sizeof(Elf64_Phdr)) {
+      return false;
+    }
+    // Set 64-bit ELF header and program header table.
+    Elf64_Ehdr *header = this->LoadElf64Header(memory_region, base_address);
+    assert(header->e_phentsize == sizeof(Elf64_Phdr));
+    // Check if the program header table is within the memory region.
+    if (memory_region->GetSize() <
+        header->e_phoff + (header->e_phentsize * header->e_phnum)) {
+      return false;
+    }
+    Elf64_Phdr *program_headers = this->LoadElf64PHeader(memory_region,
+                                                         base_address,
+                                                         header->e_phoff,
+                                                         header->e_phentsize,
+                                                         header->e_phnum);
+    // Find correct program header that corresponds to the instruction pointer.
+    for (int i = 0; i < header->e_phnum; i++) {
+      Elf64_Phdr program_header = program_headers[i];
+      // Check if instruction pointer lies within this program header's region.
+      if (program_header.p_vaddr >= instruction_ptr &&
+          program_header.p_vaddr + program_header.p_memsz < instruction_ptr) {
+        free(header);
+        free(program_headers);
+        // Return whether this program header region is executable.
+        return program_header.p_flags & 1;
+      }
+    }
+    free(header);
+    free(program_headers);
+  }
+
+  // The instruction pointer was not in an area identified by the ELF headers.
+  return false;
 }
 
+void *ExploitabilityLinux::LoadElfHeader(MinidumpMemoryRegion *memory,

[ivanpe, 2015/07/14 00:23:02]

I would suggest replacing this with a template method where the type of the
header will be passed as a template parameter.  This will allow you to replace
malloc for new and then the callers can use scoped_ptr (similar to
std::unique_ptr) to handle ownership and proper cleanup. 

[liuandrew, 2015/07/15 21:50:14]

Done.

+                                         const uint64_t base_address,
+                                         size_t header_size) {
+  void *header = malloc(header_size);
+  // Copy over each byte.
+  for (size_t i = 0; i < header_size; i++) {
+    uint8_t my_byte = 0;
+    // Get the value at the memory address.
+    memory->GetMemoryAtAddress(base_address + i, &my_byte);
+    memcpy(reinterpret_cast<char *>(header) + i, &my_byte, sizeof(uint8_t));
+  }
+  return header;
+}
+
+Elf32_Ehdr *ExploitabilityLinux::LoadElf32Header(MinidumpMemoryRegion *memory,
+                                                 const uint64_t base_address) {
+  return reinterpret_cast<Elf32_Ehdr *>(LoadElfHeader(memory,
+                                                      base_address,
+                                                      sizeof(Elf32_Ehdr)));
+}
+
+Elf64_Ehdr *ExploitabilityLinux::LoadElf64Header(MinidumpMemoryRegion *memory,
+                                                 const uint64_t base_address) {
+  return reinterpret_cast<Elf64_Ehdr *>(LoadElfHeader(memory,
+                                                      base_address,
+                                                      sizeof(Elf64_Ehdr)));
+}
+
+Elf32_Phdr *ExploitabilityLinux::LoadElf32PHeader(MinidumpMemoryRegion *memory,
+                                                  const uint64_t base_address,
+                                                  const uint64_t e_phoff,
+                                                  const uint16_t e_phentsize,
+                                                  const uint16_t e_phnum) {
+  // The base address with the offset makes the starting memory address.
+  // The entry size multiplied by the number of entries is the number of bytes.
+  return reinterpret_cast<Elf32_Phdr *>(LoadElfHeader(memory,
+                                        (base_address + e_phoff),
+                                        (e_phentsize * e_phnum)));
+}
+
+Elf64_Phdr *ExploitabilityLinux::LoadElf64PHeader(MinidumpMemoryRegion *memory,
+                                                  const uint64_t base_address,
+                                                  const uint64_t e_phoff,
+                                                  const uint16_t e_phentsize,
+                                                  const uint16_t e_phnum) {
+  // The base address with the offset makes the starting memory address.
+  // The entry size multiplied by the number of entries is the number of bytes.
+  return reinterpret_cast<Elf64_Phdr *>(LoadElfHeader(memory,
+                                        (base_address + e_phoff),
+                                        (e_phentsize * e_phnum)));
+}
+
 bool ExploitabilityLinux::BenignCrashTrigger(const MDRawExceptionStream
                                                   *raw_exception_stream) {
   // Here we check the cause of crash.