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 /libqpdf/QPDF.cc | |
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.
Diffstat (limited to 'libqpdf/QPDF.cc')
-rw-r--r-- | libqpdf/QPDF.cc | 48 |
1 files changed, 37 insertions, 11 deletions
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) { |