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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

(Blocks 2 bugs, {dev-doc-needed, site-compat})

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 7 obsolete attachments)

Assignee

Description

4 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.
Assignee

Comment 1

4 years ago
Hi Gerv,

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

Thanks!
Flags: needinfo?(gerv)

Comment 2

4 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)?
Assignee

Comment 3

4 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.
Summary: Reject end-entity EV certs with overly long validity periods → Reject EV status for end-entity EV certs with overly long validity periods
Assignee

Comment 4

4 years ago
This patch adds a 27 month check.

Brian, does this look like a reasonable approach to you? Thanks!
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Attachment #8581088 - Flags: feedback?(brian)
Assignee

Comment 5

4 years ago
Some very basic tests.
Assignee

Comment 6

4 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.
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
Flags: needinfo?(gerv)
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.
Attachment #8581088 - Flags: feedback?(brian) → feedback?(rlb)
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.
Attachment #8581088 - Flags: feedback?(rlb) → feedback-
Assignee

Comment 10

4 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.
Attachment #8581088 - Attachment is obsolete: true
Attachment #8607034 - Flags: feedback?(rlb)
Assignee

Comment 11

4 years ago
Attachment #8581089 - Attachment is obsolete: true
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.
Attachment #8607034 - Flags: feedback?(rlb) → feedback-
Assignee

Comment 13

4 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.
Assignee

Comment 14

4 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.
Attachment #8607034 - Attachment is obsolete: true
Attachment #8612200 - Flags: feedback?(rlb)
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.
Attachment #8612200 - Flags: feedback?(rlb) → feedback+
Assignee

Comment 16

4 years ago
Bug 1145679 - Reject EV status for end-entity EV certs with overly long validity periods.
Attachment #8622303 - Flags: review?(dkeeler)
Assignee

Comment 17

4 years ago
Bug 1145679 - Tests.
Attachment #8622304 - Flags: review?(dkeeler)
Assignee

Updated

4 years ago
Attachment #8607035 - Attachment is obsolete: true
Assignee

Comment 18

4 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.
Attachment #8612200 - Attachment is obsolete: true
Assignee

Comment 19

4 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
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?).
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?
Attachment #8622304 - Flags: review?(dkeeler) → review+
(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.
Assignee

Updated

4 years ago
Blocks: 908125
Assignee

Comment 23

4 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.
Assignee

Comment 24

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

Updated

4 years ago
Attachment #8622303 - Flags: review?(dkeeler)
Assignee

Comment 25

4 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.
Assignee

Comment 26

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

Bug 1145679 - Tests.
Attachment #8622304 - Flags: review+
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.
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).
Attachment #8622303 - Flags: review?(dkeeler) → review+
Assignee

Comment 30

4 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.
Assignee

Comment 31

4 years ago
+ Add comment explaining the 39 month limit
Attachment #8622303 - Attachment is obsolete: true
Attachment #8627526 - Flags: review+
Assignee

Comment 32

4 years ago
Attachment #8622304 - Attachment is obsolete: true
Attachment #8627527 - Flags: review+
Assignee

Comment 33

4 years ago
Thanks for the reviews!

(Try pushes are in comment 27 - the B2G failure looks infra related.)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a2b818a26d85
https://hg.mozilla.org/mozilla-central/rev/ade5f5dd22ea
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee

Updated

4 years ago
Blocks: 1222903
You need to log in before you can comment on or make changes to this bug.