diff options
author | Tomaž Vajngerl <tomaz.vajngerl@collabora.co.uk> | 2021-11-01 21:46:43 +0100 |
---|---|---|
committer | Tomaž Vajngerl <tomaz.vajngerl@collabora.co.uk> | 2021-11-04 10:35:26 +0100 |
commit | 23dc593d44acfb8c2185e62499f49774c4cfc1c2 (patch) | |
tree | fe551ce41ec190693671bd95f05c2dd8cd637859 | |
parent | 85f83b12840cd7e8140a6423a104a61d23e7779c (diff) |
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>
(cherry picked from commit 59c3242b75fdc6d44992919e56bc9a379c699374)
-rw-r--r-- | xmlsecurity/qa/unit/signing/signing.cxx | 205 | ||||
-rw-r--r-- | xmlsecurity/source/helper/ooxmlsecexporter.cxx | 54 | ||||
-rw-r--r-- | 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 7dffaf779fc2..3b57bf78fdfa 100644 --- a/xmlsecurity/qa/unit/signing/signing.cxx +++ b/xmlsecurity/qa/unit/signing/signing.cxx @@ -51,6 +51,7 @@ #include <sfx2/docfilt.hxx> #include <officecfg/Office/Common.hxx> #include <comphelper/configuration.hxx> +#include <vcl/scheduler.hxx> using namespace com::sun::star; @@ -971,55 +972,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) @@ -1033,54 +1051,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 a899cc1f3f3b..70adff39d201 100644 --- a/xmlsecurity/source/helper/ooxmlsecexporter.cxx +++ b/xmlsecurity/source/helper/ooxmlsecexporter.cxx @@ -63,6 +63,7 @@ public: return m_xDocumentHandler; } + void writeSignature(); void writeSignedInfo(); void writeCanonicalizationMethod(); void writeCanonicalizationTransform(); @@ -221,7 +222,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())); @@ -300,8 +301,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())); } @@ -380,7 +382,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())); } @@ -388,8 +390,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())); } @@ -477,7 +479,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())); @@ -520,6 +522,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, @@ -532,23 +553,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++; } |