OpenSystemsDevelopment / qpdf

Commit a11549a566ceed28bc9f6ba100b0d3f6ae59a1f4

Authored by Jay Berkenbilt
1 parent 28a9df51

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.
Inline Side-by-side
Showing 8 changed files with 175 additions and 4 deletions
ChangeLog
  View file @a11549a
1 1 2015-02-21 Jay Berkenbilt <ejb@ql.org>
2 2  
  3 + * Detect loops in Pages structure. Thanks to Gynvael Coldwind and
  4 + Mateusz Jurczyk of the Google Security Team for providing a sample
  5 + file with this problem.
  6 +
3 7 * Prevent buffer overrun when converting a password to an
4 8 encryption key. Thanks to Gynvael Coldwind and Mateusz Jurczyk of
5 9 the Google Security Team for providing a sample file with this
... ...
1 1 5.2.0
2 2 =====
3 3  
  4 + * Before release: remember to bump minor shared library version since
  5 + new methods were added (even though private).
  6 +
4 7 * Figure out what about a3576a73593987b26cd3eff346f8f7c11f713cbd
5 8 broke binary compatibility.
6 9  
... ...
include/qpdf/QPDF.hh
  View file @a11549a
... ... @@ -668,6 +668,9 @@ class QPDF
668 668  
669 669 void getAllPagesInternal(QPDFObjectHandle cur_pages,
670 670 std::vector<QPDFObjectHandle>& result);
  671 + void getAllPagesInternal2(QPDFObjectHandle cur_pages,
  672 + std::vector<QPDFObjectHandle>& result,
  673 + std::set<QPDFObjGen>& visited);
671 674 void insertPage(QPDFObjectHandle newpage, int pos);
672 675 int findPage(QPDFObjGen const& og);
673 676 int findPage(QPDFObjectHandle& page);
... ... @@ -1023,6 +1026,12 @@ class QPDF
1023 1026 std::map<std::string, std::vector<QPDFObjectHandle> >&,
1024 1027 std::vector<QPDFObjectHandle>& all_pages,
1025 1028 bool allow_changes, bool warn_skipped_keys);
  1029 + void pushInheritedAttributesToPageInternal2(
  1030 + QPDFObjectHandle,
  1031 + std::map<std::string, std::vector<QPDFObjectHandle> >&,
  1032 + std::vector<QPDFObjectHandle>& all_pages,
  1033 + bool allow_changes, bool warn_skipped_keys,
  1034 + std::set<QPDFObjGen>& visited);
1026 1035 void updateObjectMaps(ObjUser const& ou, QPDFObjectHandle oh);
1027 1036 void updateObjectMapsInternal(ObjUser const& ou, QPDFObjectHandle oh,
1028 1037 std::set<QPDFObjGen>& visited, bool top);
... ...
libqpdf/QPDF_optimization.cc
  View file @a11549a
... ... @@ -174,6 +174,30 @@ QPDF::pushInheritedAttributesToPageInternal(
174 174 std::vector<QPDFObjectHandle>& pages,
175 175 bool allow_changes, bool warn_skipped_keys)
176 176 {
  177 + std::set<QPDFObjGen> visited;
  178 + pushInheritedAttributesToPageInternal2(
  179 + cur_pages, key_ancestors, pages, allow_changes,
  180 + warn_skipped_keys, visited);
  181 +}
  182 +
  183 +void
  184 +QPDF::pushInheritedAttributesToPageInternal2(
  185 + QPDFObjectHandle cur_pages,
  186 + std::map<std::string, std::vector<QPDFObjectHandle> >& key_ancestors,
  187 + std::vector<QPDFObjectHandle>& pages,
  188 + bool allow_changes, bool warn_skipped_keys,
  189 + std::set<QPDFObjGen>& visited)
  190 +{
  191 + QPDFObjGen this_og = cur_pages.getObjGen();
  192 + if (visited.count(this_og) > 0)
  193 + {
  194 + throw QPDFExc(
  195 + qpdf_e_pages, this->file->getName(),
  196 + this->last_object_description, 0,
  197 + "Loop detected in /Pages structure (inherited attributes)");
  198 + }
  199 + visited.insert(this_og);
  200 +
177 201 // Extract the underlying dictionary object
178 202 std::string type = cur_pages.getKey("/Type").getName();
179 203  
... ... @@ -259,9 +283,9 @@ QPDF::pushInheritedAttributesToPageInternal(
259 283 int n = kids.getArrayNItems();
260 284 for (int i = 0; i < n; ++i)
261 285 {
262   - pushInheritedAttributesToPageInternal(
  286 + pushInheritedAttributesToPageInternal2(
263 287 kids.getArrayItem(i), key_ancestors, pages,
264   - allow_changes, warn_skipped_keys);
  288 + allow_changes, warn_skipped_keys, visited);
265 289 }
266 290  
267 291 // For each inheritable key, pop the stack. If the stack
... ... @@ -318,6 +342,7 @@ QPDF::pushInheritedAttributesToPageInternal(
318 342 this->file->getLastOffset(),
319 343 "invalid Type " + type + " in page tree");
320 344 }
  345 + visited.erase(this_og);
321 346 }
322 347  
323 348 void
... ...
libqpdf/QPDF_pages.cc
  View file @a11549a
... ... @@ -56,6 +56,24 @@ void
56 56 QPDF::getAllPagesInternal(QPDFObjectHandle cur_pages,
57 57 std::vector<QPDFObjectHandle>& result)
58 58 {
  59 + std::set<QPDFObjGen> visited;
  60 + getAllPagesInternal2(cur_pages, result, visited);
  61 +}
  62 +
  63 +void
  64 +QPDF::getAllPagesInternal2(QPDFObjectHandle cur_pages,
  65 + std::vector<QPDFObjectHandle>& result,
  66 + std::set<QPDFObjGen>& visited)
  67 +{
  68 + QPDFObjGen this_og = cur_pages.getObjGen();
  69 + if (visited.count(this_og) > 0)
  70 + {
  71 + throw QPDFExc(
  72 + qpdf_e_pages, this->file->getName(),
  73 + this->last_object_description, 0,
  74 + "Loop detected in /Pages structure (getAllPages)");
  75 + }
  76 + visited.insert(this_og);
59 77 std::string type;
60 78 QPDFObjectHandle type_key = cur_pages.getKey("/Type");
61 79 if (type_key.isName())
... ... @@ -76,7 +94,7 @@ QPDF::getAllPagesInternal(QPDFObjectHandle cur_pages,
76 94 int n = kids.getArrayNItems();
77 95 for (int i = 0; i < n; ++i)
78 96 {
79   - getAllPagesInternal(kids.getArrayItem(i), result);
  97 + getAllPagesInternal2(kids.getArrayItem(i), result, visited);
80 98 }
81 99 }
82 100 else if (type == "/Page")
... ... @@ -90,6 +108,7 @@ QPDF::getAllPagesInternal(QPDFObjectHandle cur_pages,
90 108 this->file->getLastOffset(),
91 109 "invalid Type " + type + " in page tree");
92 110 }
  111 + visited.erase(this_og);
93 112 }
94 113  
95 114 void
... ...
qpdf/qtest/qpdf.test
  View file @a11549a
... ... @@ -199,7 +199,7 @@ $td-&gt;runtest(&quot;remove page we don&#39;t have&quot;,
199 199 show_ntests();
200 200 # ----------
201 201 $td->notify("--- Miscellaneous Tests ---");
202   -$n_tests += 75;
  202 +$n_tests += 76;
203 203  
204 204 $td->runtest("qpdf version",
205 205 {$td->COMMAND => "qpdf --version"},
... ... @@ -566,6 +566,10 @@ $td-&gt;runtest(&quot;ensure arguments to R are direct&quot;,
566 566 {$td->COMMAND => "qpdf --check indirect-r-arg.pdf"},
567 567 {$td->FILE => "indirect-r-arg.out", $td->EXIT_STATUS => 2},
568 568 $td->NORMALIZE_NEWLINES);
  569 +$td->runtest("detect loops in pages structure",
  570 + {$td->COMMAND => "qpdf --check pages-loop.pdf"},
  571 + {$td->FILE => "pages-loop.out", $td->EXIT_STATUS => 2},
  572 + $td->NORMALIZE_NEWLINES);
569 573  
570 574 show_ntests();
571 575 # ----------
... ...
qpdf/qtest/qpdf/pages-loop.out 0 → 100644
  View file @a11549a
  1 +checking pages-loop.pdf
  2 +PDF Version: 1.3
  3 +File is not encrypted
  4 +File is not linearized
  5 +pages-loop.pdf (object 3 0): Loop detected in /Pages structure (getAllPages)
... ...
qpdf/qtest/qpdf/pages-loop.pdf 0 → 100644
  View file @a11549a
  1 +%PDF-1.3
  2 +%¿÷¢þ
  3 +%QDF-1.0
  4 +
  5 +%% Original object ID: 1 0
  6 +1 0 obj
  7 +<<
  8 + /Pages 2 0 R
  9 + /Type /Catalog
  10 +>>
  11 +endobj
  12 +
  13 +%% Original object ID: 2 0
  14 +2 0 obj
  15 +<<
  16 + /Count 1
  17 + /Kids [
  18 + 3 0 R
  19 + 2 0 R
  20 + ]
  21 + /Type /Pages
  22 +>>
  23 +endobj
  24 +
  25 +%% Page 1
  26 +%% Original object ID: 3 0
  27 +3 0 obj
  28 +<<
  29 + /Contents 4 0 R
  30 + /MediaBox [
  31 + 0
  32 + 0
  33 + 612
  34 + 792
  35 + ]
  36 + /Parent 2 0 R
  37 + /Resources <<
  38 + /Font <<
  39 + /F1 6 0 R
  40 + >>
  41 + /ProcSet 7 0 R
  42 + >>
  43 + /Type /Page
  44 +>>
  45 +endobj
  46 +
  47 +%% Contents for page 1
  48 +%% Original object ID: 4 0
  49 +4 0 obj
  50 +<<
  51 + /Length 5 0 R
  52 +>>
  53 +stream
  54 +BT
  55 + /F1 24 Tf
  56 + 72 720 Td
  57 + (Potato) Tj
  58 +ET
  59 +endstream
  60 +endobj
  61 +
  62 +5 0 obj
  63 +44
  64 +endobj
  65 +
  66 +%% Original object ID: 6 0
  67 +6 0 obj
  68 +<<
  69 + /BaseFont /Helvetica
  70 + /Encoding /WinAnsiEncoding
  71 + /Name /F1
  72 + /Subtype /Type1
  73 + /Type /Font
  74 +>>
  75 +endobj
  76 +
  77 +%% Original object ID: 5 0
  78 +7 0 obj
  79 +[
  80 + /PDF
  81 + /Text
  82 +]
  83 +endobj
  84 +
  85 +xref
  86 +0 8
  87 +0000000000 65535 f
  88 +0000000052 00000 n
  89 +0000000133 00000 n
  90 +0000000252 00000 n
  91 +0000000494 00000 n
  92 +0000000593 00000 n
  93 +0000000639 00000 n
  94 +0000000784 00000 n
  95 +trailer <<
  96 + /Root 1 0 R
  97 + /Size 8
  98 + /ID [<395875d4235973eebbade9c7e9e7f857><395875d4235973eebbade9c7e9e7f857>]
  99 +>>
  100 +startxref
  101 +819
  102 +%%EOF
... ...