xmlsecurity/qa/unit/signing/signing.cxx | 209 ++++++++++++++----------- xmlsecurity/source/helper/ooxmlsecexporter.cxx | 53 +++--- xmlsecurity/source/helper/xsecsign.cxx | 10 - 3 files changed, 153 insertions(+), 119 deletions(-)
New commits: commit f2e1e4ff085962a08a5d7738325b383c07afcbbd Author: Tomaž Vajngerl <[email protected]> AuthorDate: Mon Nov 1 21:46:43 2021 +0100 Commit: Tomaž Vajngerl <[email protected]> CommitDate: Tue Nov 2 14:05:50 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). Change-Id: Ifa20ea17498b117a4c57f6eddf82f8e83bc640bc Reviewed-on: https://gerrit.libreoffice.org/c/core/+/124571 Tested-by: Jenkins Reviewed-by: Tomaž Vajngerl <[email protected]> diff --git a/xmlsecurity/qa/unit/signing/signing.cxx b/xmlsecurity/qa/unit/signing/signing.cxx index ed50ade74bf8..de54150d2108 100644 --- a/xmlsecurity/qa/unit/signing/signing.cxx +++ b/xmlsecurity/qa/unit/signing/signing.cxx @@ -60,6 +60,7 @@ #include <sfx2/viewsh.hxx> #include <comphelper/propertyvalue.hxx> #include <vcl/filter/PDFiumLibrary.hxx> +#include <vcl/scheduler.hxx> using namespace com::sun::star; @@ -1062,55 +1063,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) @@ -1124,54 +1142,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 4cdf82ef8561..cac2196a3e28 100644 --- a/xmlsecurity/source/helper/ooxmlsecexporter.cxx +++ b/xmlsecurity/source/helper/ooxmlsecexporter.cxx @@ -65,6 +65,7 @@ public: return m_xDocumentHandler; } + void writeSignature(); void writeSignedInfo(); void writeCanonicalizationMethod(); void writeCanonicalizationTransform(); @@ -224,7 +225,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)); @@ -302,8 +303,8 @@ 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)); } @@ -382,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)); } @@ -390,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)); } @@ -479,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)); } @@ -521,6 +522,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)); + + 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 +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)); - - 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 76460d906c90..fdbd0f614cfb 100644 --- a/xmlsecurity/source/helper/xsecsign.cxx +++ b/xmlsecurity/source/helper/xsecsign.cxx @@ -151,14 +151,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++; }
