User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release) Build ID: 20130911164256 Steps to reproduce: 1 Browse to a page containing files, say http://packages.ubuntu.com/raring/amd64/mc/download 2 Click (or right-click) on a link and save the file. 3 Check the permissions of the saved file. Please note that if you save the file using drag and drop, instead of the above mentioned procedure, permissions are correctly set. Firefox 23 didn't have this issue. Actual results: The file is given the permissions "-rw-------" (600) Expected results: Permissions should be "-rw-rw-r--" (664)

The same problem occurs using the binaries downloaded from mozilla.org, so it's not a Ubuntu bug.

Do you have another Linux machine to test and confirm?

Unfortunately not, but I have a couple of friends who have the same problem. I'll ask them to add a comment here.

I can reproduce this. The regression range is Last good nightly: 2013-06-04 First bad nightly: 2013-06-05 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7e3a4ebcf067&tochange=8f9ba85eb61c

Maybe bug 858234.

Same problem here: User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:24.0) Gecko/20100101 Firefox/24.0 Build ID: 20130911155223 The file is given the permissions "-rw-------" (600)

Yes, it's caused by the patch from bug 858234. The first bad revision is: changeset: 133930:b6cce1e41253 user: Monica Chew date: Mon May 27 18:33:39 2013 -0700 summary: Move execution from nsExternalAppHandler to nsDownload (b=858234, r=paolo)

I guess it's limited to Linux.

It happens that we retain the permissions of the original ".part" file we create here: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.cpp#534 We could just change that to the default FileUtils.PERMS_FILE (0644) throughout all the download process. If there are good reasons to deny public readability of downloads only while in progress or in the temporary directory, or allow the user's group to read the downloaded file, we can consider using different permissions. I'm not sure if there are any useful platform guidelines there, whichever permissions we end up with with would work better for some users and less for others. Monica, any comment on this?

I think the expected result should be that the downloaded file obeys the user's umask, instead of making defaulting to 0644 or 0600. I'm not sure the best way to go about that.

(In reply to Loic from comment #8) > I guess it's limited to Linux. Probably Mac too?

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #10) > I think the expected result should be that the downloaded file obeys the > user's umask, instead of making defaulting to 0644 or 0600. > > I'm not sure the best way to go about that. Monica are you saying this is a wontfix or invalid bug? It sounds like we're having a discrepancy in behaviour here and should find a way to match drag & drop downloads to download manager's. If you're not sure how to proceed can you point this bug to someone who can weigh in on an approach to consider? This is likely doing the same thing on Mac, flagging QA to confirm.

This bug is not necessarily invalid. However, it doesn't seem urgent to me, either. The downloaded file is still readable and writable by the owner, it just has a possibly more restrictive permissions than the default. Most people won't even notice, hence the 3 month delay in even finding this bug. The "I'm not sure the best way to do that" comment is because I haven't used or come across umask in mozilla-central. It's worth investigating how much work that is before deciding whether to fix this. This should affect all platforms, since BackgroundFileSaver is used by all of them. The discrepancy with the drag and drop is due to https://bugzilla.mozilla.org/show_bug.cgi?id=837195. The bug is that we have multiple codepaths for saving files, not that drag and drop is correct and BackgroundFileSaver isn't.

Ioana, can you have someone on your team test this overnight to see which platforms and versions of Firefox are affected by this bug?

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #14) > Ioana, can you have someone on your team test this overnight to see which > platforms and versions of Firefox are affected by this bug? Sure, Paul is working on this and will update the bug as soon as he's done.

This is reproducible on Linux and Mac, FF 24, 25b2, 26.0a2 (2013-09-23), 27.0a1 (2013-09-23). FF 23.0.1 is not affected.

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #10) > I think the expected result should be that the downloaded file obeys the > user's umask, instead of making defaulting to 0644 or 0600. So, I verified that the operating system enforces the umask of the Firefox process already when we create new files (as expected). I changed the permissions we use on creation to 0666, and on my system with the default umask of 0022 the final file has the permissions 0644. However, the umask can only restrict permissions, so if we create the file with 0644 permissions (which is the default of most files we create in the codebase), the umask cannot be used to upgrade that to 0666 or any other desired value. I don't know if calling SetPermissions on an existing file ignores the umask or not, but we should pay attention to that detail. So, the questions remain: - Which permissions we should use when creating the final file. - Whether we should allow permissions that are less restrictive than the umask (I guess not). - Whether we should use restricted permissions (0600) for files in the system's temporary directory.

Note that the actual permissions are set here, as well as a few other places: http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#1346

In fact, downloads started using "Save Page As" end up with different permissions, we should apply the same logic we determine here to those downloads too.

Given the edge case of someone actually noticing this and finding it odd (it doesn't block the use of the file) we don't need to track this regression but when a fix is available, go ahead and nominate for uplift if low risk.

(In reply to :Paolo Amadini from comment #17) > (In reply to Monica Chew [:mmc] (please use needinfo) from comment #10) > > I think the expected result should be that the downloaded file obeys the > > user's umask, instead of making defaulting to 0644 or 0600. > > So, I verified that the operating system enforces the umask of the Firefox > process already when we create new files (as expected). I changed the > permissions we use on creation to 0666, and on my system with the default > umask of 0022 the final file has the permissions 0644. > > However, the umask can only restrict permissions, so if we create the file > with 0644 permissions (which is the default of most files we create in the > codebase), the umask cannot be used to upgrade that to 0666 or any other > desired value. Can we just chmod it to the right perms? > I don't know if calling SetPermissions on an existing file ignores the umask > or not, but we should pay attention to that detail. > > So, the questions remain: > - Which permissions we should use when creating the final file. Default permissions. > - Whether we should allow permissions that are less restrictive than the > umask (I guess not). Yeah, I agree. > - Whether we should use restricted permissions (0600) for files in the > system's temporary directory. I think this is ideal as well. This bug is caused by the side effect of removing the call FixFilePermissions in nsExternalHelperApp, and removed for good in bug 789888. The right fix is probably doing mode_t mask = umask(0777); umask(mask); mPermissions = 0666 & ~mask; nsIFile->SetPermissions(mPermissions) at the point where the temp file is moved in place.

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #21) > > - Which permissions we should use when creating the final file. > > Default permissions. Like the default PERMS_FILE, 0644? > > - Whether we should use restricted permissions (0600) for files in the > > system's temporary directory. > > I think this is ideal as well. > > The right fix is probably doing > > mode_t mask = umask(0777); > umask(mask); Besides being native code, doesn't this have the potential for race conditions in a multi-threaded process? Honestly, I think we could avoid the performance impact (main-thread I/O) and umask issues of the SetPermissions call by just using default permissions when creating all files, including files in the temporary directory (which, by the way, is something I assume we already do in general in other code). Pending a security sign-off on this, my understanding is that it's not much of an issue to have the file contents readable in the temporary directory, when you can see the file name anyways. Users concerned with that may obtain better results by configuring their system to use a non-publicly-accessible temporary directory (if it's not already so).

> > mode_t mask = umask(0777); > > umask(mask); > > Besides being native code, doesn't this have the potential for race > conditions in a multi-threaded > process? Yes, it's racy and gross -- and also what the old versions of FixFilePermissions did before https://bugzilla.mozilla.org/show_bug.cgi?id=878144. You're right, let's not reintroduce the grossness :) > Pending a security sign-off on this, my understanding is that it's not much > of an issue to have the file contents readable in the temporary directory, > when you can see the file name anyways. Users concerned with that may obtain > better results by configuring their system to use a non-publicly-accessible > temporary directory (if it's not already so). I'm happy to defer to Dan on this. There's a long history of /tmp file security bugs, but you are right that the right solution is per-user storage.

Where do the .part files go? if it's in global /tmp it can't be anything but 0600 because there are privacy and tampering concerns. If the .part file is created in the directory of the final file then it's OK for it to have the final permissions--presumably the user downloaded it to a protected directory if the contents are sensitive.

I just wanted to interject as I noted on #912426 that this issue has had significant impact on our 800 Firefox users. Everyone is on one big server and normal work flow is to download and share files with coworkers and the files are locked. Regular users have no idea how to change file permissions. As was noted in another comment, we prohibit viewing of tmp files with open permissions by creating a unique tmp parent folder for each user that is locked down from everyone except the owner. When the user saves files elsewhere beyond their locked folders, everyone else can view them.

This is a big deal for enterprise.

As noted in comment 16, this affects Mac as well as Linux.

I think we have to fix this, given that some users are still on shared systems. Given that there is no way to inspect the umask without also setting it(!) I think we should take Dan's input as advisory and simply create all files with the user's default permissions. I don't think the additional security benefit of having temp files with more restrictive permissions than the final file outweighs the cost of reintroducing the old FixFilePermissions code, which in itself introduces security bugs and race conditions. Paolo, what do you think?

The .part files appear to be placed in the same directory as the final file, at least on this Windows machine I can test on atm. If that's true on all platforms then there's no risk from making the part file have the same permissions as the final file: by being in the same directory it will have the same visibility.

Thanks, Dan. I am waiting to see if bug 916126 is resolved by rolling back the change in comment 7 and uplifting to beta. If so, then this would be fixed for 25 but still need to be fixed for 26 and later.

There may be a few different ways to proceed here, based on which state we want to reach and how downloads work at present. Currently, on some platforms including Linux, we use the system temporary directory in these cases, for downloads initiated with a single click: - The user has chosen the "open" action instead of "save" from the dialog. - The "open" action is the default for the file type. - For a brief time, while the user selects the action or the "save" destination. For the temporary directory location, we honor $TMPDIR and friends: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/SpecialSystemDirectory.cpp#568 However, at least on my system (Linux Mint 13 with one user), none of these environment variables are set, so we fall back to "/tmp". I assume that setting per-user temporary directories may require additional steps after the installation of the operating system. There are two main concerns with files in "/tmp", one being keeping the file contents private, and the other being preventing tampering with the file before it is copied to the final location or opened. In the interest of doing the simplest thing that works, if we go with 644 permissions for the final file, we could use the same permissions for the temporary file. We are safe from the security perspective and may just require manual configuration of per-user temporary directories to address the privacy concern. If we want to allow 664 or 666 permissions for the final file, so that users can change their umask from the default 022 to 000 to allow world-writeable downloaded files, then I think we have no choice but using 600 in the temporary directory and change the file permissions afterwards. This is how the old Downloads code worked, but it uses main-thread I/O, and we should also implement a way to get the desired umask at startup before it can incur in race conditions with other threads. This may be too late: http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/unix/nsOSHelperAppService.cpp#62 Finally, we could use a different directory for temporary downloads, like the cache directory or something. The downside is that we have more responsibility for clearing the directory after a crash.

What about having a preference setting that is false by default, so the perms 0600 are used by default (as they are now) and when set to true, use whatever the perms are defined by the user's umask? This would allow a user/sysadmin to make the choice based on their own local set up - instead of second guessing what they may or may not require?

(In reply to :Paolo Amadini from comment #35) > In the interest of doing the simplest thing that works, if we go with 644 > permissions for the final file, we could use the same permissions for the > temporary file. We are safe from the security perspective and may just > require manual configuration of per-user temporary directories to address > the privacy concern. This is reasonable, since from comment 17 no files can be created that are more permissive than the default umask. > If we want to allow 664 or 666 permissions for the final file, so that users > can change their umask from the default 022 to 000 to allow world-writeable > downloaded files, Having a umask that allows world-writable files seems like a bug that we don't want to support -- let's not do this and we can avoid (some) performance hits, too. > then I think we have no choice but using 600 in the > temporary directory and change the file permissions afterwards. This is how > the old Downloads code worked, but it uses main-thread I/O, and we should > also implement a way to get the desired umask at startup before it can incur > in race conditions with other threads. This may be too late: > > http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/unix/ > nsOSHelperAppService.cpp#62 I would love to see this code go away, the only thing that class seems to do now is find mime types. > Finally, we could use a different directory for temporary downloads, like > the cache directory or something. The downside is that we have more > responsibility for clearing the directory after a crash. I think this is unnecessarily complicated, difficult to test, and runs into the danger of missing the next merge.

(In reply to James Pearson from comment #36) > What about having a preference setting that is false by default, so the > perms 0600 are used by default (as they are now) and when set to true, use > whatever the perms are defined by the user's umask? > > This would allow a user/sysadmin to make the choice based on their own local > set up - instead of second guessing what they may or may not require? This seem unnecessarily complicated to me, since umask already exists to express this preference. I am also against adding preferences that are hardly used by anyone (enough of those already exist in about:config).

This is fixed in FF 25 and ESR 24 due to a rollback in beta (https://bugzilla.mozilla.org/show_bug.cgi?id=916126#c48). I will fix this but need to prioritize other work. It may miss the merge date but I think it will be a low-risk uplift to Aurora at that point, if necessary (or we can not worry about it, since the primary affected users are on ESR).

Paul, please confirm with tomorrow's ESR24 Nightly and the next Firefox 25 Beta that this is now resolved.

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #40) > Paul, please confirm with tomorrow's ESR24 Nightly and the next Firefox 25 > Beta that this is now resolved. The ESR change stuck and the 25 change didn't, you might want to wait until https://bugzilla.mozilla.org/show_bug.cgi?id=916126 is resolved for 25 (hopefully tomorrow).

Based on comment 41, waiting for bug 916126 to be resolved

Being respectful that merges take a while, this patch seems to have not landed in the latest FF 25 Nightly. In our case, we always use the latest FF release vs using ESR. #916126 reads as if some patches were backed out and being held until 26. If it's not going to land in 25, I'll have to do ESR and wait until 26 to upgrade.

The current release is FF 24, 25 is Beta, 26 Aurora and 27 Nightly. This should be fixed in the next FF 25 beta (25b8 ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/25.0b8-candidates/build1/) which will be available starting tomorrow. Firefox 25 beta will become an official release on 2013-10-28.

Ubuntu 12.04, FF 25b8, FF 2013-10-15-00-05-01-mozilla-esr24: -rw-rw-r-- Mac OS X 10.8.4: FF 25b8: -rw-r--r--@ FF 2013-10-15-00-05-01-mozilla-esr24: -rw-r--r-- So, there are couple of differences. What do you think?

The fact that these files are now readable on download by all users is an improvement. If the expected result is "-rw-rw-r--" and not "-rw-r-r--" then I think we're fixed on Linux and may need a follow-up patch for Mac. What do others think?

confirming wonderful things on b8/Linux. Umask of parent shell is being honored again: linux-xmvz:/home/largo/tmp # ls -lad firefox* -rw-rw-rw- 1 drichard drichard 28989122 Oct 16 16:08 firefox-25.0b8.tar.bz2

Thanks @drichard.

Yes, confirmed fixed in 25b8 on Linux. Note that the expected result will be system dependent (at least on Linux, not sure about Mac). "-rw-r--r--" and "-rw-rw-r--" are both likely to be correct, as it depends what the umask is set to. The important thing is that it's no longer "-rw-------".

Thanks a lot. This thing was driving me crazy.

Issue also with 24.0 on Arch Linux 64bit.

(In reply to bjo from comment #51) > Issue also with 24.0 on Arch Linux 64bit. A fix has not and likely will not land for Firefox 24 as we are just over a week away from releasing Firefox 25 (which contains this fix).

This is (still?) broken in 28.0a1

(In reply to Sebastian Freundt from comment #53) > This is (still?) broken in 28.0a1 Thanks for the reminder. Hopefully we can get this checked in early next week then ask for uplift to Aurora to fix this in 25.

Comment on attachment 8339467 [details] [diff] [review] Minimum set of changes to enable group and world readable files depending on umask ( Looks good. Can you confirm that the permissions on the temporary file used to create a unique name do not matter, because the file is eventually deleted and replaced? See the file creation code here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadPaths.jsm#30

(In reply to :Paolo Amadini from comment #56) > Comment on attachment 8339467 [details] [diff] [review] > Minimum set of changes to enable group and world readable files depending on > umask ( > > Looks good. Can you confirm that the permissions on the temporary file used > to create a unique name do not matter, because the file is eventually > deleted and replaced? Thanks, Paolo. I can't say I fully understand all of the code in DownloadPaths.jsm. However, I did test that when the unique name generation triggers by saving the same file twice, the final target does have correct permissions (obeying umask). I'll check in when the tree is open again.

Comment on attachment 8339467 [details] [diff] [review] Minimum set of changes to enable group and world readable files depending on umask ( Review of attachment 8339467 [details] [diff] [review]: ----------------------------------------------------------------- I know it's late in the cycle, but I think it would make sense to uplift this as far as possible since this is a regression. The regression mainly affects enterprise users. The patch is small and pretty low risk.

Comment on attachment 8339467 [details] [diff] [review] Minimum set of changes to enable group and world readable files depending on umask ( Too late for beta(Firefox 26) as we have already gone to build with our final release builds, approving for aurora at this time. It can land on the corresponding esr at the same time once verified.

https://hg.mozilla.org/releases/mozilla-aurora/rev/3b52b794b1a6 Monica, did you want to nominate this for esr24 given the comment about affecting enterprise users?

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #62) > https://hg.mozilla.org/releases/mozilla-aurora/rev/3b52b794b1a6 > > Monica, did you want to nominate this for esr24 given the comment about > affecting enterprise users? No need, it is already fixed in esr24 because of comment 39. Thanks for asking though.

Just wanted to mention that we are an Enterprise user (800+ users), but do not use ESR. The normal Firefox builds are a better fit for our use case. FF 24 which lacked this patch was a disaster for us in terms of support, and it was welcomed in FF 25 greatly. If 26 is not going to have this patch applied, it means we'll unfortunately have to wait until FF 27 to upgrade. It's just too support intensive, and users have no idea how to change file permissions on their own.

Verified as fixed on Ubuntu 12.04 x64 and Mac OS X 10.8.5 using Firefox 27 beta 1 (buildID: 20131209204824). Permissions for files downloaded are "-rw-r--r--"

Also verified in latest Aurora 28.0a2 (buildID: 2013121100400). Permissions are "-rw-r--r--".

I mean no disrespect but, please, will you stop breaking things that are working? Yesterday I've got the upgrade from Firefox 25 to 26 and this damned bug is back. What the hell does it mean "wontfix"? It was already fixed on Firefox 25.

(In reply to pryzbylewski from comment #67) > I mean no disrespect but, please, will you stop breaking things that are > working? Yesterday I've got the upgrade from Firefox 25 to 26 and this > damned bug is back. What the hell does it mean "wontfix"? It was already > fixed on Firefox 25. The fixes for 25 and 27 were two separate changes. The fix for 25 was a rollback of new code for new features that weren't fully integrated yet. The fix for 27 was a fix of the new code. Requesting uplift to beta to fix in 26, given all the other things that were going on at the time, was not going to happen. I am sorry for the inconvenience.

(In reply to Monica Chew [:mmc] (please use needinfo) from comment #68) > (In reply to pryzbylewski from comment #67) > > I mean no disrespect but, please, will you stop breaking things that are > > working? Yesterday I've got the upgrade from Firefox 25 to 26 and this > > damned bug is back. What the hell does it mean "wontfix"? It was already > > fixed on Firefox 25. > > The fixes for 25 and 27 were two separate changes. The fix for 25 was a > rollback of new code for new features that weren't fully integrated yet. The > fix for 27 was a fix of the new code. Requesting uplift to beta to fix in > 26, given all the other things that were going on at the time, was not going > to happen. I am sorry for the inconvenience. What's aggravating about this is that we tell people we want them to use the regular releases, not the ESR, but situations like this really show off why that is not practical. This is why time based releases are a bad idea. You let broken things go out the door knowing they are broken.

This is broken (again) in 29.0a1.

It is also still broken in v26.0.

Ah. I missed the bit where it is "wontfix" for, well, ever it would seem since it is at best "verified" for the next three releases. How can it be "fixed" if the primary release versions are still broken?

(In reply to Sebastian Freundt from comment #70) > This is broken (again) in 29.0a1. Please file a new bug with steps to reproduce. So far as I know this particular code path hasn't changed since it was fixed for 27, so that may be a different bug.

****. So I must wait another 3 releases (at least) for this to be fixed? I am currently using v26.0.

(In reply to Jim Moe from comment #74) > ****. So I must wait another 3 releases (at least) for this to be fixed? I > am currently using v26.0. No, you must wait ~1 month for Firefox 27 to be released. If you cannot wait until then you are more than welcome to use Firefox Beta (beta.mozilla.org). Unforunately this bug does not meet the criteria to warrant a 26.0.1 release.

I'm sorry but how is this not important enough to warrant a new build. What is the point of file permissions if they are not being used properly. Right now when i download something i have to go and manually set permissions on it so that all my apps will work with it properly...i think it's ridiculous that this is what we are left at. To say use FF-beta is also not a solution i don't want to use a beta version i want to use a stable working version and to me this is a pretty bad flaw since like i said it kinda breaks/interfere with a core function of the OS...

We don't have the resources to push out a new version of Firefox every time a bug is fixed. We have to make trade offs and let some fixes ride the trains, knowing that fix will be pushed live in no more than 6-weeks. In the case of this bug the fix will be in release in a couple weeks. We typically only put out a dot-release for critical stability and security issues. In some cases we'll include functional bugs as "ride along" fixes if we're already planning a dot-release. Usually this requires a high volume of complaints from the user base. Unfortunately this bug meets neither of these criteria. I'm sorry this doesn't resolve the issue for you as soon as you would like but this is just how it works. This will be fixed in Release when we ship Firefox 27 on February 4th.

Just tested Firefox 27 beta 6 64bit Linux, and file permissions are wrong. I just downloaded a .deb package: -rw------- 1 drichard drichard 171184 Jan 15 15:48 /home/largo/tmp/rdesktop_1.8.1-0pmjdebruijn1~precise_i386.deb Umask is set to 000 and working for Firefox 25. 26 had the regression and we were not able to upgrade. Can anyone else confirm it's still broken? This is really bad for us.

Paolo, has something else changed with the file permissions code in the last couple of weeks? Between Bogdan's verification in comment 65 and now things have broken.

Comment on attachment 8360724 [details] [diff] [review] Change Downloads.jsm createNiceUnique to create files with mode 644 ( Review of attachment 8360724 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure if something changed with the download flow, or if the save paths are non-determistic, or my previous verification and bogdan's were just plain wrong, but I was able to reproduce drichard's comment 78 and this seems to fix the problem.

I retested on two different machines with Ubuntu 13.04 x64 and 12.04 x64. Downloaded .deb,.rpm,.tar.gz,.zip files using Firefox 27 beta 6 and latest Nightly 29.0a1, all have -rw r-- r-- permissions. "-rw-r--r-- 1 bogdanmaris bogdanmaris 171184 ian 16 10:31 rdesktop_1.8.1-0pmjdebruijn1~precise_i386.deb"

@Bogdan: I gave a recipe to reproduce in bug 959540 (sample CSV files from IBM).

Comment on attachment 8360724 [details] [diff] [review] Change Downloads.jsm createNiceUnique to create files with mode 644 ( (In reply to Monica Chew [:mmc] (please use needinfo) from comment #81) > I'm not sure if something changed with the download flow, or if the save > paths are non-determistic, or my previous verification and bogdan's were > just plain wrong, but I was able to reproduce drichard's comment 78 and this > seems to fix the problem. There is definitely a difficulty in testing due to the many different code paths here. I think we just need to update this code path as well, as previously suspected.

Not sure if the new patch requires an uplift, if not we may consider moving it to a different bug for better tracking.

Comment on attachment 8360724 [details] [diff] [review] Change Downloads.jsm createNiceUnique to create files with mode 644 ( [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 858234 caused new code in new downloads.jsm to be executed. User impact if declined: We thought this was fixed in 27 as of 12/10 in comment 65 by both QA and dev testing. Evidently this was not the case. Testing completed (on m-c, etc.): Manual testing on m-i and m-c. Risk to taking this patch (and alternatives if risky): Low risk. String or IDL/UUID changes made by this patch: None.

(In reply to :Paolo Amadini from comment #85) > Not sure if the new patch requires an uplift, if not we may consider moving > it to a different bug for better tracking. I requested it because this bug has been open for a long time and we thought it was already fixed last month. https://hg.mozilla.org/integration/mozilla-inbound/rev/041808fb7879

Thank you to everyone that responded so quickly, I'm not a developer, but the line: + curFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, 0644); Seems to indicate to me that files will be built RW-R--R-- and still not honor umask. This is better than 600, but will not meet the needs of a multi-user system where we have requested 666 with umask. I apologize if I'm reading this incorrectly and want to ensure that we can upgrade to FF 27 for our users.

I also want to voice concern that while this will "fix" the reported issue it does not fix the true issue which is that files created to to retain the permissions of the folder they are saved in not just force them to be something. in most cases 644 is probably ok but for others it would not be

> Seems to indicate to me that files will be built RW-R--R-- and still not > honor umask. This is better than 600, but will not meet the needs of a > multi-user system where we have requested 666 with umask. I apologize if > I'm reading this incorrectly and want to ensure that we can upgrade to FF 27 > for our users. Hi drichard, I strongly disagree that we should allow users to create world-writable or group-writable files. Please appeal to the module owner (not me, I don't even own this code) if you feel otherwise. (In reply to Marc Grondin from comment #90) > I also want to voice concern that while this will "fix" the reported issue > it does not fix the true issue which is that files created to to retain the > permissions of the folder they are saved in not just force them to be > something. in most cases 644 is probably ok but for others it would not be Hi Marc, umask will restrict the mode of the created file if umask is more restrictive than 644. Please see Paolo's comment 17. If you are talking about setfacl and friends, I'm not sure how portable these are.

As I have mentioned before, Firefox for us needs to support umask up to and including 666. We have hundreds of users that are downloading files all day to shared folders and collaborating with each other. For files that they wish hidden from view, they place them into private folders that are locked at the parent level. When these files are dragged into public folders, their expectation is that they are sharing the file and that everyone can read and write it. There is no way that regular users can change file permissions manually after downloading based on skill levels. Firefox 24 had this bug and it was a complete disaster for us and incredibly support intensive. We were happy to get to 25 where we it was fixed and working as expected. Without reservation, this bug is a showstopper for us we cannot upgrade unless the functionality is returned. I'm not aware of any other applications that override a system administrator configured umask setting in this manner.

drichard, this workflow sounds incredibly insecure to me. Do users truly expect that by dragging their files into shared public folders, any other user can delete them at will? How do the support costs for world-readable only compare to the support costs for recovering lost files due to a rogue user, or a fat-finger error? Paolo owns the downloads submodule.

Comment on attachment 8361363 [details] [diff] [review] Enable footgun for file permissions by respecting world and group-writable umasks ( Paolo, if you think we should support drichard's use case, please review this patch. This brings us back in line with previous behavior. I asked dveditz on irc, here is his response: <•dveditz> mmc: I don't care. If people want 666 then fine, just don't write files anywhere public <mmc> dveditz, that's the whole point of the enterprise flow they are describing, sharing world-writable files in a public directory 2:53 PM <•dveditz> yeah 2:53 PM <•dveditz> your patch does 644, that last guy wants 666 2:54 PM <•dveditz> arguably 644 is just as dangerous as 666 if you download the wrong stuff to the wrong place and don't have the appropriate umask 2:54 PM <•dveditz> and if you download to safe places then either is just as good

Comment on attachment 8361363 [details] [diff] [review] Enable footgun for file permissions by respecting world and group-writable umasks ( The technical issue with this specific patch is that we should ensure that files that are initially downloaded to the global temporary directory and then moved to the final destination should not be group- or world-writable while they are in the temporary folder. With regard to the general issue of allowing downloading files with 0666 permissions when the process umask allows it, I don't have a strong opinion against it. In most desktop systems the default umask is 0022, so the file will actually get 0644 permissions in the end (and we should ensure not to override the umask). Due to how the Unix system calls to get the umask work, this requires storing the umask somewhere when the process starts and no other threads have been spawned in addition to the main thread. All this work should be defined in a separate bug, not here.

The amount of arrogance towards users displayed by certain people in handling this ticket really baffles me. It's not just this "one guy" wanting world-writable files. Nor are world-writable files a security risk per se. You have no idea what the use cases of your users are but feel obliged to ignore how things have been working in Unix/Linux for decades. If Firefox doesn't honor umask then Firefox is BROKEN, period. In my particular scenario, I need g+w to get the files out of the sandbox that Firefox is running in, so my umask is 0660. There's absolutely nothing insecure about this, but the previously proposed "fix" still breaks my setup.

Changing ESR24 flag to unaffected on this since it didn't ship with the issue. In the other channels - is bug 961080 where further work will be happening? I'm not clear on where the newly nominated patches stand wrt to uplift when this bug is still marked as fixed. Paolo: does comment 96 mean that the patch here should not be uplifted?

or Monica, for the question in comment 98

(In reply to Lukas Blakk [:lsblakk] from comment #99) > or Monica, for the question in comment 98 Hi Lukas, The patch that I requested uplift on (https://bugzilla.mozilla.org/attachment.cgi?id=8360724&action=edit) supports group and world-readable files. This is sufficient for some use cases but not all. I still think it should be uplifted. Sorry for not filing a new bug for that, that was my mistake. After that it still needs to be verified for beta and aurora. I filed bug 961080 to support group and world-writable files, since it is much more complicated to support those. Thanks, Monica

Comment on attachment 8360724 [details] [diff] [review] Change Downloads.jsm createNiceUnique to create files with mode 644 ( Thank you for the clarification, approving this for uplift in that case.

re-setting the status flags so we can re-verify with the new permissions once landed

Also, going to track so that this gets prioritized in verification

https://hg.mozilla.org/releases/mozilla-aurora/rev/b50d191b99c1 https://hg.mozilla.org/releases/mozilla-beta/rev/a8f56ea824c9

I was still not able to reproduce the issue from comment 83 nor comment 78 on three Ubuntu machines (12.04x32, 13.04x64, 13.10x64) so I can`t really verify this fix. Sebastian could you please help verifying that this is fixed using Firefox 27 beta 8 and Latest Aurora?

@Bogdan: freundt@clyde:pts/12:~/temp> umask 022 freundt@clyde:pts/12:~/temp> firefox/firefox -no-remote & [4] 11189 /* downloading http://pic.dhe.ibm.com/infocenter/tivihelp/v41r1/topic/com.ibm.ismsaas.doc/reference/LicenseImportSample.csv */ freundt@clyde:pts/12:~/temp> ll LicenseImportSample.csv -rw-r--r-- 1 freundt users 1661 2014-01-21T15:42:29 LicenseImportSample.csv So yes, it works with 27.0b8. freundt@clyde:pts/12:~/temp> umask 022 freundt@clyde:pts/12:~/temp> firefox -no-remote & [1] 11451 /* d/l'ing http://pic.dhe.ibm.com/infocenter/tivihelp/v41r1/topic/com.ibm.ismsaas.doc/reference/LicenseImportSimpleSample.csv */ freundt@clyde:pts/12:~/temp> ll LicenseImportSimpleSample.csv -rw-r--r-- 1 freundt users 1363 2014-01-21T15:52:57 LicenseImportSimpleSample.csv So yes it works with 29.0a1 too.

@Sebastian: But what about umask 0000?

Monica, can you please confirm that Sebastian's testing in comment 106 makes this verified fixed? If so do any currently open bugs address Ralph's use case?

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #108) > Monica, can you please confirm that Sebastian's testing in comment 106 makes > this verified fixed? Yes. Thanks, Sebastian. > If so do any currently open bugs address Ralph's use > case? Ralph's use case will have to be addressed in bug 961080. I updated the summary to make this more clear.

Thank you Monica.

@Bogdan: Oh you're right, umask 0 doesn't work. freundt@clyde:pts/12:~/temp> umask 0 With 27.0b8: freundt@clyde:pts/12:~/temp> ll LicenseImportSample.csv -rw-r--r-- 1 freundt users 1661 2014-01-22T06:23:11 LicenseImportSample.csv With 29.0a1: freundt@clyde:pts/12:~/temp> ll LicenseImportSimpleSample.csv -rw-r--r-- 1 freundt users 1363 2014-01-22T06:24:59 LicenseImportSimpleSample.csv

This is definitely not fixed in FF27.0 (as shipped by Ubutu at least). All files saved have permissions 600, which is utterly useless for anything but a single user. The FF code respects neither umask nor inherited directory permissions (e.g. g+wrs). I'm mystified by this continued insistence on "security" when the problem is one of basic funtionality. I don't care what happens during the download to the temporary directory; what I care about is that _every_ _single_ _file_ I download I have to go and tweak the permissions so that the file is usable. You do not know what I want to do with the files I download, nor should you care. I should be able to set whatever permissions at both the user (umask) and directory level and FF should say "Yes Sir!" and follow that completely. If you cannot manage this with the current codebase, revert it and restore the code from FF 23 where it Just Worked(TM).

To everyone new that arrives on this issue, please CC yourself on 961080 and vote. We (800 users) are down and unable to upgrade until this is fixed, but I can only cast one vote.

And FF 27 has now been released with no way for Linux and Mac users to share downloaded files with other people and no way for us to upgrade. I'm stuck on FF 25 and two versions behind. This (961080) should absolutely be a blocker.

Is this a joke? Well, it's not funny. I've just installed FF 27.0 and this god damn bug is still there. What the hell does it mean VERIFIED FIXED? I thought fixed meant something like "the bug is gone, forget about it". If you don't give a **** about Linux and Mac users just say it out loud. Take a look at this summary: 23.0 Not affected 24.0 bugged ESR 24.0 fixed 25.0 fixed 26.0 bugged 27.0 bugged This thing looks like a joke to me, still not funny, but laughable.

Still a problem in FF26 on Fedora 19 x86_64. (No FF27 release in Fedora repos yet!

Anyone care to explain in simple terms why six months and four releases after I reported this bug we still have firefox that doesn't obey the user's umask? Maybe I'm dense but I don't get it. Thank you.

FF28.0 on Arch Linux still has this issue for me. Does not obey permissions of the dir/umask. Can't use a shared folder. Tried all sort of funky stuff with ACLs and it's a Firefox issue! That was really surprising, I would have never expected that. Why does Firefox not obey my filesystem set up? Are you planning on actually fixing that (considering the bug is VERIFIED FIXED, even though it clearly is not)?

(In reply to soquel from comment #118) > FF28.0 on Arch Linux still has this issue for me. Please file a new bug to explain your configuration and use case so it may be addressed.