Closed Bug 124307 Opened 18 years ago Closed 16 years ago

Awful permissions for file saving as of recently

Categories

(Core Graveyard :: File Handling, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7beta

People

(Reporter: jmd, Assigned: Biesinger)

References

(Blocks 1 open bug, )

Details

(Keywords: helpwanted)

Attachments

(5 files, 1 obsolete file)

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: nsbeta1nsbeta1-
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?
*** 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!
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 patchSplinter Review
the patch that I checked in.
fixed.
Status: ASSIGNED → RESOLVED
Closed: 16 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.