Add debugging information to the SSL blocking page

Add debugging information to the SSL blocking page

If you click on the SSL interstitial's error code, the following is
now shown:
* subject
* issuer
* validity end date
* current date
* pem encoded chain

This CL adds a placeholder for debugging information to the security
interstitial. It's intentionally meant to be flexible, so that it could
support other debugging information in the future from other warnings.

BUG=413838
Committed: https://crrev.com/df2931a70908043e07de24d3fa48c25cb68d7dc1
Cr-Commit-Position: refs/heads/master@{#296349}

[felt, 2014-09-18 22:43:54]

felt@chromium.org changed reviewers:
+ bauerb@chromium.org, palmer@chromium.org

[felt, 2014-09-18 22:45:19]

Hi, can you please review

palmer: c/b/ssl/
bauerb: c/b/resources/security_warnings/

You can see screenshots on the bug:
https://code.google.com/p/chromium/issues/detail?id=413838

[palmer, 2014-09-18 23:12:15]

Sweet! LGTM.

[Bernhard Bauer, 2014-09-19 08:40:35]

https://codereview.chromium.org/584463005/diff/1/chrome/browser/resources/sec...
File chrome/browser/resources/security_warnings/interstitial_v2.css (right):

https://codereview.chromium.org/584463005/diff/1/chrome/browser/resources/sec...
chrome/browser/resources/security_warnings/interstitial_v2.css:48:
.debugging-par {
What does "par" mean here?

https://codereview.chromium.org/584463005/diff/1/chrome/browser/resources/sec...
File chrome/browser/resources/security_warnings/interstitial_v2.js (right):

https://codereview.chromium.org/584463005/diff/1/chrome/browser/resources/sec...
chrome/browser/resources/security_warnings/interstitial_v2.js:37: /*
One more asterisk here to make this a JSDoc comment.

https://codereview.chromium.org/584463005/diff/1/chrome/browser/ssl/ssl_block...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/584463005/diff/1/chrome/browser/ssl/ssl_block...
chrome/browser/ssl/ssl_blocking_page.cc:528:
printed_chain.append(encoded_chain[i]);
I assume you're intentionally concatenating the strings without a separator? In
any case, you can use JoinString (from base/strings/string_util.h) for this.

[felt, 2014-09-20 15:47:02]

https://codereview.chromium.org/584463005/diff/1/chrome/browser/resources/sec...
File chrome/browser/resources/security_warnings/interstitial_v2.css (right):

https://codereview.chromium.org/584463005/diff/1/chrome/browser/resources/sec...
chrome/browser/resources/security_warnings/interstitial_v2.css:48:
.debugging-par {
On 2014/09/19 08:40:35, Bernhard Bauer wrote:
> What does "par" mean here?

Renamed to debugging-content.

https://codereview.chromium.org/584463005/diff/1/chrome/browser/resources/sec...
File chrome/browser/resources/security_warnings/interstitial_v2.js (right):

https://codereview.chromium.org/584463005/diff/1/chrome/browser/resources/sec...
chrome/browser/resources/security_warnings/interstitial_v2.js:37: /*
On 2014/09/19 08:40:35, Bernhard Bauer wrote:
> One more asterisk here to make this a JSDoc comment.

Done.

https://codereview.chromium.org/584463005/diff/1/chrome/browser/ssl/ssl_block...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/584463005/diff/1/chrome/browser/ssl/ssl_block...
chrome/browser/ssl/ssl_blocking_page.cc:528:
printed_chain.append(encoded_chain[i]);
On 2014/09/19 08:40:35, Bernhard Bauer wrote:
> I assume you're intentionally concatenating the strings without a separator? 

I don't need to because the pem file format already has the separators in it.

> In
> any case, you can use JoinString (from base/strings/string_util.h) for this.

Done.

[Bernhard Bauer, 2014-09-22 09:13:27]

https://codereview.chromium.org/584463005/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/584463005/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:527: load_time_data.SetString("pem",
JoinString(encoded_chain, '\0'));
Urr... does that actually work? JoinString does have an overload that takes a
std::string.

[felt, 2014-09-22 16:29:02]

https://codereview.chromium.org/584463005/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/584463005/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:527: load_time_data.SetString("pem",
JoinString(encoded_chain, '\0'));
On 2014/09/22 09:13:27, Bernhard Bauer wrote:
> Urr... does that actually work? JoinString does have an overload that takes a
> std::string.

Indeed it does work. I can't find the overload anywhere in code search, do you
know where it's defined? Everyone seems to be using it with separators.

[Bernhard Bauer, 2014-09-22 19:19:27]

https://codereview.chromium.org/584463005/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/584463005/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:527: load_time_data.SetString("pem",
JoinString(encoded_chain, '\0'));
On 2014/09/22 16:29:02, felt wrote:
> On 2014/09/22 09:13:27, Bernhard Bauer wrote:
> > Urr... does that actually work? JoinString does have an overload that takes
a
> > std::string.
> 
> Indeed it does work. I can't find the overload anywhere in code search, do you
> know where it's defined? Everyone seems to be using it with separators.

https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...

It's just that using a NUL character makes me a bit queasy (and it will result
e.g. in strange behavior when the user copies the text).

[felt, 2014-09-22 20:25:49]

https://codereview.chromium.org/584463005/diff/20001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/584463005/diff/20001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:527: load_time_data.SetString("pem",
JoinString(encoded_chain, '\0'));
On 2014/09/22 19:19:27, Bernhard Bauer wrote:
> On 2014/09/22 16:29:02, felt wrote:
> > On 2014/09/22 09:13:27, Bernhard Bauer wrote:
> > > Urr... does that actually work? JoinString does have an overload that
takes
> a
> > > std::string.
> > 
> > Indeed it does work. I can't find the overload anywhere in code search, do
you
> > know where it's defined? Everyone seems to be using it with separators.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin...
> 
> It's just that using a NUL character makes me a bit queasy (and it will result
> e.g. in strange behavior when the user copies the text).

Ohh I misunderstood. Fixed.

[Bernhard Bauer, 2014-09-22 20:51:30]

Yay! LGTM w/ a nit:

https://codereview.chromium.org/584463005/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/584463005/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:527: load_time_data.SetString("pem",
JoinString(encoded_chain, ""));
Using an empty std::string() constructor saves some instructions.

[felt, 2014-09-22 21:41:34]

https://codereview.chromium.org/584463005/diff/40001/chrome/browser/ssl/ssl_b...
File chrome/browser/ssl/ssl_blocking_page.cc (right):

https://codereview.chromium.org/584463005/diff/40001/chrome/browser/ssl/ssl_b...
chrome/browser/ssl/ssl_blocking_page.cc:527: load_time_data.SetString("pem",
JoinString(encoded_chain, ""));
On 2014/09/22 20:51:30, Bernhard Bauer wrote:
> Using an empty std::string() constructor saves some instructions.

Done.

[felt, 2014-09-22 21:41:58]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2014-09-22 21:43:14]

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

[commit-bot: I haz the power, 2014-09-22 22:53:35]

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

[commit-bot: I haz the power, 2014-09-22 22:53:36]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[felt, 2014-09-23 19:19:29]

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

[felt, 2014-09-23 19:22:24]

agl@: Can you review the change to net/cert/x509_certificate.cc? The
chrome://interstitials page doesn't have a proper cert chain, which is a problem
when ssl_blocking_page.cc calls ssl_info_.cert->GetPEMEncodedChain. This adds a
check to GetPEMEncodedChain to fail gracefully.

On 2014/09/22 22:53:36, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[agl, 2014-09-23 19:59:44]

https://codereview.chromium.org/584463005/diff/80001/net/cert/x509_certificat...
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/584463005/diff/80001/net/cert/x509_certificat...
net/cert/x509_certificate.cc:700: if (!os_cert_handle() ||
!GetPEMEncoded(os_cert_handle(), &pem_data))
Should this not be fixed in GetDEREncoded and thus magically fix all the higher
functions?

[felt, 2014-09-23 20:26:50]

https://codereview.chromium.org/584463005/diff/80001/net/cert/x509_certificat...
File net/cert/x509_certificate.cc (right):

https://codereview.chromium.org/584463005/diff/80001/net/cert/x509_certificat...
net/cert/x509_certificate.cc:700: if (!os_cert_handle() ||
!GetPEMEncoded(os_cert_handle(), &pem_data))
On 2014/09/23 19:59:44, agl wrote:
> Should this not be fixed in GetDEREncoded and thus magically fix all the
higher
> functions?

sure, did this.

[agl, 2014-09-23 20:30:00]

lgtm

LGTM

https://codereview.chromium.org/584463005/diff/100001/net/cert/x509_certifica...
File net/cert/x509_certificate_win.cc (right):

https://codereview.chromium.org/584463005/diff/100001/net/cert/x509_certifica...
net/cert/x509_certificate_win.cc:239: !cert_handle->cbCertEncoded)
this needs { } now that it's a multi-line conditional.

[felt, 2014-09-24 03:53:49]

https://codereview.chromium.org/584463005/diff/100001/net/cert/x509_certifica...
File net/cert/x509_certificate_win.cc (right):

https://codereview.chromium.org/584463005/diff/100001/net/cert/x509_certifica...
net/cert/x509_certificate_win.cc:239: !cert_handle->cbCertEncoded)
On 2014/09/23 20:30:00, agl wrote:
> this needs { } now that it's a multi-line conditional.

Done.

[felt, 2014-09-24 03:55:05]

The CQ bit was checked by felt@chromium.org

[commit-bot: I haz the power, 2014-09-24 03:55:17]

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

[commit-bot: I haz the power, 2014-09-24 04:49:09]

Committed patchset #7 (id:120001) as e18de6443b860af5bc3b385304ab1f94acc965fa

[commit-bot: I haz the power, 2014-09-24 04:50:00]

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

[Ryan Sleevi, 2014-09-24 04:54:33]

On 2014/09/23 19:22:24, felt wrote:
> agl@: Can you review the change to net/cert/x509_certificate.cc? The
> chrome://interstitials page doesn't have a proper cert chain, which is a
problem
> when ssl_blocking_page.cc calls ssl_info_.cert->GetPEMEncodedChain. This adds
a
> check to GetPEMEncodedChain to fail gracefully.
> 
> On 2014/09/22 22:53:36, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

I'd previously objected to this because 99/100, calling GetDEREncoded without a
valid cert is endemic of a bigger issue. I didn't add the DCHECK simply because
the null-deref would be enough to cause a crash.

Is there any reason we can't do this check on the blocking page for the cases
where we truly don't have a cert?

[felt, 2014-09-24 06:23:04]

On 2014/09/24 04:54:33, Ryan Sleevi (expect_delays) wrote:
> On 2014/09/23 19:22:24, felt wrote:
> > agl@: Can you review the change to net/cert/x509_certificate.cc? The
> > chrome://interstitials page doesn't have a proper cert chain, which is a
> problem
> > when ssl_blocking_page.cc calls ssl_info_.cert->GetPEMEncodedChain. This
adds
> a
> > check to GetPEMEncodedChain to fail gracefully.
> > 
> > On 2014/09/22 22:53:36, I haz the power (commit-bot) wrote:
> > > Try jobs failed on following builders:
> > >   linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
> > >
> >
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> I'd previously objected to this because 99/100, calling GetDEREncoded without
a
> valid cert is endemic of a bigger issue. I didn't add the DCHECK simply
because
> the null-deref would be enough to cause a crash.
> 
> Is there any reason we can't do this check on the blocking page for the cases
> where we truly don't have a cert?

Is it possible that INVALID means a cert chain wasn't built & it'll be null
there too?
If so then I think the check ought to be in GetDEREncoded. If not I can move it.