:Cykesiopka Assignee
	Description • 9 years ago

According to the current revision of the CAB Forum EV guidelines, EV certs aren't supposed to have validity periods longer than 27 months.
This should probably be enforced via code.

	:Cykesiopka Assignee
	Comment 1 • 9 years ago

Hi Gerv,

As far as I can tell, the limit has always been 27 months. Is this correct to your understanding?

Thanks!

	Rob Stradling
	Comment 2 • 9 years ago

Rejecting overly long EV certs seems a bit harsh.  Why not treat the certs as DV instead (just as Bug 921127 proposes to do for EV certs that contain wildcards)?

	:Cykesiopka Assignee
	Comment 3 • 9 years ago

(In reply to Rob Stradling from comment #2)
> Rejecting overly long EV certs seems a bit harsh.  Why not treat the certs
> as DV instead (just as Bug 921127 proposes to do for EV certs that contain
> wildcards)?

Right, sorry about that. This was bad phrasing on my part. This is exactly my intention.

	:Cykesiopka Assignee
	Comment 4 • 9 years ago

This patch adds a 27 month check.

Brian, does this look like a reasonable approach to you? Thanks!

	:Cykesiopka Assignee
	Comment 5 • 9 years ago

Some very basic tests.

	:Cykesiopka Assignee
	Comment 6 • 9 years ago

Comment on attachment 8581088 [details] [diff] [review]
bug1145679_reject-ev-too-long-validity_WIPv1.patch

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

::: security/certverifier/CertVerifier.cpp
@@ +158,5 @@
>  
>  static const unsigned int MIN_RSA_BITS = 2048;
>  static const unsigned int MIN_RSA_BITS_WEAK = 1024;
> +static const uint64_t MAX_VALIDITY_DAYS_DV = UINT64_MAX;
> +static const uint64_t MAX_VALIDITY_DAYS_EV = 2 * 365 + 3 * 31; // 27 months

This is a bit of a handwave... I haven't checked if the guidelines have a strict definition on exactly how many days 27 months should be.

::: security/pkix/include/pkix/pkixtypes.h
@@ +333,5 @@
> +
> +  // Check that the validity in days is acceptable.
> +  //
> +  // Return Success if the validity is acceptable,
> +  // Result::ERROR_VALIDITY_TOO_LONG if the curve is not acceptable,

Oops. I'll fix this in the next patch. The wording for this entire comment block is a bit weird anyways.

	Gervase Markham [:gerv]
	Comment 7 • 9 years ago

Yes, the maximum length has always been 27 months. However, there is currently discussion in the CAB Forum about whether it might be possible to extend that to match the new, lower limit for other certs (39 months) - perhaps with some mid-term revalidation of the containing data. So I think now is not a good time to add this check. If that discussion resolves itself in favour of keeping the status quo, then we can consider it again.

Gerv

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 8 • 9 years ago

Comment on attachment 8581088 [details] [diff] [review]
bug1145679_reject-ev-too-long-validity_WIPv1.patch

Sorry, I'm way behind on feedback requests. Punting to Richard.

	Richard Barnes [:rbarnes]
	Comment 9 • 9 years ago

Comment on attachment 8581088 [details] [diff] [review]
bug1145679_reject-ev-too-long-validity_WIPv1.patch

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

This needs re-scoping.  The lifetime constraint only applies to certs that are subject to the BRs, i.e., certs processed by NSSCertDBTrustDomain.  So let's try to constrain changes to the files for that class.  That will simplify out a lot of this patch.

::: security/apps/AppTrustDomain.cpp
@@ +296,5 @@
> +                                          Time /*notBefore*/, Time /*notAfter*/,
> +                                          uint64_t /*validityInDays*/)
> +{
> +  return Success;
> +}

Please delete this method after refactoring (see below).

::: security/apps/AppTrustDomain.h
@@ +59,5 @@
> +  virtual Result CheckValidityIsAcceptable(
> +                   mozilla::pkix::EndEntityOrCA endEntityOrCA,
> +                   mozilla::pkix::Time notBefore,
> +                   mozilla::pkix::Time notAfter,
> +                   uint64_t validityInDays) MOZ_OVERRIDE;

Delete after refactoring (see below).

::: security/certverifier/CertVerifier.cpp
@@ +158,5 @@
>  
>  static const unsigned int MIN_RSA_BITS = 2048;
>  static const unsigned int MIN_RSA_BITS_WEAK = 1024;
> +static const uint64_t MAX_VALIDITY_DAYS_DV = UINT64_MAX;
> +static const uint64_t MAX_VALIDITY_DAYS_EV = 2 * 365 + 3 * 31; // 27 months

I think this is probably close enough.  But it should go in NSSCertDBTrustDomain.cpp

::: security/pkix/include/pkix/Time.h
@@ +41,5 @@
>  // Pass by value, not by reference.
>  class Time final
>  {
>  public:
> +  // Construct an uninitialized instance.

Nice catch :)

@@ +94,5 @@
>  
>    static const uint64_t ONE_DAY_IN_SECONDS
>      = UINT64_C(24) * UINT64_C(60) * UINT64_C(60);
>  
> +  // TODO: handle uninitialized case?

Yes, please.

@@ +104,5 @@
> +    } else {
> +      secondsDifference = other.elapsedSecondsAD - elapsedSecondsAD;
> +    }
> +    return secondsDifference / ONE_DAY_IN_SECONDS;
> +  }

This seems like kind of a hack.  It seems like the other methods are in units of seconds, so this one should be too.  Callers can use ONE_DAY_IN_SECONDS to convert.

The method name should note that this is computing an absolute difference, e.g.:

> uint64_t AbsoluteDifferenceInSeconds(const Time& other)

::: security/pkix/include/pkix/pkixtypes.h
@@ +337,5 @@
> +  // Result::ERROR_VALIDITY_TOO_LONG if the curve is not acceptable,
> +  // or another error code if another error occurred.
> +  virtual Result CheckValidityIsAcceptable(EndEntityOrCA endEntityOrCA,
> +                                           Time notBefore, Time notAfter,
> +                                           uint64_t validityInDays) = 0;

It's not clear to me at all that this method is necessary at the TrustDomain level.  The requirement this patch is enforcing only applies to certificates that are subject to the BRs, i.e., certificates within our root program.  So please scope this down to NSSCertDBTrustDomain.

Regardless of where you put this method, the validityInDays argument seems unnecessary, given that you're already passing notBefore and notAfter.

::: security/pkix/lib/pkixcheck.cpp
@@ +125,5 @@
>  // 4.1.2.5 Validity
>  
>  Result
> +CheckValidity(TrustDomain& trustDomain, EndEntityOrCA endEntityOrCA,
> +              Input encodedValidity, Time time)

This seems wrong.  Right now, this method is performing checks with regard to compliance with RFC 5280, whereas this is a check for compliance with the Baseline Requirements.  Let's not cross the streams.

Instead, add the call to CheckValidityIsAcceptable() to CheckIssuerIndependentProperties(), as a follow-on to the RFC 5280 checking.

	:Cykesiopka Assignee
	Comment 10 • 9 years ago

Hi Richard,

Could you take a look at this new approach?

A few notes:
 - I've changed the implementation to be located in CertVerifier instead, primarily because I want to take advantage of the IsCertBuiltInRoot() function for Bug 908125.
 - I've bumped the limit to 39 months in the interest of getting the patches here landed quicker.
   - I will reduce the limit to 27 months some time in the future, if the CABF decides to not raise the limit. (If I missed a CABF decision on this, I apologise).

Thanks.

	Richard Barnes [:rbarnes]
	Comment 12 • 9 years ago

Comment on attachment 8607034 [details] [diff] [review]
bug1145679_reject-ev-too-long-validity_WIPv2.patch

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

This needs some refactoring, so f- for now.

::: security/certverifier/CertVerifier.cpp
@@ +271,5 @@
>                                            KeyPurposeId::id_kp_serverAuth,
>                                            evPolicy, stapledOCSPResponse,
>                                            ocspStaplingStatus);
>          if (rv == Success) {
> +          rv = CheckEEValidityPeriod(builtChainTemp, true);

Rather than defining your own CheckEEValidityPeriod, you should re-use the CheckValidity function in pkixcheck.h.  That header isn't available to this file, so you might have to re-think where the lifetime check is done.

We should consider whether this could be done in a way that would be forward-compatible with enforcing the 39-month restriction on DV certificates that took effect on April 1.

::: security/manager/ssl/tests/unit/psm_common_py/CertUtils.py
@@ +18,5 @@
>                               'policyIdentifier = ' +
>                               '1.3.6.1.4.1.13769.666.666.666.1.500.9.1\n\n' +
>                               'CPS.1 = "http://mytestdomain.local/cps"')
>  
> +default_validity_in_days = 10 * 365

Shouldn't you go ahead and bump this down to two years or so?  It might be good to note that setting this value to something greater than 27 months for EV and 39 months for DV could cause tests to fail.

	:Cykesiopka Assignee
	Comment 13 • 9 years ago

Thanks for the feedback.

(In reply to Richard Barnes [:rbarnes] from comment #12)
> Rather than defining your own CheckEEValidityPeriod, you should re-use the
> CheckValidity function in pkixcheck.h.  That header isn't available to this
> file, so you might have to re-think where the lifetime check is done.

Sure, I'll see what I can do.

> We should consider whether this could be done in a way that would be
> forward-compatible with enforcing the 39-month restriction on DV
> certificates that took effect on April 1.

I developed the patches for that in tandem with the patches here, so this is already forward-compatible.

> Shouldn't you go ahead and bump this down to two years or so?  It might be
> good to note that setting this value to something greater than 27 months for
> EV and 39 months for DV could cause tests to fail.

I stared at Chrome's implementation for a while and decided to do what they did and not enforce the BR validity limits on anything not chaining up to built-in root. In the DV limiting patch, I add a comment explaining this.

If the intent is to enforce validity limits everywhere, then I'll lower the limit and move the comment as appropriate.

	:Cykesiopka Assignee
	Comment 14 • 9 years ago

+ Replace use of NSS functions with optional arguments to pass notBefore and notAfter up to the CertVerifier level.
  - I considered using CheckValidity() here, but this seems less efficient than having optional args

Hopefully I didn't misinterpret your feedback from before.

	Richard Barnes [:rbarnes]
	Comment 15 • 9 years ago

Comment on attachment 8612200 [details] [diff] [review]
bug1145679_reject-ev-too-long-validity_WIPv3.patch

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

Thanks for doing the work to refactor this.  I'm still not totally thrilled with all the passing of times up and down the stack, but I can't really think of a better answer.

	:Cykesiopka Assignee
	Comment 16 • 9 years ago

Bug 1145679 - Reject EV status for end-entity EV certs with overly long validity periods.

	:Cykesiopka Assignee
	Comment 17 • 9 years ago

Bug 1145679 - Tests.

	:Cykesiopka Assignee
	Comment 18 • 9 years ago

Comment on attachment 8612200 [details] [diff] [review]
bug1145679_reject-ev-too-long-validity_WIPv3.patch

(In reply to Richard Barnes [:rbarnes] from comment #15)
> Thanks for doing the work to refactor this.  I'm still not totally thrilled
> with all the passing of times up and down the stack, but I can't really
> think of a better answer.

Thanks for the feedback.

	:Cykesiopka Assignee
	Comment 19 • 9 years ago

https://reviewboard.mozilla.org/r/11345/#review9715

I haven't been successful in adding functionality to pycert.py to use existing keys, so sadly this has to continue using CertUtils.py.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02a5ba6d1797
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a100bb1d75b2

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 20 • 9 years ago

Comment on attachment 8622303 [details]
MozReview Request: Bug 1145679 - Reject EV status for end-entity EV certs with overly long validity periods.

https://reviewboard.mozilla.org/r/11343/#review9879

Good work, but I think we should restructure this slightly. See below.

::: security/pkix/include/pkix/pkix.h:121
(Diff revision 1)
> +                      /*optional out*/ Time* eeNotAfter = nullptr);

I'm not sure this is the best place for this. My concern is that this changes BuildCertChain from "give a bunch of inputs and get a yes/no answer" to "give a bunch of inputs and get a yes/no answer and maybe some additional information that also has to be checked". This also feels like something that should be checked by the TrustDomain. We could add another callback to the interface like so:

Result CheckValidityDuration(Duration validityDuration, EndEntityOrCA endEntityOrCA)

This seems a little heavy-weight, though. Another option would be to add the check to CheckRevocation, since that already gets the validity duration as a parameter. The downside of that is it changes the meaning of "CheckRevocation" slightly (although maybe there's an argument that a certificate with overlong validity is treated as basically revoked with respect to EV?).

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 21 • 9 years ago

Comment on attachment 8622304 [details]
MozReview Request: Bug 1145679 - Tests.

https://reviewboard.mozilla.org/r/11345/#review9883

This looks great.

::: security/manager/ssl/tests/unit/test_validity/generate_ev.py:49
(Diff revision 1)
> +    cert_name = 'ev_%s_%u' % (cert_name_prefix, validity_in_months)

Maybe 'ev_%s_%u_month_validity' for a bit more clarity?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 22 • 9 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #20)
> I'm not sure this is the best place for this. My concern is that this
> changes BuildCertChain from "give a bunch of inputs and get a yes/no answer"
> to "give a bunch of inputs and get a yes/no answer and maybe some additional
> information that also has to be checked". This also feels like something
> that should be checked by the TrustDomain. We could add another callback to
> the interface like so:
> 
> Result CheckValidityDuration(Duration validityDuration, EndEntityOrCA
> endEntityOrCA)

I personally like this way. In particular, I agree with what David said, that checking whether BuildCertChain returning Success or an error should be the only check that a caller of BuildCertChain needs to do.

	:Cykesiopka Assignee
	Comment 23 • 9 years ago

https://reviewboard.mozilla.org/r/11345/#review10267

> Maybe 'ev_%s_%u_month_validity' for a bit more clarity?

I went with 'ev_%s_%u_months' to avoid running into the 64 char CN limit.

	:Cykesiopka Assignee
	Comment 24 • 9 years ago

https://reviewboard.mozilla.org/r/11343/#review10269

> I'm not sure this is the best place for this. My concern is that this changes BuildCertChain from "give a bunch of inputs and get a yes/no answer" to "give a bunch of inputs and get a yes/no answer and maybe some additional information that also has to be checked". This also feels like something that should be checked by the TrustDomain. We could add another callback to the interface like so:
> 
> Result CheckValidityDuration(Duration validityDuration, EndEntityOrCA endEntityOrCA)
> 
> This seems a little heavy-weight, though. Another option would be to add the check to CheckRevocation, since that already gets the validity duration as a parameter. The downside of that is it changes the meaning of "CheckRevocation" slightly (although maybe there's an argument that a certificate with overlong validity is treated as basically revoked with respect to EV?).

Make sense - I added another TrustDomain callback (slightly different than what you suggested though, for forward compat with Bug 908125).

	:Cykesiopka Assignee
	Comment 25 • 9 years ago

Comment on attachment 8622303 [details]
MozReview Request: Bug 1145679 - Reject EV status for end-entity EV certs with overly long validity periods.

Bug 1145679 - Reject EV status for end-entity EV certs with overly long validity periods.

	:Cykesiopka Assignee
	Comment 26 • 9 years ago

Comment on attachment 8622304 [details]
MozReview Request: Bug 1145679 - Tests.

Bug 1145679 - Tests.

	:Cykesiopka Assignee
	Comment 27 • 9 years ago

Comment on attachment 8622304 [details]
MozReview Request: Bug 1145679 - Tests.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7d6295d5656
https://treeherder.mozilla.org/#/jobs?repo=try&revision=825ac71af16a

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 28 • 9 years ago

Just as a heads-up, I might not have time to review this this week. I'll get to it as soon as I can, though.

	Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)
	Comment 29 • 9 years ago

Comment on attachment 8622303 [details]
MozReview Request: Bug 1145679 - Reject EV status for end-entity EV certs with overly long validity periods.

https://reviewboard.mozilla.org/r/11343/#review10683

Looks great!

::: security/certverifier/NSSCertDBTrustDomain.cpp:857
(Diff revision 2)
> +  Duration DURATION_39_MONTHS((3 * 365 + 3 * 31) * Time::ONE_DAY_IN_SECONDS);

We should probably comment that while the EV Guidelines say the maximum is 27 months, we're starting with a higher limit a) to (hopefully) minimize compatibility breakage and b) because there was some talk about raising the limit (although the most recent documents still say 27 months).

	:Cykesiopka Assignee
	Comment 30 • 9 years ago

https://reviewboard.mozilla.org/r/11343/#review10683

> We should probably comment that while the EV Guidelines say the maximum is 27 months, we're starting with a higher limit a) to (hopefully) minimize compatibility breakage and b) because there was some talk about raising the limit (although the most recent documents still say 27 months).

Done - I added the comment just above the |maxValidityDuration = DURATION_39_MONTHS;| line.

	:Cykesiopka Assignee
	Comment 31 • 9 years ago

+ Add comment explaining the 39 month limit

	:Cykesiopka Assignee
	Comment 33 • 9 years ago

Thanks for the reviews!

(Try pushes are in comment 27 - the B2G failure looks infra related.)

	Pulsebot
	Comment 34 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b818a26d85
https://hg.mozilla.org/integration/mozilla-inbound/rev/ade5f5dd22ea

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 35 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/a2b818a26d85
https://hg.mozilla.org/mozilla-central/rev/ade5f5dd22ea