Closed Bug 915334 Opened 7 years ago Closed 7 years ago

NSS cannot create OCSP responses with embedded certificates

Categories

(NSS :: Libraries, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: briansmith, Unassigned)

Details

Attachments

(1 file)

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.
Attached patch ocspsig-1.patchSplinter Review
Attachment #803223 - Flags: review?(rrelyea)
Assignee: nobody → brian
Priority: -- → P1
Blocks: 915931
Status: NEW → ASSIGNED
Target Milestone: --- → 3.15.3
Attachment #803223 - Flags: review?(ryan.sleevi)
Priority: P1 → P2
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)
Attachment #803223 - Flags: review?(ryan.sleevi) → review-
changing target milestone to 3.15.4
Target Milestone: 3.15.3 → 3.15.4
Target Milestone: 3.15.4 → 3.16
Version: 3.15.2 → trunk
No longer blocks: 915931
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.
Assignee: brian → nobody
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Target Milestone: 3.16 → ---
You need to log in before you can comment on or make changes to this bug.