1141189 - add ability to skip expensive revocation checks for "short-lived" certificates

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

We have been exploring a mode of operation whereby we skip expensive revocation checks (i.e. OCSP fetching) for certificates that have a sufficiently short validity period (on the order of days or even hours). This bug will track that effort.

Comment 1

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

I'm not sure why I made this a tracking bug. I'll just have this be for the patch to add the ability to do this. We can file a bug to turn it on by default later.

Comment 2

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

Attachment: patch

Cykesiopka, would you mind having a look? Thanks.
Also asking for feedback from :rbarnes to make sure I'm on the right track.

Comment 3

[:Cykesiopka]

Comment on attachment 8589279 [details] [diff] [review]
patch

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

Today I learned review drafts are not saved server side, and that it's therefore a bad idea to log out with permanent private browsing on!
*Sigh*

Anyways, LGTM.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +490,2 @@
>    if ((mOCSPFetching == NeverFetchOCSP) ||
> +      (validityDuration < shortLifetime) ||

Incredibly minor nit: should this be <= instead of just < ?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +209,5 @@
>         ? CertVerifier::ocspGetEnabled
>         : CertVerifier::ocspGetDisabled;
>  
> +  *ocspShortLifetimeInDays =
> +    Preferences::GetInt("security.OCSP.short_lifetime_in_days", 0);

GetUint() seems more appropriate.

::: security/manager/ssl/tests/unit/test_ocsp_caching.js
@@ +57,5 @@
> +  });
> +  add_connection_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess,
> +                      clearSessionCache);
> +  add_test(function() {
> +    do_check_eq(gFetchCount, 0);

Nit: I would prefer Assert.jsm methods in new code (with or without expected result strings), but I'm fine with the old methods as well.

@@ +67,5 @@
> +  add_connection_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess,
> +                      clearSessionCache);
> +  add_test(function() {
> +    do_check_eq(gFetchCount, 1);
> +    Services.prefs.clearUserPref("security.OCSP.short_lifetime_in_days");

Might be a good idea to do this in a do_register_cleanup() as well just to be extra safe.

Comment 4

[Richard Barnes [:rbarnes]]

Comment on attachment 8589279 [details] [diff] [review]
patch

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

You might check on whether it makes sense for this to overlap at all with Bug 1145679.

::: security/apps/AppTrustDomain.h
@@ +33,5 @@
>                              mozilla::pkix::Time time) override;
>    virtual Result CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
>                                   const mozilla::pkix::CertID& certID,
>                                   mozilla::pkix::Time time,
> +                                 mozilla::pkix::TimeDifference validityDuration,

Does this need to be done for app signing certificates?  Not sure if it's harmful, just don't know if it's required.  And if we want to apply this to all certificates, maybe it should be up at the TrustDomain level?  E.g. in pkixbuild.cpp

It seems like it would be slightly cleaner to split out a method of the form 

> bool RevocationRequired(const TimeDifference&)

(Or whatever informatino we want to provide to make that decision) Then you could have a stub at the TrustDomain level that just returns false (and add a stub CheckRevocation that returns Success).  That way, you wouldn't need to touch the signature for CheckRevocation, and it seems like the impact would be smaller overall.

::: security/pkix/include/pkix/Time.h
@@ +121,5 @@
>  
>  // Note the epoch is the unix epoch (ie 00:00:00 UTC, 1 January 1970)
>  Time TimeFromEpochInSeconds(uint64_t secondsSinceEpoch);
>  
> +class TimeDifference final

This seems like slightly the wrong concept.  Compare the golang time module, which uses Duration in this role, and Time.Sub() to compute a difference.

@@ +131,5 @@
> +    : differenceInSeconds(timeDifferenceInSeconds)
> +  {
> +  }
> +
> +  Result TakeTimeDifference(Time laterTime, Time earlierTime)

So the idea here is something like the following?

> TimeDifference delta;
> delta.TakeTimeDifference(before, after)

That seems kind of convoluted.  I would expect something more like:

> TimeDifference delta = after.DifferenceFrom(before)

::: security/pkix/lib/pkixcheck.cpp
@@ +126,5 @@
>  
>  Result
> +CheckValidity(Input encodedValidity, Time time,
> +              /*optional out*/ Time* notBeforeOut,
> +              /*optional out*/ Time* notAfterOut)

Just use "Time&"?  (Don't know what the C++ best practice is here.)  I guess using pointers allows for null arguments.

Comment 5

[:Cykesiopka]

Comment on attachment 8589279 [details] [diff] [review]
patch

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

(Removing accidental r? so I stop request reminder e-mails).

Comment 6

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

Thanks for the reviews.

(In reply to Cykesiopka from comment #3)
> ::: security/certverifier/NSSCertDBTrustDomain.cpp
> @@ +490,2 @@
> >    if ((mOCSPFetching == NeverFetchOCSP) ||
> > +      (validityDuration < shortLifetime) ||
> 
> Incredibly minor nit: should this be <= instead of just < ?

My intention was that if shortLifetime == 0, this feature would be disabled. If the comparison were <= and validityDuration were 0, that wouldn't work as expected. Of course, it's quite unlikely that validityDuration would be 0 (particularly given that CheckRevocation isn't called for expired certificates), but I think it's best to keep it that way anyway.

> ::: security/manager/ssl/src/nsNSSComponent.cpp
> @@ +209,5 @@
> >         ? CertVerifier::ocspGetEnabled
> >         : CertVerifier::ocspGetDisabled;
> >  
> > +  *ocspShortLifetimeInDays =
> > +    Preferences::GetInt("security.OCSP.short_lifetime_in_days", 0);
> 
> GetUint() seems more appropriate.

Good call.

> ::: security/manager/ssl/tests/unit/test_ocsp_caching.js
> @@ +57,5 @@
> > +  });
> > +  add_connection_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess,
> > +                      clearSessionCache);
> > +  add_test(function() {
> > +    do_check_eq(gFetchCount, 0);
> 
> Nit: I would prefer Assert.jsm methods in new code (with or without expected
> result strings), but I'm fine with the old methods as well.

Sounds good.

> @@ +67,5 @@
> > +  add_connection_test("ocsp-stapling-none.example.com", PRErrorCodeSuccess,
> > +                      clearSessionCache);
> > +  add_test(function() {
> > +    do_check_eq(gFetchCount, 1);
> > +    Services.prefs.clearUserPref("security.OCSP.short_lifetime_in_days");
> 
> Might be a good idea to do this in a do_register_cleanup() as well just to
> be extra safe.

Unless I'm mistaken, xpcshell tests each run in their own environment, so prefs don't need to be cleared from test to test.

Comment 7

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

(In reply to Richard Barnes [:rbarnes] from comment #4)
> You might check on whether it makes sense for this to overlap at all with
> Bug 1145679.

I think we'll need to do something slightly different there, since mozilla::pkix internally makes the decision on whether or not a policy is present and valid. I imagine we can re-use the Duration (previously known as TimeDifference) type, though.

> ::: security/apps/AppTrustDomain.h
> @@ +33,5 @@
> >                              mozilla::pkix::Time time) override;
> >    virtual Result CheckRevocation(mozilla::pkix::EndEntityOrCA endEntityOrCA,
> >                                   const mozilla::pkix::CertID& certID,
> >                                   mozilla::pkix::Time time,
> > +                                 mozilla::pkix::TimeDifference validityDuration,
> 
> Does this need to be done for app signing certificates?  Not sure if it's
> harmful, just don't know if it's required.  And if we want to apply this to
> all certificates, maybe it should be up at the TrustDomain level?  E.g. in
> pkixbuild.cpp

This should be a policy decision made by each TrustDomain implementation. Revocation checking isn't done for app signing certificates anyway, so that implementation basically ignores the parameter.

> It seems like it would be slightly cleaner to split out a method of the form 
> 
> > bool RevocationRequired(const TimeDifference&)
> 
> (Or whatever informatino we want to provide to make that decision) Then you
> could have a stub at the TrustDomain level that just returns false (and add
> a stub CheckRevocation that returns Success).  That way, you wouldn't need
> to touch the signature for CheckRevocation, and it seems like the impact
> would be smaller overall.

I think this is less work overall than adding a new function on TrustDomain. There's also a precedent for this kind of thing: in bug 1071308, we added a time parameter to IsChainValid because some refactoring made it a good solution to pass in the information that way.

> ::: security/pkix/include/pkix/Time.h
> @@ +121,5 @@
> >  
> >  // Note the epoch is the unix epoch (ie 00:00:00 UTC, 1 January 1970)
> >  Time TimeFromEpochInSeconds(uint64_t secondsSinceEpoch);
> >  
> > +class TimeDifference final
> 
> This seems like slightly the wrong concept.  Compare the golang time module,
> which uses Duration in this role, and Time.Sub() to compute a difference.

Good call - changed it to Duration.

> @@ +131,5 @@
> > +    : differenceInSeconds(timeDifferenceInSeconds)
> > +  {
> > +  }
> > +
> > +  Result TakeTimeDifference(Time laterTime, Time earlierTime)
> 
> So the idea here is something like the following?
> 
> > TimeDifference delta;
> > delta.TakeTimeDifference(before, after)
> 
> That seems kind of convoluted.  I would expect something more like:
> 
> > TimeDifference delta = after.DifferenceFrom(before)

I just made it a constructor that takes the absolute difference of two Times:

> Duration delta(after, before);

> ::: security/pkix/lib/pkixcheck.cpp
> @@ +126,5 @@
> >  
> >  Result
> > +CheckValidity(Input encodedValidity, Time time,
> > +              /*optional out*/ Time* notBeforeOut,
> > +              /*optional out*/ Time* notAfterOut)
> 
> Just use "Time&"?  (Don't know what the C++ best practice is here.)  I guess
> using pointers allows for null arguments.

Using pointers just means they can be omitted if the caller doesn't care about the output values. This is fairly common in mozilla::pkix.

Comment 8

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

Attachment: patch v2

Comment 9

[:Cykesiopka]

Comment on attachment 8596077 [details] [diff] [review]
patch v2

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

::: security/pkix/include/pkix/Time.h
@@ +139,5 @@
> +
> +  bool operator<=(const Duration& other) const
> +  {
> +    return durationInSeconds <= other.durationInSeconds;
> +  }

operator< needs to be here as well, or this won't compile.

Comment 10

[Richard Barnes [:rbarnes]]

Comment on attachment 8596077 [details] [diff] [review]
patch v2

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

Besides a few nits on formatting and naming, LGTM.

::: security/apps/AppTrustDomain.cpp
@@ +222,5 @@
>    return DigestBufNSS(item, digestAlg, digestBuf, digestBufLen);
>  }
>  
>  Result
> +AppTrustDomain::CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,

Nit: Move Time and Duration to their own lines?

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +54,5 @@
>    , mOCSPFetching(ocspFetching)
>    , mOCSPCache(ocspCache)
>    , mPinArg(pinArg)
>    , mOCSPGetConfig(ocspGETConfig)
> +  , mOCSPShortLifetimeInDays(ocspShortLifetimeInDays)

Nit: It occurs to me after looking at this variable name several times that it doesn't actually have anything to do with the OCSP lifetime, it's the cert lifetime.   mShortLifetimeInDays? mShortCertLifetimeInDays?  mThresholdShortLifetimeInDays?

::: security/manager/ssl/src/nsNSSComponent.cpp
@@ +185,5 @@
>  static void
>  GetOCSPBehaviorFromPrefs(/*out*/ CertVerifier::OcspDownloadConfig* odc,
>                           /*out*/ CertVerifier::OcspStrictConfig* osc,
>                           /*out*/ CertVerifier::OcspGetConfig* ogc,
> +                         /*out*/ uint32_t* ocspShortLifetimeInDays,

Nit: GetRevocationBehaviorFromPrefs?  In light of the above comment about OCSP.  (It looks like there's only one caller, so the refactor would be simple.)

Nit: It would be nice to have either all the variables be short or all of them be long.

@@ +210,5 @@
>         : CertVerifier::ocspGetDisabled;
>  
> +  // If we pass in just 0 as the second argument to Preferences::GetUint, there
> +  // are two function signatures that match (given that 0 can be intepreted as
> +  // a null pointer). Thus the compiler will complain without it.

Nit: "without it" -> "without the cast"

@@ +1359,5 @@
>        ConfigureTLSSessionIdentifiers();
>      } else if (prefName.EqualsLiteral("security.OCSP.enabled") ||
>                 prefName.EqualsLiteral("security.OCSP.require") ||
>                 prefName.EqualsLiteral("security.OCSP.GET.enabled") ||
> +               prefName.EqualsLiteral("security.OCSP.short_lifetime_in_days") ||

As above, putting this under OCSP seems a little inapt.  "security.pki.short_lifetime_in_days"?

Comment 11

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

Attachment: patch v2.1

Thanks for the reviews.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=017d2406419a

Brian, I'd appreciate any feedback you have about the mozilla::pkix api change.

(In reply to Richard Barnes [:rbarnes] from comment #10)
> Review of attachment 8596077 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: security/apps/AppTrustDomain.cpp
> @@ +222,5 @@
> >    return DigestBufNSS(item, digestAlg, digestBuf, digestBufLen);
> >  }
> >  
> >  Result
> > +AppTrustDomain::CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
> 
> Nit: Move Time and Duration to their own lines?

I'm not sure that's necessary - they fit in 80 characters.

Comment 12

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

Comment on attachment 8598275 [details] [diff] [review]
patch v2.1

JC, I think the best approach here would be for you to have a quick look at this. Then, if you have any questions, we can discuss this and/or the overall architecture of mozilla::pkix (which we should probably do anyway eventually).

Comment 13

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8598275 [details] [diff] [review]
patch v2.1

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

It looks to me like all the comments have already been made! I worked through the code and the plumbing and don't see anything of concern. LGTM!

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1853f12d7d8c

Comment 15

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

I've gone ahead and landed this. If there are concerns about the API changes, we can address them in a follow-up.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1853f12d7d8c

Comment 17

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

Comment on attachment 8598275 [details] [diff] [review]
patch v2.1

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

I didn't look at this carefully but it seems OK to me. I don't like that CheckValidity is used for two different things (extracting times and validating times), but its understandable as to why it is done that way and it's probably not worth spending time to change it. I also don't like that the Duration constructor goes out of its way to allow somebody to pass the components of the duration in either order. The caller should always know which is larger and we should always consistently pass time pairs in the same order, which means that part of the conditional logic there is basically dead code. Again, that's just a minor thing.