Closed Bug 1337007 Opened 3 years ago Closed 3 years ago

Updater takes umask into account, leading to corrupted installs on multi-user systems

Categories

(Toolkit :: Application Update, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox54 --- wontfix
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: dthayer)

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.
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)
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)
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)
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: nobody → dothayer
Status: NEW → ASSIGNED
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 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+
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)
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)
Thanks Boris! Just reproduced without the fix, and attempted with the fix and it correctly did not take umask into account.
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+
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b055ba326f1f
Clear umask while updating on OSX r=rstrong,spohl
https://hg.mozilla.org/mozilla-central/rev/b055ba326f1f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(robert.strong.bugs)
54 RC build is released. Mark 54 won't fix.
Duplicate of this bug: 1399632
You need to log in before you can comment on or make changes to this bug.