svl/source/crypto/cryptosign.cxx | 14 ++++- xmlsecurity/qa/unit/pdfsigning/data/bad-empty-digestalgorithms.pdf |binary xmlsecurity/qa/unit/pdfsigning/data/bad-missing-certificates.pdf |binary xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx | 27 ++++++++++ 4 files changed, 38 insertions(+), 3 deletions(-)
New commits: commit 9d1064fd5008ec1c3689069dac6e4b18e241cbf7 Author: Juraj Šarinay <[email protected]> AuthorDate: Mon May 19 17:52:38 2025 +0200 Commit: Miklos Vajna <[email protected]> CommitDate: Tue May 20 08:35:17 2025 +0200 Fix two crashes when verifying (incomplete) signatures using NSS. Even though SignedData.digestAlgorithms is mandatory in CMS, the set can be empty. One ends up with a valid list containing a single nullptr. According to RFC 5652, one MAY reject signatures without signedData.digestAlgorithms. SignedData.certificates is OPTIONAL in CMS. If omitted, one gets nullptr instead of a list of entries. Rejecting such signatures is apparently OK for adbe.pkcs7.detached and required for ETSI.CAdES.detached. Change-Id: If0270bd3f2d4312947562ab68d2206ae47370b06 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/185540 Tested-by: Jenkins Reviewed-by: Miklos Vajna <[email protected]> diff --git a/svl/source/crypto/cryptosign.cxx b/svl/source/crypto/cryptosign.cxx index 085c00b86b5d..08240d62517e 100644 --- a/svl/source/crypto/cryptosign.cxx +++ b/svl/source/crypto/cryptosign.cxx @@ -1790,8 +1790,10 @@ bool Signing::Verify(const std::vector<unsigned char>& aData, // possible to verify the signature, even if we didn't have the certificate // previously. std::vector<CERTCertificate*> aDocumentCertificates; - for (size_t i = 0; pCMSSignedData->rawCerts[i]; ++i) - aDocumentCertificates.push_back(CERT_NewTempCertificate(CERT_GetDefaultCertDB(), pCMSSignedData->rawCerts[i], nullptr, 0, 0)); + if (auto aCerts = pCMSSignedData->rawCerts) { + while (*aCerts) + aDocumentCertificates.push_back(CERT_NewTempCertificate(CERT_GetDefaultCertDB(), *aCerts++, nullptr, 0, 0)); + } NSSCMSSignerInfo* pCMSSignerInfo = NSS_CMSSignedData_GetSignerInfo(pCMSSignedData, 0); if (!pCMSSignerInfo) @@ -1800,7 +1802,13 @@ bool Signing::Verify(const std::vector<unsigned char>& aData, return false; } - SECItem aAlgorithm = NSS_CMSSignedData_GetDigestAlgs(pCMSSignedData)[0]->algorithm; + auto aDigestAlgs = NSS_CMSSignedData_GetDigestAlgs(pCMSSignedData); + if (!aDigestAlgs || !*aDigestAlgs) { + SAL_WARN("svl.crypto", "ValidateSignature: digestAlgorithms missing"); + return false; + } + + SECItem aAlgorithm = aDigestAlgs[0]->algorithm; SECOidTag eOidTag = SECOID_FindOIDTag(&aAlgorithm); // Map a sign algorithm to a digest algorithm. diff --git a/xmlsecurity/qa/unit/pdfsigning/data/bad-empty-digestalgorithms.pdf b/xmlsecurity/qa/unit/pdfsigning/data/bad-empty-digestalgorithms.pdf new file mode 100644 index 000000000000..4e786ee2a15f Binary files /dev/null and b/xmlsecurity/qa/unit/pdfsigning/data/bad-empty-digestalgorithms.pdf differ diff --git a/xmlsecurity/qa/unit/pdfsigning/data/bad-missing-certificates.pdf b/xmlsecurity/qa/unit/pdfsigning/data/bad-missing-certificates.pdf new file mode 100644 index 000000000000..b614afd3582e Binary files /dev/null and b/xmlsecurity/qa/unit/pdfsigning/data/bad-missing-certificates.pdf differ diff --git a/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx b/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx index 7d176a7e2781..4621c60cd2b2 100644 --- a/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx +++ b/xmlsecurity/qa/unit/pdfsigning/pdfsigning.cxx @@ -661,6 +661,33 @@ CPPUNIT_TEST_FIXTURE(PDFSigningTest, testBadNonDetached) } } +/// Test that we do not crash when processing incomplete signatures +CPPUNIT_TEST_FIXTURE(PDFSigningTest, testIncompleteCMS) +{ + std::shared_ptr<vcl::pdf::PDFium> pPDFium = vcl::pdf::PDFiumLibrary::get(); + if (!pPDFium) + { + return; + } + + const std::initializer_list<std::u16string_view> aNames + = { // empty digestAlgorithms; used to crash when using NSS + u"bad-empty-digestalgorithms.pdf", + // missing certificates; used to crash when using NSS + u"bad-missing-certificates.pdf" + }; + + for (const auto& rName : aNames) + { + std::vector<SignatureInformation> aInfos + = verify(m_directories.getURLFromSrc(DATA_DIRECTORY) + rName, 1); + CPPUNIT_ASSERT(!aInfos.empty()); + SignatureInformation& rInformation = aInfos[0]; + CPPUNIT_ASSERT_EQUAL(xml::crypto::SecurityOperationStatus::SecurityOperationStatus_UNKNOWN, + rInformation.nStatus); + } +} + CPPUNIT_PLUGIN_IMPLEMENT(); /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
