Closed
Bug 1242886
Opened 8 years ago
Closed 8 years ago
Use dedicated "Learn more..." link on sec_error_unknown_issuer error pages
Categories
(Core :: Security: PSM, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox48 | --- | verified |
People
(Reporter: philipp, Assigned: johannh)
References
Details
(Whiteboard: [fxprivacy])
Attachments
(1 file, 4 obsolete files)
i would ask that on the "Your Connection is not Secure" error pages for sec_error_unknown_issuer errors, users are led directly to a dedicated/targeted support site for their issue, so that they might be put in a more likely position to be able to fix their issue themselves. atm we have created https://support.mozilla.org/kb/troubleshoot-SEC_ERROR_UNKNOWN_ISSUER for this purpose (its content is depending on further vetting of course). background to this request: user questions about sec_error_unknown_issuer certificate errors make up a good part of incoming questions on sumo. those are generally caused by man-in-the-middling software, including many popular av vendors (like kaspersky, bitdefender, avast, bullguard, eset), windows features like ms family safety filters, and sometimes malware and will make the browser almost unusable for affected users. they might run into this once they refresh firefox for example, as the injected certificate is no longer in the trust store. though some affected users won't be able to load any https page and therefore can't get to the sumo article either, they would at least end up with a relevant url they can put into another browser...
Comment 1•8 years ago
|
||
So this is effectively a request to use a different target for the "Learn more..." link in the about:certerror page in case the error is sec_error_unknown_issuer? This should be very easy to change, the place to do it should be aboutCertError.xhtml:initPage() and there is already similar code in aboutNetError.xhtml:initPage(). We could even wait for bug 1240594 to get a unified error page and just add to that single callback.
Priority: -- → P3
Whiteboard: [fxprivacy]
Comment 2•8 years ago
|
||
(In reply to Panos Astithas [:past] from comment #1) > So this is effectively a request to use a different target for the "Learn > more..." link in the about:certerror page in case the error is > sec_error_unknown_issuer? Exactly. > We could even wait for bug 1240594 > to get a unified error page and just add to that single callback. Yes, sounds like a good plan.
Updated•8 years ago
|
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Updated•8 years ago
|
Priority: P3 → --
Updated•8 years ago
|
Priority: -- → P4
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
We want to wait for Bug 1240594 to land before getting this in. Another small obstacle is that the about:CertError page is currently not cleanly getting the error code (like SEC_ERROR_UNKNOWN_ISSUER). The simplest solution would be a small change to nsDocShell.cpp to pass it on, I suppose.
Depends on: 1240594
Assignee | ||
Comment 4•8 years ago
|
||
Assignee | ||
Comment 5•8 years ago
|
||
Comment on attachment 8739442 [details] [diff] [review] Pass error string to neterror pages So this is a pretty rough first attempt at solving the problem, which turned out less trivial than we expected. My main challenge was getting the nsresult turned into an error string like SEC_ERROR_UNKNOWN_ISSUER that the frontend can maintain. I added a method GetErrorStringName (we can change the name) inside NSSErrorsService. The initial version we talked about had nsDocShell.cpp attach the error string to the url parameters, but that was more messy than I liked it. Now I'm calling the NSSErrorsService XPCOM interface via browser.js and sending the code to content.js which assigns the correct URL. We could well use the same switch statement in Bug 712612 to tell if we should show the clock message.
Attachment #8739442 -
Flags: feedback?(ttaubert)
Comment 6•8 years ago
|
||
Comment on attachment 8739442 [details] [diff] [review] Pass error string to neterror pages Review of attachment 8739442 [details] [diff] [review]: ----------------------------------------------------------------- This looks good but I don't think we actually need to introduce a new function to get the NSS error as a string. Why not just send secInfo.errorCode to the child and then compare it with SEC_ERROR_UNKNOWN_ISSUER (== Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE + 13) in the switch statement? You can define it as a const in the function. The offset 13 is spread throughout the code base and changing it would break a lot of things, I think we can expect it won't change anytime soon. ::: browser/base/content/browser.js @@ +2775,5 @@ > if (isTopFrame) { > secHistogram.add(Ci.nsISecurityUITelemetry.WARNING_BAD_CERT_TOP_UNDERSTAND_RISKS); > } > > + var secInfo = getSecurityInfo(securityInfoAsString); nit: let ::: browser/base/content/content.js @@ +245,5 @@ > let div = content.document.getElementById("certificateErrorText"); > div.textContent = msg.data.info; > + > + switch (msg.data.code) { > + case 'SEC_ERROR_UNKNOWN_ISSUER': nit: please use double quotes, these are usually preferred @@ +247,5 @@ > + > + switch (msg.data.code) { > + case 'SEC_ERROR_UNKNOWN_ISSUER': > + let learnMoreLink = content.document.getElementById("learnMoreLink"); > + learnMoreLink.href = "https://support.mozilla.org/kb/troubleshoot-SEC_ERROR_UNKNOWN_ISSUER"; Not sure whether we should hard-code that link? ::: netwerk/base/nsINSSErrorsService.idl @@ +30,5 @@ > AString getErrorMessage(in nsresult aXPCOMErrorCode); > > /** > * Function will fail if aXPCOMErrorCode is not an NSS error code. > * @param aXPCOMErrorCode An error code obtain using getXPCOMFromNSSError nit: obtained @@ +33,5 @@ > * Function will fail if aXPCOMErrorCode is not an NSS error code. > * @param aXPCOMErrorCode An error code obtain using getXPCOMFromNSSError > + * return The error code as a string > + */ > + AString getErrorStringName(in nsresult aXPCOMErrorCode); Maybe getErrorCodeAsString or something? getErrorCodeName? getErrorCode? @@ +42,1 @@ > * return the I know you didn't write this but maybe fix it while you're here? "An error code obtained using..." and "return the ${something}".
Attachment #8739442 -
Flags: feedback?(ttaubert)
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Tim Taubert [:ttaubert] from comment #6) > Comment on attachment 8739442 [details] [diff] [review] > Pass error string to neterror pages > > Review of attachment 8739442 [details] [diff] [review]: > ----------------------------------------------------------------- > > This looks good but I don't think we actually need to introduce a new > function to get the NSS error as a string. Why not just send > secInfo.errorCode to the child and then compare it with > SEC_ERROR_UNKNOWN_ISSUER (== Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE + 13) > in the switch statement? You can define it as a const in the function. The > offset 13 is spread throughout the code base and changing it would break a > lot of things, I think we can expect it won't change anytime soon. > That was the kind of feedback I was hoping for :) You're right, I might just do it that way. Makes the whole thing less complex. I have to say this function sounds pretty useful to have anyway, but I guess we should only introduce it once we have an actual use case. Thanks!
Assignee | ||
Comment 8•8 years ago
|
||
>
> @@ +247,5 @@
> > +
> > + switch (msg.data.code) {
> > + case 'SEC_ERROR_UNKNOWN_ISSUER':
> > + let learnMoreLink = content.document.getElementById("learnMoreLink");
> > + learnMoreLink.href = "https://support.mozilla.org/kb/troubleshoot-SEC_ERROR_UNKNOWN_ISSUER";
>
> Not sure whether we should hard-code that link?
>
Well they're all hardcoded in aboutNetError.xhtml, so I don't really know what to do with this one.
Comment 9•8 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #8) > Well they're all hardcoded in aboutNetError.xhtml, so I don't really know > what to do with this one. It's probably fine to hardcode this.
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8739442 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8740437 -
Attachment is obsolete: true
Assignee | ||
Comment 12•8 years ago
|
||
Comment on attachment 8740438 [details] [diff] [review] Change learn more link for unknown issuers on about:certerror I removed the C++ part for now and added a really small test. Giving this to Gijs for review! (I hope that's okay).
Attachment #8740438 -
Flags: review?(gijskruitbosch+bugs)
Comment 13•8 years ago
|
||
Comment on attachment 8740438 [details] [diff] [review] Change learn more link for unknown issuers on about:certerror Review of attachment 8740438 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes below. ::: browser/base/content/browser.js @@ +2738,5 @@ > } > > let serhelper = Cc["@mozilla.org/network/serialization-helper;1"] > .getService(Ci.nsISerializationHelper); > + securityInfo = serhelper.deserializeObject(securityInfoAsString); Can you replace these 3 lines with just: securityInfo = getSecurityInfo(securityInfoAsString); so we avoid duplicating the logic for that? ::: browser/base/content/test/general/browser_aboutCertError.js @@ +253,5 @@ > + return learnMoreLink.href; > + }); > + is(href, "https://support.mozilla.org/kb/troubleshoot-SEC_ERROR_UNKNOWN_ISSUER"); > + > + gBrowser.removeCurrentTab(); Please use: yield BrowserTestUtils.removeTab(gBrowser.selectedTab); instead, because otherwise there will be intermittent issues under some conditions. ::: netwerk/base/nsINSSErrorsService.idl @@ +32,5 @@ > /** > * Function will fail if aXPCOMErrorCode is not an NSS error code. > * @param aXPCOMErrorCode An error code obtain using getXPCOMFromNSSError > + * return the error class of the code, either ERROR_CLASS_BAD_CERT > + * or ERROR_CLASS_SSL_PROTOCOL While we're here, can you replace "obtain" with "obtained" in the @param line? ::: security/manager/ssl/NSSErrorsService.cpp @@ -171,5 @@ > if (NS_ERROR_GET_MODULE(aXPCOMErrorCode) != NS_ERROR_MODULE_SECURITY || > NS_ERROR_GET_SEVERITY(aXPCOMErrorCode) != NS_ERROR_SEVERITY_ERROR) { > return NS_ERROR_FAILURE; > } > - Can you revert this so we don't touch this file? :-)
Attachment #8740438 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #13) > > ::: browser/base/content/test/general/browser_aboutCertError.js > @@ +253,5 @@ > > + return learnMoreLink.href; > > + }); > > + is(href, "https://support.mozilla.org/kb/troubleshoot-SEC_ERROR_UNKNOWN_ISSUER"); > > + > > + gBrowser.removeCurrentTab(); > > Please use: > > yield BrowserTestUtils.removeTab(gBrowser.selectedTab); > > instead, because otherwise there will be intermittent issues under some > conditions. > Ok, can do, but this is used many times in these tests. Should I replace all occurrences?
Flags: needinfo?(gijskruitbosch+bugs)
Comment 15•8 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #14) > (In reply to :Gijs Kruitbosch from comment #13) > > > > ::: browser/base/content/test/general/browser_aboutCertError.js > > @@ +253,5 @@ > > > + return learnMoreLink.href; > > > + }); > > > + is(href, "https://support.mozilla.org/kb/troubleshoot-SEC_ERROR_UNKNOWN_ISSUER"); > > > + > > > + gBrowser.removeCurrentTab(); > > > > Please use: > > > > yield BrowserTestUtils.removeTab(gBrowser.selectedTab); > > > > instead, because otherwise there will be intermittent issues under some > > conditions. > > > > Ok, can do, but this is used many times in these tests. Should I replace all > occurrences? Might as well, while we're here. Thanks!
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 16•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8740438 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8740774 -
Flags: review?(gijskruitbosch+bugs)
Comment 17•8 years ago
|
||
Comment on attachment 8740774 [details] [diff] [review] Change learn more link for unknown issuers on about:certerror Review of attachment 8740774 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thanks!
Attachment #8740774 -
Flags: review?(gijskruitbosch+bugs) → review+
Assignee | ||
Comment 18•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5a4970d617b
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 19•8 years ago
|
||
failed to apply: applying bug1242886.patch patching file browser/base/content/test/general/browser_aboutCertError.js Hunk #1 succeeded at 5 with fuzz 2 (offset 2 lines). Hunk #5 FAILED at 154 Hunk #6 FAILED at 226 2 out of 6 hunks FAILED -- saving rejects to file browser/base/content/test/general/browser_aboutCertError.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh bug1242886.patch could you take a look, thanks!
Flags: needinfo?(jhofmann)
Keywords: checkin-needed
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8740774 -
Attachment is obsolete: true
Assignee | ||
Comment 21•8 years ago
|
||
Mmh, this one rebased cleanly for me, what am I doing wrong? In any case I updated the patch, hope that does it!
Flags: needinfo?(jhofmann) → needinfo?(cbook)
Comment 22•8 years ago
|
||
(In reply to Johann Hofmann [:johannh] from comment #21) > Mmh, this one rebased cleanly for me, what am I doing wrong? In any case I > updated the patch, hope that does it! thats worked great ! thanks ! pushed the change and pulsebot will add the cset id here :)
Flags: needinfo?(cbook)
Comment 24•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/82d61d3c9821
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Updated•8 years ago
|
Iteration: --- → 48.3 - Apr 25
Flags: qe-verify?
Priority: P4 → P1
Comment 25•8 years ago
|
||
FWIW, you shouldn't be hard-coding sumo URIs into the code, but instead use app.support.baseURL and contact the sumo content manager. I've filed bug 1275788 to get the code fixed.
Comment 26•8 years ago
|
||
Tested on https://self-signed.badssl.com/ Verified fixed FX 48b6 Win 7.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•