Add check for Linux minidump ending on bad write for exploitability rating.

src/processor/exploitability_linux.cc

Index: src/processor/exploitability_linux.cc
===================================================================
--- src/processor/exploitability_linux.cc	(revision 1480)
+++ src/processor/exploitability_linux.cc	(working copy)
@@ -36,6 +36,16 @@
 
 #include "processor/exploitability_linux.h"
 
+#include <regex.h>
+#include <stdio.h>
+#include <string.h>
+#include <sysexits.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include <sstream>
+#include <iterator>
+
 #include "google_breakpad/common/minidump_exception_linux.h"
 #include "google_breakpad/processor/call_stack.h"
 #include "google_breakpad/processor/process_state.h"
@@ -42,6 +52,10 @@
 #include "google_breakpad/processor/stack_frame.h"
 #include "processor/logging.h"
 
+#define MAX_INSTRUCTION_LEN 15
+// This is the buffer size for objdump's output.
+#define MAX_OBJDUMP_BUFFER_LEN 4096
+
 namespace {
 
 // This function in libc is called if the program was compiled with
@@ -115,8 +129,10 @@
     return EXPLOITABILITY_ERR_PROCESSING;
   }
 
-  // Checking for the instruction pointer in a valid instruction region.
-  if (!this->InstructionPointerInCode(instruction_ptr)) {
+  // Checking for the instruction pointer in a valid instruction region
+  // or if the crash resulted during an illegal write.
+  if (!this->InstructionPointerInCode(instruction_ptr) ||
+       this->EndedOnIllegalWrite(instruction_ptr)) {
     return EXPLOITABILITY_HIGH;
   }
 
@@ -125,6 +141,318 @@
   return EXPLOITABILITY_INTERESTING;
 }
 
+bool ExploitabilityLinux::EndedOnIllegalWrite(uint64_t instruction_ptr) {

[ahonig, 2015/08/10 22:53:11]

This is a really long function, please break it into smaller pieces.

[liuandrew, 2015/08/17 21:37:36]

Done.

+  // Get memory region containing instruction pointer.
+  MinidumpMemoryList *memory_list = dump_->GetMemoryList();
+  MinidumpMemoryRegion *memory_region =
+      memory_list ?
+      memory_list->GetMemoryRegionForAddress(instruction_ptr) : NULL;
+  if (memory_region == NULL) {
+    BPLOG(ERROR) << "No memory region around instruction pointer.";
+    return false;
+  }
+
+  // Get exception data to find architecture.
+  string architecture = "";
+  MinidumpException *exception = dump_->GetException();
+  // This should never evaluate to true, since this should not be reachable
+  // without checking for exception data earlier.
+  if (exception == NULL) {
+    BPLOG(INFO) << "No exception data.";
+    return false;
+  }
+  const MDRawExceptionStream *raw_exception_stream = exception->exception();
+  const MinidumpContext *context = exception->GetContext();
+  // This should not evaluate to true, for the same reason mentioned above.
+  if (raw_exception_stream == NULL || context == NULL) {
+    BPLOG(INFO) << "No exception or architecture data.";
+    return false;
+  }
+  // Check architecture and set architecture variable to corresponding flag
+  // in objdump.
+  switch (context->GetContextCPU()) {
+    case MD_CONTEXT_X86:
+      architecture = "i386";
+      break;
+    case MD_CONTEXT_AMD64:
+      architecture = "i386:x86-64";
+      break;
+    default:
+      // Unsupported architecture. Note that ARM architectures are not
+      // supported because objdump does not support ARM.
+      return false;
+      break;
+  }
+
+  // Get memory region around instruction pointer and the number of bytes
+  // before and after the instruction pointer in the memory region.
+  const uint8_t *raw_memory = memory_region->GetMemory();
+  const uint64_t base = memory_region->GetBase();
+  if (base > instruction_ptr) {
+    BPLOG(ERROR) << "Memory region base value exceeds instruction pointer.";
+    return false;
+  }
+  const uint64_t offset = instruction_ptr - base;
+  if (memory_region->GetSize() - offset < MAX_INSTRUCTION_LEN) {

[ivanpe, 2015/08/10 23:27:58]

Can "memory_region->GetSize() - offset" become negative and underflow?  Maybe a
safer way to write this would be:

if (memory_region->GetSize() < offset + MAX_INSTRUCTION_LEN) {

[liuandrew, 2015/08/11 22:55:39]

Done.

+    BPLOG(ERROR) << "Not enough bytes left to guarantee complete instruction.";
+    return false;
+  }
+
+  // Write raw bytes around instruction pointer to a temporary file to
+  // pass as an argument to objdump.
+  char raw_bytes_tmpfile[] = "/tmp/breakpad_mem_region-raw_bytes-XXXXXX";
+  int raw_bytes_fd = mkstemp(raw_bytes_tmpfile);
+  if (raw_bytes_fd < 0) {
+    BPLOG(ERROR) << "Failed to create tempfile.";

[ahonig, 2015/08/10 22:53:11]

This should return rather than continue running with an invalid fd.

[liuandrew, 2015/08/11 22:55:38]

Done.

+  }
+  if (write(raw_bytes_fd, raw_memory + offset, MAX_INSTRUCTION_LEN)
+      != MAX_INSTRUCTION_LEN) {

[ahonig, 2015/08/10 22:53:11]

Add call to unlink and return.

[liuandrew, 2015/08/11 22:55:38]

Done.

+    BPLOG(ERROR) << "Writing of raw bytes failed.";
+  }
+
+  char objdump_output_buffer[MAX_OBJDUMP_BUFFER_LEN];
+  int pipe_fd[2];
+
+  // Open pipe between STDOUT and the objdump output buffer.
+  if (pipe(pipe_fd) < 0) {
+    BPLOG(ERROR) << "Failed to pipe.";
+    unlink(raw_bytes_tmpfile);
+    return false;
+  }
+
+  // Fork process to call objdump.
+  pid_t child_pid;
+  if ((child_pid = fork()) < 0) {
+    BPLOG(ERROR) << "Forking failed.";
+    unlink(raw_bytes_tmpfile);
+    return false;
+  }
+
+  if (child_pid) {  // Parent code.
+    close(pipe_fd[1]);
+    // Read piped output from objdump.
+    ssize_t bytes_read = read(pipe_fd[0],
+                              objdump_output_buffer,
+                              MAX_OBJDUMP_BUFFER_LEN);
+    int status;
+    wait(&status);  // Wait for child process to run objdump.
+    unlink(raw_bytes_tmpfile);
+    if (WEXITSTATUS(status)) {
+      BPLOG(ERROR) << "Child process encountered error.";
+      return false;
+    }
+    if (bytes_read < 0) {
+      BPLOG(ERROR) << "Failed to read objdump output.";
+      return false;
+    }
+    close(pipe_fd[0]);
+  } else {  // Child code.
+    close(pipe_fd[0]);
+    dup2(pipe_fd[1], STDOUT_FILENO);  // Send objdump output across pipe.
+    // Exec objdump.
+    execlp("objdump", "objdump", "-D", "-b", "binary", "-M", "intel",
+           "-m", architecture.c_str(), raw_bytes_tmpfile, NULL);
+    BPLOG(ERROR) << "Exec failed.";
+    exit(EX_OSERR);
+  }
+
+  // Set up regular expression to catch first instruction from objdump.
+  // The line with the instruction will begin with "0:".
+  regex_t regex;
+  regcomp(&regex, "0:", REG_EXTENDED | REG_NOSUB);
+
+  // Put buffer data into stream to output line-by-line.
+  std::stringstream objdump_stream;
+  objdump_stream.str(string(objdump_output_buffer));
+  string line;
+
+  // Pipe each output line into the string until the string contains
+  // the first instruction from objdump.
+  do {
+    if (!getline(objdump_stream, line)) {
+      BPLOG(ERROR) << "Objdump instructions not found";
+      return false;
+    }
+  } while (regexec(&regex, line.c_str(), 0, NULL, 0));

[ahonig, 2015/08/10 22:53:11]

What happens if this finishes prior to finding a line that matches your regex?

[liuandrew, 2015/08/11 22:55:39]

What do you mean? The terminating condition for the loop is a regex match.
Otherwise, if there are no lines, we get the error and return false.

I added a comment to clarify this in the code.

+  regfree(&regex);  // Free regex data.
+
+  // Tokenize the objdump line.
+  vector<string> tokens;
+  std::istringstream line_stream(line);
+  copy(std::istream_iterator<string>(line_stream),
+       std::istream_iterator<string>(),
+       std::back_inserter(tokens));
+
+  // Parse out the operator and operands from the instruction.
+  string instruction = "";
+  string operands = "";
+
+  // Regex for the data in hex form. Each byte is two hex digits.
+  regcomp(&regex, "^[[:xdigit:]]{2}$", REG_EXTENDED | REG_NOSUB);
+
+  // Find and set the location of the operator. The operator appears
+  // directly after the chain of bytes that define the instruction. The
+  // operands will be the last token, given that the instruction has operands.
+  // If not, the operator is the last token. The loop skips the first token
+  // because the first token is the instruction number (namely "0:").
+  for (size_t i = 1; i < tokens.size(); i++) {
+    // Check if current token no longer is in byte format.
+    if (regexec(&regex, tokens[i].c_str(), 0, NULL, 0)) {
+      instruction = tokens[i];
+      // If the operator is the last token, there are no operands.
+      if (i != tokens.size() - 1) {
+        operands = tokens[tokens.size() - 1];
+      }
+      break;
+    }
+  }
+  regfree(&regex);
+
+  if (instruction.empty()) {
+    BPLOG(ERROR) << "Failed to parse out operation from objdump instruction.";
+    return false;
+  }
+
+  // Split operands into source and destination (if applicable).
+  string dest = "";
+  string src = "";
+  if (!operands.empty()) {
+    size_t delim = operands.find(',');
+    if (delim == string::npos) {
+      dest = operands;
+    } else {
+      dest = operands.substr(0, delim);
+      src = operands.substr(delim + 1);
+    }
+  }
+
+  // Check if the operation is a write to memory. First, the instruction
+  // must one that can write to memory. Second, the write destination
+  // must be a spot in memory rather than a register. Since there are no
+  // symbols from objdump, the destination will be enclosed by brackets.
+  if (dest.at(0) == '[' && dest.at(dest.size() - 1) == ']' &&

[ahonig, 2015/08/10 22:53:11]

Is this reasonable if dest is empty?

[liuandrew, 2015/08/11 22:55:38]

No this is not reasonable at all. I added a length check.

+      (!instruction.compare("mov") || !instruction.compare("inc") ||
+       !instruction.compare("dec") || !instruction.compare("and") ||
+       !instruction.compare("or") || !instruction.compare("xor") ||
+       !instruction.compare("not") || !instruction.compare("neg") ||
+       !instruction.compare("add") || !instruction.compare("sub") ||
+       !instruction.compare("shl") || !instruction.compare("shr"))) {
+    // Strip away enclosing brackets from the destination address.
+    dest = dest.substr(1, dest.size() - 2);

[ahonig, 2015/08/10 22:53:11]

What happens if dest is []

[liuandrew, 2015/08/11 22:55:39]

This is addressed in the comment above.

+
+    // The destination should be the format [reg+a] or [reg-a], where reg
+    // is a register and a is a hexadecimal constant. Although more complex
+    // expressions can make valid instructions, objdump's disassembly outputs
+    // it in this simpler format.
+
+    // Parse out the constant that is added to the address (if it exists).
+    size_t delim = dest.find('+');
+    bool positive_add_constant = true;
+    // Check if constant is subtracted instead of added.
+    if (delim == string::npos) {
+      positive_add_constant = false;
+      delim = dest.find('-');

[ahonig, 2015/08/10 22:53:11]

There are definitely more complicated destinations that just reg+/reg-.  Some
examples include

mov [eax+4*ecx+5], edx

I'm not saying you need to handle that case in this change, but make sure you
handle it gracefully and put a TODO that it's not done yet.

[liuandrew, 2015/08/11 22:55:38]

Added TODO.

I'm not sure if that's the case. I tried writing in x86 some cases where the
destination address had some more complicated arithmetic, such as
multiplication. The only cases that would compile for me were the cases of a
single register and addition/subtraction.

Then, for those cases, after compiling and disassembling in objdump, the
resultant format would be a register +/- offset.

+    }
+    uint32_t add_constant = 0;
+    // Save constant and remove it from the expression.
+    if (delim != string::npos) {
+      sscanf(dest.substr(delim + 1).c_str(), "%x", &add_constant);
+      dest = dest.substr(0, delim);
+    }
+
+    // Calculate and set the address that is the target of the write operation.
+    uint64_t write_address = 0;
+
+    // Set the the write address to the corresponding register.
+    // TODO(liuandrew): Add support for partial registers, such as
+    // the rax/eax/ax/ah/al chain.
+    switch (context->GetContextCPU()) {
+      case MD_CONTEXT_X86:
+        if (!dest.compare("eax")) {
+            write_address = context->GetContextX86()->eax;
+        } else if (!dest.compare("ebx")) {
+            write_address = context->GetContextX86()->ebx;
+        } else if (!dest.compare("ecx")) {
+            write_address = context->GetContextX86()->ecx;
+        } else if (!dest.compare("edx")) {
+            write_address = context->GetContextX86()->edx;
+        } else if (!dest.compare("edi")) {
+            write_address = context->GetContextX86()->edi;
+        } else if (!dest.compare("esi")) {
+            write_address = context->GetContextX86()->esi;
+        } else if (!dest.compare("ebp")) {
+            write_address = context->GetContextX86()->ebp;
+        } else if (!dest.compare("esp")) {
+            write_address = context->GetContextX86()->esp;
+        } else if (!dest.compare("eip")) {
+            write_address = context->GetContextX86()->eip;
+        } else {
+          BPLOG(ERROR) << "Unsupported register";
+          return false;
+        }
+        break;
+      case MD_CONTEXT_AMD64:
+        if (!dest.compare("rax")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("rbx")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("rcx")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("rdx")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("rdi")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("rsi")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("rbp")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("rsp")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("rip")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("r8")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("r9")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("r10")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("r11")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("r12")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("r13")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("r14")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else if (!dest.compare("r15")) {
+          write_address = context->GetContextAMD64()->rax;
+        } else {
+          BPLOG(ERROR) << "Unsupported register";
+          return false;
+        }
+        break;
+      default:
+        // This should not occur since the same switch condition
+        // should have terminated this method.
+        return false;
+        break;
+    }
+
+    // Add or subtract constant from write address (if applicable).
+    write_address =
+        positive_add_constant ?
+        write_address + add_constant : write_address - add_constant;
+
+    // If the program crashed as a result of a write, the destination of
+    // the write must have been an address that did not permit writing.
+    // However, if the address is under 4k, due to program protections,
+    // the crash does not suggest exploitability for writes with such a
+    // low target address.
+    return write_address > 4096;
+  }
+  return false;
+}
+
 bool ExploitabilityLinux::InstructionPointerInCode(uint64_t instruction_ptr) {
   // Get Linux memory mapping from /proc/self/maps. Checking whether the
   // region the instruction pointer is in has executable permission can tell