commit 9b0ce8ab57a38fe8d4fd76146b36a7876a3e0c8b
author Lei Zhang <thestig@chromium.org> Thu Jul 01 17:21:00 2021 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Thu Jul 01 17:21:00 2021 +0000
tree 2369ba833661d599137ee0717e1f5166b4961ea3
parent 0763803236fdb32477027b29d8fd5c74fa59be6e

Disallow empty keys in dictionaries when parsing with CPDF_SyntaxParser.

- Add a new test file and embedder test for this case.
- Add a CHECK() to CPDF_Dictionary::SetFor() to flush out other cases.

Bug: chromium:1191313
Change-Id: I308a29420a5986b078f75ecdf0bcc34d6f483591
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/82790
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

core/fpdfapi/parser/cpdf_dictionary.cpp

diff --git a/core/fpdfapi/parser/cpdf_dictionary.cpp b/core/fpdfapi/parser/cpdf_dictionary.cpp
index a798b6e..2f77a60 100644
--- a/core/fpdfapi/parser/cpdf_dictionary.cpp
+++ b/core/fpdfapi/parser/cpdf_dictionary.cpp
@@ -210,6 +210,7 @@
 
 CPDF_Object* CPDF_Dictionary::SetFor(const ByteString& key,
                                      RetainPtr<CPDF_Object> pObj) {
+  CHECK(!key.IsEmpty());
   CHECK(!IsLocked());
   if (!pObj) {
     m_Map.erase(key);

core/fpdfapi/parser/cpdf_parser_embeddertest.cpp

diff --git a/core/fpdfapi/parser/cpdf_parser_embeddertest.cpp b/core/fpdfapi/parser/cpdf_parser_embeddertest.cpp
index 814bfa2..873bf50 100644
--- a/core/fpdfapi/parser/cpdf_parser_embeddertest.cpp
+++ b/core/fpdfapi/parser/cpdf_parser_embeddertest.cpp
@@ -80,3 +80,8 @@
   EXPECT_NE(nullptr, page);
   UnloadPage(page);
 }
+
+// crbug.com/1191313
+TEST_F(CPDFParserEmbedderTest, InvalidDictionaryKeys) {
+  ASSERT_TRUE(OpenDocument("bad_dict_keys.pdf"));
+}

core/fpdfapi/parser/cpdf_syntax_parser.cpp

diff --git a/core/fpdfapi/parser/cpdf_syntax_parser.cpp b/core/fpdfapi/parser/cpdf_syntax_parser.cpp
index 9538513..47f4f1a 100644
--- a/core/fpdfapi/parser/cpdf_syntax_parser.cpp
+++ b/core/fpdfapi/parser/cpdf_syntax_parser.cpp
@@ -589,9 +589,10 @@
         return nullptr;
       }
 
-      if (!key.IsEmpty()) {
-        ByteString keyNoSlash(key.raw_str() + 1, key.GetLength() - 1);
-        pDict->SetFor(keyNoSlash, std::move(pObj));
+      // `key` has to be "/X" at the minimum.
+      if (key.GetLength() > 1) {
+        ByteString key_no_slash(key.raw_str() + 1, key.GetLength() - 1);
+        pDict->SetFor(key_no_slash, std::move(pObj));
       }
     }
 

testing/resources/bad_dict_keys.in

diff --git a/testing/resources/bad_dict_keys.in b/testing/resources/bad_dict_keys.in
new file mode 100644
index 0000000..3f53c28
--- /dev/null
+++ b/testing/resources/bad_dict_keys.in
@@ -0,0 +1,23 @@
+{{header}}
+{{object 1 0}} <<
+  /Type /Catalog
+  /Pages 2 0 R
+>>
+endobj
+{{object 2 0}} <<
+  /Type /Pages
+  /MediaBox [0 0 300 300]
+  /Count 1
+  /Kids [3 0 R]
+  / [3 0 R]
+>>
+endobj
+{{object 3 0}} <<
+  /Type /Page
+  /Parent 2 0 R
+>>
+endobj
+{{xref}}
+{{trailer}}
+{{startxref}}
+%%EOF

testing/resources/bad_dict_keys.pdf

diff --git a/testing/resources/bad_dict_keys.pdf b/testing/resources/bad_dict_keys.pdf
new file mode 100644
index 0000000..b55c434
--- /dev/null
+++ b/testing/resources/bad_dict_keys.pdf
@@ -0,0 +1,33 @@
+%PDF-1.7
+% ò¤ô
+1 0 obj <<
+  /Type /Catalog
+  /Pages 2 0 R
+>>
+endobj
+2 0 obj <<
+  /Type /Pages
+  /MediaBox [0 0 300 300]
+  /Count 1
+  /Kids [3 0 R]
+  / [3 0 R]
+>>
+endobj
+3 0 obj <<
+  /Type /Page
+  /Parent 2 0 R
+>>
+endobj
+xref
+0 4
+0000000000 65535 f 
+0000000015 00000 n 
+0000000068 00000 n 
+0000000169 00000 n 
+trailer <<
+  /Root 1 0 R
+  /Size 4
+>>
+startxref
+220
+%%EOF