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.

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.
Jay Berkenbilt
1 parent 3eb4b066
Showing 2 changed files with 42 additions and 11 deletions
ChangeLog
libqpdf/QPDF.cc
 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
@@ -699,7 +699,26 @@ QPDF::processXRefStream(qpdf_offset_t xref_offset, QPDFObjectHandle&amp; 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&amp; 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&amp; 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)
     {