[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)

Gbp-Pq: Name Improve-adbe.pkcs7.sha1-signature-verification.diff

diff --git a/svl/source/crypto/cryptosign.cxx b/svl/source/crypto/cryptosign.cxx
index 1d63378455690064db302110ca50bb802a4fd315..d59569bd9b22884d28cccfbb6cf8c8d3c9db05f0 100644 (file)
--- a/svl/source/crypto/cryptosign.cxx
+++ b/svl/source/crypto/cryptosign.cxx
@@ -2085,23 +2085,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
     SECITEM_FreeItem(&aOidData.oid, false);
     PORT_Free(pActualResultBuffer);
@@ -2134,19 +2141,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))
@@ -2222,6 +2231,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.
@@ -2232,19 +2243,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;