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

Status: NEW

(NSS :: Libraries, defect, P3)

Description

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

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

Comment 1

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

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

Comment 2

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

Attachment: replace-md5-and-md2-with-sha2.patch

Comment 3

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

Comment on attachment 8338870 [details] [diff] [review]
replace-md5-and-md2-with-sha2.patch

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

Sorry, wrong patch.

Comment 4

[Ryan Sleevi]

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.

Comment 5

[Wan-Teh Chang]

Attachment: Replace MD2 and MD5 with SHA-256

I didn't bother adding SHA-384 because I think it is unlikely
to be used.

Comment 6

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

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.

Comment 7

[Ryan Sleevi]

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.

Comment 8

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

(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?

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last months and this bug has priority 'P2'.
:beurdouche, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 10 votes.
:beurdouche, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 11

[Kathleen Wilson]

In case it is relevant to this bug:

https://www.mozilla.org/en-US/about/governance/policies/security-group/certs/policy#513-sha-1

July 1, 2023: CAs SHALL NOT sign SHA-1 hashes over certificates with an EKU extension containing the id-kp-ocspSigning key purpose; intermediate certificates that chain up to roots in Mozilla's program; OCSP responses; or CRLs.

Comment 12

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.