Notify user when the certificate attribute check fails

RESOLVED FIXED in mozilla2.0b7

Status

()

Toolkit
Application Update
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: rstrong, Assigned: rstrong)

Tracking

Trunk
mozilla2.0b7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(6 attachments, 1 obsolete attachment)

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.
Taking for design.
Assignee: nobody → beltzner
beltzner, any update on this?
blocking2.0: --- → ?
blocking2.0: ? → beta5+
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.
Rob, lemme know if you think we can at least get the strings in for beta5.
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"
Created attachment 469997 [details] [diff] [review]
patch strings
Assignee: beltzner → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #469997 - Flags: ui-review?(beltzner)
Attachment #469997 - Flags: review?(dtownsend)
Comment on attachment 469997 [details] [diff] [review]
patch strings

uir=beltzner
Attachment #469997 - Flags: ui-review?(beltzner) → ui-review+
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()">
Attachment #469997 - Flags: review?(dtownsend) → review+
Created attachment 470032 [details] [diff] [review]
patch strings - ready to import (checked in)
Attachment #470032 - Flags: review+
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
Attachment #470032 - Attachment description: patch - ready to import → patch strings - ready to import (checked in)
Gonna move this to b6; strings are in, we can just hook 'em up later.
blocking2.0: beta5+ → beta6+
Created attachment 470959 [details] [diff] [review]
1. patch rev1

Still want to write a few more app update tests and I'll submit them hopefully tonight.
Attachment #470959 - Flags: review?(dtownsend)
Created attachment 471057 [details] [diff] [review]
2. test patch rev1
Attachment #471057 - Flags: review?(dtownsend)
Created attachment 471060 [details] [diff] [review]
combined patch

Dave, if you haven't started the review yet here is a patch containing both patches 1 and 2.
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?
Attachment #470959 - Flags: review?(dtownsend) → review+
(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
Created attachment 471279 [details] [diff] [review]
1. patch rev2 - comments addressed
Attachment #470959 - Attachment is obsolete: true
Attachment #471279 - Flags: review+
Attachment #471057 - Flags: review?(dtownsend) → review+
Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/c9ee9a98f2d4
http://hg.mozilla.org/mozilla-central/rev/3c3c95c37d20
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b6

Updated

7 years ago
Blocks: 593135
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.
Created attachment 471673 [details] [diff] [review]
followup test only fix for SeaMonkey and Thunderbird

Showed this to Mossop and he verbally r+'d it.
Attachment #471673 - Flags: review+
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
Blocks: 583667
Blocks: 593571
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.
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.
You need to log in before you can comment on or make changes to this bug.