The functions NSS_CMSSignedData_GetDigestValue and NSS_CMSSignedData_GetDigestByAlgTag are duplicates. Neither is exported from the NSS S/MIME library, so we can simply delete one. I think it is better to delete the function NSS_CMSSignedData_GetDigestValue because it is not being used, but use its code because it has better error checking (it tests for a null sigd->digestAlgorithms and a -1 return value from NSS_CMSAlgArray_GetIndexByAlgTag).
Created attachment 136376 [details] [diff] [review] Proposed patch Upon closer inspection, I think it is better to make NSS_CMSSignedData_GetDigestValue the surviving function because it has a corresponding NSS_CMSSignedData_SetDigestValue, which is exported. This patch deletes NSS_CMSSignedData_GetDigestByAlgTag and replaces it by NSS_CMSSignedData_GetDigestValue. Note that NSS_CMSSignedData_GetDigestValue already handles errors correctly. Its only problem is that it doesn't set error code, which I noted with a comment.
Comment on attachment 136376 [details] [diff] [review] Proposed patch The fix isn't quite complete. The caller of NSS_CMSSignedDAta_GetDigestValue is not expecting a failure. Either NSS_CMSSignedData_GetDigestValue or its caller should set SEC_ERROR_DIGEST_NOT_FOUND Other than these two issues, the patch is fine.
Created attachment 136385 [details] [diff] [review] Proposed patch v1.1 This is the same patch, but the code it patches has just changed. It shows that the caller of NSS_CMSSignedData_GetDigestValue is expecting a failure and sets the error code SEC_ERROR_PKCS7_BAD_SIGNATURE. The error code is not what Bob suggested (SEC_ERROR_DIGEST_NOT_FOUND), but that's the subject of Bugscape bug 54057 comments 3 and 4.
Fix checked in on the NSS trunk (NSS 3.9). Checking in cms.h; /cvsroot/mozilla/security/nss/lib/smime/cms.h,v <-- cms.h new revision: 1.18; previous revision: 1.17 done Checking in cmssigdata.c; /cvsroot/mozilla/security/nss/lib/smime/cmssigdata.c,v <-- cmssigdata.c new revision: 1.20; previous revision: 1.19 done
Comment on attachment 136385 [details] [diff] [review] Proposed patch v1.1 r=relyea