Last Comment Bug 663313 - Stop accepting MD5- and MD2- based OCSP signatures
: Stop accepting MD5- and MD2- based OCSP signatures
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: trunk
: All All
P2 normal with 2 votes (vote)
: 3.15.2
Assigned To: Ryan Sleevi
: 904757 (view as bug list)
Depends on:
Blocks: 590364
  Show dependency treegraph
Reported: 2011-06-09 21:49 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2013-09-26 15:08 PDT (History)
18 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

patch1.diff (2.31 KB, patch)
2013-08-15 14:43 PDT, Ryan Sleevi
rrelyea: review+
Details | Diff | Splinter Review
patch2.diff (2.21 KB, patch)
2013-08-15 14:53 PDT, Ryan Sleevi
wtc: review+
ryan.sleevi: checked‑in+
Details | Diff | Splinter Review

Description User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-06-09 21:49:47 PDT
+++ 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 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-06-10 14:16:22 PDT
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.
Comment 2 User image Kai Engert (:kaie) 2012-03-01 14:09:51 PST
whoops. changed wrong bug. But I believe this is probably soon a duplicated of bug 590364
Comment 3 User image Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2012-07-26 12:42:37 PDT
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 4 User image Ryan Sleevi 2013-08-13 16:54:18 PDT
*** Bug 904757 has been marked as a duplicate of this bug. ***
Comment 5 User image Ryan Sleevi 2013-08-15 14:43:07 PDT
Created attachment 790966 [details] [diff] [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.
Comment 6 User image Robert Relyea 2013-08-15 14:49:07 PDT
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...

Comment 7 User image Ryan Sleevi 2013-08-15 14:53:22 PDT
Created attachment 790968 [details] [diff] [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.
Comment 8 User image Ryan Sleevi 2013-08-15 15:03:51 PDT
Comment on attachment 790968 [details] [diff] [review]

Checked in as

Note You need to log in before you can comment on or make changes to this bug.