Treat failure to parse certificates as SSL protocol errors.

Treat failure to parse certificates as SSL protocol errors.

Right now, failure to parse certificates map to ERR_SSL_PROTOCOL_ERROR
(or ERR_FAILED in NSS because our error-mapping logic isn't very good
for either case), however since we use two certificate libraries
together, it's possible for BoringSSL to accept a certificate while
the platform doesn't. (Especially so because BoringSSL is still using
OpenSSL's legacy X.509 stack which is very shoddy. Also Windows will
refuse to parse things like certificates which expire on 0001-01-01.)

Today, this results in ERR_CERT_INVALID (IsCertificateError gives true)
with a NULL X509Certificate, crashing everywhere. Instead, these errors
should be treated the same as if BoringSSL internally rejected the
certificate. Map them to a new error code (for ease of debugging),
ERR_SSL_SERVER_CERT_BAD_FORMAT. IsCertificateError will return false
for this error.

We should now actually maintain the invariant that IsCertificateError
implies there is a certificate available.

The user-visible error page just inherits ERR_SSL_PROTOCOL_ERROR's
strings, as there is no meaningful difference between "BoringSSL
rejected the cert" and "BoringSSL rejected the cert but Windows didn't".
Likewise, this error is unrecoverable, matching ERR_SSL_PROTOCOL_ERROR.

This removes support for using SSLClientSocket in an environment
where X509Certificates cannot be created. With remoting no longer using
the internal plugin, this is no longer necessary.

BUG=91341
Committed: https://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888
Cr-Commit-Position: refs/heads/master@{#343720}

[davidben, 2015-08-11 19:48:56]

davidben@chromium.org changed reviewers:
+ felt@chromium.org, rsleevi@chromium.org

[davidben, 2015-08-11 19:48:57]

rsleevi: Do you have opinions on whether we add a separate error code or just
reuse ERR_SSL_PROTOCOL_ERROR? I figured it'd be good to do a separate one just
so we can tell it happened, but I don't really care.


felt: Adding you since this affects UI, although only in so far as we have to
show some kind of UI rather than crash. :-)

I've intentionally made this share strings with ERR_SSL_PROTOCOL_ERROR because
it's fundamentally the same thing as BoringSSL internally rejecting the cert's
format (currently maps to the generic ERR_SSL_PROTOCOL_ERROR). Just due to
implementation details, we detect two variants of this in very places.

(If there's some desire to give a dedicated UI for "this certificate is so
nonsense that we can't even parse it", we can certainly funnel the more common
case into this error code too and then show UI. But this hasn't come up before
to my knowledge, and I don't think it's really worth calling out this case
separately without also handling the more common one.)

https://drive.google.com/a/chromium.org/folderview?id=0Bz14lOW5Hke4fkpKdEgwVE...

[Ryan Sleevi, 2015-08-11 23:25:25]

Where muh tests at? :D

https://codereview.chromium.org/1286793002/diff/1/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/1286793002/diff/1/net/base/net_error_list.h#n...
net/base/net_error_list.h:351: // not a certificate error code as now
X509Certificate object is available. This
'code as now' -> 'code, as no'

https://codereview.chromium.org/1286793002/diff/1/net/base/net_error_list.h#n...
net/base/net_error_list.h:352: // error is fatal and may not be clicked though.
'fatal and may not be clicked through' -> 'fatal, and may not be overridden.'

[davidben, 2015-08-12 21:40:02]

Added a test at the SSLClientSocket layer.

https://codereview.chromium.org/1286793002/diff/1/net/base/net_error_list.h
File net/base/net_error_list.h (right):

https://codereview.chromium.org/1286793002/diff/1/net/base/net_error_list.h#n...
net/base/net_error_list.h:351: // not a certificate error code as now
X509Certificate object is available. This
On 2015/08/11 23:25:25, Ryan Sleevi wrote:
> 'code as now' -> 'code, as no'

Done.

https://codereview.chromium.org/1286793002/diff/1/net/base/net_error_list.h#n...
net/base/net_error_list.h:352: // error is fatal and may not be clicked though.
On 2015/08/11 23:25:25, Ryan Sleevi wrote:
> 'fatal and may not be clicked through' -> 'fatal, and may not be overridden.'

Done.

[Ryan Sleevi, 2015-08-12 22:03:19]

https://codereview.chromium.org/1286793002/diff/20001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/1286793002/diff/20001/net/data/ssl/certificat...
net/data/ssl/certificates/README:133: Windows refuses to parse this certificate.
It'd be preferable to not manually generate this. Can you add it to one of the
test scripts?

https://codereview.chromium.org/1286793002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1286793002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:1105: #if defined(OS_WIN)
Why only Windows?

Suggest you go for the multi-platform parse failure ;)

8K+ RSA key (OS X), validity range of 0000 to 9999 (Windows). I think the
validity range might also cause NSS to choke. I think an id-rsa-PSS cert might
cause NSS to choke as well, but that'd probably cause BoringSSL to choke.

[davidben, 2015-08-12 22:14:05]

https://codereview.chromium.org/1286793002/diff/20001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/1286793002/diff/20001/net/data/ssl/certificat...
net/data/ssl/certificates/README:133: Windows refuses to parse this certificate.
On 2015/08/12 22:03:19, Ryan Sleevi wrote:
> It'd be preferable to not manually generate this. Can you add it to one of the
> test scripts?

This was generated from the Go program attached in the bug. There's no reason to
ever regenerate it since it will never expire. If you give me an OpenSSL
command-line to generate such a certificate, I'll include it, but I as a general
rule try to avoid that tool. :-P Generating certificates with anything but Go is
just a pain.

https://codereview.chromium.org/1286793002/diff/20001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1286793002/diff/20001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:1105: #if defined(OS_WIN)
On 2015/08/12 22:03:19, Ryan Sleevi wrote:
> Why only Windows?
> 
> Suggest you go for the multi-platform parse failure ;)
> 
> 8K+ RSA key (OS X)

That doesn't work. It's not a parse failure. It parses, but the verifier fails
with a fatal error later. That's why I couldn't use it to repro the crash on my
Mac and why the Mac bug was not a duplicate of this one.

> validity range of 0000 to 9999 (Windows).

Already there.

> I think the
> validity range might also cause NSS to choke.

In that case, how did we ever have this crash pre-BoringSSL? An NSS choke would
not have triggered the crash.

> I think an id-rsa-PSS cert might
> cause NSS to choke as well, but that'd probably cause BoringSSL to choke.

BoringSSL will not accept id-rsa-PSS in the SPKI. It is okay in the signature
algorithms though.

[felt, 2015-08-13 16:42:02]

felt@chromium.org changed reviewers:
- felt@chromium.org

[felt, 2015-08-13 16:42:20]

felt@chromium.org changed reviewers:
+ felt@chromium.org

[felt, 2015-08-13 16:42:21]

this is a nice idea, thanks for the heads up !

[Ryan Sleevi, 2015-08-13 22:54:57]

https://codereview.chromium.org/1286793002/diff/20001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/1286793002/diff/20001/net/data/ssl/certificat...
net/data/ssl/certificates/README:133: Windows refuses to parse this certificate.
On 2015/08/12 22:14:05, David Benjamin wrote:
> On 2015/08/12 22:03:19, Ryan Sleevi wrote:
> > It'd be preferable to not manually generate this. Can you add it to one of
the
> > test scripts?
> 
> This was generated from the Go program attached in the bug. There's no reason
to
> ever regenerate it since it will never expire. 

Eh, that's what we said about the certs until we switched SHA-1 to SHA-256 ;)

> If you give me an OpenSSL
> command-line to generate such a certificate, I'll include it, but I as a
general
> rule try to avoid that tool. :-P Generating certificates with anything but Go
is
> just a pain.

That may be, but it's also the consistent way that multiple developers can
collaborate (especially developers who hate Go :P). 

Load up net/data/ssl/scripts/generate-test-certs.sh . You can see explicit date
assignment at
https://code.google.com/p/chromium/codesearch#chromium/src/net/data/ssl/scrip...
as an example.

You can see explicit name assignment at
https://code.google.com/p/chromium/codesearch#chromium/src/net/data/ssl/scrip...

It uses ASN1_TIME_set_string, which should handle dates even outside the
representation of a time_t

[Ryan Sleevi, 2015-08-14 00:49:32]

Oh, forgot to add an LGTM, since I know (well, trust) you'll clean up after
yourself :)

If the script ends up being a pain, it's fine to go without - just that it
should (hopefully) be a simple thing to add and at least then it's reproducible.
The Windows-only nature of the test makes me a little sad, but having to dig
into the parsing of NSS and OS X (which uses an old version of NSS) is even less
exciting, so I'll live :)

[davidben, 2015-08-14 22:42:36]

Fired new try jobs to see if generated cert also trips up Windows since I had to
switch to Linux to generate that.

https://codereview.chromium.org/1286793002/diff/20001/net/data/ssl/certificat...
File net/data/ssl/certificates/README (right):

https://codereview.chromium.org/1286793002/diff/20001/net/data/ssl/certificat...
net/data/ssl/certificates/README:133: Windows refuses to parse this certificate.
On 2015/08/13 22:54:57, Ryan Sleevi (out until 8-24) wrote:
> On 2015/08/12 22:14:05, David Benjamin wrote:
> > On 2015/08/12 22:03:19, Ryan Sleevi wrote:
> > > It'd be preferable to not manually generate this. Can you add it to one of
> the
> > > test scripts?
> > 
> > This was generated from the Go program attached in the bug. There's no
reason
> to
> > ever regenerate it since it will never expire. 
> 
> Eh, that's what we said about the certs until we switched SHA-1 to SHA-256 ;)
> 
> > If you give me an OpenSSL
> > command-line to generate such a certificate, I'll include it, but I as a
> general
> > rule try to avoid that tool. :-P Generating certificates with anything but
Go
> is
> > just a pain.
> 
> That may be, but it's also the consistent way that multiple developers can
> collaborate (especially developers who hate Go :P). 
> 
> Load up net/data/ssl/scripts/generate-test-certs.sh . You can see explicit
date
> assignment at
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/data/ssl/scrip...
> as an example.
> 
> You can see explicit name assignment at
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/data/ssl/scrip...
> 
> It uses ASN1_TIME_set_string, which should handle dates even outside the
> representation of a time_t

Done. Yeah, I really don't like using openssl x509. It's obnoxious. :-) Used Go
to do CVE-2015-1793 as well.

[davidben, 2015-08-14 22:43:14]

davidben@chromium.org changed reviewers:
+ sky@chromium.org

[davidben, 2015-08-14 22:43:15]

Oh, +sky for chrome/common/localized_error.cc

[sky, 2015-08-14 22:56:30]

LGTM

[davidben, 2015-08-17 17:03:56]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-08-17 17:03:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1286793002/#ps60001
(title: "and now with more keys")

[commit-bot: I haz the power, 2015-08-17 17:04:09]

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

[commit-bot: I haz the power, 2015-08-17 18:29:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-08-17 18:29:34]

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