Open Bug 663315 Opened 10 years ago Updated 6 years ago

Start accepting SHA-2-based hashes for OCSP response matching (CertID.hashAlgorithm)

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

ASSIGNED
3.15.4

People

(Reporter: briansmith, Assigned: wtc)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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

See ocsp_matchcert. Thinking ahead to the NSA Suite B profile for TLS, we may need to provide an option to libpkix so the application can tell it which hash functions are acceptable for OCSP signatures, CRL signatures, and CA signatures.
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #0)
> See ocsp_matchcert. Thinking ahead to the NSA Suite B profile for TLS, we
> may need to provide an option to libpkix so the application can tell it
> which hash functions are acceptable for OCSP signatures, CRL signatures, and
> CA signatures.

It seems like I was very confused when I filed this bug. The real problem is that we don't support SHA-2 algorithms for matching issuerNameHash or issuerKeyHash. This doesn't have anything to do with the signatures.
Summary: Start accepting SHA-2-based hashes for OCSP & CRL signatures → Start accepting SHA-2-based hashes for OCSP response matching (CertID.hashAlgorithm)
Assignee: nobody → brian
Status: NEW → ASSIGNED
Attachment #8338870 - Flags: review?(ryan.sleevi)
Comment on attachment 8338870 [details] [diff] [review]
replace-md5-and-md2-with-sha2.patch

Review of attachment 8338870 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, wrong patch.
Attachment #8338870 - Flags: review?(ryan.sleevi)
Attachment #8338870 - Attachment is obsolete: true
Comment on attachment 8338870 [details] [diff] [review]
replace-md5-and-md2-with-sha2.patch

Review of attachment 8338870 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/certhigh/ocsp.c
@@ +3881,1 @@
>      item.len = SHA1_LENGTH;

BUG: Because item.len == SHA1_LENGTH, the calls to CERT_GetSPKIDigest will fail for SEC_OID_SHA256 and SEC_OID_SHA384.

This is because ocsp_digestValue will result in SEC_ERROR_INVALID_ARGS, since fill->len < digestObject->length.

I don't think this code was working for MD5 / MD2 in the past, because fill->len was never updated, thus SECITEM_ItemsAreEqual would fail.
Attachment #8338870 - Flags: review-
I didn't bother adding SHA-384 because I think it is unlikely
to be used.
Assignee: brian → wtc
Attachment #8345421 - Flags: superreview?(brian)
Attachment #8345421 - Flags: review?(ryan.sleevi)
Priority: -- → P2
Target Milestone: --- → 3.15.4
Comment on attachment 8345421 [details] [diff] [review]
Replace MD2 and MD5 with SHA-256

Review of attachment 8345421 [details] [diff] [review]:
-----------------------------------------------------------------

The patch writen so far looks reasonable to me.

This is the type of thing that I was talking about in the NSS meeting today:

    static PRIntn PR_CALLBACK
    ocsp_CacheKeyCompareFunction(const void *v1, const void *v2)
    {
        CERTOCSPCertID *cid1 = (CERTOCSPCertID *)v1;
        CERTOCSPCertID *cid2 = (CERTOCSPCertID *)v2;
  
        return (SECEqual == SECITEM_CompareItem(&cid1->issuerNameHash, 
                                                &cid2->issuerNameHash)
                && SECEqual == SECITEM_CompareItem(&cid1->issuerKeyHash, 
                                                   &cid2->issuerKeyHash)
                && SECEqual == SECITEM_CompareItem(&cid1->serialNumber, 
                                                   &cid2->serialNumber));
    }

Presumably one reason for wanting to support SHA-2 is that it is more collision-resistant. But, here, we're using issuerNameHash/issuerKeyHash which are SHA-1 hashes, even if the OCSP response used SHA-2. IMO, it would be more correct to make issuerNameHash and issuerKeyHash SHA-2.

Also, I understand that you expect that nobody will use SHA-384. However, I would expect that if a server is using the TLS Suite B profile at the 256-bit security level, then they may (must?) use SHA-384. Since we don't support the TLS Suite B profile at the 256-bit security level, then we can defer this for now. However, it seems like it might be more straightforward to just add the SHA-384 support now since SHA-256/SHA-384 would directly mirror the MD5/MD2 support it is replacing. That is, it might be harder to remember out how how to add SHA-384 support later. If we don't add SHA-384 support now, please file a follow-up bug.

I am concerned that, since the previous MD5/MD2 didn't seem to run, and we don't know of any SHA-2-generating OCSP servers, that we don't really know if this patch works. I think it should be pretty straightforward to modify our OCSP tests (either the fetching ones, or the stapling ones, if not both) to test the SHA-2 functionality, and I think it would be useful to do so. Note, in particular, that normal usage of the OCSP in NSS has issuerNameHash == issuerSHA1NameHash == the hash actually used, but with this SHA-2 support, as it is currently written, that is not the case.

Note that some OCSP responders may not give us SHA-2-based OCSP responses until we add the PreferredSignatureAlgorithms extension to our OCSP requests. I already filed bug 943651 for that. Also, SSL servers may not give us stapled SHA-2-based OCSP responses unless we add the PreferredSignatureAlgorithms extension to the cert_status extension in the SSL handshake. I just filed bug 949918 about adding that extension to the ssl status_request (OCSP stapling) extension. This bug can be fixed independently of those bugs, of course.
Attachment #8345421 - Flags: superreview?(brian) → superreview-
Comment on attachment 8345421 [details] [diff] [review]
Replace MD2 and MD5 with SHA-256

Review of attachment 8345421 [details] [diff] [review]:
-----------------------------------------------------------------

Implementation, this R+ to me.

I've read Brian's comment several times, and I fail to see what exactly the superreview- was for.
Attachment #8345421 - Flags: review?(ryan.sleevi) → review+
(In reply to Ryan Sleevi from comment #7)
> I've read Brian's comment several times, and I fail to see what exactly the
> superreview- was for.

AFAICT, this new SHA-2 support has never been tested, even manually. So, how do we know it works?
No longer blocks: 942515
You need to log in before you can comment on or make changes to this bug.