663313 - Stop accepting MD5- and MD2- based OCSP signatures

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

+++ 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;
    }

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 2

[Kai Engert (:KaiE:)]

whoops. changed wrong bug. But I believe this is probably soon a duplicated of bug 590364

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 5

[Ryan Sleevi]

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

Comment 6

[Robert Relyea]

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

Comment 7

[Ryan Sleevi]

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

Comment 8

[Ryan Sleevi]

Comment on attachment 790968 [details] [diff] [review]
patch2.diff

Checked in as https://hg.mozilla.org/projects/nss/rev/57582eec5eeb