From 266cb438df69bae261666a2927a52cb7873f6a11 Mon Sep 17 00:00:00 2001 From: Miklos Vajna Date: Fri, 24 Jul 2020 11:29:27 +0200 Subject: xmlsecurity: detect unsigned incremental update between signatures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (cherry picked from commit 7468d5df5ec79783eae84b62bdc5ecf12f0ca255) Conflicts: vcl/source/filter/ipdf/pdfdocument.cxx xmlsecurity/source/pdfio/pdfdocument.cxx Change-Id: I269ed858852ee7d1275adf340c8cc1565fc30693 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/99382 Tested-by: Jenkins Reviewed-by: Caolán McNamara (cherry picked from commit ed21fa8d6edd97fad3a5b606b3522ab3bff14e05) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/99745 Reviewed-by: Andras Timar Reviewed-by: Thorsten Behrens Tested-by: Thorsten Behrens --- include/vcl/filter/pdfdocument.hxx | 2 + vcl/source/filter/ipdf/pdfdocument.cxx | 13 +- xmlsecurity/inc/pdfio/pdfdocument.hxx | 6 +- .../qa/unit/pdfsigning/data/partial-in-between.pdf | Bin 0 -> 104501 bytes xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx | 17 ++- xmlsecurity/source/helper/pdfsignaturehelper.cxx | 3 +- xmlsecurity/source/pdfio/pdfdocument.cxx | 140 +++++++++++++++++---- xmlsecurity/workben/pdfverify.cxx | 6 +- 8 files changed, 153 insertions(+), 34 deletions(-) create mode 100644 xmlsecurity/qa/unit/pdfsigning/data/partial-in-between.pdf diff --git a/include/vcl/filter/pdfdocument.hxx b/include/vcl/filter/pdfdocument.hxx index 65e84880c789..e05e7d9d749c 100644 --- a/include/vcl/filter/pdfdocument.hxx +++ b/include/vcl/filter/pdfdocument.hxx @@ -426,6 +426,8 @@ public: std::vector GetSignatureWidgets(); /// Remove the nth signature from read document in the edit buffer. bool RemoveSignature(size_t nPosition); + /// Get byte offsets of the end of incremental updates. + const std::vector& GetEOFs() const; //@} }; diff --git a/vcl/source/filter/ipdf/pdfdocument.cxx b/vcl/source/filter/ipdf/pdfdocument.cxx index 02bde3fcaedb..1afc3167ae91 100644 --- a/vcl/source/filter/ipdf/pdfdocument.cxx +++ b/vcl/source/filter/ipdf/pdfdocument.cxx @@ -147,6 +147,8 @@ bool PDFDocument::RemoveSignature(size_t nPosition) return m_aEditBuffer.good(); } +const std::vector& PDFDocument::GetEOFs() const { return m_aEOFs; } + sal_uInt32 PDFDocument::GetNextSignature() { sal_uInt32 nRet = 0; @@ -1979,7 +1981,16 @@ bool PDFCommentElement::Read(SvStream& rStream) m_aComment = aBuf.makeStringAndClear(); if (m_aComment.startsWith("%%EOF")) - m_rDoc.PushBackEOF(rStream.Tell()); + { + sal_uInt64 nPos = rStream.Tell(); + if (ch == '\r') + { + // If the comment ends with a \r\n, count the \n as well to match Adobe Acrobat + // behavior. + nPos += 1; + } + m_rDoc.PushBackEOF(nPos); + } SAL_INFO("vcl.filter", "PDFCommentElement::Read: m_aComment is '" << m_aComment << "'"); return true; diff --git a/xmlsecurity/inc/pdfio/pdfdocument.hxx b/xmlsecurity/inc/pdfio/pdfdocument.hxx index 996bb1527bb8..f7e36492e746 100644 --- a/xmlsecurity/inc/pdfio/pdfdocument.hxx +++ b/xmlsecurity/inc/pdfio/pdfdocument.hxx @@ -18,6 +18,7 @@ namespace vcl namespace filter { class PDFObjectElement; +class PDFDocument; } } struct SignatureInformation; @@ -29,12 +30,13 @@ namespace pdfio { /** * @param rInformation The actual result. - * @param bLast If this is the last signature in the file, so it covers the whole file physically. + * @param rDocument the parsed document to see if the signature is partial. * @return If we can determinate a result. */ XMLSECURITY_DLLPUBLIC bool ValidateSignature(SvStream& rStream, vcl::filter::PDFObjectElement* pSignature, - SignatureInformation& rInformation, bool bLast); + SignatureInformation& rInformation, + vcl::filter::PDFDocument& rDocument); } // namespace pdfio } // namespace xmlsecurity diff --git a/xmlsecurity/qa/unit/pdfsigning/data/partial-in-between.pdf b/xmlsecurity/qa/unit/pdfsigning/data/partial-in-between.pdf new file mode 100644 index 000000000000..211a111cb394 Binary files /dev/null and b/xmlsecurity/qa/unit/pdfsigning/data/partial-in-between.pdf differ diff --git a/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx b/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx index 97e0b7d28f97..8a7cbbdc3730 100644 --- a/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx +++ b/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx @@ -96,9 +96,8 @@ std::vector PDFSigningTest::verify(const OUString& rURL, s for (size_t i = 0; i < aSignatures.size(); ++i) { SignatureInformation aInfo(i); - bool bLast = i == aSignatures.size() - 1; CPPUNIT_ASSERT( - xmlsecurity::pdfio::ValidateSignature(aStream, aSignatures[i], aInfo, bLast)); + xmlsecurity::pdfio::ValidateSignature(aStream, aSignatures[i], aInfo, aVerifyDocument)); aRet.push_back(aInfo); if (!rExpectedSubFilter.isEmpty()) @@ -243,7 +242,7 @@ CPPUNIT_TEST_FIXTURE(PDFSigningTest, testPDFRemove) CPPUNIT_ASSERT_EQUAL(static_cast(1), aSignatures.size()); SignatureInformation aInfo(0); CPPUNIT_ASSERT( - xmlsecurity::pdfio::ValidateSignature(aStream, aSignatures[0], aInfo, /*bLast=*/true)); + xmlsecurity::pdfio::ValidateSignature(aStream, aSignatures[0], aInfo, aDocument)); } // Remove the signature and write out the result as remove.pdf. @@ -399,6 +398,18 @@ CPPUNIT_TEST_FIXTURE(PDFSigningTest, testPartial) CPPUNIT_ASSERT(rInformation.bPartialDocumentSignature); } +CPPUNIT_TEST_FIXTURE(PDFSigningTest, testPartialInBetween) +{ + std::vector aInfos + = verify(m_directories.getURLFromSrc(DATA_DIRECTORY) + "partial-in-between.pdf", 2, + /*rExpectedSubFilter=*/OString()); + CPPUNIT_ASSERT(!aInfos.empty()); + SignatureInformation& rInformation = aInfos[0]; + // Without the accompanying fix in place, this test would have failed, as unsigned incremental + // update between two signatures were not detected. + CPPUNIT_ASSERT(rInformation.bPartialDocumentSignature); +} + /// Test writing a PAdES signature. CPPUNIT_TEST_FIXTURE(PDFSigningTest, testSigningCertificateAttribute) { diff --git a/xmlsecurity/source/helper/pdfsignaturehelper.cxx b/xmlsecurity/source/helper/pdfsignaturehelper.cxx index 809e089b0f4f..f10f29c61840 100644 --- a/xmlsecurity/source/helper/pdfsignaturehelper.cxx +++ b/xmlsecurity/source/helper/pdfsignaturehelper.cxx @@ -56,8 +56,7 @@ bool PDFSignatureHelper::ReadAndVerifySignature( { SignatureInformation aInfo(i); - bool bLast = i == aSignatures.size() - 1; - if (!xmlsecurity::pdfio::ValidateSignature(*pStream, aSignatures[i], aInfo, bLast)) + if (!xmlsecurity::pdfio::ValidateSignature(*pStream, aSignatures[i], aInfo, aDocument)) SAL_WARN("xmlsecurity.helper", "failed to determine digest match"); m_aSignatureInfos.push_back(aInfo); diff --git a/xmlsecurity/source/pdfio/pdfdocument.cxx b/xmlsecurity/source/pdfio/pdfdocument.cxx index febcd2e3a1ea..7cf2c137c1c4 100644 --- a/xmlsecurity/source/pdfio/pdfdocument.cxx +++ b/xmlsecurity/source/pdfio/pdfdocument.cxx @@ -23,12 +23,124 @@ using namespace com::sun::star; +namespace +{ +/// Turns an array of floats into offset + length pairs. +bool GetByteRangesFromPDF(vcl::filter::PDFArrayElement& rArray, + std::vector>& rByteRanges) +{ + size_t nByteRangeOffset = 0; + const std::vector& rByteRangeElements = rArray.GetElements(); + for (size_t i = 0; i < rByteRangeElements.size(); ++i) + { + auto pNumber = dynamic_cast(rByteRangeElements[i]); + if (!pNumber) + { + SAL_WARN("xmlsecurity.pdfio", + "ValidateSignature: signature offset and length has to be a number"); + return false; + } + + if (i % 2 == 0) + { + nByteRangeOffset = pNumber->GetValue(); + continue; + } + size_t nByteRangeLength = pNumber->GetValue(); + rByteRanges.emplace_back(nByteRangeOffset, nByteRangeLength); + } + + return true; +} + +/// Determines the last position that is covered by a signature. +bool GetEOFOfSignature(vcl::filter::PDFObjectElement* pSignature, size_t& rEOF) +{ + vcl::filter::PDFObjectElement* pValue = pSignature->LookupObject("V"); + if (!pValue) + { + return false; + } + + auto pByteRange = dynamic_cast(pValue->Lookup("ByteRange")); + if (!pByteRange || pByteRange->GetElements().size() < 2) + { + return false; + } + + std::vector> aByteRanges; + if (!GetByteRangesFromPDF(*pByteRange, aByteRanges)) + { + return false; + } + + rEOF = aByteRanges[1].first + aByteRanges[1].second; + return true; +} + +/// Checks if there are unsigned incremental updates between the signatures or after the last one. +bool IsCompleteSignature(SvStream& rStream, vcl::filter::PDFDocument& rDocument, + vcl::filter::PDFObjectElement* pSignature) +{ + std::set aSignedEOFs; + for (const auto& i : rDocument.GetSignatureWidgets()) + { + size_t nEOF = 0; + if (!GetEOFOfSignature(i, nEOF)) + { + return false; + } + + aSignedEOFs.insert(nEOF); + } + + size_t nSignatureEOF = 0; + if (!GetEOFOfSignature(pSignature, nSignatureEOF)) + { + return false; + } + + const std::vector& rAllEOFs = rDocument.GetEOFs(); + bool bFoundOwn = false; + for (const auto& rEOF : rAllEOFs) + { + if (rEOF == nSignatureEOF) + { + bFoundOwn = true; + continue; + } + + if (!bFoundOwn) + { + continue; + } + + if (aSignedEOFs.find(rEOF) == aSignedEOFs.end()) + { + // Unsigned incremental update found. + return false; + } + } + + // Make sure we find the incremental update of the signature itself. + if (!bFoundOwn) + { + return false; + } + + // No additional content after the last incremental update. + rStream.Seek(STREAM_SEEK_TO_END); + size_t nFileEnd = rStream.Tell(); + return std::find(rAllEOFs.begin(), rAllEOFs.end(), nFileEnd) != rAllEOFs.end(); +} +} + namespace xmlsecurity { namespace pdfio { bool ValidateSignature(SvStream& rStream, vcl::filter::PDFObjectElement* pSignature, - SignatureInformation& rInformation, bool bLast) + SignatureInformation& rInformation, vcl::filter::PDFDocument& rDocument) { vcl::filter::PDFObjectElement* pValue = pSignature->LookupObject("V"); if (!pValue) @@ -110,25 +222,9 @@ bool ValidateSignature(SvStream& rStream, vcl::filter::PDFObjectElement* pSignat // Build a list of offset-length pairs, representing the signed bytes. std::vector> aByteRanges; - size_t nByteRangeOffset = 0; - const std::vector& rByteRangeElements = pByteRange->GetElements(); - for (size_t i = 0; i < rByteRangeElements.size(); ++i) + if (!GetByteRangesFromPDF(*pByteRange, aByteRanges)) { - auto pNumber = dynamic_cast(rByteRangeElements[i]); - if (!pNumber) - { - SAL_WARN("xmlsecurity.pdfio", - "ValidateSignature: signature offset and length has to be a number"); - return false; - } - - if (i % 2 == 0) - { - nByteRangeOffset = pNumber->GetValue(); - continue; - } - size_t nByteRangeLength = pNumber->GetValue(); - aByteRanges.emplace_back(nByteRangeOffset, nByteRangeLength); + return false; } // Detect if the byte ranges don't cover everything, but the signature itself. @@ -150,11 +246,7 @@ bool ValidateSignature(SvStream& rStream, vcl::filter::PDFObjectElement* pSignat "ValidateSignature: second range start is not the end of the signature"); return false; } - rStream.Seek(STREAM_SEEK_TO_END); - size_t nFileEnd = rStream.Tell(); - if (bLast && (aByteRanges[1].first + aByteRanges[1].second) != nFileEnd) - // Second range end is not the end of the file. - rInformation.bPartialDocumentSignature = true; + rInformation.bPartialDocumentSignature = !IsCompleteSignature(rStream, rDocument, pSignature); // At this point there is no obviously missing info to validate the // signature. diff --git a/xmlsecurity/workben/pdfverify.cxx b/xmlsecurity/workben/pdfverify.cxx index 191ca8c087eb..3076d1c47a43 100644 --- a/xmlsecurity/workben/pdfverify.cxx +++ b/xmlsecurity/workben/pdfverify.cxx @@ -159,8 +159,8 @@ int pdfVerify(int nArgc, char** pArgv) for (size_t i = 0; i < aSignatures.size(); ++i) { SignatureInformation aInfo(i); - bool bLast = i == aSignatures.size() - 1; - if (!xmlsecurity::pdfio::ValidateSignature(aStream, aSignatures[i], aInfo, bLast)) + if (!xmlsecurity::pdfio::ValidateSignature(aStream, aSignatures[i], aInfo, + aDocument)) { SAL_WARN("xmlsecurity.pdfio", "failed to determine digest match"); return 1; @@ -169,6 +169,8 @@ int pdfVerify(int nArgc, char** pArgv) bool bSuccess = aInfo.nStatus == xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED; std::cerr << "signature #" << i << ": digest match? " << bSuccess << std::endl; + std::cerr << "signature #" << i << ": partial? " << aInfo.bPartialDocumentSignature + << std::endl; } } -- cgit