Closed Bug 921895 Opened 7 years ago Closed 6 years ago

Verify certificate extended key usage extension in insanity::pkix

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Attachment #8370958 - Flags: review?(dkeeler)
Attachment #8370958 - Flags: review?(cviecco)
Comment on attachment 8370958 [details] [diff] [review]
Verify extended key usage extension in insanity::pkix

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

r- for now - see second comment. Otherwise looks good.

::: security/insanity/lib/pkixcheck.cpp
@@ +204,5 @@
> +      PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0);
> +      return RecoverableError;
> +    }
> +
> +    for (const SECItem * const *oids = seq->oids;

nit: SECItem* const*

@@ +205,5 @@
> +      return RecoverableError;
> +    }
> +
> +    for (const SECItem * const *oids = seq->oids;
> +      oids && *oids && (!found || endEntityOrCA == MustBeCA); ++oids) {

I don't understand the 'endEntityOrCA == MustBeCA' check. We keep looking even if we've found the requiredEKU if we're checking a CA cert?

@@ +206,5 @@
> +    }
> +
> +    for (const SECItem * const *oids = seq->oids;
> +      oids && *oids && (!found || endEntityOrCA == MustBeCA); ++oids) {
> +      const SECItem *oid = *oids;

nit: SECITem*

@@ +265,5 @@
>                  unsigned int subCACount)
>  {
> +  if (endEntityOrCA != MustBeCA && isTrustAnchor) {
> +    PR_NOT_REACHED("only CAs can be trust anchors.");
> +    return Fail(FatalError, PR_INVALID_STATE_ERROR);

I don't think this is related to EKU checking. If you feel like doing more work, this should probably go in the first patch with this function.
Attachment #8370958 - Flags: review?(dkeeler) → review-
Comment on attachment 8370958 [details] [diff] [review]
Verify extended key usage extension in insanity::pkix

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

::: security/insanity/lib/pkixcheck.cpp
@@ +192,5 @@
> +
> +  // XXX: We're using SEC_ERROR_INADEQUATE_CERT_TYPE here so that callers can
> +  // distinguish EKU mismatch from KU mismatch from basic constraints mismatch.
> +  // We should probably add a new error code that is more clear for this type
> +  // of problem.

Would it not be easier to start with:

if (requiredEKU == anyExtendedKeyUsage) {
  return Success;
}

if (!encnodedEKUs) {
  PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0);
  return RecoverableError.
}

and remove all the if (encodedEKUs) && ocsp section? It the ocsp code makes the return values of this function not very understandable (given the parameter names)

@@ +205,5 @@
> +      return RecoverableError;
> +    }
> +
> +    for (const SECItem * const *oids = seq->oids;
> +      oids && *oids && (!found || endEntityOrCA == MustBeCA); ++oids) {

ditto me.

@@ +234,5 @@
> +      PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0);
> +      return RecoverableError;
> +    }
> +
> +    // CA certificates must never have id-kp-OCSPSigning

where is this? ( I dont see it on rfc6960)
Comment on attachment 8370958 [details] [diff] [review]
Verify extended key usage extension in insanity::pkix

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

r- for unaddressed comments (and to remove annying reminder).
Attachment #8370958 - Flags: review?(cviecco) → review-
Comment on attachment 8370958 [details] [diff] [review]
Verify extended key usage extension in insanity::pkix

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

Thanks for the great review comments.

::: security/insanity/lib/pkixcheck.cpp
@@ +192,5 @@
> +
> +  // XXX: We're using SEC_ERROR_INADEQUATE_CERT_TYPE here so that callers can
> +  // distinguish EKU mismatch from KU mismatch from basic constraints mismatch.
> +  // We should probably add a new error code that is more clear for this type
> +  // of problem.

Please see the updated patch, which is rewritten and which is better documented.

@@ +205,5 @@
> +      return RecoverableError;
> +    }
> +
> +    for (const SECItem * const *oids = seq->oids;
> +      oids && *oids && (!found || endEntityOrCA == MustBeCA); ++oids) {

Addressed in the new patch.

@@ +234,5 @@
> +      PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0);
> +      return RecoverableError;
> +    }
> +
> +    // CA certificates must never have id-kp-OCSPSigning

Explained in the comments of the new patch.

@@ +265,5 @@
>                  unsigned int subCACount)
>  {
> +  if (endEntityOrCA != MustBeCA && isTrustAnchor) {
> +    PR_NOT_REACHED("only CAs can be trust anchors.");
> +    return Fail(FatalError, PR_INVALID_STATE_ERROR);

As discussed in the other bug, this will be removed in an upcoming patch in the series, which has already been r+d.
Attachment #8370958 - Attachment is obsolete: true
Attachment #8372952 - Flags: review?(dkeeler)
Attachment #8372952 - Flags: review?(cviecco)
Comment on attachment 8372952 [details] [diff] [review]
Verify extended key usage extension in insanity::pkix [v2]

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

Ok - looks good.

::: security/insanity/lib/pkixcheck.cpp
@@ +157,5 @@
> +      //
> +      // Note, in particular, that this check prevents a delegated OCSP
> +      // response signing certificate with the CA bit from successfully
> +      // validating when we check it from pkixocsp.cpp, which is a good thing.
> +      // 

nit: trailing space

@@ +184,5 @@
>  
> +// 4.2.1.12. Extended Key Usage (id-ce-extKeyUsage)
> +// 4.2.1.12. Extended Key Usage (id-ce-extKeyUsage)
> +Result
> +CheckExtendedKeyUsage(EndEntityOrCA endEntityOrCA, const SECItem* encodedEKUs,

I'm fairly confident I understand this function now, but again it's subtle. We should definitely be sure we have good test coverage.
Attachment #8372952 - Flags: review?(dkeeler) → review+
Attachment #8372952 - Flags: review?(cviecco) → review+
https://hg.mozilla.org/mozilla-central/rev/da84c4ee315b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 8372952 [details] [diff] [review]
Verify extended key usage extension in insanity::pkix [v2]

[Approval Request Comment]
See bug 878932 comment 37.
Attachment #8372952 - Flags: approval-mozilla-aurora?
Comment on attachment 8372952 [details] [diff] [review]
Verify extended key usage extension in insanity::pkix [v2]

Uplifted granted to the patches relative to the new feature: "Add insanity::pkix as certificate verification option"
Lukas and I discussed with Brian and we think it is important to have this feature for 29 (but disabled by default).
It is early in the aurora process and they have plenty of tests for these feature (and to make sure that the current behaviors are still performing correctly).
Attachment #8372952 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.