Closed Bug 731382 Opened 8 years ago Closed 8 years ago

Port security enhancements for silent updates to Aurora

Categories

(Toolkit :: Application Update, defect, P1)

12 Branch
x86_64
Windows 7
defect

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox12 --- fixed

People

(Reporter: bbondy, Assigned: bbondy)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

Attached patch Aggregated changesets (obsolete) — Splinter Review
This bug is to port the security enhancements that the security team said is needed before the service can land on Mozilla-Beta.  All of these bugs landed on v13 but need to be on v12 Aurora before the next migration.
  
The sooner this can land the better because more testing can be done on Aurora before it reaches beta. Also there is a test day this Friday for Aurora and I'd like to get 2 Nightly builds in before then.

Almost everything stayed 100% the same as what's on mozilla-central (no rebasing).
The only change is the MAR_CHANNEL_ID and ACCEPTED_MAR_CHANNEL_IDS values in browser/confvars.sh which needed to be adjusted to: firefox-mozilla-aurora.

---

From oldest to newest the changesets included in this patch are:

http://hg.mozilla.org/mozilla-central/rev/033ca7c5bce8
Bug 699700 - Add support for signing and verifying MAR files in libmar and the mar program.

http://hg.mozilla.org/mozilla-central/rev/94f7927299e5
Bug 704285 - Include certificates inside updater.exe and use them to verify MARs.

http://hg.mozilla.org/mozilla-central/rev/58402b43c9e1 
Bug 704285 - Certificates for updater cert check.

http://hg.mozilla.org/mozilla-central/rev/ef967515537b
Bug 711139 - MOZ_VERIFY_MAR_SIGNATURE build option for verifying MAR signatures.

http://hg.mozilla.org/mozilla-central/rev/57ab36828a9e
Bug 708690 - libmar enhancements for product information blocks.

http://hg.mozilla.org/mozilla-central/rev/4de2d63b21cf
Bug 708690 - Updater enhancements for product information blocks.

http://hg.mozilla.org/mozilla-central/rev/c7f5af7dad8d
Bug 708690 - Build config for product information blocks.

http://hg.mozilla.org/mozilla-central/rev/ad8fe9d36d16
Bug 720688 - Ability to strip MAR signatures in libmar.

http://hg.mozilla.org/mozilla-central/rev/df3801e30225
Bug 708690 - XPCShell test enhancements for product information blocks.

http://hg.mozilla.org/mozilla-central/rev/52bd72d77fda
Bug 708690 - Updated MAR files for product information blocks.

http://hg.mozilla.org/mozilla-central/rev/d74612c58245
Bug 721758 - Ability to configure updater to accept multiple MAR IDs.

http://hg.mozilla.org/mozilla-central/rev/0bde78f1b76a
Bug 728301 - Enable new security checks only for the service.

http://hg.mozilla.org/mozilla-central/rev/bfb987083bcb
Bug 725180 - Create test suite for libmar.

http://hg.mozilla.org/mozilla-central/rev/4d9778e932ba
Bug 725180 - Misc libmar fixes found from tests.

http://hg.mozilla.org/mozilla-central/rev/2b1a53905350
Bug 725180 - Updater JS error handling for new error codes.

http://hg.mozilla.org/mozilla-central/rev/5439f4751116
Bug 730862 - Disable signmar by default and provide an option to enable it.

http://hg.mozilla.org/integration/mozilla-inbound/rev/5b9b207ac6ea
Bug 730821 - Don't do security enhancements on x64 Windows builds.

---

[Approval Request Comment]
Regression caused by (bug #): These are security enhancements that should be included and landed from the service work in bug 481815
User impact if declined: Users may be subject to concerns from the security team
Testing completed (on m-c, etc.): Everything except the last patch which is trivial and safe has been tested on m-c for several days.
Risk to taking this patch (and alternatives if risky): There are some risks since it is over 300KB of changes, but it is a recommendation from the security team.
String changes made by this patch: None
Attachment #601380 - Flags: review?(robert.bugzilla)
Attachment #601380 - Flags: approval-mozilla-aurora?
Summary: Port security enhancements to Aurora → Port security enhancements for silent updates to Aurora
Attached patch Patch v2. (obsolete) — Splinter Review
Added in these ones as well:

https://hg.mozilla.org/mozilla-central/rev/1c3efb9a3fed
Bug 725876 - Rename MOZ_UTILS_LDFLAGS to MOZ_GLUE_LDFLAGS and MOZ_UTILS_PROGRAM_LDFLAGS to MOZ_GLUE_PROGRAM_LDFLAGS

https://hg.mozilla.org/mozilla-central/rev/7e3d5c2879a8
Bug 725568 - MozillaMaintenance service can crash after applying updates
Attachment #601380 - Attachment is obsolete: true
Attachment #601398 - Flags: review?(robert.bugzilla)
Attachment #601398 - Flags: approval-mozilla-aurora?
Attachment #601380 - Flags: review?(robert.bugzilla)
Attachment #601380 - Flags: approval-mozilla-aurora?
Comment on attachment 601398 [details] [diff] [review]
Patch v2.

[Triage Comment]
QA has signed off on this feature, it's a requirement for landing UAC for FF12, and we have a test day for silent updates this week which should shake out any regressions. Not to mention the fact that Aurora will update at least 14 times before it goes to Beta. Approving for Aurora 12. Please land ASAP (once we get r+'d on the rolled up patch if necessary).
Attachment #601398 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 601398 [details] [diff] [review]
Patch v2.

Please also port the fix to UpdateLog::Init in updatelogging.cpp and updatelogging.h
Specifically, removal of const from NS_tchar* fileName
Comment on attachment 601398 [details] [diff] [review]
Patch v2.

The only difference that isn't clear to me is the following (+ denotes existence on m-a and not m-c).

--- ../../_1_mozilla-central/mozilla/toolkit/mozapps/update/updater/updater.cpp	2012-02-28 13:14:09 -0800
+++ toolkit/mozapps/update/updater/updater.cpp	2012-02-28 13:18:05 -0800
@@ -1457,17 +1457,16 @@
 
   static const char kApplying[] = "applying";
   if (fwrite(kApplying, strlen(kApplying), 1, file) != 1)
     return false;
 
   return true;
 }
 
-#ifdef MOZ_MAINTENANCE_SERVICE
 /* 
  * Read the update.status file and sets isPendingService to true if
  * the status is set to pending-service.
  *
  * @param  isPendingService Out parameter for specifying if the status
  *         is set to pending-service or not.
  * @return true if the information was retrieved and it is pending
  *         or pending-service.
@@ -1492,19 +1491,17 @@
   const char kPendingService[] = "pending-service";
   isPending = strncmp(buf, kPending, 
                       sizeof(kPending) - 1) == 0;
 
   isPendingService = strncmp(buf, kPendingService, 
                              sizeof(kPendingService) - 1) == 0;
   return isPending;
 }
-#endif
 
-#ifdef XP_WIN
 /* 
  * Read the update.status file and sets isSuccess to true if
  * the status is set to succeeded.
  *
  * @param  isSucceeded Out parameter for specifying if the status
  *         is set to succeeded or not.
  * @return true if the information was retrieved and it is succeeded.
 */
@@ -1524,16 +1521,17 @@
   fread(buf, sizeof(buf), 1, file);
 
   const char kSucceeded[] = "succeeded";
   isSucceeded = strncmp(buf, kSucceeded, 
                         sizeof(kSucceeded) - 1) == 0;
   return true;
 }
 
+#ifdef XP_WIN
 static void 
 WaitForServiceFinishThread(void *param)
 {
   // We wait at most 10 minutes, we already waited 5 seconds previously
   // before deciding to show this UI.
   WaitForServiceStop(SVC_NAME, 595);
   LOG(("calling QuitProgressUI\n"));
   QuitProgressUI();



Please remove isComplete as well

@@ -2705,32 +2614,34 @@
     LOG(("DoUpdate: error opening manifest file: " LOG_S "\n", manifest));
     return READ_ERROR;
   }
 
 
   ActionList list;
   NS_tchar *line;
   bool isFirstAction = true;
+  bool isComplete = false;
 
   while((line = mstrtok(kNL, &rb)) != 0) {
     // skip comments
     if (*line == NS_T('#'))
       continue;
 
     NS_tchar *token = mstrtok(kWhitespace, &line);
     if (!token) {
       LOG(("DoUpdate: token not found in manifest\n"));
       return PARSE_ERROR;
     }
 
     if (isFirstAction && NS_tstrcmp(token, NS_T("type")) == 0) {
       const NS_tchar *type = mstrtok(kQuote, &line);
       LOG(("UPDATE TYPE " LOG_S "\n", type));
       if (NS_tstrcmp(type, NS_T("complete")) == 0) {
+        isComplete = true;
         rv = AddPreCompleteActions(&list);
         if (rv)
           return rv;
       }
       isFirstAction = false;
       continue;
     }
 
r-ing mainly because of the first item above where it isn't clear why these ifdef's don't also exist on aurora (is it due to the x64 patch?).
Attachment #601398 - Flags: review?(robert.bugzilla) → review-
> it isn't clear why these ifdef's don't also exist on aurora 
>(is it due to the x64 patch?).

That change is because there's another compiler warning patch for non Windows platforms for unreferenced functions.  I'll include that patch as well
Attached patch Patch v3.Splinter Review
Implicitly carrying forward the a+ since I can't mark an a+ myself.

Turns out both of those warning changes are the same changeset:

Bug 727156 - fix compiler warnings in toolkit/mozapps/update/
https://hg.mozilla.org/mozilla-central/rev/a8634484e86b
Attachment #601398 - Attachment is obsolete: true
Attachment #601459 - Flags: review?(robert.bugzilla)
Comment on attachment 601459 [details] [diff] [review]
Patch v3.

The interdiff between m-c and m-a with this patch looks good!
Attachment #601459 - Flags: review?(robert.bugzilla) → review+
Depends on: 731473
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.