Record UMA metrics for Must-Staple certificates on private roots

Record UMA metrics for Must-Staple certificates on private roots

We'd like to get an idea of what MITM proxies tend to do with the TLS
Feature Extension (colloquially known as Must-Staple). If MITM proxies
blindly copy the extension into generated certificates, then deploying
Must-Staple will cause a lot of breakage, due to MITM proxies generating
Must-Staple certificates but not stapling OCSP responses.

This CL adds an UMA metric for the presence of the TLS Feature Extension
in certificates that chain to private roots, as a baby step in this
investigation. (Note that this conflates misbehaving MITM proxies with
private PKIs that are using Must-Staple, so it's only a starting point
for an investigation.)

A new asn1::HasTLSFeatureExtension() function is used to record this
histogram.

BUG=633732
Committed: https://crrev.com/2e2e27cf8745fc190864cf182d5467363d59fb13
Cr-Commit-Position: refs/heads/master@{#426971}

[estark, 2016-10-20 23:31:02]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 23:32:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-20 23:52:37]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 23:52:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-20 23:59:58]

estark@chromium.org changed reviewers:
+ eroman@chromium.org

[estark, 2016-10-20 23:59:59]

Eric, could you take a look, please? I might have done this in a completely
wrong way so feel free to tell me to rewrite the whole thing. :)

What I've done here is add a new asn1_util function to parse the TLS Feature
Extension, and I'm calling that at the end of cert verification for certs
chaining to private roots. I did it this way (instead of adding it to
X509Certificate and having platform-specific implementations) because I hear
that there's a desire to move away from the platform-specific X509Certificate
stuff. But, I'm not sure if it's unacceptably expensive to do this extra parsing
with net::der on a relatively large percentage of cert verifications.

(Also cc'ing davidben)

[eroman, 2016-10-21 01:49:23]

This general approach looks reasonable to me.

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.cc
File net/cert/asn1_util.cc (right):

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.cc#n...
net/cert/asn1_util.cc:196: LOG(ERROR) << "No extensions!";
nit: Why add the log statement?

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h
File net/cert/asn1_util.h (right):

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h#ne...
net/cert/asn1_util.h:46: // HasTLSFeatureExtensions parses the DER encoded
certificate in |cert|
typo: HasTLSFeatureExtension

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h#ne...
net/cert/asn1_util.h:48: // (https://tools.ietf.org/html/rfc7633) if present. On
successful
Thanks for including the RFC reference in the comment.

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h#ne...
net/cert/asn1_util.h:49: // return, |*has_tls_feature_extension| to true if the
TLS feature
Can you re-work the wording on this? Feels like something is missing.

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h#ne...
net/cert/asn1_util.h:51: NET_EXPORT_PRIVATE bool
HasTLSFeatureExtension(base::StringPiece cert,
API question: Instead of two bool outputs, can the API be:
   bool HasTLSFeatureExtension(base::StringPiece)

The meaning of false here would now include any failures parsing -- which for
the consumers I don't think is of particular interest, nor for that matter is
likely to be reached.

WDYT?

https://codereview.chromium.org/2436233002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2436233002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:325: if (!asn1::HasTLSFeatureExtension(cert_der,
&has_tls_feature_extension))
Should we be checking more specifically for the presence of status_request  (and
status_request_2) in the feature list?

The assumption here is that if it has a TLS feature extension, that extension
includes status_request.
Which might be a reasonable assumption to make. Can the feature list also be
empty? Or are there values other than status_request that might be in there?

[estark, 2016-10-21 02:10:26]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 02:10:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-21 02:11:29]

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.cc
File net/cert/asn1_util.cc (right):

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.cc#n...
net/cert/asn1_util.cc:196: LOG(ERROR) << "No extensions!";
On 2016/10/21 01:49:22, eroman wrote:
> nit: Why add the log statement?

Doh, sorry, thought I deleted that. It's gone now.

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h
File net/cert/asn1_util.h (right):

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h#ne...
net/cert/asn1_util.h:46: // HasTLSFeatureExtensions parses the DER encoded
certificate in |cert|
On 2016/10/21 01:49:23, eroman wrote:
> typo: HasTLSFeatureExtension

Done.

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h#ne...
net/cert/asn1_util.h:49: // return, |*has_tls_feature_extension| to true if the
TLS feature
On 2016/10/21 01:49:23, eroman wrote:
> Can you re-work the wording on this? Feels like something is missing.

Done.

https://codereview.chromium.org/2436233002/diff/20001/net/cert/asn1_util.h#ne...
net/cert/asn1_util.h:51: NET_EXPORT_PRIVATE bool
HasTLSFeatureExtension(base::StringPiece cert,
On 2016/10/21 01:49:22, eroman wrote:
> API question: Instead of two bool outputs, can the API be:
>    bool HasTLSFeatureExtension(base::StringPiece)
> 
> The meaning of false here would now include any failures parsing -- which for
> the consumers I don't think is of particular interest, nor for that matter is
> likely to be reached.
> 
> WDYT?

Yeah, I think that'll work. Done.

https://codereview.chromium.org/2436233002/diff/20001/net/cert/cert_verify_pr...
File net/cert/cert_verify_proc.cc (right):

https://codereview.chromium.org/2436233002/diff/20001/net/cert/cert_verify_pr...
net/cert/cert_verify_proc.cc:325: if (!asn1::HasTLSFeatureExtension(cert_der,
&has_tls_feature_extension))
On 2016/10/21 01:49:23, eroman wrote:
> Should we be checking more specifically for the presence of status_request 
(and
> status_request_2) in the feature list?
> 
> The assumption here is that if it has a TLS feature extension, that extension
> includes status_request.
> Which might be a reasonable assumption to make. Can the feature list also be
> empty? Or are there values other than status_request that might be in there?

My understanding is that the extension is only used for status_request or
status_request2 in practice (and either of those are interesting for my
purposes), and would only appear when a Must-Staple certificate is explicitly
requested (and thus wouldn't be empty). The RFC is pretty OCSP-focused too. So I
think this is a reasonable assumption to make. I'll add a comment explaining it
though.

[commit-bot: I haz the power, 2016-10-21 02:27:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 02:27:44]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[eroman, 2016-10-21 18:37:37]

lgtm

[estark, 2016-10-21 18:49:35]

estark@chromium.org changed reviewers:
+ holte@chromium.org

[estark, 2016-10-21 18:49:35]

holte, can you please review histograms.xml?

[Steven Holte, 2016-10-21 23:01:37]

histograms lgtm

[estark, 2016-10-21 23:05:06]

The CQ bit was checked by estark@chromium.org

[estark, 2016-10-21 23:05:07]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2436233002/#ps80001
(title: "... and to README")

[commit-bot: I haz the power, 2016-10-21 23:06:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-22 04:33:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-10-22 04:38:01]

Description was changed from

==========
Record UMA metrics for Must-Staple certificates on private roots

We'd like to get an idea of what MITM proxies tend to do with the TLS
Feature Extension (colloquially known as Must-Staple). If MITM proxies
blindly copy the extension into generated certificates, then deploying
Must-Staple will cause a lot of breakage, due to MITM proxies generating
Must-Staple certificates but not stapling OCSP responses.

This CL adds an UMA metric for the presence of the TLS Feature Extension
in certificates that chain to private roots, as a baby step in this
investigation. (Note that this conflates misbehaving MITM proxies with
private PKIs that are using Must-Staple, so it's only a starting point
for an investigation.)

A new asn1::HasTLSFeatureExtension() function is used to record this
histogram.

BUG=633732
==========

to

==========
Record UMA metrics for Must-Staple certificates on private roots

We'd like to get an idea of what MITM proxies tend to do with the TLS
Feature Extension (colloquially known as Must-Staple). If MITM proxies
blindly copy the extension into generated certificates, then deploying
Must-Staple will cause a lot of breakage, due to MITM proxies generating
Must-Staple certificates but not stapling OCSP responses.

This CL adds an UMA metric for the presence of the TLS Feature Extension
in certificates that chain to private roots, as a baby step in this
investigation. (Note that this conflates misbehaving MITM proxies with
private PKIs that are using Must-Staple, so it's only a starting point
for an investigation.)

A new asn1::HasTLSFeatureExtension() function is used to record this
histogram.

BUG=633732

Committed: https://crrev.com/2e2e27cf8745fc190864cf182d5467363d59fb13
Cr-Commit-Position: refs/heads/master@{#426971}
==========

[commit-bot: I haz the power, 2016-10-22 04:38:02]

Patchset 5 (id:??) landed as
https://crrev.com/2e2e27cf8745fc190864cf182d5467363d59fb13
Cr-Commit-Position: refs/heads/master@{#426971}