Closed Bug 124307 Opened 23 years ago Closed 21 years ago

Awful permissions for file saving as of recently

Categories

(Core Graveyard :: File Handling, defect)

Product:
Core Graveyard
Component:
File Handling
Type:
defect
Priority:
Not set
Severity:
major

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla1.7beta

People

(Reporter: jmd, Assigned: Biesinger)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: helpwanted)

Attachments

(5 files, 1 obsolete file)

Still-stupid patch
2.86 KB, patch
Details | Diff | Splinter Review
Enhanced still-stupid patch
2.02 KB, patch
Details | Diff | Splinter Review
alternative solution
17.41 KB, patch
bzbarsky
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
final patch
15.48 KB, patch
Details | Diff | Splinter Review
bustage fix for final patch
1.13 KB, patch
Details | Diff | Splinter Review
Clicking on "xpdf-1.00.tar.gz" at the URL saves the file mode 0600. Selecting 'Save link as' in the context menu gives me proper permissions. Expected: Save the file "0666 & ~umask"
*** Bug 123959 has been marked as a duplicate of this bug. ***
Deryk Lister reports this affects him on Windows XP/NTFS as well, in bug 123959.
OS: Linux → All
Hardware: PC → All
This is because we create the temp file as 0600 and permissions are preserved by file copy/move operations. I'm not sure how to solve this short of 1) Not creating a temp file 2) Creating the temp file 0666 and then changing it to 0600 when we know that we're going to use a helper app.
*** Bug 124814 has been marked as a duplicate of this bug. ***
Just to clarify, temp files _should_ be 0600 because the user does not know they exist. See bug 107088.
From dup bug: Ideally, this should be user-configureable. There are different security setups and needs and some need conflicting permissions. Note dure, if umask will do the trick in all cases, because it influences all files created by Mozilla, while savefiles are different than the profile files. I might want to share the former (limited), but not the latter.
> Note dure, if umask will do the trick in all cases It will absolutely positively not. It's not even present on all platforms where we set permissions.
*** Bug 124814 has been marked as a duplicate of this bug. ***
We should use nothing for Win 9x, umask for Unix/Linux/OS X, and the equivalent of umask for NT/2000/XP and MacOS 8-9. Can I throw a spanner in the works and mention Win 9x with Netware as well...?
VMS? BeOS? OS/2? Pete? Could we just get something on nsIFile to support "change permissions taking umask into account" and then let the various classes handle it?
There is no equivalent to umask for Win32 or Mac Classic. MacOSX is a Unix and therefore theoretically has a umask. BeOS is generally not multiuser, but BeFS supports the same stupid fields that umask was designed for so honoring umask makes sense. OS/2 Probably matches Unix or Win32 (inclusive or ;-?)
I must ask why anyone cares about umask. The user sets it. The user can change it at any time for any process. All the unix open commands do the right thing so why should mozilla care? Mozilla should use a permission mode that's appropriate for the occasion. It might be nice to have a pref for default permissions but there's no way mozilla should even consider umask.
Agree, why bother w/ umask? If you are using ::Create, you need to set the permission bits. If you are using Open through the file transport, you need to set the permission bits. In jslib we use a default permission of 0644 which is the canonical setting for most permission based OS's anyway. I think the fact that you are getting *bad* permissions when clicking on a link and *good* permission bits using the "Save link as" menu, is an example of bad client code not a bad nsIFile iimplemenation. Remember at this stage of the game, we want to limit the amount of new code and really tighten up and scrub what is there, get the interface frozen and start addressing any performance issues. --pete
Pete, tenthumbs, please read this bug from the beginning: 1) We create a temporary file without the user knowing on link click. Since this file is temporary, in a world-readable directory (/tmp) and the user does not know it exists permissions are set to 0600 at Create() time for security reasons. Not doing this would be bad. 2) Once the user has picked a final location for the file we MoveTo() to that location. MoveTo preserves file permissions, naturally. At this point we need to set permissions to something that's not as restrictive as 0600. And _this_ is when we would need umask, since just doing chmod to 0664 or 0644 or something is unacceptable. The difference between "save as" and "link click" that Pete points out is due to the creation of this temp file in the latter case. This is an architectural problem that has already been discussed to death in other bugs and is not likely to see a fix soon, IMO, so let's assume it as a given when addressing this bug.
No, you need to use chmod or fchmod, umask is irrelevant.
How is it irrelevant? Say mozilla does chmod to 0666. My umask is 077. I do _not_ want mozilla creating files with 0666 permissions. Ever. The point is that there is currently no way to make "Create at point foo with permissions 1. Move to point bar. Change permissions to 2." do the same thing as "Create at point bar with permissions 2."
nsLocalFile::SetPermissions ??
I still disagree, this is a matter of consumer logic. MoveTo just wraps ::CopyTo which uses Create, which in that case persists the file permissions. Why is it unacceptable to change the permission bits on a file the user owns before or after the move? You are setting the bits for the 0 size temp file that is being created. Then you say you can't change the permissions when the file's destination is chosen by the user? In addition to this, the temp file is only a create of 0 bits, where is the security risk here? This is all consumer logic IMO and should be addressed at that level. --pete
Indeed it is far safer to move/copy a file with very restrictive permissions and then relax them. Think what could happen if something goes wrong with the transaction. Boris, your comment shows the danger of messing with umask. The permissions are mode & ~umask so setting umask to 0777 means files get 000 permission. Getting this wrong in the code is a security breach and, I think, isn't worth the risk particularly since there is a chmod wrapper.
I agree that this is a matter of consumer logic... I'm not sure how to make the logic better in light of the current nsIFile interface, short of eliminating the temp file completely.... > MoveTo just wraps ::CopyTo which uses Create Nope. MoveTo uses rename(). Only if that fails does it fall back to CopyTo(). rename() does not change permissions. I just tested this. See comments below. > Why is it unacceptable to change the permission bits on a file the user owns > before or after the move? This results in the wrong permissions. See below. > the temp file is only a create of 0 bits The temp file gets all the data streamed to it. Then it's moved to the final destination. It can easily get to be many megabytes and can persist for a while on a computer with a slow connection. tenthumbs, you seem to be missing the point. Let me illustrate. My umask is 077. If I want to save a file it should end up on disk with permissions set to 0600, correct? Now, if we use nsLocalFile::SetPermissions() to set the permissions to 0666 either before or after the move the permissions on the final file are 0666. This is completely unacceptable. I just tried both of these scenarios in my tree and I get the same result in both cases. Please read the last sentence of comment #16 again, carefully. The issue is that _creating_ a file with permissions 0666 will set the permissions on the file to 0666 & !umask. The system calls handle that. Doing SetPermissions(0666) will just set permissions to 0666 period.
OS/2 doesn't have a copy of multiuser permissions so this is not an issue for us. Thanks for thinking of us though.
Boris, I often miss the point but I believe not in this case. The fundamental issue is producing a file on disk with permissions that meet the user's expectations. The theory her so far is that mozilla can use a default set of permissions not set by the user and combine them with the umask to automatically produce a permission set that pleases the user. I say that's impossible, that the umask alone does not contain adequate information. I also say that umask is risky. Besides the security implications it must be used in a precise way at a precise time because mozilla is multi-threaded. As I see it the only proper solution is to allow the user to specify the default permissions. That means prefs, at least one for directories and one for everything else. Then mozilla can use chmod as it should. One can argue about the factory defaults but that's another bug.
I fail to see how creating the files with the permissions Create() would normally create them with could fail to please the user. After all, those are the permissions the user gets when just touch()-ing a file... am I missing something?
Because mozilla is _not_ doing the equivalent of touching a file. If you want that behavior then you should open the temp fiel for reading, open a new file for writing with your default permissions and the read from the old file and write to the new file. That's grossly inefficient, of course. What mozilla _is_ doing is the equivalent of "cp foo bar" and bar has the same permissions as foo. If you want to change the bar permissions you have to use chmod. That's the usual unix behavior and that's what mozilla should do.
Precisely my point. Mozilla _should_ just be touching a file. Instead, because of an architectural flaw it's forced to first create a temp file and then copy it. At the same time, the user expects mozilla to not be doing such idiotic things and to just write the file to the location the user chose to start with, therefore giving it the correct permissions automatically. As it is we have to work for those permissions, because we _do_ create this unnecessary temp file.
Can you workaround the problem (of the umask not being honored) by creating an empty file as you would create a final savefile, read its permissions and set the permissions of the moved file to the same permissions?
That would work.....
> Mozilla _should_ just be touching a file No it shouldn't, it should be creating a file. touch changes file access and modification times. touch is a program that implements utime(). By default, it will create a file if it doesn't exist. If you pass the '-c' flag it won't create the file. --pete
Boris: I agree completely that the current behavior is absurd. If mozilla created just one file, as it should, then umask would be handled automatically. However, that's not likely to change any time soon so, if mozilla is to be a well-behaved unix app, then it need to know how to move/copy files and change permissions correctly. That does not require umask either. Also, note that there is no guarantee that the umask mozilla sees accurately reflects the user's true wishes. Any shared library can change the umask. In fact, umask is a weak symbol in glibc so any library can redefine it.
Future-ing, nominate with the nsbeta1 keyword and change back to --- if you think this is important.
Target Milestone: --- → Future
Yes, this is very annoying - I have to change permissions manually for each file I download. OK, I could hack my local copy, but there are more people with the problem. Lowering severity (not "critical").
Severity: critical → major
Target Milestone: Future → ---
Keywords: nsbeta1
Setting target milestone. We need to decide to minus this or step up to fixing it. I'm not sure how close we were to debating a resolution; may not be hard to fix.
Target Milestone: --- → mozilla1.0
This is definitely a major issue for me under Windows XP (using NTFS), as reported in bug 123959. To provide an example for why it's a problem: I share my download directory over a home LAN, so that my parents can have easy access to any useful free software I download, without redownloading it themselves (with a 64K connection, this is important.) With the current permissions problem, it doesn't allow them to access the file at all. To work around this, I can either: a) Move the file to a temporary directory (eg. the desktop) and back to 'my downloads' again, which resets the permissions; b) Use a download manager or c) Use another browser for downloading. All of the above are a hassle, and obviously option 'c' isn't desirable at all. Whilst I may be in a minority right now, home networks are getting more popular all the time. I set one up for a non-technical friend just a few weeks ago. Therefore, I think it's well worth fixing by 1.0.
I also think the current behavior absurd, and it is causing several other defects, some of them critical. Is there any chance we could fix them all by not using the temporary file to download everything before the user even says "OK"?
> fix them all by not using the temporary file In addition to fixing these permission problems you mention, there are a number of other very severe problems related to the whole temp file mess (some which are major perf problems and/or cause OOM kills), that would be quite risky fixes. This late in the game, removing the usage of a temp file is the only sane way to go. And it's arguably the best permenent solution as well. This needs to get machv and 1.0 +'d.
Keywords: mozilla1.0
As soon as netwerk supports that sort of thing, we could try to suspend the channel till the user makes a decision... Yes, that would fix this bug. Last I checked, most channels (http included) were not suspendable.
Filed bug 129834 to track the removal of temp files in that case and to act as dumping ground for that general discussion.
bz: you can suspend http, but you might still have to eat (and buffer) some ODA events that might already be in the event queue when you call Suspend.
darin: that should be doable. I seem to recall that we can suspend ftp. What about file:? If we can suspend those three, I think it would be safe to try and remove the temp file thing... (though it would be nice to be able to suspend data: and gopher: as well)
suspending a file channel works like suspending a http channel. gopher and data do not implement Suspend. bz: suspending the channel in this manner might be part of the solution for the "saving in the background" bug. gagan once suggested solving that bug by keeping a limited sized buffer in memory (say 100k max) and then let the download proceed in the background up to that limit... suspending just before we reach the limit, so we still have enough space for that last ODA event (max 16k for internal necko channels).
nsbeta1- per Nav triage team
Keywords: nsbeta1 → nsbeta1-
Target Milestone: mozilla1.0 → mozilla1.2
er... uh...
Keywords: helpwanted
Just a further comment: While I am not experiencing problems in Linux, I did start having problems saving files in Windows w/release 0.99 The particular system in question is running Windows 98 SE (fairly recent install) and was the beneficiary of a fresh install of Mozilla 0.99. I noticed that any time I tried to download a file (click, shift-click, right-click+save as) I would get an error message telling me that the destination folder's permissions would not allow me to save the file (after downloading completed). If I then tried again to save the same file, I had about an 80% success rate. (same file, same location, same server, etc....) Soemtimes, nothing I could do would convince the app to copy the temp file to the final location. I un-installed & re-installed 0.99 a couple of times with no noticeable improvements. Finally, I backed it out and installed v 0.98 -- everything works fine with this one. I'm not sure what happened, I've never seen this problem throughout the development of Mozilla. I hope this information proves useful -- let me know if I can try some more test(s) to assist.
*** Bug 134820 has been marked as a duplicate of this bug. ***
This should make people happy, methinks. jmd? tenthumbs? what do you think?
bz, thanks for provoding a fix! From how I read it, it first creates the file with rw-rw-rw-, allowing the umask to remove unwanted permissions. It then saves the actual permissions and sets them to rw-------. After download, it changes them back to the saved permissions and moves it to the final location. My comments: - You shouldn't make it world-writable and most likely not group-writable, not even for a moment (I think that's called a "race-condition"). - If you use 644, for a moment, it will be world-readable, the thing you wanted to prevent in the first place. Not sure, if anybody bothers. - You can easily avoid the same situation later, if you set the final permissions *after* you moved the file.
I generally agree with Ben. Having a world-writeable file for any amount of time is a potential security hole. Without explicit user instructions, no program should ever try to create a file with anything less restrictive than 0644. As I said before relying on umask is risky. Any shared library loaded into the process can alter the umask value. You also can't depend on the file system remaining constant between system calls. Your patch assumes that GetPermissions returns data pertinent to the file you created but GetPermissions (and SetPermissions for that matter) refer to the file by _name_ and there's no guarantee the name and the open file are related any more. Unfortunately, mozilla doesn't allow correct behavior. You could try creating a file, getting its permissions, deleting it, and using the new permissions but that's still risky. BTW, bug 134163 demonstrates that setting permissions isn't always guaranteed to work.
Doh. I forgot to cc myself... :( Good points. Updated (still-hacky) patch coming up. As tenthumbs points out, this still has some issues.... It would be good to get a security person to comment.
Attachment #80138 - Attachment is obsolete: true
So, let me get this straight... you claim mFinalFilePermissions = 0666 & ~(umaskval = umask()); won't work because 9x doesn't have umask. I still think that's the only way to do this cleanly, though. NSPR or one of the libraries really needs umask stuff. If the platform doesn't have umask(), umaskval = 0; The current patch just makes everything even more ugly. I'd rather do it with umask internally, or just wait for all the download crap to be rewritten, even though this bug bites me every day, since I run mozilla as a seperate user. That 0644 should be 0666. OK. How about this. Leave it broken, since by petes comment, it seems the whole fucking app is broken WRT file permissions (grep found nearly 100 occurances of 0644 in the source). Just chmod the file to 0644 when it's copied for now. Then we can open a bug (I think I may have already, ages ago) to get the app to use umask globally. Which would involve the stuff I started this comment with.
What about creating a temporary "download" directory in /tmp with restrictive permissions then create the file with default permission in it?
That's an excellent suggestion... I think that would probably work quite happily...
Sorry to bring bas news, but read bug 69938 (Downloads are stored in /tmp regardless of path selection) before doing any serious work on the idea in comment 51.
Yes, yes. See the recent comments (and patch) in bug 55690. Bug 69938 is about to go bye-bye (and this bug with it, of course).
*** Bug 135424 has been marked as a duplicate of this bug. ***
*** Bug 173940 has been marked as a duplicate of this bug. ***
QA Contact: sairuh → petersen
*** Bug 183692 has been marked as a duplicate of this bug. ***
*** Bug 186764 has been marked as a duplicate of this bug. ***
is this a duplicate of bug 120679?
Blocks: 120679
*** Bug 196199 has been marked as a duplicate of this bug. ***
*** Bug 197240 has been marked as a duplicate of this bug. ***
*** Bug 202742 has been marked as a duplicate of this bug. ***
I don't understand why this has been debated so long. This bug needs to be fixed. All _sane_ unix software should use umask. If umask is unreliable because a library can change it, then save the umask upon Mozilla startup and use the saved value. There is _no_ valid excuse to not use the umask. Also, if the user specifically set the umask to allow world-writable files, and they get burned by it, then that is the user's own fault. It is not Mozilla's job to protect the user from his/her own self. I think files should be written out with 666 & ~umask. Also, to give some perspective on the /tmp file debate -- Opera for example downloads files into the users home directory under ~/.opera/cache4/<tmpfile> with 600 permissions. Once the user picks a location, it immediately moves the (partially downloaded) file to the chosen destination and continues to download the file. Opera also follows the umask -- which causes me to end up using Opera a lot for downloading files. Stop debating this bug and just fix the stupid thing: 1) Save the file in /tmp w/ 600 permissions 2) Once the user has chosen a destination: a) close the temp file b) move it to the new destination, name it the correct name if necessary c) chmod it to (666 & ~umask) d) re-open it for write and continue downloading The sooner you fix this bug, the sooner I can stop listing to my co-worker in the next cubicle complain about how Mozilla/Netscape doesn't download files right. Thanks!
Depends on: 55690
Supporting Paul's opinion from comment #64. I urge everyone interested to vote fot this bug to show support for fixing this.
For everyone interested in a quick "solution" I added an enhanced "still-stupid patch", which I applied on every mozilla version since 1.1. This patch fixes a segmentation fault with an additional check. I would be glad if this bug would be fixed in "the right way" soon.
*** Bug 205787 has been marked as a duplicate of this bug. ***
*** Bug 219311 has been marked as a duplicate of this bug. ***
*** Bug 229239 has been marked as a duplicate of this bug. ***
how about this instead?
Assignee: law → cbiesinger
Status: NEW → ASSIGNED
Comment on attachment 140256 [details] [diff] [review] alternative solution bz, what do you think?
Attachment #140256 - Flags: review?(bz-vacation)
That doesn't help the similar win32 problems, does it?
no, it doesn't. win32 should be able to use a similar patch though, or so I hope. and I believe there's a separate bug on that.
Would the win32 patch really work with the directory acl stuff going on there? And I'm pretty sure the win32 bugs got dupped to here....
well, wouldn't win32 just copy the acl from the directory to the file?
Comment on attachment 140256 [details] [diff] [review] alternative solution If that's doable, sure. I guess we just keep this bug open for that... I'm not sure how safe the changes to use sSrv are; are there circumstances when the HelperAppHandler objects will live past the point when the service manager shuts down and releases its refs? Note that before your change these objects hold a ref to the service and all... I don't really see any code to make sure we properly kill off all pending downloads at shutdown, in fact. >Index: unix/nsOSHelperAppService.cpp >+ mode_t mask = umask(0); >+ umask(mask); >+ mPermissions = 0666 & ~mask; This is ok, I guess, if you use 0777 instead of 0. That way if you race with something you won't have a too-permissive umask set at any time...
Comment on attachment 140256 [details] [diff] [review] alternative solution r=bzbarsky with that umask comment addressed.
Attachment #140256 - Flags: review?(bzbarsky) → review+
Comment on attachment 140256 [details] [diff] [review] alternative solution ok, I will make that change. I have currently no tree to post a patch with that change though... sSrv going away seems to be no issue here, shutting down mozilla doesn't crash.
Attachment #140256 - Flags: superreview?(darin)
Comment on attachment 140256 [details] [diff] [review] alternative solution sr=darin (i second bz's review comments)
Attachment #140256 - Flags: superreview?(darin) → superreview+
Attached patch final patch — — Splinter Review
the patch that I checked in.
fixed.
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
well, for reference... this is the bustage fix patch I checked in after the final patch...
It this really Fix? accrding to the data it was fixed on 2004-03-06 However I'm still getting this problem on my FC2 mozilla version 1.6. Anyone can tell me why?
Target Milestone: mozilla1.2alpha → mozilla1.7beta
*** Bug 264870 has been marked as a duplicate of this bug. ***
Does this fix the permissions of mail attachments? It doesn't seem to in my testing (1.7.3, Fedora 1)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: