diff options
author | Jay Berkenbilt <ejb@ql.org> | 2013-10-05 18:28:52 +0200 |
---|---|---|
committer | Jay Berkenbilt <ejb@ql.org> | 2013-10-10 02:57:07 +0200 |
commit | 10bceb552f1cfd2ddae3c8bfd7d9b38a66e710c4 (patch) | |
tree | 35fab8055e7eb30f4a13aa6aabba1ec0aeac2d6f | |
parent | 3eb4b066ab3f25f6454214d33b2fc17161812dfa (diff) | |
download | qpdf-10bceb552f1cfd2ddae3c8bfd7d9b38a66e710c4.tar.zst |
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.
-rw-r--r-- | ChangeLog | 5 | ||||
-rw-r--r-- | libqpdf/QPDF.cc | 48 |
2 files changed, 42 insertions, 11 deletions
@@ -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) { |