Bug 709183 - Callback path and arguments untrusted, don't use/pass them. r=rstrong.

toolkit/components/maintenanceservice/workmonitor.cpp

@@ -97,8 +97,6 @@ StartUpdateProcess(LPCWSTR updaterPath,
 
   // The updater command line is of the form:
   // updater.exe update-dir apply [wait-pid [callback-dir callback-path args]]
-  // So update callback-dir is the 4th, callback-path is the 5th and its args 
-  // are the 6th index.
   LPWSTR cmdLine = MakeCommandLine(argcTmp, argvTmp);
 
   // If we're about to start the update process from session 0,
@@ -187,10 +185,9 @@ StartUpdateProcess(LPCWSTR updaterPath,
   if (selfHandlePostUpdate) {
     MoveFileEx(updaterINITemp, updaterINI, MOVEFILE_REPLACE_EXISTING);
 
-    // Only run the PostUpdate if the update was successful and if we have
-    // a callback application.  This is the same thing updater.exe does.
-    if (updateWasSuccessful && argcTmp > 5) {
-      LPCWSTR callbackApplication = argvTmp[5];
+    // Only run the PostUpdate if the update was successful
+    if (updateWasSuccessful && argcTmp > 2) {
+      LPCWSTR installationDir = argvTmp[2];
       LPCWSTR updateInfoDir = argvTmp[1];
 
       // Launch the PostProcess with admin access in session 0.  This is
@@ -200,8 +197,10 @@ StartUpdateProcess(LPCWSTR updaterPath,
       // the unelevated updater.exe after the update process is complete
       // from the service.  We don't know here which session to start
       // the user PostUpdate process from.
-      LOG(("Launching post update process as the service in session 0."));
-      LaunchWinPostProcess(callbackApplication, updateInfoDir, true, NULL);
+      LOG(("Launching post update process as the service in session 0.\n"));
+      if (!LaunchWinPostProcess(installationDir, updateInfoDir, true, NULL)) {
+        LOG(("The post update process could not be launched.\n"));
+      }
     }
   }
 

toolkit/mozapps/update/common/updatehelper.cpp

@@ -110,30 +110,28 @@ PathGetSiblingFilePath(LPWSTR destinationBuffer,
  * of helper.exe.  For service updates this is called from both the system
  * account and the current user account.
  *
- * @param  appExe        The path to the callback application binary.
- * @param  updateInfoDir The directory where update info is stored.
- * @param  forceSync     If true even if the ini file specifies async, the
- *                       process will wait for termination of PostUpdate.
- * @param  userToken     The user token to run as, if NULL the current user
- *                       will be used.
+ * @param  installationDir The path to the callback application binary.
+ * @param  updateInfoDir   The directory where update info is stored.
+ * @param  forceSync       If true even if the ini file specifies async, the
+ *                         process will wait for termination of PostUpdate.
+ * @param  userToken       The user token to run as, if NULL the current user
+ *                         will be used.
  * @return TRUE if there was no error starting the process.
  */
 BOOL
-LaunchWinPostProcess(const WCHAR *appExe,
+LaunchWinPostProcess(const WCHAR *installationDir,
                      const WCHAR *updateInfoDir,
                      bool forceSync,
                      HANDLE userToken)
 {
   WCHAR workingDirectory[MAX_PATH + 1];
-  wcscpy(workingDirectory, appExe);
-  if (!PathRemoveFileSpecW(workingDirectory)) {
-    return FALSE;
-  }
+  wcscpy(workingDirectory, installationDir);
 
   // Launch helper.exe to perform post processing (e.g. registry and log file
   // modifications) for the update.
   WCHAR inifile[MAX_PATH + 1];
-  if (!PathGetSiblingFilePath(inifile, appExe, L"updater.ini")) {
+  wcscpy(inifile, installationDir);
+  if (!PathAppendSafe(inifile, L"updater.ini")) {
     return FALSE;
   }
 
@@ -153,13 +151,15 @@ LaunchWinPostProcess(const WCHAR *appExe,
 
   if (!GetPrivateProfileStringW(L"PostUpdateWin", L"ExeAsync", L"TRUE", 
                                 exeasync,
-                                sizeof(exeasync)/sizeof(exeasync[0]), inifile)) {
+                                sizeof(exeasync)/sizeof(exeasync[0]), 
+                                inifile)) {
     return FALSE;
   }
 
   WCHAR exefullpath[MAX_PATH + 1];
-  if (!PathGetSiblingFilePath(exefullpath, appExe, exefile)) {
-    return FALSE;
+  wcscpy(exefullpath, installationDir);
+  if (!PathAppendSafe(exefullpath, exefile)) {
+    return false;
   }
 
   WCHAR dlogFile[MAX_PATH + 1];
@@ -420,7 +420,10 @@ WinLaunchServiceCommand(LPCWSTR exePath, int argc, LPWSTR* argv)
                           &commandIDWrote, NULL);
 
   // Write out the command line arguments that are passed to updater.exe
-  WCHAR *commandLineBuffer = MakeCommandLine(argc, argv);
+  // updater.exe's command line arguments look like this normally:
+  // updater.exe update-dir apply [wait-pid [callback-dir callback-path args]]
+  // We want everything except the callback application and its arguments.
+  LPWSTR commandLineBuffer = MakeCommandLine(min(argc, 4), argv);
   if (!commandLineBuffer) {
     return FALSE;
   }

toolkit/mozapps/update/common/updatehelper.h

@@ -35,7 +35,7 @@
  *
  * ***** END LICENSE BLOCK ***** */
 
-BOOL LaunchWinPostProcess(const WCHAR *appExe, 
+BOOL LaunchWinPostProcess(const WCHAR *installationDir,
                           const WCHAR *updateInfoDir, 
                           bool forceSync,
                           HANDLE userToken);

toolkit/mozapps/update/updater/updater.cpp

@@ -1756,7 +1756,10 @@ int NS_main(int argc, NS_tchar **argv)
         bool updateStatusSucceeded = false;
         if (IsUpdateStatusSucceeded(updateStatusSucceeded) && 
             updateStatusSucceeded) {
-          LaunchWinPostProcess(argv[callbackIndex], gSourcePath, false, NULL);
+          if (!LaunchWinPostProcess(argv[2], gSourcePath, false, NULL)) {
+            fprintf(stderr, "The post update process which runs as the user"
+                    " for service update could not be launched.");
+          }
         }
       }