Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 91341 Chrome crashes on SSL certificate
Starred by 27 users Reported by m.maly...@gmail.com, Aug 2 2011 Back to list
Status: Fixed
Owner:
Closed: Aug 2015
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment
Chrome Version       : 13.0.782.107 (Official version 94237) beta-m
URLs (if applicable) :
Other browsers tested:
Add OK or FAIL after other browsers where you have tested this issue:
    Firefox: OK
      Opera: OK
     Safari: FAIL - says, that secure connection with host cannot be established
       IE 7: FAIL - says, that host doesn't exist

What steps will reproduce the problem?
1. Define virtual host in apache using attached files - customize ip address as needed, mod_ssl must be enabled. The apache config file was prepared on Debian.
2. Type in Chrome https://<address>:8443/

What is the expected result?
"It Works" page from Debian

What happens instead?
Chrome crashes - all tabs and all windows disappear. When I try to start it and restore tabs, it crashes again. I can restore tabs after disabling virtual host in apache.


Please provide any additional information below. Attach a screenshot if possible.

 
test
1.4 KB View Download
key_2821079685904462798.pem
906 bytes View Download
cert_4839765633438139854.pem
800 bytes View Download
Labels: Stability-Crash OS-Linux
Cc: rsleevi@chromium.org
Labels: -Area-Undefined Area-Internals Internals-Network-SSL
m.maly...: Do you have a crash report ID?

You can find out about how to best report crash bugs at http://www.chromium.org/for-testers/bug-reporting-guidelines/reporting-crash-bug , which includes directions for how to get the ID.
Project Member Comment 4 by bugdroid1@chromium.org, Aug 2 2011
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=95179

------------------------------------------------------------------------
r95179 | agl@chromium.org | Tue Aug 02 16:10:26 PDT 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/ssl/ssl3con.c?r1=95179&r2=95178&pathrev=95179
 M http://src.chromium.org/viewvc/chrome/trunk/src/net/third_party/nss/patches/cachedinfo.patch?r1=95179&r2=95178&pathrev=95179

net: fix caching of peer's cert chain in session objects.

BUG= 91341 
TEST=none


Review URL: http://codereview.chromium.org/7549020
------------------------------------------------------------------------
Comment 5 by agl@chromium.org, Aug 2 2011
(The above bugdroid email is bogus. It should have been  bug 91400 .)
Comment 6 by agl@chromium.org, Aug 2 2011
Labels: -Pri-2 Pri-1 Mstone-14
Cannot repo on Linux but confirmed on Windows. Crash id 6de11873f2d3666a. Looks like HttpCache::Transaction::GetResponseInfo() is returning NULL and we're crashing at net/http/http_cache_transaction.cc:722.

I've left a test server running with the example certificates on agl-linux.nyc:10443 should anyone have a Windows box to debug with.
I confirm, Chrome was running on Windows XP. Apache was on Linux.
chrome.zip
19.4 KB Download
Project Member Comment 8 by bugdroid1@chromium.org, Aug 3 2011
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=95258

------------------------------------------------------------------------
r95258 | agl@chromium.org | Wed Aug 03 10:04:39 PDT 2011

Changed paths:
 M http://src.chromium.org/viewvc/chrome/branches/835/src/net/third_party/nss/patches/cachedinfo.patch?r1=95258&r2=95257&pathrev=95258
 M http://src.chromium.org/viewvc/chrome/branches/835/src/net/third_party/nss/ssl/ssl3con.c?r1=95258&r2=95257&pathrev=95258

Merge 95179 - net: fix caching of peer's cert chain in session objects.

BUG= 91341 
TEST=none


Review URL: http://codereview.chromium.org/7549020

TBR=agl@chromium.org
Review URL: http://codereview.chromium.org/7564016
------------------------------------------------------------------------
Comment 9 by wtc@chromium.org, Aug 8 2011
Labels: -OS-Linux OS-Windows
Owner: wtc@chromium.org
I will try to reproduce the crash on Windows today.

agl: please keep your test server agl-linux.nyc:10443 running while I
debug this.  Thanks.
Status: Untriaged
The crash appears to be caused by CryptoAPI being unable to parse the server's certificate. CertAddEncodedCertificateToStore is failing, and GetLastError returns 2148081666 (0x80092002), which is CRYPT_E_BAD_ENCODE ("An error occurred during encode or decode operation. ")

As a result of this, we're unable to create an OS-native cert handle for the server's certificate. SSLClientSocketNSS::UpdateServerCert, which calls X509Certificate::CreateFromDERCertChain, is thus unable to set |server_cert_| on the socket.

This then continues to bubble outward - when SSLClientSocketNSS::DoVerifyCert is called, because there is no |server_cert_|, it sets the response to ERR_CERT_INVALID and the cert status to CERT_STATUS_INVALID (lines 1566-1571)

When SSLClientSocketNSS::GetSSLInfo() is called, the returned |ssl_info| has no |cert|, but has a |cert_status| of CERT_STATUS_INVALID.  The ssl_info continues to propogate through the HTTP stack until it reaches the HttpCache::Transaction that owns the HttpNetworkTransaction.

In debug mode, this will trigger a DCHECK in HttpCache::Transaction::DoSendRequestComplete. Because the error code is a cert error (ERR_CERT_INVALID), HttpCache::Transaction expects that there will be response info from the HttpNetworkTransaction. However, HttpNetworkTransaction::GetResponseInfo() only returns |&response| if there is a valid OS-native certificate handle.

If there isn't (and in this case, there isn't), it returns NULL for the HttpResponseInfo. This is then DCHECK()ed in a Debug build. In a release build, this will cause an invalid read, as response->ssl_info is copied, which should be a member object in the first page of memory, thus triggering the read error.

Since this is a valid scenario (where a server certificate simply cannot be parsed), we do need to shore up some of this logic so that it can handle ERR_CERT_INVALID with no server certificate. Remoting ran into something similar, but because they weren't going through the HTTP stack, they didn't hit this exact error.

As to the reason this certificate is rejected by CryptoAPI, this is due to the fact that the dates on the certificate are messy. An OpenSSL dump below:
        Validity
            Not Before: Apr 12 14:37:47 2010 GMT
            Not After : Jan 11 06:00:00 1 GMT

This date is outside the range of that representable by Windows' FILETIME structure (which is the underlying representation in CERT_CONTEXT.pCertInfo->[NotBefore/NotAfter]. The Windows ASN.1 routines convert certificate times (GeneralizedTime/UTCTime) into FILETIME, which has a limited range of January 1, 1601 to the year 20,000 (roughly). This is because FILETIME uses two DWORDs, both of which are unsigned, to represent the 100-nanosecond internals since Jan 1, 1601.

This is "stupid" of Microsoft, in that ASN.1 defines the valid dates as anything between Jan 1, 0000 to Dec 31, 9999 (+/- GMT offsets), so they've chosen a datatype that cannot represent this date range. The parsing routines on Mac and NSS are more aware of this fact/lenient of such times. It's also unlikely to change, since this API has been part of Windows since 95/IE 3, and there is much riding on backwards compat.

As I mentioned earlier though, our response should be, for now, to handle when the OS cannot parse certificates gracefully, rather than DCHECKing or a possibly NULL access.
Status: Assigned
Labels: -Mstone-14 Mstone-15
Comment 13 by wtc@chromium.org, Sep 9 2011
Labels: -Mstone-15 Mstone-16
Comment 14 by laforge@google.com, Oct 24 2011
Labels: -Mstone-16 MovedFrom-16 Mstone-17
 Issue 103549  has been merged into this issue.
Comment 16 by k...@google.com, Jan 27 2012
Labels: -Mstone-17 Mstone-18 MovedFrom-17
Punteroo.  17 has sailed for the most part, please re-target and tag with a release block keyword if this needs to be in a 17 release.
Comment 17 by kareng@google.com, Mar 30 2012
Labels: -Mstone-18 Mstone-20
Comment 18 by kareng@google.com, Mar 30 2012
Labels: MovedFrom18
Labels: -Mstone-20 Mstone-21 MovedFrom-20
Comment 20 by wtc@chromium.org, Jun 19 2012
Labels: -Mstone-21 Mstone-22
I'm doing a pass over M22 Networking bugs. Will this be fixed for M22? Is it still an issue?
Labels: -MovedFrom-16 -MovedFrom-17 -Mstone-22 -MovedFrom18 -MovedFrom-20
Owner: ----
Status: Available
It isn't fixed for M22, it is still an issue. The general issue is any cert that NSS can parse but the underlying OS lib cannot.
Labels: -Pri-1 -OS-Windows Pri-2 OS-All
Comment 24 by wtc@chromium.org, Feb 1 2013
Cc: droger@chromium.org
I meant to look at this when this bug was originally reported.

I attached a patch that sketches a possible solution.

If a server certificate cannot be decoded, we should simply
terminate the connection. It would not be useful to try to
ask the user if he wants to accept the broken certificate
because we can't even display the certificate for the user
to inspect.

My only concern is this code in net/socket/ssl_client_socket_nss.cc,
SSLClientSocketNSS::DoVerifyCert:

  // If the certificate is expected to be bad we can use the expectation as
  // the cert status.
  base::StringPiece der_cert(
      reinterpret_cast<char*>(
          core_->state().server_cert_chain[0]->derCert.data),
      core_->state().server_cert_chain[0]->derCert.len);
  CertStatus cert_status;
  if (ssl_config_.IsAllowedBadCert(der_cert, &cert_status)) {
    DCHECK(start_cert_verification_time_.is_null());
    VLOG(1) << "Received an expected bad cert with status: " << cert_status;
    server_cert_verify_result_.Reset();
    server_cert_verify_result_.cert_status = cert_status;
    server_cert_verify_result_.verified_cert = core_->state().server_cert;
    return OK;
  }

  // We may have failed to create X509Certificate object if we are
  // running inside sandbox.
  if (!core_->state().server_cert) {
    server_cert_verify_result_.Reset();
    server_cert_verify_result_.cert_status = CERT_STATUS_INVALID;
    return ERR_CERT_INVALID;
  }

We need to find out if the last comment is still current.
I am hoping that the comment is stale because the code
that does SSL inside the sandbox should set up an allowed
bad cert (encoded) and be handled by the first if statement.
cannot-decode-cert-patch.txt
1.3 KB View Download
Wan-Teh: We do not ask the user if they want to accept the connection - we show a fatal page.

The issue is when different parts of the stack expect an SSL error to have an SSL certificate associated with it. For example, when showing the error page, the Page Info Bubble may expect to be able to view the certificate.

A solution that I had contemplated was to use a "Dummy" X509Certificate object (like we do/did for testing), so that we can provide some sort of 'meaningful' error message. Alternatively, we would need to plumb up all the way to the highest UI layers awareness that an SSL error may not always have an SSL cert.

Or we just don't treat that error as an SSL error, in which case, it will flow through the HTTP error flow and not even mention that the URI was (broken) SSL, nor give the URI SSL treatment. I'm not sure how that will interact with it originally being an https:// request, but that's an alternative.
Comment 26 by wtc@chromium.org, Feb 1 2013
rsleevi: Yes, your second proposal is the approach I sketched in
my patch. We can add special treatment for the new network error
(which I called ERR_CANNOT_DECODE_CERT in my patch). The new
network error won't be in the -200 range so it won't be handled
as a certificate error.

I also considered your first proposal. Its drawback is that it
would require us to inspect more code that uses
cert->os_cert_handle() and add null pointer checks.
Project Member Comment 27 by bugdroid1@chromium.org, Mar 10 2013
Labels: -Area-Internals -Internals-Network-SSL Cr-Internals Cr-Internals-Network-SSL
FYI: still happening on M29 on iOS.  It's not very frequent though (0.12% of the crashes at the moment).

Example: https://crash-old.corp.google.com/reportdetail?reportid=5f4393bed3c265c1#crashing_thread
 Issue 394759  has been merged into this issue.
 Issue 437659  has been merged into this issue.
 Issue 443531  has been merged into this issue.
Issue 472291 has been merged into this issue.
Cc: palmer@chromium.org davidben@chromium.org moham...@chromium.org rsesek@chromium.org
 Issue 477771  has been merged into this issue.
Cc: -rsleevi@chromium.org
Owner: rsleevi@chromium.org
Status: Assigned
rsleevi@: Based on #1 please take a look at this issue. 
Cc: rsleevi@chromium.org
Owner: ----
Status: Available
And unassigning *this* bug. You want Issue 472291 for this, for OS X specific, and which davidben@ already has a fix being worked on for.
Issue 499297 has been merged into this issue.
Given how many different things get merged into this, should we perhaps close this one and deal with them case-by-case? This bug seems to just be a dead end. If I hadn't looked into the OS X one on a whim after merging, we probably wouldn't have noticed the problem. Issue #499297 may well also be actionable once we get a crash ID.
Really, should just fix this bug (if parsing fails in OS but works in [BoringSSL, NSS], things get crashy). And then there are no more duplicates - everything is a special snowflake :)
I'm confused. SSLClientSocketOpenSSL handles this case just fine. It returns ERR_CERT_INVALID. The problem is that some code elsewhere crashes in this case... and if we dup everything into this bug without even finding out where it crashes, how can we fix anything?
Or are you suggesting that invalid certificates shouldn't pass net::IsCertificateError, so we don't break the IsCertificateError => has certificate not-really-invariant? I would be quite happy with that outcome.
Comment 42 by sleevi@google.com, Jun 17 2015
 Issue 500935  has been merged into this issue.
 Issue 510935  has been merged into this issue.
Now that the internal remoting plugin is gone, this should be much more straightforward. I *think* we no longer need to support the weird case where X509Certificates can't be created but allowed_bad_certs works.

So we should just be able to route this case up to a new error code that's not IsCertificateError, probably showing the same kind of error page as ERR_SSL_PROTOCOL_ERROR (since a certificate which BoringSSL internally cannot parse either would flag as that).
Owner: davidben@chromium.org
Status: Assigned
Project Member Comment 47 by bugdroid1@chromium.org, Aug 17 2015
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c6435a7aa061b2c32d2a015cd7acb57f4e395888

commit c6435a7aa061b2c32d2a015cd7acb57f4e395888
Author: davidben <davidben@chromium.org>
Date: Mon Aug 17 18:28:52 2015

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 

Review URL: https://codereview.chromium.org/1286793002

Cr-Commit-Position: refs/heads/master@{#343720}

[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/chrome/common/localized_error.cc
[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/base/net_error_list.h
[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/data/ssl/certificates/README
[add] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/data/ssl/certificates/bad_validity.pem
[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/data/ssl/scripts/generate-test-certs.sh
[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/socket/ssl_client_socket_nss.cc
[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/socket/ssl_client_socket_openssl.cc
[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/socket/ssl_client_socket_unittest.cc
[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/test/spawned_test_server/base_test_server.cc
[modify] http://crrev.com/c6435a7aa061b2c32d2a015cd7acb57f4e395888/net/test/spawned_test_server/base_test_server.h

Status: Fixed
Ah, the joys of closing a four-year-old bug.
Cc: est...@chromium.org jww@chromium.org f...@chromium.org bhanudev@google.com
 Issue 515044  has been merged into this issue.
Sign in to add a comment