xmlsec: fix OOXML signing with multiple certs, extend the test

Signing OOXML with 3 or more times didn't work as other ids
("idPackageObject", "idOfficeObject", ...) were not uniqe. This
change makes those ids unique by appending the signature id. The
signature ID is now generated for OOXML too, while previously it
was a hardcoded string ("idPackageSignature").

The test for signing multiple OOXML was written before, but didn't
catch the issues because it didn't assert the status of the
document after loading it again. This is which is now fixed (and
also added changed for the ODF test case).

Change-Id: Ifa20ea17498b117a4c57f6eddf82f8e83bc640bc
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124571
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
(cherry picked from commit f2e1e4ff085962a08a5d7738325b383c07afcbbd)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124598
Reviewed-by: Jan Holesovsky <kendy@collabora.com>
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>

3 files changed, 154 insertions, 119 deletions

diff --git a/xmlsecurity/qa/unit/signing/signing.cxx b/xmlsecurity/qa/unit/signing/signing.cxx
index 3f632366bf94..75b99e230ba5 100644
--- a/xmlsecurity/qa/unit/signing/signing.cxx
+++ b/xmlsecurity/qa/unit/signing/signing.cxx
@@ -58,6 +58,7 @@
 #include <sfx2/viewsh.hxx>
 #include <comphelper/propertyvalue.hxx>
 #include <vcl/filter/PDFiumLibrary.hxx>
+#include <vcl/scheduler.hxx>
 
 #if HAVE_FEATURE_PDFIUM
 #include <fpdf_annot.h>
@@ -1018,55 +1019,72 @@ CPPUNIT_TEST_FIXTURE(SigningTest, testSigningMultipleTimes_ODT)
     aMediaDescriptor["FilterName"] <<= OUString("writer8");
     xStorable->storeAsURL(aTempFile.GetURL(), aMediaDescriptor.getAsConstPropertyValueList());
 
-    DocumentSignatureManager aManager(mxComponentContext, DocumentSignatureMode::Content);
-    CPPUNIT_ASSERT(aManager.init());
-    uno::Reference<embed::XStorage> xStorage
-        = comphelper::OStorageHelper::GetStorageOfFormatFromURL(
-            ZIP_STORAGE_FORMAT_STRING, aTempFile.GetURL(), embed::ElementModes::READWRITE);
-    CPPUNIT_ASSERT(xStorage.is());
-    aManager.setStore(xStorage);
-    aManager.getSignatureHelper().SetStorage(xStorage, "1.2");
-
-    // Create a signature.
-    uno::Reference<security::XCertificate> xCertificate
-        = getCertificate(aManager, svl::crypto::SignatureMethodAlgorithm::RSA);
-    if (!xCertificate.is())
-        return;
-    sal_Int32 nSecurityId;
-    aManager.add(xCertificate, mxSecurityContext, /*rDescription=*/OUString(), nSecurityId,
-                 /*bAdESCompliant=*/true);
-
-    // Read back the signature and make sure that it's valid.
-    aManager.read(/*bUseTempStream=*/true);
     {
-        std::vector<SignatureInformation>& rInformations
-            = aManager.getCurrentSignatureInformations();
-        CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(1), rInformations.size());
-        CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
-                             rInformations[0].nStatus);
+        DocumentSignatureManager aManager(mxComponentContext, DocumentSignatureMode::Content);
+        CPPUNIT_ASSERT(aManager.init());
+        uno::Reference<embed::XStorage> xStorage
+            = comphelper::OStorageHelper::GetStorageOfFormatFromURL(
+                ZIP_STORAGE_FORMAT_STRING, aTempFile.GetURL(), embed::ElementModes::READWRITE);
+        CPPUNIT_ASSERT(xStorage.is());
+        aManager.setStore(xStorage);
+        aManager.getSignatureHelper().SetStorage(xStorage, "1.2");
+
+        // Create a signature.
+        uno::Reference<security::XCertificate> xCertificate
+            = getCertificate(aManager, svl::crypto::SignatureMethodAlgorithm::RSA);
+
+        if (!xCertificate.is())
+            return;
+        sal_Int32 nSecurityId;
+        aManager.add(xCertificate, mxSecurityContext, /*rDescription=*/OUString(), nSecurityId,
+                     /*bAdESCompliant=*/true);
+
+        // Read back the signature and make sure that it's valid.
+        aManager.read(/*bUseTempStream=*/true);
+        {
+            std::vector<SignatureInformation>& rInformations
+                = aManager.getCurrentSignatureInformations();
+            CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(1), rInformations.size());
+            CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
+                                 rInformations[0].nStatus);
+        }
+
+        aManager.add(xCertificate, mxSecurityContext, /*rDescription=*/OUString(), nSecurityId,
+                     /*bAdESCompliant=*/true);
+        aManager.read(/*bUseTempStream=*/true);
+        {
+            std::vector<SignatureInformation>& rInformations
+                = aManager.getCurrentSignatureInformations();
+            CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(2), rInformations.size());
+            CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
+                                 rInformations[1].nStatus);
+        }
+
+        aManager.add(xCertificate, mxSecurityContext, /*rDescription=*/OUString(), nSecurityId,
+                     /*bAdESCompliant=*/true);
+        aManager.read(/*bUseTempStream=*/true);
+        {
+            std::vector<SignatureInformation>& rInformations
+                = aManager.getCurrentSignatureInformations();
+            CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(3), rInformations.size());
+            CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
+                                 rInformations[2].nStatus);
+        }
+
+        aManager.write(/*bXAdESCompliantIfODF=*/true);
+        uno::Reference<embed::XTransactedObject> xTransactedObject(xStorage, uno::UNO_QUERY);
+        xTransactedObject->commit();
     }
 
-    aManager.add(xCertificate, mxSecurityContext, /*rDescription=*/OUString(), nSecurityId,
-                 /*bAdESCompliant=*/true);
-    aManager.read(/*bUseTempStream=*/true);
-    {
-        std::vector<SignatureInformation>& rInformations
-            = aManager.getCurrentSignatureInformations();
-        CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(2), rInformations.size());
-        CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
-                             rInformations[1].nStatus);
-    }
+    Scheduler::ProcessEventsToIdle();
 
-    aManager.add(xCertificate, mxSecurityContext, /*rDescription=*/OUString(), nSecurityId,
-                 /*bAdESCompliant=*/true);
-    aManager.read(/*bUseTempStream=*/true);
-    {
-        std::vector<SignatureInformation>& rInformations
-            = aManager.getCurrentSignatureInformations();
-        CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(3), rInformations.size());
-        CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
-                             rInformations[2].nStatus);
-    }
+    createDoc(aTempFile.GetURL());
+
+    SfxBaseModel* pBaseModel = dynamic_cast<SfxBaseModel*>(mxComponent.get());
+    CPPUNIT_ASSERT(pBaseModel);
+    SfxObjectShell* pObjectShell = pBaseModel->GetObjectShell();
+    CPPUNIT_ASSERT(pObjectShell);
+    CPPUNIT_ASSERT_EQUAL(SignatureState::OK, pObjectShell->GetDocumentSignatureState());
 }
 
 CPPUNIT_TEST_FIXTURE(SigningTest, testSigningMultipleTimes_OOXML)
@@ -1080,54 +1098,67 @@ CPPUNIT_TEST_FIXTURE(SigningTest, testSigningMultipleTimes_OOXML)
     aMediaDescriptor["FilterName"] <<= OUString("MS Word 2007 XML");
     xStorable->storeAsURL(aTempFile.GetURL(), aMediaDescriptor.getAsConstPropertyValueList());
 
-    DocumentSignatureManager aManager(mxComponentContext, DocumentSignatureMode::Content);
-    CPPUNIT_ASSERT(aManager.init());
-    uno::Reference<embed::XStorage> xStorage
-        = comphelper::OStorageHelper::GetStorageOfFormatFromURL(
-            ZIP_STORAGE_FORMAT_STRING, aTempFile.GetURL(), embed::ElementModes::READWRITE);
-    CPPUNIT_ASSERT(xStorage.is());
-    aManager.setStore(xStorage);
-    aManager.getSignatureHelper().SetStorage(xStorage, "1.2");
-
-    // Create a signature.
-    uno::Reference<security::XCertificate> xCertificate
-        = getCertificate(aManager, svl::crypto::SignatureMethodAlgorithm::ECDSA);
-    if (!xCertificate.is())
-        return;
-
-    sal_Int32 nSecurityId;
-    aManager.add(xCertificate, mxSecurityContext, "", nSecurityId, /*bAdESCompliant=*/false);
-    aManager.read(/*bUseTempStream=*/true);
     {
-        std::vector<SignatureInformation>& rInformations
-            = aManager.getCurrentSignatureInformations();
-        CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(1), rInformations.size());
-        CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
-                             rInformations[0].nStatus);
+        DocumentSignatureManager aManager(mxComponentContext, DocumentSignatureMode::Content);
+        CPPUNIT_ASSERT(aManager.init());
+        uno::Reference<embed::XStorage> xStorage
+            = comphelper::OStorageHelper::GetStorageOfFormatFromURL(
+                ZIP_STORAGE_FORMAT_STRING, aTempFile.GetURL(), embed::ElementModes::READWRITE);
+        CPPUNIT_ASSERT(xStorage.is());
+        aManager.setStore(xStorage);
+        aManager.getSignatureHelper().SetStorage(xStorage, "1.2");
+
+        // Create a signature.
+        uno::Reference<security::XCertificate> xCertificate
+            = getCertificate(aManager, svl::crypto::SignatureMethodAlgorithm::ECDSA);
+        if (!xCertificate.is())
+            return;
+
+        sal_Int32 nSecurityId;
+        aManager.add(xCertificate, mxSecurityContext, "", nSecurityId, /*bAdESCompliant=*/false);
+        aManager.read(/*bUseTempStream=*/true);
+        {
+            std::vector<SignatureInformation>& rInformations
+                = aManager.getCurrentSignatureInformations();
+            CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(1), rInformations.size());
+            CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
+                                 rInformations[0].nStatus);
+        }
+
+        aManager.add(xCertificate, mxSecurityContext, "", nSecurityId, /*bAdESCompliant=*/false);
+        aManager.read(/*bUseTempStream=*/true);
+        {
+            std::vector<SignatureInformation>& rInformations
+                = aManager.getCurrentSignatureInformations();
+            CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(2), rInformations.size());
+            CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
+                                 rInformations[1].nStatus);
+        }
+
+        aManager.add(xCertificate, mxSecurityContext, "", nSecurityId, /*bAdESCompliant=*/false);
+        aManager.read(/*bUseTempStream=*/true);
+        {
+            std::vector<SignatureInformation>& rInformations
+                = aManager.getCurrentSignatureInformations();
+            CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(3), rInformations.size());
+            CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
+                                 rInformations[2].nStatus);
+        }
+
+        aManager.write(/*bXAdESCompliantIfODF=*/true);
+        uno::Reference<embed::XTransactedObject> xTransactedObject(xStorage, uno::UNO_QUERY);
+        xTransactedObject->commit();
     }
 
-    aManager.add(xCertificate, mxSecurityContext, "", nSecurityId, /*bAdESCompliant=*/false);
-    aManager.read(/*bUseTempStream=*/true);
-    {
-        std::vector<SignatureInformation>& rInformations
-            = aManager.getCurrentSignatureInformations();
-        CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(2), rInformations.size());
-        CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
-                             rInformations[1].nStatus);
-    }
+    Scheduler::ProcessEventsToIdle();
 
-    aManager.add(xCertificate, mxSecurityContext, "", nSecurityId, /*bAdESCompliant=*/false);
-    aManager.read(/*bUseTempStream=*/true);
-    {
-        std::vector<SignatureInformation>& rInformations
-            = aManager.getCurrentSignatureInformations();
-        CPPUNIT_ASSERT_EQUAL(static_cast<std::size_t>(3), rInformations.size());
-        CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
-                             rInformations[2].nStatus);
-    }
-    aManager.write(/*bXAdESCompliantIfODF=*/true);
-    uno::Reference<embed::XTransactedObject> xTransactedObject(xStorage, uno::UNO_QUERY);
-    xTransactedObject->commit();
+    createDoc(aTempFile.GetURL());
+
+    SfxBaseModel* pBaseModel = dynamic_cast<SfxBaseModel*>(mxComponent.get());
+    CPPUNIT_ASSERT(pBaseModel);
+    SfxObjectShell* pObjectShell = pBaseModel->GetObjectShell();
+    CPPUNIT_ASSERT(pObjectShell);
+    CPPUNIT_ASSERT_EQUAL(SignatureState::PARTIAL_OK, pObjectShell->GetDocumentSignatureState());
 }
 
 /// Works with an existing good XAdES signature.
diff --git a/xmlsecurity/source/helper/ooxmlsecexporter.cxx b/xmlsecurity/source/helper/ooxmlsecexporter.cxx
index 133e78cc4416..b7dc8da9f7b7 100644
--- a/xmlsecurity/source/helper/ooxmlsecexporter.cxx
+++ b/xmlsecurity/source/helper/ooxmlsecexporter.cxx
@@ -64,6 +64,7 @@ public:
         return m_xDocumentHandler;
     }
 
+    void writeSignature();
     void writeSignedInfo();
     void writeCanonicalizationMethod();
     void writeCanonicalizationTransform();
@@ -222,7 +223,7 @@ void OOXMLSecExporter::Impl::writeKeyInfo()
 void OOXMLSecExporter::Impl::writePackageObject()
 {
     rtl::Reference<SvXMLAttributeList> pAttributeList(new SvXMLAttributeList());
-    pAttributeList->AddAttribute("Id", "idPackageObject");
+    pAttributeList->AddAttribute("Id", "idPackageObject_" + m_rInformation.ouSignatureId);
     m_xDocumentHandler->startElement(
         "Object", uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
 
@@ -301,8 +302,9 @@ void OOXMLSecExporter::Impl::writePackageObjectSignatureProperties()
         "SignatureProperties", uno::Reference<xml::sax::XAttributeList>(new SvXMLAttributeList()));
     {
         rtl::Reference<SvXMLAttributeList> pAttributeList(new SvXMLAttributeList());
-        pAttributeList->AddAttribute("Id", "idSignatureTime");
-        pAttributeList->AddAttribute("Target", "#idPackageSignature");
+
+        pAttributeList->AddAttribute("Id", "idSignatureTime_" + m_rInformation.ouSignatureId);
+        pAttributeList->AddAttribute("Target", "#" + m_rInformation.ouSignatureId);
         m_xDocumentHandler->startElement(
             "SignatureProperty", uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
     }
@@ -381,7 +383,7 @@ void OOXMLSecExporter::Impl::writeOfficeObject()
 {
     {
         rtl::Reference<SvXMLAttributeList> pAttributeList(new SvXMLAttributeList());
-        pAttributeList->AddAttribute("Id", "idOfficeObject");
+        pAttributeList->AddAttribute("Id", "idOfficeObject_" + m_rInformation.ouSignatureId);
         m_xDocumentHandler->startElement(
             "Object", uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
     }
@@ -389,8 +391,8 @@ void OOXMLSecExporter::Impl::writeOfficeObject()
         "SignatureProperties", uno::Reference<xml::sax::XAttributeList>(new SvXMLAttributeList()));
     {
         rtl::Reference<SvXMLAttributeList> pAttributeList(new SvXMLAttributeList());
-        pAttributeList->AddAttribute("Id", "idOfficeV1Details");
-        pAttributeList->AddAttribute("Target", "#idPackageSignature");
+        pAttributeList->AddAttribute("Id", "idOfficeV1Details_" + m_rInformation.ouSignatureId);
+        pAttributeList->AddAttribute("Target", "#" + m_rInformation.ouSignatureId);
         m_xDocumentHandler->startElement(
             "SignatureProperty", uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
     }
@@ -478,7 +480,7 @@ void OOXMLSecExporter::Impl::writePackageSignature()
     {
         rtl::Reference<SvXMLAttributeList> pAttributeList(new SvXMLAttributeList());
         pAttributeList->AddAttribute("xmlns:xd", NS_XD);
-        pAttributeList->AddAttribute("Target", "#idPackageSignature");
+        pAttributeList->AddAttribute("Target", "#" + m_rInformation.ouSignatureId);
         m_xDocumentHandler->startElement(
             "xd:QualifyingProperties",
             uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
@@ -521,6 +523,25 @@ void OOXMLSecExporter::Impl::writeSignatureLineImages()
     m_xDocumentHandler->endElement("Object");
 }
 
+void OOXMLSecExporter::Impl::writeSignature()
+{
+    rtl::Reference<SvXMLAttributeList> pAttributeList(new SvXMLAttributeList());
+    pAttributeList->AddAttribute("xmlns", NS_XMLDSIG);
+    pAttributeList->AddAttribute("Id", m_rInformation.ouSignatureId);
+    getDocumentHandler()->startElement(
+        "Signature", uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
+
+    writeSignedInfo();
+    writeSignatureValue();
+    writeKeyInfo();
+    writePackageObject();
+    writeOfficeObject();
+    writePackageSignature();
+    writeSignatureLineImages();
+
+    getDocumentHandler()->endElement("Signature");
+}
+
 OOXMLSecExporter::OOXMLSecExporter(
     const uno::Reference<uno::XComponentContext>& xComponentContext,
     const uno::Reference<embed::XStorage>& xRootStorage,
@@ -533,23 +554,6 @@ OOXMLSecExporter::OOXMLSecExporter(
 
 OOXMLSecExporter::~OOXMLSecExporter() = default;
 
-void OOXMLSecExporter::writeSignature()
-{
-    rtl::Reference<SvXMLAttributeList> pAttributeList(new SvXMLAttributeList());
-    pAttributeList->AddAttribute("xmlns", NS_XMLDSIG);
-    pAttributeList->AddAttribute("Id", "idPackageSignature");
-    m_pImpl->getDocumentHandler()->startElement(
-        "Signature", uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
-
-    m_pImpl->writeSignedInfo();
-    m_pImpl->writeSignatureValue();
-    m_pImpl->writeKeyInfo();
-    m_pImpl->writePackageObject();
-    m_pImpl->writeOfficeObject();
-    m_pImpl->writePackageSignature();
-    m_pImpl->writeSignatureLineImages();
-
-    m_pImpl->getDocumentHandler()->endElement("Signature");
-}
+void OOXMLSecExporter::writeSignature() { m_pImpl->writeSignature(); }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/xmlsecurity/source/helper/xsecsign.cxx b/xmlsecurity/source/helper/xsecsign.cxx
index e0cda6c43695..8676f70c1041 100644
--- a/xmlsecurity/source/helper/xsecsign.cxx
+++ b/xmlsecurity/source/helper/xsecsign.cxx
@@ -150,14 +150,14 @@ css::uno::Reference< css::xml::crypto::sax::XReferenceResolvedListener > XSecCon
     }
     else // OOXML
     {
-        internalSignatureInfor.signatureInfor.ouSignatureId = "idPackageSignature";
+        OUString aID = createId();
+        internalSignatureInfor.signatureInfor.ouSignatureId = aID;
 
-        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idPackageObject", -1, OUString());
+        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idPackageObject_" + aID, -1, OUString());
         size++;
-        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idOfficeObject", -1, OUString());
+        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idOfficeObject_" + aID, -1, OUString());
         size++;
-        OUString aId = "idSignedProperties_" +  internalSignatureInfor.signatureInfor.ouSignatureId;
-        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, aId, -1, OUString());
+        internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, digestID, "idSignedProperties_" + aID, -1, OUString());
         size++;
     }