From a11549a566ceed28bc9f6ba100b0d3f6ae59a1f4 Mon Sep 17 00:00:00 2001 From: Jay Berkenbilt Date: Sat, 21 Feb 2015 18:22:32 -0500 Subject: [PATCH] Detect loops in /Pages structure Pushing inherited objects to pages and getting all pages were both prone to stack overflow infinite loops if there were loops in the Pages dictionary. There is a general weakness in the code in that any part of the code that traverses the Pages structure would be prone to this and would have to implement its own loop detection. A more robust fix may provide some general method for handling the Pages structure, but it's probably not worth doing. Note: addition of *Internal2 private functions was done rather than changing signatures of existing methods to avoid breaking compatibility. --- ChangeLog | 4 ++ TODO | 3 + include/qpdf/QPDF.hh | 9 +++ libqpdf/QPDF_optimization.cc | 29 +++++++++- libqpdf/QPDF_pages.cc | 21 ++++++- qpdf/qtest/qpdf.test | 6 +- qpdf/qtest/qpdf/pages-loop.out | 5 ++ qpdf/qtest/qpdf/pages-loop.pdf | 102 +++++++++++++++++++++++++++++++++ 8 files changed, 175 insertions(+), 4 deletions(-) create mode 100644 qpdf/qtest/qpdf/pages-loop.out create mode 100644 qpdf/qtest/qpdf/pages-loop.pdf diff --git a/ChangeLog b/ChangeLog index 88def950..798addd4 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,9 @@ 2015-02-21 Jay Berkenbilt + * Detect loops in Pages structure. Thanks to Gynvael Coldwind and + Mateusz Jurczyk of the Google Security Team for providing a sample + file with this problem. + * Prevent buffer overrun when converting a password to an encryption key. Thanks to Gynvael Coldwind and Mateusz Jurczyk of the Google Security Team for providing a sample file with this diff --git a/TODO b/TODO index 777d7038..41d2c625 100644 --- a/TODO +++ b/TODO @@ -1,6 +1,9 @@ 5.2.0 ===== + * Before release: remember to bump minor shared library version since + new methods were added (even though private). + * Figure out what about a3576a73593987b26cd3eff346f8f7c11f713cbd broke binary compatibility. diff --git a/include/qpdf/QPDF.hh b/include/qpdf/QPDF.hh index dfce40c1..71ff7e27 100644 --- a/include/qpdf/QPDF.hh +++ b/include/qpdf/QPDF.hh @@ -668,6 +668,9 @@ class QPDF void getAllPagesInternal(QPDFObjectHandle cur_pages, std::vector& result); + void getAllPagesInternal2(QPDFObjectHandle cur_pages, + std::vector& result, + std::set& visited); void insertPage(QPDFObjectHandle newpage, int pos); int findPage(QPDFObjGen const& og); int findPage(QPDFObjectHandle& page); @@ -1023,6 +1026,12 @@ class QPDF std::map >&, std::vector& all_pages, bool allow_changes, bool warn_skipped_keys); + void pushInheritedAttributesToPageInternal2( + QPDFObjectHandle, + std::map >&, + std::vector& all_pages, + bool allow_changes, bool warn_skipped_keys, + std::set& visited); void updateObjectMaps(ObjUser const& ou, QPDFObjectHandle oh); void updateObjectMapsInternal(ObjUser const& ou, QPDFObjectHandle oh, std::set& visited, bool top); diff --git a/libqpdf/QPDF_optimization.cc b/libqpdf/QPDF_optimization.cc index 5299d90f..fad710d0 100644 --- a/libqpdf/QPDF_optimization.cc +++ b/libqpdf/QPDF_optimization.cc @@ -174,6 +174,30 @@ QPDF::pushInheritedAttributesToPageInternal( std::vector& pages, bool allow_changes, bool warn_skipped_keys) { + std::set visited; + pushInheritedAttributesToPageInternal2( + cur_pages, key_ancestors, pages, allow_changes, + warn_skipped_keys, visited); +} + +void +QPDF::pushInheritedAttributesToPageInternal2( + QPDFObjectHandle cur_pages, + std::map >& key_ancestors, + std::vector& pages, + bool allow_changes, bool warn_skipped_keys, + std::set& visited) +{ + QPDFObjGen this_og = cur_pages.getObjGen(); + if (visited.count(this_og) > 0) + { + throw QPDFExc( + qpdf_e_pages, this->file->getName(), + this->last_object_description, 0, + "Loop detected in /Pages structure (inherited attributes)"); + } + visited.insert(this_og); + // Extract the underlying dictionary object std::string type = cur_pages.getKey("/Type").getName(); @@ -259,9 +283,9 @@ QPDF::pushInheritedAttributesToPageInternal( int n = kids.getArrayNItems(); for (int i = 0; i < n; ++i) { - pushInheritedAttributesToPageInternal( + pushInheritedAttributesToPageInternal2( kids.getArrayItem(i), key_ancestors, pages, - allow_changes, warn_skipped_keys); + allow_changes, warn_skipped_keys, visited); } // For each inheritable key, pop the stack. If the stack @@ -318,6 +342,7 @@ QPDF::pushInheritedAttributesToPageInternal( this->file->getLastOffset(), "invalid Type " + type + " in page tree"); } + visited.erase(this_og); } void diff --git a/libqpdf/QPDF_pages.cc b/libqpdf/QPDF_pages.cc index 44db064c..f9b421c6 100644 --- a/libqpdf/QPDF_pages.cc +++ b/libqpdf/QPDF_pages.cc @@ -56,6 +56,24 @@ void QPDF::getAllPagesInternal(QPDFObjectHandle cur_pages, std::vector& result) { + std::set visited; + getAllPagesInternal2(cur_pages, result, visited); +} + +void +QPDF::getAllPagesInternal2(QPDFObjectHandle cur_pages, + std::vector& result, + std::set& visited) +{ + QPDFObjGen this_og = cur_pages.getObjGen(); + if (visited.count(this_og) > 0) + { + throw QPDFExc( + qpdf_e_pages, this->file->getName(), + this->last_object_description, 0, + "Loop detected in /Pages structure (getAllPages)"); + } + visited.insert(this_og); std::string type; QPDFObjectHandle type_key = cur_pages.getKey("/Type"); if (type_key.isName()) @@ -76,7 +94,7 @@ QPDF::getAllPagesInternal(QPDFObjectHandle cur_pages, int n = kids.getArrayNItems(); for (int i = 0; i < n; ++i) { - getAllPagesInternal(kids.getArrayItem(i), result); + getAllPagesInternal2(kids.getArrayItem(i), result, visited); } } else if (type == "/Page") @@ -90,6 +108,7 @@ QPDF::getAllPagesInternal(QPDFObjectHandle cur_pages, this->file->getLastOffset(), "invalid Type " + type + " in page tree"); } + visited.erase(this_og); } void diff --git a/qpdf/qtest/qpdf.test b/qpdf/qtest/qpdf.test index 75726ca4..bd509264 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 += 75; +$n_tests += 76; $td->runtest("qpdf version", {$td->COMMAND => "qpdf --version"}, @@ -566,6 +566,10 @@ $td->runtest("ensure arguments to R are direct", {$td->COMMAND => "qpdf --check indirect-r-arg.pdf"}, {$td->FILE => "indirect-r-arg.out", $td->EXIT_STATUS => 2}, $td->NORMALIZE_NEWLINES); +$td->runtest("detect loops in pages structure", + {$td->COMMAND => "qpdf --check pages-loop.pdf"}, + {$td->FILE => "pages-loop.out", $td->EXIT_STATUS => 2}, + $td->NORMALIZE_NEWLINES); show_ntests(); # ---------- diff --git a/qpdf/qtest/qpdf/pages-loop.out b/qpdf/qtest/qpdf/pages-loop.out new file mode 100644 index 00000000..08643aa4 --- /dev/null +++ b/qpdf/qtest/qpdf/pages-loop.out @@ -0,0 +1,5 @@ +checking pages-loop.pdf +PDF Version: 1.3 +File is not encrypted +File is not linearized +pages-loop.pdf (object 3 0): Loop detected in /Pages structure (getAllPages) diff --git a/qpdf/qtest/qpdf/pages-loop.pdf b/qpdf/qtest/qpdf/pages-loop.pdf new file mode 100644 index 00000000..a41b7502 --- /dev/null +++ b/qpdf/qtest/qpdf/pages-loop.pdf @@ -0,0 +1,102 @@ +%PDF-1.3 +%¿÷¢þ +%QDF-1.0 + +%% Original object ID: 1 0 +1 0 obj +<< + /Pages 2 0 R + /Type /Catalog +>> +endobj + +%% Original object ID: 2 0 +2 0 obj +<< + /Count 1 + /Kids [ + 3 0 R + 2 0 R + ] + /Type /Pages +>> +endobj + +%% Page 1 +%% Original object ID: 3 0 +3 0 obj +<< + /Contents 4 0 R + /MediaBox [ + 0 + 0 + 612 + 792 + ] + /Parent 2 0 R + /Resources << + /Font << + /F1 6 0 R + >> + /ProcSet 7 0 R + >> + /Type /Page +>> +endobj + +%% Contents for page 1 +%% Original object ID: 4 0 +4 0 obj +<< + /Length 5 0 R +>> +stream +BT + /F1 24 Tf + 72 720 Td + (Potato) Tj +ET +endstream +endobj + +5 0 obj +44 +endobj + +%% Original object ID: 6 0 +6 0 obj +<< + /BaseFont /Helvetica + /Encoding /WinAnsiEncoding + /Name /F1 + /Subtype /Type1 + /Type /Font +>> +endobj + +%% Original object ID: 5 0 +7 0 obj +[ + /PDF + /Text +] +endobj + +xref +0 8 +0000000000 65535 f +0000000052 00000 n +0000000133 00000 n +0000000252 00000 n +0000000494 00000 n +0000000593 00000 n +0000000639 00000 n +0000000784 00000 n +trailer << + /Root 1 0 R + /Size 8 + /ID [<395875d4235973eebbade9c7e9e7f857><395875d4235973eebbade9c7e9e7f857>] +>> +startxref +819 +%%EOF