982932 - Check issuance date of intermediate certificate in effort to obsolete SGC EKU

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Kathleen Wilson]

Please throw an error during certificate verification when the SSL cert chains up to a new intermediate cert that uses the "Netscape Server Gated Crypto (2.16.840.1.113730.4.1)" EKU instead of the "TLS Web Server Authentication (1.3.6.1.5.5.7.3.1)" EKU.

Mozilla's CA Certificate Policy version 2.1 (https://wiki.mozilla.org/CA:CertificatePolicyV2.1), states that intermediate certificates must either be technically constrained or publicly disclosed and audited. For a subordinate CA certificate to be considered technically constrained, the certificate must include an Extended Key Usage (EKU) extension specifying all extended key usages that the subordinate CA is authorized to issue certificates for.

This is also described in section 9.7 of the CA/Browser Forum's Baseline Requirements (https://cabforum.org/baseline-requirements-documents/)

As per Bug #725351, Mozilla is adding code to enforce consistency in the EKUs in the certificate chain, in the effort to help enforce the policy.

However, in Bug #982292 it was found that some intermediate certificates are using the "Netscape Server Gated Crypto (2.16.840.1.113730.4.1)" EKU instead of the "TLS Web Server Authentication (1.3.6.1.5.5.7.3.1)" EKU.

The "Netscape Server Gated Crypto (2.16.840.1.113730.4.1)" (SGC) EKU is obsolete, so new intermediate certificates should not use it.

As a temporary solution to Bug #982292 a hack was added to treat the SGC EKU as "TLS Web Server Authentication" in certificates that contain an X.509v3 basicConstraints extension with the cA boolean set to true. 

This bug request that checks be added to alert users/CAs when they try to use a new intermediate certificate with the obsolete SGC EKU.

Comment 1

[Kathleen Wilson]

From https://bugzilla.mozilla.org/show_bug.cgi?id=982292#c17
> The issuance check idea assumes that the notBefore date won't be backdated. 
> Backdating intermediates is not uncommon.  Backdating cross-certificates is
> very common.

This request is meant to help CAs by alerting them that they are using an obsolete EKU when they create and test a new intermediate certificate. 

Of course, you are correct that it won't help CAs who backdate their new intermediate certs.
https://wiki.mozilla.org/CA:Problematic_Practices#Backdating_the_notBefore_date

Comment 2

[Gervase Markham [:gerv]]

We can also make it clear in policy that "TLS Web Server Authentication" must be present. Although I'm not sure why anyone would want to leave it out today.

Gerv

Comment 3

[Rob Stradling]

(In reply to Kathleen Wilson from comment #1)
<snip>
> This request is meant to help CAs by alerting them that they are using an
> obsolete EKU when they create and test a new intermediate certificate. 
> 
> Of course, you are correct that it won't help CAs who backdate their new
> intermediate certs.

OK.  Makes sense.

Comment 4

[Rob Stradling]

(In reply to Gervase Markham [:gerv] from comment #2)
> We can also make it clear in policy that "TLS Web Server Authentication"
> must be present. Although I'm not sure why anyone would want to leave it out
> today.

Gerv, I presume you mean that _if_ the EKU extension is present, then the "TLS Web Server Authentication" OID MUST be included.  That makes sense.

The EKU extension is not required for the "publicly disclosed and audited" option.

Comment 5

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

Attachment: MozReview Request: bug 982932 - only allow Netscape-stepUp to be used for serverAuth for old CA certificates

Review commit: https://reviewboard.mozilla.org/r/51223/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51223/

Comment 6

[:Cykesiopka]

Comment on attachment 8750544 [details]
MozReview Request: bug 982932 - only allow Netscape-stepUp to be used for serverAuth for old CA certificates

https://reviewboard.mozilla.org/r/51223/#review48345

Looks good - r+ assuming the PR_NOT_REACHED thing is fixed.

I'm going to assume the telemetry numbers are good enough for the public PKI, and our response to private PKIs is "regenerate your certs, or change the pref".

::: security/certverifier/CertVerifier.h:43
(Diff revision 1)
> +// * Always match: the step-up OID is considered equivalent to serverAuth
> +// * Match before 23 August 2016: the OID is considered equivalent if the
> +//   certificate's notBefore is before 23 August 2016
> +// * Match before 23 August 2015: similarly, but for 23 August 2015
> +// * Never match: the OID is never considered equivalent to serverAuth
> +enum class NetscapeStepUpPolicy : uint32_t {

To be honest I've always found it strange to have enums that only affect NSSCertDBTrustDomain be defined in CertVerifier.
In my mind, CertVerifier depends on NSSCertDBTrustDomain, but not the other way around.
Maybe I should just think of NSSCertDBTrustDomain as an integral part of CertVerifier.

::: security/certverifier/NSSCertDBTrustDomain.h:66
(Diff revision 1)
>                         UniqueCERTCertList& builtChain,
> +                       NetscapeStepUpPolicy netscapeStepUpPolicy,
>            /*optional*/ PinningTelemetryInfo* pinningTelemetryInfo = nullptr,

Nit: builtChain is sort of like an outparam, so maybe put netscapeStepUpPolicy before it.

::: security/certverifier/NSSCertDBTrustDomain.cpp:956
(Diff revision 1)
> +      return Success;
> +    case NetscapeStepUpPolicy::NeverMatch:
> +      matches = false;
> +      return Success;
> +    default:
> +      PR_NOT_REACHED("We're not handling every NetscapeStepUpPolicy type");

My compiler errors out on this line:
> NSSCertDBTrustDomain.cpp:958:1: error: control reaches end of non-void function [-Werror=return-type]

Maybe use MOZ_FALLTHROUGH_ASSERT(), MOZ_ASSERT() + return Result::FATAL_ERROR_INVALID_STATE, or something else?

We should probably move away from unneeded direct uses of NSPR stuff regardless.

::: security/manager/ssl/tests/unit/test_cert_eku/ee-int-nsSGC-old.pem.certspec:2
(Diff revision 1)
> +issuer:int-nsSGC-old
> +subject:ee-int-nsSGC

Nit: ee-int-nsSGC-old, to match the file name.
Same for the other certs.

::: security/pkix/lib/pkixcheck.cpp:725
(Diff revision 1)
>          // Comodo has issued certificates that require this behavior that don't
> -        // expire until June 2020! TODO(bug 982932): Limit this exception to
> +        // expire until June 2020!

orz

Comment 7

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

Comment on attachment 8750544 [details]
MozReview Request: bug 982932 - only allow Netscape-stepUp to be used for serverAuth for old CA certificates

https://reviewboard.mozilla.org/r/51223/#review48709

Cykesiopka has covered all of my remarks :). r=me with Cykesiopka's comments addressed.

Comment 8

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

https://reviewboard.mozilla.org/r/51223/#review48345

Thanks for the reviews. As for the public vs private pki, I don't see a good way to not enforce this for private roots. I'll keep thinking about it, but I think we'll have to go with telling folks to change the pref or regenerate certs if they're still issuing new CAs that rely on nsSGC rather than serverAuth. We don't actually have telemetry, unfortunately, but as far as we've ever seen, this was only an issue with Comodo, and they've said it won't be an issue going foward.

> To be honest I've always found it strange to have enums that only affect NSSCertDBTrustDomain be defined in CertVerifier.
> In my mind, CertVerifier depends on NSSCertDBTrustDomain, but not the other way around.
> Maybe I should just think of NSSCertDBTrustDomain as an integral part of CertVerifier.

I originally tried to put it in NSSCertDBTrustDomain but ran into #include troubles. Now that you reminded me of this, a forward declaration seems to work just fine, so I went ahead and moved it. In a way, though, NSSCertDBTrustDomain is pretty tightly coupled with CertVerifier.

> Nit: builtChain is sort of like an outparam, so maybe put netscapeStepUpPolicy before it.

Sounds good.

> My compiler errors out on this line:
> > NSSCertDBTrustDomain.cpp:958:1: error: control reaches end of non-void function [-Werror=return-type]
> 
> Maybe use MOZ_FALLTHROUGH_ASSERT(), MOZ_ASSERT() + return Result::FATAL_ERROR_INVALID_STATE, or something else?
> 
> We should probably move away from unneeded direct uses of NSPR stuff regardless.

Good call. I went with MOZ_FALLTHROUGH_ASSERT + FATAL_ERROR_LIBRARY_FAILURE, although I'm not sure the macro is quite the right thing.

> Nit: ee-int-nsSGC-old, to match the file name.
> Same for the other certs.

Ah, good catch.

> orz

Eh?

Comment 9

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

Comment on attachment 8750544 [details]
MozReview Request: bug 982932 - only allow Netscape-stepUp to be used for serverAuth for old CA certificates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51223/diff/1-2/

Comment 10

[:Cykesiopka]

https://reviewboard.mozilla.org/r/51223/#review48345

OK, SGTM.

> Eh?

https://en.wikipedia.org/wiki/Emoticon#Orz
> This stick figure represents failure and despair.
In this case anyways.

Comment 11

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

Comment on attachment 8750544 [details]
MozReview Request: bug 982932 - only allow Netscape-stepUp to be used for serverAuth for old CA certificates

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51223/diff/2-3/

Comment 12

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

(In reply to :Cykesiopka from comment #10)
> https://en.wikipedia.org/wiki/Emoticon#Orz
> > This stick figure represents failure and despair.
> In this case anyways.

Ah, I see.

I had to fiddle around with the right macro and placement of the return, but I eventually got the compilers to be happy with the code, I think: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4890649f078

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b2fb1aabf14

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b2fb1aabf14

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

A run of the TLS canary on 290k top sites indicated no unique failures for this change.

Comment 16

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

Would it be possible (or did you already?) to run again with the pref "security.pki.netscape_step_up_policy" set to 3? Thanks!

Comment 17

[Matt Wobensmith [:mwobensmith][:matt:]]

Oh, darn. I missed that pref, thought the change was on by default. Sorry.

I ran with the new setting, and we have some breakage - 64 sites.

https://tlscanary.mozilla.org/runs/2016-05-18-11-49-18/

To see just errors for this change, click on "sec_error_inadequate_cert_type" and choose "show only".

Also, for some reason, my script does not flag these errors as certificate-related, so it did not capture these sites' end-entity certs. Therefore, it doesn't display the validityNotBefore field. If we need to know that information, tell me and I'll obtain it.

Comment 18

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: netscape_step_up_broken_sites.txt

Broken sites from Alexa top 1 million, sorted by site ranking

Comment 19

[Matt Wobensmith [:mwobensmith][:matt:]]

(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #17)

> Therefore, it doesn't display the validityNotBefore field. If we need to
> know that information, tell me and I'll obtain it.

Correction... if we need to capture the intermediate certs that these sites' certs chain to - in order to examine their issuance dates - I can do that.

Comment 20

[Kathleen Wilson]

(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #19)
> Correction... if we need to capture the intermediate certs that these sites'
> certs chain to - in order to examine their issuance dates - I can do that.

Yes, please provide the issuance dates of the intermediate certs. 
We're looking for which CAs are still doing this, so we can have the necessary discussion with them.

Comment 21

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

I did a quick scan of the sites in attachment 8754155 [details]. It looks like the problematic issuers were these:

https://crt.sh/?id=164
https://crt.sh/?id=211
https://crt.sh/?id=212
https://crt.sh/?id=213
https://crt.sh/?id=3350
https://crt.sh/?id=445

none of which have a recent notBefore, so I think we're good here (the change is on by default - setting the pref to 3 enables strict enforcement, which we're not actually going to ship by default. I wanted to see how much usage of these old intermediates there still was, as well as if there were any more recent problematic intermediates, but there don't seem to be.)

Comment 22

[Rob Stradling]

Dana, as it happens I did a full scan of the crt.sh DB a couple of days ago for Kathleen:

https://docs.google.com/spreadsheets/d/16kZOVojxc-BcV-nneWvTXNtu2QZ013dS_GBL4U2Wgu0/edit?usp=sharing

The conclusion is still "none of which have a recent notBefore".