The default bug view has changed. See this FAQ.

Stop accepting MD5- and MD2- based OCSP signatures

RESOLVED FIXED in 3.15.2

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: briansmith, Assigned: Ryan Sleevi)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.21 KB, patch
Wan-Teh Chang
: review+
Ryan Sleevi
: checked-in+
Details | Diff | Splinter Review
+++ 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;
	break;
    case SEC_OID_MD2:
	keyHash = &requestCertID->issuerMD2KeyHash;
	nameHash = &requestCertID->issuerMD2NameHash;
	break;

Probably there are other locations as well. I won't comment on every one.
Assignee: bsmith → nobody

Updated

5 years ago
Assignee: nobody → kaie

Comment 2

5 years ago
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.
(Assignee)

Updated

4 years ago
Duplicate of this bug: 904757
(Assignee)

Updated

4 years ago
Assignee: nobody → ryan.sleevi
Target Milestone: --- → 3.15.2
(Assignee)

Comment 5

4 years ago
Created attachment 790966 [details] [diff] [review]
patch1.diff

Bob,

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 6

4 years ago
Comment on attachment 790966 [details] [diff] [review]
patch1.diff

r+

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...

bob
Attachment #790966 - Flags: review?(rrelyea) → review+
(Assignee)

Comment 7

4 years ago
Created attachment 790968 [details] [diff] [review]
patch2.diff

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)
(Assignee)

Comment 8

4 years ago
Comment on attachment 790968 [details] [diff] [review]
patch2.diff

Checked in as https://hg.mozilla.org/projects/nss/rev/57582eec5eeb
Attachment #790968 - Flags: checked-in+
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Priority: -- → P2

Updated

4 years ago
Attachment #790968 - Flags: review+
(Assignee)

Updated

4 years ago
Attachment #790968 - Flags: review?(rrelyea)
You need to log in before you can comment on or make changes to this bug.