xmlsecurity: detect unsigned incremental update between signatures

Change-Id: I269ed858852ee7d1275adf340c8cc1565fc30693
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/99361
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins

include/vcl/filter/pdfdocument.hxx

@@ -422,6 +422,8 @@ public:
     std::vector<PDFObjectElement*> 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<size_t>& GetEOFs() const;
     //@}
 
     /// See vcl::PDFObjectContainer::createObject().

vcl/source/filter/ipdf/pdfdocument.cxx

@@ -156,6 +156,8 @@ bool PDFDocument::RemoveSignature(size_t nPosition)
     return m_aEditBuffer.good();
 }
 
+const std::vector<size_t>& PDFDocument::GetEOFs() const { return m_aEOFs; }
+
 sal_Int32 PDFDocument::createObject()
 {
     sal_Int32 nObject = m_aXRef.size();
@@ -2154,7 +2156,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;

xmlsecurity/inc/pdfio/pdfdocument.hxx

@@ -16,6 +16,7 @@
 namespace vcl::filter
 {
 class PDFObjectElement;
+class PDFDocument;
 }
 struct SignatureInformation;
 class SvStream;
@@ -24,12 +25,13 @@ namespace xmlsecurity::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 xmlsecurity::pdfio
 

xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx

@@ -89,9 +89,8 @@ std::vector<SignatureInformation> 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())
@@ -236,7 +235,7 @@ CPPUNIT_TEST_FIXTURE(PDFSigningTest, testPDFRemove)
         CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(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.
@@ -392,6 +391,18 @@ CPPUNIT_TEST_FIXTURE(PDFSigningTest, testPartial)
     CPPUNIT_ASSERT(rInformation.bPartialDocumentSignature);
 }
 
+CPPUNIT_TEST_FIXTURE(PDFSigningTest, testPartialInBetween)
+{
+    std::vector<SignatureInformation> 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)
 {

xmlsecurity/source/helper/pdfsignaturehelper.cxx

@@ -143,8 +143,7 @@ bool PDFSignatureHelper::ReadAndVerifySignature(
     {
         SignatureInformation aInfo(i);
 
-        bool bLast = i == aSignatures.size() - 1;
+        if (!xmlsecurity::pdfio::ValidateSignature(*pStream, aSignatures[i], aInfo, aDocument))
-        if (!xmlsecurity::pdfio::ValidateSignature(*pStream, aSignatures[i], aInfo, bLast))
             SAL_WARN("xmlsecurity.helper", "failed to determine digest match");
 
         m_aSignatureInfos.push_back(aInfo);

xmlsecurity/source/pdfio/pdfdocument.cxx

@@ -23,10 +23,122 @@
 
 using namespace com::sun::star;
 
+namespace
+{
+/// Turns an array of floats into offset + length pairs.
+bool GetByteRangesFromPDF(vcl::filter::PDFArrayElement& rArray,
+                          std::vector<std::pair<size_t, size_t>>& rByteRanges)
+{
+    size_t nByteRangeOffset = 0;
+    const std::vector<vcl::filter::PDFElement*>& rByteRangeElements = rArray.GetElements();
+    for (size_t i = 0; i < rByteRangeElements.size(); ++i)
+    {
+        auto pNumber = dynamic_cast<vcl::filter::PDFNumberElement*>(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<vcl::filter::PDFArrayElement*>(pValue->Lookup("ByteRange"));
+    if (!pByteRange || pByteRange->GetElements().size() < 2)
+    {
+        return false;
+    }
+
+    std::vector<std::pair<size_t, size_t>> 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<size_t> 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<size_t>& 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::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)
@@ -108,25 +220,9 @@ bool ValidateSignature(SvStream& rStream, vcl::filter::PDFObjectElement* pSignat
 
     // Build a list of offset-length pairs, representing the signed bytes.
     std::vector<std::pair<size_t, size_t>> aByteRanges;
-    size_t nByteRangeOffset = 0;
+    if (!GetByteRangesFromPDF(*pByteRange, aByteRanges))
-    const std::vector<vcl::filter::PDFElement*>& rByteRangeElements = pByteRange->GetElements();
-    for (size_t i = 0; i < rByteRangeElements.size(); ++i)
     {
-        auto pNumber = dynamic_cast<vcl::filter::PDFNumberElement*>(rByteRangeElements[i]);
+        return false;
-        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);
     }
 
     // Detect if the byte ranges don't cover everything, but the signature itself.
@@ -148,11 +244,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);
+    rInformation.bPartialDocumentSignature = !IsCompleteSignature(rStream, rDocument, pSignature);
-    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;
 
     // At this point there is no obviously missing info to validate the
     // signature.

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,
-                if (!xmlsecurity::pdfio::ValidateSignature(aStream, aSignatures[i], aInfo, bLast))
+                                                           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;
             }
         }