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-on: https://gerrit.libreoffice.org/c/core/+/183079
Tested-by: Xisco Fauli <xiscofauli@libreoffice.org>
Reviewed-by: Xisco Fauli <xiscofauli@libreoffice.org>
(cherry picked from commit d4158cb720099c7554a8ae26a7998faeae0500f9)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/183685
Tested-by: allotropia jenkins <jenkins@allotropia.de>
Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>

1 files changed, 31 insertions, 23 deletions

diff --git a/svl/source/crypto/cryptosign.cxx b/svl/source/crypto/cryptosign.cxx
index 1b882bb89deb..048b5a8af0ac 100644
--- a/svl/source/crypto/cryptosign.cxx
+++ b/svl/source/crypto/cryptosign.cxx
@@ -2154,23 +2154,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);
@@ -2201,19 +2208,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))
@@ -2289,6 +2298,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.
@@ -2299,19 +2310,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;