User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:32.0) Gecko/20100101 Firefox/32.0 Build ID: 20140923175406 Steps to reproduce: On an attachment in a email, click "Open" and select "Open With..." and "Ok" Actual results: The attached file is opened using the selected external program, but the file is left world readable in /tmp Expected results: The attached file is opened using the selected external program, but the file is not accessible for other users (change permissions, remove file, etc). This bug is similar to bug 251297

Mark can you confirm?

Using ubuntu, it is world readable. iirc I remember some discussions and changes around these types of things in the past. I've a feeling Magnus might remember some things.

Probably fallout from bug 858234. See bug 1009465 too. In the flushed code there is - mFinalFileDestination->SetPermissions(0400) ... which I guess was what we did earlier (correctly). In tb24 it is correct.

Ok looking try run @ https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c9ac4897e0f2 I'm not sure why bugzilla refuses to let me ask for review here. (probably should be paolo.mozmail@amadzone.org)

The patch fixes bug bug 1009465 too.

Thanks for submitting a patch. Before proceeding with a more detailed review, I'd like to clarify the rationale behind the current behavior, as comment 0 may be slightly misleading. The bug was originally opened against Thunderbird, but the patch affects Firefox code as well, so I'll start from the Firefox situation. In Firefox, we now create files in the temporary directory that are world-readable. This doesn't mean that the file can modified by other users, and also note how it cannot be deleted or renamed, as the directory has the "sticky" bit. Thus, there isn't a security concern in the sense of the possibility of injecting foreign data or code into a user profile. There is a privacy concern related to this data being visible to other users, but it applies to limited circumstances. In fact, we don't download in "/tmp", but we honor the system setting for the temporary directory. Linux distributions designed for concurrent multi-user access, or system administrators who install such systems, would normally follow the best practice of configuring per-user temporary folders. In any case, temporary files are deleted at the end of the session, so the potential privacy issue would only apply to machines users can use at the same time. At the time, we decided this was an acceptable trade-off to get the bigger issue of bug 919076 fixed (the bug has the full context). Now that we have the OS.File API to change file permissions (bug 1001849), I agree we may want to use that in Firefox and set 600 permissions while downloading. We should take all the time required to build a proper patch for this, keeping the various possible download paths in mind. I don't have a lot of context with regard to the specific issue in Thunderbird. Most of the nsDownloadManager.cpp code is not used anymore in Firefox, and the implementation details may vary, but I guess the situation would be quite similar. I can't make an exact determination of the desired behavior there, but I guess that using 0600 in the temporary directory would also solve the original concern. (In reply to Magnus Melin from comment #5) > The patch fixes bug bug 1009465 too. Actually, the referenced bug is about making the file read-only for the current user for Firefox on all platforms, and on Windows we don't have an OS.File API to set a file's "read only" attribute. It is important to clarify that the maximum this patch can do would only fix a subset of that bug.

Comment on attachment 8511271 [details] [diff] [review] bug1074793_tmpfile_prems.patch Review of attachment 8511271 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libpref/init/all.js @@ +217,3 @@ > pref("browser.helperApps.deleteTempFileOnExit", false); > +#else > +pref("browser.helperApps.deleteTempFileOnExit", true); Is there a reason to keep this preference "false" on Mac OS X? Also, this change should probably apply to Thunderbird only. The preference is already unconditionally "true" in Firefox, and changing it probably won't have any practical effect on other products, but I'm not totally sure, and any change would need to be reviewed by Android and Firefox OS download module peers. ::: netwerk/base/src/BackgroundFileSaver.cpp @@ +620,5 @@ > // Create the target file, or append to it if we already started writing it. > nsCOMPtr<nsIOutputStream> outputStream; > rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), > mActualTarget, > + PR_WRONLY | creationIoFlags, 0666); These doesn't look like the permissions you may want for in-progress downloads. Did you mean 0600, or am I missing something? ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +3375,5 @@ > + rv = NS_GetSpecialDirectory(NS_OS_TEMP_DIR, getter_AddRefs(tempDir)); > + NS_ENSURE_SUCCESS(rv, rv); > + bool inSystemTmpDir = false; > + (void)dir->Equals(tempDir, &inSystemTmpDir); > + if (inSystemTmpDir) { nit: you may want to use the "if (NS_FAILED(...) || ...)" pattern for clarity. @@ +3424,5 @@ > // 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); These permissions don't apply to the "private" case, just the "deleteTempFileOnExit" one. ::: toolkit/components/jsdownloads/src/DownloadCore.jsm @@ +500,5 @@ > + let targetDir = new FileUtils.File(this.target.path).parent; > + // In the temp dir, use user readonly. Elsewhere follow umask. > + let mode = (tmpDir.path == targetDir.path) ? > + 0o400 : 0o666 & ~OS.Constants.Sys.umask; > + OS.File.setPermissions(this.target.path, { unixMode: mode }); This should be handled in DownloadIntegration.downloadDone. Also, the ".launchWhenSucceeded" flag is already a good indication of whether we downloaded to the temporary folder as a final destination. In other words, as you would expect, we never leave a file in the temporary directory unless we want to open it (and delete it later). You should adjust the production code, as well as the jsdownloads tests, to be based on that flag. We need tests to ensure that the partial data files for stopped downloads always have 0600 permissions. ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ +1917,5 @@ > do_check_true(yield OS.File.exists(noAutoDeleteTargetPath)); > > + do_check_true(yield OS.File.exists(autoDeleteTargetPathOne)); > + do_check_true(yield OS.File.exists(autoDeleteTargetPathTwo)); > + do_check_true(yield OS.File.exists(noAutoDeleteTargetPath)); I haven't looked at the test code in detail yet, but these seem duplicate. ::: toolkit/components/jsdownloads/test/unit/head.js @@ +130,5 @@ > > // Get a file reference under the temporary directory for this test file. > + // We use a Downloads folder underneath temp, so we can test for different > + // file privileges in system temp dir vs. system downloads dir. > + let file = FileUtils.getFile("TmpD", ["Downloads", leafName]); So, you don't need this global change (and handling the folder deletion) since you don't check the actual directory name. Thank you again for taking the time to write and test a patch for both Thunderbird and Firefox!

(In reply to :Paolo Amadini from comment #6) > In fact, we don't download in "/tmp", but we honor the system setting for > the temporary directory. Linux distributions designed for concurrent > multi-user access, or system administrators who install such systems, would > normally follow the best practice of configuring per-user temporary folders. I'm not convinced that is true. As I understand it that's a bit of trickery, and from what i've seen e.g. universities with shared general purpose servers seem to just use the global temp. That is also one common place the privacy issue would be more substantial. > I don't have a lot of context with regard to the specific issue in > Thunderbird. Most of the nsDownloadManager.cpp code is not used anymore in > Firefox, and the implementation details may vary, but I guess the situation > would be quite similar. I can't make an exact determination of the desired > behavior there, but I guess that using 0600 in the temporary directory would > also solve the original concern. Yes, but not bug 1009465. > (In reply to Magnus Melin from comment #5) > > The patch fixes bug bug 1009465 too. > > Actually, the referenced bug is about making the file read-only for the > current user for Firefox on all platforms, and on Windows we don't have an > OS.File API to set a file's "read only" attribute. It is important to > clarify that the maximum this patch can do would only fix a subset of that > bug. The paths used for opening files from the "Open with" dialog does not use OS.File API, and for files in general, the mozilla file apis do handle setting readonly on Windows for a file if its permissions that would imply it. So I'm not sure what part of it this patch would not fix? At least it's restoring the behavior we had for many years.

(In reply to :Paolo Amadini from comment #7) > ::: modules/libpref/init/all.js > @@ +217,3 @@ > > pref("browser.helperApps.deleteTempFileOnExit", false); > > +#else > > +pref("browser.helperApps.deleteTempFileOnExit", true); > > Is there a reason to keep this preference "false" on Mac OS X? I'm not sure, see http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/downloads/nsDownloadManager.cpp#3399 But, if there haven't been any complaints, maybe we can just drop that. > Also, this change should probably apply to Thunderbird only. The preference > is already unconditionally "true" in Firefox, and changing it probably won't > have any practical effect on other products, but I'm not totally sure, and It wouldn't no, except for thunderbird, seamonkey and possibly external 3rd parties that don't currently set the pref. > ::: netwerk/base/src/BackgroundFileSaver.cpp > @@ +620,5 @@ > > // Create the target file, or append to it if we already started writing it. > > nsCOMPtr<nsIOutputStream> outputStream; > > rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), > > mActualTarget, > > + PR_WRONLY | creationIoFlags, 0666); > > These doesn't look like the permissions you may want for in-progress > downloads. Did you mean 0600, or am I missing something? 0666 will give you permissions accoding to umask, and that is what I think is optimal. It doesn't matter that much if it's that or 0600 though as perms are fixed up later on success. > @@ +3424,5 @@ > > // 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); > > These permissions don't apply to the "private" case, just the > "deleteTempFileOnExit" one. Why not? In private mode files opened will get deleted when you end the session, so you we definitley want the file to be readonly in private mode.

(In reply to Magnus Melin from comment #8) > > I don't have a lot of context with regard to the specific issue in > > Thunderbird. Most of the nsDownloadManager.cpp code is not used anymore in > > Firefox, and the implementation details may vary, but I guess the situation > > would be quite similar. I can't make an exact determination of the desired > > behavior there, but I guess that using 0600 in the temporary directory would > > also solve the original concern. > > Yes, but not bug 1009465. Sorry for being unclear, 0600 while downloading and 0400 when finished is fine. > > (In reply to Magnus Melin from comment #5) > > Actually, the referenced bug is about making the file read-only for the > > current user for Firefox on all platforms, and on Windows we don't have an > > OS.File API to set a file's "read only" attribute. It is important to > > clarify that the maximum this patch can do would only fix a subset of that > > bug. > > The paths used for opening files from the "Open with" dialog does not use > OS.File API They do in Firefox (jsdownloads), and the bug is about the jsdownloads behavior. This will be true for Thunderbird once it migrates to jsdownloads as well. > So I'm not sure what part of it this patch would not fix? If you try it with Firefox on Windows, the opened file can still be overwritten. OS.File does not try to map permissions to file attributes (the "readonly" attribute in this case). (In reply to Magnus Melin from comment #9) > > Is there a reason to keep this preference "false" on Mac OS X? > > I'm not sure, see > http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/components/ > downloads/nsDownloadManager.cpp#3399 > But, if there haven't been any complaints, maybe we can just drop that. I've not come across any evidence that the comment is still valid on modern versions of the operating system. Firefox does not have any conditional behavior there. > > Also, this change should probably apply to Thunderbird only. The preference > > is already unconditionally "true" in Firefox, and changing it probably won't > > have any practical effect on other products, but I'm not totally sure, and > > It wouldn't no, except for thunderbird, seamonkey and possibly external 3rd > parties that don't currently set the pref. I'm fine with changing it in the global prefs file if you also get review for Android and Firefox OS. Or you may want to do it only for Thunderbird at first, and file a bug to change the pref globally later. > > ::: netwerk/base/src/BackgroundFileSaver.cpp > > @@ +620,5 @@ > > > // Create the target file, or append to it if we already started writing it. > > > nsCOMPtr<nsIOutputStream> outputStream; > > > rv = NS_NewLocalFileOutputStream(getter_AddRefs(outputStream), > > > mActualTarget, > > > + PR_WRONLY | creationIoFlags, 0666); > > > > These doesn't look like the permissions you may want for in-progress > > downloads. Did you mean 0600, or am I missing something? > > 0666 will give you permissions accoding to umask, and that is what I think > is optimal. It doesn't matter that much if it's that or 0600 though as perms > are fixed up later on success. These 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. Maybe worth adding a comment here and in the IDL... > > @@ +3424,5 @@ > > > // 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); > > > > These permissions don't apply to the "private" case, just the > > "deleteTempFileOnExit" one. > > Why not? In private mode files opened will get deleted when you end the > session, so you we definitley want the file to be readonly in private mode. Uh, my bad. You're right, this is always the path for "open with application".

Address review comments. Two things I did not change: - the move to DownloadIntegration.downloadDone. For some reason that doesn't work. And, the checks we need are conveniently in downloadcore already - the use of a "Downloads" dir in temp for. It doesn't really matter, but sure helped a great deal while trying to understand whats' going on

the use of a "Downloads" dir in temp for ... for the simulated Downloads dir, as opposed to the temp dir

(In reply to :Paolo Amadini from comment #10) > Firefox does not have any conditional behavior there. I was wrong in comment 10 about the conditional behavior. While there is no preprocessor directive in the file, there is one for the value of the "deleteTempFileOnExit" preference in Firefox: http://mxr.mozilla.org/mozilla-central/source/browser/app/profile/firefox.js#380 This is done only because on Mac OS X all downloads, including those to be opened with an external application, are moved by default in the user's "Downloads" directory instead of remaining in the user's temporary directory, and so they should look like normal downloads. Probably this should be controlled by a preference with a better name and behavior consistency, I've filed bug 1093054 for that. For the moment, we should change the "deleteTempFileOnExit" preference only in Thunderbird to minimize risk and speed up the process, with bug 1093054 to sort out the best solution for all platforms. There are already some download-related preferences that may be cleaned up there: http://mxr.mozilla.org/comm-central/source/mail/app/profile/all-thunderbird.js#400 To make up for the incorrect information I provided, I've attached a new patch where I've already written a detailed test for the permission modifications. The logic about when to make the file read-only needed a few tweaks, and will not be perfect for non-default preference values until bug 1093054 is fixed, but should be a good approximation for now. Please let me know if this (combined with the preference change in Thunderbird) would solve the issue for you. I've filed bug 1093063 for the proposed change to the location of the test files, as it can have larger implications than what may seem by looking at the apparently trivial change to the patch. If I remember correctly, we used to have a subdirectory, and then moved up one level to resolve intermittent failures.

Comment on attachment 8515980 [details] [diff] [review] Patch with updated changes Review of attachment 8515980 [details] [diff] [review]: ----------------------------------------------------------------- Thx for the help! ::: toolkit/components/downloads/nsDownloadManager.cpp @@ +3338,5 @@ > // open with the appropriate application > + rv = OpenWithApplication(); > + if (NS_SUCCEEDED(rv)) { > + rv = FixTargetPermissions(); > + } I had a mistake here, this should obviously be in the "saveToDisk" case. ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ +210,5 @@ > + } > + > + // Temporary downloads should be read-only and not accessible to other > + // users, while permanently downloaded files should be readable and > + // writable by everyone. The restrictions from the umask should apply. This isn't quite true. Umask doesn't necessarily have stuff readable and writable by everyone. @@ +811,5 @@ > 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. Technically, the umask of the > + // process still applies, but normally it doesn't restrict the current user. I don't think the last sentence is true. We do always use 0600 mode for this. @@ +814,5 @@ > + // accessible by other users on the system. Technically, the umask of the > + // process still applies, but normally it doesn't restrict the current user. > + if (Services.appinfo.OS == "Darwin" || Services.appinfo.OS == "Linux") { > + do_check_eq((yield OS.File.stat(download.target.partFilePath)).unixMode, > + 0o600 & ~OS.Constants.Sys.umask); don't need the & ~OS.Constants.Sys.umask here

Patch with the changes above applied

Comment on attachment 8518147 [details] [diff] [review] bug-1074793.diff Review of attachment 8518147 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Magnus Melin from comment #14) > I had a mistake here, this should obviously be in the "saveToDisk" case. Thanks for finding this case I missed! > > + // On Unix, verify that the file with the partially downloaded data is not > > + // accessible by other users on the system. Technically, the umask of the > > + // process still applies, but normally it doesn't restrict the current user. > > I don't think the last sentence is true. We do always use 0600 mode for this. When the file is created, the operating system automatically applies the umask to the permissions specified in the open call. But the umask is not applied if you explicitly change the permissions on an existing file. Confusing! :-) But it makes little difference, as our test environments don't have umasks that restrict the current user. ::: toolkit/components/jsdownloads/test/unit/common_test_Download.js @@ +213,5 @@ > + // 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, > + (isTemporary ? 0o400 : 0o666 & ~OS.Constants.Sys.umask)); nit: parenthesize like this for clarity (outer parentheses unnecessary): isTemporary ? 0o400 : (0o666 & ~OS.Constants.Sys.umask)

Ok looking try run here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2e270ef66073 https://hg.mozilla.org/integration/mozilla-inbound/rev/ca0a0888311b

Just a heads up: This patch broken downloading APKs (as far as we know) on Android.

Daniel, per comment 6 this isn't a security bug, just a privacy concern that was previously known and applies to limited situations. Probably it can be unflagged in order to help tracking the Android regression in bug 1096014.

Without work on getting the Android regression fixed, I'll likely back this patch out.

Comment on attachment 8518147 [details] [diff] [review] bug-1074793.diff Approval Request Comment [Feature/regressing bug #]: bug 858234 [User impact if declined]: 1) users can easily lose changes they make to documents opened from the web (e.g. changes to word docs). 2) private documents opened is viewable to other users on the system. [Describe test coverage new/current, TBPL]: has xpcshell tests (now) [Risks and why]: I don't think it's high risk [String/UUID change made/needed]: none Bug 1096014 was a regression caused by this and that one-liner needs to be landed together with this.

(In reply to Magnus Melin from comment #22) > Bug 1096014 was a regression caused by this and that one-liner needs to be > landed together with this. Please also nominate that patch for uplift in that case, so we can ensure they both have approval and can be landed at the same time.

Comment on attachment 8518147 [details] [diff] [review] bug-1074793.diff Given that this is a decent size patch that we've shipped since Firefox 24 and that we're at the final 34 beta, I think we can wait to ship this fix until Firefox 35. Please renom if you think I have miss understood the impact of fixing this bug in 34. Beta-

Comment on attachment 8518147 [details] [diff] [review] bug-1074793.diff Approving for Aurora - this doesn't meet ESR criteria, we'd need evidence that the ESR community wants this fix or they can get it by default in 38 ESR