1732435 - (CVE-2022-22753) Arbitrary permissions overwrite due to folder locking TOCTOU in Maintenance Service

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect, P1)

Description

[Seb Patane (he/him)]

Attachment: poc.zip

There is a time of check to time of use bug in the way the fix-update-directory-perms functionality of the maintenance service locks folders that allows an attacker to swap out a subfolder with a junction. The files in the junction target will then have 'All Permissions' for the 'Users' group added to them. This effectively allows privilege escalation to SYSTEM, eg. by targeting the Mozilla Maintenance Service installation directory and replacing the maintenanceservice.exe binary with a malicious one.

The TOCTOU is in the commonupdatedir.cpp:Lock() function [0]. There is a brief window between the initial call to GetFileAttributesW() on line 413 to when the directory is locked with the mozlock file on line 435 and then again to when the attributes are updated on line 448.

During this time, the directory referenced in path can be replaced with a directory junction to a temporary directory. When this happens, the mozlock file is created in the temporary directory target, which doesn't lock access to original path. The junction can then be removed and replaced with a normal folder before the second call to GetFileAttributesW() so the class records attributes for a standard folder.

It needs to be left as a normal folder throughout the filesystem link checks in the Reset() function (line 508) and up until after the call to file.PermsOK() in EnsureCorrectPermissions() (line 1486) to avoid the directory being moved out of the way. After this point, it can be replaced with a junction to the actual target directory which the maintenance service will recurse into (line 1528) and update the permissions of all files contained.

The fact the mAttributes property is only updated/checked at lock time means once it's poisoned we don't have to worry about it.

The timing is very tight, but I've confirmed it's possible with a proof of concept (attached). I did have to run Prime95 to generate some load at the same time and I manually changed it's priority to Above Normal - the idea being that the maintenance service runs at Normal priority and the POC runs at High priority so the load should only slow down the maintenance service.

I had success with 0ms delay for stage 1, 181ms delay for stage 2 and 11ms delay for stage 3. The only mandatory parameter the POC takes is the target directory to overwrite the permissions on, but will also take lower/upper bounds for the delays on the three race "stages". The following command line worked on my system after around 20 minutes:

.\FirefoxPoc.exe 'C:\Program Files (x86)\Mozilla Maintenance Service\' 0 100 150 250 0 50

Where 0 100 means stage 1 will start at a delay of 0ms and keep trying up until 100ms, 150 250 is stage 2 starting at 150ms up to 250ms and stage 3 starting at 0ms up to 50ms. If that yields no results after running a few times, you can just run the command line without the timing hints and it will try to work it out itself - that took me a good five or so hours though. Eg.

.\FirefoxPoc.exe 'C:\Program Files (x86)\Mozilla Maintenance Service\'

Please let me know if you need more information

[0] - https://hg.mozilla.org/mozilla-central/file/00858d05a9e2685dac987d90866f201989def64d/toolkit/mozapps/update/common/commonupdatedir.cpp#l412

Comment 1

[Robin Steuber (they/them) [:bytesized]]

Although it sounds pretty difficult to exploit this, this does seem to be a valid privilege escalation bug, so I'm marking it S1. I'm not planning to run the POC code, but the description of the exploit sounds valid to me. I'll see about getting this fixed right after what I'm currently working on.

Comment 2

[Robin Steuber (they/them) [:bytesized]]

Well, I've spent some time thinking about this, and I can't really figure out a way to fix the existing code. So instead I want to remove the Maintenance Service's fix-update-directory-perms functionality altogether.

However, it does exist to solve an important problem. The default permissions on the ProgramData directory grant edit permissions to a file's creator, but not to other users. We make sure to always to create ProgramData/Mozilla such that all containing files will inherit permissions that allow any user to edit them, so that any user can update. But, the directory might already exist since we used it in an old version of Firefox and didn't set the permissions then the way that we do now. This is essentially why the fix-update-directory-perms functionality exists: to fix the permissions on an existing update directory to allow all users to successfully write to that directory.

Thus, I don't really want to remove fix-update-directory-perms without replacing it with something that can address the above problem. So here is what I want to replace this with:

1. At installation time, check the permissions on ProgramData/Mozilla. If members of the Users group cannot write to it, move it out of the way (somewhere like ProgramData/Mozilla-backup-<UUID>) and forget about it.
2. Right now, we explicitly set the permissions when creating ProgramData/Mozilla, but I don't believe that we set them for any of the files/directories within. We just allow them to inherit from the permissions on ProgramData/Mozilla. I want to additionally explicitly set the permissions when creating ProgramData/Mozilla/<install_hash>.

Advantages:

• If step 1 succeeds, the problem should be permanently solved for that machine.
• No need to check for links. If ProgramData/Mozilla is a link in step 1, we just rename the link. No harm done.
• No ability for malicious code to repeatedly invoke the elevated parts of this.
• No recursing and making changes to paths with arbitrary filenames.

Disadvantages:

• If step 1 is invoked, any existing installations will lose their update history and, if they have an update in progress it will be lost.
• If there is a permission problem, the updater will no longer have the ability to fix it. Update will fail and the user will be forced to run the installer to fix things.

I consider these disadvantages to be acceptable, particularly because this problem should be quite rare.

I'm additionally changing the severity of this from S1 to S2 at the request of Release Management.

Comment 3

[Robin Steuber (they/them) [:bytesized]]

Hmm, I just realized a less acceptable drawback of the approach I outlined in Comment 2. It would also cause Firefox installations to lose some update settings. That wouldn't be great. I think that I'm going to have to rethink this. I'll report back when I have a better idea.

Comment 4

[Seb Patane (he/him)]

It really is a tricky problem to solve. I've been mulling it over as well and after playing around with some folder permissions it looks like if you remove all write/modify permissions from the folder it would achieve the original behaviour that Lock() is trying to achieve.

Without those permissions, files/subfolders within can't be created or moved so the state of the folder is effectively "locked" from outside influence. Subfolders can be deleted if the user still holds write/modify permissions for the subfolder, but I don't think this can be used maliciously if it can't be replaced with a link to somewhere else?

Could the flow go like this:

1. Remove all permissions except for SYSTEM from C:\ProgramData\Mozilla
2. For each file in the folder, check if they're links and set permissions appropriately

• The files shouldn't be able to be replaced with a link of any kind since the parent folder restricts new file creation, so link checking would be accurate

3. For each subfolder in the folder, remove all permissions except for SYSTEM, do the link checking and recurse into them

• Similar to the files, once the permissions are removed on the folder/subfolder they shouldn't be able to be replaced

4. Once all files/subfolders in the folder are processed, set the final permissive permissions on the folders themselves

• Since there's no more work to be done within the folder it doesn't matter what users can do in it

I think without any permissions to perform a bait and switch it should prevent the problem without losing any data? Though I'm not sure how permissions inheritance would effect things, including if some subfolders were set to not inherit permissions (which normal Users are able to do).

Comment 5

[Robin Steuber (they/them) [:bytesized]]

I've actually considered that solution, but I am not keen on it. The problem I have is that, in certain cases, I don't think that there is any good way to recover from errors. For example, let's say that we lock down a directory by removing most permissions from it. Then we go to sanitize the directory and discover a hard link inside. We attempt to remove the hard link but, for whatever reason, we fail to remove it. What do we do now? If we stop altogether, we may break update for every user of every installation. If we try to correct the permissions before we bail out, they could be inherited by the hard link, potentially changing the permissions of an arbitrary file in an unacceptable way.

Here is what I think that I want to do instead. This problem basically exists because the update directory may already exist with the wrong permissions. So we are simply going to migrate the update directory to a new location. We'll pick an unusually named location, probably by putting a UUID in it to make it very unlikely that the directory already exists (potentially with the wrong permissions). And we will make very, very sure that we create the directory with the correct permissions in the first place. I believe that this should adequately solve the problem and allow us to remove the permission fixing code altogether.

Comment 6

[Seb Patane (he/him)]

I think the current failure mode is to just ignore any kind of link if they exist, since they shouldn’t under normal operation.

Your plan to get rid of the permissions fixing functionality sounds much better though.

Comment 7

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1732435 - r=nalexander!

Comment 8

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1732435 - r=nalexander!

Depends on D127894

Comment 9

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1732435 - r=nalexander!

Depends on D127895

Comment 10

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1732435 - r=nalexander!

Depends on D127896

Comment 11

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1732435 - r=nalexander!

Depends on D127897

Comment 12

[Seb Patane (he/him)]

I had a quick look at the patch - I think there's an issue in that creating a symbolic link at C:\ProgramData\Mozilla-1de4eec8-1241-4177-a864-e594e8d1fb38\updates\<hash> to an arbitrary directory will result in the arbitrary directory being created with full access for users. The same outcome can also be achieved, but without control over the directory name, by making C:\ProgramData\Mozilla-1de4eec8-1241-4177-a864-e594e8d1fb38\updates a junction to a target directory.

Eg. Using this to create a C:\Program Files\Mozilla Firefox\updated directory and running the maintenance service with the /replace option will end up with C:\Program Files\Mozilla Firefox full of attacker controlled files (as well as full permissions to the Mozilla Firefox directory). I'm fairly certain this could still be used for elevated code execution.

I haven't verified by making a custom build though, so I could be wrong.

Comment 13

[Robin Steuber (they/them) [:bytesized]]

(In reply to Seb Patane (he/him) from comment #12)

I had a quick look at the patch - I think there's an issue in that creating a symbolic link at C:\ProgramData\Mozilla-1de4eec8-1241-4177-a864-e594e8d1fb38\updates\<hash> to an arbitrary directory will result in the arbitrary directory being created with full access for users. The same outcome can also be achieved, but without control over the directory name, by making C:\ProgramData\Mozilla-1de4eec8-1241-4177-a864-e594e8d1fb38\updates a junction to a target directory.

Eg. Using this to create a C:\Program Files\Mozilla Firefox\updated directory and running the maintenance service with the /replace option will end up with C:\Program Files\Mozilla Firefox full of attacker controlled files (as well as full permissions to the Mozilla Firefox directory). I'm fairly certain this could still be used for elevated code execution.

I believe that I have removed every place where this code can be called in an elevated context. Therefore, the code will not be able to create a directory at C:\Program Files\Mozilla Firefox\updated because it will not have the necessary permissions to do so. However, even if it cannot elevate privileges, it seems like it might be possible for malware to jump from one user to another with this mechanism. So I'll take out the permission setting for the \hash directory.

If you see a place where this can happen in an elevated context though, please let me know. I would like to avoid doing that.

Comment 14

[Robin Steuber (they/them) [:bytesized]]

Wait, I just realized that my earlier assertion that this cannot be called from elevated code logically must be incorrect. The elevated updater needs to know where the update directory is, so presumably it calls that function at some point. In any event, I've removed the permission setting of the \<hash> directory. And I think that I'll remove the setting of the \updates directory as well now.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

This isn't something we're going to want to backport once it lands. Dropping it from the Fx94 radar.

Comment 16

[Robin Steuber (they/them) [:bytesized]]

We are right up against the soft code freeze and this patch isn't quite ready yet. Additionally, since this is such a large change, I would be happier with it landing earlier in the cycle so it gets a bit more time on Nightly. So I'm going to plan on this landing in version 96 rather than 95.

I don't usually adjust tracking flags myself. Hopefully I got them right.

Comment 17

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 9244877 [details]
Bug 1732435 - r=nalexander!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it would be pretty difficult. The patch is pretty big, and doesn't directly fix the bug. I think it would be more accurate to say that it removes the attach surface that the POC exploits. Even if someone could discern what the bug was from this patch, it is quite difficult to 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?: all supported releases
• If not all supported branches, which bug introduced the flaw?: Bug 1551913
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: This is quite a large patch. Backporting it would be difficult and risky. Given how hard this bug is to exploit, I would prefer not to backport this.
• How likely is this patch to cause regressions; how much testing does it need?: The patch is complex, but it should be well exercised by automated testing.

Comment 18

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

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

Comment 19

[Robin Steuber (they/them) [:bytesized]]

I've been discussing with the security team how to land this. We've decided that a separate, stripped-down patch for ESR will be necessary. I'll have it up as soon as possible.

Comment 20

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1732435 - ESR patch r=nalexander!

Comment 21

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 9253526 [details]
Bug 1732435 - ESR patch r=nalexander!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is sec-high.
• User impact if declined: The ESR maintenance service will continue to be vulnerable to the vulnerability described in this bug.
• Fix Landed on Version:
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This patch is sort of a compromise. I think that it is too risky to uplift the whole patch stack to ESR, but the security team thinks that it is too risky to leave the bug unpatched. So this patch strips the problematic functionality out of the maintenance service, but does not migrate the update directory to a new location. This means that it is possible for the update directory to have the wrong permissions, breaking update. However, the likelihood of this is quite low, and if it breaks, it will break loudly (the user will be informed that update is not working an prompted to update manually). I think that this will get rid of the security risk of this bug existing while introducing the minimum amount of risk of breaking things.
• String or UUID changes made by this patch: None

Comment 22

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 9253526 [details]
Bug 1732435 - ESR patch r=nalexander!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: I think it would be pretty difficult. The patch doesn't directly fix the bug. I think it would be more accurate to say that it removes the attach surface that the POC exploits. Even if someone could discern what the bug was from this patch, it is quite difficult to 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?: all supported releases
• If not all supported branches, which bug introduced the flaw?: Bug 1551913
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: N/A - This is a backport for the ESR branch.
• How likely is this patch to cause regressions; how much testing does it need?: The patch is complex, but it should be well exercised by automated testing.

Comment 23

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9244882 [details]
Bug 1732435 - r=nalexander!

Approved to land in -central

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing build bustages in toolkit/xre/nsXREDirProvider.cpp:
https://hg.mozilla.org/integration/autoland/rev/5a84459b540a02cfe7c4baba89659b4cfed9f788

After it had landed:
https://hg.mozilla.org/integration/autoland/rev/aaa7386b1c23c011bbb18e999b8497f2b8769e7e
https://hg.mozilla.org/integration/autoland/rev/0c48fcbb4a1a1b58c938daebcb2a737713eb5ad9
https://hg.mozilla.org/integration/autoland/rev/6af39bb5d079bbec7259b5c37dc7d437b483bfa7
https://hg.mozilla.org/integration/autoland/rev/99d43bb37d9e44e87422892d0633ed6da8413dc0
https://hg.mozilla.org/integration/autoland/rev/14947028770eefac1942c4a0e6204b63fa816db7

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=pending%2Crunning%2Ctestfailed%2Cbusted%2Cexception%2Cusercancel&revision=14947028770eefac1942c4a0e6204b63fa816db7
Failure log: https://treeherder.mozilla.org/logviewer?job_id=360462933&repo=autoland

toolkit/xre/MultiInstanceLock.cpp:17:12: fatal error: 'shlwapi.h' file not found

Comment 25

[Robin Steuber (they/them) [:bytesized]]

Gah, whoops. I included that file incorrectly. I'll get a fix up right away.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for causing bustages in nsXREDirProvider.cpp

https://hg.mozilla.org/integration/autoland/rev/740fad344989d083fad880469a0555ac68010d34

Failure log: https://treeherder.mozilla.org/logviewer?job_id=360543532&repo=autoland

toolkit/xre/nsXREDirProvider.cpp:126:20: error: unused function 'GetAppVendor' [-Werror,-Wunused-function] 

Comment 27

[Robin Steuber (they/them) [:bytesized]]

On it.

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/8da34eb6cbfafbea2f99e6fa09c9b4a65af13532
https://hg.mozilla.org/mozilla-central/rev/8da34eb6cbfa

r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/e033669757329332fdb736eb924026434a460d73
https://hg.mozilla.org/mozilla-central/rev/e03366975732

r=nalexander
https://hg.mozilla.org/integration/autoland/rev/e1442bbd4a7efa48c5caca6a788ea88afd2a2eef
https://hg.mozilla.org/mozilla-central/rev/e1442bbd4a7e

r=nalexander
https://hg.mozilla.org/integration/autoland/rev/8f6cccd15bdd345b948a5f2ba28becdae159c56a
https://hg.mozilla.org/mozilla-central/rev/8f6cccd15bdd

r=nalexander,application-update-reviewers
https://hg.mozilla.org/integration/autoland/rev/91496a50d143bd4fded519e9e1b11752c35c350c
https://hg.mozilla.org/mozilla-central/rev/91496a50d143

Comment 29

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

The patch landed in nightly and beta is affected.
:bytesized, 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 30

[Robin Steuber (they/them) [:bytesized]]

I'm setting beta to wontfix. This bug was introduced many versions back, it's quite hard to exploit, and the patch is fairly complex. It doesn't seem like a good candidate for uplift.

Comment 31

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

As part of a security bug pattern analysis, we are requesting your help with a high level analysis of this bug. It is our hope to develop static analysis (or potentially runtime/dynamic analysis) in the future to identify classes of bugs.

Please visit this google form to reply.

Comment 32

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Comment on attachment 9253526 [details]
Bug 1732435 - ESR patch r=nalexander!

I'm going to clear sec-approval on this until we're ready to land it to stop bugzilla emails

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9253526 [details]
Bug 1732435 - ESR patch r=nalexander!

We're halfway through the Beta97 cycle with no known issues with the full patch stack, so I'm feeling confident that it's going to ship. Approving the ESR-specific patch for 91.6esr.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/e33e4da7a596

Comment 35

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Attachment: advisory.txt