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)

44 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla48
Iteration:
48.3 - Apr 25
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...
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]
(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.
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Priority: P3 → --
Priority: -- → P4
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
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
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)
Blocks: 712612
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)
(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!
> 
> @@ +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.
(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.
Attachment #8739442 - Attachment is obsolete: true
Attachment #8740437 - Attachment is obsolete: true
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 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+
(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)
(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)
Attachment #8740438 - Attachment is obsolete: true
Attachment #8740774 - Flags: review?(gijskruitbosch+bugs)
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+
Keywords: checkin-needed
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
Attachment #8740774 - Attachment is obsolete: true
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)
(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)
https://hg.mozilla.org/mozilla-central/rev/82d61d3c9821
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Iteration: --- → 48.3 - Apr 25
Flags: qe-verify?
Priority: P4 → P1
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.
Depends on: 1275896
See Also: → 1282455
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.