Closed Bug 663313 Opened 9 years ago Closed 7 years ago

Stop accepting MD5- and MD2- based OCSP signatures


(NSS :: Libraries, defect, P2)



(Not tracked)



(Reporter: briansmith, Assigned: ryan.sleevi)




(1 file, 1 obsolete file)

+++ This bug was initially created as a clone of Bug #590364 +++

See ocsp_matchcert:

    if (CERT_GetSPKIDigest(NULL,testCert,SEC_OID_MD5, &item) == NULL) {
	return PR_FALSE;
    if  (SECITEM_ItemsAreEqual(certIndex,&item)) {
	return PR_TRUE;
    if (CERT_GetSPKIDigest(NULL,testCert,SEC_OID_MD2, &item) == NULL) {
	return PR_FALSE;
    if  (SECITEM_ItemsAreEqual(certIndex,&item)) {
	return PR_TRUE;
See also ocsp_CertIDsMatch:

    case SEC_OID_MD5:
	keyHash = &requestCertID->issuerMD5KeyHash;
	nameHash = &requestCertID->issuerMD5NameHash;
    case SEC_OID_MD2:
	keyHash = &requestCertID->issuerMD2KeyHash;
	nameHash = &requestCertID->issuerMD2NameHash;

Probably there are other locations as well. I won't comment on every one.
Assignee: bsmith → nobody
Assignee: nobody → kaie
whoops. changed wrong bug. But I believe this is probably soon a duplicated of bug 590364
Assignee: kaie → nobody
No longer blocks: 650355
Hashing is used for two purposes in OCSP; one part is the signature, which is security-critical, and another part is in the certificate ID, which is (AFAICT) less security-critical. AFAICT, in practice everybody uses SHA-1* for the certificate ID part, so we should be able to remove the MD5- and MD2- support for that.

* Though some people have asked us to add SHA-2 support, which is probably required for Suite B.
Duplicate of this bug: 904757
Assignee: nobody → ryan.sleevi
Target Milestone: --- → 3.15.2
Attached patch patch1.diff (obsolete) — Splinter Review

Per the call today, this unifies the OCSP signature checking code with the CERT/CRL checking code by marshalling the OCSP signature (ocspSignature) into a CERTSignedData structure, then using CERT_VerifySignedDataWithPublicKey(), which does the algorithm policy checking.
Attachment #790966 - Flags: review?(rrelyea)
Attachment #790966 - Flags: feedback?(brian)
Comment on attachment 790966 [details] [diff] [review]


Thanks ryan..

I do have a style issue: In general we always put the braces around the if's even if there is only one line. It's sort of a safety issue (it's relatively easy to add a second line in the if and forget to add the braces).

I only noticed because you specifically removed existing braces in the code...

Attachment #790966 - Flags: review?(rrelyea) → review+
Attached patch patch2.diffSplinter Review
patch1 was just trying to unify the brace style within the function (which had a mix of both braced and braceless one-lines). It also normalized (within the function) the tabs vs spaces, since the lines were being altered.

patch2 just switches the brace normalization to always include, per your style request.
Attachment #790966 - Attachment is obsolete: true
Attachment #790966 - Flags: feedback?(brian)
Attachment #790968 - Flags: review?(rrelyea)
Comment on attachment 790968 [details] [diff] [review]

Checked in as
Attachment #790968 - Flags: checked-in+
Closed: 7 years ago
Resolution: --- → FIXED
Priority: -- → P2
Attachment #790968 - Flags: review+
Attachment #790968 - Flags: review?(rrelyea)
You need to log in before you can comment on or make changes to this bug.