From 0f740c43ab4a9c386fba46cd1d2aa4864e498e25 Mon Sep 17 00:00:00 2001
From: William Woodruff <william.woodruff@trailofbits.com>
Date: Thu, 20 Sep 2018 10:57:22 -0400
Subject: [PATCH] buffer, parse: Fix overread on {d,q,}words

When read 2, 4, or 8 bytes from a bounded_buffer, we only
checked to see if the offset, not the whole span, was in bounds.

This results in an arbitrary memory read of up to 1, 3, or 7 bytes
when the offset is aligned with the very end of the buffer.
---
 pe-parser-library/include/parser-library/parse.h |  4 +++-
 pe-parser-library/src/buffer.cpp                 | 14 +++++++++++---
 pe-parser-library/src/parse.cpp                  |  4 +++-
 3 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/pe-parser-library/include/parser-library/parse.h b/pe-parser-library/include/parser-library/parse.h
index b3fa974..194af11 100644
--- a/pe-parser-library/include/parser-library/parse.h
+++ b/pe-parser-library/include/parser-library/parse.h
@@ -133,7 +133,9 @@ enum pe_err {
   PEERR_READ = 6,
   PEERR_OPEN = 7,
   PEERR_STAT = 8,
-  PEERR_MAGIC = 9
+  PEERR_MAGIC = 9,
+  PEERR_BUFFER = 10,
+  PEERR_ADDRESS = 11,
 };
 
 bool readByte(bounded_buffer *b, std::uint32_t offset, std::uint8_t &out);
diff --git a/pe-parser-library/src/buffer.cpp b/pe-parser-library/src/buffer.cpp
index ca9f05a..60be897 100644
--- a/pe-parser-library/src/buffer.cpp
+++ b/pe-parser-library/src/buffer.cpp
@@ -86,10 +86,12 @@ struct buffer_detail {
 
 bool readByte(bounded_buffer *b, std::uint32_t offset, std::uint8_t &out) {
   if (b == nullptr) {
+    PE_ERR(PEERR_BUFFER);
     return false;
   }
 
   if (offset >= b->bufLen) {
+    PE_ERR(PEERR_ADDRESS);
     return false;
   }
 
@@ -101,10 +103,12 @@ bool readByte(bounded_buffer *b, std::uint32_t offset, std::uint8_t &out) {
 
 bool readWord(bounded_buffer *b, std::uint32_t offset, std::uint16_t &out) {
   if (b == nullptr) {
+    PE_ERR(PEERR_BUFFER);
     return false;
   }
 
-  if (offset >= b->bufLen) {
+  if (offset + 1 >= b->bufLen) {
+    PE_ERR(PEERR_ADDRESS);
     return false;
   }
 
@@ -120,10 +124,12 @@ bool readWord(bounded_buffer *b, std::uint32_t offset, std::uint16_t &out) {
 
 bool readDword(bounded_buffer *b, std::uint32_t offset, std::uint32_t &out) {
   if (b == nullptr) {
+    PE_ERR(PEERR_BUFFER);
     return false;
   }
 
-  if (offset >= b->bufLen) {
+  if (offset + 3 >= b->bufLen) {
+    PE_ERR(PEERR_ADDRESS);
     return false;
   }
 
@@ -139,10 +145,12 @@ bool readDword(bounded_buffer *b, std::uint32_t offset, std::uint32_t &out) {
 
 bool readQword(bounded_buffer *b, std::uint32_t offset, std::uint64_t &out) {
   if (b == nullptr) {
+    PE_ERR(PEERR_BUFFER);
     return false;
   }
 
-  if (offset >= b->bufLen) {
+  if (offset + 7 >= b->bufLen) {
+    PE_ERR(PEERR_ADDRESS);
     return false;
   }
 
diff --git a/pe-parser-library/src/parse.cpp b/pe-parser-library/src/parse.cpp
index 9bf6e4e..540eb6f 100644
--- a/pe-parser-library/src/parse.cpp
+++ b/pe-parser-library/src/parse.cpp
@@ -136,7 +136,9 @@ static const char *pe_err_str[] = {"None",
                                    "Unable to read data",
                                    "Unable to open",
                                    "Unable to stat",
-                                   "Bad magic"};
+                                   "Bad magic",
+                                   "Invalid buffer",
+                                   "Invalid address",};
 
 std::uint32_t GetPEErr() {
   return err;