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.

Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124278
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
(cherry picked from commit fd5463343ab7f784070f1ab87a345eed20803d07)

Change-Id: I53c7249a82fc0623586548db9fa25bdc0e7c4101

5 files changed, 152 insertions, 23 deletions

diff --git a/xmlsecurity/qa/unit/signing/signing.cxx b/xmlsecurity/qa/unit/signing/signing.cxx
index 36034ee41d6f..5ba3a0776155 100644
--- a/xmlsecurity/qa/unit/signing/signing.cxx
+++ b/xmlsecurity/qa/unit/signing/signing.cxx
@@ -967,13 +967,13 @@ void 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");
 }
 
@@ -1031,12 +1031,132 @@ void 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();
 }
 
 void SigningTest::testXAdESGood()
diff --git a/xmlsecurity/source/helper/documentsignaturehelper.cxx b/xmlsecurity/source/helper/documentsignaturehelper.cxx
index fd81f4640bf0..8f006ccc7d0c 100644
--- a/xmlsecurity/source/helper/documentsignaturehelper.cxx
+++ b/xmlsecurity/source/helper/documentsignaturehelper.cxx
@@ -542,7 +542,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 141ae49176ed..15b13efc9e5d 100644
--- a/xmlsecurity/source/helper/ooxmlsecexporter.cxx
+++ b/xmlsecurity/source/helper/ooxmlsecexporter.cxx
@@ -161,14 +161,14 @@ 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
                     pAttributeList->AddAttribute("Type", "http://uri.etsi.org/01903#SignedProperties");
                 pAttributeList->AddAttribute("URI", "#" + rReference.ouURI);
                 m_xDocumentHandler->startElement("Reference", uno::Reference<xml::sax::XAttributeList>(pAttributeList.get()));
             }
-            if (rReference.ouURI == "idSignedProperties")
+            if (rReference.ouURI.startsWith("idSignedProperties"))
             {
                 m_xDocumentHandler->startElement("Transforms", uno::Reference<xml::sax::XAttributeList>(new SvXMLAttributeList()));
                 writeCanonicalizationTransform();
diff --git a/xmlsecurity/source/helper/xsecctl.cxx b/xmlsecurity/source/helper/xsecctl.cxx
index e87c8cafee56..c0e3d03e5ab6 100644
--- a/xmlsecurity/source/helper/xsecctl.cxx
+++ b/xmlsecurity/source/helper/xsecctl.cxx
@@ -537,7 +537,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()));
     }
 
@@ -654,16 +654,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 fd6d5c9c8f24..ae9880188e56 100644
--- a/xmlsecurity/source/helper/xsecsign.cxx
+++ b/xmlsecurity/source/helper/xsecsign.cxx
@@ -139,8 +139,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++;
         }
 
@@ -152,13 +153,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++;
     }