Bug 1074793 - Set more restrictive permissions for downloads in the temporary directory. r=paolo

netwerk/base/src/BackgroundFileSaver.cpp

@@ -618,10 +618,15 @@ BackgroundFileSaver::ProcessStateChange()
   }
 
   // Create the target file, or append to it if we already started writing it.
+  // The 0600 permissions are used while the file is being downloaded, and for
+  // interrupted downloads. Those may be located in the system temporary
+  // directory, as well as the target directory, and generally have a ".part"
+  // extension. Those part files should never be group or world-writable even
+  // if the umask allows it.
   nsCOMPtr<nsIOutputStream> outputStream;
   rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream),
                                    mActualTarget,
-                                   PR_WRONLY | creationIoFlags, 0644);
+                                   PR_WRONLY | creationIoFlags, 0600);
   NS_ENSURE_SUCCESS(rv, rv);
 
   outputStream = NS_BufferOutputStream(outputStream, BUFFERED_IO_SIZE);

toolkit/components/downloads/nsDownloadManager.cpp

@@ -3326,23 +3326,45 @@ nsDownload::ExecuteDesiredAction()
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
-  nsresult retVal = NS_OK;
+  nsresult rv = NS_OK;
   switch (action) {
     case nsIMIMEInfo::saveToDisk:
       // Move the file to the proper location
-      retVal = MoveTempToTarget();
+      rv = MoveTempToTarget();
+      if (NS_SUCCEEDED(rv)) {
+        rv = FixTargetPermissions();
+      }
       break;
     case nsIMIMEInfo::useHelperApp:
     case nsIMIMEInfo::useSystemDefault:
       // For these cases we have to move the file to the target location and
       // open with the appropriate application
-      retVal = OpenWithApplication();
+      rv = OpenWithApplication();
       break;
     default:
       break;
   }
 
-  return retVal;
+  return rv;
+}
+
+nsresult
+nsDownload::FixTargetPermissions()
+{
+  nsCOMPtr<nsIFile> target;
+  nsresult rv = GetTargetFile(getter_AddRefs(target));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // Set perms according to umask.
+  nsCOMPtr<nsIPropertyBag2> infoService =
+      do_GetService("@mozilla.org/system-info;1");
+  uint32_t gUserUmask = 0;
+  rv = infoService->GetPropertyAsUint32(NS_LITERAL_STRING("umask"),
+                                        &gUserUmask);
+  if (NS_SUCCEEDED(rv)) {
+    rv = target->SetPermissions(0666 & ~gUserUmask);
+  }
+  return rv;
 }
 
 nsresult
@@ -3368,9 +3390,7 @@ nsDownload::MoveTempToTarget()
   rv = target->GetParent(getter_AddRefs(dir));
   NS_ENSURE_SUCCESS(rv, rv);
   rv = mTempFile->MoveTo(dir, fileName);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  return NS_OK;
+  return rv;
 }
 
 nsresult
@@ -3385,12 +3405,6 @@ nsDownload::OpenWithApplication()
   rv = MoveTempToTarget();
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // We do not verify the return value here because, irrespective of success
-  // or failure of the method, the deletion of temp file has to take place, as
-  // per the corresponding preference. But we store this separately as this is
-  // what we ultimately return from this function.
-  nsresult retVal = mMIMEInfo->LaunchWithFile(target);
-
   bool deleteTempFileOnExit;
   nsCOMPtr<nsIPrefBranch> prefs(do_GetService(NS_PREFSERVICE_CONTRACTID));
   if (!prefs || NS_FAILED(prefs->GetBoolPref(PREF_BH_DELETETEMPFILEONEXIT,
@@ -3409,6 +3423,10 @@ nsDownload::OpenWithApplication()
   // Always schedule files to be deleted at the end of the private browsing
   // mode, regardless of the value of the pref.
   if (deleteTempFileOnExit || mPrivate) {
+
+    // Make the tmp file readonly so users won't lose changes.
+    target->SetPermissions(0400);
+
     // Use the ExternalHelperAppService to push the temporary file to the list
     // of files to be deleted on exit.
     nsCOMPtr<nsPIExternalAppLauncher> appLauncher(do_GetService
@@ -3425,7 +3443,7 @@ nsDownload::OpenWithApplication()
     }
   }
 
-  return retVal;
+  return mMIMEInfo->LaunchWithFile(target);
 }
 
 void

toolkit/components/downloads/nsDownloadManager.h

@@ -307,6 +307,11 @@ protected:
    */
   nsresult MoveTempToTarget();
 
+  /**
+   * Set the target file permissions to be appropriate.
+   */
+  nsresult FixTargetPermissions();
+
   /**
    * Update the start time which also implies the last update time is the same.
    */

toolkit/components/jsdownloads/src/DownloadIntegration.jsm

@@ -612,15 +612,29 @@ this.DownloadIntegration = {
       }
 #endif
 
-      // Now that the file is completely downloaded, make it accessible by other
-      // users on this system.  On Unix, the umask of the process is respected.
-      // This call has no effect on Windows.
+      // The file with the partially downloaded data has restrictive permissions
+      // that don't allow other users on the system to access it.  Now that the
+      // download is completed, we need to adjust permissions based on whether
+      // this is a permanently downloaded file or a temporary download to be
+      // opened read-only with an external application.
       try {
-        yield OS.File.setPermissions(aDownload.target.path,
-                                     { unixMode: 0o666 });
+        // The following logic to determine whether this is a temporary download
+        // is due to the fact that "deleteTempFileOnExit" is false on Mac, where
+        // downloads to be opened with external applications are preserved in
+        // the "Downloads" folder like normal downloads.
+        let isTemporaryDownload =
+          aDownload.launchWhenSucceeded && (aDownload.source.isPrivate ||
+          Services.prefs.getBoolPref("browser.helperApps.deleteTempFileOnExit"));
+        // Permanently downloaded files are made accessible by other users on
+        // this system, while temporary downloads are marked as read-only.
+        let unixMode = isTemporaryDownload ? 0o400 : 0o666;
+        // On Unix, the umask of the process is respected.  This call has no
+        // effect on Windows.
+        yield OS.File.setPermissions(aDownload.target.path, { unixMode });
       } catch (ex) {
-        // Errors with making the permissions less restrictive should be
-        // reported, but should not prevent the download from completing.
+        // We should report errors with making the permissions less restrictive
+        // or marking the file as read-only on Unix and Mac, but this should not
+        // prevent the download from completing.
         Cu.reportError(ex);
       }
 

toolkit/components/jsdownloads/test/unit/common_test_Download.js

@@ -14,6 +14,8 @@
 ////////////////////////////////////////////////////////////////////////////////
 //// Globals
 
+const kDeleteTempFileOnExit = "browser.helperApps.deleteTempFileOnExit";
+
 /**
  * Creates and starts a new download, using either DownloadCopySaver or
  * DownloadLegacySaver based on the current test run.
@@ -170,21 +172,55 @@ add_task(function test_basic_tryToKeepPartialData()
 /**
  * Tests the permissions of the final target file once the download finished.
  */
-add_task(function test_basic_unix_permissions()
+add_task(function test_unix_permissions()
 {
   // This test is only executed on Linux and Mac.
   if (Services.appinfo.OS != "Darwin" && Services.appinfo.OS != "Linux") {
-    do_print("Skipping test_basic_unix_permissions");
+    do_print("Skipping test.");
     return;
   }
 
-  let download = yield promiseStartDownload(httpUrl("source.txt"));
-  yield promiseDownloadStopped(download);
+  let launcherPath = getTempFile("app-launcher").path;
 
-  // The file should readable and writable by everyone, but the restrictions
-  // from the umask of the process should still apply.
+  for (let autoDelete of [false, true]) {
+    for (let isPrivate of [false, true]) {
+      for (let launchWhenSucceeded of [false, true]) {
+        do_print("Checking " + JSON.stringify({ autoDelete,
+                                                isPrivate,
+                                                launchWhenSucceeded }));
+
+        Services.prefs.setBoolPref(kDeleteTempFileOnExit, autoDelete);
+
+        let download;
+        if (!gUseLegacySaver) {
+          download = yield Downloads.createDownload({
+            source: { url: httpUrl("source.txt"), isPrivate },
+            target: getTempFile(TEST_TARGET_FILE_NAME).path,
+            launchWhenSucceeded,
+            launcherPath,
+          });
+          yield download.start();
+        } else {
+          download = yield promiseStartLegacyDownload(httpUrl("source.txt"), {
+            isPrivate,
+            launchWhenSucceeded,
+            launcherPath: launchWhenSucceeded && launcherPath,
+          });
+          yield promiseDownloadStopped(download);
+        }
+
+        // Temporary downloads should be read-only and not accessible to other
+        // users, while permanently downloaded files should be readable and
+        // writable as specified by the system umask.
+        let isTemporary = launchWhenSucceeded && (autoDelete || isPrivate);
         do_check_eq((yield OS.File.stat(download.target.path)).unixMode,
-              0o666 & ~OS.Constants.Sys.umask);
+                    isTemporary ? 0o400 : (0o666 & ~OS.Constants.Sys.umask));
+      }
+    }
+  }
+
+  // Clean up the changes to the preference.
+  Services.prefs.clearUserPref(kDeleteTempFileOnExit);
 });
 
 /**
@@ -774,6 +810,13 @@ add_task(function test_cancel_midway_restart_tryToKeepPartialData_false()
   do_check_false(download.hasPartialData);
   do_check_true(yield OS.File.exists(download.target.partFilePath));
 
+  // On Unix, verify that the file with the partially downloaded data is not
+  // accessible by other users on the system.
+  if (Services.appinfo.OS == "Darwin" || Services.appinfo.OS == "Linux") {
+    do_check_eq((yield OS.File.stat(download.target.partFilePath)).unixMode,
+                0o600);
+  }
+
   yield download.cancel();
 
   // The ".part" file should be deleted now that the download is canceled.

uriloader/exthandler/nsExternalHelperAppService.cpp

@@ -1454,7 +1454,7 @@ nsresult nsExternalAppHandler::SetUpTempFile(nsIChannel * aChannel)
   rv = mTempFile->Append(NS_ConvertUTF8toUTF16(tempLeafName));
   // make this file unique!!!
   NS_ENSURE_SUCCESS(rv, rv);
-  rv = mTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0644);
+  rv = mTempFile->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Now save the temp leaf name, minus the ".part" bit, so we can use it later.
@@ -2401,7 +2401,7 @@ NS_IMETHODIMP nsExternalAppHandler::LaunchWithApplication(nsIFile * aApplication
   fileToUse->Append(mSuggestedFileName);  
 #endif
 
-  nsresult rv = fileToUse->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0644);
+  nsresult rv = fileToUse->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
   if(NS_SUCCEEDED(rv)) {
     mFinalFileDestination = do_QueryInterface(fileToUse);
     // launch the progress window now that the user has picked the desired action.