915931 - Add OCSP support to insanity::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

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

Attachment: Tests #1: Error responses

Comment 2

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

Attachment: Tests #2: Successful responses signed directly by the issuer

Comment 3

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

Attachment: Tests #3: OCSP responses signed by a missing delegated OCSP responder

Comment 4

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

Attachment: Tests #4: Responses signed by trusted delegated OCSP response signer

Comment 5

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

Attachment: Tests #5: Responses signed by a delegated response signer with the wrong EKU

Comment 6

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

Attachment: Tests #6: Responses signed by a delegated OCSP response signer with no EKU

Comment 7

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

Attachment: Tests #7: Test that delegated OCSP responder certs must be directly issued by the CA that issued the cert

Comment 8

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

Attachment: Tests #8: Test that signature of OCSP response signer cert is verified

Comment 9

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

Attachment: Tests #9: Test that actively distructed OCSP signer certs are rejected

Comment 10

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

Attachment: Tests #10: Test OCSP signer certs outside notBefore-notAfter validity period are rejected

Comment 11

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

Still need to test:

* thisUpdate/nextUpdate logic.
* VerifyEncodedOCSPResponse returns SEC_ERROR_OCSP_MALFORMED_RESPONSE and not SEC_ERROR_BAD_DER on syntax error.
* Mismatched CertID in SingleResponse is handled correctly.

Comment 12

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

Attachment: Part 1: Add OCSP response parsing & validation to insanity::pkix

Comment 13

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

Attachment: Part 2: Add OCSP request encoding to insanity::pkix

Comment 14

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

Attachment: Part 3: Integrate insanity::pkix OCSP support into CertVerifier

Comment 15

[Camilo Viecco (:cviecco)]

Comment on attachment 8373835 [details] [diff] [review]
Part 3: Integrate insanity::pkix OCSP support into CertVerifier

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

Almost r+ for me. (once I figure out the .h thingy).

::: security/certverifier/CertVerifier.cpp
@@ +220,5 @@
>  
>  SECStatus
>  CertVerifier::InsanityVerifyCert(
>                     CERTCertificate* cert,
> +      /*optional*/ const SECItem* stapledOCSPResponse,

put it at the end with a default null value. And where did you modified the .h?

Comment 16

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8373831 [details] [diff] [review]
Part 1: Add OCSP response parsing & validation to insanity::pkix

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

Ok - this took a while to understand, but it actually is well architected. I basically have a bunch of nits, but I would like to see it again after those are addressed.

::: security/insanity/include/insanity/pkix.h
@@ +37,5 @@
>  SECStatus VerifySignedData(const CERTSignedData* sd,
>                             const CERTCertificate* cert,
>                             void* pkcs11PinArg);
>  
> +SECItem * CreateEncodedOCSPRequest(PLArenaPool* arena,

nit: SECItem*
Also, looks like this isn't implemented yet?

::: security/insanity/lib/pkixcheck.cpp
@@ +352,5 @@
> +  }
> +
> +  // TODO: Is this correct?
> +  bool isTrustAnchor = endEntityOrCA == MustBeCA &&
> +                       trustLevel == TrustDomain::TrustAnchor;

If we want to disallow end-entity certificates from being trust anchors, then yes, this looks correct. We would have to be consistent about this, though, and never treat an end-entity certificate as a trust anchor elsewhere.

::: security/insanity/lib/pkixcheck.h
@@ +29,5 @@
> +          PRTime time,
> +          EndEntityOrCA endEntityOrCA,
> +          KeyUsages requiredKeyUsagesIfPresent,
> +          SECOidTag requiredEKUIfPresent,
> +          unsigned int depth,

Looks like depth is called subCACount in pkixcheck.cpp, which I think is more descriptive. I guess it was always this way - my bad for not catching it when it landed initially.

::: security/insanity/lib/pkixocsp.cpp
@@ +14,5 @@
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include "insanity/bind.h"

nit: does this need to be before <limits>? (I would put it with the other insanity/pkix includes)

@@ +67,5 @@
> +  TrustDomain& trustDomain;
> +  const CERTCertificate& cert;
> +  CERTCertificate& issuerCert;
> +  const PRTime time;
> +  CertStatus certStatus;

nit: for me, it's definitely helpful to have member variables prefixed with 'm' (so mTrustDomain, etc.)

@@ +77,5 @@
> +
> +static Result
> +CheckOCSPResponseSignerCert(TrustDomain& trustDomain,
> +                            CERTCertificate& potentialSigner,
> +                            const CERTCertificate& issuerCert, PRTime time)

Documentation from rfc 6960 section 4.2.2.2. Authorized Responders (e.g. that we're checking for a delegated signer, that the issuer issued this signer, that we trust the issuer because of where we are in the verification process, etc.)

@@ +113,5 @@
> +  // It is possible that there exists a certificate with the same key as the
> +  // issuer but with a different name, so we need to compare names
> +  // TODO: needs test
> +  if (!SECITEM_ItemsAreEqual(&cert.GetNSSCert()->derIssuer,
> +                            &issuerCert.derSubject) &&

nit: indentation

@@ +163,5 @@
> +                                   /*out*/ bool& match);
> +
> +// TODO: const
> +static CERTCertificate*
> +GetOCSPSignerCertificate(TrustDomain& trustDomain,

some documentation from rfc 6960 section 4.2.2.2. Authorized Responders (e.g. that the response may be directly issued from the issuer certificate, or that it may be from a delegated cert)

@@ +169,5 @@
> +                         const SECItem& responderIDItem,
> +                         const SECItem* certs, size_t numCerts,
> +                         CERTCertificate& issuerCert, PRTime time)
> +{
> +  // TODO: maybe these assertions need to be always-on runtiume checks?

what assertions?

@@ +171,5 @@
> +                         CERTCertificate& issuerCert, PRTime time)
> +{
> +  // TODO: maybe these assertions need to be always-on runtiume checks?
> +
> +  // TODO: we need an OCSP test that uses delegated responder to catch this bug!

what bug?

@@ +285,5 @@
> +
> +  return SECSuccess;
> +}
> +
> +// TODO: Remove arena parameter

I don't see an arena parameter here

@@ +298,5 @@
> +  // TODO: PR_Assert(pinArg)
> +  PR_ASSERT(encodedResponse);
> +  if (!cert || !issuerCert) {
> +      PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
> +      return SECFailure;

nit: indentation on this line and the one above

@@ +301,5 @@
> +      PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
> +      return SECFailure;
> +  }
> +
> +  if (encodedResponse->len > 0xffff) {

This check is also done by input.Init, right? I think we should rely on the error checking der::Input, etc. does so as to not obfuscate the logic here.

@@ +314,5 @@
> +
> +  Context context(trustDomain, *cert, *issuerCert, time);
> +
> +  if (der::Nested(input, der::SEQUENCE,
> +                   bind(OCSPResponse, _1, ref(context))) != der::Success) {

nit: indentation (move the second line to the left 1 space)

@@ +340,5 @@
> +}
> +
> +// OCSP
> +#define ID_PKIX_OCSP 0x2B, 0x06, 0x01, 0x05, 0x05, 0x07, 0x30, 0x01
> +static const uint8_t id_pkix_ocsp_basic[] = { ID_PKIX_OCSP, 0x01 };

I think this should go closer to where it's used.

@@ +344,5 @@
> +static const uint8_t id_pkix_ocsp_basic[] = { ID_PKIX_OCSP, 0x01 };
> +
> +// OCSPResponse ::= SEQUENCE {
> +//       responseStatus         OCSPResponseStatus,
> +//       responseBytes          [0] EXPLICIT ResponseBytes OPTIONAL }

These comments are great, but it seems like they're inconsistent regarding where the ending "}" goes - I would prefer the style OCSPResponseStatus uses, a few lines below.

@@ +391,5 @@
> +  return der::Nested(input, der::OCTET_STRING, der::SEQUENCE,
> +                     bind(BasicResponse, _1, ref(context)));
> +}
> +
> +// BasicOCSPResponse       ::= SEQUENCE {

It also doesn't look like where the "::=" goes is consistent.

@@ +409,5 @@
> +  }
> +
> +  der::Input tbsResponseData;
> +
> +  // TODO: maybe we don't need 3-arg Skip anymore?

The 3-argument der::Skip is still being used. There isn't a 3-argument der::Input::Skip that I could find.

@@ +416,5 @@
> +  }
> +
> +  CERTSignedData signedData;
> +
> +  input.GetSECItem(siBuffer, mark, signedData.data);

This confused me for a while (and maybe I still don't get it). What I'm reading is that the signature covers the entire DER encoding of tbsResponseData, including the beginning tag and length. However, when we're parsing tbsResponseData, we want to strip off the tag and length because we don't need it after we've confirmed it's there and figured out what length it is. Is this correct? If so, I would appreciate a comment to this effect.

@@ +420,5 @@
> +  input.GetSECItem(siBuffer, mark, signedData.data);
> +
> +  if (der::Nested(input, der::SEQUENCE,
> +                   bind(der::AlgorithmIdentifier, _1,
> +                             ref(signedData.signatureAlgorithm)))

nit: indentation on this line and the one above

@@ +428,5 @@
> +
> +  if (der::Skip(input, der::BIT_STRING, signedData.signature) != der::Success) {
> +    return der::Failure;
> +  }
> +  // strip "number of bits to skip" byte

This comment is unclear. a) it's the number of unused bits at the end, right? b) we're expecting it to be 0.

@@ +431,5 @@
> +  }
> +  // strip "number of bits to skip" byte
> +  if (signedData.signature.len == 0 ||
> +      signedData.signature.data[0] != 0) {
> +    return der::Fail(SEC_ERROR_BAD_KEY);

maybe SEC_ERROR_BAD_SIGNATURE or SEC_ERROR_OCSP_BAD_SIGNATURE?

@@ +446,5 @@
> +  if (!input.AtEnd()) {
> +    // [0] wrapper
> +    if (der::ExpectTagAndGetLength(input,
> +                                   der::CONSTRUCTED | der::CONTEXT_SPECIFIC | 0,
> +                                   length) != der::Success) {

It seems like we're not using length here. Maybe there should be a der::ExpectTagAndIgnoreLength?

@@ +463,5 @@
> +        return der::Fail(SEC_ERROR_BAD_DER);
> +      }
> +
> +      der::Input::Mark mark(input.GetMark());
> +      if (der::Skip(input, der::SEQUENCE) != der::Success) {

Skipping this SEQUENCE tag seemed unnecessary to me, but from investigating some DER encodings of OCSP responses, I can see that it's there. Is this just part of how a SEQUENCE of certificates is encoded? Either way, I'd appreciate a short explanation of what's going on here in a comment.

@@ +467,5 @@
> +      if (der::Skip(input, der::SEQUENCE) != der::Success) {
> +        return der::Failure;
> +      }
> +
> +      input.GetSECItem(siBuffer, mark, certs[numCerts]);

It seems like you're doing Mark, Skip, GetSECItem a lot (well, at least twice). Should this be a utility in pkixder.h?

@@ +486,5 @@
> +ResponseData(der::Input& input, Context& context,
> +             const CERTSignedData& signedResponseData,
> +             /*const*/ SECItem* certs, size_t numCerts)
> +{
> +  // TODO: check status

Don't you do this below? (in SingleResponse)

@@ +524,5 @@
> +                      signedResponseData) != SECSuccess) {
> +    return der::Failure;
> +  }
> +
> +  // TODO: Do we even need to parse this? Should we just skip it?

I think producedAt should be bounded by thisUpdate and nextUpdate, but IIRC there are some compatibility issues with this.

@@ +535,5 @@
> +  // responder will never return an empty response, and handling the case of an
> +  // empty response makes things unnecessarily complicated.
> +  if (der::NestedOf(input, der::SEQUENCE, der::SEQUENCE,
> +                     der::MustNotBeEmpty,
> +                     bind(SingleResponse, _1, ref(context))) != der::Success) {

nit: indentation on these two lines

@@ +559,5 @@
> +//                                              CrlEntryExtensions, ...}
> +//                                              } OPTIONAL }
> +#ifdef _MSC_VER
> +#pragma warning(push)
> +#pragma warning(disable:4189)

Which warning is being disabled?

@@ +585,5 @@
> +  // statuses, we use the following precedence rules:
> +  //
> +  // * revoked overrides good and unknown
> +  // * good overrides unknown
> +  PR_ASSERT(context.certStatus == CertStatus::Unknown);

If we're going by the rules in the comment above, I think this PR_ASSERT gets in the way of that.

@@ +591,5 @@
> +    if (ExpectTagAndLength(input, static_cast<uint8_t>(CertStatus::Good), 0)
> +          != der::Success) {
> +      return der::Failure;
> +    }
> +    if (context.certStatus != CertStatus::Revoked) {

How could context.certStatus be CertStatus::Revoked? We just asserted it was CertStatus::Unknown a few lines above.

@@ +619,5 @@
> +  //    greater than the current time.
> +
> +  PRTime OLDEST_ACCEPTABLE = INT64_C(10) * ONE_DAY;
> +
> +  if (OLDEST_ACCEPTABLE + SLOP > context.time) {

This needs a comment (i.e. you're checking that 'context.time - OLDEST_ACCEPTABLE - SLOP' > 0 because you don't want to underflow later)

@@ +641,5 @@
> +    der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0;
> +  if (input.Peek(NEXT_UPDATE_TAG)) {
> +    PRTime nextUpdate;
> +    if (der::Nested(input, NEXT_UPDATE_TAG,
> +                     bind(der::GeneralizedTime, _1, ref(nextUpdate)))

nit: indentation

@@ +652,5 @@
> +    }
> +    if (nextUpdate - thisUpdate <= OLDEST_ACCEPTABLE) {
> +      notAfter = nextUpdate;
> +    } else {
> +      notAfter = thisUpdate + OLDEST_ACCEPTABLE;

It seems like we're limiting the range of (thisUpdate,nextUpdate) to 10 days. We should document this if so.

@@ +657,5 @@
> +    }
> +  } else {
> +    // NSS requires all OCSP responses without a nextUpdate to be recent.
> +    // Match that stricter behavior.
> +    notAfter = thisUpdate + ONE_DAY;

We don't seem to do anything with notAfter... is this just here to make the eventual caching patch easier to write? Or is the idea to only check that thisUpdate is sufficiently recent?

@@ +681,5 @@
> +  match = false;
> +
> +  SECAlgorithmID hashAlgorithm;
> +  if (der::Nested(input, der::SEQUENCE,
> +                   bind(der::AlgorithmIdentifier, _1, ref(hashAlgorithm)))

nit: indentation

@@ +715,5 @@
> +  if (hashAlg != SEC_OID_SHA1) {
> +    return der::Success;
> +  }
> +
> +  uint8_t hashBuf[SHA1_LENGTH];

This should go closer to where it's used.

@@ +726,5 @@
> +  // "The hash shall be calculated over the DER encoding of the
> +  // issuer's name field in the certificate being checked."
> +  if (PK11_HashBuf(SEC_OID_SHA1, hashBuf, cert.derIssuer.data,
> +                   cert.derIssuer.len)
> +          != SECSuccess) {

nit: indentation (move left 2 spaces, looks like)

@@ +750,5 @@
> +  if (issuerKeyHash.len != SHA1_LENGTH)  {
> +    return der::Fail(SEC_ERROR_OCSP_MALFORMED_RESPONSE);
> +  }
> +
> +   // TODO: support SHA-2 hashes

nit: indentation

@@ +752,5 @@
> +  }
> +
> +   // TODO: support SHA-2 hashes
> +
> +  static uint8_t hashBuf[SHA1_LENGTH];

This could go closer to where it's used.

@@ +781,5 @@
> +  if (ExpectTagAndGetLength(input, der::OIDTag, toSkip) != der::Success) {
> +    return der::Failure;
> +  }
> +
> +  // TODO: maybe we should check the syntax of the OID value

But if it's not critical, do we care?

@@ +803,5 @@
> +static der::Result
> +CheckExtensionsForCriticality(der::Input& input)
> +{
> +  return der::NestedOf(input, der::SEQUENCE | 1, der::SEQUENCE,
> +                        der::MustNotBeEmpty, CheckExtensionForCriticality);

nit: indentation

Comment 17

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8373832 [details] [diff] [review]
Part 2: Add OCSP request encoding to insanity::pkix

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

Looks good. Just two questions and a nit.

::: security/insanity/lib/pkixocsp.cpp
@@ +831,5 @@
>  //
>  // http://tools.ietf.org/html/rfc5019#section-4
>  
> +SECItem*
> +CreateEncodedOCSPRequest(PLArenaPool* arena,

Is the arena necessary/desirable? I think SECITEM_AllocItem can be called with a null arena.

@@ +881,5 @@
> +  // we allow for some amount of non-conformance with that requirement while
> +  // still ensuring we can encode the length values in the ASN.1 TLV structures
> +  // in a single byte.
> +  if (issuerCert->serialNumber.len > 127u - totalLenWithoutSerialNumberData) {
> +    PR_NOT_REACHED("should have been caught by encodedSerialNumber.len check above");

what check?

@@ +889,5 @@
> +
> +  uint8_t totalLen = static_cast<uint8_t>(totalLenWithoutSerialNumberData +
> +    cert->serialNumber.len);
> +
> +  SECItem * encodedRequest = SECITEM_AllocItem(arena, nullptr, totalLen);

nit: SECItem*

Comment 18

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8373835 [details] [diff] [review]
Part 3: Integrate insanity::pkix OCSP support into CertVerifier

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

LGTM. Just some nits and a request for more documentation.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +119,5 @@
>    return ::insanity::pkix::VerifySignedData(signedData, cert, mPinArg);
>  }
>  
> +SECStatus
> +NSSCertDBTrustDomain::CheckRevocation(

Overall documentation for this function would be appreciated. My understanding is this is what we're doing:
* If present, attempt to verify the stapled OCSP response. If the response is good or if it indicates anything other than an old response, return that status.
* If OCSP downloading is enabled and this is an end-entity certificate, attempt to fetch a response (no OCSP fetching for intermediates - return success in this case). If the certificate has no OCSP URI, return success if there was no stapled response or indicate that the stapled response was old.
* If a response is returned, verify it and return that status.
* If there is a non-OCSP-server-related error, return that
* If strict OCSP checking is enabled, return failure at this point because a recent OCSP status was not able to be obtained.
* Otherwise, return success.

@@ +133,5 @@
> +  // TODO: need to verify that IsRevoked isn't called for trust anchors AND
> +  // that that fact is documented in insanity.
> +
> +  PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
> +         ("NSSCertDBTrustDomain: Top of CheckRevocatoin \n"));

nit: CheckRevocation\n

::: security/manager/ssl/src/AppsTrustDomain.cpp
@@ +151,5 @@
> +                       /*const*/ CERTCertificate* /*issuerCert*/,
> +                                 PRTime /*time*/,
> +                    /*optional*/ const SECItem* /*stapledOCSPResponse*/)
> +{
> +  // We don't currently do revocation checking. IF we need to distrust an Apps

nit: If?

Comment 19

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

Comment on attachment 8373831 [details] [diff] [review]
Part 1: Add OCSP response parsing & validation to insanity::pkix

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

Thanks for the careful review.

::: security/insanity/lib/pkixocsp.cpp
@@ +446,5 @@
> +  if (!input.AtEnd()) {
> +    // [0] wrapper
> +    if (der::ExpectTagAndGetLength(input,
> +                                   der::CONSTRUCTED | der::CONTEXT_SPECIFIC | 0,
> +                                   length) != der::Success) {

OK. I also added some comments.

@@ +463,5 @@
> +        return der::Fail(SEC_ERROR_BAD_DER);
> +      }
> +
> +      der::Input::Mark mark(input.GetMark());
> +      if (der::Skip(input, der::SEQUENCE) != der::Success) {

I added a comment and I also filed bug 972753 for verifying this with some tests.

@@ +467,5 @@
> +      if (der::Skip(input, der::SEQUENCE) != der::Success) {
> +        return der::Failure;
> +      }
> +
> +      input.GetSECItem(siBuffer, mark, certs[numCerts]);

The first time has der::ExpectTagAndGetLength in the middle. The second time has der::Skip in the middle. They seem a little to different and the duplication is so small, so I think more abstraction may hurt more than it helps. (Do you really want more std::bind?!?)

@@ +524,5 @@
> +                      signedResponseData) != SECSuccess) {
> +    return der::Failure;
> +  }
> +
> +  // TODO: Do we even need to parse this? Should we just skip it?

No, producedAt is the time the response was generated. The response could have been generated LONG before thisUpdate. For example, some CAs unlock their HSMs for their root CA once per year and generate OCSP responses for all their intermediates for the entire year at once.

The NSS code tries to do funny things with producedAt. I think the NSS code is silly in this respect. If you think differently, let me know.

@@ +585,5 @@
> +  // statuses, we use the following precedence rules:
> +  //
> +  // * revoked overrides good and unknown
> +  // * good overrides unknown
> +  PR_ASSERT(context.certStatus == CertStatus::Unknown);

Good point.

@@ +591,5 @@
> +    if (ExpectTagAndLength(input, static_cast<uint8_t>(CertStatus::Good), 0)
> +          != der::Success) {
> +      return der::Failure;
> +    }
> +    if (context.certStatus != CertStatus::Revoked) {

Good point.

@@ +657,5 @@
> +    }
> +  } else {
> +    // NSS requires all OCSP responses without a nextUpdate to be recent.
> +    // Match that stricter behavior.
> +    notAfter = thisUpdate + ONE_DAY;

See the comment I added:

  // We won't accept any OCSP responses that are more than 10 days old, even if
  // the nextUpdate time is further in the future.

If nextUpdate is too far in the future then it is irrelevant. Note that this is different than NSS.

Comment 20

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

Comment on attachment 8373832 [details] [diff] [review]
Part 2: Add OCSP request encoding to insanity::pkix

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

::: security/insanity/lib/pkixocsp.cpp
@@ +831,5 @@
>  //
>  // http://tools.ietf.org/html/rfc5019#section-4
>  
> +SECItem*
> +CreateEncodedOCSPRequest(PLArenaPool* arena,

SECITEm_AllocItem can be used with a null arena. The advantage of using the arena is that it reduces calls to malloc. insanity::pkix tries to avoid calls to malloc for perf and it also *has* to use arenas for other reasons, so we might as well use one here. If we didn't *have* to use arenas for calling certain APIs on other parts of insanity::pkix then I would consider avoiding it here.

@@ +881,5 @@
> +  // we allow for some amount of non-conformance with that requirement while
> +  // still ensuring we can encode the length values in the ASN.1 TLV structures
> +  // in a single byte.
> +  if (issuerCert->serialNumber.len > 127u - totalLenWithoutSerialNumberData) {
> +    PR_NOT_REACHED("should have been caught by encodedSerialNumber.len check above");

Forgot to remove the assertion after refactoring. Removed now.

Comment 21

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

Attachment: Part 1: Add OCSP response parsing & validation to insanity::pkix

Review comments addressed.

Comment 22

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

Attachment: Part 2: Add OCSP request encoding to insanity::pkix, r=keeler

Review comments addressed. Carrying over r+.

Comment 23

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

Attachment: Part 3: Integrate insanity::pkix OCSP support into CertVerifier [v2]

Camilo: The declaration of CertVerifier::InsanityVerifyCert in the header file was accidentally added in the patch for bug 878932. That is why it didn't show up in this patch.

David: I documented NSSCertDBTrustDomain::CheckRevocation more. I didn't add all the documentation about the control flow that you added, though, because it is 100% certain to get out of date. You understand the control flow correctly. I did clarify the control flow and added comments.

Comment 24

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

Attachment: Part 1: Add OCSP response parsing & validation to insanity::pkix [v3]

David, I see why you were confused about my coding of the notAfter/notBefore part of OCSP response verification. Please check that part again very carefully after these modifications, which also address your other concerns.

Comment 25

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

Attachment: Part 2: Add OCSP request encoding to insanity::pkix [v2]

Comment 26

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

Attachment: Part 3: Integrate insanity::pkix OCSP support into CertVerifier [v3] (Please re-review NSSCertDBTrustDomain::CheckRevocation)

Please re-review NSSCertDBTrustDomain::CheckRevocation.

I changed the way that soft fail works to match the NSS classic code, to avoid possible regressions. This is less strict than the previous version of the patch, which tried to only "soft succeed" on networking-specific errors. Without this modification to be more lenient, the existing OCSP tests fail. It may be worth exploring being stricter about OCSP errors in the soft fail case in the future, but not in the middle of this massive refactoring/rewriting.

Comment 27

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

Attachment: Part 4: Expand OCSP xpcshell tests to test insanity::pkix

Comment 28

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

https://tbpl.mozilla.org/?tree=Try&rev=4db11332bac4

Comment 29

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

Removing dependency on bug 916629. When I filed this bug, we basically had no OCSP testing in Gecko. Now we have quite a bit. I expanded the existing tests to cover insanity::pkix. The GTests will move to bug 969938. I'm also removing dependencies on bug 967153 and bug 915334 since the OCSP GTests are moving to bug 969938.

Comment 30

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #29)
> The GTests will move to bug 969938. I'm also
> removing dependencies on bug 967153 and bug 915334 since the OCSP GTests are
> moving to bug 969938.

And, of course, I meant bug 916629.

Comment 31

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

Comment on attachment 8377296 [details] [diff] [review]
Part 4: Expand OCSP xpcshell tests to test insanity::pkix

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

::: security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ +106,5 @@
>    add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
>  
>    //---------------------------------------------------------------------------
>  
> +  add_test(function() { ocspResponder.stop(run_next_test); });

Note that I only made this simple change because I was debugging some test failures, which this change didn't even fix.

This test can't be modified to pass in a reasonable way until bug 915932 is fixed. So, the expansion of this test to include insanity::pkix should be done in that bug.

Comment 32

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8377293 [details] [diff] [review]
Part 1: Add OCSP response parsing & validation to insanity::pkix [v3]

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

Awesome. Just a few nits. r=me with those addressed.

::: security/insanity/lib/pkixocsp.cpp
@@ +307,5 @@
> +  PR_ASSERT(cert);
> +  PR_ASSERT(issuerCert);
> +  // TODO: PR_Assert(pinArg)
> +  PR_ASSERT(encodedResponse);
> +  if (!cert || !issuerCert) {

'|| !encodedResponse' as well, right?

@@ +385,5 @@
> +//     response       OCTET STRING }
> +static inline der::Result
> +ResponseBytes(der::Input& input, Context& context)
> +{
> +# define ID_PKIX_OCSP 0x2B, 0x06, 0x01, 0x05, 0x05, 0x07, 0x30, 0x01

I'm more used to '#define', but I don't know if there are any style guidelines around this.

@@ +386,5 @@
> +static inline der::Result
> +ResponseBytes(der::Input& input, Context& context)
> +{
> +# define ID_PKIX_OCSP 0x2B, 0x06, 0x01, 0x05, 0x05, 0x07, 0x30, 0x01
> +  static const uint8_t id_pkix_ocsp_basic[] = { ID_PKIX_OCSP, 0x01 };

Is this trailing 0x01 not part of the encoding of the OID?

@@ +391,5 @@
> +
> +  if (der::OID(input, id_pkix_ocsp_basic) != der::Success) {
> +    return der::Failure;
> +  }
> +# undef ID_PKIX_OCSP

Same here ('#undef')

@@ +437,5 @@
> +
> +  if (der::Skip(input, der::BIT_STRING, signedData.signature) != der::Success) {
> +    return der::Failure;
> +  }
> +  // strip "number of bits to skip" byte

I would still appreciate a comment along the lines of "The first byte indicates how many bits at the end to skip. It will be 0  for valid signatures."

Comment 33

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8377295 [details] [diff] [review]
Part 3: Integrate insanity::pkix OCSP support into CertVerifier [v3] (Please re-review NSSCertDBTrustDomain::CheckRevocation)

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

Looks good.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +166,5 @@
> +
> +  // TODO: when !mOCSPDownloadEnabled, we still need to handle the fallback for
> +  // expired responses. But, if/when we disable OCSP fetching by default, it
> +  // would be ambiguous whether !mOCSPDownloadEnabled means "I want the default"
> +  // or "I really never want you to ever fetch OCSP."

We should just add another preference that means "never do OCSP fetching ever".

Comment 34

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8377296 [details] [diff] [review]
Part 4: Expand OCSP xpcshell tests to test insanity::pkix

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

Looks great.

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +134,5 @@
> +                                       : ["ev-valid"]);
> +    check_ee_for_ev("ev-valid", isDebugBuild);
> +    ocspResponder.stop(run_next_test);
> +  });
> +  

nit: unnecessary whitespace here and elsewhere

@@ +144,5 @@
> +  });
> +  
> +  add_test(function() {
> +    clearOCSPCache();
> +    // libpkix will attempt to validate the intermediate, which does have an

does insanity as well?

@@ +187,5 @@
>  
> +  add_test(function () {
> +    check_no_ocsp_requests("ev-valid",
> +                           isDebugBuild ? SEC_ERROR_REVOKED_CERTIFICATE :
> +                                          SEC_ERROR_EXTENSION_NOT_FOUND);

nit: ':' on this line

::: security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ -106,5 @@
>    add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
>  
>    //---------------------------------------------------------------------------
>  
> -  add_test(function() { ocspResponder.stop(run_next_test); run_next_test(); });

So, is this a bug? This looks like a bug. Are you going to include this fix when you check this in?

::: security/manager/ssl/tests/unit/test_ocsp_required.js
@@ +25,3 @@
>  
>    let ocspResponder = new HttpServer();
> +  ocspResponder.registerPrefixHandler("/", function (request, response) {

nit: is the style for anonymous functions to be declared with a space after "function" and before "("?

Comment 35

[Camilo Viecco (:cviecco)]

Comment on attachment 8377295 [details] [diff] [review]
Part 3: Integrate insanity::pkix OCSP support into CertVerifier [v3] (Please re-review NSSCertDBTrustDomain::CheckRevocation)

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

r+ with comments addressed. Logic is fine, but comments are misleading for the EV case.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +168,5 @@
> +  // expired responses. But, if/when we disable OCSP fetching by default, it
> +  // would be ambiguous whether !mOCSPDownloadEnabled means "I want the default"
> +  // or "I really never want you to ever fetch OCSP."
> +  if (mOCSPDownloadEnabled) {
> +    // We don't do OCSP fetching for intermediates.

never? This should have a comment that this is true only for non-ev certs. (so that we do not forget to change when we do the ev insanity). Better yet a test for ev with a revoked intermediate is needed.

@@ +180,5 @@
> +
> +    // Nothing to do if we don't have an OCSP responder URI for the cert; just
> +    // assume it is good. Note that this is the confusing, but intended,
> +    // interpretation of "strict" revocation checking in the face of a
> +    // certificate that lacks an OCSP responder URI.

Again rules differ for EV certs. Add into comment. 

Do BR rules require OCSP for ee and/or intermediates? If so then we might want to add logic to this check to ensure that trusted roots enforce this. (probably in the same place for the callback for pinning).

Comment 36

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

Comment on attachment 8377293 [details] [diff] [review]
Part 1: Add OCSP response parsing & validation to insanity::pkix [v3]

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

::: security/insanity/lib/pkixocsp.cpp
@@ +307,5 @@
> +  PR_ASSERT(cert);
> +  PR_ASSERT(issuerCert);
> +  // TODO: PR_Assert(pinArg)
> +  PR_ASSERT(encodedResponse);
> +  if (!cert || !issuerCert) {

Yes, and || !encodedResponse->data.

@@ +386,5 @@
> +static inline der::Result
> +ResponseBytes(der::Input& input, Context& context)
> +{
> +# define ID_PKIX_OCSP 0x2B, 0x06, 0x01, 0x05, 0x05, 0x07, 0x30, 0x01
> +  static const uint8_t id_pkix_ocsp_basic[] = { ID_PKIX_OCSP, 0x01 };

I simplified this so that it is clearer, by doing this:

  static const uint8_t id_pkix_ocsp_basic[] = {
    0x2B, 0x06, 0x01, 0x05, 0x05, 0x07, 0x30, 0x01, 0x01
  };

No more #define/#undef

Comment 37

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

I redid the OCSP signature parsing code slightly, to make it clearer:

  if (signedData.signature.len == 0) {
    return der::Fail(SEC_ERROR_OCSP_BAD_SIGNATURE);
  }
  unsigned int unusedBitsAtEnd = signedData.signature.data[0];
  // XXX: Really the constraint should be that unusedBitsAtEnd must be less
  // than 7. But, we suspect there are no valid OCSP response signatures with
  // non-zero unused bits. It seems like NSS assumes this in various places, so
  // we enforce it. If we find compatibility issues, we'll know we're wrong.
  if (unusedBitsAtEnd != 0) {
    return der::Fail(SEC_ERROR_OCSP_BAD_SIGNATURE);
  }
  ++signedData.signature.data;
  --signedData.signature.len;
  signedData.signature.len = (signedData.signature.len << 3); // Bytes to bits

Comment 38

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

Comment on attachment 8377295 [details] [diff] [review]
Part 3: Integrate insanity::pkix OCSP support into CertVerifier [v3] (Please re-review NSSCertDBTrustDomain::CheckRevocation)

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

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +166,5 @@
> +
> +  // TODO: when !mOCSPDownloadEnabled, we still need to handle the fallback for
> +  // expired responses. But, if/when we disable OCSP fetching by default, it
> +  // would be ambiguous whether !mOCSPDownloadEnabled means "I want the default"
> +  // or "I really never want you to ever fetch OCSP."

Let's discuss that later.

@@ +168,5 @@
> +  // expired responses. But, if/when we disable OCSP fetching by default, it
> +  // would be ambiguous whether !mOCSPDownloadEnabled means "I want the default"
> +  // or "I really never want you to ever fetch OCSP."
> +  if (mOCSPDownloadEnabled) {
> +    // We don't do OCSP fetching for intermediates.

I agree. I added a comment. This will be changed when EV support is converted to use insanity::pkix in bug 921885. We already have some tests to verify that we try to do revocation checking for the intermediate in test_ev_certs.js, and we may want to expand the testing when we modify test_ev_certs.js in bug 921885.

@@ +180,5 @@
> +
> +    // Nothing to do if we don't have an OCSP responder URI for the cert; just
> +    // assume it is good. Note that this is the confusing, but intended,
> +    // interpretation of "strict" revocation checking in the face of a
> +    // certificate that lacks an OCSP responder URI.

The BR do require OCSP URIs but many certificates in active use are not conformant with that requirement, including https://bing.com for example. So, we can't enforce such a check yet.

Comment 39

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

Comment on attachment 8377296 [details] [diff] [review]
Part 4: Expand OCSP xpcshell tests to test insanity::pkix

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

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +144,5 @@
> +  });
> +  
> +  add_test(function() {
> +    clearOCSPCache();
> +    // libpkix will attempt to validate the intermediate, which does have an

We don't use insanity::pkix for EV yet.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ -106,5 @@
>    add_test(function() { do_check_eq(gFetchCount, 1); run_next_test(); });
>  
>    //---------------------------------------------------------------------------
>  
> -  add_test(function() { ocspResponder.stop(run_next_test); run_next_test(); });

Yes, it will be fixed with this checkin. (Technically it should be a separate checkin but...)

::: security/manager/ssl/tests/unit/test_ocsp_required.js
@@ +25,3 @@
>  
>    let ocspResponder = new HttpServer();
> +  ocspResponder.registerPrefixHandler("/", function (request, response) {

It seems there is no consistency here, after looking at other parts of the tree. This change was made accidentally because Visual Studio reformatted the line. But, other parts of the file use this style so I won't undo the change.

Comment 40

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f47a37f59752
https://hg.mozilla.org/integration/mozilla-inbound/rev/853227869c6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/302def56019a
https://hg.mozilla.org/integration/mozilla-inbound/rev/571a1bf8c8ab

Note that the first changeset got checked in with the wrong bug number. Sorry.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/853227869c6b
https://hg.mozilla.org/mozilla-central/rev/302def56019a
https://hg.mozilla.org/mozilla-central/rev/571a1bf8c8ab

Comment 42

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

(In reply to Carsten Book [:Tomcat] from comment #41)
> https://hg.mozilla.org/mozilla-central/rev/853227869c6b
> https://hg.mozilla.org/mozilla-central/rev/302def56019a
> https://hg.mozilla.org/mozilla-central/rev/571a1bf8c8ab

In addition:
> https://hg.mozilla.org/mozilla-central/rev/f47a37f59752

It got checked in with a commit message referencing the wrong bug number. This is the actual bug for it.

Comment 43

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

Comment on attachment 8377293 [details] [diff] [review]
Part 1: Add OCSP response parsing & validation to insanity::pkix [v3]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): The goal is to have all of the new certificate verification logic available as an off-by-default option for Firefox 29 to minimize risk for Firefox 30 when it will be the default. In particular, we want our partner certificate authorities to test this in Firefox 29.

User impact if declined: Less testing of new certificate verification code for Firefox 30.

Testing completed (on m-c, etc.): There are a lot of automated tests already, and we are writing more that would get uplifted with a=test-only next week. Also, this has been on mozilla-central for a week now.

Risk to taking this patch (and alternatives if risky): Very low risk. This code won't be used AT ALL unless the user changes the security.use_insanity_verification pref from false to true. Note that the packaged app signature checking logic (bug 896620) does not use this OCSP code at all.

String or IDL/UUID changes made by this patch: none.

Comment 44

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

Comment on attachment 8377294 [details] [diff] [review]
Part 2: Add OCSP request encoding to insanity::pkix [v2]

[Approval Request Comment]
See comment 43.

Comment 45

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

Comment on attachment 8377295 [details] [diff] [review]
Part 3: Integrate insanity::pkix OCSP support into CertVerifier [v3] (Please re-review NSSCertDBTrustDomain::CheckRevocation)

[Approval Request Comment]
See comment 43.

Comment 46

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

Parts 1-3: https://hg.mozilla.org/releases/mozilla-aurora/rev/4d6c9d219ee3
Part 4: https://hg.mozilla.org/releases/mozilla-aurora/rev/5f6217119443

(I should have folded parts 1-3 together when I landed on inbound/central, because part 1 and part 2 won't build without part 3, but I forgot.)

Alas, for Parts 1-3, I repeated the wrong-bug-number problem that I made on Mozilla-Inbound. Sorry.

Comment 47

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

Comment on attachment 8377293 [details] [diff] [review]
Part 1: Add OCSP response parsing & validation to insanity::pkix [v3]

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

::: security/insanity/lib/pkixocsp.cpp
@@ +437,5 @@
> +
> +  if (der::Skip(input, der::BIT_STRING, signedData.signature) != der::Success) {
> +    return der::Failure;
> +  }
> +  // strip "number of bits to skip" byte

From http://blog.bro.org/2014/03/dissecting-gnutls-bug.html:

"From an attacker perspective, making the signature lookup fail is probably the easiest method to exploit the bug. According to the function _gnutls_x509_get_signature, the signature verification fails if the number of bits in the certificate signature is not divisible by 8. Certificate signatures have an ASN.1 bit string encoding, which theoretically may exhibit an arbitrary number of zeros and ones, and may not necessarily be divisible by 8. But the GnuTLS function assumes a byte-aligned signature, hence the check. In the ASN.1 DER encoding, used in X509 certificates, a bit string begins with a single byte which denotes the number of missing bits in the last byte. For all normal certificates, this value is always zero, and the function continues. But if we change this byte to a non-zero value, the function fails the divisibility check and returns with wrong error code, causing the certificate verification to succeed."