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

RESOLVED FIXED in Firefox 49

Status

()

enhancement
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: kwilson, Assigned: keeler)

Tracking

(Blocks 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

(Whiteboard: [psm-assigned])

Attachments

(2 attachments)

Reporter

Description

5 years ago
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.
Reporter

Comment 1

5 years ago
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
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

5 years ago
(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

5 years ago
(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.
Reporter

Updated

5 years ago
Assignee: nobody → dkeeler
Whiteboard: [psm-assigned]

Comment 6

3 years ago
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
Attachment #8750544 - Flags: review?(cykesiopka.bmo) → review+
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.
Attachment #8750544 - Flags: review?(jjones) → review+
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 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

3 years ago
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 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/
(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 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b2fb1aabf14
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
A run of the TLS canary on 290k top sites indicated no unique failures for this change.
Would it be possible (or did you already?) to run again with the pref "security.pki.netscape_step_up_policy" set to 3? Thanks!
Flags: needinfo?(mwobensmith)
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.
Flags: needinfo?(mwobensmith)
Broken sites from Alexa top 1 million, sorted by site ranking
(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.
Reporter

Comment 20

3 years ago
(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.
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

3 years ago
David, 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".
You need to log in before you can comment on or make changes to this bug.