From 44a13951940def9699b2c169e8a8eabd35973934 Mon Sep 17 00:00:00 2001 From: m-holger Date: Fri, 20 Sep 2024 13:58:47 +0100 Subject: [PATCH 1/3] Refactor QPDF::Xref_table::read_entry and read_bad_entry Return results rather than using reference parameters. Fixes bug in #1272 where parameters were not reinitialized when calling read_bad_entry from read_entry. --- libqpdf/QPDF.cc | 54 +++++++++++++++++++----------------- libqpdf/qpdf/QPDF_private.hh | 4 +-- 2 files changed, 30 insertions(+), 28 deletions(-) diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc index f263551a..e9802b52 100644 --- a/libqpdf/QPDF.cc +++ b/libqpdf/QPDF.cc @@ -832,10 +832,6 @@ std::vector QPDF::Xref_table::bad_subsections(std::string& line, qpdf_offset_t start) { std::vector result; - qpdf_offset_t f1 = 0; - int f2 = 0; - char type = '\0'; - file->seek(start, SEEK_SET); while (true) { @@ -844,7 +840,7 @@ QPDF::Xref_table::bad_subsections(std::string& line, qpdf_offset_t start) auto [obj, num, offset] = result.emplace_back(subsection(line)); file->seek(offset, SEEK_SET); for (qpdf_offset_t i = obj; i - num < obj; ++i) { - if (!read_entry(f1, f2, type)) { + if (!std::get<0>(read_entry())) { QTC::TC("qpdf", "QPDF invalid xref entry"); throw damaged_table("invalid xref entry (obj=" + std::to_string(i) + ")"); } @@ -890,9 +886,13 @@ QPDF::Xref_table::subsections(std::string& line) } } -bool -QPDF::Xref_table::read_bad_entry(qpdf_offset_t& f1, int& f2, char& type) +// Returns (success, f1, f2, type). +std::tuple +QPDF::Xref_table::read_bad_entry() { + qpdf_offset_t f1{0}; + int f2{0}; + char type{'\0'}; // Reposition after initial read attempt and reread. file->seek(file->getLastOffset(), SEEK_SET); auto line = file->readLine(30); @@ -910,7 +910,7 @@ QPDF::Xref_table::read_bad_entry(qpdf_offset_t& f1, int& f2, char& type) } // Require digit if (!QUtil::is_digit(*p)) { - return false; + return {false, 0, 0, '\0'}; } // Gather digits std::string f1_str; @@ -919,7 +919,7 @@ QPDF::Xref_table::read_bad_entry(qpdf_offset_t& f1, int& f2, char& type) } // Require space if (!QUtil::is_space(*p)) { - return false; + return {false, 0, 0, '\0'}; } if (QUtil::is_space(*(p + 1))) { QTC::TC("qpdf", "QPDF ignore first extra space in xref entry"); @@ -931,7 +931,7 @@ QPDF::Xref_table::read_bad_entry(qpdf_offset_t& f1, int& f2, char& type) } // Require digit if (!QUtil::is_digit(*p)) { - return false; + return {false, 0, 0, '\0'}; } // Gather digits std::string f2_str; @@ -940,7 +940,7 @@ QPDF::Xref_table::read_bad_entry(qpdf_offset_t& f1, int& f2, char& type) } // Require space if (!QUtil::is_space(*p)) { - return false; + return {false, 0, 0, '\0'}; } if (QUtil::is_space(*(p + 1))) { QTC::TC("qpdf", "QPDF ignore second extra space in xref entry"); @@ -953,7 +953,7 @@ QPDF::Xref_table::read_bad_entry(qpdf_offset_t& f1, int& f2, char& type) if ((*p == 'f') || (*p == 'n')) { type = *p; } else { - return false; + return {false, 0, 0, '\0'}; } if ((f1_str.length() != 10) || (f2_str.length() != 5)) { QTC::TC("qpdf", "QPDF ignore length error xref entry"); @@ -967,18 +967,23 @@ QPDF::Xref_table::read_bad_entry(qpdf_offset_t& f1, int& f2, char& type) f1 = QUtil::string_to_ll(f1_str.c_str()); f2 = QUtil::string_to_int(f2_str.c_str()); - return true; + return {true, f1, f2, type}; } // Optimistically read and parse xref entry. If entry is bad, call read_bad_xrefEntry and return -// result. -bool -QPDF::Xref_table::read_entry(qpdf_offset_t& f1, int& f2, char& type) +// result. Returns (success, f1, f2, type). +std::tuple +QPDF::Xref_table::read_entry() { + qpdf_offset_t f1{0}; + int f2{0}; + char type{'\0'}; std::array line; + f1 = 0; + f2 = 0; if (file->read(line.data(), 20) != 20) { // C++20: [[unlikely]] - return false; + return {false, 0, 0, '\0'}; } line[20] = '\0'; char const* p = line.data(); @@ -1002,7 +1007,7 @@ QPDF::Xref_table::read_entry(qpdf_offset_t& f1, int& f2, char& type) if (!QUtil::is_space(*p++)) { // Entry doesn't start with space or digit. // C++20: [[unlikely]] - return false; + return {false, 0, 0, '\0'}; } // Gather digits. NB No risk of overflow as 99'999 < max int. while (*p == '0') { @@ -1019,10 +1024,10 @@ QPDF::Xref_table::read_entry(qpdf_offset_t& f1, int& f2, char& type) // No test for valid line[19]. if (*(++p) && *(++p) && (*p == '\n' || *p == '\r') && f1_len == 10 && f2_len == 5) { // C++20: [[likely]] - return true; + return {true, f1, f2, type}; } } - return read_bad_entry(f1, f2, type); + return read_bad_entry(); } // Read a single cross-reference table section and associated trailer. @@ -1064,10 +1069,8 @@ QPDF::Xref_table::process_section(qpdf_offset_t xref_offset) first_item_offset_ = file->tell(); } // For xref_table, these will always be small enough to be ints - qpdf_offset_t f1 = 0; - int f2 = 0; - char type = '\0'; - if (!read_entry(f1, f2, type)) { + auto [success, f1, f2, type] = read_entry(); + if (!success) { throw damaged_table("invalid xref entry (obj=" + std::to_string(i) + ")"); } if (type == 'f') { @@ -1585,8 +1588,7 @@ QPDF::Xref_table::read_trailer() { qpdf_offset_t offset = file->tell(); bool empty = false; - auto object = - QPDFParser(*file, "trailer", tokenizer, nullptr, &qpdf, true).parse(empty, false); + auto object = QPDFParser(*file, "trailer", tokenizer, nullptr, &qpdf, true).parse(empty, false); if (empty) { // Nothing in the PDF spec appears to allow empty objects, but they have been encountered in // actual PDF files and Adobe Reader appears to ignore them. diff --git a/libqpdf/qpdf/QPDF_private.hh b/libqpdf/qpdf/QPDF_private.hh index b055763a..fa14fdc3 100644 --- a/libqpdf/qpdf/QPDF_private.hh +++ b/libqpdf/qpdf/QPDF_private.hh @@ -292,8 +292,8 @@ class QPDF::Xref_table std::vector subsections(std::string& line); std::vector bad_subsections(std::string& line, qpdf_offset_t offset); Subsection subsection(std::string const& line); - bool read_entry(qpdf_offset_t& f1, int& f2, char& type); - bool read_bad_entry(qpdf_offset_t& f1, int& f2, char& type); + std::tuple read_entry(); + std::tuple read_bad_entry(); // Methods to parse streams qpdf_offset_t read_stream(qpdf_offset_t offset); From 21f176d374dd229401b5de5bf8a4cb89b10e1731 Mon Sep 17 00:00:00 2001 From: m-holger Date: Fri, 20 Sep 2024 14:20:34 +0100 Subject: [PATCH 2/3] Add sanity check on trailer /Size entry --- libqpdf/QPDF.cc | 5 ++++- qpdf/qpdf.testcov | 1 + qpdf/qtest/qpdf/issue-fuzz.out | 19 +++++++++++++++++++ qpdf/qtest/qpdf/issue-fuzz.pdf | Bin 0 -> 81 bytes qpdf/qtest/specific-bugs.test | 1 + 5 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 qpdf/qtest/qpdf/issue-fuzz.out create mode 100644 qpdf/qtest/qpdf/issue-fuzz.pdf diff --git a/libqpdf/QPDF.cc b/libqpdf/QPDF.cc index e9802b52..5a38ec94 100644 --- a/libqpdf/QPDF.cc +++ b/libqpdf/QPDF.cc @@ -1057,7 +1057,10 @@ QPDF::Xref_table::process_section(qpdf_offset_t xref_offset) QTC::TC("qpdf", "QPDF trailer size not integer"); throw qpdf.damagedPDF("trailer", "/Size key in trailer dictionary is not an integer"); } - + if (sz >= static_cast(max_id_)) { + QTC::TC("qpdf", "QPDF trailer size impossibly large"); + throw qpdf.damagedPDF("trailer", "/Size key in trailer dictionary is impossibly large"); + } table.resize(sz); } diff --git a/qpdf/qpdf.testcov b/qpdf/qpdf.testcov index b66ba83f..25e4dd70 100644 --- a/qpdf/qpdf.testcov +++ b/qpdf/qpdf.testcov @@ -55,6 +55,7 @@ QPDF invalid xref entry 0 QPDF missing trailer 0 QPDF trailer lacks size 0 QPDF trailer size not integer 0 +QPDF trailer size impossibly large 0 QPDF trailer prev not integer 0 QPDFParser bad brace 0 QPDFParser bad brace in parseRemainder 0 diff --git a/qpdf/qtest/qpdf/issue-fuzz.out b/qpdf/qtest/qpdf/issue-fuzz.out new file mode 100644 index 00000000..456485b2 --- /dev/null +++ b/qpdf/qtest/qpdf/issue-fuzz.out @@ -0,0 +1,19 @@ +WARNING: issue-fuzz.pdf: can't find PDF header +WARNING: issue-fuzz.pdf (xref table, offset 19): accepting invalid xref table entry +WARNING: issue-fuzz.pdf (trailer, offset 36): unknown token while reading object; treating as string +WARNING: issue-fuzz.pdf (trailer, offset 53): unexpected > +WARNING: issue-fuzz.pdf (trailer, offset 54): unknown token while reading object; treating as string +WARNING: issue-fuzz.pdf (trailer, offset 58): unknown token while reading object; treating as string +WARNING: issue-fuzz.pdf (trailer, offset 72): unknown token while reading object; treating as string +WARNING: issue-fuzz.pdf (trailer, offset 36): dictionary ended prematurely; using null as value for last key +WARNING: issue-fuzz.pdf (trailer, offset 36): expected dictionary key but found non-name object; inserting key /QPDFFake1 +WARNING: issue-fuzz.pdf (trailer, offset 36): expected dictionary key but found non-name object; inserting key /QPDFFake2 +WARNING: issue-fuzz.pdf (trailer, offset 36): expected dictionary key but found non-name object; inserting key /QPDFFake3 +WARNING: issue-fuzz.pdf (trailer, offset 36): expected dictionary key but found non-name object; inserting key /QPDFFake4 +WARNING: issue-fuzz.pdf (trailer, offset 36): expected dictionary key but found non-name object; inserting key /QPDFFake5 +WARNING: issue-fuzz.pdf (trailer, offset 36): expected dictionary key but found non-name object; inserting key /QPDFFake6 +WARNING: issue-fuzz.pdf (trailer, offset 36): expected dictionary key but found non-name object; inserting key /QPDFFake7 +WARNING: issue-fuzz.pdf: file is damaged +WARNING: issue-fuzz.pdf (trailer, offset 32): /Size key in trailer dictionary is impossibly large +WARNING: issue-fuzz.pdf: Attempting to reconstruct cross-reference table +qpdf: issue-fuzz.pdf: unable to find /Root dictionary diff --git a/qpdf/qtest/qpdf/issue-fuzz.pdf b/qpdf/qtest/qpdf/issue-fuzz.pdf new file mode 100644 index 0000000000000000000000000000000000000000..288a6b5c2898c8c9c44be73b6e0b3bdee5e2cb1b GIT binary patch literal 81 zcmZ={XDBX7EGnreN=@T6u;4M|;W1M%R7m40DN4-DNiE{yva!+C56-Mg Date: Fri, 20 Sep 2024 14:56:57 +0100 Subject: [PATCH 3/3] Add additional fuzz test cases --- fuzz/CMakeLists.txt | 3 ++ fuzz/qpdf_extra/99999a.fuzz | 63 ++++++++++++++++++++++++++++++++++++ fuzz/qpdf_extra/99999b.fuzz | Bin 0 -> 81 bytes fuzz/qpdf_extra/99999c.fuzz | Bin 0 -> 13650 bytes fuzz/qtest/fuzz.test | 2 +- 5 files changed, 67 insertions(+), 1 deletion(-) create mode 100644 fuzz/qpdf_extra/99999a.fuzz create mode 100644 fuzz/qpdf_extra/99999b.fuzz create mode 100644 fuzz/qpdf_extra/99999c.fuzz diff --git a/fuzz/CMakeLists.txt b/fuzz/CMakeLists.txt index 73d886c0..adb68cd4 100644 --- a/fuzz/CMakeLists.txt +++ b/fuzz/CMakeLists.txt @@ -142,6 +142,9 @@ set(CORPUS_OTHER 70306b.fuzz 71624.fuzz 71689.fuzz + 99999a.fuzz + 99999b.fuzz + 99999c.fuzz ) set(CORPUS_DIR ${CMAKE_CURRENT_BINARY_DIR}/qpdf_corpus) diff --git a/fuzz/qpdf_extra/99999a.fuzz b/fuzz/qpdf_extra/99999a.fuzz new file mode 100644 index 00000000..026c7427 --- /dev/null +++ b/fuzz/qpdf_extra/99999a.fuzz @@ -0,0 +1,63 @@ +%PDF-1.5 +%€€€€ +1 0 obj +<< + /Type /Catalog + /Pages 2 0 R +>> +endobj +2 0 obj +<< + /Count 6 Ri + 0K/ds [3 0 R] + /Type /Pages +>> +endobj +3 0 obj +<< + /Resources << + /Font << + /F1 5 0 R + >> + >> + /MediaBox [0 0 795 842] + /Parent 2 0 R + /Contents 4 0 R + /Type /Page +=> +endobj +4 0 obj +<<444444444444444444444444 1 Tr /F1 30 Tf 350 750 Td (foobar) Tj ET +endstream +endobj +5 0 obj +<< + /Name /F1 + /BaseFont /Helvetica + /Type /Font + /Subtype /Type1 +>> +e„dobj +6 0 obj +<< /Length 6 0 R >> +stre444444444444444444444444444444<<>> +endobj +xref +0 8 +0000000000 65535 f +0000000015 00000 n +0000000066 00000 n +0000000130 00000 n +0000000269 00000 n +0000000362 00000 n +000000ÎËËÉßÏÏÏ00 n +0000000500 00000 n +trailer +<< + /Size 713115528178535 + /Root 1 0 R + /Info 7 0 R +>> +startxref +520 +%%EOF \ No newline at end of file diff --git a/fuzz/qpdf_extra/99999b.fuzz b/fuzz/qpdf_extra/99999b.fuzz new file mode 100644 index 0000000000000000000000000000000000000000..288a6b5c2898c8c9c44be73b6e0b3bdee5e2cb1b GIT binary patch literal 81 zcmZ={XDBX7EGnreN=@T6u;4M|;W1M%R7m40DN4-DNiE{yva!+C56-Mg`qwNJR~7cfk4tZ z=l+yRlDqBh3_TE5V5{oZt$WWs=iGC?d(KfLn^}m_xDk$g`p+NzT{uJ&22qJuItr&! zB&@8|o37KYKXEIB!tFYt2@}3=lRZMIktCXFk|Opanq`;=wCwZ!mvnrRKuqS<=?yD*!sYTDo&x~2`itLxSfeR|@I z@ho+CJX;@rH<=jXfu1~rr>Ft_e3|*>kxtiXcbTN@OPA=SOByOvlZ=eK@uQGEG7{km zmk34v@<0Fhe?pNBrhjTE1R_Jd3#C#aLX??u+gtG34Z?1rA(JCI^vJ*sFzrV!mTTAzXe>I&*E;L25Ei*Co-+gr?G@N>!a|2}3txYGPbf$Jz2&Pp?f< zRkvDI)r6(0mZqv^621-iPT)ObOt$S+PRp*})hP3BCUj@7Kvx5XFir6XLzp`JK|6F& zb!gXI_@?O~;V7ye;R{|9mMg`tz?|j0+Z9YzK{Xizm4f@I%Xk;y0E)Rc&`cHtCbdOb zbt`pe)_WueVxu7U2{Eb0zO5vF3u&}nAVVsVl5;!WX&W^~Dmx=n+@Q8oV#v9K9+3}x z;ti>%T*0^qQQ|>1L?Q&6K-3{LDuI>E0ZV|K4zZ+)E8lwk%lVgcmeY_Q0(-PjWx53> zp^ss{Tsb2$TU_gyc76M^_$&|uWT-3@V2d2Gt18ixx<<_qxd!cEhuFiyBiM3;?KT3^ zErWsq%T;ip6uhO9KP`3HKHIKWYM_1Z6i5_aFKqHJcr>caG@Gz7l`U-QDPd0sfdcQe z*?smIet**qZ zmT=FXso_Xuetm(%7r-GnHCP0WlyG;gI3@QOcQH}cP8-NbmEB}PiU5B!c9SPc8*~87 z5U_w)!4Ap(4kmU8QyT07eh7z$K{9L*4f}vM=xbyzQa+lPJFIb1{1;&$BxztcYO0AO zd!Wl9IkA8UkQi-%MBp|w7uhrEWfIIDA@u8$H;4#WwCYvB!EAQi_0#S#fXfc_^Dk+F zIU;CUb-PXlybvsnnaMu*NG)O1k@LU`Y9v6p2u*4P5UK{C1n|Le7hH+5a10(Q-tx4pyo^cBV(({?+IYpKd&T|Bdfj z?|*Y->$kpOzJ2}R`@iw!4=#P<-MQG?Bc+*W_26yy$G`XQfArm7`3}? z_X_{^58r?K$;if6|LW<%ym;e*=y@4mBB4ZS(H^X{8}|Jy77y>t}*{qKDH zqd%nOFa5zt=%X+Gm-mZ*{U={bkG%1vktB^gL!c~36Ofq`joL;HK-aPr(0D)xeN#?N zhH4lirDuYUDtaRVo`_-gti#+8_2m`xL%=$8mB?W5KGb6F2gU5$Y0CWt7k$s^z?_A{ z(?*Hd%*(>2|j zf&uFv5<8zgB+^^&9s`V7>ijEUBoa%L!~{W9MG#T-{#`>27A%u`%4b89vj&g>sFGv| zDoxHeI3O`9O}RBPE)Wf8D6ZUROokHbV+sy@-U^ypA8cwqUdpORxN-3~)q23J=B8AAg=d(w0k8f0;d#W_{ zob53@W(t~h&RHCPh*W%Z4yh8JBA%r12RPCMcxnO0L~L2LRNc{4y#yGyRDnMO{@@w3 zbLhVOfS`JrIM6vfilTaT>g7YP4bgzOCZx)Rnj@$w)Btd; z63bEpLiVL9ge-}adw?q&P&PQmkRb8R3la$%8cAeZK|OCf`xFW;ORh=^nUxhAqTePA zkth%v#{_-0D`;puU?ZbYZViodaSPCGAM2#~5Ui8>FU2p-kPyFA7O_2o@)r;cxXk7l zqPq{vT!e0JWRUI`T1sDI1#9JwmMh>OS?+MI3c~{qk=y6?E<`TZR_%O(C69&hA*kgW z!*gW+e82TP*^4b?pUXwbHlYVn(1J#RiO-P45aaO8^S01ZC#au%57Fd>X%v!{x+!Vo ze7y*b0aphG>qS708+|lnJs{T({Un$D_@Q1iF2h60ZTI*+w_7heAkz6J z;L19LbCm6RbEes;`wajGoCe@rfKG6Nk;gsMQ*7Y+%Es&S@fcxa!U1{uXo#UKBxDcX zga8E~T|^q;qTeGZQ6B+H6XqY__zCni;=iP{T`!6M0-orF(&ujEWo7@PpzJS)zS1Xq zCVJ^)0z$yu=j#Hpn4S4vOq~yyUrIHNpl<%m3N4*MAC8RFz{yDJ07E~06takp{`qSG zdC^1yR<%fq&6d^(38>8EAiZwlhc`aO1rE6 zdU{ywkSY>8%v{L?K6!n7OTSTT#OCMS?oMQK)*Cx*Yn{&ZwS#zUH2z@m z{)xGm$&808%ihYUz3g0>-?}-yHM)Of?=F;D=KOprS#D0PnMbKrd*jyL73*^4WSpke zW_2=pb#i0VENy1Sua&3wL&;cWKQl8`-c>i!wWZauvB`2pTc5t_OzzxVYupScwXN9ga1N`I*vqV zGfU0`OSyfuu{HXjk=gW&ok@GLxRuUEi%ZnWm7~k_-l>}o)fSZKjYVhKWAcS`tq2wJ z%}HbqUbl5fhp8k9DX(c|o9t1DM^KjkYOX-mi@43c9P29_bwBkqQh z44@?O`3DL3*E`(ZE<0Pcr)%jsNRpNDV}XAR8Xwl1zJuHjGEJtSdx` zFg%DKd_m+ySVWemK7eTulVJG~bL^z)u54%88&@V1`}yep*vYuDannv5#O%p@bZYnV zc7Aq!>-xg#HRI;3+2YMys#eObO7@*vC-`_H$L;%2*&Ubvec}& zk03)0p||(yWjD7tOEk!Zuwnpo>b;W*i!29VDI~0TQHV%0VKE6Vf*<3!0J&)ss8Pg6 zdEy!Z{H!<2UxG$(g$rxkT!;W5Rx`keIv<8-O^FC-8hkcpnsP)Onq>k>#FIrRNSyjj zK8%Qdv$R7Sj(ZCb<~lbEsbnBS2pS`@kSzU;xonp5#?&|DsWCw;LXV~uw^YhnsGf-H zMiQ>scJa@TJY5LQknD(-y++BaKxmz^Tqr+NvaBI?5R>vKCeCO=Hk~Eh`?KSIS{Mp{ zfN>xzS$~1~M_FLOc*;nF!W&>OzhqmIUowEgE*Xp26=NN{Vgh1UTrB)D(8}b>TDjc$ zWp7YQKf^TV3x@jgvuFPDIGW~^^wv+4y=Bp)Tmlo5JbHif!;k)n^GI22_a*#$^%p%W zCl@J7GI)ZaL$G#0n6kl4J{zScC?4xW#SzI{PBPmZaLGvP9ZJ|kUYPLQ@`N*p+3cpb@^~*^N%P1!6Y_;ps=b*>y5? z!YxbR$O6@fO)`6S$p_V~xDKNbQhn9h1Nyx34LhWv%Jfzz)sFD2CKCTr;-1MCr3}^x z*0izh%^97UAyG9B5q%xj9oOIs_IzBUP(%XL2)t$($RU10lWvW9t#kgNlk0rr>(bqU z z3HUE;s<2U^h+ljV5;~nqv6U;x8G)-I$*pV)c*WsBGMyIkc*MBOlH8#;MwkzRG!x{W z`bk2b_Ef5$q>HMA$#}S6$U>c0`M!W#6*l1K#XT?ZV`n@XD(?CG&;tgBz{)@xl6}Eh z67OsQ=dhaCcQ;rbg6BYas$`fA6qTpfpq8XpfCzPeRHYOURzjBrh!}JnEL4>JDAaBF z{rcwM2IA)^hr_&!pd9bC=g8-?RtwIU9skA^%sMwxhc5*^hQ%bO# zd~L3+qe)g(Uq&N=-#<{>P+{}o$Nx0=YfOiSv*IujOMdkoj g>)MbeXu2M38!jVG7#FV%h7y36s11giR8bxLzi)vX#Q*>R literal 0 HcmV?d00001 diff --git a/fuzz/qtest/fuzz.test b/fuzz/qtest/fuzz.test index 02dbc98a..51a35532 100644 --- a/fuzz/qtest/fuzz.test +++ b/fuzz/qtest/fuzz.test @@ -11,7 +11,7 @@ my $td = new TestDriver('fuzz'); my $qpdf_corpus = $ENV{'QPDF_FUZZ_CORPUS'} || die "must set QPDF_FUZZ_CORPUS"; -my $n_qpdf_files = 79; # increment when adding new files +my $n_qpdf_files = 82; # increment when adding new files my @fuzzers = ( ['ascii85' => 1],