968359 - Allow the direct verification of CA certificates in insanity::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

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

Attachment: Allow the direct verification of CA certificates in insanity::pkix

Comment 2

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

Comment on attachment 8370960 [details] [diff] [review]
Allow the direct verification of CA certificates in insanity::pkix

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

I have a concern: if an intermediate CA cert is signed by another CA cert that has pathLenConstraint == 0, as I understand it, our path length calculation will cause a verification failure. Is this correct? On one hand, it makes sense, because the intermediate cert won't be able to sign anything else and so is basically useless, but I would expect the verification failure to happen when verifying something supposedly signed by the intermediate, not the intermediate itself. r- for that for now. Either way this goes, we should document our eventual decision.

::: security/insanity/lib/pkixcheck.cpp
@@ -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);

Ah, well. Nevermind then about my previous comment on this bit of code - let's just remove it entirely?

@@ +297,5 @@
>    }
>  
>    // 4.2.1.10. Name Constraints
>    // 4.2.1.11. Policy Constraints
> +

I'm assuming this was a code/patch separation issue. If you feel like more work, let's not have this blank line in this patch.

Comment 3

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

(In reply to David Keeler (:keeler) from comment #2)
> Comment on attachment 8370960 [details] [diff] [review]
> I have a concern: if an intermediate CA cert is signed by another CA cert
> that has pathLenConstraint == 0, as I understand it, our path length
> calculation will cause a verification failure. Is this correct? On one hand,
> it makes sense, because the intermediate cert won't be able to sign anything
> else and so is basically useless, but I would expect the verification
> failure to happen when verifying something supposedly signed by the
> intermediate, not the intermediate itself. r- for that for now. Either way
> this goes, we should document our eventual decision.

I agree we should document this more clearly.

The direct verification of an CA certificate should answer the question "Is it possible to have a valid end-entity certificate for the given usages for this intermediate?" So, if there's no way to verify an EE certificate that chains to this intermediate (due to path length constraints, or whatever), then the verification should fail. Also, the path length constraint is defined to prohibit any certificates with isCA=true from outside the limit set by the path length constraint.

Does this explanation change your opinion about the correctness of the patch?

Also, do you have some suggested wording the the comment to explain this? I think it would be good if you wrote the comment because it would help me make sure we have a common understanding.

> >                  unsigned int subCACount)
> >  {
> > -  if (endEntityOrCA != MustBeCA && isTrustAnchor) {
> > -    PR_NOT_REACHED("only CAs can be trust anchors.");
> > -    return Fail(FatalError, PR_INVALID_STATE_ERROR);
> 
> Ah, well. Nevermind then about my previous comment on this bit of code -
> let's just remove it entirely?

I believe that, before I made this change, it *Is* important to do that check, because some of the relevant CheckXXX functions do not handle that case properly.  So, I think we should leave the patches as they are. What do you think?

Thanks for the questions. Let me know if there are any other questions or concerns.

Comment 4

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

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #3)
> (In reply to David Keeler (:keeler) from comment #2)
> > Comment on attachment 8370960 [details] [diff] [review]
> > I have a concern: if an intermediate CA cert is signed by another CA cert
> > that has pathLenConstraint == 0, as I understand it, our path length
> > calculation will cause a verification failure. Is this correct? On one hand,
> > it makes sense, because the intermediate cert won't be able to sign anything
> > else and so is basically useless, but I would expect the verification
> > failure to happen when verifying something supposedly signed by the
> > intermediate, not the intermediate itself. r- for that for now. Either way
> > this goes, we should document our eventual decision.
> 
> I agree we should document this more clearly.
> 
> The direct verification of an CA certificate should answer the question "Is
> it possible to have a valid end-entity certificate for the given usages for
> this intermediate?" So, if there's no way to verify an EE certificate that
> chains to this intermediate (due to path length constraints, or whatever),
> then the verification should fail. Also, the path length constraint is
> defined to prohibit any certificates with isCA=true from outside the limit
> set by the path length constraint.
> 
> Does this explanation change your opinion about the correctness of the patch?

Your explanation clarifies my understanding. I believe the patch is correct.

> Also, do you have some suggested wording the the comment to explain this? I
> think it would be good if you wrote the comment because it would help me
> make sure we have a common understanding.

How about something like this:

A successful verification of a certificate with endEntityOrCA = MustBeCA indicates that other successful certificate verifications may chain through that certificate. In particular, this indicates that no pathLenConstraint will be violated if an end-entity certificate signed by the given intermediate is verified instead.

> > >                  unsigned int subCACount)
> > >  {
> > > -  if (endEntityOrCA != MustBeCA && isTrustAnchor) {
> > > -    PR_NOT_REACHED("only CAs can be trust anchors.");
> > > -    return Fail(FatalError, PR_INVALID_STATE_ERROR);
> > 
> > Ah, well. Nevermind then about my previous comment on this bit of code -
> > let's just remove it entirely?
> 
> I believe that, before I made this change, it *Is* important to do that
> check, because some of the relevant CheckXXX functions do not handle that
> case properly.  So, I think we should leave the patches as they are. What do
> you think?

Sounds good.

Comment 5

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

(In reply to David Keeler (:keeler) from comment #4)
> (In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from
> comment #3)
> > Also, do you have some suggested wording the the comment to explain this? I
> > think it would be good if you wrote the comment because it would help me
> > make sure we have a common understanding.
> 
> How about something like this:
> 
> A successful verification of a certificate with endEntityOrCA = MustBeCA
> indicates that other successful certificate verifications may chain through
> that certificate. In particular, this indicates that no pathLenConstraint
> will be violated if an end-entity certificate signed by the given
> intermediate is verified instead.

Hmmm - maybe this should be added:

A failure in verification in this case indicates no certificate will be successfully verified that chains through this certificate for the given usages.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/28beb2db0cd1

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/28beb2db0cd1

Comment 8

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

Comment on attachment 8370960 [details] [diff] [review]
Allow the direct verification of CA certificates in insanity::pkix

[Approval Request Comment]
See bug 878932 comment 37.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8370960 [details] [diff] [review]
Allow the direct verification of CA certificates in insanity::pkix

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 10

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/0adaa7bc58d6