diff options
author | Tomaž Vajngerl <tomaz.vajngerl@collabora.co.uk> | 2021-11-01 21:46:43 +0100 |
---|---|---|
committer | Tomaž Vajngerl <quikee@gmail.com> | 2021-11-02 14:05:50 +0100 |
commit | f2e1e4ff085962a08a5d7738325b383c07afcbbd (patch) | |
tree | a82c2fcafc23dd202749dc52d211f97e98f983ef /xmlsecurity | |
parent | 1076f634c19527770a2e465ea0d8c561c8491f26 (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>
Diffstat (limited to 'xmlsecurity')
-rw-r--r-- | xmlsecurity/qa/unit/signing/signing.cxx | 209 | ||||
-rw-r--r-- | xmlsecurity/source/helper/ooxmlsecexporter.cxx | 53 | ||||
-rw-r--r-- | xmlsecurity/source/helper/xsecsign.cxx | 10 |
3 files changed, 153 insertions, 119 deletions
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++; } |