From b48e37b40e38744bba2cf5f51508ee17c8da678e Mon Sep 17 00:00:00 2001 From: Tomaž Vajngerl Date: Mon, 1 Nov 2021 21:46:43 +0100 Subject: xmlsec: fix OOXML signing with multiple certs, extend the test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 (cherry picked from commit f2e1e4ff085962a08a5d7738325b383c07afcbbd) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124598 Reviewed-by: Jan Holesovsky Tested-by: Jenkins CollaboraOffice (cherry picked from commit 59c3242b75fdc6d44992919e56bc9a379c699374) Change-Id: Ifa20ea17498b117a4c57f6eddf82f8e83bc640bc --- xmlsecurity/qa/unit/signing/signing.cxx | 205 ++++++++++++++----------- xmlsecurity/source/helper/ooxmlsecexporter.cxx | 54 ++++--- xmlsecurity/source/helper/xsecsign.cxx | 10 +- 3 files changed, 152 insertions(+), 117 deletions(-) diff --git a/xmlsecurity/qa/unit/signing/signing.cxx b/xmlsecurity/qa/unit/signing/signing.cxx index 8c42f63730cb..07cfce5dcb2b 100644 --- a/xmlsecurity/qa/unit/signing/signing.cxx +++ b/xmlsecurity/qa/unit/signing/signing.cxx @@ -53,6 +53,7 @@ #include #include #include +#include using namespace com::sun::star; @@ -959,55 +960,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 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 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 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& rInformations + = aManager.getCurrentSignatureInformations(); + CPPUNIT_ASSERT_EQUAL(static_cast(1), rInformations.size()); + CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED, + rInformations[0].nStatus); + } - // Create a signature. - uno::Reference 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& rInformations + = aManager.getCurrentSignatureInformations(); + CPPUNIT_ASSERT_EQUAL(static_cast(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& rInformations - = aManager.getCurrentSignatureInformations(); - CPPUNIT_ASSERT_EQUAL(static_cast(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& rInformations + = aManager.getCurrentSignatureInformations(); + CPPUNIT_ASSERT_EQUAL(static_cast(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& rInformations - = aManager.getCurrentSignatureInformations(); - CPPUNIT_ASSERT_EQUAL(static_cast(2), rInformations.size()); - CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED, - rInformations[1].nStatus); + aManager.write(/*bXAdESCompliantIfODF=*/true); + uno::Reference xTransactedObject(xStorage, uno::UNO_QUERY); + xTransactedObject->commit(); } - aManager.add(xCertificate, mxSecurityContext, /*rDescription=*/OUString(), nSecurityId, - /*bAdESCompliant=*/true); - aManager.read(/*bUseTempStream=*/true); - { - std::vector& rInformations - = aManager.getCurrentSignatureInformations(); - CPPUNIT_ASSERT_EQUAL(static_cast(3), rInformations.size()); - CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED, - rInformations[2].nStatus); - } + Scheduler::ProcessEventsToIdle(); + + createDoc(aTempFile.GetURL()); + + SfxBaseModel* pBaseModel = dynamic_cast(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) @@ -1021,54 +1039,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 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 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 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& rInformations + = aManager.getCurrentSignatureInformations(); + CPPUNIT_ASSERT_EQUAL(static_cast(1), rInformations.size()); + CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED, + rInformations[0].nStatus); + } - // Create a signature. - uno::Reference xCertificate - = getCertificate(aManager, svl::crypto::SignatureMethodAlgorithm::ECDSA); - if (!xCertificate.is()) - return; + aManager.add(xCertificate, mxSecurityContext, "", nSecurityId, /*bAdESCompliant=*/false); + aManager.read(/*bUseTempStream=*/true); + { + std::vector& rInformations + = aManager.getCurrentSignatureInformations(); + CPPUNIT_ASSERT_EQUAL(static_cast(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& rInformations - = aManager.getCurrentSignatureInformations(); - CPPUNIT_ASSERT_EQUAL(static_cast(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& rInformations + = aManager.getCurrentSignatureInformations(); + CPPUNIT_ASSERT_EQUAL(static_cast(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& rInformations - = aManager.getCurrentSignatureInformations(); - CPPUNIT_ASSERT_EQUAL(static_cast(2), rInformations.size()); - CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED, - rInformations[1].nStatus); + aManager.write(/*bXAdESCompliantIfODF=*/true); + uno::Reference xTransactedObject(xStorage, uno::UNO_QUERY); + xTransactedObject->commit(); } - aManager.add(xCertificate, mxSecurityContext, "", nSecurityId, /*bAdESCompliant=*/false); - aManager.read(/*bUseTempStream=*/true); - { - std::vector& rInformations - = aManager.getCurrentSignatureInformations(); - CPPUNIT_ASSERT_EQUAL(static_cast(3), rInformations.size()); - CPPUNIT_ASSERT_EQUAL(css::xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED, - rInformations[2].nStatus); - } - aManager.write(/*bXAdESCompliantIfODF=*/true); - uno::Reference xTransactedObject(xStorage, uno::UNO_QUERY); - xTransactedObject->commit(); + Scheduler::ProcessEventsToIdle(); + + createDoc(aTempFile.GetURL()); + + SfxBaseModel* pBaseModel = dynamic_cast(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 pAttributeList(new SvXMLAttributeList()); - pAttributeList->AddAttribute("Id", "idPackageObject"); + pAttributeList->AddAttribute("Id", "idPackageObject_" + m_rInformation.ouSignatureId); m_xDocumentHandler->startElement( "Object", uno::Reference(pAttributeList.get())); @@ -299,8 +300,9 @@ void OOXMLSecExporter::Impl::writePackageObjectSignatureProperties() "SignatureProperties", uno::Reference(new SvXMLAttributeList())); { rtl::Reference 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(pAttributeList.get())); } @@ -379,7 +381,7 @@ void OOXMLSecExporter::Impl::writeOfficeObject() { { rtl::Reference pAttributeList(new SvXMLAttributeList()); - pAttributeList->AddAttribute("Id", "idOfficeObject"); + pAttributeList->AddAttribute("Id", "idOfficeObject_" + m_rInformation.ouSignatureId); m_xDocumentHandler->startElement( "Object", uno::Reference(pAttributeList.get())); } @@ -387,8 +389,8 @@ void OOXMLSecExporter::Impl::writeOfficeObject() "SignatureProperties", uno::Reference(new SvXMLAttributeList())); { rtl::Reference 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(pAttributeList.get())); } @@ -476,7 +478,7 @@ void OOXMLSecExporter::Impl::writePackageSignature() { rtl::Reference 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(pAttributeList.get())); @@ -519,6 +521,25 @@ void OOXMLSecExporter::Impl::writeSignatureLineImages() } } +void OOXMLSecExporter::Impl::writeSignature() +{ + rtl::Reference pAttributeList(new SvXMLAttributeList()); + pAttributeList->AddAttribute("xmlns", NS_XMLDSIG); + pAttributeList->AddAttribute("Id", m_rInformation.ouSignatureId); + getDocumentHandler()->startElement( + "Signature", uno::Reference(pAttributeList.get())); + + writeSignedInfo(); + writeSignatureValue(); + writeKeyInfo(); + writePackageObject(); + writeOfficeObject(); + writePackageSignature(); + writeSignatureLineImages(); + + getDocumentHandler()->endElement("Signature"); +} + OOXMLSecExporter::OOXMLSecExporter( const uno::Reference& xComponentContext, const uno::Reference& xRootStorage, @@ -531,23 +552,6 @@ OOXMLSecExporter::OOXMLSecExporter( OOXMLSecExporter::~OOXMLSecExporter() = default; -void OOXMLSecExporter::writeSignature() -{ - rtl::Reference pAttributeList(new SvXMLAttributeList()); - pAttributeList->AddAttribute("xmlns", NS_XMLDSIG); - pAttributeList->AddAttribute("Id", "idPackageSignature"); - m_pImpl->getDocumentHandler()->startElement( - "Signature", uno::Reference(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++; } -- cgit v1.2.3