Issue 122498 in chromium: The error message for ERR_CERT_INVALID should not reference HSTS in the prompt

[chro...@googlecode.com]

Status: Assigned
Owner: pal...@chromium.org
Labels: Type-Polish Pri-2 Area-UI Internals-Network-SSL Mstone-20

New issue 122498 by rsl...@chromium.org: The error message for
ERR_CERT_INVALID should not reference HSTS in the prompt
http://code.google.com/p/chromium/issues/detail?id=122498

Version: 19.0.1084.15
OS: OS X 10.6.8

What steps will reproduce the problem?
1. Access a site that causes Chrome to return ERR_CERT_INVALID as a network
error code (https://www.penfed.org is on for 10.6.8, but any 'invalid' cert
such as one set to expire in the year 9000 will work)

What is the expected output? What do you see instead?
Expected:
A lovely message explaining that the server's certificate is invalid (or at
least, invalid as far as we see it), along with a button to go back.

Actual:
A lovely error message explaining that the server's certificate is invalid.
However, then the UI goes to assert that the user cannot proceed because
the website operator has requested heightened security for this domain.

This bug is a regression, caused by http://crrev.com/118557 . The
assumption is the inability to proceed is due to HSTS, but there are
non-HSTS reasons why proceeding may not work.

[chro...@googlecode.com]

Comment #1 on issue 122498 by pal...@chromium.org: The error message for

ERR_CERT_INVALID should not reference HSTS in the prompt
http://code.google.com/p/chromium/issues/detail?id=122498

Yes, I suppose overridable_ should be true in cases like this.

Forgive me, but what is invalid about this certificate? Mac OS X'
certificate viewer even says it's valid (even though Chrome says it isn't).
The presence of seemingly digressing KU vs. EKU is a bit odd, but...?

[chro...@googlecode.com]

Comment #2 on issue 122498 by rsl...@chromium.org: The error message for

ERR_CERT_INVALID should not reference HSTS in the prompt
http://code.google.com/p/chromium/issues/detail?id=122498

Dunno yet. That bug is Issue 122500 / http://crbug.com/122500

[chro...@googlecode.com]

Updates:
Cc: rsl...@chromium.org w...@chromium.org

Comment #4 on issue 122498 by pal...@chromium.org: The error message for

ERR_CERT_INVALID should not reference HSTS in the prompt
http://code.google.com/p/chromium/issues/detail?id=122498

Crawling up the deep and convoluted call stack, we seem ultimately to
arrive at this code from URLRequestHttpJob::OnStartCompleted:

708 } else if (IsCertificateError(result)) {
709 // We encountered an SSL certificate error. Ask our delegate to
decide
710 // what we should do.
711
712 TransportSecurityState::DomainState domain_state;
713 const bool fatal =
714 context_->transport_security_state() &&
715 context_->transport_security_state()->GetDomainState(
716 request_info_.url.host(),
717
SSLConfigService::IsSNIAvailable(context_->ssl_config_service()),
718 &domain_state);
719
NotifySSLCertificateError(transaction_->GetResponseInfo()->ssl_info, fatal);

Then to URLRequest::NotifySSLCertificateError; then to
delegate_->OnSSLCertificateError; then to void
SSLManager::OnSSLCertificateError; then to new SSLCertErrorHandler. Then
to SSLCertErrorHandler::OnDispatched; then to
manager_->policy()->OnCertError. Conveniently, the meaning of fatal is
reversed to mean "overridable" (line 67 below); it is here that the 1 bit
of information ("has TSS metadata, i.e. 'fatal'?") decays to less than 1
bit ("overridable or not, for whatever reason"). :)

46 void SSLPolicy::OnCertError(SSLCertErrorHandler* handler) {
47 // First we check if we know the policy for this error.
48 net::CertPolicy::Judgment judgment =
49 backend_->QueryPolicy(handler->ssl_info().cert,
50 handler->request_url().host());
51
52 if (judgment == net::CertPolicy::ALLOWED) {
53 handler->ContinueRequest();
54 return;
55 }
56
57 // The judgment is either DENIED or UNKNOWN.
58 // For now we handle the DENIED as the UNKNOWN, which means a blocking
59 // page is shown to the user every time he comes back to the page.
60
61 switch (handler->cert_error()) {
62 case net::ERR_CERT_COMMON_NAME_INVALID:
63 case net::ERR_CERT_DATE_INVALID:
64 case net::ERR_CERT_AUTHORITY_INVALID:
65 case net::ERR_CERT_WEAK_SIGNATURE_ALGORITHM:
66 case net::ERR_CERT_WEAK_KEY:
67 OnCertErrorInternal(handler, !handler->fatal());
68 break;
69 case net::ERR_CERT_NO_REVOCATION_MECHANISM:
70 // Ignore this error.
71 handler->ContinueRequest();
72 break;
73 case net::ERR_CERT_UNABLE_TO_CHECK_REVOCATION:
74 // We ignore this error but will show a warning status in the
location
75 // bar.
76 handler->ContinueRequest();
77 break;
78 case net::ERR_CERT_CONTAINS_ERRORS:
79 case net::ERR_CERT_REVOKED:
80 case net::ERR_CERT_INVALID:
81 case net::ERR_CERT_NOT_IN_DNS:
82 OnCertErrorInternal(handler, false);
83 break;
84 default:
85 NOTREACHED();
86 handler->CancelRequest();
87 break;
88 }
89 }

OnCertErrorInternal goes on to call
content::GetContentClient()->browser()->AllowCertificateError, which in
turn calls new SSLBlockingPage, where the error string is erroneously added
(due to interpreting "overridable" as "not fatal, i.e. not having TSS
metadata").

Thus, one could argue that the bug is in SSLPolicy::OnCertError, where the
once-clear meaning of "fatal" ("is there a DomainState entry") is degraded.
We could fix it by giving OnCertErrorInternal and its callees more
information, such as distinguishing overridable from has-TSS-metadata by
passing it a second bool. But I don't know if that is a really good way to
do it. I do know that the error message should be included exactly when
fatal = true, so passing fatal through to the callees seems like a direct,
even if not elegant, fix. Directness is good...

rsleevi, wtc, any preferences as to a fix strategy? I believe the direct
approach is likely to require a small, simple CL.

[chro...@googlecode.com]

Comment #5 on issue 122498 by rsl...@chromium.org: The error message for

ERR_CERT_INVALID should not reference HSTS in the prompt
http://code.google.com/p/chromium/issues/detail?id=122498

I disagree with the assertion that "fatal" (in the SSLCertErrorHandler) was
tied to "Is there a DomainState entry". Both "fatal" and "overridable"
existed prior to TSS/pinning, and both were indicative of whether or not
the user could, assuming perfect knowledge, successfully override the
error. If it's a protocol failure, for example, no matter how many times
they click Proceed, it will always fail.

Given that HSTS & pinning are progressing nicely as draft-specs and the
draft-specs do define some UA-specific behaviours (IIRC), I have no
objection between passing either two bools or a set of flags (which are
easier to read and harder to accidentally swap) that indicate whether the
error is fatal (it's not possible at the protocol layer to proceed) and
whether or not there was HSTS and/or pinning at play.

[chro...@googlecode.com]

Updates:
Status: Started

Comment #6 on issue 122498 by pal...@chromium.org: The error message for

ERR_CERT_INVALID should not reference HSTS in the prompt
http://code.google.com/p/chromium/issues/detail?id=122498

(No comment was entered for this change.)

[chro...@googlecode.com]

Comment #7 on issue 122498 by bugdro...@chromium.org: The error message for

ERR_CERT_INVALID should not reference HSTS in the prompt

http://code.google.com/p/chromium/issues/detail?id=122498#c7

The following revision refers to this bug:
http://src.chromium.org/viewvc/chrome?view=rev&revision=137349

------------------------------------------------------------------------
r137349 | pal...@chromium.org | Tue May 15 20:41:22 PDT 2012

Changed paths:
M
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_content_browser_client.h?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ssl/ssl_blocking_page.cc?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/ash/shell/content_client/shell_content_browser_client.h?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/mock_content_browser_client.cc?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/content/public/browser/content_browser_client.h?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/ssl/ssl_policy.cc?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/ssl/ssl_policy.h?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/mock_content_browser_client.h?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ssl/ssl_blocking_page.h?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/ash/shell/content_client/shell_content_browser_client.cc?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/chrome_content_browser_client.cc?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/examples/content_client/examples_content_browser_client.h?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/content/shell/shell_content_browser_client.h?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/content/shell/shell_content_browser_client.cc?r1=137349&r2=137348&pathrev=137349
M
http://src.chromium.org/viewvc/chrome/trunk/src/ui/views/examples/content_client/examples_content_browser_client.cc?r1=137349&r2=137348&pathrev=137349

Show the "cannot proceed" text only when appropriate.

Fix some simple lints while here.

BUG=122498

Review URL: https://chromiumcodereview.appspot.com/10381051
------------------------------------------------------------------------

[chro...@googlecode.com]

Updates:
Status: Fixed

Comment #8 on issue 122498 by pal...@chromium.org: The error message for

ERR_CERT_INVALID should not reference HSTS in the prompt