Fix crashes in FPDFAttachment_GetStringValue()
When the key argument is "CheckSum", FPDFAttachment_GetStringValue() has
some special code to handle that case. This code path assumed the value
of the checksum will always be a string, per spec. In the wild, the
value could have some other type. Add some tests that would confuse the
existing implementation and cause a nullptr crash. Then fix the
implementation to not crash.
Change-Id: I4cc7d6a568634512fa16ffa1bea15d9713cfa00f
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/119410
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Thomas Sepez <tsepez@google.com>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/fpdfsdk/fpdf_attachment.cpp b/fpdfsdk/fpdf_attachment.cpp
index ccc2154..8c86b84 100644
--- a/fpdfsdk/fpdf_attachment.cpp
+++ b/fpdfsdk/fpdf_attachment.cpp
@@ -173,30 +173,40 @@
FPDF_BYTESTRING key,
FPDF_WCHAR* buffer,
unsigned long buflen) {
- CPDF_Object* pFile = CPDFObjectFromFPDFAttachment(attachment);
- if (!pFile)
+ CPDF_Object* file = CPDFObjectFromFPDFAttachment(attachment);
+ if (!file) {
return 0;
+ }
- CPDF_FileSpec spec(pdfium::WrapRetain(pFile));
- RetainPtr<const CPDF_Dictionary> pParamsDict = spec.GetParamsDict();
- if (!pParamsDict)
+ CPDF_FileSpec spec(pdfium::WrapRetain(file));
+ RetainPtr<const CPDF_Dictionary> params = spec.GetParamsDict();
+ if (!params) {
return 0;
+ }
- ByteString bsKey = key;
- WideString value = pParamsDict->GetUnicodeTextFor(bsKey);
- if (bsKey == kChecksumKey && !value.IsEmpty()) {
- const CPDF_String* stringValue =
- pParamsDict->GetObjectFor(bsKey)->AsString();
- if (stringValue->IsHex()) {
+ // SAFETY: required from caller.
+ auto buffer_span = UNSAFE_BUFFERS(SpanFromFPDFApiArgs(buffer, buflen));
+
+ ByteString key_str = key;
+ RetainPtr<const CPDF_Object> object = params->GetObjectFor(key_str);
+ if (!object || (!object->IsString() && !object->IsName())) {
+ // Per API description, return an empty string in these cases.
+ return Utf16EncodeMaybeCopyAndReturnLength(WideString(), buffer_span);
+ }
+
+ if (key_str == kChecksumKey) {
+ RetainPtr<const CPDF_String> string_object = ToString(object);
+ if (string_object && string_object->IsHex()) {
ByteString encoded =
- PDF_HexEncodeString(stringValue->GetString().AsStringView());
- value =
- pdfium::MakeRetain<CPDF_String>(nullptr, encoded)->GetUnicodeText();
+ PDF_HexEncodeString(string_object->GetString().AsStringView());
+ return Utf16EncodeMaybeCopyAndReturnLength(
+ pdfium::MakeRetain<CPDF_String>(nullptr, encoded)->GetUnicodeText(),
+ buffer_span);
}
}
- // SAFETY: required from caller.
- return Utf16EncodeMaybeCopyAndReturnLength(
- value, UNSAFE_BUFFERS(SpanFromFPDFApiArgs(buffer, buflen)));
+
+ return Utf16EncodeMaybeCopyAndReturnLength(object->GetUnicodeText(),
+ buffer_span);
}
FPDF_EXPORT FPDF_BOOL FPDF_CALLCONV
diff --git a/fpdfsdk/fpdf_attachment_embeddertest.cpp b/fpdfsdk/fpdf_attachment_embeddertest.cpp
index 6c156d6..e87461f 100644
--- a/fpdfsdk/fpdf_attachment_embeddertest.cpp
+++ b/fpdfsdk/fpdf_attachment_embeddertest.cpp
@@ -366,3 +366,40 @@
EXPECT_EQ(26u, FPDFAttachment_GetName(attachment, buf.data(), length_bytes));
EXPECT_EQ(L"attached.pdf", GetPlatformWString(buf.data()));
}
+
+TEST_F(FPDFAttachmentEmbedderTest, GetStringValueForChecksumNotString) {
+ ASSERT_TRUE(OpenDocument("embedded_attachments_invalid_types.pdf"));
+ EXPECT_EQ(2, FPDFDoc_GetAttachmentCount(document()));
+
+ FPDF_ATTACHMENT attachment = FPDFDoc_GetAttachment(document(), 0);
+ ASSERT_TRUE(attachment);
+
+ // The checksum key is a name, which violates the spec. This will still return
+ // the value, but should not crash.
+ constexpr unsigned long kExpectedLength = 8u;
+ ASSERT_EQ(kExpectedLength, FPDFAttachment_GetStringValue(
+ attachment, kChecksumKey, nullptr, 0));
+ std::vector<FPDF_WCHAR> buf = GetFPDFWideStringBuffer(kExpectedLength);
+ EXPECT_EQ(kExpectedLength,
+ FPDFAttachment_GetStringValue(attachment, kChecksumKey, buf.data(),
+ kExpectedLength));
+ EXPECT_EQ(L"Bad", GetPlatformWString(buf.data()));
+}
+
+TEST_F(FPDFAttachmentEmbedderTest, GetStringValueForNotString) {
+ ASSERT_TRUE(OpenDocument("embedded_attachments_invalid_types.pdf"));
+ EXPECT_EQ(2, FPDFDoc_GetAttachmentCount(document()));
+
+ FPDF_ATTACHMENT attachment = FPDFDoc_GetAttachment(document(), 1);
+ ASSERT_TRUE(attachment);
+
+ // The checksum key is a stream, while the API requires a string or name.
+ constexpr unsigned long kExpectedLength = 2u;
+ ASSERT_EQ(kExpectedLength, FPDFAttachment_GetStringValue(
+ attachment, kChecksumKey, nullptr, 0));
+ std::vector<FPDF_WCHAR> buf = GetFPDFWideStringBuffer(kExpectedLength);
+ EXPECT_EQ(kExpectedLength,
+ FPDFAttachment_GetStringValue(attachment, kChecksumKey, buf.data(),
+ kExpectedLength));
+ EXPECT_EQ(L"", GetPlatformWString(buf.data()));
+}
diff --git a/testing/resources/embedded_attachments_invalid_types.in b/testing/resources/embedded_attachments_invalid_types.in
new file mode 100644
index 0000000..6893408
--- /dev/null
+++ b/testing/resources/embedded_attachments_invalid_types.in
@@ -0,0 +1,81 @@
+{{header}}
+{{object 1 0}} <<
+ /Type /Catalog
+ /Names 4 0 R
+ /Pages 2 0 R
+>>
+endobj
+{{object 2 0}} <<
+ /Type /Pages
+ /Count 1
+ /Kids [3 0 R]
+>>
+endobj
+{{object 3 0}} <<
+ /Type /Page
+ /Parent 2 0 R
+ /MediaBox [0 0 612 792]
+ /Resources <<
+ /ProcSet [/PDF]
+ >>
+>>
+endobj
+{{object 4 0}} <<
+ /EmbeddedFiles 5 0 R
+>>
+endobj
+{{object 5 0}} <<
+ /Names [(1.txt) 6 0 R (2.txt) 8 0 R]
+>>
+endobj
+{{object 6 0}} <<
+ /Type /Filespec
+ /Desc ()
+ /EF <<
+ /F 7 0 R
+ >>
+ /F (1.txt)
+ /UF (1.txt)
+>>
+endobj
+{{object 7 0}} <<
+ /Params <<
+ /CheckSum /Bad % should be a string, not a name
+ >>
+ {{streamlen}}
+>>
+stream
+12
+endstream
+endobj
+{{object 8 0}} <<
+ /Type /Filespec
+ /Desc ()
+ /EF <<
+ /F 9 0 R
+ >>
+ /F (2.txt)
+ /UF (2.txt)
+>>
+endobj
+{{object 9 0}} <<
+ /Params <<
+ /CheckSum 10 0 R % should be a string, not a stream
+ >>
+ {{streamlen}}
+>>
+stream
+345
+endstream
+endobj
+{{object 10 0}} <<
+ {{streamlen}}
+>>
+stream
+6789
+endstream
+endobj
+{{xref}}
+{{trailer}}
+{{startxref}}
+%%EOF
diff --git a/testing/resources/embedded_attachments_invalid_types.pdf b/testing/resources/embedded_attachments_invalid_types.pdf
new file mode 100644
index 0000000..509c8f4
--- /dev/null
+++ b/testing/resources/embedded_attachments_invalid_types.pdf
@@ -0,0 +1,98 @@
+%PDF-1.7
+% ò¤ô
+1 0 obj <<
+ /Type /Catalog
+ /Names 4 0 R
+ /Pages 2 0 R
+>>
+endobj
+2 0 obj <<
+ /Type /Pages
+ /Count 1
+ /Kids [3 0 R]
+>>
+endobj
+3 0 obj <<
+ /Type /Page
+ /Parent 2 0 R
+ /MediaBox [0 0 612 792]
+ /Resources <<
+ /ProcSet [/PDF]
+ >>
+>>
+endobj
+4 0 obj <<
+ /EmbeddedFiles 5 0 R
+>>
+endobj
+5 0 obj <<
+ /Names [(1.txt) 6 0 R (2.txt) 8 0 R]
+>>
+endobj
+6 0 obj <<
+ /Type /Filespec
+ /Desc ()
+ /EF <<
+ /F 7 0 R
+ >>
+ /F (1.txt)
+ /UF (1.txt)
+>>
+endobj
+7 0 obj <<
+ /Params <<
+ /CheckSum /Bad % should be a string, not a name
+ >>
+ /Length 3
+>>
+stream
+12
+endstream
+endobj
+8 0 obj <<
+ /Type /Filespec
+ /Desc ()
+ /EF <<
+ /F 9 0 R
+ >>
+ /F (2.txt)
+ /UF (2.txt)
+>>
+endobj
+9 0 obj <<
+ /Params <<
+ /CheckSum 10 0 R % should be a string, not a stream
+ >>
+ /Length 4
+>>
+stream
+345
+endstream
+endobj
+10 0 obj <<
+ /Length 5
+>>
+stream
+6789
+endstream
+endobj
+xref
+0 11
+0000000000 65535 f
+0000000015 00000 n
+0000000083 00000 n
+0000000146 00000 n
+0000000264 00000 n
+0000000308 00000 n
+0000000368 00000 n
+0000000472 00000 n
+0000000595 00000 n
+0000000699 00000 n
+0000000827 00000 n
+trailer <<
+ /Root 1 0 R
+ /Size 11
+>>
+startxref
+883
+%%EOF
