[PATCH] Improve adbe.pkcs7.sha1 signature verification

For PDF signatures with SubFilter == adbe.pkcs7.sha1, we only
compared hash values and never actually checked SignatureValue
within SignerInfo.

Fix bugs introduced by 055fd58711d57af4d96214aebd71b713303d5527 and
e58ed17e35989350afe3e9fd77b24515df782eac by verifying the actual
(public-key) signature after the hash values compare equal.

Change-Id: I5fa3d60df214cc5efedd1c0eba6cf1b9faf05360
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/183059
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins
(cherry picked from commit 9f687b06fc25156a2a3f4d688b56542612995aa9)

Reviewed-By: Daniel Leidert <dleidert@debian.org>
Origin: https://git.libreoffice.org/core/+/9f687b06fc25156a2a3f4d688b56542612995aa9%5E%21
Bug: https://www.libreoffice.org/about-us/security/advisories/cve-2025-2866
Bug: https://github.com/advisories/GHSA-22mj-r7hq-f9h2
Bug-Debian-Security: https://security-tracker.debian.org/tracker/CVE-2025-2866
Bug-Freexian-Security: https://deb.freexian.com/extended-lts/tracker/CVE-2025-2866

Gbp-Pq: Name CVE-2025-2866.patch

diff --git a/svl/source/crypto/cryptosign.cxx b/svl/source/crypto/cryptosign.cxx
index a015f00576a44ad4bc380c750bb2693a9fba4f39..1ab990bbfb7bf4f5486dd90ebda3e2348c1a12d6 100644 (file)
--- a/svl/source/crypto/cryptosign.cxx
+++ b/svl/source/crypto/cryptosign.cxx
@@ -2151,23 +2151,30 @@ bool Signing::Verify(const std::vector<unsigned char>& aData,
     if (pAttribute)
         rInformation.bHasSigningCertificate = true;
 
+    SECItem aSignedDigestItem {siBuffer, nullptr, 0};
+
     SECItem* pContentInfoContentData = pCMSSignedData->contentInfo.content.data;
     if (bNonDetached && pContentInfoContentData && pContentInfoContentData->data)
     {
         // Not a detached signature.
-        if (!std::memcmp(pActualResultBuffer, pContentInfoContentData->data, nMaxResultLen) && nActualResultLen == pContentInfoContentData->len)
-            rInformation.nStatus = xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED;
+        if (nActualResultLen == pContentInfoContentData->len &&
+            !std::memcmp(pActualResultBuffer, pContentInfoContentData->data, nMaxResultLen) &&
+            HASH_HashBuf(eHashType, pActualResultBuffer, pContentInfoContentData->data, nActualResultLen) == SECSuccess)
+        {
+            aSignedDigestItem.data = pActualResultBuffer;
+            aSignedDigestItem.len = nActualResultLen;
+        }
     }
     else
     {
         // Detached, the usual case.
-        SECItem aActualResultItem;
-        aActualResultItem.data = pActualResultBuffer;
-        aActualResultItem.len = nActualResultLen;
-        if (NSS_CMSSignerInfo_Verify(pCMSSignerInfo, &aActualResultItem, nullptr) == SECSuccess)
-            rInformation.nStatus = xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED;
+        aSignedDigestItem.data = pActualResultBuffer;
+        aSignedDigestItem.len = nActualResultLen;
     }
 
+    if (aSignedDigestItem.data && NSS_CMSSignerInfo_Verify(pCMSSignerInfo, &aSignedDigestItem, nullptr) == SECSuccess)
+            rInformation.nStatus = xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED;
+
     // Everything went fine
     PORT_Free(pActualResultBuffer);
     HASH_Destroy(pHASHContext);
@@ -2198,19 +2205,21 @@ bool Signing::Verify(const std::vector<unsigned char>& aData,
         return false;
     }
 
-    // Update the message with the content blob.
-    if (!CryptMsgUpdate(hMsg, aData.data(), aData.size(), FALSE))
+    if (!bNonDetached)
     {
-        SAL_WARN("svl.crypto", "ValidateSignature, CryptMsgUpdate() for the content failed: " << WindowsErrorString(GetLastError()));
-        return false;
-    }
+        // Update the message with the content blob.
+        if (!CryptMsgUpdate(hMsg, aData.data(), aData.size(), FALSE))
+        {
+            SAL_WARN("svl.crypto", "ValidateSignature, CryptMsgUpdate() for the content failed: " << WindowsErrorString(GetLastError()));
+            return false;
+        }
 
-    if (!CryptMsgUpdate(hMsg, nullptr, 0, TRUE))
-    {
-        SAL_WARN("svl.crypto", "ValidateSignature, CryptMsgUpdate() for the last content failed: " << WindowsErrorString(GetLastError()));
-        return false;
+        if (!CryptMsgUpdate(hMsg, nullptr, 0, TRUE))
+        {
+            SAL_WARN("svl.crypto", "ValidateSignature, CryptMsgUpdate() for the last content failed: " << WindowsErrorString(GetLastError()));
+            return false;
+        }
     }
-
     // Get the CRYPT_ALGORITHM_IDENTIFIER from the message.
     DWORD nDigestID = 0;
     if (!CryptMsgGetParam(hMsg, CMSG_SIGNER_HASH_ALGORITHM_PARAM, 0, nullptr, &nDigestID))
@@ -2286,6 +2295,8 @@ bool Signing::Verify(const std::vector<unsigned char>& aData,
         rInformation.X509Datas.emplace_back(temp);
     }
 
+    std::vector<BYTE> aContentParam;
+
     if (bNonDetached)
     {
         // Not a detached signature.
@@ -2296,19 +2307,16 @@ bool Signing::Verify(const std::vector<unsigned char>& aData,
             return false;
         }
 
-        std::vector<BYTE> aContentParam(nContentParam);
+        aContentParam.resize(nContentParam);
         if (!CryptMsgGetParam(hMsg, CMSG_CONTENT_PARAM, 0, aContentParam.data(), &nContentParam))
         {
             SAL_WARN("svl.crypto", "ValidateSignature: CryptMsgGetParam() failed");
             return false;
         }
-
-        if (VerifyNonDetachedSignature(aData, aContentParam))
-            rInformation.nStatus = xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED;
     }
-    else
+
+    if (!bNonDetached || VerifyNonDetachedSignature(aData, aContentParam))
     {
-        // Detached, the usual case.
         // Use the CERT_INFO from the signer certificate to verify the signature.
         if (CryptMsgControl(hMsg, 0, CMSG_CTRL_VERIFY_SIGNATURE, pSignerCertContext->pCertInfo))
             rInformation.nStatus = xml::crypto::SecurityOperationStatus_OPERATION_SUCCEEDED;