Closed
Bug 1337007
Opened 8 years ago
Closed 8 years ago
Updater takes umask into account, leading to corrupted installs on multi-user systems
Categories
(Toolkit :: Application Update, defect)
Toolkit
Application Update
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: bzbarsky, Assigned: alexical)
References
Details
Attachments
(1 file)
STEPS TO REPRODUCE:
1) Install Firefox in /Applications on Mac
2) Set umask to 007.
3) Update Firefox (by running it and waiting for the update to happen).
EXPECTED RESULTS: Other users on the system can still use (the now-updated) Firefox.
ACTUAL RESULTS: Firefox files are now no longer readable by other users on the system because the user that did the update owns them and all the "other" access bits are off because of the umask.
Comment 1•8 years ago
|
||
Stephen, I haven't had time to look at this yet and wondered if you would be able to? Thanks!
Flags: needinfo?(spohl.mozilla.bugs)
Comment 2•8 years ago
|
||
Boris, could you elaborate on what should happen here? The updater does indeed run as the current user and will therefore take umask into account. The only time we run the updater as anything else than the current user is during an elevated update (bug 394984), which doesn't help us here. There have been calls for a type of updater daemon before, but I'm not aware that this is a priority at the moment (see bug 1349649 comment 4, for example).
Flags: needinfo?(spohl.mozilla.bugs) → needinfo?(bzbarsky)
Reporter | ||
Comment 3•8 years ago
|
||
Well, ideally the updater would create the same exact state as gets created by just dragging the dmg into /Applications during a non-update install. Doing _that_ does not apply the user's umask to all the file permissions inside the dmg. Imo neither should the updater.
The result of the current setup is that I can't actually use the updater, and if it does happen to run it effectively corrupts the Firefox installation so I have to trash it and reinstall from the dmg...
Flags: needinfo?(bzbarsky)
Comment 4•8 years ago
|
||
Ok, so what might work here is for the updater to temporarily suspend the umask and reset it once the update has occurred. It might be as simple as something along the lines of:
> mode_t oldMask = umask(0);
> {run update}
> umask(oldMask);
I'm not sure if setting umask(0) is enough to create files in the same state as the remaining files in the .dmg or if we need to be smarter. Unfortunately, I won't be able to try this anytime soon. Robert, hopefully this is a start.
Flags: needinfo?(robert.strong.bugs)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
Stephen, would you mind taking a look at this patch in case there are any issues with what you and the security team decided upon regarding permissions? Thanks!
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8871956 [details]
Bug 1337007 - Clear umask while updating on OSX
https://reviewboard.mozilla.org/r/143480/#review147242
This looks like a very elegant solution. Have you been able to confirm that this fixes the reported issue? Also, I cannot approve this from a security standpoint. If this is needed here, as rstrong suggest, you will have to get the appropriate approval for that. Thanks!
::: toolkit/mozapps/update/updater/updater.cpp:239
(Diff revision 1)
> + mode_t mPreviousUmask;
> +};
> +
> +UmaskContext::UmaskContext(mode_t umaskToSet)
> +{
> + this->mPreviousUmask = umask(umaskToSet);
No need for |this->|, as it is implied.
::: toolkit/mozapps/update/updater/updater.cpp:244
(Diff revision 1)
> + this->mPreviousUmask = umask(umaskToSet);
> +}
> +
> +UmaskContext::~UmaskContext()
> +{
> + umask(this->mPreviousUmask);
same
Attachment #8871956 -
Flags: review+
Assignee | ||
Comment 8•8 years ago
|
||
Boris, embarrassingly I didn't try to reproduce this before trying to fix it, and I can't seem to reproduce it now, so I can't be confident that I've fixed it. Is there any more detail that you can provide to get this to show up? I've tried with a range of versions and with different users initiating the update, all with umask set to 007, and all files that I can find still have read and execute permissions for other users.
Flags: needinfo?(bzbarsky)
Reporter | ||
Comment 9•8 years ago
|
||
Here are some specific steps to reproduce:
1) Have "umask 007" in your shell dotfile (.profile or whatnot).
2) Ensure that "umask" executed in your shell shows "7" or "0007" or equivalent.
2) Download https://download.mozilla.org/?product=firefox-52.0.1-SSL&os=osx&lang=en-US
3) Open the resulting dmg.
4) Drag the Firefox icon to the Applications icon.
5) At your shell prompt, run:
/Applications/Firefox.app/Contents/MacOS/firefox -profile /tmp/test_profile
6) Go to Firefox > About Firefox
7) Wait for the update to download and get applied.
8) Click the "Restart Firefox to Update" button.
9) Quit Firefox.
10) At your shell prompt, run:
ls -l /Applications/Firefox.app/Contents/MacOS/XUL
At step 10, I see "-rwxrwx---" as the permissions with me as the owner, so other users can no longer read the XUL library and hence can't start the browser.
Flags: needinfo?(bzbarsky)
Assignee | ||
Comment 10•8 years ago
|
||
Thanks Boris! Just reproduced without the fix, and attempted with the fix and it correctly did not take umask into account.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
mozreview-review |
Comment on attachment 8871956 [details]
Bug 1337007 - Clear umask while updating on OSX
https://reviewboard.mozilla.org/r/143480/#review149330
Attachment #8871956 -
Flags: review?(robert.strong.bugs) → review+
Comment 14•8 years ago
|
||
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b055ba326f1f
Clear umask while updating on OSX r=rstrong,spohl
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
Flags: needinfo?(robert.strong.bugs)
Comment 16•7 years ago
|
||
54 RC build is released. Mark 54 won't fix.
You need to log in
before you can comment on or make changes to this bug.
Description
•