xmlsecurity/qa/unit/signing/signing.cxx               |  167 +++++++++++++++++-
 xmlsecurity/source/helper/documentsignaturehelper.cxx |    2 
 xmlsecurity/source/helper/ooxmlsecexporter.cxx        |   58 +++---
 xmlsecurity/source/helper/xsecctl.cxx                 |   23 +-
 xmlsecurity/source/helper/xsecsign.cxx                |   14 -
 5 files changed, 214 insertions(+), 50 deletions(-)

New commits:
commit ead080e88b3f87e0ccce134832af5af472fcfd06
Author:     Tomaž Vajngerl <tomaz.vajng...@collabora.co.uk>
AuthorDate: Mon Nov 1 21:46:43 2021 +0100
Commit:     Tomaž Vajngerl <tomaz.vajng...@collabora.co.uk>
CommitDate: Thu Nov 4 10:32:47 2021 +0100

    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).
    
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124571
    Tested-by: Jenkins
    Reviewed-by: Tomaž Vajngerl <qui...@gmail.com>
    (cherry picked from commit f2e1e4ff085962a08a5d7738325b383c07afcbbd)
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124598
    Reviewed-by: Jan Holesovsky <ke...@collabora.com>
    Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoff...@gmail.com>
    (cherry picked from commit 59c3242b75fdc6d44992919e56bc9a379c699374)
    
    Change-Id: Ifa20ea17498b117a4c57f6eddf82f8e83bc640bc

diff --git a/xmlsecurity/qa/unit/signing/signing.cxx 
b/xmlsecurity/qa/unit/signing/signing.cxx
index 7ac9aa8dc880..2a0e39d5c977 100644
--- a/xmlsecurity/qa/unit/signing/signing.cxx
+++ b/xmlsecurity/qa/unit/signing/signing.cxx
@@ -52,6 +52,7 @@
 #include <sfx2/docfilt.hxx>
 #include <officecfg/Office/Common.hxx>
 #include <comphelper/configuration.hxx>
+#include <vcl/scheduler.hxx>
 
 using namespace com::sun::star;
 
@@ -974,55 +975,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");
+    {
+        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);
+        }
 
-    // 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);
+        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);
+        }
 
-    // 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>(3), 
rInformations.size());
+            
CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
+                                 rInformations[2].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.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>(3), 
rInformations.size());
-        
CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
-                             rInformations[2].nStatus);
-    }
+    Scheduler::ProcessEventsToIdle();
+
+    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)
@@ -1036,54 +1054,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");
+    {
+        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);
+        }
 
-    // Create a signature.
-    uno::Reference<security::XCertificate> xCertificate
-        = getCertificate(aManager, 
svl::crypto::SignatureMethodAlgorithm::ECDSA);
-    if (!xCertificate.is())
-        return;
+        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);
+        }
 
-    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>(3), 
rInformations.size());
+            
CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED,
+                                 rInformations[2].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.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>(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();
+    Scheduler::ProcessEventsToIdle();
+
+    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 4797bd8d8796..9267b5458783 100644
--- a/xmlsecurity/source/helper/ooxmlsecexporter.cxx
+++ b/xmlsecurity/source/helper/ooxmlsecexporter.cxx
@@ -62,6 +62,7 @@ public:
         return m_xDocumentHandler;
     }
 
+    void writeSignature();
     void writeSignedInfo();
     void writeCanonicalizationMethod();
     void writeCanonicalizationTransform();
@@ -220,7 +221,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()));
 
@@ -299,8 +300,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()));
     }
@@ -379,7 +381,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()));
     }
@@ -387,8 +389,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()));
     }
@@ -476,7 +478,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()));
@@ -519,6 +521,25 @@ void OOXMLSecExporter::Impl::writeSignatureLineImages()
     }
 }
 
+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,
@@ -531,23 +552,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++;
     }
 
commit 3af5c6b05a0d1c0e8629dcb797f102bbd830132d
Author:     Tomaž Vajngerl <tomaz.vajng...@collabora.co.uk>
AuthorDate: Wed Oct 27 14:15:17 2021 +0200
Commit:     Tomaž Vajngerl <tomaz.vajng...@collabora.co.uk>
CommitDate: Thu Nov 4 10:31:32 2021 +0100

    xmlsec: signing the document fails the 3rd time (invalid signature)
    
    Signing the document 3 or more times produces an invalid signature.
    The cause of this is that xmlsec is confused because we have 3
    signatures, which all have the same SignedProperties with the ID
    "idSignedProperties", but it expect them to be unique.
    
    This issue is fixed by making the ID unique with adding the ID of
    the Signature to the SignedProperties ID, so this makes them unique
    inside the same Signature.
    
    Also UnsignedProperties have a unique ID usign the same approach,
    but they aren't referenced - luckily.
    
    Change-Id: I53c7249a82fc0623586548db9fa25bdc0e7c4101
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124278
    Tested-by: Jenkins
    Reviewed-by: Tomaž Vajngerl <qui...@gmail.com>
    (cherry picked from commit fd5463343ab7f784070f1ab87a345eed20803d07)
    Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124327
    Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoff...@gmail.com>
    (cherry picked from commit b883bc9d8ca4a9c6037166b2eff09095aef145e0)

diff --git a/xmlsecurity/qa/unit/signing/signing.cxx 
b/xmlsecurity/qa/unit/signing/signing.cxx
index e960a21d2bce..7ac9aa8dc880 100644
--- a/xmlsecurity/qa/unit/signing/signing.cxx
+++ b/xmlsecurity/qa/unit/signing/signing.cxx
@@ -893,13 +893,13 @@ CPPUNIT_TEST_FIXTURE(SigningTest, testXAdESNotype)
     // attribute", i.e. the signature without such an attribute was not 
preserved correctly.
     assertXPathNoAttribute(pXmlDoc,
                            
"/odfds:document-signatures/dsig:Signature[1]/dsig:SignedInfo/"
-                           "dsig:Reference[@URI='#idSignedProperties']",
+                           "dsig:Reference[starts-with(@URI, 
'#idSignedProperties')]",
                            "Type");
 
     // New signature always has the Type attribute.
     assertXPath(pXmlDoc,
                 "/odfds:document-signatures/dsig:Signature[2]/dsig:SignedInfo/"
-                "dsig:Reference[@URI='#idSignedProperties']",
+                "dsig:Reference[starts-with(@URI, '#idSignedProperties')]",
                 "Type", "http://uri.etsi.org/01903#SignedProperties";);
 }
 
@@ -958,12 +958,132 @@ CPPUNIT_TEST_FIXTURE(SigningTest, testXAdES)
     // Assert that the digest of the signing certificate is included.
     assertXPath(pXmlDoc, "//xd:CertDigest", 1);
 
-    // Assert that the Type attribute on the idSignedProperties reference is
-    // not missing.
-    assertXPath(pXmlDoc,
-                "/odfds:document-signatures/dsig:Signature/dsig:SignedInfo/"
-                "dsig:Reference[@URI='#idSignedProperties']",
-                "Type", "http://uri.etsi.org/01903#SignedProperties";);
+    // Assert that the Type attribute is set on all URI's that start with 
#idSignedProperties
+    assertXPath(pXmlDoc, "//dsig:Reference[starts-with(@URI, 
'#idSignedProperties')]", "Type",
+                "http://uri.etsi.org/01903#SignedProperties";);
+}
+
+CPPUNIT_TEST_FIXTURE(SigningTest, testSigningMultipleTimes_ODT)
+{
+    createDoc("");
+
+    utl::TempFile aTempFile;
+    aTempFile.EnableKillingFile();
+    uno::Reference<frame::XStorable> xStorable(mxComponent, uno::UNO_QUERY);
+    utl::MediaDescriptor aMediaDescriptor;
+    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);
+    }
+
+    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);
+    }
+}
+
+CPPUNIT_TEST_FIXTURE(SigningTest, testSigningMultipleTimes_OOXML)
+{
+    createDoc("");
+
+    utl::TempFile aTempFile;
+    aTempFile.EnableKillingFile();
+    uno::Reference<frame::XStorable> xStorable(mxComponent, uno::UNO_QUERY);
+    utl::MediaDescriptor aMediaDescriptor;
+    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);
+    }
+
+    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();
 }
 
 /// Works with an existing good XAdES signature.
diff --git a/xmlsecurity/source/helper/documentsignaturehelper.cxx 
b/xmlsecurity/source/helper/documentsignaturehelper.cxx
index ddff308ee52f..65135d758a1a 100644
--- a/xmlsecurity/source/helper/documentsignaturehelper.cxx
+++ b/xmlsecurity/source/helper/documentsignaturehelper.cxx
@@ -522,7 +522,7 @@ void DocumentSignatureHelper::writeSignedProperties(
 {
     {
         rtl::Reference<SvXMLAttributeList> pAttributeList(new 
SvXMLAttributeList());
-        pAttributeList->AddAttribute("Id", "idSignedProperties");
+        pAttributeList->AddAttribute("Id", "idSignedProperties_" + 
signatureInfo.ouSignatureId);
         xDocumentHandler->startElement("xd:SignedProperties", 
uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
     }
 
diff --git a/xmlsecurity/source/helper/ooxmlsecexporter.cxx 
b/xmlsecurity/source/helper/ooxmlsecexporter.cxx
index 1e031644b0b5..4797bd8d8796 100644
--- a/xmlsecurity/source/helper/ooxmlsecexporter.cxx
+++ b/xmlsecurity/source/helper/ooxmlsecexporter.cxx
@@ -158,7 +158,7 @@ void OOXMLSecExporter::Impl::writeSignedInfoReferences()
         {
             {
                 rtl::Reference<SvXMLAttributeList> pAttributeList(new 
SvXMLAttributeList());
-                if (rReference.ouURI != "idSignedProperties")
+                if (!rReference.ouURI.startsWith("idSignedProperties"))
                     pAttributeList->AddAttribute("Type",
                                                  
"http://www.w3.org/2000/09/xmldsig#Object";);
                 else
@@ -168,7 +168,7 @@ void OOXMLSecExporter::Impl::writeSignedInfoReferences()
                 m_xDocumentHandler->startElement(
                     "Reference", 
uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
             }
-            if (rReference.ouURI == "idSignedProperties")
+            if (rReference.ouURI.startsWith("idSignedProperties"))
             {
                 m_xDocumentHandler->startElement(
                     "Transforms",
diff --git a/xmlsecurity/source/helper/xsecctl.cxx 
b/xmlsecurity/source/helper/xsecctl.cxx
index 6bd88e24f91e..cdfe643bd0f7 100644
--- a/xmlsecurity/source/helper/xsecctl.cxx
+++ b/xmlsecurity/source/helper/xsecctl.cxx
@@ -532,7 +532,7 @@ void writeUnsignedProperties(
 {
     {
         rtl::Reference<SvXMLAttributeList> pAttributeList(new 
SvXMLAttributeList());
-        pAttributeList->AddAttribute("Id", "idUnsignedProperties");
+        pAttributeList->AddAttribute("Id", "idUnsignedProperties_" + 
signatureInfo.ouSignatureId);
         xDocumentHandler->startElement("xd:UnsignedProperties", 
uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
     }
 
@@ -649,16 +649,21 @@ void XSecController::exportSignature(
                  * same-document reference
                  */
                 {
-                    pAttributeList->AddAttribute(
+                    if (refInfor.ouURI.startsWith("idSignedProperties"))
+                    {
+                        pAttributeList->AddAttribute("URI", 
"#idSignedProperties_" + signatureInfo.ouSignatureId);
+                        if (bXAdESCompliantIfODF && !refInfor.ouType.isEmpty())
+                        {
+                            // The reference which points to the 
SignedProperties
+                            // shall have this specific type.
+                            pAttributeList->AddAttribute("Type", 
refInfor.ouType);
+                        }
+                    }
+                    else
+                    {
+                        pAttributeList->AddAttribute(
                         "URI",
                         "#" + refInfor.ouURI);
-
-                    if (bXAdESCompliantIfODF && refInfor.ouURI == 
"idSignedProperties" && !refInfor.ouType.isEmpty())
-                    {
-                        // The reference which points to the SignedProperties
-                        // shall have this specific type.
-                        pAttributeList->AddAttribute("Type",
-                                                     refInfor.ouType);
                     }
                 }
 
diff --git a/xmlsecurity/source/helper/xsecsign.cxx 
b/xmlsecurity/source/helper/xsecsign.cxx
index fd33a320d9bd..e0cda6c43695 100644
--- a/xmlsecurity/source/helper/xsecsign.cxx
+++ b/xmlsecurity/source/helper/xsecsign.cxx
@@ -134,8 +134,9 @@ css::uno::Reference< 
css::xml::crypto::sax::XReferenceResolvedListener > XSecCon
 
         if (bXAdESCompliantIfODF)
         {
+            OUString aId = "idSignedProperties_" +  
internalSignatureInfor.signatureInfor.ouSignatureId;
             // We write a new reference, so it's possible to use the correct 
type URI.
-            
internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, 
digestID, "idSignedProperties", -1, 
"http://uri.etsi.org/01903#SignedProperties";);
+            
internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, 
digestID, aId, -1, "http://uri.etsi.org/01903#SignedProperties";);
             size++;
         }
 
@@ -147,13 +148,16 @@ css::uno::Reference< 
css::xml::crypto::sax::XReferenceResolvedListener > XSecCon
             size++;
         }
     }
-    else
+    else // OOXML
     {
+        internalSignatureInfor.signatureInfor.ouSignatureId = 
"idPackageSignature";
+
         
internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, 
digestID, "idPackageObject", -1, OUString());
         size++;
         
internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, 
digestID, "idOfficeObject", -1, OUString());
         size++;
-        
internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, 
digestID, "idSignedProperties", -1, OUString());
+        OUString aId = "idSignedProperties_" +  
internalSignatureInfor.signatureInfor.ouSignatureId;
+        
internalSignatureInfor.addReference(SignatureReferenceType::SAMEDOCUMENT, 
digestID, aId, -1, OUString());
         size++;
     }
 

Reply via email to