Cleanup HPKP error handling and weak DH key handling

Cleanup HPKP error handling and weak DH key handling

HPKP and WeakDH went through special-case logic in
URLRequestHttpJob to map them 'as if' they were cert
verification errors. However, the WeakDH code was
partially reverted, leaving half-implemented code
around, and the HPKP code added an unnecessary
conditional.

This cleans up the WeakDH exception by removing it
as a cert verification error (because it wasn't, and
the CertStatus flag was reverted) and leaving it as
a standard net-error (which is intrinsically
non-bypassable), and removes the associated error
page that wasn't.

This cleans up the HPKP exception by removing the
dead methods on TransportSecurityState and removing
the conditional from the URLRequestHttpJob, because
it's duplicating the logic that the TSS already
handles.

This moves the HPKP fixing up of the cert verification
result from the HttpJob and into the SSL/QUIC socket
code bits, which is more correct anyways, as they
handle certificate verification, and HPKP belongs
close to cert verification anyways. This portion of
code was an artifact of when HPKP was evaluated on
the URLRequestHttpJob, which it hasn't been for over
four years.

BUG=none
R=davidben@chromium.org

Committed: https://crrev.com/9545d3478138019ef62b9c9fd768766725577452
Cr-Commit-Position: refs/heads/master@{#400880}

[Ryan Sleevi, 2016-06-20 23:59:28]

Emily: FYI
David: I was going to put this in the RequireCT code, but it seems sufficiently
distinct to just put in its own. It's random bits of cleanup I noticed along the
way

[Ryan Sleevi, 2016-06-21 00:11:45]

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

[Ryan Sleevi, 2016-06-21 00:11:46]

And since I'll need Emily's review for //components/ssl_errors, I should add her

Emily: This code was half-completed and never landed (because it crashed,
because there's no cert when this error triggered)

The actual UI for this page is in
//components/error_page/common/localized_error.cc - which does the needful and
shows the LearnMore link

[davidben, 2016-06-21 00:14:11]

davidben@chromium.org changed reviewers:
- estark@chromium.org

[davidben, 2016-06-21 00:14:12]

lgtm with comments

https://codereview.chromium.org/2084783002/diff/1/components/ssl_errors/error...
File components/ssl_errors/error_info.cc (left):

https://codereview.chromium.org/2084783002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_info.cc:135: details =
l10n_util::GetStringFUTF16(IDS_CERT_ERROR_WEAK_KEY_DETAILS,
Remove this string? (Huh. Given the text of this string, I rather suspect
someone was confused about what weak DH group was. The text says it's in the
certificate, but it's not.)

https://codereview.chromium.org/2084783002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_info.cc:138:
l10n_util::GetStringUTF16(IDS_CERT_ERROR_WEAK_KEY_DESCRIPTION);
Ditto.

https://codereview.chromium.org/2084783002/diff/1/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_impl.cc (right):

https://codereview.chromium.org/2084783002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_impl.cc:1339:
server_cert_verify_result_.cert_status |= CERT_STATUS_PINNED_KEY_MISSING;
Think it's worth adding a line to SSLClientSocketTest.PKPEnforced?

https://codereview.chromium.org/2084783002/diff/1/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_impl.cc:1340: result =
ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN;
[Confirmed these are the two places in //net which produce this error.]

https://codereview.chromium.org/2084783002/diff/1/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (left):

https://codereview.chromium.org/2084783002/diff/1/net/url_request/url_request...
net/url_request/url_request_http_job.cc:1087: result ==
ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN) {
To confirm, if result is ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN, fatal switches
from true to ShouldSSLErrorsBeFatal. But I believe that's fine because
ShouldSSLErrorsBeFatal is always true if there is a pin. Is that your reasoning
as well?

[davidben, 2016-06-21 00:15:34]

(I assume you've confirmed with https://dh480.badssl.com/ that that error still
works. I'll go try to find another reviewer for the DHE removal because the
current policy reviewer is being slow...)

[Ryan Sleevi, 2016-06-21 00:23:04]

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

[Ryan Sleevi, 2016-06-21 00:23:05]

Somehow lost estark as a reviewer

https://codereview.chromium.org/2084783002/diff/1/components/ssl_errors/error...
File components/ssl_errors/error_info.cc (left):

https://codereview.chromium.org/2084783002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_info.cc:135: details =
l10n_util::GetStringFUTF16(IDS_CERT_ERROR_WEAK_KEY_DETAILS,
On 2016/06/21 00:14:11, davidben wrote:
> Remove this string? (Huh. Given the text of this string, I rather suspect
> someone was confused about what weak DH group was. The text says it's in the
> certificate, but it's not.)

It's used right above :)

https://codereview.chromium.org/2084783002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_info.cc:138:
l10n_util::GetStringUTF16(IDS_CERT_ERROR_WEAK_KEY_DESCRIPTION);
On 2016/06/21 00:14:11, davidben wrote:
> Ditto.

Ditto :)

https://codereview.chromium.org/2084783002/diff/1/net/url_request/url_request...
File net/url_request/url_request_http_job.cc (left):

https://codereview.chromium.org/2084783002/diff/1/net/url_request/url_request...
net/url_request/url_request_http_job.cc:1087: result ==
ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN) {
On 2016/06/21 00:14:12, davidben wrote:
> To confirm, if result is ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN, fatal switches
> from true to ShouldSSLErrorsBeFatal. But I believe that's fine because
> ShouldSSLErrorsBeFatal is always true if there is a pin. Is that your
reasoning
> as well?

Correct. Tried to explain this in the description, but happy to take wording
changes if you feel it's unclear (feel free to just edit it directly)

[davidben, 2016-06-21 00:27:52]

still lgtm

https://codereview.chromium.org/2084783002/diff/1/components/ssl_errors/error...
File components/ssl_errors/error_info.cc (left):

https://codereview.chromium.org/2084783002/diff/1/components/ssl_errors/error...
components/ssl_errors/error_info.cc:135: details =
l10n_util::GetStringFUTF16(IDS_CERT_ERROR_WEAK_KEY_DETAILS,
On 2016/06/21 00:23:04, Ryan Sleevi wrote:
> On 2016/06/21 00:14:11, davidben wrote:
> > Remove this string? (Huh. Given the text of this string, I rather suspect
> > someone was confused about what weak DH group was. The text says it's in the
> > certificate, but it's not.)
> 
> It's used right above :)

...wow, I'm blind. Also apparently I get confused by codesearch's UI. :-)

[estark, 2016-06-21 00:29:24]

components/ssl_errors lgtm (one question inline though)

https://codereview.chromium.org/2084783002/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2084783002/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1087: TransportSecurityState* state =
context->transport_security_state();
Is this guaranteed to be non-null? The old code had a check for null |state|
before using it.

[Ryan Sleevi, 2016-06-21 00:39:04]

https://codereview.chromium.org/2084783002/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2084783002/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1087: TransportSecurityState* state =
context->transport_security_state();
On 2016/06/21 00:29:24, estark wrote:
> Is this guaranteed to be non-null? The old code had a check for null |state|
> before using it.

Yes. See the CL this depends on

In particular,
https://codereview.chromium.org/2067843003/diff/270001/net/socket/ssl_client_...
and
https://codereview.chromium.org/2067843003/diff/270001/net/quic/crypto/proof_...

TransportSecurityState's actually been required for quite some time -
specifically, since
https://chromium.googlesource.com/chromium/src/+/b1c988bcd7869765e1bd56e59278...

This was just an artifact that wasn't caught during that cleanup.

[estark, 2016-06-21 00:49:06]

https://codereview.chromium.org/2084783002/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/2084783002/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:1087: TransportSecurityState* state =
context->transport_security_state();
On 2016/06/21 00:39:04, Ryan Sleevi wrote:
> On 2016/06/21 00:29:24, estark wrote:
> > Is this guaranteed to be non-null? The old code had a check for null |state|
> > before using it.
> 
> Yes. See the CL this depends on
> 
> In particular,
>
https://codereview.chromium.org/2067843003/diff/270001/net/socket/ssl_client_...
> and
>
https://codereview.chromium.org/2067843003/diff/270001/net/quic/crypto/proof_...
> 
> TransportSecurityState's actually been required for quite some time -
> specifically, since
>
https://chromium.googlesource.com/chromium/src/+/b1c988bcd7869765e1bd56e59278...
> 
> This was just an artifact that wasn't caught during that cleanup.

Ahh, I see, I missed that that had landed. Thanks. Still lgtm.

[Ryan Sleevi, 2016-06-21 01:38:24]

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

[commit-bot: I haz the power, 2016-06-21 01:38:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084783002/40001

[commit-bot: I haz the power, 2016-06-21 02:09:19]

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

[commit-bot: I haz the power, 2016-06-21 02:09:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ryan Sleevi, 2016-06-21 02:17:19]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2016-06-21 02:17:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, estark@chromium.org
Link to the patchset: https://codereview.chromium.org/2084783002/#ps60001
(title: "Fix bad test")

[commit-bot: I haz the power, 2016-06-21 02:17:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2084783002/60001

[commit-bot: I haz the power, 2016-06-21 03:17:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-21 03:20:04]

Description was changed from

==========
Cleanup HPKP error handling and weak DH key handling

HPKP and WeakDH went through special-case logic in
URLRequestHttpJob to map them 'as if' they were cert
verification errors. However, the WeakDH code was
partially reverted, leaving half-implemented code
around, and the HPKP code added an unnecessary
conditional.

This cleans up the WeakDH exception by removing it
as a cert verification error (because it wasn't, and
the CertStatus flag was reverted) and leaving it as
a standard net-error (which is intrinsically
non-bypassable), and removes the associated error
page that wasn't.

This cleans up the HPKP exception by removing the
dead methods on TransportSecurityState and removing
the conditional from the URLRequestHttpJob, because
it's duplicating the logic that the TSS already
handles.

This moves the HPKP fixing up of the cert verification
result from the HttpJob and into the SSL/QUIC socket
code bits, which is more correct anyways, as they
handle certificate verification, and HPKP belongs
close to cert verification anyways. This portion of
code was an artifact of when HPKP was evaluated on
the URLRequestHttpJob, which it hasn't been for over
four years.

BUG=none
R=davidben@chromium.org
==========

to

==========
Cleanup HPKP error handling and weak DH key handling

HPKP and WeakDH went through special-case logic in
URLRequestHttpJob to map them 'as if' they were cert
verification errors. However, the WeakDH code was
partially reverted, leaving half-implemented code
around, and the HPKP code added an unnecessary
conditional.

This cleans up the WeakDH exception by removing it
as a cert verification error (because it wasn't, and
the CertStatus flag was reverted) and leaving it as
a standard net-error (which is intrinsically
non-bypassable), and removes the associated error
page that wasn't.

This cleans up the HPKP exception by removing the
dead methods on TransportSecurityState and removing
the conditional from the URLRequestHttpJob, because
it's duplicating the logic that the TSS already
handles.

This moves the HPKP fixing up of the cert verification
result from the HttpJob and into the SSL/QUIC socket
code bits, which is more correct anyways, as they
handle certificate verification, and HPKP belongs
close to cert verification anyways. This portion of
code was an artifact of when HPKP was evaluated on
the URLRequestHttpJob, which it hasn't been for over
four years.

BUG=none
R=davidben@chromium.org

Committed: https://crrev.com/9545d3478138019ef62b9c9fd768766725577452
Cr-Commit-Position: refs/heads/master@{#400880}
==========

[commit-bot: I haz the power, 2016-06-21 03:20:05]

Patchset 4 (id:??) landed as
https://crrev.com/9545d3478138019ef62b9c9fd768766725577452
Cr-Commit-Position: refs/heads/master@{#400880}