In aboutCertError's advanced information pane, there is information about the exact error that occurred, like: Error code: ssl_error_no_cypher_overlap The error code should be a link that displays additional technical information for expert users, when clicked: http://brampitoyo.github.io/fx-untrusted-connection/rc4.xhtml#technicalInformation This new page should provide buttons to copy the text to clipboard, as can be seen in the above mockup.

Posting my WIP.

I got it almost all the way to the finish line until I hit a snag. I can't get the SSLStatus from the SecureBrowserUI when the page has certificate issues, which means I can't get to the certificate. It comes back as null in https://wrong.host.badssl.com/ for example. Also the lack of a certificate display in the Page Info dialog is probably quite telling. Reading through the relevant code I think this is by design when STATE_IS_INSECURE. If this is true, then I don't see how we could display the certificate in the about:certerror page as Bram's design requires. Tanvi, do you know this code and whether my conclusions above are correct?

(In reply to Panos Astithas [:past] from comment #2) > Created attachment 8689607 [details] [diff] [review] > WIP v2 > > I got it almost all the way to the finish line until I hit a snag. I can't > get the SSLStatus from the SecureBrowserUI when the page has certificate > issues, which means I can't get to the certificate. > > It comes back as null in https://wrong.host.badssl.com/ for example. Also > the lack of a certificate display in the Page Info dialog is probably quite > telling. Reading through the relevant code I think this is by design when > STATE_IS_INSECURE. If this is true, then I don't see how we could display > the certificate in the about:certerror page as Bram's design requires. > > Tanvi, do you know this code and whether my conclusions above are correct? keeler? Also cc'ing Tim and Franziskus who are closer to your timezone, so might get to this first.

I would modify the cert exception case to behave more like "Browser:SendSSLErrorReport", which sends the whole securityInfo, not just the securityInfo's SSLStatus. That way, you can get at the failedCertChain, the 0th element of which should be the end-entity certificate. https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/browser/base/content/content.js#325 https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/browser/base/content/browser.js#2897

Thanks David, this was very helpful. I was able to get the certificate and display it. Bram, one thing that is not clear to me is what information exactly did you want displayed? Your mockup has a lot of stuff in it, some of which I don't know how they were generated. In my WIP I am displaying the PEM-encoded server certificate, but perhaps it would have to be the entire certificate chain, to help with identifying issues with one of the parent certs. Another option would be to have the cert (or cert chain) along with all of the information we show in the Page Info -> Security tab, on a properly functioning secure page. What are the things we want the user to be able to inspect and copy?

This works, assuming the response to my comment 5 is positive, and is only missing a test.

I've heard that Bram is probably out today, so perhaps April or David can answer this. From comment 5: One thing that is not clear to me is what information exactly do we want displayed? The mockup has a lot of stuff in it, some of which I don't know how they were generated. In my WIP I am displaying the PEM-encoded server certificate, but perhaps it would have to be the entire certificate chain, to help with identifying issues with one of the parent certs. Another option would be to have the cert (or cert chain) along with all of the information we show in the Page Info -> Security tab, on a properly functioning secure page. What are the things we want the user to be able to inspect and copy, in order to generate a useful bug report for a misconfigured web site?

The mockup was entirely generated with openssl s_client, minus a few bits about session tickets and what not. I think it should have the entire certificate chain for sure, as the problem can lie anywhere in the chain. We should also have the Page Info stuff, which would tell us which protocol and cipher suite it used. If we can get at other stuff, such as what curve it used, that would be nice as well. But simply having the certificate chain, protocol, cipher suite, hostname, port, and error code would go a very long ways.

Also including HPKP / HSTS status might be useful (like the security tab of the network tab of the devtools).

@keeler: Great idea! Also we should probably include the OCSP information, if the failure was related to something in that area.

Added more information according to feedback and fixed a failing test. I couldn't add cipher and protocol as SSLStatus isn't available (see comment 2) and I think we can leave OCSP for a followup, as it's not clear to me how to get the required information or how to test it. Next I am going to write a test and then ask for review.

Now with tests. I had to tweak the about:neterror page too, because cert error messages now contain links and we don't display them the same way there. Some other minor changes were made in order to be consistent from now on in displaying error codes in uppercase. Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4144ff6e71d8

I'm sorry, I won't be able to get to this today, so it'll likely be Monday before I can review this.

No worries, that's what I expected.

I have nothing to add here, except for the fact that I’m following April’s lead on the kind of technical info that this section should provide. It looks like that has been done on comment 8 and comment 9.

Comment on attachment 8693013 [details] [diff] [review] Patch v5 Review of attachment 8693013 [details] [diff] [review]: ----------------------------------------------------------------- This looks really interesting, but on the whole there are quite some loose ends here still, so it feels like this needs another round of review (and review from an NSS/PSM person). ::: browser/base/content/aboutcerterror/aboutCertError.xhtml @@ +56,5 @@ > var node = document.getElementById(id); > node.style.visibility = node.style.visibility == "" ? "hidden" : ""; > + if (id == "advancedPanel") { > + var div = document.getElementById("certificateErrorDebugInformation"); > + div.style.display = "none"; I don't really understand why this is necessary. Can you add a comment? @@ +259,5 @@ > + <div id="certificateErrorDebugInformation"> > + <a name="technicalInformation"></a> > + <button id="copyToClipboard">&certerror.copyToClipboard.label;</button> > + <div id="certificateErrorText"/> > + <button id="copyToClipboard">&certerror.copyToClipboard.label;</button> It doesn't look like these ever have event handlers attached? ::: browser/base/content/browser.js @@ +2894,5 @@ > + let sslStatus = null; > + try { > + sslStatus = securityInfo.QueryInterface(Ci.nsISSLStatusProvider) > + .SSLStatus; > + } catch (e) {} Why is there a try catch here with an empty catch block? There should at least be a comment as to why this is necessary. @@ +2931,5 @@ > secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_UNDERSTAND_RISKS); > } > + > + let errorInfo = getDetailedCertErrorInfo(securityInfoAsString); > + gBrowser.selectedBrowser.messageManager Shouldn't this use browser.messageManager ? @@ +3202,5 @@ > + if (!securityInfoAsString) > + return ""; > + > + let details = []; > + details.push(gBrowser.selectedBrowser.lastURI.spec); We should pass this (ie |browser| or its URI from the message handler) in so this function is reusable without the footgun that the URL might be wrong. Also, the message we get might not be for the selectedBrowser, and so I think this could already show the wrong URI. @@ +3234,5 @@ > + let certs = securityInfo.failedCertChain.getEnumerator(); > + while (certs.hasMoreElements()) { > + if (count == 0) { > + > + } ?? ::: browser/base/content/test/general/browser_aboutCertError.js @@ +204,5 @@ > + "Correct HPKP value found"); > + ok(message.text.contains("-----BEGIN CERTIFICATE-----"), > + "Found certificate header"); > + ok(message.text.contains("-----END CERTIFICATE-----"), > + "Found certificate footer"); Is there a particular reason not to actually check the cert itself here? ::: browser/locales/en-US/chrome/browser/browser.properties @@ +807,5 @@ > +# the server administrators for troubleshooting. %1$S is the visited URL, %2$S > +# is the error message, %3$S is true or false, depending on whether the server > +# supports HSTS, %4$S is true or false, depending on whether the server > +# supports HPKP, %5$S is the certificate chain in PEM format. > +certErrorDetails.label = %1$S\n\n%2$S\n\nHTTP Strict Transport Security: %3$S\nHTTP Public Key Pinning: %4$S\n\nCertificate chain:\n\n%5$S We're letting the user copy this to the clipboard, but it's going to have mixed line endings because of the cert details which use \r\n. Did you test this on Windows? Maybe use \r\n in here as well? ::: browser/locales/en-US/chrome/overrides/netError.dtd @@ +216,5 @@ > > <!ENTITY sslv3Used.title "Unable to Connect Securely"> > <!-- LOCALIZATION NOTE (sslv3Used.longDesc) - Do not translate > "ssl_error_unsupported_version". --> > +<!ENTITY sslv3Used.longDesc "Advanced info: SSL_ERROR_UNSUPPORTED_VERSION"> We shouldn't update strings and keep the same ID. You've also not updated the l10n note (and you've done the inverse for weakCryptoUsed.longDesc). ::: browser/themes/shared/aboutCertError.css @@ +60,5 @@ > + position: absolute; > + left: 0%; > + top: 100%; > + width: 65%; > + padding: 1% 17.5% 2%; What is the top and bottom padding here about? Seems like that should just be in px or em and/or at least be the same? ::: browser/themes/shared/aboutNetError.css @@ +152,5 @@ > + > +#errorCode { > + color: var(--in-content-page-color); > + cursor: text; > + text-decoration: none; Why are we hiding the fact that this is a link? ::: security/manager/ssl/TransportSecurityInfo.cpp @@ -881,5 @@ > const char *codeName = nsNSSErrors::getDefaultErrorStringName(errorCodeToReport); > if (codeName) > { > nsCString error_id(codeName); > - ToLowerCase(error_id); I don't understand why these changes are necessary, and they should probably be reviewed by one of the PSM folks.

Thanks for the review! Clarifications below. David, can you take a look at the few PSM changes? (In reply to :Gijs Kruitbosch from comment #16) > I don't really understand why this is necessary. Can you add a comment? Added. > @@ +259,5 @@ > > + <div id="certificateErrorDebugInformation"> > > + <a name="technicalInformation"></a> > > + <button id="copyToClipboard">&certerror.copyToClipboard.label;</button> > > + <div id="certificateErrorText"/> > > + <button id="copyToClipboard">&certerror.copyToClipboard.label;</button> > > It doesn't look like these ever have event handlers attached? There is ClickEventHandler in content.js that sends the event to BrowserOnClick.onAboutCertError, which deals with copyToClipboard specifically. > ::: browser/base/content/browser.js > @@ +2894,5 @@ > > + let sslStatus = null; > > + try { > > + sslStatus = securityInfo.QueryInterface(Ci.nsISSLStatusProvider) > > + .SSLStatus; > > + } catch (e) {} > > Why is there a try catch here with an empty catch block? There should at > least be a comment as to why this is necessary. I copied it from some other place that I can't remember right now, but I took it out, since the other accesses of SSLStatus that I see don't guard against QI failure. > Shouldn't this use browser.messageManager ? Good point, done. > We should pass this (ie |browser| or its URI from the message handler) in so > this function is reusable without the footgun that the URL might be wrong. > Also, the message we get might not be for the selectedBrowser, and so I > think this could already show the wrong URI. Right, fixed. > ?? Argh, that's just leftover from a previous refactoring. Removed. > Is there a particular reason not to actually check the cert itself here? Apart from laziness? Nope. Done. > We're letting the user copy this to the clipboard, but it's going to have > mixed line endings because of the cert details which use \r\n. Did you test > this on Windows? Maybe use \r\n in here as well? Good catch, I switched to \r\n. > We shouldn't update strings and keep the same ID. You've also not updated > the l10n note (and you've done the inverse for weakCryptoUsed.longDesc). Thanks, fixed. > ::: browser/themes/shared/aboutCertError.css > @@ +60,5 @@ > > + position: absolute; > > + left: 0%; > > + top: 100%; > > + width: 65%; > > + padding: 1% 17.5% 2%; > > What is the top and bottom padding here about? Seems like that should just > be in px or em and/or at least be the same? This was taken straight from the UX prototype, but I agree, just using 1em for both seems better. > ::: browser/themes/shared/aboutNetError.css > @@ +152,5 @@ > > + > > +#errorCode { > > + color: var(--in-content-page-color); > > + cursor: text; > > + text-decoration: none; > > Why are we hiding the fact that this is a link? This new design that displays detailed error information only applies to about:certerror. Unfortunately the same error message format is used in about:neterror too, so I had to hide it in order to avoid user confusion. It may be possible to provide a similar detailed information section for easy error reporting to about:neterror after bug 1218971, which brings the two designs closer. I'll file a followup to discuss with Bram what kind of information could be displayed in that case. > ::: security/manager/ssl/TransportSecurityInfo.cpp > @@ -881,5 @@ > > const char *codeName = nsNSSErrors::getDefaultErrorStringName(errorCodeToReport); > > if (codeName) > > { > > nsCString error_id(codeName); > > - ToLowerCase(error_id); > > I don't understand why these changes are necessary, and they should probably > be reviewed by one of the PSM folks. The new design uses uppercase strings for the error codes, which is both what the lower level APIs provide and what Chrome uses. The UX prototype used a text-transform for this, but it seemed pointless to keep doing redundant work.

(In reply to Panos Astithas [:past] from comment #17) > > ::: browser/themes/shared/aboutNetError.css > > @@ +152,5 @@ > > > + > > > +#errorCode { > > > + color: var(--in-content-page-color); > > > + cursor: text; > > > + text-decoration: none; > > > > Why are we hiding the fact that this is a link? > > This new design that displays detailed error information only applies to > about:certerror. Unfortunately the same error message format is used in > about:neterror too, so I had to hide it in order to avoid user confusion. > > It may be possible to provide a similar detailed information section for > easy error reporting to about:neterror after bug 1218971, which brings the > two designs closer. I'll file a followup to discuss with Bram what kind of > information could be displayed in that case. Can you test if you can use moz-document() to make this only apply in about:neterror? (URIs for error documents are... interesting... so I don't know if that works.)

Comment on attachment 8694225 [details] [diff] [review] v5-v6 interdiff Review of attachment 8694225 [details] [diff] [review]: ----------------------------------------------------------------- r=me either with or without this and the css thing addressed. Thanks! ::: browser/base/content/browser.js @@ +2932,5 @@ > > case "copyToClipboard": > const gClipboardHelper = Cc["@mozilla.org/widget/clipboardhelper;1"] > .getService(Ci.nsIClipboardHelper); > + let detailedInfo = getDetailedCertErrorInfo(browser.lastURI, FWIW, I am paranoid about this being wrong in some cases, but there's only one way to find out, I guess... I'm assuming we can't just take the URI from the error page's query parameters? I thought we could...

(In reply to :Gijs Kruitbosch from comment #18) > Can you test if you can use moz-document() to make this only apply in > about:neterror? > > (URIs for error documents are... interesting... so I don't know if that > works.) It doesn't seem to work, which is not that surprising given how error pages are loaded. But I doubt we would want to do that even if it did, as aboutNetError.css is also included in aboutSocialError.xhtml, which we don't have a design with a clickable link for, either.

Comment on attachment 8694222 [details] [diff] [review] Patch v6 Review of attachment 8694222 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the PSM bits. Also, I think I found a bug - see below. ::: browser/base/content/aboutcerterror/aboutCertError.xhtml @@ +211,5 @@ > > // If we set a link, meaning there's something helpful for > // the user here, expand the section by default > if (link.href && getCSSClass() != "expertBadCert") > toggleVisibility("advancedPanel"); I think there's a bug here. If this line runs, the advanced panel is displayed by default. If the user then clicks on the error code, the debugging information shows up empty because browser.js never saw a click event on the advanced button (which is what populates that information). ::: browser/locales/en-US/chrome/browser/browser.properties @@ +801,5 @@ > # LOCALIZATION NOTE (weakCryptoOverriding.message): %S is brandShortName > weakCryptoOverriding.message = %S recommends that you don't enter your password, credit card and other personal information on this website. > revokeOverride.label = Don't Trust This Website > revokeOverride.accesskey = D > +# LOCALIZATION NOTE (weakCryptoOverriding.message): This is a text string that s/weakCryptoOverriding.message/certErrorDetails.label/ ::: security/manager/locales/en-US/chrome/pipnss/pipnss.properties @@ +292,5 @@ > # LOCALIZATION NOTE (certErrorNotYetValidNow): Do not translate %1$S (date+time certificate will become valid) or %2$S (current date+time) > certErrorNotYetValidNow=The certificate will not be valid until %1$S. The current time is %2$S. > > +# LOCALIZATION NOTE (certErrorCodePrefix2): Do not translate <a id="errorCode" title="%1$S">%1$S</a> > +certErrorCodePrefix2=Error code: <a id="errorCode" title="%1$S">%1$S</a> In the long run, I think attempting to format HTML in C++ is a bad idea. However, this code already does this elsewhere and fixing it will be a significant amount of work that we don't need to do right now.

(In reply to :Gijs Kruitbosch from comment #20) > FWIW, I am paranoid about this being wrong in some cases, but there's only > one way to find out, I guess... I'm assuming we can't just take the URI from > the error page's query parameters? I thought we could... You are right, we can use the query parameters and I've tweaked the patch to do just that. Thanks. (In reply to David Keeler [:keeler] (use needinfo?) from comment #22) > ::: browser/base/content/aboutcerterror/aboutCertError.xhtml > @@ +211,5 @@ > > > > // If we set a link, meaning there's something helpful for > > // the user here, expand the section by default > > if (link.href && getCSSClass() != "expertBadCert") > > toggleVisibility("advancedPanel"); > > I think there's a bug here. If this line runs, the advanced panel is > displayed by default. If the user then clicks on the error code, the > debugging information shows up empty because browser.js never saw a click > event on the advanced button (which is what populates that information). Do you know of a test page that I can use to verify this? It doesn't look like anything at badssl.com triggers this path and I couldn't find anything else so far.

I believe it shows up for prefix mismatches. That is, if the certificate is valid for example.com but you visited www.example.com, there's a good chance that going to example.com will work. I was just doing some local testing - I can't find any live examples right now. I'll file an issue on badssl to get an example for that.

I verified that the issue pointed out in comment 22 is real and this version contains a fix for it, along with the rest of the review suggestions. I could see two ways to fix it and I opted for the one with the less code changes, so I'd like to make sure that Gijs agrees. I'll post an interdiff in a second. I followed about:neterror's path and used a custom event to indicate page readiness, which triggers the security info addition to the "expert" section. The funky thing about it is that I reused the existing ClickEventHandler.onAboutCertError() machinery by firing the custom event on the advancedButton. Another option would be to fire the event on the document and add an AboutCertErrorListener, similar to AboutNetErrorListener, which will only handle a single event. Furthermore, I would need to add a message handler to browser.js, similar to BrowserOnClick, to handle a single message. This might seem cleaner architecturally, but with a lot more code.

Comment on attachment 8695877 [details] [diff] [review] Patch v7 Review of attachment 8695877 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/browser.js @@ +2894,5 @@ > + > + let errorInfo = getDetailedCertErrorInfo(location, > + securityInfoAsString); > + browser.messageManager.sendAsyncMessage("AboutCertErrorDetails", > + { info: errorInfo }); Isn't this unnecessary now, because we will have done this on load?

(In reply to :Gijs Kruitbosch from comment #28) > Isn't this unnecessary now, because we will have done this on load? The custom load event takes this same path, so it is only unnecessary on any future clicks of the "Advanced" button.

45.0a1 (2015-12-06) Win 7 The link is displayed on https://expired.badssl.com/. But no link displayed on https://rc4.badssl.com/, https://sslv3.dshield.org/. Thoughts?

(In reply to Paul Silaghi, QA [:pauly] from comment #32) > The link is displayed on https://expired.badssl.com/. > But no link displayed on https://rc4.badssl.com/, https://sslv3.dshield.org/. This is because the last two errors cause the about:neterror page to appear, whereas this patch concerns about:certerror.

Verified fixed FF 45 based on comments 32, 33.

While working on bug 1220481, I noticed that this bug added a call to toggleVisibility() in aboutNetError.xhtml - the function is only defined in aboutCertError.xhtml, as is the element whose id is being passed to it. Worth investigating? https://dxr.mozilla.org/mozilla-central/rev/789a12291942763bc1e3a89f97e0b82dc1c9d00b/browser/base/content/aboutNetError.xhtml#379

(In reply to Nihanth Subramanya [:nhnt11] from comment #35) > While working on bug 1220481, I noticed that this bug added a call to > toggleVisibility() in aboutNetError.xhtml - the function is only defined in > aboutCertError.xhtml, as is the element whose id is being passed to it. > Worth investigating? > > https://dxr.mozilla.org/mozilla-central/rev/ > 789a12291942763bc1e3a89f97e0b82dc1c9d00b/browser/base/content/aboutNetError. > xhtml#379

I have recently spotted this as well and I plan to remove it in bug 1246162.

(In reply to Panos Astithas [:past] from comment #37) > I have recently spotted this as well and I plan to remove it in bug 1246162. Yes, but aren't we shipping this in 45 now? Should we back (part of) this out of 45?

We could, but I'm not sure we should bother as it's almost dead code. I don't know of any error that ends up in about:neterror with a link in the error code, such errors appear only in about:certerror in my testing. And in order for the user to hit that code path she would have to have set the pref browser.xul.error_pages.expert_bad_cert to true, which is a pref I never knew existed before working on this. And if you are thinking then why did I add this check in about:neterror in the first place, well, I was thinking about a future unification of the two error pages and tried to eliminate needless differences in the code.