Closed
Bug 961080
Opened 10 years ago
Closed 10 years ago
Support group and world-writable umasks for downloaded files on linux and mac
Categories
(Toolkit :: Downloads API, defect)
Tracking
()
VERIFIED
FIXED
mozilla33
People
(Reporter: mmc, Assigned: zwol)
References
Details
(Keywords: regression)
Attachments
(1 file, 5 obsolete files)
1.43 KB,
patch
|
Paolo
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
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.
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•10 years ago
|
||
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).
Assignee | ||
Comment 4•10 years ago
|
||
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.
Flags: needinfo?(benjamin)
Comment 5•10 years ago
|
||
I think you should put it on nsSystemInfo.cpp and store the value in NS_InitXPCOM.
Flags: needinfo?(benjamin)
Assignee | ||
Comment 6•10 years ago
|
||
(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?
Assignee: nobody → zackw
Status: NEW → ASSIGNED
Attachment #8375781 -
Flags: review?(benjamin)
Flags: needinfo?(mmc)
Assignee | ||
Comment 7•10 years ago
|
||
Meanwhile, https://tbpl.mozilla.org/?tree=Try&rev=fe3648f73da4
Reporter | ||
Comment 8•10 years ago
|
||
> 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)
Flags: needinfo?(mmc) → needinfo?(paolo.mozmail)
Comment 9•10 years ago
|
||
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
Attachment #8375781 -
Flags: review?(benjamin) → review+
Comment 10•10 years ago
|
||
(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.
Flags: needinfo?(paolo.mozmail)
Comment 11•10 years ago
|
||
Isn't this a dupe of bug 120679?
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
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.
Attachment #8375781 -
Attachment is obsolete: true
Attachment #8383304 -
Flags: review+
Assignee | ||
Comment 14•10 years ago
|
||
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.
Attachment #8383306 -
Flags: review?(khuey)
Assignee | ||
Comment 15•10 years ago
|
||
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.
Attachment #8383314 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 16•10 years ago
|
||
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.
Attachment #8383318 -
Flags: review?(paolo.mozmail)
Assignee | ||
Comment 17•10 years ago
|
||
Tryserver job: https://tbpl.mozilla.org/?tree=Try&rev=a80294712029
Assignee | ||
Comment 18•10 years ago
|
||
And for the record, I have manually tested that the complete patch stack does what it's supposed to do.
Comment 19•10 years ago
|
||
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).
Flags: needinfo?(dteller)
Updated•10 years ago
|
Attachment #8383314 -
Flags: review?(paolo.mozmail)
Comment 20•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8383318 -
Flags: review?(paolo.mozmail) → feedback+
Assignee | ||
Comment 22•10 years ago
|
||
(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.
Assignee | ||
Comment 23•10 years ago
|
||
(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•10 years ago
|
||
(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.
Attachment #8383306 -
Flags: review?(khuey) → review+
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
Also, I agree that the OS.File part deserves its own bug.
Flags: needinfo?(dteller)
Comment 27•10 years ago
|
||
Ping - Our users are now almost 4 versions behind because I cannot upgrade until this is fixed.
Assignee | ||
Comment 28•10 years ago
|
||
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•10 years ago
|
||
Do you think there's any possibility this could be worked around in an extension?
Assignee | ||
Comment 30•10 years ago
|
||
(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•10 years ago
|
||
> 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.
Assignee | ||
Comment 32•10 years ago
|
||
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.
Attachment #8383304 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8383306 -
Attachment is obsolete: true
Assignee | ||
Comment 33•10 years ago
|
||
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.
Attachment #8383314 -
Attachment is obsolete: true
Comment 34•10 years ago
|
||
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.
Assignee | ||
Comment 35•10 years ago
|
||
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 :-/
Assignee | ||
Comment 36•10 years ago
|
||
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.
Attachment #8383318 -
Attachment is obsolete: true
Attachment #8437644 -
Flags: review?(paolo.mozmail)
Comment 37•10 years ago
|
||
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.
Attachment #8437644 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 38•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c6426f08a828
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 40•10 years ago
|
||
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
Assignee | ||
Comment 41•10 years ago
|
||
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
Attachment #8437644 -
Flags: approval-mozilla-beta?
Attachment #8437644 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
status-firefox31:
--- → affected
status-firefox32:
--- → affected
status-firefox33:
--- → fixed
Keywords: qawanted
Comment 42•10 years ago
|
||
Like 1001849, taking it because the next release is an ESR and it had coverage for a week in m-c + verified.
Updated•10 years ago
|
Attachment #8437644 -
Flags: approval-mozilla-beta?
Attachment #8437644 -
Flags: approval-mozilla-beta+
Attachment #8437644 -
Flags: approval-mozilla-aurora?
Attachment #8437644 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 43•10 years ago
|
||
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•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/f571f0ec2ad3 https://hg.mozilla.org/releases/mozilla-beta/rev/0be5734ba289
Comment 45•10 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Comment 46•10 years ago
|
||
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!
You need to log in
before you can comment on or make changes to this bug.
Description
•