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

RESOLVED FIXED in Firefox 55

Status

()

--
critical
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: bzbarsky, Assigned: dthayer)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 wontfix, firefox55 fixed)

Details

Attachments

(1 attachment)

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)

Updated

2 years ago
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
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

2 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

2 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)
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

2 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 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

2 years ago
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
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(robert.strong.bugs)
54 RC build is released. Mark 54 won't fix.
status-firefox54: affected → wontfix
You need to log in before you can comment on or make changes to this bug.