991815 - (mozilla::pkix) Sites not getting EV treatment with mozilla::pkix is on, but do get EV treatment when mozilla::pkix is off because OCSP response is old

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kathleen Wilson]

The following sites get EV treatment when mozilla::pkix off (FF 29), but don’t get EV treatment when mozilla::pkix is on (FF31.0a1, 4/3/2014)

https://www.camerfirma.com
https://ev.omniroot.com
https://2021.globalsign.com
https://2029.globalsign.com/
https://secure.omniroot.com
https://servicios.izenpe.com/
https://evsslrca2-v.quovadisglobal.com
https://repo2.secomtrust.net/ev.gif

Comment 1

[Camilo Viecco (:cviecco)]

All the failures that are still happedning are due to old thisUpdate fields in the intermediate certificate OCSP response. From http://tools.ietf.org/html/rfc6960#section-2.4:
 thisUpdate      The most recent time at which the status being
                   indicated is known by the responder to have been
                   correct.

Here are the investigation results:

> https://www.camerfirma.com -> the intermediate OCSP response is invalid, in particular the thisUpdate field is set up to 2013-10-24 . This is considered too old 

> https://ev.omniroot.com -> Works for me
> https://2021.globalsign.comn -> Intermediate too old: thisUpdate 2014-01-01 
> https://2029.globalsign.com/ -> (Assuming the same as above)
> https://secure.omniroot.com  -> Works for me

> https://servicios.izenpe.com/ -> Again thisUpdate too old: 2013-06-19
> https://evsslrca2-v.quovadisglobal.com -> thisUpdate too old: 2014-02-26
> https://repo2.secomtrust.net/ev.gif ->  thisUpdate too old: 2014-02-25

Comment 2

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

What about nextUpdate? Which ones have a missing nextUpdate? And for the ones where nextUpdate is provided, what is it?

Comment 3

[Camilo Viecco (:cviecco)]

Summary of failing intermediate OCSP date values.

               | thisUpdate | nextUpdate 
       ----------------------------------- 
camerafirma    | 2013-10-24 | 2014-10-24     
globalsign     | 2014-01-01 | 2014-07-15     
inzepe         | 2013-06-19 | 2014-04-10 
quovadisglobal | 2014-02-26 | 2014-08-25    
secomtrust     | 2014-02-25 | 2014-06-29

Comment 4

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

(In reply to Camilo Viecco (:cviecco) from comment #3)
> Summary of failing intermediate OCSP date values.
> 
>                | thisUpdate | nextUpdate 
>        ----------------------------------- 
> camerafirma    | 2013-10-24 | 2014-10-24     
> globalsign     | 2014-01-01 | 2014-07-15     
> inzepe         | 2013-06-19 | 2014-04-10 
> quovadisglobal | 2014-02-26 | 2014-08-25    
> secomtrust     | 2014-02-25 | 2014-06-29

Looks like RESOLVED INVALID is the appropriate resolution here.

The baseline requirements say: "OCSP responses from this service MUST have a maximum expiration time of ten days." Here is the code in mozilla::pkix's pkixocsp.cpp:

  // We won't accept any OCSP responses that are more than 10 days old, even if
  // the nextUpdate time is further in the future.
  static const PRTime OLDEST_ACCEPTABLE = INT64_C(10) * ONE_DAY;

Up to this point, we've linked the EV indicator to having reasonable revocation checking. The above OCSP behavior is not reasonable. For example, look at camerafirma's OCSP response. If camerafirma actually revokes that intermediate today because an attacker got its private key, then an attacker could still MitM users for SIX MONTHS from today. Totally useless.

We should notify the CAs in our program about this change.

Note that the user impact is minimal: the websites still work, just the EV indicator isn't shown. That small impact is not worth spending engineering time to *REDUCE* our enforcement of the baseline requirements.

Comment 5

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

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #4)
> (In reply to Camilo Viecco (:cviecco) from comment #3)
> > Summary of failing intermediate OCSP date values.
> > 
> >                | thisUpdate | nextUpdate 
> >        ----------------------------------- 
> > camerafirma    | 2013-10-24 | 2014-10-24     
> > globalsign     | 2014-01-01 | 2014-07-15     
> > inzepe         | 2013-06-19 | 2014-04-10 
> > quovadisglobal | 2014-02-26 | 2014-08-25    
> > secomtrust     | 2014-02-25 | 2014-06-29
> 
> Looks like RESOLVED INVALID is the appropriate resolution here.
> 
> The baseline requirements say: "OCSP responses from this service MUST have a
> maximum expiration time of ten days."

Spoke too soon. Right below that, it says:

"For the status of Subordinate CA Certificates: The CA SHALL update information provided via an Online Certificate Status Protocol at least (i) every 
twelve months and (ii) within 24 hours after revoking a Subordinate CA Certificate."

So technically these CAs are conformant to the baseline requirements. But, still their behavior doesn't make any sense in terms of security. I still save RESOLVED INVALID is the correct response.

Comment 6

[Camilo Viecco (:cviecco)]

I would like to also mark this as invalid for security reasons. Kathleen what do you think?

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #4)
> The baseline requirements say: "OCSP responses from this service MUST have a
> maximum expiration time of ten days." Here is the code in mozilla::pkix's
> pkixocsp.cpp:
> 
>   // We won't accept any OCSP responses that are more than 10 days old, even
> if
>   // the nextUpdate time is further in the future.
>   static const PRTime OLDEST_ACCEPTABLE = INT64_C(10) * ONE_DAY;

Is it worth also filing a bug on checking those baseline requirements as stated, and rejecting OCSP responses whose expiration time is greater than 10 days, even if the responses are less than 10 days old right now?

Comment 8

[Kathleen Wilson]

I think for compatibility reasons we need to take the same approach with this as we did with the other problems listed here:
https://wiki.mozilla.org/SecurityEngineering/mozpkix-testing#Things_for_CAs_to_Fix

i.e. allow it to work for now. And file a bug to fix it correctly after it's been communicated to CAs and a certain time frame allowed for them to make the changes.

Comment 9

[Daniel Veditz [:dveditz]]

but we ARE allowing it to work "for now"... this new code won't be officially released until July so that's several months notice to the CA's. Only pre-release/test versions are affected.

Comment 10

[Kathleen Wilson]

It's more than just asking these particular CAs to change their OCSP nextUpdates for their intermediate certs. We'll need to discuss it in CAB Forum and get the BRs changed, and as part of that a date will be determined by when CAs should be compliant with the change. And I'll need to communicate that to all the EV CAs and track their progress.

I agree we should work to get this fixed the right way. But I think we should allow it to work the way it has been working (and the way the BRs say) for now so we can continue on the testing and release path we're on, and not have this potentially mask other issues.

Comment 11

[Kathleen Wilson]

(In reply to Brian Smith from comment #4)
> Note that the user impact is minimal: the websites still work, just the EV
> indicator isn't shown. That small impact is not worth spending engineering
> time to *REDUCE* our enforcement of the baseline requirements.

After considering this some more, I think it's reasonable to let EV fail in these cases. So, I'm OK with resolving as WONTFIX.

I'll add this to https://wiki.mozilla.org/SecurityEngineering/mozpkix-testing#Future_Considerations so we remember to follow up on this regarding the BRs and having CAs fix it.

Comment 12

[Gervase Markham [:gerv]]

Before I communicate with the CAB Forum regarding this issue, can I make sure I understand it entirely?

* Do we check OCSP for intermediate certs in all cases, or just for EV?

* Is it our plan in the future to continue to do such checks, even when we get a CRLSets implementation?

* The requirement is precisely that the thisUpdate date/time of the retrieved response is less than 10 days before the current date/time. Is that correct?

* If a response is received which is too old, we act as if:
  a) no response was received (so what happens next depends on the soft/hard fail setting);
  b) an invalid response was received;
  c) a valid "revoked" response was received;
  d) something else?

Gerv

Comment 13

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

(In reply to Gervase Markham [:gerv] from comment #12)
> Before I communicate with the CAB Forum regarding this issue, can I make
> sure I understand it entirely?
> 
> * Do we check OCSP for intermediate certs in all cases, or just for EV?

Just for EV.

> * Is it our plan in the future to continue to do such checks, even when we
> get a CRLSets implementation?

I'd argue that we should stop making OCSP requests ever (by default) once we support Must-Staple and once we have a reasonable way to intermediate certificate revocations (CRLSets or whatnot).

> * The requirement is precisely that the thisUpdate date/time of the
> retrieved response is less than 10 days before the current date/time. Is
> that correct?

Yes. (The implementation allows for some wiggle room due to clock skew.)

> * If a response is received which is too old, we act as if:
>   a) no response was received (so what happens next depends on the soft/hard
> fail setting);
>   b) an invalid response was received;
>   c) a valid "revoked" response was received;
>   d) something else?

We retry validating the certificate as DV, which doesn't require OCSP for intermediates.

Comment 14

[Gervase Markham [:gerv]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #13)
> > * Is it our plan in the future to continue to do such checks, even when we
> > get a CRLSets implementation?
> 
> I'd argue that we should stop making OCSP requests ever (by default) once we
> support Must-Staple and once we have a reasonable way to intermediate
> certificate revocations (CRLSets or whatnot).

OK... so basically we are asking CAs to rejig things to provide OCSP responses for intermediate certs used in EV chains, but in a few months time we are going to stop requesting those responses?

It seems wrong to me to require them to make this big effort for a short time, when we could instead change mozilla::pkix to be more lenient for the intermediate period before CRLSets arrive.

Gerv

Comment 16

[Matěj Cepl]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #4)
> Note that the user impact is minimal: the websites still work, just the EV
> indicator isn't shown. That small impact is not worth spending engineering
> time to *REDUCE* our enforcement of the baseline requirements.

This is not correct (see bug 1005718), with "When an OCSP server connection fails, treat the certificate as invalid" switched on, it actually breaks SSL connections. If mozilla::pkix is more strict than reasonable, than it should be changed IMHO.

Comment 17

[Kathleen Wilson]

(In reply to Matěj Cepl from comment #16)
> (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response)
> from comment #4)
> > Note that the user impact is minimal: the websites still work, just the EV
> > indicator isn't shown. That small impact is not worth spending engineering
> > time to *REDUCE* our enforcement of the baseline requirements.
> 
> This is not correct (see bug 1005718), with "When an OCSP server connection
> fails, treat the certificate as invalid" switched on, it actually breaks SSL
> connections. If mozilla::pkix is more strict than reasonable, than it should
> be changed IMHO.


I confirm that this is indeed the case.  Matěj, thank you for following-up on this.

For all users who have OCSP hard-fail turned on, the sites with this problem will get the sec_error_ocsp_old_response error, and will not be able to continue until they turn off OCSP hard-fail.

This is much worse than just not getting EV treatment.

I don't think that's acceptable, so re-opening this bug. 

We should allow the previous behavior to continue until we either have a different revocation-checking mechanism in place (per comment #14), or confirm that all of the EV CAs have updated their intermediate OCSP responses to have a maximum validity of 10 days.

Comment 18

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

(In reply to Kathleen Wilson from comment #17)
> (In reply to Matěj Cepl from comment #16)
> > This is not correct (see bug 1005718), with "When an OCSP server connection
> > fails, treat the certificate as invalid" switched on, it actually breaks SSL
> > connections. If mozilla::pkix is more strict than reasonable, than it should
> > be changed IMHO.

I don't think mozilla::pkix is being more strict than reasonable.

> For all users who have OCSP hard-fail turned on, the sites with this problem
> will get the sec_error_ocsp_old_response error, and will not be able to
> continue until they turn off OCSP hard-fail.
> 
> This is much worse than just not getting EV treatment.
> 
> I don't think that's acceptable, so re-opening this bug. 

This bug is about sites not getting the EV indicator because of old OCSP responses. This bug isn't about every possible compatibility issue caused by being stricter about old OCSP responses. So, I'd prefer to re-close this bug and re-open bug 1005718. Then bug 1005718 can be about the unfortunate interaction between us being strict in this respect and the security.OCSP.require pref with respect to non-EV certificates.

> We should allow the previous behavior to continue until we either have a
> different revocation-checking mechanism in place (per comment #14), or
> confirm that all of the EV CAs have updated their intermediate OCSP
> responses to have a maximum validity of 10 days.

I think this largely depends on the resolution of bug 1005718. Personally, I'd rather keep this bug RESOLVED WONTFIX and I'd also like to resolve bug 1005718 WONTFIX. The real compatibility problem is with security.OCSP.require=true and I don't think we need to expend a lot of effort on making that non-default configuration work the same as before; that configuration is already unusable for most people for many other reasons.

Comment 19

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

(In reply to Gervase Markham [:gerv] from comment #14)
> (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response)
> from comment #13)
> > > * Is it our plan in the future to continue to do such checks, even when we
> > > get a CRLSets implementation?
> > 
> > I'd argue that we should stop making OCSP requests ever (by default) once we
> > support Must-Staple and once we have a reasonable way to intermediate
> > certificate revocations (CRLSets or whatnot).
> 
> OK... so basically we are asking CAs to rejig things to provide OCSP
> responses for intermediate certs used in EV chains, but in a few months time
> we are going to stop requesting those responses?
> 
> It seems wrong to me to require them to make this big effort for a short
> time, when we could instead change mozilla::pkix to be more lenient for the
> intermediate period before CRLSets arrive.

1. The more lenient we are, the more useless doing the check is in the first place. How lenient should we be? I already think 10 days is much too lenient already.

I thought we had some agreement that we (eventually) want to do multi-stapling. The validity period of OCSP responses for intermediates is still relevant for multi-stapling.

Shorter OCSP response validity periods are a good security practice regardless of what Firefox does, because those OCSP responses will be used by all clients. If we're pushing CAs to implement better practices then that's a good thing that benefits everybody's security.

Comment 20

[Kathleen Wilson]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #19)
> I thought we had some agreement that we (eventually) want to do
> multi-stapling. The validity period of OCSP responses for intermediates is
> still relevant for multi-stapling.

I see. Thanks for clarifying.


> 
> Shorter OCSP response validity periods are a good security practice
> regardless of what Firefox does, because those OCSP responses will be used
> by all clients. If we're pushing CAs to implement better practices then
> that's a good thing that benefits everybody's security.

I agree, and I will gladly communicate that to the CAs. I think we should also ask the CAB Forum to update the subCA section of BR #13.2.2 to say that if OCSP is provided, then "OCSP responses from this service MUST have a maximum expiration time of ten days." 

Both Mozilla CA policy and the Baseline Requirements state that for end-entity certs the OCSP response must have a maximum expiration time of 10 days. However, there is currently no policy that says that if an intermediate OCSP response is provided, it has to have a maximum expiration time of 10 days. So, this is a completely new requirement.

Note that we have previously communicated to CAs that they should send Mozilla revoked intermediate certificates, so we do have a (manual) way of handling these.

So I'm not convinced that introducing this incompatibility now is necessary, or a good idea.

Comment 21

[Robin Lin]

Baseline Requirements state that for end-entity certs the OCSP response must have a maximum expiration time of 10 days.

Maybe I have some miss understanding about "must have".
Currently, our OCSP response expiration time are 24 hours.
Should we change our configuration from 24 hours to 10 days?

Comment 22

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

(In reply to Robin Lin from comment #21)
> Baseline Requirements state that for end-entity certs the OCSP response must
> have a maximum expiration time of 10 days.
> 
> Maybe I have some miss understanding about "must have".
> Currently, our OCSP response expiration time are 24 hours.
> Should we change our configuration from 24 hours to 10 days?

The requirement is that the expiration time be no more than 10 days. 24 hours fits that requirement.

Comment 23

[Gervase Markham [:gerv]]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #19)
> 1. The more lenient we are, the more useless doing the check is in the first
> place. How lenient should we be? I already think 10 days is much too lenient
> already.

Given that we plan to stop doing this check (OCSP for EV intermediates) at all soon, I am just suggesting that we stop doing it now.

> I thought we had some agreement that we (eventually) want to do
> multi-stapling. The validity period of OCSP responses for intermediates is
> still relevant for multi-stapling.

It is. How far do you think multi-stapling is from being standardized, implemented and deployed in Firefox, and implemented and deployed in major web servers?

Giving CAs a fair amount of time to make this change is different from requiring it before we ship mozilla::pkix.

> Shorter OCSP response validity periods are a good security practice
> regardless of what Firefox does, because those OCSP responses will be used
> by all clients. If we're pushing CAs to implement better practices then
> that's a good thing that benefits everybody's security.

I agree that they are a better practice. I don't agree that us requiring CAs to roll out the infrastructure to support the greater volume of OCSP responses on a short schedule, when we plan to stop paying attention to them in a few months anyway, is reasonable. I can see it generating bad feeling, and I don't see much security gain for us.

So here's my proposal: we change mozilla::pkix to not enforce age constraints on the OCSP responses for intermediate certificates. Then, we take a proposal to the CAB Forum to set the max age to 10 days (same as EE certs), and work that proposal through their normal channels into the BRs, with an agreed effective date that we will then adopt in our program.

Gerv

Comment 24

[Kathleen Wilson]

(In reply to Gervase Markham [:gerv] from comment #23)
> So here's my proposal: we change mozilla::pkix to not enforce age
> constraints on the OCSP responses for intermediate certificates. Then, we
> take a proposal to the CAB Forum to set the max age to 10 days (same as EE
> certs), and work that proposal through their normal channels into the BRs,
> with an agreed effective date that we will then adopt in our program.

I concur.

Comment 25

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

Gerv wrote:
> (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response)
> from comment #19)
> > 1. The more lenient we are, the more useless doing the check is in the first
> > place. How lenient should we be? I already think 10 days is much too lenient
> > already.
> 
> Given that we plan to stop doing this check (OCSP for EV intermediates) at
> all soon, I am just suggesting that we stop doing it now.

This seems like the best choice to me.

> It is. How far do you think multi-stapling is from being standardized,
> implemented and deployed in Firefox, and implemented and deployed in major
> web servers?

One form of it is already standardized: http://tools.ietf.org/html/rfc6961. But, I don't think anybody has implemented it yet.

> Giving CAs a fair amount of time to make this change is different from
> requiring it before we ship mozilla::pkix.

Like we'd agreed to before (see the comments above), I think it's fine to stop showing the EV indicator for the CAs that are slow to make the change, since the website will still work exactly the same. We have so many problems with our EV indicator (bug 947079 in particular, as well as others) that it seems doubtful that many end-users are going to notice and/or care anyway.

> I agree that they are a better practice. I don't agree that us requiring CAs
> to roll out the infrastructure to support the greater volume of OCSP
> responses on a short schedule, when we plan to stop paying attention to them
> in a few months anyway, is reasonable. I can see it generating bad feeling,
> and I don't see much security gain for us.

Again, the sites would continue to work, just they wouldn't get the EV indicator. (security.OCSP.requre=true is a separate issue about end-entity certificates, not intermediate certificates).

I agree the security gain is not that big; the fact that this doesn't affect end-users' security that much is why we should just stop doing the OCSP request in the first place.

> So here's my proposal: we change mozilla::pkix to not enforce age
> constraints on the OCSP responses for intermediate certificates.

I think what you mean to suggest is that we should revert back to behavior similar to what libpkix did for intermediate certificates, which was to accept any OCSP response  that had a nextUpdate and where the current time is within [thisUpdate, nextUpdate]. An alternative would be to enforce the EV requirement and say that thisUpdate must be less than a year from today, which would be ridiculous, but still stronger than the old behavior.

I would be willing to r+ such a patch but I also don't think it is really a blocker to shipping Firefox 31 with mozilla::pkix enabled either.

> Then, we
> take a proposal to the CAB Forum to set the max age to 10 days (same as EE
> certs), and work that proposal through their normal channels into the BRs,
> with an agreed effective date that we will then adopt in our program.

A while back we had a discussion about shortening the max age to two (2) days. Ryan Hurst recently said on Twitter that 24 hours would be a good max age. I think before we make a proposal we should try to agree within the Mozilla community what we think is reasonable for the max age. I'd rather not suggest 10 days and then come back later to fight again for a shorter max age. FWIW, a 10-day max age is mostly useless even with security.OCSP.require=true because that gives the attacker on average 5 days (and 10 days, if he plans ahead) to mount whatever attack he can mount with a revoked certificate.

My main objection to a patch for this is that we have more important things to do. Diddling with mostly-useless OCSP fetching is a waste of time that would be better spent implementing Must-Staple and other much-needed enhancements.

I am going to re-open bug 1005718 for the security.OCSP.require=true issue. They are very related but they are separate issues.

Comment 26

[Gervase Markham [:gerv]]

I don't think we should ship in a configuration which knowingly removes the EV indicator from sites whose configuration has not changed for no security benefit.

> I think what you mean to suggest is that we should revert back to behavior similar to what libpkix did 
> for intermediate certificates, which was to accept any OCSP response  that had a nextUpdate and where 
> the current time is within [thisUpdate, nextUpdate].

Yes, that would be fine. Enforcing the 1 year would also be fine, but the difference in practice is minimal. Whatever's easiest to code.

Making this change would also fix bug 1005718. You may not think it's important, but I do. :-)

Camilo: I note this bug is assigned to you. Are you able to make a patch to either:
a) change the OCSP age value for EV intermediates from 10 days to 1 year; or
b) change OCSP checking for EV intermediates to simply check lastUpdate and nextUpdate
?

Gerv

Comment 27

[Camilo Viecco (:cviecco)]

I will do option b (as it is agreed it will be r+ed)

Comment 28

[Kathleen Wilson]

This will be communicated to CAs:
https://wiki.mozilla.org/SecurityEngineering/mozpkix-testing#Things_for_CAs_to_Fix
"6. OCSP responses must have a maximum expiration time of ten days. This applies to OCSP responses for intermediate certificates as well as end-entity certificates"

I filed bug #1009110 -- For intermediate certs, don't accept OCSP responses that are more than 10 days old

Comment 29

[Camilo Viecco (:cviecco)]

Attachment: augment-length-ocsp-responses

Comment 30

[Camilo Viecco (:cviecco)]

Attachment: augment-length-ocsp-responses (v1.1)

Comment 31

[Camilo Viecco (:cviecco)]

Comment on attachment 8422076 [details] [diff] [review]
augment-length-ocsp-responses (v1.1)

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

Went with option a.

Comment 32

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

Comment on attachment 8422076 [details] [diff] [review]
augment-length-ocsp-responses (v1.1)

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

This looks good. Top-level items:
1. I think it would be good to keep arguments in basically the same order in similar contexts (so, if the lifetime argument comes before the thisUpdate/validThrough arguments in one context, it should do so in all).
2. We probably don't need to keep saying "maxOCSPLifetime" when we're already in OCSP-verifying code - I think "maxLifetimeInDays" would work.
3. We should have automated tests for this.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +186,5 @@
>  
> +  // Bug 991815: The BR allow EV intermediates to be up to one year old
> +  // Hopefully the EV BR requirements will change in 2014 so that this can be
> +  // removed.
> +  uint16_t maxOCSPLifeTimeDays = 10;

nit: "lifetime" is one word, so this would be "maxOCSPLifetimeDays"

@@ +408,5 @@
>  SECStatus
>  NSSCertDBTrustDomain::VerifyAndMaybeCacheEncodedOCSPResponse(
>    const CERTCertificate* cert, CERTCertificate* issuerCert, PRTime time,
> +  const SECItem* encodedResponse, EncodedResponseSource responseSource,
> +  uint16_t maxOCSPLifetimeDays)

I think it makes sense to keep these arguments in a similar order to VerifyEncodedOCSPResponse, meaning either maxOCSPLifetimeDays goes before encodedResponse here or the order is switched in VerifyEncodedOCSPResponse.

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +90,5 @@
>    static const PRTime ServerFailureDelay = 5 * 60 * PR_USEC_PER_SEC;
>    SECStatus VerifyAndMaybeCacheEncodedOCSPResponse(
>      const CERTCertificate* cert, CERTCertificate* issuerCert, PRTime time,
> +    const SECItem* encodedResponse, EncodedResponseSource responseSource,
> +    uint16_t maxOCSPLifetimeDays);

Since this function is dealing with OCSP responses, I think we can call the new parameter just "maxLifetimeInDays".

::: security/pkix/include/pkix/pkix.h
@@ +113,5 @@
>  SECStatus VerifyEncodedOCSPResponse(TrustDomain& trustDomain,
>                                      const CERTCertificate* cert,
>                                      CERTCertificate* issuerCert,
>                                      PRTime time,
> +                                    uint16_t maxOCSPLifetimeDays,

Same: "maxLifetimeDays" (or maybe "maxLifetimeInDays")

::: security/pkix/lib/pkixocsp.cpp
@@ +58,5 @@
>      , time(time)
>      , certStatus(CertStatus::Unknown)
>      , thisUpdate(thisUpdate)
>      , validThrough(validThrough)
> +    , maxOCSPLifetimeDays(maxOCSPLifetime)

Again, I think it makes the most sense to keep parameters in as much the same order as possible in similar contexts. So, since VerifyEncodedOCSPResponse takes the lifetime parameter before thisUpdate/validThrough, it makes the most sense to me to either change where maxLifetimeInDays appears in Context or change VerifyEncodedOCSPResponse.

@@ +676,5 @@
>    // 6. When available, the time at or before which newer information will
>    //    be available about the status of the certificate (nextUpdate) is
>    //    greater than the current time.
>  
> +  const PRTime MAX_LIFETIME =

This should probably be "PRTime maxLifetime = context.maxLifetimeInDays * ONE_DAY;"

@@ +702,5 @@
>  
>      if (nextUpdate < thisUpdate) {
>        return der::Fail(SEC_ERROR_OCSP_MALFORMED_RESPONSE);
>      }
> +    if (nextUpdate <= MAX_LIFETIME + thisUpdate) {

To me, the calculation/comparison made more sense as it was (i.e. taking the difference between two times yields a time span that we want to make sure is no more than a certain length).

Comment 33

[Iñigo Barreira]

Hi,

I think that such change (modifying the OCSP response for intermediate CAs) should be asked to the CAB Fórum and modify if so the Baseline Requirements. Izenpe is complying what the BR says (we´ve audited against it) and I see that services.izenpe.com is not valid with this new pkix::library implementation.

I´m not agains the change, it´s not a big issue, but not doing it arbitrary bacause from a CA point of view I should meet the BRs, plus the Mozilla one (or even Google, MS, etc.) then translate that into ETSI or Webtrust and finally get the certification. We can créate inconsistencies between CABF requirements, Mozilla requirements and audit frameworks and would be a mess.

So, I´m agree with Gerv to move/ask the CABF.

Regards

Comment 34

[Camilo Viecco (:cviecco)]

Attachment: augment-length-ocsp-responses (v2)

Comment 35

[Camilo Viecco (:cviecco)]

Attachment: test-long-lived-intermediates

Comment 36

[Camilo Viecco (:cviecco)]

Comment on attachment 8430296 [details] [diff] [review]
test-long-lived-intermediates

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

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +222,5 @@
> +                                       : ["good"]);
> +    check_ee_for_ev("ev-valid", isDebugBuild);
> +    ocspResponder.stop(run_next_test);
> +  });
> +

yes extra space!

Comment 37

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

Comment on attachment 8430295 [details] [diff] [review]
augment-length-ocsp-responses (v2)

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

Cool. r=me with comments addressed.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +183,5 @@
>      PORT_SetError(SEC_ERROR_INVALID_ARGS);
>      return SECFailure;
>    }
>  
> +  // Bug 991815: The BR allow EV intermediates to be up to one year old

OCSP responses for EV intermediates, not the intermediates themselves.

@@ +187,5 @@
> +  // Bug 991815: The BR allow EV intermediates to be up to one year old
> +  // Hopefully the EV BR requirements will change in 2014 so that this can be
> +  // removed.
> +  uint16_t maxOCSPLifetimeInDays = 10;
> +  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {

&& (mOCSPFetching == FetchOCSPForEV || mOCSPFetching == LocalOnlyOCSPForEV)

@@ +188,5 @@
> +  // Hopefully the EV BR requirements will change in 2014 so that this can be
> +  // removed.
> +  uint16_t maxOCSPLifetimeInDays = 10;
> +  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {
> +    maxOCSPLifetimeInDays = 366;

We already allow a slop of one day, so I think we can go with 365 here.

::: security/pkix/lib/pkixocsp.cpp
@@ +55,5 @@
>    Context(TrustDomain& trustDomain,
>            const CERTCertificate& cert,
>            CERTCertificate& issuerCert,
>            PRTime time,
> +          uint16_t maxOCSPLifetimeInDays,

Again, we're already in a file that deals with OCSP, so I think including OCSP in the field name is necessary. Maybe maxResponseLifetimeInDays if we need to be specific about what the lifetime refers to? (But I think maxLifetimeInDays is clear enough).

@@ +79,5 @@
>    TrustDomain& trustDomain;
>    const CERTCertificate& cert;
>    CERTCertificate& issuerCert;
>    const PRTime time;
> +  const uint16_t maxOCSPLifetimeInDays;

Same here.

Comment 38

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

Comment on attachment 8430296 [details] [diff] [review]
test-long-lived-intermediates

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

Looks great, but I think we also need to test that in a non-ev context, attempting to verify the old-but-still-valid OCSP response for the intermediate fails. r=me for this (with nits addressed). Feel free to do the second test in another patch that gets folded with this one before checking in. Also, it would probably be good to have a test that an OCSP response that says it's still valid but was generated more than a year ago also fails in any context.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +467,3 @@
>        aResponse.setStatusLine(aRequest.httpVersion, 200, "OK");
>        aResponse.setHeader("Content-Type", "application/ocsp-response");
> +      let args = [ [responseType, expectedNick, "unused" ] ];

nit: space after second "["

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +218,5 @@
> +                          isDebugBuild ? ["int-ev-valid", "ev-valid"]
> +                                       : ["ev-valid"],
> +                          [], [],
> +                          isDebugBuild ? ["longvalidityalmostold", "good"]
> +                                       : ["good"]);

nit: alignment of this whole section. Precompute things like the expected requests/responses arrays if need be.

::: security/manager/ssl/tests/unit/tlsserver/cmd/GenerateOCSPResponse.cpp
@@ +56,5 @@
>    { "unauthorized",    ORTUnauthorized},   // the responder does not know about
>                                             //   the cert
>    { "bad-signature",   ORTBadSignature},   // the response has a bad signature
> +  { "longvalidityalmostold", ORTLongValidityAlmostExpired}, // the response is
> +                                           // still valid, but the generation

generation time

Comment 39

[Camilo Viecco (:cviecco)]

Comment on attachment 8430295 [details] [diff] [review]
augment-length-ocsp-responses (v2)

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

Thanks for the fast review. One question tough:

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +187,5 @@
> +  // Bug 991815: The BR allow EV intermediates to be up to one year old
> +  // Hopefully the EV BR requirements will change in 2014 so that this can be
> +  // removed.
> +  uint16_t maxOCSPLifetimeInDays = 10;
> +  if (endEntityOrCA == EndEntityOrCA::MustBeCA) {

Why only on those two cases? The logic gets more complex for no gain. (NeverFetchOCSP does not fetch, FetchOCSPForDVHardFail, FetchOCSPForDVSoftFail dont fetch intermediates anyway)

Comment 40

[Camilo Viecco (:cviecco)]

Attachment: augment-length-ocsp-responses (v3)

keeping r+ from keeler

Comment 41

[Camilo Viecco (:cviecco)]

Attachment: ancient-value-test

Comment 42

[Camilo Viecco (:cviecco)]

Attachment: ancient-stapled-tests

This seems like the least incorrect place to put this.

Comment 43

[Camilo Viecco (:cviecco)]

Comment on attachment 8431737 [details] [diff] [review]
ancient-stapled-tests

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

I think this is the least incorrect place to put this. But the more I think I dont think this test should live in this bug.

Comment 44

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

Comment on attachment 8431736 [details] [diff] [review]
ancient-value-test

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

r=me with comments addressed.

::: security/manager/ssl/tests/unit/test_ev_certs.js
@@ +248,5 @@
>      check_ee_for_ev("ev-valid", !useMozillaPKIX && isDebugBuild);
>      ocspResponder.stop(run_next_test);
>    });
>  
> +  // Bug 991815 Ancient responses are Not OK for EV ever (still OK for

The issue is that the validity period is very long, right? I think it's important to include that point in this comment.

@@ +263,5 @@
> +    let ocspResponder = startOCSPResponder(SERVER_PORT, "www.example.com", [],
> +                          "test_ev_certs",
> +                          isDebugBuild ? debugCertNickArray : ["ev-valid"],
> +                          [], [],
> +                          isDebugBuild ? debugResponseArray

I was thinking there would be no inline-ifs here (which would probably allow this to be indented to the same level as the first three arguments, but it's not a big deal).

::: security/manager/ssl/tests/unit/tlsserver/lib/OCSPCommon.h
@@ +37,5 @@
>    ORTDelegatedIncludedLast, // same, but multiple other certificates are included
>    ORTDelegatedMissing, // the response is signed by a not included delegated responder
>    ORTDelegatedMissingMultiple, // same, but multiple other certificates are included
> +  ORTLongValidityAlmostExpired, // a good response, but that was generated a almost a year ago
> +  ORTAncientAlmostExpired, // a good response, more than one year old

I think it's important to mention that the validity is very long in the comment.

Comment 45

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

(In reply to David Keeler (:keeler) [use needinfo?] from comment #32)
> >      if (nextUpdate < thisUpdate) {
> >        return der::Fail(SEC_ERROR_OCSP_MALFORMED_RESPONSE);
> >      }
> > +    if (nextUpdate <= MAX_LIFETIME + thisUpdate) {
> 
> To me, the calculation/comparison made more sense as it was (i.e. taking the
> difference between two times yields a time span that we want to make sure is
> no more than a certain length).

The code was originally written that way to reduce/eliminate the chance of integer overflow.

Comment 46

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

Comment on attachment 8431737 [details] [diff] [review]
ancient-stapled-tests

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

This looks good.

::: security/manager/ssl/tests/unit/test_ocsp_stapling_expired.js
@@ +100,5 @@
>    add_ocsp_test("ocsp-stapling-expired.example.com",
>                  getXPCOMStatusFromNSS(SEC_ERROR_OCSP_UNKNOWN_CERT),
>                  ocspResponseUnknown);
> +
> +  if (useMozillaPKIX) {

Maybe add a comment that these tests are verifying that an old stapled response with a very long validity is rejected (which is evident by that the code attempts to fetch a more recent response).

@@ +104,5 @@
> +  if (useMozillaPKIX) {
> +    add_ocsp_test("ocsp-stapling-ancient-valid.example.com", Cr.NS_OK,
> +                  ocspResponseGood);
> +    add_ocsp_test("ocsp-stapling-ancient-valid.example.com", Cr.NS_OK,
> +                  expiredOCSPResponseGood);

Due to soft-fail, this test isn't really testing much.

@@ +106,5 @@
> +                  ocspResponseGood);
> +    add_ocsp_test("ocsp-stapling-ancient-valid.example.com", Cr.NS_OK,
> +                  expiredOCSPResponseGood);
> +    add_ocsp_test("ocsp-stapling-ancient-valid.example.com", Cr.NS_OK,
> +                  oldValidityPeriodOCSPResponseGood);

Same here.

@@ +124,5 @@
>                      .snapshot();
> +
> +  do_check_eq(histogram.counts[0], 2 * 0);  // histogram bucket 0 is unused
> +  do_check_eq(histogram.counts[1], 2 * 0);  // 0 connections with a good response
> +  do_check_eq(histogram.counts[2], 2 * 0);  // 0 connections with no stapled resp.

All of these other lines don't need to be changed.

@@ +126,5 @@
> +  do_check_eq(histogram.counts[0], 2 * 0);  // histogram bucket 0 is unused
> +  do_check_eq(histogram.counts[1], 2 * 0);  // 0 connections with a good response
> +  do_check_eq(histogram.counts[2], 2 * 0);  // 0 connections with no stapled resp.
> +  do_check_eq(histogram.counts[3], 2 * 9 + 5); // 9 connections with an expired response
> +                                               // 5 connection with an ancient response

how about "with a response considered expired due to being old but having an overly-long validity period"

Comment 47

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=746ec15e26b3

Comment 48

[Camilo Viecco (:cviecco)]

In inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/71b7b1f1e87b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d4fd9141e5ab

Comment 49

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/71b7b1f1e87b
https://hg.mozilla.org/mozilla-central/rev/d4fd9141e5ab

Comment 50

[Camilo Viecco (:cviecco)]

Attachment: test-long-lived-intermediates

folded r+ patch

Comment 51

[Camilo Viecco (:cviecco)]

Comment on attachment 8431734 [details] [diff] [review]
augment-length-ocsp-responses (v3)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 915930
User impact if declined: Some EV sites will no longer display as EV, making our users and CAs in our root program unhappy.
Testing completed (on m-c, etc.):  m-c since friday, patch includes tests
Risk to taking this patch (and alternatives if risky): minor, base risks of accepting longer certs now in tests attached in part2.
String or IDL/UUID changes made by this patch: None

Comment 52

[Camilo Viecco (:cviecco)]

Comment on attachment 8432578 [details] [diff] [review]
test-long-lived-intermediates

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 915930
User impact if declined: Aome EV sites will no longer display as EV, making our users and CAs in our root program unhappy.
Testing completed (on m-c, etc.):  m-c since friday, patch includes tests
Risk to taking this patch (and alternatives if risky): none these are the tests for part1
String or IDL/UUID changes made by this patch: None

Comment 53

[Camilo Viecco (:cviecco)]

aurora try:
 https://tbpl.mozilla.org/?tree=Try&rev=f87c60dd064c

Comment 54

[Camilo Viecco (:cviecco)]

Attachment: folded-rebased-for-aurora

keeping r+

Comment 55

[Camilo Viecco (:cviecco)]

pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/af7af149b5dd

Comment 56

[Ryan VanderMeulen [:RyanVM]]

This was backed out from Aurora for Windows bustage.
https://hg.mozilla.org/releases/mozilla-aurora/rev/4c21b252632f

Comment 57

[Camilo Viecco (:cviecco)]

aurora try (green)
https://tbpl.mozilla.org/?tree=Try&rev=e07c553decfc

Comment 58

[Camilo Viecco (:cviecco)]

push to inbound

 https://hg.mozilla.org/releases/mozilla-aurora/rev/6830ccd24825

Comment 59

[Camilo Viecco (:cviecco)]

(In reply to Camilo Viecco (:cviecco) from comment #58)
> and by inbound I meant aurora 
> 
>  https://hg.mozilla.org/releases/mozilla-aurora/rev/6830ccd24825