961080 - Support group and world-writable umasks for downloaded files on linux and mac

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

The original code in the old download manager had something like this:

mode_t mask = umask(0777);
umask(mask);
mPermissions = 0666 & ~mask;
nsIFile->SetPermissions(mPermissions)

This is prone to race conditions. The new download manager module doesn't support this. From Paolo in https://bugzilla.mozilla.org/show_bug.cgi?id=919076#c96:

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.

Comment 1

[drichard]

Confirming still not working in beta 8.

Regarding the issue of temporary directories and world readable files, if umask is 000 that means that it's been done by an administrator and they have already accounted for the temporary files.  This won't happen with a home user on a desktop distro.

In our case we use:

export TMPDIR=/tmp/$USER.mozilla

wherein /tmp/$USER.mozilla is set by us to 700.  So even if the files go in there world readable, no one can see them except $USER.   We all agree that security issues should be paramount, but in this case it doesn't have to be over engineered because those that deploy are taking this all into account and designing accordingly.

Comment 2

[Cathy]

I am not a techie but in FF26 on Mac 10.6.8 downloaded files have permissions that are original user read and write and everyone else "no access". I ran the umask command in Terminal and it's set to the Mac default. Files I download in Safari do not have this issue. It doesn't seem to matter what directory the files are downloaded to (desktop or even a shared folder).

Comment 3

[mattpratt.au]

Being bitten by this. I sometimes download files on my main workstation that are to be used on my Windows machine. So I share my download folder (using samba I guess - whatever is the default in Linux Mint under MATE).

This sharing setup must rely on group or world readability however since files downloaded with FF24 stopped working on the windows side with a permission error. I now have to open a terminal and chmod (or navigate & change in the filemanager).

Comment 4

[Zack Weinberg (:zwol)]

So I was going to tackle this - wonky Unix low-level stuff, that's usually right up my alley.

The difficulty is not so much figuring out where to *call* `umask` - XRE_main ought to be early enough, we do plenty of things in there that need to be done before any threads get started - as figuring out where to *put the result* so that chrome JS can get at it.  I thought the appdata object might do, but if it's exposed to JS I don't see how it's done.  Paging bsmedberg.

Comment 5

[Benjamin Smedberg]

I think you should put it on nsSystemInfo.cpp and store the value in NS_InitXPCOM.

Comment 6

[Zack Weinberg (:zwol)]

Attachment: save umask on startup (NOT A COMPLETE FIX)

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> I think you should put it on nsSystemInfo.cpp and store the value in
> NS_InitXPCOM.

Plausible.  Passing the value from NS_InitXPCOM2 to nsSystemInfo::Init is a little ugly, but I think it's better to do it this way than to change when nsSystemInfo::Init gets called.

This is not the fix; this only *enables* the fix.  The actual fix would be to *use* the new nsSystemInfo property in all the places where it's relevant, and I haven't the foggiest idea even how to find them all.  Bug 919076 attachment 8361363 [details] [diff] [review] gives a hint, but I think those places might actually want to change back to 0600, and then chmod() calls should be added in strategic locations.  I am willing to write the code for that but I need someone to tell me where to put it.  Maybe that's you, Monica?

Comment 7

[Zack Weinberg (:zwol)]

Meanwhile, https://tbpl.mozilla.org/?tree=Try&rev=fe3648f73da4

Comment 8

[[:mmc] Monica Chew (no longer reading bugmail)]

> chmod() calls should be added in strategic locations.  I am willing to write
> the code for that but I need someone to tell me where to put it.  Maybe
> that's you, Monica?

Nope, Paolo owns the download code (https://wiki.mozilla.org/Toolkit/Submodules#Download_Manager)

Comment 9

[Benjamin Smedberg]

Comment on attachment 8375781 [details] [diff] [review]
save umask on startup (NOT A COMPLETE FIX)

This is fine but follow the naming conventions: this should be nsSystemInfo::gUserUmask; r=me with that change

Comment 10

[:Paolo Amadini]

(In reply to Zack Weinberg (:zwol) from comment #6)
> This is not the fix; this only *enables* the fix.  The actual fix would be
> to *use* the new nsSystemInfo property in all the places where it's
> relevant, and I haven't the foggiest idea even how to find them all.  Bug
> 919076 attachment 8361363 [details] [diff] [review] gives a hint, but I
> think those places might actually want to change back to 0600, and then
> chmod() calls should be added in strategic locations.

A good place to set the final permissions on Unix systems is the downloadDone method:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadIntegration.jsm#555

As for reverting the permissions in the other places, it is correct, but it is also safer to postpone that until all products migrate to the new Downloads API, to avoid possible regressions on common use cases.

Comment 11

[Cork]

Isn't this a dupe of bug 120679?

Comment 12

[:Paolo Amadini]

(In reply to Cork from comment #11)
> Isn't this a dupe of bug 120679?

This bug is specifically for files that go through the download mechanism, the other bug seems related to nsIFile in general. I don't know if there are any separate permissions issues with nsIFile, but that is another topic anyways.

Comment 13

[Zack Weinberg (:zwol)]

Attachment: patch 1/4 v1a: record umask on startup

Here comes a four-part patch series that (collectively) does fix this specific bug, i.e. if you run a browser with these patches applied and umask set to something other than 0022, downloaded files will be chmod'ed appropriately upon download completion.  It also adds infrastructure to OS.File which should make further cases where browser-created files should respect the umask easier to fix in the future.  I have not made any attempt to find such other places.

This first piece is just the earlier patch with the name change requested by Benjamin.  Carrying r+ forward.

Comment 14

[Zack Weinberg (:zwol)]

Attachment: patch 2/4 v1: reflect the umask into OS.Constants

This patch copies the umask value from the "@mozilla.org/system-info;1" property bag into OS.Constants.Sys, which is more convenient for OS.File.

Comment 15

[Zack Weinberg (:zwol)]

Attachment: patch 3/4 v1: new OS.File methods to set permissions

And this piece adds two new methods to OS.File:

 setPerms([path,] mode) - set the Unix file access permissions to a
    specific numeric mode. Throws an exception if called on Windows.

 makeAccessibleByOthers([path,] exec) - Adjust file access permissions
    to make the file accessible by others, honoring any relevant system
    configuration about that.  If 'exec' is true, make the file executable,
    otherwise don't. On Unix, that's

        chmod(path, (exec ? 0777 : 0666) & ~umask);

    On Windows, currently does nothing.  I suspect there is something useful
    it could do on Windows, but I don't know what it is.

No other behavior is changed.  My vintage 1995 Unix hat says we ought to make OS.File.open() and OS.File.mkdir() default to respecting the umask as well, but that would require an audit of all existing calls (perhaps including those in extensions) that might need adjustment, and therefore would be better done in a separate bug.

Comment 16

[Zack Weinberg (:zwol)]

Attachment: 961080-4-apply-umask-to-downloads.diff

And finally this patch uses OS.File.makeAccessibleByOthers() in downloadDone as suggested in comment 10.

I could not figure out how the download module's tests work, so I did not add one for this.

Comment 17

[Zack Weinberg (:zwol)]

Tryserver job: https://tbpl.mozilla.org/?tree=Try&rev=a80294712029

Comment 18

[Zack Weinberg (:zwol)]

And for the record, I have manually tested that the complete patch stack does what it's supposed to do.

Comment 19

[:Paolo Amadini]

Comment on attachment 8383314 [details] [diff] [review]
patch 3/4 v1: new OS.File methods to set permissions

I think it's better to design this API in another dependent bug in the OS.File component - there might have already been design or there might be work underway that relates to setting Unix permissions. David, what do you think?

(We can move the umask patches there as they are a prerequisite, or land them in yet another bug that is a dependency of the others.)

My take is that this call could be a platform-specific "unixSetPermissions(path, permissions, options)" call, that respects umask by default and has an option like "{ ignoreUmask: true }".

I don't see the need of a cross-platform makeAccessibleByOthers because downloads are the only use case that comes to my mind, and we typically avoid generalizing an API when it has only one consumer (though we might do this in the future if more consumers need to the same thing).

Comment 20

[:Paolo Amadini]

Comment on attachment 8383306 [details] [diff] [review]
patch 2/4 v1: reflect the umask into OS.Constants

David might want to weigh in on this patch too.

Comment 21

[:Paolo Amadini]

Comment on attachment 8383318 [details] [diff] [review]
961080-4-apply-umask-to-downloads.diff

As mentioned, this could look like:

if (OS.File.unixSetPermissions) {
  yield OS.File.unixSetPermissions(aDownload.target.path,
                                   parseInt("666", 8));
}

Marking feedback+ as I think this is the right place for the operation.

Comment 22

[Zack Weinberg (:zwol)]

(In reply to :Paolo Amadini from comment #19)
> I don't see the need of a cross-platform makeAccessibleByOthers because
> downloads are the only use case that comes to my mind, and we typically
> avoid generalizing an API when it has only one consumer (though we might do
> this in the future if more consumers need to the same thing).

I'll defer to the tastes of the people responsible for OS.File, but I do think the download manager would prefer to be able to make an unconditional "do whatever makes sense on this OS" call and not worry about whether it's Unix or Windows or what.

Comment 23

[Zack Weinberg (:zwol)]

(In reply to :Paolo Amadini from comment #21)
> if (OS.File.unixSetPermissions) {
>   yield OS.File.unixSetPermissions(aDownload.target.path,
>                                    parseInt("666", 8));
> }

... yeah, that's exactly the sort of logic that I think belongs consolidated in OS.File rather than every caller having to know about it. :-/

Comment 24

[:Paolo Amadini]

(In reply to Zack Weinberg (:zwol) from comment #22)
> I'll defer to the tastes of the people responsible for OS.File, but I do
> think the download manager would prefer to be able to make an unconditional
> "do whatever makes sense on this OS" call and not worry about whether it's
> Unix or Windows or what.

We do have platform-specific code in the jsdownloads module, even with regard to features that are accessible to C++ code only, so it's fine to have a platform-specific check here.

I'm only suggesting that there aren't other places in the code base that would benefit from this function, and from my experience I'd say that both OS.File peers and super-reviewers are wary of introducing a generalized API without a strong case for it. In case they feel it is useful, of course, we can then use it here.

By the way, thanks a lot for your work here. Checking umask appropriately may seem trivial on the surface, but of course it's not, and I'm glad that we're now doing it in a way that is better that the older hacks.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8383314 [details] [diff] [review]
patch 3/4 v1: new OS.File methods to set permissions

Review of attachment 8383314 [details] [diff] [review]:
-----------------------------------------------------------------

I do not really like this API.
I would largely prefer
 OS.File.setPermissions(path, options)
where options may contain fields
 {number} unixMode
and, in the future
 {object} winPermissions

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Also, I agree that the OS.File part deserves its own bug.

Comment 27

[drichard]

Ping - Our users are now almost 4 versions behind because I cannot upgrade until this is fixed.

Comment 28

[Zack Weinberg (:zwol)]

Regrettably, I can make no further progress on this or any other bug until Debian unstable picks up kernel 3.14; and even after that happens, I have many other things that take priority.

Comment 29

[Mike Kaply [:mkaply]]

Do you think there's any possibility this could be worked around in an extension?

Comment 30

[Zack Weinberg (:zwol)]

(In reply to Mike Kaply (:mkaply) from comment #29)
> Do you think there's any possibility this could be worked around in an
> extension?

If I land the first patch, which I suppose I could just go ahead and do, then the umask value would be visible to privileged JavaScript via 

  let umask = Components.classes["@mozilla.org/system-info;1"].
    getService(Components.interfaces.nsIPropertyBag2).
    getProperty("umask");

and if I land the second patch as well, it would also be available as OS.Constants.Sys.umask.  Those patches are r+ and no one has objected to them, so I'm happy to do that if you think it's worthwhile.  I *am* a little concerned about letting this bug slip into 31esr; it seems likely that enterprise users would care.

Comment 31

[Mike Kaply [:mkaply]]

> I *am* a little concerned about letting this bug slip into 31esr; it seems likely that enterprise users would care.

That's my main concern as well which I why I'm bring it up. We really need something for the esr.

Comment 32

[Zack Weinberg (:zwol)]

Comment on attachment 8383304 [details] [diff] [review]
patch 1/4 v1a: record umask on startup

I have migrated patches 1 and 2 from here to bug 1001842 and landed them there.  I will shortly break patch 3 out to its own bug as requested.

Comment 33

[Zack Weinberg (:zwol)]

Comment on attachment 8383314 [details] [diff] [review]
patch 3/4 v1: new OS.File methods to set permissions

The "new OS.File methods" part is now bug 1001849.

Comment 34

[drichard]

Ping, looks like FF 31 moves to Beta on June 10th and as far as I can tell, this isn't yet working.  We've been on FF 25 for many months and our security guy asks me about it weekly.

Comment 35

[Zack Weinberg (:zwol)]

I am going to try to revise the patches this weekend, but I can't promise anything.  I'm very, very busy on the stuff I'm actually getting paid to do :-/

Comment 36

[Zack Weinberg (:zwol)]

Attachment: patch v2 (dependent on 1001849)

The OS.File API design has concluded and the resultant patch has been pushed to inbound, so here is the (quite small) change to the download manager that finally fixes the bug.

My plan, assuming this can be reviewed and committed today-ish, is to let this bake for a week on -central and then ask for uplift to aurora and beta, so that it does get into ESR31.

Comment 37

[:Paolo Amadini]

Comment on attachment 8437644 [details] [diff] [review]
patch v2 (dependent on 1001849)

Review of attachment 8437644 [details] [diff] [review]:
-----------------------------------------------------------------

Let's land this! I do think the API needs some refinements, but it is already quite close to what we need. I'll go ahead and r+ this considering we'll handle the API review after the fact, in bug 1023402.

Comment 38

[Zack Weinberg (:zwol)]

Landed along with bug 1001849 reland, let's hope it sticks this time:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5714e1eafca
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6426f08a828

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6426f08a828

Comment 40

[drichard]

Nightly Firefox 33 build has fixed umask permissions.  Confirmed it's working!  Thank you!

linux-xmvz:/home/largo/tmp # ls -lad fire*
-rw-rw-rw- 1 drichard drichard 49186919 Jun 25 09:46 firefox-33.0a1.en-US.linux-x86_64.tar.bz2

Comment 41

[Zack Weinberg (:zwol)]

Comment on attachment 8437644 [details] [diff] [review]
patch v2 (dependent on 1001849)

Approval Request Comment
[Feature/regressing bug #]:
Regression in new download manager (bug 858234)

[User impact if declined]:
On POSIXy operating systems, downloaded files will continue to be accessible only to the user who downloaded them, even if user-global configuration (the 'umask') specifies otherwise.  Per discussion in this bug, that is a serious regression in some enterprise installations, where users regularly download
files into shared directories, and expect them to be accessible by others without further action.  One installation refused to upgrade past Firefox 25 because of this regression.

This patch depends on, and is the sole user of, the code added by the patch in bug 1001849; it does not make sense to uplift one but not the other.

Due to the impact on enterprise users, I am requesting uplift to -beta so that  31ESR will not regress relative to 24ESR.  My apologies for cutting it so close.

[Describe test coverage new/current, TBPL]:

This patch has no automated tests (because the test environment doesn't support manipulating the umask, as would be required for a sufficient test) but has been reported to work correctly by one of the affected users.

[Risks and why]: 

Low risk; confirmed to work as intended, restores older behavior, underpinnings are coded very defensively.  The worst thing that could happen is that a downloaded file might still have the wrong access permissions, and the "wrong" permissions are always more restrictive than the "correct" ones.  

[String/UUID change made/needed]: None

Comment 42

[Sylvestre Ledru [:Sylvestre]]

Like 1001849, taking it because the next release is an ESR and it had coverage for a week in m-c + verified.

Comment 43

[Zack Weinberg (:zwol)]

QA: It will be easiest to manually verify this on a Linux desktop installation.  Here's how you do it:

0) Download and unpack a Nightly build somewhere.  Quit all running instances of Firefox.
1) Open a shell window.
2) Enter the command "umask 002".
3) Start your Nightly build *from that shell window*, i.e. enter the command /a/b/c/firefox, replacing /a/b/c with the absolute pathname of the Nightly build you downloaded.
4) Using the instance you just started, download a file.  It doesn't matter what file or where you put it, *except* that the place you put it MUST NOT be a FAT or NTFS file system.
5) Quit Firefox.
6) Back in the shell window, enter the command "ls -l /a/b/c/downloaded-file", again replacing /a/b/c with the absolute pathname of the place you downloaded the file.  The output of this command should be something like

-rw-rw-r-- 1 zack zack 31567 Jun 30 15:59 /a/b/c/downloaded-file

The only part of this that matters is the code at the far left.  If it is not exactly '-rw-rw-r--', the bug is not fixed.

7) Enter the command "umask 027".
8) Repeat steps 3 through 6 with a different file.  The output of the "ls" command this time should be

-rw-r----- 1 zack zack 31567 Jun 30 15:59 /a/b/c/different-file

Again, the only thing that matters is the code at the far left; if it is not exactly '-rw-r-----', the bug is not fixed.

9) If you get here without having been told that the bug was not fixed, congratulations! The bug is fixed.

All the above should also work on an OSX installation, except that you may have to take extra steps to make the "umask" commands affect the test build.  Unfortunately I don't know how to do that.

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f571f0ec2ad3
https://hg.mozilla.org/releases/mozilla-beta/rev/0be5734ba289

Comment 45

[Bogdan Maris, Desktop QA]

Verified as fixed using steps from comment 43 on Ubuntu 13.04 32bit and 64bit using Firefox 31 beta 6, latest Aurora and latest Nightly, with 'umask 027' I get '-rw-r-----' and with 'umask 002' i get '-rw-rw-r--' for the downloaded files.

Comment 46

[drichard]

Confirming that FF 31b6 works in our use case of 'umask 000'.  Downloaded files are correctly being set to '666' and working as expected.  Thank you!