583408 - Notify user when the certificate attribute check fails

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[Robert Strong (they/them - no direct email)]

Spinoff of bug 544442 to provide UI when the certificate attribute check fails. My current thought is this should only happen after x number of failures (by default we check for updates once per day so the x is equivalent to days) and that the notification should happen at startup.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Taking for design.

Comment 2

[Robert Strong (they/them - no direct email)]

beltzner, any update on this?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Talked to Rob in person, and we'll be able to differentiate between two cases:

Case A: check fails, we are not being served an update snippet
Case B: check fails, we are being served an update snippet

Case A implies a network configuration error, or an active portal, a poorly configured proxy, or a global MITM attack - but not one specific to Firefox. In this case I believe that we can suggest that the user talk to their network administrator, or we can recommend that users go to www.firefox.com to get the latest update.

Case B implies a targeted attack against Firefox, and we should say that, specifically, and nothing more. Such an attack would likely also include a MITM attack that makes sending users to www.firefox.com pretty much useless.

Suggested wording:

Case A:

Something is preventing Firefox from updating securely. Please check www.firefox.com to make sure you have the latest version.

Case B:

Something is trying to trick Firefox into accepting an insecure update. Please contact your network provider and seek help.

We should only show these errors after the automatic update fails to pass the check 5 times in a row (minimum of 5 days). If a user manually attempts to update and the attribute check fails, we should always show the warning.

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob, lemme know if you think we can at least get the strings in for beta5.

Comment 5

[Robert Strong (they/them - no direct email)]

beltzner, could we go with these slightly modified strings instead to avoid putting the url in the middle of the string? Putting the url in the middle of the string makes it so there needs to be multiple strings and iirc can cause issues with layout (e.g. font size of the string could be larger than the url, etc.)

Something is preventing &brandShortName; from updating securely. Please check you have the latest version of &brandShortName; at:
www.firefox.com

Something is trying to trick &brandShortName; into accepting an insecure update. Please contact your network provider and seek help.

Also, I am going with the error page title of "Update Failed"

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch strings

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 469997 [details] [diff] [review]
patch strings

uir=beltzner

Comment 8

[Dave Townsend [:mossop]]

Comment on attachment 469997 [details] [diff] [review]
patch strings

># HG changeset patch
># Parent 81b52091ecd70fdc2e02d81b7b6ad2445d530146
>
>diff --git a/toolkit/locales/en-US/chrome/mozapps/update/updates.dtd b/toolkit/locales/en-US/chrome/mozapps/update/updates.dtd
>--- a/toolkit/locales/en-US/chrome/mozapps/update/updates.dtd
>+++ b/toolkit/locales/en-US/chrome/mozapps/update/updates.dtd
>@@ -47,26 +47,32 @@
> <!ENTITY  verificationFailedText.label    "&brandShortName; was unable to verify the integrity of the 
>                                            incremental update it downloaded, so it is now downloading
>                                            the complete update package.">
> 
> <!ENTITY  viewDetails.tooltip             "View details for this update">
> 
> <!ENTITY  details.link                    "Details">
> 
>+<!ENTITY  error.title                     "Update Failed">
>+
> <!ENTITY  error.label                     "There were problems checking for, downloading, or installing this 
>                                            update. &brandShortName; could not be updated because:">
>                                            
> <!ENTITY  errorManual.label               "You can update &brandShortName; manually by visiting this link
>                                            and downloading the latest version:">
>                                            
>-<!ENTITY  errorpatching.title             "Update Failed">
> <!ENTITY  errorpatching.intro             "The partial Update could not be applied. 
>                                            &brandShortName; will try again by downloading a complete Update.">
> 
>+<!ENTITY  errorCertAttrNoUpdate.label     "Something is preventing &brandShortName; from updating securely.
>+                                           Please check you have the latest version of &brandShortName; at:">
>+<!ENTITY  errorCertAttrHasUpdate.label    "Something is trying to trick &brandShortName; into accepting an
>+                                           insecure update. Please contact your network provider and seek help.">
>+
> <!ENTITY  finishedPage.title              "Update Ready to Install">
> <!ENTITY  finishedPage.text               "The update will be installed the next time &brandShortName; starts. You 
>                                            can restart &brandShortName; now, or continue working and restart later.">
> 
> <!ENTITY  finishedBackgroundPage.text     "A security and stability update for &brandShortName; has been
>                                            downloaded and is ready to be installed.">
> <!ENTITY  finishedBackground.name         "Update:">
> <!-- LOCALIZATION NOTE (finishedBackground.more): This string describes the button labels defined by restartNowButton and restartLaterButton in updates.properties. -->
>diff --git a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>--- a/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>+++ b/toolkit/locales/en-US/chrome/mozapps/update/updates.properties
>@@ -27,17 +27,16 @@ intro_minor=A security and stability upd
> # Example: 2.1.5
> addonLabel=%1$S %2$S
> 
> updateType_major=New Version
> updateType_minor=Security Update
> 
> # LOCALIZATION NOTE: When present %S is brandShortName
> verificationError=%S could not confirm the integrity of the update package.
>-errorsPageHeader=Update Failed
> licenseContentNotFound=The license file for this version could not be found. Please visit the %S homepage for more information.
> updateMoreInfoContentNotFound=Additional details about this version could not be found. Please visit the %S homepage for more information.
> resumePausedAfterCloseTitle=Software Update
> resumePausedAfterCloseMsg=You have paused downloading this update. Do you want to download the update in the background while you continue to use %S?
> updaterIOErrorTitle=Software Update Failed
> updaterIOErrorMsg=The update could not be installed. Please make sure there are no other copies of %S running on your computer, and then restart %S to try again.
> okButton=OK
> okButton.accesskey=O
>diff --git a/toolkit/mozapps/update/content/updates.js b/toolkit/mozapps/update/content/updates.js
>--- a/toolkit/mozapps/update/content/updates.js
>+++ b/toolkit/mozapps/update/content/updates.js
>@@ -1574,19 +1574,16 @@ var gDownloadingPage = {
> var gErrorsPage = {
>   /**
>    * Initialize
>    */
>   onPageShow: function() {
>     gUpdates.setButtons(null, null, "okButton", true);
>     gUpdates.wiz.getButton("finish").focus();
> 
>-    var errorsTitle = gUpdates.getAUSString("errorsPageHeader");
>-    document.getElementById("errorsHeader").setAttribute("label", errorsTitle);
>-
>     var statusText = gUpdates.update.statusText;
>     LOG("gErrorsPage" , "onPageShow - update.statusText: " + statusText);
> 
>     var errorReason = document.getElementById("errorReason");
>     errorReason.value = statusText;
>     var manualURL = Services.urlFormatter.formatURLPref(PREF_APP_UPDATE_MANUAL_URL);
>     var errorLinkLabel = document.getElementById("errorLinkLabel");
>     errorLinkLabel.value = manualURL;
>diff --git a/toolkit/mozapps/update/content/updates.xul b/toolkit/mozapps/update/content/updates.xul
>--- a/toolkit/mozapps/update/content/updates.xul
>+++ b/toolkit/mozapps/update/content/updates.xul
>@@ -210,17 +210,17 @@
>         <image id="verificationFailedIcon"/>
>         <label flex="1">&verificationFailedText.label;</label>
>       </hbox>
>     </vbox>
>   </wizardpage>
>   
>   <wizardpage id="errors" pageid="errors" object="gErrorsPage"
>               onpageshow="gErrorsPage.onPageShow();">
>-    <updateheader id="errorsHeader" label=""/>
>+    <updateheader label="&error.title;"/>
>     <vbox class="update-content" flex="1">
>       <label id="errorIntro">&error.label;</label>
>       <separator/>
>       <textbox class="plain" readonly="true" id="errorReason" multiline="true"
>                rows="3"/>
>       <separator/>
>       <label id="errorManual">&errorManual.label;</label>
>       <hbox>
>@@ -228,17 +228,17 @@
>                onclick="openUpdateURL(event);"/>
>       </hbox>
>     </vbox>
>   </wizardpage>
>   
>   <wizardpage id="errorpatching" pageid="errorpatching" next="downloading"
>               object="gErrorPatchingPage"
>               onpageshow="gErrorPatchingPage.onPageShow();">
>-    <updateheader label="&errorpatching.title;"/>
>+    <updateheader label="&error.title;"/>
>     <vbox class="update-content" flex="1">
>       <label>&errorpatching.intro;</label>
>     </vbox>
>   </wizardpage>
>   
>   <wizardpage id="finished" pageid="finished" object="gFinishedPage"
>               onpageshow="gFinishedPage.onPageShow();"
>               onextra1="gFinishedPage.onExtra1()">

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: patch strings - ready to import (checked in)

Comment 10

[Robert Strong (they/them - no direct email)]

Comment on attachment 470032 [details] [diff] [review]
patch strings - ready to import (checked in)

Strings pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/998223ee5613

I'm going to try to finish the rest over the weekend

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Gonna move this to b6; strings are in, we can just hook 'em up later.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: 1. patch rev1

Still want to write a few more app update tests and I'll submit them hopefully tonight.

Comment 13

[Robert Strong (they/them - no direct email)]

Attachment: 2. test patch rev1

Comment 14

[Robert Strong (they/them - no direct email)]

Attachment: combined patch

Dave, if you haven't started the review yet here is a patch containing both patches 1 and 2.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 470959 [details] [diff] [review]
1. patch rev1

Are we clearing the error count for every manual update check?

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js

>+// When |app.update.cert.requireBuiltIn| is true or not specified the
>+// certificate and all intermediate certficates used to connect to the url

*certificates, though I think this needs re-wording to avoid the term "intermediate certificates" which has other connotations for SSL. How about "all certificates for requests that redirected to the final response"?

>diff --git a/toolkit/mozapps/update/content/updates.xul b/toolkit/mozapps/update/content/updates.xul

>+  <wizardpage id="errorcertcheck" pageid="errorcertcheck"
>+              object="gErrorCertCheckPage"
>+              onpageshow="gErrorCertCheckPage.onPageShow();">
>+    <updateheader label="&error.title;"/>
>+    <vbox class="update-content" flex="1">
>+      <label id="errorCertAttrHasUpdateLabel"
>+             hidden="true">&errorCertAttrHasUpdate.label;</label>
>+      <label id="errorCertCheckNoUpdateLabel"
>+             hidden="true">&errorCertAttrNoUpdate.label;</label>
>+      <hbox>
>+        <label id="errorCertAttrLinkLabel" class="text-link" hidden="true"
>+               value="" onclick="openUpdateURL(event);"/>
>+      </hbox>
>+    </vbox>
>+  </wizardpage>

Are there meant to be some strings here too?

Comment 16

[Robert Strong (they/them - no direct email)]

(In reply to comment #15)
> Comment on attachment 470959 [details] [diff] [review]
> 1. patch rev1
> 
> Are we clearing the error count for every manual update check?
It is cleared on every successful manual check and it is also cleared if it fails during a manual check.

> >diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
> 
> >+// When |app.update.cert.requireBuiltIn| is true or not specified the
> >+// certificate and all intermediate certficates used to connect to the url
> 
> *certificates, though I think this needs re-wording to avoid the term
> "intermediate certificates" which has other connotations for SSL. How about
> "all certificates for requests that redirected to the final response"?
Will do

> 
> >diff --git a/toolkit/mozapps/update/content/updates.xul b/toolkit/mozapps/update/content/updates.xul
> 
> >+  <wizardpage id="errorcertcheck" pageid="errorcertcheck"
> >+              object="gErrorCertCheckPage"
> >+              onpageshow="gErrorCertCheckPage.onPageShow();">
> >+    <updateheader label="&error.title;"/>
> >+    <vbox class="update-content" flex="1">
> >+      <label id="errorCertAttrHasUpdateLabel"
> >+             hidden="true">&errorCertAttrHasUpdate.label;</label>
> >+      <label id="errorCertCheckNoUpdateLabel"
> >+             hidden="true">&errorCertAttrNoUpdate.label;</label>
> >+      <hbox>
> >+        <label id="errorCertAttrLinkLabel" class="text-link" hidden="true"
> >+               value="" onclick="openUpdateURL(event);"/>
> >+      </hbox>
> >+    </vbox>
> >+  </wizardpage>
> 
> Are there meant to be some strings here too?
Already checked in

Comment 17

[Robert Strong (they/them - no direct email)]

Attachment: 1. patch rev2 - comments addressed

Comment 18

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/c9ee9a98f2d4
http://hg.mozilla.org/mozilla-central/rev/3c3c95c37d20

Comment 19

[Robert Strong (they/them - no direct email)]

SeaMonkey already has a bug to implement this so just cc'ing Thunderbird people.

See bug 593135 where kairo has listed what needs to be done.

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: followup test only fix for SeaMonkey and Thunderbird

Showed this to Mossop and he verbally r+'d it.

Comment 21

[Robert Strong (they/them - no direct email)]

Comment on attachment 471673 [details] [diff] [review]
followup test only fix for SeaMonkey and Thunderbird

Followup pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/6b4c1d5a9c14

Comment 22

[Rich Gray (:rbgray)]

I believe this should be revisited or a new bug opened to apply Heinlein's Razor:

"Never attribute to malice that which can be adequately explained by stupidity, but don't rule out malice"

to the errorCertAttrHasUpdate.label "Something is trying to trick &brandShortName; into accepting an insecure update. Please contact your network provider and seek help." message.

Bug 653830 and related illustrate how many non-malicious causes (firewalls/proxies, anti-virus, Mozilla infrastructure problems, etc.) can cause this message.  SeaMonkey experienced a problem with it's update server certificate.  Version 2.33.1 and earlier will fail with this message.  The only way to update is to download a full install.  http://www.dslreports.com/forum/r30314865-WTF-SeaMonkey-being-tricked-to-bad-update

So perhaps the message should include alternate, non-malicious possibilities for the error and maybe suggest that the user visit the product web page and/or check Mozilla support pages.

Comment 23

[Robert Strong (they/them - no direct email)]

For Firefox we no longer do cert attribute checks.
SeaMonkey has bug 1189845 to no longer use the cert attribute checks.
App update has bug 1182352 to remove the cert attribute checking code entirely instead of apps just disabling them.

Because of this there is no reason to revisit or open a new bug since there are already bugs to remove it entirely.