svl/source/crypto/cryptosign.cxx | 54 ++++++++++++++++++++++-----------------
1 file changed, 31 insertions(+), 23 deletions(-)
New commits:
commit 04e774710f10890de651104f1f3ab0e24eb676c3
Author: Juraj Šarinay <ju...@sarinay.com>
AuthorDate: Thu Mar 6 16:44:01 2025 +0100
Commit: Caolán McNamara <caolan.mcnam...@collabora.com>
CommitDate: Tue Mar 18 14:48:16 2025 +0100
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 <vmik...@collabora.com>
Tested-by: Jenkins
(cherry picked from commit 9f687b06fc25156a2a3f4d688b56542612995aa9)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/183078
Reviewed-by: Caolán McNamara <caolan.mcnam...@collabora.com>
diff --git a/svl/source/crypto/cryptosign.cxx b/svl/source/crypto/cryptosign.cxx
index 7b410b38b32d..9031ef40bdd8 100644
--- a/svl/source/crypto/cryptosign.cxx
+++ b/svl/source/crypto/cryptosign.cxx
@@ -2100,23 +2100,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);
@@ -2149,19 +2156,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))
@@ -2237,6 +2246,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.
@@ -2247,19 +2258,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;
