921895 - Verify certificate extended key usage extension in insanity::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

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

Attachment: Verify extended key usage extension in insanity::pkix

Comment 2

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

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.

Comment 3

[Camilo Viecco (:cviecco)]

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 4

[Camilo Viecco (: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 unaddressed comments (and to remove annying reminder).

Comment 5

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

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.

Comment 6

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

Attachment: Verify extended key usage extension in insanity::pkix [v2]

Comment 7

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

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.

Comment 8

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/da84c4ee315b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da84c4ee315b

Comment 10

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

Comment on attachment 8372952 [details] [diff] [review]
Verify extended key usage extension in insanity::pkix [v2]

[Approval Request Comment]
See bug 878932 comment 37.

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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

Comment 12

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b528490f2560