1742682 - On macOS, umask of application launched after update to new version is 0

Status: VERIFIED FIXED

(Toolkit :: Application Update, defect, P2)

Description

[long]

Steps to reproduce:

Firefox notified me there was a new version available (94.0.2) and I told it to download. Then I went to About Firefox and told it to restart to install the new version.

Actual results:

After the update installed and FF restarted when I go to download files the files have '-rw-rw-rw-' permissions.

Expected results:

Before the update my files downloaded with '-rw-------' permissions as expected. If I fully quit FF after the update and restart it normally it does revert to this behavior as desired.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Toolkit::Application Update' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[[e-mail] (:bhearsum)]

Thank you for the report! It certainly sounds like umask is somehow lost in our update-for-restart in your circumstances.

I tried to reproduce this with:

1. Open terminal, set umask to 077
2. Download file; observe 600 permissions
3. Update & Restart
4. Download file

I still ended up with 600 permissions after the restart, not the 666 that you experienced.

If you could provide some additional details, we might be able to narrow this down. Specifically:

• How/where is your umask set?
• How do you launch Firefox (through the terminal? gui launcher? etc.) normally?
• Do you run Firefox as root, or some other user?

Comment 3

[long]

Hi, I suspect it has to do with setting the "umask for user apps" via https://support.apple.com/en-us/HT201684. I set the umask to 077 via that method. I also have 'umask 077' in both my .bashrc and .bash_profile. I start Firefox by clicking it in my dock normally. I'm running Firefox as myself (non-root).

Comment 4

[[e-mail] (:bhearsum)]

I was actually able to reproduce this on macOS without any custom umask. Simply by:

1. Downloading Firefox 94.0
2. Dragging it to Applications
3. Running it
4. Downloading a file, seeing that it receives 644 permissions
5. Applying and restarting for the update to 94.0.2
6. Downloading another file

...and that other file ends up with 666 permissions.

(My original test was done on Linux, so it can be ignored.)

I'm tentatively marking this as a security bug, as this seems like a potential issue on multi-user systems.

Comment 5

[Daniel Veditz [:dveditz]]

Adding bounty flag per request to our security alias

Comment 6

[[e-mail] (:bhearsum)]

I dug into this a little bit today. I found that we purposely do set the umask to 0 at the start of the update process. It does get set back as part of the UmaskContext destructor, but I suspect that Firefox has already been launched by the time that destructor runs -- which I think means Firefox has inherited the umask of 0 already.

I intend to confirm this by changing the umask we set to 077 instead, but I haven't had time to finish that test yet.

If my theory is correct the obvious thing to do here is reset the umask after we've finished doing all of the update stuff, but before Firefox gets launched.

Kirk, you're the clear expert here so I'm adding you in case you have any obvious insights.

Comment 7

[[e-mail] (:bhearsum)]

(In reply to [e-mail] (:bhearsum) from comment #6)

I intend to confirm this by changing the umask we set to 077 instead, but I haven't had time to finish that test yet.

I finished this test this morning, and it appears to confirm my theory. Instead of the files getting 666 after the first restart, they end up with 600.

Comment 8

[Nick Alexander :nalexander [he/him]]

Thanks for the excellent report, OP! We'll discuss this in our team meeting this week.

Comment 9

[Nick Alexander :nalexander [he/him]]

dveditz: I noticed that you flagged this as sec-bounty?. Can we have a discussion of whether this is even a security vulnerability? I'm fixing this -- it's a bug -- but it's hard for me to envision an exploit here. We change the umask, but what can be done with the resulting Firefox with the incorrect umask? I'm struggling to see how to do anything more without some kind of exploit chain.

Comment 10

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1742682. r?bytesized

This expression uses a UniquePtr to ensure the umask is reset in all
control flow paths, while also allowing it to be reset explicitly after
updating but before launching child process apps.

Comment 11

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1742682 - Post: Add test ensuring callback app is launched with umask from before updating. r?bytesized

This adds a new check-umask command to TestAUSHelper. For
convenience, we invoke the new command with the umask from before the
update but have the command output its current umask (i.e., from when
it is launched). This allows to reuse the existing checkCallbackLog
function, which verifies that the arguments and the callback log
output are the same.

Depends on D133803

Comment 12

[Nick Alexander :nalexander [he/him]]

Notes to self before landing:

• this needs testing on Linux, Windows, and an M1 Mac (just in case)
• commit messages stripped before landing
• test commit pushed to separate ticket for follow-up
  (I assume this will stay as a security ticket.)

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to Nick Alexander :nalexander [he/him] from comment #9)

dveditz: I noticed that you flagged this as sec-bounty?. Can we have a discussion of whether this is even a security vulnerability?

As I noted in the comment, the reporter requested the bounty, which means the bounty team needs to track it and eventually decide yea or nay. If it's not a security vulnerability that decision gets pretty easy.

it's a bug -- but it's hard for me to envision an exploit here. We change the umask, but what can be done with the resulting Firefox with the incorrect umask? I'm struggling to see how to do anything more without some kind of exploit chain.

Am I understanding the bug correctly: the problem is not with the permissions on Firefox itself, but with files the user downloads during the session launched by the updater? And I guess it applies to files we write in the profile? I have a lot of 666 files mixed in with the 644 and 600 ones. In Nightly I'm almost always in a "launched by the updater" session unless I crash.

The profile directory itself has the correct permissions, and it's in the usual place inside my user directory that also has correct permissions. Seems hard for another user on a multi-user system to take advantage of that, though it's not great that the permissions are wrong. Same for files the user downloads to the default (personal) Downloads directory -- not great, but the location itself should keep other users out.

The user might save things to a shared directory on a multi-user machine, though I'm not sure why you'd do that if you didn't mean to share.

Looks like we might write things to the /tmp directory with unsafe permissions because of this. What are the consequences? I currently seem to have both

666 /tmp/MozillaUpdateLock-31210A081F86E80E (from 3 days ago)
644 /tmp/MozillaUpdateLock-4F03746F6B452CFD (from 2 days ago)

Don't see one from today's update. Not sure what the above mean, and why I have more than 1. There'd be zillions if we didn't do cleanup.

This seems somewhere between sec-low and not a security problem (more of a privacy gotcha), although depending on what we write into shared /tmp it could go higher on a multi-user machine.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nalexander, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 15

[Robin Steuber (she/her) [:bytesized]]

Presumably, these patches didn't get merged because Nick has been on vacation. I'll leave Nick's needinfo, just to make sure that this doesn't slip off his radar.

Comment 16

[long]

Hi, I do use /tmp as I don’t want to permanently keep many files I download. That’s why it struck me as a security issue. Definitely less of an issue if not sharing the system

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to [Back in January] Daniel Veditz [:dveditz] from comment #13)

(In reply to Nick Alexander :nalexander [he/him] from comment #9)

dveditz: I noticed that you flagged this as sec-bounty?. Can we have a discussion of whether this is even a security vulnerability?

As I noted in the comment, the reporter requested the bounty, which means the bounty team needs to track it and eventually decide yea or nay. If it's not a security vulnerability that decision gets pretty easy.

it's a bug -- but it's hard for me to envision an exploit here. We change the umask, but what can be done with the resulting Firefox with the incorrect umask? I'm struggling to see how to do anything more without some kind of exploit chain.

Am I understanding the bug correctly: the problem is not with the permissions on Firefox itself, but with files the user downloads during the session launched by the updater? And I guess it applies to files we write in the profile? I have a lot of 666 files mixed in with the 644 and 600 ones. In Nightly I'm almost always in a "launched by the updater" session unless I crash.

The profile directory itself has the correct permissions, and it's in the usual place inside my user directory that also has correct permissions. Seems hard for another user on a multi-user system to take advantage of that, though it's not great that the permissions are wrong. Same for files the user downloads to the default (personal) Downloads directory -- not great, but the location itself should keep other users out.

The user might save things to a shared directory on a multi-user machine, though I'm not sure why you'd do that if you didn't mean to share.

The Unix model is that you need to have access to the entire directory hierarchy to access a file, so yes: we should be saved by the profile location. Otherwise, the running Firefox with umask 0 could just write its passwords DB or something else sensitive and every user could read it: not good.

Looks like we might write things to the /tmp directory with unsafe permissions because of this. What are the consequences? I currently seem to have both

666 /tmp/MozillaUpdateLock-31210A081F86E80E (from 3 days ago)
644 /tmp/MozillaUpdateLock-4F03746F6B452CFD (from 2 days ago)

Don't see one from today's update. Not sure what the above mean, and why I have more than 1. There'd be zillions if we didn't do cleanup.

The suffix is the installation hash; this just means you have multiple Firefox versions installed (and that have been run). I'm not aware of any attacks on these particular update locks, and I don't see how this particular ticket will impact those locks specifically. So I believe that it's only downloaded files that will have the wrong permissions.

This seems somewhere between sec-low and not a security problem (more of a privacy gotcha), although depending on what we write into shared /tmp it could go higher on a multi-user machine.

I was hoping to avoid the secbug landing headache, but I'm okay with jumping through the hoops. We have the process for a reason, after all :) I'll do a little more cross-system testing and get this landed.

Comment 18

[Phabricator Automation]

Comment on attachment 9255369 [details]
Bug 1742682 - Post: Add test ensuring callback app is launched with umask from before updating. r?bytesized

Revision D133804 was moved to bug 1748485. Setting attachment 9255369 [details] to obsolete.

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9255368 [details]
Bug 1742682. r?bytesized

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think almost anybody reading the patch would be able to determine what we're protecting against. I can't think of any particular new capability gained by an attacker once Firefox is running with umask 0, so I don't think the patch will help construct an exploit.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: esr
• If not all supported branches, which bug introduced the flaw?: Bug 1337007
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: I expect the patch to apply without modification and to have no additional risk.
• How likely is this patch to cause regressions; how much testing does it need?: I think it unlikely to cause regressions or to require much testing. I have an automated test that passes on Linux and macOS (Unix-y systems that have a concept of umask) and builds on Windows.

Comment 20

[Tom Ritter [:tjr]]

Comment on attachment 9255368 [details]
Bug 1742682. r?bytesized

sec-low's don't need approval - only sec-high's.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:nalexander, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 23

[Nick Alexander :nalexander [he/him]]

(In reply to Release mgmt bot [:marco/ :calixte] from comment #22)

The patch landed in nightly and beta is affected.
:nalexander, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

It's probably worth an uplift to beta and ESR.

Comment 24

[Nick Alexander :nalexander [he/him]]

Comment on attachment 9255368 [details]
Bug 1742682. r?bytesized

Beta/Release Uplift Approval Request

• User impact if declined: Instances of Firefox (re-)launched by the updater after an update will have umask set to 0 on macOS. Files written by Firefox, such as downloads, may have incorrect permissions.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is very small and we have had no bug reports on Nightly (yet). In addition, an automated test will follow that will give confidence.
• String changes made/needed:

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Honestly, I'm not sure it's worthy of uplift to ESR. But it's a security-related ticket that may turn out to be exploitable and it's a trivial fix, so we might uplift it out of an abundance of caution.
• User impact if declined: Instances of Firefox (re-)launched by the updater after an update will have umask set to 0 on macOS. Files written by Firefox, such as downloads, may have incorrect permissions.
• Fix Landed on Version: 98
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The change is very small and we have had no bug reports on Nightly (yet). In addition, an automated test will follow that will give confidence.

Comment 25

[Donal Meehan [:dmeehan]]

Comment on attachment 9255368 [details]
Bug 1742682. r?bytesized

Approved for 97.0b5. Thanks.

Comment 26

[Donal Meehan [:dmeehan]]

Landed for 97.0b5
https://hg.mozilla.org/releases/mozilla-beta/rev/f2108f905990

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9255368 [details]
Bug 1742682. r?bytesized

Approved for 91.6esr.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/971eb5523eed

Comment 29

[Andrei Purice]

Hey Nick,
I tested this issue following the steps from Comment 4 on the latest version of Nightly. This is a multi-user system (MacOS 11.6 big sur) and it was reproduced from the 96.0a1 version of Nightly to the latest 98.0a1 one (from January 19). The permissions were changed after update going from 644 to 666 (this is given to the same file or even a different file). I used firefox directory site and downloaded one of the builds there.
I will test for beta and esr as soon as the update can be triggered automatically.
Please let me know if the test was done incorrectly and I will re-do them. Following the steps from Comment 4 it would seem that the issue is still present for the latest Nightly.
Attaching 2 screenshots as well with the issue.

Comment 30

[Andrei Purice]

Attachment: 1.jpg

Comment 31

[Andrei Purice]

Attachment: 2.jpg

Comment 32

[Nick Alexander :nalexander [he/him]]

(In reply to Andrei Purice from comment #29)

Hey Nick,
I tested this issue following the steps from Comment 4 on the latest version of Nightly. This is a multi-user system (MacOS 11.6 big sur) and it was reproduced from the 96.0a1 version of Nightly to the latest 98.0a1 one (from January 19). The permissions were changed after update going from 644 to 666 (this is given to the same file or even a different file). I used firefox directory site and downloaded one of the builds there.
I will test for beta and esr as soon as the update can be triggered automatically.
Please let me know if the test was done incorrectly and I will re-do them. Following the steps from Comment 4 it would seem that the issue is still present for the latest Nightly.
Attaching 2 screenshots as well with the issue.

I think this is a misunderstanding. The issue is with updater.exe, so the update needs to be triggered by a sufficiently up-to-date version of Firefox. To test, I fetched:

https://archive.mozilla.org/pub/firefox/nightly/2022/01/2022-01-18-21-55-06-mozilla-central/firefox-98.0a1.en-US.mac.dmg

and copied out of the DMG to ~/Applications/Firefox Nightly.app. about:support yields build ID 20220118215506.

To verify the umask, from within the target Firefox's Browser Console, I ran the following privileged script:

const { Subprocess } = Cu.import("resource://gre/modules/Subprocess.jsm");

let p;
p = await Subprocess.call({ command: "/usr/bin/umask" });

await p.wait();

let stdout;
stdout = await p.stdout.read();
console.log(new TextDecoder().decode(stdout));

Before update, this yielded 0022, as expected. (My system umask is octal 22.)

Then I went to About and manually updated. I verified in about:support that I'm now on build ID 20220119093435. Then I returned to the Browser Console and ran the script again, which again yielded 0022.

If you wanted to verify both sides of this, try it with the Nightly before this patch landed -- which is, I think, https://archive.mozilla.org/pub/firefox/nightly/2022/01/2022-01-11-21-32-55-mozilla-central/. That should fail as you describe.

Comment 33

[Andrei Purice]

Reproduced the initial issue on Firefox 98.0a1 (https://archive.mozilla.org/pub/firefox/nightly/2022/01/2022-01-11-21-32-55-mozilla-central/) on macOS Big Sur 11.6.
Verified as fixed using the latest Nightly 98.0a1(https://archive.mozilla.org/pub/firefox/nightly/2022/01/2022-01-18-21-55-06-mozilla-central/firefox-98.0a1.en-US.mac.dmg) and Firefox 97.0b5 (https://hg.mozilla.org/releases/mozilla-beta/rev/f2108f905990) on macOS big sur 11.6.
Could not verify on esr91.6.0 since the update cannot be triggered yet. I will verify it on esr as soon as the update kicks in.
Nick is there a way to verify this on the esr build since the build doesn't update like the beta and Nightly one?

Comment 34

[Nick Alexander :nalexander [he/him]]

(In reply to Andrei Purice from comment #33)

Reproduced the initial issue on Firefox 98.0a1 (https://archive.mozilla.org/pub/firefox/nightly/2022/01/2022-01-11-21-32-55-mozilla-central/) on macOS Big Sur 11.6.
Verified as fixed using the latest Nightly 98.0a1(https://archive.mozilla.org/pub/firefox/nightly/2022/01/2022-01-18-21-55-06-mozilla-central/firefox-98.0a1.en-US.mac.dmg) and Firefox 97.0b5 (https://hg.mozilla.org/releases/mozilla-beta/rev/f2108f905990) on macOS big sur 11.6.
Could not verify on esr91.6.0 since the update cannot be triggered yet. I will verify it on esr as soon as the update kicks in.
Nick is there a way to verify this on the esr build since the build doesn't update like the beta and Nightly one?

There are -- you could set up an update server -- but it's not worth the effort. If it's good on Nightly it'll be fine on ESR.

Comment 35

[Daniel Veditz [:dveditz]]

Unfortunately minor sec-low bugs do not qualify for our bug bounty program.

Comment 36

[Andrei Purice]

Verified as fixed using the latest ESR build (https://hg.mozilla.org/releases/mozilla-esr91/rev/971eb5523eed) on macOS big sur 11.6.