From 10bceb552f1cfd2ddae3c8bfd7d9b38a66e710c4 Mon Sep 17 00:00:00 2001
From: Jay Berkenbilt <ejb@ql.org>
Date: Sat, 5 Oct 2013 12:28:52 -0400
Subject: [PATCH] Security: sanitize /W in xref stream

The /W array was not sanitized, possibly causing an integer overflow
in a multiplication. An analysis of the code suggests that there were
no possible exploits based on this since the problems were in checking
expected values but bounds checks were performed on actual values.
---
 ChangeLog       |  5 +++++
 libqpdf/QPDF.cc | 48 +++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8a10865f..124a086d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,10 @@
 2013-10-05  Jay Berkenbilt  <ejb@ql.org>
 
+	* Security fix: sanitize /W array in cross reference stream to
+	avoid a potential integer overflow in a multiplication.  It is
+	unlikely that any exploits were possible from this bug as
+	additional checks were also performed.
+
 	* Security fix: avoid buffer overrun that could be caused by bogus
 	data in linearization hint streams.  The incorrect code could only
 	be triggered when checking linearization data, which must be
diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc
index 73207ec9..39fd7208 100644
--- a/libqpdf/QPDF.cc
+++ b/libqpdf/QPDF.cc
@@ -699,7 +699,26 @@ QPDF::processXRefStream(qpdf_offset_t xref_offset, QPDFObjectHandle& xref_obj)
 		      "Cross-reference stream does not have"
 		      " proper /W and /Index keys");
     }
-    std::vector<int> indx;
+
+    int W[3];
+    size_t entry_size = 0;
+    int max_bytes = sizeof(qpdf_offset_t);
+    for (int i = 0; i < 3; ++i)
+    {
+	W[i] = W_obj.getArrayItem(i).getIntValue();
+        if (W[i] > max_bytes)
+        {
+            throw QPDFExc(qpdf_e_damaged_pdf, this->file->getName(),
+                          "xref stream", xref_offset,
+                          "Cross-reference stream's /W contains"
+                          " impossibly large values");
+        }
+	entry_size += W[i];
+    }
+    long long max_num_entries =
+        static_cast<unsigned long long>(-1) / entry_size;
+
+    std::vector<long long> indx;
     if (Index_obj.isArray())
     {
 	int n_index = Index_obj.getArrayNItems();
@@ -731,25 +750,29 @@ QPDF::processXRefStream(qpdf_offset_t xref_offset, QPDFObjectHandle& xref_obj)
     else
     {
 	QTC::TC("qpdf", "QPDF xref /Index is null");
-	int size = dict.getKey("/Size").getIntValue();
+	long long size = dict.getKey("/Size").getIntValue();
 	indx.push_back(0);
 	indx.push_back(size);
     }
 
-    int num_entries = 0;
+    long long num_entries = 0;
     for (unsigned int i = 1; i < indx.size(); i += 2)
     {
+        if (indx[i] > max_num_entries - num_entries)
+        {
+            throw QPDFExc(qpdf_e_damaged_pdf, this->file->getName(),
+                          "xref stream", xref_offset,
+                          "Cross-reference stream claims to contain"
+                          " too many entries: " +
+                          QUtil::int_to_string(indx[i]) + " " +
+                          QUtil::int_to_string(max_num_entries) + " " +
+                          QUtil::int_to_string(num_entries));
+        }
 	num_entries += indx[i];
     }
 
-    int W[3];
-    int entry_size = 0;
-    for (int i = 0; i < 3; ++i)
-    {
-	W[i] = W_obj.getArrayItem(i).getIntValue();
-	entry_size += W[i];
-    }
-
+    // entry_size and num_entries have both been validated to ensure
+    // that this multiplication does not cause an overflow.
     size_t expected_size = entry_size * num_entries;
 
     PointerHolder<Buffer> bp = xref_obj.getStreamData();
@@ -777,6 +800,9 @@ QPDF::processXRefStream(qpdf_offset_t xref_offset, QPDFObjectHandle& xref_obj)
 
     bool saw_first_compressed_object = false;
 
+    // Actual size vs. expected size check above ensures that we will
+    // not overflow any buffers here.  We know that entry_size *
+    // num_entries is equal to the size of the buffer.
     unsigned char const* data = bp->getBuffer();
     for (int i = 0; i < num_entries; ++i)
     {