From f45ace2897dca1dd8f3553e46415df4fe7ad62d8 Mon Sep 17 00:00:00 2001 From: Miklos Vajna Date: Wed, 26 Oct 2016 17:52:28 +0200 Subject: xmlsecurity PDF signing: fix byte range check for multiple signatures We can mandate that the byte range end is the end of the file for the last signature only. With this, signing a previously unsigned file multiple times works, so add a matching testcase for that as well. Change-Id: I8fe5482890fca4dab8da6305aa7fc7f60df612d8 --- xmlsecurity/inc/pdfio/pdfdocument.hxx | 8 ++- xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx | 70 ++++++++++++++++++------ xmlsecurity/source/helper/pdfsignaturehelper.cxx | 3 +- xmlsecurity/source/pdfio/pdfdocument.cxx | 4 +- xmlsecurity/source/pdfio/pdfverify.cxx | 3 +- 5 files changed, 65 insertions(+), 23 deletions(-) diff --git a/xmlsecurity/inc/pdfio/pdfdocument.hxx b/xmlsecurity/inc/pdfio/pdfdocument.hxx index 59c7f5dd6079..f42350ac5a20 100644 --- a/xmlsecurity/inc/pdfio/pdfdocument.hxx +++ b/xmlsecurity/inc/pdfio/pdfdocument.hxx @@ -88,8 +88,12 @@ public: /// Serializes the contents of the edit buffer. bool Write(SvStream& rStream); std::vector GetSignatureWidgets(); - /// Return value is about if we can determine a result, rInformation is about the actual result. - static bool ValidateSignature(SvStream& rStream, PDFObjectElement* pSignature, SignatureInformation& rInformation); + /** + * @param rInformation The actual result. + * @param bLast If this is the last signature in the file, so it covers the whole file physically. + * @return If we can determinate a result. + */ + static bool ValidateSignature(SvStream& rStream, PDFObjectElement* pSignature, SignatureInformation& rInformation, bool bLast); /// Remove the nth signature from read document in the edit buffer. bool RemoveSignature(size_t nPosition); }; diff --git a/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx b/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx index 6a3800fc8084..a2980db573e6 100644 --- a/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx +++ b/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx @@ -29,17 +29,25 @@ class PDFSigningTest : public test::BootstrapFixture { uno::Reference mxComponentContext; + /** + * Sign rInURL once and save the result as rOutURL, asserting that rInURL + * had nOriginalSignatureCount signatures. + */ + void sign(const OUString& rInURL, const OUString& rOutURL, size_t nOriginalSignatureCount); public: PDFSigningTest(); void setUp() override; /// Test adding a new signature to a previously unsigned file. void testPDFAdd(); + /// Test signing a previously unsigned file twice. + void testPDFAdd2(); /// Test remove a signature from a previously signed file. void testPDFRemove(); CPPUNIT_TEST_SUITE(PDFSigningTest); CPPUNIT_TEST(testPDFAdd); + CPPUNIT_TEST(testPDFAdd2); CPPUNIT_TEST(testPDFRemove); CPPUNIT_TEST_SUITE_END(); }; @@ -67,25 +75,20 @@ void PDFSigningTest::setUp() #endif } -void PDFSigningTest::testPDFAdd() +void PDFSigningTest::sign(const OUString& rInURL, const OUString& rOutURL, size_t nOriginalSignatureCount) { -#ifndef _WIN32 - // Make sure that no.pdf has no signatures. + // Make sure that input has nOriginalSignatureCount signatures. uno::Reference xSEInitializer = xml::crypto::SEInitializer::create(mxComponentContext); uno::Reference xSecurityContext = xSEInitializer->createSecurityContext(OUString()); xmlsecurity::pdfio::PDFDocument aDocument; { - OUString aSourceDir = m_directories.getURLFromSrc(DATA_DIRECTORY); - OUString aInURL = aSourceDir + "no.pdf"; - SvFileStream aStream(aInURL, StreamMode::READ); + SvFileStream aStream(rInURL, StreamMode::READ); CPPUNIT_ASSERT(aDocument.Read(aStream)); std::vector aSignatures = aDocument.GetSignatureWidgets(); - CPPUNIT_ASSERT(aSignatures.empty()); + CPPUNIT_ASSERT_EQUAL(nOriginalSignatureCount, aSignatures.size()); } - // Sign it and write out the result as add.pdf. - OUString aTargetDir = m_directories.getURLFromWorkdir("/CppunitTest/xmlsecurity_signing.test.user/"); - OUString aOutURL = aTargetDir + "add.pdf"; + // Sign it and write out the result. { uno::Reference xSecurityEnvironment = xSecurityContext->getSecurityEnvironment(); uno::Sequence> aCertificates = xSecurityEnvironment->getPersonalCertificates(); @@ -95,21 +98,54 @@ void PDFSigningTest::testPDFAdd() return; } CPPUNIT_ASSERT(aDocument.Sign(aCertificates[0], "test")); - SvFileStream aOutStream(aOutURL, StreamMode::WRITE | StreamMode::TRUNC); + SvFileStream aOutStream(rOutURL, StreamMode::WRITE | StreamMode::TRUNC); CPPUNIT_ASSERT(aDocument.Write(aOutStream)); } // Read back the signed pdf and make sure that it has one valid signature. { - SvFileStream aStream(aOutURL, StreamMode::READ); + SvFileStream aStream(rOutURL, StreamMode::READ); xmlsecurity::pdfio::PDFDocument aVerifyDocument; CPPUNIT_ASSERT(aVerifyDocument.Read(aStream)); std::vector aSignatures = aVerifyDocument.GetSignatureWidgets(); - // This was 0 when PDFDocument::Sign() silently returned success, without doing anything. - CPPUNIT_ASSERT_EQUAL(static_cast(1), aSignatures.size()); - SignatureInformation aInfo(0); - CPPUNIT_ASSERT(xmlsecurity::pdfio::PDFDocument::ValidateSignature(aStream, aSignatures[0], aInfo)); + // This was nOriginalSignatureCount when PDFDocument::Sign() silently returned success, without doing anything. + CPPUNIT_ASSERT_EQUAL(nOriginalSignatureCount + 1, aSignatures.size()); + for (size_t i = 0; i < aSignatures.size(); ++i) + { + SignatureInformation aInfo(i); + bool bLast = i == aSignatures.size() - 1; + CPPUNIT_ASSERT(xmlsecurity::pdfio::PDFDocument::ValidateSignature(aStream, aSignatures[i], aInfo, bLast)); + } } +} + +void PDFSigningTest::testPDFAdd() +{ +#ifndef _WIN32 + OUString aSourceDir = m_directories.getURLFromSrc(DATA_DIRECTORY); + OUString aInURL = aSourceDir + "no.pdf"; + OUString aTargetDir = m_directories.getURLFromWorkdir("/CppunitTest/xmlsecurity_signing.test.user/"); + OUString aOutURL = aTargetDir + "add.pdf"; + sign(aInURL, aOutURL, 0); +#endif +} + +void PDFSigningTest::testPDFAdd2() +{ +#ifndef _WIN32 + // Sign. + OUString aSourceDir = m_directories.getURLFromSrc(DATA_DIRECTORY); + OUString aInURL = aSourceDir + "no.pdf"; + OUString aTargetDir = m_directories.getURLFromWorkdir("/CppunitTest/xmlsecurity_signing.test.user/"); + OUString aOutURL = aTargetDir + "add.pdf"; + sign(aInURL, aOutURL, 0); + + // Sign again. + aInURL = aTargetDir + "add.pdf"; + aOutURL = aTargetDir + "add2.pdf"; + // This failed with "second range end is not the end of the file" for the + // first signature. + sign(aInURL, aOutURL, 1); #endif } @@ -128,7 +164,7 @@ void PDFSigningTest::testPDFRemove() std::vector aSignatures = aDocument.GetSignatureWidgets(); CPPUNIT_ASSERT_EQUAL(static_cast(1), aSignatures.size()); SignatureInformation aInfo(0); - CPPUNIT_ASSERT(xmlsecurity::pdfio::PDFDocument::ValidateSignature(aStream, aSignatures[0], aInfo)); + CPPUNIT_ASSERT(xmlsecurity::pdfio::PDFDocument::ValidateSignature(aStream, aSignatures[0], aInfo, /*bLast=*/true)); } // Remove the signature and write out the result as remove.pdf. diff --git a/xmlsecurity/source/helper/pdfsignaturehelper.cxx b/xmlsecurity/source/helper/pdfsignaturehelper.cxx index a1b1d2038e0a..859a47975972 100644 --- a/xmlsecurity/source/helper/pdfsignaturehelper.cxx +++ b/xmlsecurity/source/helper/pdfsignaturehelper.cxx @@ -58,7 +58,8 @@ bool PDFSignatureHelper::ReadAndVerifySignature(const uno::Reference PDFDocument::DecodeHexString(PDFHexStringElement* pEl return aRet; } -bool PDFDocument::ValidateSignature(SvStream& rStream, PDFObjectElement* pSignature, SignatureInformation& rInformation) +bool PDFDocument::ValidateSignature(SvStream& rStream, PDFObjectElement* pSignature, SignatureInformation& rInformation, bool bLast) { PDFObjectElement* pValue = pSignature->LookupObject("V"); if (!pValue) @@ -1285,7 +1285,7 @@ bool PDFDocument::ValidateSignature(SvStream& rStream, PDFObjectElement* pSignat } rStream.Seek(STREAM_SEEK_TO_END); size_t nFileEnd = rStream.Tell(); - if ((aByteRanges[1].first + aByteRanges[1].second) != nFileEnd) + if (bLast && (aByteRanges[1].first + aByteRanges[1].second) != nFileEnd) { SAL_WARN("xmlsecurity.pdfio", "PDFDocument::ValidateSignature: second range end is not the end of the file"); return false; diff --git a/xmlsecurity/source/pdfio/pdfverify.cxx b/xmlsecurity/source/pdfio/pdfverify.cxx index f980db8a4a78..8e4ba4212900 100644 --- a/xmlsecurity/source/pdfio/pdfverify.cxx +++ b/xmlsecurity/source/pdfio/pdfverify.cxx @@ -113,7 +113,8 @@ SAL_IMPLEMENT_MAIN_WITH_ARGS(nArgc, pArgv) for (size_t i = 0; i < aSignatures.size(); ++i) { SignatureInformation aInfo(i); - if (!xmlsecurity::pdfio::PDFDocument::ValidateSignature(aStream, aSignatures[i], aInfo)) + bool bLast = i == aSignatures.size() - 1; + if (!xmlsecurity::pdfio::PDFDocument::ValidateSignature(aStream, aSignatures[i], aInfo, bLast)) { SAL_WARN("xmlsecurity.pdfio", "failed to determine digest match"); return 1; -- cgit