From 0bfe9024893ebb1f62108fe6c24410e6ba589c3e Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sat, 5 Oct 2013 11:30:27 -0400 Subject: [PATCH] Security: avoid pre-allocating vectors based on file data In places where std::vector(size_t) was used, either validate that the size parameter is sane or refactor code to avoid the need to pre-allocate the vector. --- ChangeLog | 6 +++ libqpdf/QPDF_linearization.cc | 43 ++++++++++++++---- qpdf/qtest/qpdf.test | 9 +++- .../qpdf/linearization-large-vector-alloc.out | 6 +++ .../qpdf/linearization-large-vector-alloc.pdf | Bin 0 -> 12302 bytes 5 files changed, 55 insertions(+), 9 deletions(-) create mode 100644 qpdf/qtest/qpdf/linearization-large-vector-alloc.out create mode 100644 qpdf/qtest/qpdf/linearization-large-vector-alloc.pdf diff --git a/ChangeLog b/ChangeLog index 124a086d..f87c4418 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,11 @@ 2013-10-05 Jay Berkenbilt + * Security fix: In places where std::vector(size_t) was used, + either validate that the size parameter is sane or refactor code + to avoid the need to pre-allocate the vector. This reduces the + likelihood of allocating a lot of memory in response to invalid + data in linearization hint streams. + * 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 diff --git a/libqpdf/QPDF_linearization.cc b/libqpdf/QPDF_linearization.cc index dd09b1c0..7d2560d3 100644 --- a/libqpdf/QPDF_linearization.cc +++ b/libqpdf/QPDF_linearization.cc @@ -23,13 +23,22 @@ static void load_vector_int(BitStream& bit_stream, int nitems, std::vector& vec, int bits_wanted, int_type T::*field) { + bool append = vec.empty(); // nitems times, read bits_wanted from the given bit stream, // storing results in the ith vector entry. for (int i = 0; i < nitems; ++i) { + if (append) + { + vec.push_back(T()); + } vec[i].*field = bit_stream.getBits(bits_wanted); } + if (static_cast(vec.size()) != nitems) + { + throw std::logic_error("vector has wrong size in load_vector_int"); + } // The PDF spec says that each hint table starts at a byte // boundary. Each "row" actually must start on a byte boundary. bit_stream.skipToNextByte(); @@ -255,6 +264,17 @@ QPDF::readLinearizationData() // Store linearization parameter data + // Various places in the code use linp.npages, which is + // initialized from N, to pre-allocate memory, so make sure it's + // accurate and bail right now if it's not. + if (N.getIntValue() != static_cast(getAllPages().size())) + { + throw QPDFExc(qpdf_e_damaged_pdf, this->file->getName(), + "linearization hint table", + this->file->getLastOffset(), + "/N does not match number of pages"); + } + // file_size initialized by isLinearized() this->linp.first_page_object = O.getIntValue(); this->linp.first_page_end = E.getIntValue(); @@ -396,10 +416,9 @@ QPDF::readHPageOffset(BitStream h) t.nbits_shared_numerator = h.getBits(16); // 12 t.shared_denominator = h.getBits(16); // 13 - unsigned int nitems = this->linp.npages; std::vector& entries = t.entries; - entries = std::vector(nitems); - + entries.clear(); + unsigned int nitems = this->linp.npages; load_vector_int(h, nitems, entries, t.nbits_delta_nobjects, &HPageOffsetEntry::delta_nobjects); @@ -441,10 +460,9 @@ QPDF::readHSharedObject(BitStream h) QTC::TC("qpdf", "QPDF lin nshared_total > nshared_first_page", (t.nshared_total > t.nshared_first_page) ? 1 : 0); - int nitems = t.nshared_total; std::vector& entries = t.entries; - entries = std::vector(nitems); - + entries.clear(); + int nitems = t.nshared_total; load_vector_int(h, nitems, entries, t.nbits_delta_group_length, &HSharedObjectEntry::delta_group_length); @@ -1466,8 +1484,11 @@ QPDF::calculateLinearizationData(std::map const& object_stream_data) // validation code can compute them relatively easily given the // rest of the information. + // npages is the size of the existing pages vector, which has been + // created by traversing the pages tree, and as such is a + // reasonable size. this->c_linp.npages = npages; - this->c_page_offset_data.entries = std::vector(npages); + this->c_page_offset_data.entries = std::vector(npages); // Part 4: open document objects. We don't care about the order. @@ -1861,6 +1882,7 @@ QPDF::calculateHPageOffset( HPageOffset& ph = this->page_offset_hints; std::vector& phe = ph.entries; + // npages is the size of the existing pages array. phe = std::vector(npages); for (unsigned int i = 0; i < npages; ++i) @@ -1935,7 +1957,7 @@ QPDF::calculateHSharedObject( std::vector& csoe = cso.entries; HSharedObject& so = this->shared_object_hints; std::vector& soe = so.entries; - soe = std::vector(cso.nshared_total); + soe.clear(); int min_length = outputLengthNextN( csoe[0].object, 1, lengths, obj_renumber); @@ -1948,8 +1970,13 @@ QPDF::calculateHSharedObject( csoe[i].object, 1, lengths, obj_renumber); min_length = std::min(min_length, length); max_length = std::max(max_length, length); + soe.push_back(HSharedObjectEntry()); soe[i].delta_group_length = length; } + if (soe.size() != static_cast(cso.nshared_total)) + { + throw std::logic_error("soe has wrong size after initialization"); + } so.nshared_total = cso.nshared_total; so.nshared_first_page = cso.nshared_first_page; diff --git a/qpdf/qtest/qpdf.test b/qpdf/qtest/qpdf.test index 87c9d8c1..d07a54c5 100644 --- a/qpdf/qtest/qpdf.test +++ b/qpdf/qtest/qpdf.test @@ -199,7 +199,7 @@ $td->runtest("remove page we don't have", show_ntests(); # ---------- $td->notify("--- Miscellaneous Tests ---"); -$n_tests += 69; +$n_tests += 70; $td->runtest("qpdf version", {$td->COMMAND => "qpdf --version"}, @@ -537,6 +537,13 @@ $td->runtest("bounds check linearization data 2", {$td->FILE => "linearization-bounds-2.out", $td->EXIT_STATUS => 2}, $td->NORMALIZE_NEWLINES); +# Throws logic error, not bad_alloc +$td->runtest("sanity check array size", + {$td->COMMAND => + "qpdf --check linearization-large-vector-alloc.pdf"}, + {$td->FILE => "linearization-large-vector-alloc.out", + $td->EXIT_STATUS => 2}, + $td->NORMALIZE_NEWLINES); show_ntests(); # ---------- diff --git a/qpdf/qtest/qpdf/linearization-large-vector-alloc.out b/qpdf/qtest/qpdf/linearization-large-vector-alloc.out new file mode 100644 index 00000000..2c807d3c --- /dev/null +++ b/qpdf/qtest/qpdf/linearization-large-vector-alloc.out @@ -0,0 +1,6 @@ +checking linearization-large-vector-alloc.pdf +PDF Version: 1.3 +File is not encrypted +File is linearized +WARNING: linearization-large-vector-alloc.pdf (linearization hint stream: object 62 0, file position 1183): attempting to recover stream length +overflow reading bit stream diff --git a/qpdf/qtest/qpdf/linearization-large-vector-alloc.pdf b/qpdf/qtest/qpdf/linearization-large-vector-alloc.pdf new file mode 100644 index 0000000000000000000000000000000000000000..1807b08f8950586aa9242ba3f5066e782223d9b7 GIT binary patch literal 12302 zcmc&)&5I;S72kDq4h0K>H&21l2e6-c&x;l(hc2!nSt<_aa zRrWF}ii02uf`hjO@!-+3coSVj@Fe~L9z1vwyoj>P3gUb5QJoo)+0b3RWo6d`TIsH~KCJe3!-joMA^DGN!f~i1fA$TA|NLBGwxy+}tVo5Na`in(L64*fcBsze!BET`)Ul#9T zvM3oONc6@od9d@yLRW{UQg@dlKtUHIfHy(5p)r1xSt;t_wvJHw$j=^ zI5}QH;!}OGPqwZDPr~)p0g0AiQ3+K7G*w%X)mJ3A0s@2na7@^m%dY0a^_tjiG(@w} zaB~|WaL5P5n>D})b@JG;gvOcvLb&BdTgX*h?r8{F4OH})al!q?xFFG&SH;>v+A}Kx zOtqf|-+Tua+|J8*&ql-H@NhW%(cv&34nO?k?|%Qn6_R}B%B#=+@XznP_$isa_T-hTS0?ib@} zerxemrv!zx=8(-A4)^mVtX#XNU#u1<%Mm0N#%d4vm=BC3fJeYeJ9@bo9TZRi|P6luH?hG1i zMwJ}@TluQc`bKw)*}FwK9pwP?bBNt^eng_LOy{r9SJP9%gZvm00c1x9Cx@lZ3N!+) zjV+p2B|p$sl=TP_ZFdh=6;Dg@c%432hEA2+ixco$0^Fy|6?_0h#UR|v@uHT8>nyDZ z!qs(75=y3ky^o09{MJ{$M&>88S>+Gh*^D%W_e3X6S^Ro{$~w_DxTFhnx09Q2}=(ame63hPQ+_dy?{L&_6S&v>-iWy!!>@!?*#A_x&030 z$OQPqeg!gQYS#fT0W`q@nN3B7i38<+>J=NO(=hdQ;xOpkQc4t92w^j^sXM}js**md z>FCK~360=QZbM`uv8Hd zo92~Vvc7>_dJ`HI?KGQ9wo@>g1Ff72GT4k5-+;~$9ILn1qd^VZ_ zyt;a{iZ=Vws%&mB205`&tE9iv(zw~yv2}AzWrHKAawzI7=2ao!{v8{);!F7@Tf(X>=2co!{v8 z9*j4_81EFuwDir>yS)$NjX1_T#W5{?bEDgPF@31#1gx7ec!JhpdTZ%UC)bRfKfT+# zF?dYa0A!+5Ak)$}H@dwY!zXkhdW%khOiSO~==Oe0AE-D1r|I1(kcp;#?ELB7-jIn# zAQPPenU?MXaq*gN@5n@BMr%O>60! z8{OWK#f?A~cM4=$`sPNrcVuy6M}|{f9Y*?6ep>SOjx25jGMFXk(&)O-m&icFh_!aF*EuwzJ`Ah1p7FsrvI7m zaefksW(6}#^r2Lr=wllAXO`%n@^EFA=%0Ha#Bj0CSap*OM6*OUQhj2d2J4$GTp2+c zkU=}6i!NHYGJ~##(H~II2bBZZ1TNzSqimPr@`=K zU`Gs-y}>$xM?T(OiL(+OEylqCkETMu#3?>|@YZEGC0M8FaYdg7Q?J1~K?8n?#Jh&a zFUAb=Xh|0GQ(4HbVE`9KeFA=9Zop6RG~lNso-@>SQ8CtuM@z!|gOm*UWqABs-3iCP5U|`%|m%-GhN6Q!%d!1j#v8(rJ0%k0I znhb}jvjp+Q+pF-n!=t5PEo50R=*%cXNUQB-l;bJ1M-u@?Fc{|Y?PV}T7V=Zo`lt?0 T>se`-W&gP5p*c(JMD+gvAB9OH literal 0 HcmV?d00001