Give SSLErrorNavigationThrottle a safe way to distinguish if a net error happened. |
|||
Issue descriptionWe have a bit of a problem with the way our code is structured: 1) When --committed-interstitials is enabled, ChromeContentBrowserClient::AllowCertificate() provides a decision of CERTIFICATE_REQUEST_RESULT_TYPE_DENY. 2) The code path that sends ssl_info to the throttle only does so if there is a certificate error: https://cs.chromium.org/chromium/src/content/browser/loader/navigation_resource_handler.cc?l=168&rcl=e55529a209fdc46dccc0138bfa73c20e7a5f6d60 3) SSLErrorNavigationThrottle::WillFailRequest() [2] tries to look at the cert_status of the available SSLInfo to find the net error. It then uses that net error to decide if the net error was a certificate error. But *the SSLInfo may be bogus* if the failure wasn't a certificate error in the first place. It appears that visiting rc4.badssl.com results in bogus SSLInfo data, which means that the throttle will non-deterministically sometimes detect an SSL error and try to show an interstitial – which can result in a crash. I'm looking at alternatives: - Modify 1) to support cancelling with a custom net error. - Modify 2) to always pass ssl_info. I don't yet know if either is sufficient. estark@, any thoughts? [1] https://cs.chromium.org/chromium/src/chrome/browser/chrome_content_browser_client.cc?l=2256&rcl=e55529a209fdc46dccc0138bfa73c20e7a5f6d60 [2] As of https://crrev.com/c/621236 patch #22.
,
Oct 31 2017
Looks like: - Modify 2) to always pass ssl_info. reliably results in a cert_status of 0 (as opposed to an uninitialized integer). I'll send a short patch for that.
,
Oct 31 2017
What do you mean by bogus? An empty SSLInfo should have a cert_status of 0 as set in SSLInfo::Reset, which is what I would think is what we would get in the case of a non-cert error. I see that SSLErrorNavigationThrottle::WillFailRequest() is retrieving the value from GetSSLInfo() without checking that the value exists. I'm guessing you just need to check GetSSLInfo().has_value() before accessing the value? (Though, I see that there is a DCHECK inside value() that should catch this case... do you build without DCHECKs enabled?)
,
Oct 31 2017
> do you build without DCHECKs enabled? aaaaarrrrggggggghhhh I was building with DCHECKs for months and months, but I turned them off a few hours ago because unrelated bugs are making my local build unusably crashy otherwise. :-( > I'm guessing you just need to check GetSSLInfo().has_value() before accessing the value? Thanks; I just checked more carefully, and that *does* appear to be safe. |
|||
►
Sign in to add a comment |
|||
Comment 1 by lgar...@chromium.org
, Oct 31 2017