diff --git a/ChangeLog b/ChangeLog index 8a10865f..124a086d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ 2013-10-05 Jay Berkenbilt + * 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 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(-1) / entry_size; + + std::vector 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 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) {