Closed Bug 968817 Opened 10 years ago Closed 9 years ago

Collect telemetry on how many server TLS certificates use EKU (and the TLS Server Authentication EKU)

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: gerv, Assigned: rbarnes)

References

Details

Attachments

(2 files, 3 obsolete files)

http://dxr.mozilla.org/mozilla-central/source/security/nss/lib/certdb/certdb.c#l489
shows how we decide whether to accept a cert for TLS.

The current logic is, I believe:

SSL_Server   == !(NS_Type_Extension || EKU_Extension)        // 608-621
                || NS_Type_SSL_Server                        // 516
                || !BC_isCA && (
                  EKU_Server_Auth                            // 553-562
                  || NS_Govt_Approved                        // 563-576
                )

(See https://cabforum.org/pipermail/public/2013-August/002126.html )

I propose that we change NSS to only accept for TLS connections certs which are not CA certs and which have the TLS Web Server Authentication EKU, perhaps with some warning to CAs and/or a period of UI warnings to ease the transition.

Background: There is currently no consensus on which certs are covered by the CAB Forum BRs. This is partly because different browsers have different rules for what they'll accept for a TLS connection. Do we cover AnyEKU certs, for example? Covering them with the Requirements pulls in lots of certs not intended for TLS. But not covering them leaves a big hole if we accept them in the code.

I have been given access to a database of certs from Certificate Transparency. Here is the data I have retrieved:

Total certs in DB:                8526519
Certs without TLS Web Server EKU:    4149 (0.05%)

That seems like a pretty good start.

Of those:

Netscape SGC:  408
Microsoft SGC: 267
Combined SGC:  409 (i.e. only one MS-only SGC cert)

So if we also recognised these, we'd accept an additional 0.005% of certs. That doesn't seem worth it to me.

IPSec Protection: 2687 (OID 1.3.6.1.5.5.8.2.2) - almost always alone

I suspect these are people running VPNs on the SSL port. We should ignore these.

TLS Web Client EKU: 740 (251 alone)
Any EKU: 3
No EKU: None (as far as I can see)

FYI, I attach a dump of all the EKU values which do not contain "TLS Web Server Authentication". And below is an aggregated list of all the EKU sets with 10 or more certs.

Gerv


 10 (critical) 2.5.29.37.0
 12 E-mail Protection, TLS Web Client Authentication, Microsoft Encrypted File System, Microsoft Server Gated Crypto, Netscape Server Gated Crypto
 15 (critical) TLS Web Client Authentication 
 18 Code Signing 
 19 2.16.124.113600.1 
 23 TLS Web Client Authentication, E-mail Protection
 27 (critical) E-mail Protection
 34 2.5.29.37.0
 38 (critical) Code Signing 
 39 Microsoft Encrypted File System, E-mail Protection, TLS Web Client Authentication 
 60 1.2.840.113635.100.4.4
 65 Microsoft Encrypted File System
 89 Netscape Server Gated Crypto, Microsoft Server Gated Crypto 
135 Netscape Server Gated Crypto
163 Microsoft Server Gated Crypto, Netscape Server Gated Crypto 
251 TLS Web Client Authentication 
348 Microsoft Trust List Signing, Microsoft Encrypted File System, E-mail Protection, TLS Web Client Authentication 
2671 1.3.6.1.5.5.8.2.2 

1.3.6.1.5.5.8.2.2 is an IPSec OID. 1.2.840.113635.100.4.4 is something Apple-y.
(In reply to Gervase Markham [:gerv] from comment #0)
> I propose that we change NSS to only accept for TLS connections certs which
> are not CA certs and which have the TLS Web Server Authentication EKU,

This is what the insanity::pkix mode of PSM's CertVerifier does as of bug 878932. It won't let CA certs be validated as end-entity certificates, it won't accept any certificate that contains the id-kp-OCSPSigning EKU at all except for when it validates an OCSP response signer certificate, and it requires the id-kp-serverAuth EKU if there is any EKU extension in the certificate.

The question is whether this should be enforced by the certificate verifier (which may be outside of NSS), by libssl itself, or both. I suspect that due to the compatibility concerns, we should enforce it in the certificate verifier. Also, because libssl could be (and is) used for non-HTTPS connections, it doesn't make sense for libssl to enforce the EKU requirement given that the EKU depends on the application protocol.

Are you suggesting that we make it mandatory that the id-kp-serverAuth EKU be present; i.e. reject certificates without an EKU extension? I think that is too much of a compatibility issue and also non-standard according to RFC 5280.

> Background: There is currently no consensus on which certs are covered by
> the CAB Forum BRs. This is partly because different browsers have different
> rules for what they'll accept for a TLS connection. Do we cover AnyEKU
> certs, for example? Covering them with the Requirements pulls in lots of
> certs not intended for TLS. But not covering them leaves a big hole if we
> accept them in the code.

Yes, of course anyEKU is covered, and so is a missing EKU, because according to RFC 5280 those certificates are valid for server authentication. (Note: insanity::pkix doesn't support anyEKU yet, and neither does NSS, IIRC.)


> I have been given access to a database of certs from Certificate
> Transparency. Here is the data I have retrieved:
> 
> Total certs in DB:                8526519
> Certs without TLS Web Server EKU:    4149 (0.05%)
> 
> That seems like a pretty good start.

This is not the right way to partition the dataset. We need to distinguish between missing EKU, EKU with id-kp-serverAuth, EKU without id-kp-serverAuth.

Note that I rearranged your list a little bit.

>  10 (critical) 2.5.29.37.0 [briansmith: anyExtendedKeyUsage]
>  34 2.5.29.37.0 [briansmith: anyExtendedKeyUsage]

These are acceptable as far as RFC 5280 is concerned, though we won't support them, AFAICT.

>  12 E-mail Protection, TLS Web Client Authentication, Microsoft Encrypted
> File System, Microsoft Server Gated Crypto, Netscape Server Gated Crypto

This is not acceptable because it is missing both anyEKU and id-kp-serverAuth.

>  15 (critical) TLS Web Client Authentication 
>  18 Code Signing 
>  19 2.16.124.113600.1 
>  23 TLS Web Client Authentication, E-mail Protection
>  27 (critical) E-mail Protection
>  38 (critical) Code Signing 
>  39 Microsoft Encrypted File System, E-mail Protection, TLS Web Client
> Authentication 
>  60 1.2.840.113635.100.4.4
>  65 Microsoft Encrypted File System
>  89 Netscape Server Gated Crypto, Microsoft Server Gated Crypto 
> 135 Netscape Server Gated Crypto
> 163 Microsoft Server Gated Crypto, Netscape Server Gated Crypto 
> 251 TLS Web Client Authentication 
> 348 Microsoft Trust List Signing, Microsoft Encrypted File System, E-mail
> Protection, TLS Web Client Authentication 
> 2671 1.3.6.1.5.5.8.2.2 
> 
> 1.3.6.1.5.5.8.2.2 is an IPSec OID. 1.2.840.113635.100.4.4 is something
> Apple-y.

None of these are SSL server EKUs.
Also, as far as the summary of the bug is worded (require the EKU extension, and require that it contain id-kp-serverAuth, my vote is that this bug be RESOLVED INVALID. Instead, we should use the RFC 5280 semantics for determining acceptable EKUs and for dealing with missing EKUs.
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #1)
> Are you suggesting that we make it mandatory that the id-kp-serverAuth EKU
> be present; i.e. reject certificates without an EKU extension? I think that
> is too much of a compatibility issue and also non-standard according to RFC
> 5280.

I am suggesting that. Where do you find it non-standard in RFC 5280? I read (section 4.2.1.2):

  "Certificate using
   applications MAY require that the extended key usage extension be
   present and that a particular purpose be indicated in order for the
   certificate to be acceptable to that application."

and also, specifically:

  "Applications that require the presence of a
   particular purpose MAY reject certificates that include the
   anyExtendedKeyUsage OID but not the particular OID expected for the
   application."

> Yes, of course anyEKU is covered, and so is a missing EKU, because according
> to RFC 5280 those certificates are valid for server authentication. (Note:
> insanity::pkix doesn't support anyEKU yet, and neither does NSS, IIRC.)

Really? That is interesting.

> > I have been given access to a database of certs from Certificate
> > Transparency. Here is the data I have retrieved:
> > 
> > Total certs in DB:                8526519
> > Certs without TLS Web Server EKU:    4149 (0.05%)
> > 
> > That seems like a pretty good start.
> 
> This is not the right way to partition the dataset. We need to distinguish
> between missing EKU, EKU with id-kp-serverAuth, EKU without id-kp-serverAuth.

Maybe I wasn't doing the right queries, but I couldn't find any certs in the data set with missing EKU. So the partitioning above is between those with EKU and id-kp-serverAuth (all except 4149) and those with EKU but without id-kp-serverAuth (4149).

Gerv
I think we should put some telemetry in place which would tell us how often we accept certificates which don't have id-kp-serverAuth, both end-entity certs and intermediate certs (with separate metrics). Then we can try and work out whether changing to use this restriction is going to break anything.

The significant advantage of doing this is that (I believe) there are loads of certs out there which are not intended for SSL use but have no EKU. Under the current rules, they would be accepted. And the issuance policies for such certs are not necessarily BR-compliant, or aware of the risks.

Gerv
(In reply to Gervase Markham [:gerv] from comment #3)
> I am suggesting that. Where do you find it non-standard in RFC 5280? I read
> (section 4.2.1.2):
> 
>   "Certificate using
>    applications MAY require that the extended key usage extension be
>    present and that a particular purpose be indicated in order for the
>    certificate to be acceptable to that application."
> 
> and also, specifically:
> 
>   "Applications that require the presence of a
>    particular purpose MAY reject certificates that include the
>    anyExtendedKeyUsage OID but not the particular OID expected for the
>    application."

Good. Thanks for pointing this out. Based on this, I agree that it is worth trying your suggestion to require an explicit serverAuth EKU for SSL. However, let's do it one release after we turn on insanity::pkix to reduce risk of incompatibilities during the switchover.
Yes - I'm very happy to make sure we don't entangle this with insanity-related issues. It can go in the queue behind that.

Gerv
Note Bug #982292 and Bug #982932 regarding the Netscape SGC EKU.

Also, Let's make sure we do compatibility testing for this change.
We've now turned on insanity; is it reasonable to proceed with this? rbarnes and/or bsmith: do you have a view?

Gerv
Flags: needinfo?(rlb)
Flags: needinfo?(brian)
How about getting telemetry on this first? as per Comment #4.
I was under the impression that this was already begin done:
"When the EKU extension is specified, must assert the serverAuth bit."
<https://wiki.mozilla.org/SecurityEngineering/mozpkix-testing#Behavior_Changes>

I guess that lets certs without the EKU extension slip by.  But this doesn't really seem like a major security issue.
Flags: needinfo?(rlb)
I'm happy to let others deal with this now.
Flags: needinfo?(brian)
(In reply to Richard Barnes [:rbarnes] from comment #10)
> I guess that lets certs without the EKU extension slip by.  But this doesn't
> really seem like a major security issue.

Actually, that's the purpose of this bug. There are many certs out there which are not intended for SSL; accepting certs without an EKU means we will accept these certs for SSL. We should not do so. The data above suggests that the compatibility concern is near zero - certainly less than the concern associated with requiring the serverAuth EKU when EKU is present, which we already do anyway (as Brian pointed out in comment 1).

Kathleen suggested getting telemetry - we could do that, but we already know that for publicly-visible certs in the CT database, we could not find a single one out of millions which didn't have EKU.

The big advantage of doing this is that once we've done it, we have a clear and well-defined scope for what we consider an "web PKI SSL certificate" - that is, it has id-kpServerAuth. As other vendors follow along, we can then also define the scope of the CAB Forum's work to be that narrow, which will prevent CAB Forum standards covering a load of certificates which they are not intended to cover. That scope ambiguity causes problems when trying to write policy and satisfy auditors.

Gerv
(In reply to Gervase Markham [:gerv] from comment #12)
> Kathleen suggested getting telemetry - we could do that, but we already know
> that for publicly-visible certs in the CT database, we could not find a
> single one out of millions which didn't have EKU.

If we've learned anything from the mozilla::pkix roll-out, it's that surveys of public certs are at best a rough metric.  There are a lot of certs out there that users encounter in other ways.  I would support adding telemetry for this, but not flipping the switch quite yet.
OK. rbarnes: how do we go about getting such telemetry? Do we need to file a bug somewhere else?

Gerv
Flags: needinfo?(rlb)
Summary: Only accept certs for server TLS which have the TLS Server Authentication EKU → Only accept certs for server TLS which use EKU (and which assert the TLS Server Authentication EKU)
(In reply to Gervase Markham [:gerv] from comment #14)
> OK. rbarnes: how do we go about getting such telemetry? Do we need to file a
> bug somewhere else?

You would add a telemetry probe here, I think:
http://dxr.mozilla.org/mozilla-central/source/security/pkix/lib/pkixcheck.cpp#687

It'll be a short patch.  Let's just use this bug, and I'll try to get to it soon.
Flags: needinfo?(rlb)
rbarnes: any news on when we might get that probe?

Gerv
Flags: needinfo?(rlb)
Attached patch bug-968817.patch (obsolete) — Splinter Review
This patch adds telemetry for EKUs under SSL_SERVER_AUTH_EKU.  It logs four cases:
0. No EKU extension is present
1. EKU extension present + contains server auth EKU
2. EKU extension present + contains other EKU
3. EKU extension present + contains both server auth EKU and other(s)

It only logs these for cert chains that are rooted in built-in roots.

Gerv: Does that set of data look sufficient for you?
Flags: needinfo?(rlb)
Attachment #8519214 - Flags: review?(dkeeler)
Attachment #8519214 - Flags: feedback?(gerv)
* Does this telemetry apply only to end-entity certs?

* Does this data get gathered before we decide to accept or reject the cert? Because certs in categories 0 and 2 should not be accepted by mozilla::pkix.

As long as the answers are Yes and Yes, that all sounds great.

Gerv
Comment on attachment 8519214 [details] [diff] [review]
bug-968817.patch

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

In general this looks good, but I noticed a few issues that should be addressed. In particular, we won't be able to gather telemetry for cases where we would fail to validate the certificate because of an inadequate EKU extension (currently bucket 2 in the histogram). We may not need to, but we at least shouldn't make it look like we do.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +967,5 @@
> +{
> +  CERTCertListNode* endEntityNode = CERT_LIST_HEAD(certList);
> +  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> +  PR_ASSERT(endEntityNode && rootNode);
> +  if (!endEntityNode || !rootNode) {

I think the conditions we actually want to test here are CERT_LIST_END(endEntityNode, certList) / CERT_LIST_END(rootNode, certList) (if I understand correctly, CERT_LIST_HEAD/TAIL(certList) will return a non-null pointer even for an empty list, since they're backed by circular lists).

@@ +985,5 @@
> +    // Find the EKU extension, if present
> +    bool foundEKU = false;
> +    SECOidTag oidTag;
> +    CERTCertExtension* ekuExtension = nullptr;
> +    for (int i=0; endEntityCert->extensions[i] != nullptr; i++) {

nit: 'size_t i = 0;'
Also, I think the style is to omit the '!= nullptr' here.

@@ +999,5 @@
> +      return;
> +    }
> +
> +    // Parse the EKU extension
> +    CERTOidSequence *ekuSequence = CERT_DecodeOidSequence(&ekuExtension->value);

nit 'CERTOidSequence* ekuSequence'
Also, we should define a scoped type for this (see e.g. http://hg.mozilla.org/mozilla-central/annotate/a926116946f8/security/manager/ssl/src/nsCrypto.cpp#l15 - use CERT_DestroyOidSequence)

@@ +1016,5 @@
> +        foundOther = true;
> +      }
> +    }
> +
> +    if (foundServerAuth && !foundOther) {

Let's sort these in order of what we'd accept to what we would reject. If I understand correctly, we currently accept no EKU at all (handled above), (foundServerAuth && !foundOther), and (foundServerAuth && foundOther), whereas we wouldn't accept (!foundServerAuth && foundOther) and (!foundServerAuth && !foundOther), right?
(So, actually, as it turns out, we'll never collect telemetry for the reject cases, because we would never have a successful validation at the point that this code runs.)

@@ +1025,5 @@
> +      Telemetry::Accumulate(Telemetry::SSL_SERVER_AUTH_EKU, 3);
> +    }
> +  }
> +}
> +

nit: unnecessary extra blank line

@@ +1049,5 @@
>  
>  // There are various things that we want to measure about certificate
>  // chains that we accept.  This is a single entry point for all of them.
>  void
>  GatherSuccessfulValidationTelemetry(const ScopedCERTCertList& certList)

Does this patch depend on another patch? This function doesn't seem to exist in the source code yet.

::: toolkit/components/telemetry/Histograms.json
@@ +6518,5 @@
> +  "SSL_SERVER_AUTH_EKU": {
> +    "expires_in_version": "never",
> +    "kind": "enumerated",
> +    "n_values": 10,
> +    "description": "Presence of of the Server Authenticaton EKU in accepted SSL server certificates (0=No EKU, 1=EKU present and has id_kp_serverAuth, 2=EKU extension missing id_kp_serverAuth)"

This description doesn't include what bucket 3 means.
Attachment #8519214 - Flags: review?(dkeeler) → review-
Comment on attachment 8519214 [details] [diff] [review]
bug-968817.patch

dkeeler seems to be addressing the technical issues; I already gave feedback further up.

Gerv
Attachment #8519214 - Flags: feedback?(gerv) → feedback-
Attached patch bug-968817.1.patch (obsolete) — Splinter Review
(In reply to David Keeler (:keeler) [use needinfo?] from comment #19)
> Comment on attachment 8519214 [details] [diff] [review]
> bug-968817.patch
> 
> Review of attachment 8519214 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general this looks good, but I noticed a few issues that should be
> addressed. In particular, we won't be able to gather telemetry for cases
> where we would fail to validate the certificate because of an inadequate EKU
> extension (currently bucket 2 in the histogram). We may not need to, but we
> at least shouldn't make it look like we do.

As I noted above to Gerv, I don't think it's a bad thing that we don't measure certs that we won't accept.  The fix to this bug would be purely restrictive, so we're trying to measure how many certs would change from being accepted to being rejected.  

I would propose that we keep category 2 just for completeness (bins are cheap), but I've added some commentary to indicate that it shouldn't happen.


> ::: security/manager/ssl/src/SSLServerCertVerification.cpp
> @@ +967,5 @@
> > +{
> > +  CERTCertListNode* endEntityNode = CERT_LIST_HEAD(certList);
> > +  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> > +  PR_ASSERT(endEntityNode && rootNode);
> > +  if (!endEntityNode || !rootNode) {
> 
> I think the conditions we actually want to test here are
> CERT_LIST_END(endEntityNode, certList) / CERT_LIST_END(rootNode, certList)
> (if I understand correctly, CERT_LIST_HEAD/TAIL(certList) will return a
> non-null pointer even for an empty list, since they're backed by circular
> lists).

Fixed this both here and in GatherBaselineRequirementsTelemetry (whence it was copied).


> @@ +1016,5 @@
> > +        foundOther = true;
> > +      }
> > +    }
> > +
> > +    if (foundServerAuth && !foundOther) {
> 
> Let's sort these in order of what we'd accept to what we would reject. If I
> understand correctly, we currently accept no EKU at all (handled above),
> (foundServerAuth && !foundOther), and (foundServerAuth && foundOther),
> whereas we wouldn't accept (!foundServerAuth && foundOther) and
> (!foundServerAuth && !foundOther), right?
> (So, actually, as it turns out, we'll never collect telemetry for the reject
> cases, because we would never have a successful validation at the point that
> this code runs.)

I regrouped these as:
0. No EKU
1. EKU with id-kp-serverAuth
2. EKU with id-kp-serverAuth and something else
3. EKU without id-kp-serverAuth


> @@ +1025,5 @@
> > +      Telemetry::Accumulate(Telemetry::SSL_SERVER_AUTH_EKU, 3);
> > +    }
> > +  }
> > +}
> > +
> 
> nit: unnecessary extra blank line
> 
> @@ +1049,5 @@
> >  
> >  // There are various things that we want to measure about certificate
> >  // chains that we accept.  This is a single entry point for all of them.
> >  void
> >  GatherSuccessfulValidationTelemetry(const ScopedCERTCertList& certList)
> 
> Does this patch depend on another patch? This function doesn't seem to exist
> in the source code yet.

Yep, bug 1088255, which is currently inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ac497a25cd8
Assignee: nobody → rlb
Attachment #8519214 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8527980 - Flags: review?(dkeeler)
Attached file bug-968817.0-1.interdiff (obsolete) —
Interdiff for convenience.
Comment on attachment 8527980 [details] [diff] [review]
bug-968817.1.patch

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

Great - LGTM with comments addressed.

::: security/manager/ssl/src/SSLServerCertVerification.cpp
@@ +814,5 @@
>  GatherBaselineRequirementsTelemetry(const ScopedCERTCertList& certList)
>  {
>    CERTCertListNode* endEntityNode = CERT_LIST_HEAD(certList);
>    CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
>    PR_ASSERT(endEntityNode && rootNode);

The assert should also be fixed: PR_ASSERT(!(CERT_LIST_END(...) || CERT_LIST_END(...)));

@@ +969,5 @@
> +GatherEKUTelemetry(const ScopedCERTCertList& certList)
> +{
> +  CERTCertListNode* endEntityNode = CERT_LIST_HEAD(certList);
> +  CERTCertListNode* rootNode = CERT_LIST_TAIL(certList);
> +  PR_ASSERT(endEntityNode && rootNode);

Same here.

@@ +978,5 @@
> +  CERTCertificate* endEntityCert = endEntityNode->cert;
> +
> +  // Only log telemetry if the certificate list is non-empty,
> +  // and the root CA is built-in
> +  if (!CERT_LIST_END(rootNode, certList)) {

We already know rootNode points to a valid certificate at this point, so no need for this if.

@@ +989,5 @@
> +    // Find the EKU extension, if present
> +    bool foundEKU = false;
> +    SECOidTag oidTag;
> +    CERTCertExtension* ekuExtension = nullptr;
> +    for (size_t i=0; endEntityCert->extensions[i]; i++) {

nit: 'size_t i = 0;'

@@ +1003,5 @@
> +      return;
> +    }
> +
> +    // Parse the EKU extension
> +    ScopedCERTOidSequence ekuSequence(CERT_DecodeOidSequence(&ekuExtension->value));

Lines >80 chars should be broken up. You can probably do something like this:

ScopedCERTOidSequence ekuSequence(
  CERT_DecodeOidSequence(&ekuExtension->value));
Attachment #8527980 - Flags: review?(dkeeler) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/672742f81e51
Attachment #8527980 - Attachment is obsolete: true
Attachment #8527982 - Attachment is obsolete: true
Attachment #8528056 - Flags: review+
Summary: Only accept certs for server TLS which use EKU (and which assert the TLS Server Authentication EKU) → Collect telemetry on how many server TLS certificates use EKU (and the TLS Server Authentication EKU)
https://hg.mozilla.org/mozilla-central/rev/672742f81e51
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Component: Libraries → Security: PSM
Product: NSS → Core
Target Milestone: --- → mozilla36
Version: trunk → Trunk
You need to log in before you can comment on or make changes to this bug.