915334 - NSS cannot create OCSP responses with embedded certificates

Status: RESOLVED WONTFIX

(NSS :: Libraries, enhancement, P2)

Description

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

The OCSP response generation code in ocspsig.c hard-codes an empty sequence of certificates in the responses it generates. This means that that code cannot be used to generate responses for tests of validation of OCSP responses signed by delegated OCSP responders.

In the patch, note that the certificates are given as SECItem* instead of CERTCertificate* because we want to be able to test inputs that cannot even be parsed into CERTCertificate. Also, we avoid verifying that the passed-in certificates are valid for the given OCSP response because want to be able to embed arbitrary certificates, not just certificates that are valid for the OCSP response.

The original ASN.1 template in ocspsig.c seemed to be incorrect. I copied the template used for certificates in the CMS code to fix it.

Comment 1

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

Attachment: ocspsig-1.patch

Comment 2

[Ryan Sleevi]

Comment on attachment 803223 [details] [diff] [review]
ocspsig-1.patch

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

::: lib/certhigh/ocspsig.c
@@ -251,5 @@
>          offsetof(ocspBasicOCSPResponse, responseSignature.signature) },
>      { SEC_ASN1_OPTIONAL | SEC_ASN1_EXPLICIT |
> -      SEC_ASN1_CONSTRUCTED | SEC_ASN1_CONTEXT_SPECIFIC | 0,
> -        offsetof(ocspBasicOCSPResponse, responseSignature.derCerts),
> -        mySEC_PointerToSequenceOfAnyTemplate },

Seems like you can/should remove this variable (line 143 - http://mxr.mozilla.org/nss/source/lib/certhigh/ocspsig.c#143 )

@@ +365,5 @@
>      CERTCertificate *responderCert,
>      CERTOCSPResponderIDType responderIDType,
>      PRTime producedAt,
>      CERTOCSPSingleResponse **responses,
> +    SECItem ** derCerts,

STYLE: As far as ordering goes, shouldn't this be placed immediately following responderCert, since this is about the responder cert chain.

DESIGN: Taking both a "CERTCertificate" and a set of derCerts (that chain from the CERTCertificate) seems a very... inconsistent... design.

Would it make more sense to take a CERTCertList / CERTCertificateList?

Alternatively, if you're concerned about the overhead involved with parsing the CERTCertificates (which I think would be a premature optimization), then having *all* the certificates be input as SECItems seems more consistent.

Regardless, the inconsistency here leaves me a little uncomfortable.

@@ +560,5 @@
> +    return CERT_CreateEncodedOCSPSuccessResponseWithCerts(arena,
> +                                                          responderCert,
> +                                                          responderIDType,
> +                                                          producedAt,
> +                                                          responses, derCerts,

STYLE: I would suggest you put |derCerts| on a new line, if only because things are already wrapping on my default display (and it seems consistent with the rest of the call, rather than trying to shoe horn variables around any time a name changes)

Comment 3

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 4

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

For Gecko, David Keeler is creating a more flexible way for us to generate test OCSP responses in bug 974715. So, we don't need this any more. Thanks for the review and for pointing out the limitations and confusing nature of my patch.