SUMMARY The the privileged status/log file creation performed by the MozillaMaintenance update service on Windows can be abused to create arbitrary files with write access. This can be used by an unprivileged user to obtain SYSTEM privileges. VERSION Firefox Version tested: 61.0.1 stable MozillaMaintenance service version: 61.0.1.6759 Operating System: Windows 10 1803 (10.0.17134.112) x64 VULNERABILITY DETAILS The MozillaMaintenance service creates files in a user-provided update location: - maintenanceservice.exe creates an update.status file when it runs alone (no updater.exe) - (Firefox's) updater.exe creates update.status and update.log These files are actually created as temporary files via GetTempFileName (with 'svc', 'sta' and 'log' prefixes respectively + 3 or 4 hexadecimal digits + '.tmp' extension), then renamed/moved. They are created and renamed/moved with SYSTEM privileges, without impersonation, in a user-provided directory (absolute path that must end with ...\updates\0). This can be abused by an unprivileged user to create files as SYSTEM. Indeed, because the user has control over the directory where these operations take place, an unprivileged user can change it to an NTFS junction that points to an arbitrary directory like C:\Windows\System32, and/or change its content to links targeting other files. A quick illustration of this problem can be obtained with the following PowerShell commands: New-Item -Type Junction -Force -Path C:\Temp\updates\0 -Value C:\Windows\System32 (Get-Service MozillaMaintenance).Start(@("MozillaMaintenance", "software-update", "updater.exe", "C:\Temp\updates\0", "C:\X", "C:\X")) The above commands make the MozillaMaintenance service create an update.status file in C:\Windows\System32, because C:\Temp\updates\0 "points to" (reparses to) that directory. Now, creating files with no control over the content isn't really that interesting, so we have to do it in a way that gives us control over the content and (optionally) the path of the resulting file. One way to do this is by using symbolic links with the name of the manipulated files, e.g. predict/bruteforce the name of the temporary file (or change its content before the rename/move operation), and use a link with the name of the final file to choose the destination path. Unprivileged users can't create "real" NTFS symbolic links on the filesystem by default, but they can create something close enough to work in most instances (including this case), by leveraging a technique found by James Forshaw of Google Project Zero. Indeed, the parent directory (some\path\updates\0 in our case) can be made a junction to the \RPC Control\ object directory, where users can create symbolic link with arbitrary name and target path. The CreateFile call will reparse as needed (one time for the junction and another for the symlink), resulting in the application opening the desired file. So, with the following setup: C:\Temp\updates\0 -> \RPC Control\ (NTFS junction) \RPC Control\update.status -> C:\Windows\System32\evil.dll (object manager symbolic link) A CreateFile operation on C:\Temp\updates\0\update.status will actually open the file C:\Windows\System32\evil.dll By making the temporary file creation in a user-writable location, before moving it to System32, we keep the file's access rights, so its content can then be replaced at will after it has moved to System32 even from our unprivileged user account. We also need to create a symbolic link for the temporary file. The additional benefit of using object manager symlinks is that we don't have to guess or "bruteforce" the name of the temporary file. Indeed, the temporary file name creation (GetTempFileName), will cycle through 0000-FFFF for us when trying to create the name svcXXXX.tmp, as it gets REPARSE results when there is no symlink in \RPC Control\ with that name, because of the way GetTempFileName works. So we just have to create one single link, e.g. svcAAAA.tmp, pointing to a file that does not yet exist in a user-writable location (C:\Temp\updates\moz_svc.tmp in the PoC). Once these links are set up, we just have to call the MozillaMaintenance service with our trap directory to have it create our target file (arbitrary path) with SYSTEM privileges, and then we can replace its content with our evil payload (arbitrary content). There are several ways to escalate privileges given an arbitrary file creation, for this PoC I chose to create the wow64log.dll file in C:\Windows\System32, which on x64 systems is loaded in 32-bits WOW64 processes. Because the MozillaMaintenance executable is 32-bits, we just start the service a second time to have it load the DLL and run our payload as SYSTEM. PROOF OF CONCEPT I've provided a PoC that implement the above step in order to spawn a shell running as SYSTEM from an unprivileged user session. This PoC has the following components: - CreateSymlink.exe: a tool by James Forshaw that implements the pseudo-symlink technique described above. Part of https://github.com/google/symboliclink-testing-tools - wow64log.dll: the DLL that gets written to System32, gets loaded in MM service, and runs our final payload (SpawnShell.exe) with SYSTEM privileges (before the service trims down its privileges) - SpawnShell.exe: the payload, spawns a SYSTEM shell in the user's session - poc.ps1: a PowerShell scripts to automate the attack I've separated the arbitrary file creation and its use for privilege escalation into 2 functions in the script, so that you can reproduce the former without the latter if you'd like to. Usage instructions: 1) Unzip the poc 2) Execute the poc.ps1 PowerShell script by running the command "powershell -exec bypass path\to\poc.ps1" (or right-clicking on poc.ps1 in explorer and choosing "Run with PowerShell") Tested on Windows 10 1703 & 1803 x64

I can reproduce the privilege escalation using the provided PoC. The choice of DLL file does require a 32-bit Maintenance Service to be installed, which is only the case if the most recently updated copy of Firefox (the one which would have last updated the service) is a 32-bit build; 64-bit Firefox builds install a native 64-bit service, and we have migrated most of our users over to those builds. But the substance of the issue at hand is the arbitrary file write capability, not the particular file that's chosen. I'm labeling this sec-moderate because local code execution is a prerequisite, this isn't remotely exploitable on its own, so that seems to be the classifications that our guidelines (https://wiki.mozilla.org/Security_Severity_Ratings) indicate. I'm also requesting review for a bounty payout. Thanks for the report, this is excellent work and excellent documentation.

Geez; can I just pile onto this bug and say fantastic bug and fantastic report!

Thank you very much :) The choice of the wow64log DLL is indeed because the installed service on my x64 test VM is a 32-bits build. I used this technique because I've stumbled upon research describing it while searching for alternative "arbitrary file write to SYSTEM code execution" techniques. There are of course alternatives that would work in a non-WOW64 setup (either x86/ARM Windows OS or full 64-bits Firefox builds), including planting other DLLs with the right arch targeting the MozillaMaintenance service (VERSION, CRYPTSP and bcrypt popped up in ProcMon on my test install IIRC), or just targeting other system services / components. One generic technique that I really like was found by James Forshaw and described here: https://googleprojectzero.blogspot.com/2018/04/windows-exploitation-tricks-exploiting.html Basically, the privileged DiagHub service can be made to load any file from C:\Windows\System32 as a DLL. So, the new attachment is a second PoC that uses this technique and should work on full 64-bits Firefox builds. It drops a payload DLL (FakeDll.dll) in C:\Windows\System32 and then uses the DiagHub service (that also runs as SYSTEM) to load it (FakeDllLoader.exe makes the appropriate COM calls). It is based on a exploit created by James for Project Zero's bug number 1428 and described in his article above. The original code is in poc_sd_full.zip at https://bugs.chromium.org/p/project-zero/issues/detail?id=1428#c9 ; FakeDllLoader.cpp is basically PocStorSvc.cpp without the parts specific to P0 bug no 1428, and with the name of the file to load from System32 passed as parameter to the exe. (I realize there's no need for a second PoC to demonstrate impact, as you said the substance is the arbitrary file write, I just think it's a cool technique.)

This patch adds a check for paths passed to the updater or maintenance service for any symlinks or junctions pointing into the object manager namespace. The updater/service will exit upon detecting such a path and will not attempt to use it. Applying this patch to my copy of the maintenance service blocked both of the PoC's attached here. Robert, I haven't included a test for the new code because I'm not entirely sure how to set up the necessary links and then clean them up again from within xpcshell, but I can try to figure it out if you think it's important enough.

This patch isn't strictly necessary to fix the bug, but it's an additional defensive measure. I've never been comfortable with GetTempFileName and its 16 whole bits of random data, so the manipulation of its output that's part of this exploit made me think getting rid of it might be a good thing to pursue.

Comment on attachment 8999763 [details] [diff] [review] Part 2 - Remove uses of GetTempFileName from updater code >diff --git a/toolkit/mozapps/update/common/updatecommon.cpp b/toolkit/mozapps/update/common/updatecommon.cpp >--- a/toolkit/mozapps/update/common/updatecommon.cpp >+++ b/toolkit/mozapps/update/common/updatecommon.cpp >@@ -68,23 +68,25 @@ void UpdateLog::Init(NS_tchar* sourcePat > sizeof(mDstFilePath)/sizeof(mDstFilePath[0]), > NS_T("%s/%s"), sourcePath, fileName); > // If the destination path was over the length limit, > // disable logging by skipping opening the file and setting logFP. > if ((dstFilePathLen > 0) && > (dstFilePathLen < > static_cast<int>(sizeof(mDstFilePath)/sizeof(mDstFilePath[0])))) { > #ifdef XP_WIN >- if (GetTempFileNameW(sourcePath, L"log", 0, mTmpFilePath) != 0) { >- logFP = NS_tfopen(mTmpFilePath, NS_T("w")); >+ if (!GetUUIDTempFilePath(sourcePath, L"log", mTmpFilePath)) { >+ return; Is there any reason not to continue with the update without logging as was done previously? Bascially: if (GetUUIDTempFilePath(sourcePath, L"log", mTmpFilePath)) { logFP = NS_tfopen(mTmpFilePath, NS_T("w")); DeleteFileW(mDstFilePath); } >+ } > >- // Delete this file now so it is possible to tell from the unelevated >- // updater process if the elevated updater process has written the log. >- DeleteFileW(mDstFilePath); >- } >+ logFP = NS_tfopen(mTmpFilePath, NS_T("w")); >+ >+ // Delete this file now so it is possible to tell from the unelevated >+ // updater process if the elevated updater process has written the log. >+ DeleteFileW(mDstFilePath); >diff --git a/toolkit/mozapps/update/common/updatehelper.cpp b/toolkit/mozapps/update/common/updatehelper.cpp >--- a/toolkit/mozapps/update/common/updatehelper.cpp >+++ b/toolkit/mozapps/update/common/updatehelper.cpp >@@ -257,28 +257,68 @@ PathAppendSafe(LPWSTR base, LPCWSTR extr > if (wcslen(base) + wcslen(extra) >= MAX_PATH) { > return FALSE; > } > > return PathAppendW(base, extra); > } > > /** >+ * Build a temporary file path whose name component is a UUID. >+ * >+ * @param basePath The base directory path for the temp file >+ * @parm prefix Optional prefix for the beginning of the file name nit: s/@parm/@param/ >+ * @param tmpPath Output full path, with the base directory and the file name. >+ * Must already have been allocated with size >= MAX_PATH. >+ * @return TRUE if tmpPath was successfully filled in, FALSE on errors >+ */ >+BOOL >+GetUUIDTempFilePath(LPCWSTR basePath, LPCWSTR prefix, LPWSTR tmpPath) BTW: really nice patches! Only minusing due to the desire to update without logging as was done previously if possible.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #8) > Is there any reason not to continue with the update without logging as was > done previously? Bascially: > if (GetUUIDTempFilePath(sourcePath, L"log", mTmpFilePath)) { > logFP = NS_tfopen(mTmpFilePath, NS_T("w")); > DeleteFileW(mDstFilePath); > } I wasn't trying to change any behavior here; I've switched it back to this and fixed the other typo. > BTW: really nice patches! Only minusing due to the desire to update without > logging as was done previously if possible. Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/710af6f30b99 https://hg.mozilla.org/integration/mozilla-inbound/rev/d3e5f5bcef60

If I may share my thoughts on the proposed fix: Using unpredictable file names is indeed a good defense-in-depth measure whether it is part of the fix for this bug or not. While checking for symlinks is a also good defensive measure, it targets the exploitation method (symlinks) and not directly the root cause (writing files in a user-controlled directory with high privileges). The risk I see is that other exploitation methods could remain that do not use symlinks and/or bypass the checks. One example (theoretical, I have not tried to build the service with these patches to verify this) would be to replace the directory by a symlink after the check but before the temporary file is created, or better after it is created but before it is moved/renamed (so we also know its name even if patch #2 is applied). Winning these races is not always easy, but on NTFS, file OpLocks may help a lot (basically you can lock a thread trying to access a specific file or folder, see SetOpLock in https://github.com/google/symboliclink-testing-tools). From a security standpoint, I would recommend not doing a privileged write to a user-controlled directory in the first place, either by writing to a non-user controlled directory or by impersonating the user before writing the file. (The latter requires knowing which user called the service/updater, which I am not sure is easily/safely doable in all situations, so the first one seems simpler to me.) This would eliminate the need for additional checks by changing a potentially risky operation to a safer one, although it does require more code changes (e.g. to handle the location of the created status/log file). Of course, not knowing the codebase I only have little data to base my suggestions on, I just wanted to point out potential issues in case it's helpful.

https://hg.mozilla.org/mozilla-central/rev/710af6f30b99 https://hg.mozilla.org/mozilla-central/rev/d3e5f5bcef60

Quick followup because I messed up my MakeUnique call: https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3423f706f79e09da412dd8be1ea4a58a5265aa

Is this something you wanted to nominate for backport, Matt?

Yeah, thanks for the reminder, I think I'll nominate this for beta. Anything past that probably doesn't make sense.

This is the part 1 patch with the followup from comment 13 folded in. Approval Request Comment [Feature/Bug causing the regression]: N/A [User impact if declined]: sec-moderate local privilege elevation exploit (moderate only because this is not remotely exploitable on its own). [Is this code covered by automated tests?]: Yes; the function added here does not have a specific test, but there's plenty of coverage for the updater and the maintenance service, so I'm highly confident this patch does not introduce regressions. [Has the fix been verified in Nightly?]: Yes, it's been on Nightly for a couple of days and I've verified it manually. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None, just both patches in this bug. [Is the change risky?]: No [Why is the change risky/not risky?]: Part 1 adds a security check that is almost entirely skipped over during a normal update on all but the rarest Windows configurations, and I've even manually verified that it works in some of those. Part 2 just changes the names of some temp files that aren't critical to the update process. [String changes made/needed]: None

Comment on attachment 9002000 [details] [diff] [review] Part 1 revised Fix for sec issue, let's uplift for beta 19.

The linked patch does not seem to prevent directory junctions, the following PowerShell commands from the original report still create the update.status file in C:\Windows\System32: New-Item -Type Junction -Force -Path C:\Temp\updates\0 -Value C:\Windows\System32 (Get-Service MozillaMaintenance).Start(@("MozillaMaintenance", "software-update", "updater.exe", "C:\Temp\updates\0", "C:\X", "C:\X")) Tested with an install of https://archive.mozilla.org/pub/firefox/nightly/2018/08/2018-08-16-10-00-35-mozilla-central/firefox-63.0a1.en-US.win32.installer.exe on Windows 1703 x64. To back the point I was trying to make in comment #11, please find attached a new PoC that works with this version, and demonstrates the use of OpLock to place object manager symbolic links after the initial checks and the temporary file creation, but before the file is moved. (Note: the PoC expects the standard install path "C:\Program Files (x86)\Firefox Nightly", or the script can be adjusted accordingly.) As for a full fix suggestion, it would be safer IMHO to change the location of the status file (e.g. to C:\Program Files\Mozilla Maintenance Service\status\) when it's generated by the service/updater, because it directly tackles the root cause / risky behaviour (manipulating user-controlable resources from a privileged process) instead of trying to address individual exploitation techniques (junctions/symlinks) which requires checking for all possible bypasses and corner cases.

I forgot to mention that the truly random file name made the exploitation harder, so that's definitely a win.

Since we already landed this fix on m-c and it's an improvement, let's also fix it for 62 and then maybe Matt can file a followup bug.

https://hg.mozilla.org/releases/mozilla-beta/rev/7138f70c03dd https://hg.mozilla.org/releases/mozilla-beta/rev/f87d4e897762 https://hg.mozilla.org/releases/mozilla-beta/rev/fdc1b7954e76

Thanks again, clavoillotte. It took me a little while to understand what you were saying, but I think I get it now; the only complete fix for this class of exploit is putting all our files in locations that are not world-writable. Otherwise there's always some kind of workaround or race that an exploit can win against anything we might do, either through this oplock method or something else that I'm sure could be found. Unfortunately that means I don't think we can fix this, because under our app update architecture, update.status has to be both readable and writable by Firefox, which of course never runs with elevated privileges. The PoC currently relies on (the file that eventually becomes) update.log, but I'm sure that using update.status instead would be possible without too much more difficulty. So we might just have to live with this problem. We do happen to have bug 1458314 in progress, which will probably end up moving the entire update directory to ProgramData, but I don't think that's going to address this issue because it hasn't been looking to change how update.status works. I want to get a second opinion though, because I could have missed something; Robert, do you have any thoughts? (Also everyone else on the CC list please feel free to chime in)

Apologies if my explanations were not clear. I was indeed suggesting to put all files (written to by privileged processes) in locations that are not world-writable to avoid having to check for all possible filesystem attacks (which may be doable, just a lot harder and error-prone). Also worth noting for bug 1458314 that copy/creation of files in a ProgramData subfolder could lead to similar problems if performed by a privileged process and ACLs are not carefully set. Indeed, by default users have "create" access to ProgramData, i.e. they can create files and directories but not modify existing ones which are "owner-locked". Subdirectories will inherit these rights unless an explicit ACL is put on a directory in the chain. So, if a user creates a file or directory before the update service does, that user has full control over its content and can conduct the same kind of attacks. Coming back to the update.status file, I understand it's not feasible to have this file writable by SYSTEM only without changing the whole update architecture. An alternative could be to have that file at a fixed location: - create the ProgramData subdirectory (e.g. C:\ProgramData\Mozilla), preferably at install or first run of the service, and lock its ACL so that only administrators & SYSTEM can write to it (and its subdirectories, via inheritance) - create the update.status file (e.g. C:\ProgramData\Mozilla\update.status), and make only that file R/W (but not deletable) by users - instead of deleting the file when they are finished using it, updaters can just empty it - user processes can copy the file if they need to, the other way around could be dangerous (arbitrary read) - file locking/sharing mechanisms can be used to prevent multiple processes (e.g. multiple users) from writing to this file at the same time This way, users can write to this file, but there's no way (AFAIK) to redirect it to somewhere else or to trick the update service/executable into writing somewhere else, as they don't have much control over it nor its location. This is just a general idea of how the write access requirement could be satisfied, I imagine there are more I don't know about that also need to be taken into account.

Perhaps something along these lines 1. Use a sub-directory of the maintenance service directory with the same structure as the updates directory (<hash>/0/) for all file writes except for the actual update from the updater launched by the maintenance service. 2. Firefox copies the files from this directory to the user writable updates directory when there is an update.status in the maintenance service/<hash>/0/ directory and then launches the maintenance service to clean up this directory. The biggest issue I can think of with this approach is that since there is a single maintenance service it would need to determine whether the version of the application supports this change. It might be possible to have the updater inform the maintenance service that the method is supported.

I think that would work so long as the method the updater uses to inform the service that it wants the new behavior is something that an unprivileged user can't spoof; if it were a command-line argument to the service for instance, the attacker could just leave it off. Maybe we could have PostUpdate write a registry value to the per-install maintenance service key, since those can only be in HKLM?

We might have the maintenance service impersonate the user who's running it when doing anything in user-writable space. I don't know of a way of getting the user who is starting the service, but I'm looking into it.

We could use an impersonation token restricted by the Users group, which only gives the access that would be granted to any member of that group (I'm fairly certain that is the semantics). This would work with bug 1458314's change to put the updates in ProgramData with permissions granted to Users. The updater will need to do the same with any writes to directories it hasn't verified. I think this should work with multiple installations. I'll try to get this together tomorrow, I already have a small scale test working. Please point out anything I'm overlooking.

That would work. I was searching for a way to safely get the user to impersonate, but I didn't think of using a token restricted to the Users group. This would address the root cause (the write is no longer privileged), and it's far easier to implement than changing the update.status path. Cool!

I ran into some issues [1], I think the best way to proceed is to impersonate LocalService, that's what I'll be using for now. This is mostly equivalent to a user account. [1] By restricting to Users the owner can't open a file for appending; even though LocalSystem is the owner, it can't write because only the owner is allowed write access, and the restriction means that LocalSystem can only write to files writeable by all Users! Also LocalSystem isn't itself in Users, so permissions for SYSTEM would have to be explicitly included (bug 1458314 wasn't initially doing this).

Re-resolving this bug in light of bug 1486637 tracking the follow-up work. Matt, did we want this on ESR60 still?

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31) > Matt, did we want this on ESR60 still? Since the patches here didn't end up fully fixing the problem, I'd say there's no reason to do that, unless they end up being a dependency of bug 1486637.

Al, looks like you marked this as fixed after comment 20 (and updated the whiteboard accordingly). This isn't shipping in ESR 60.2, so please update the advisories accordingly.

This needs to get fixed in the corresponding ESR 60 release. Security of the maintenance service is arguably more important in an Enterprise environment where user accounts are more likely to be locked down.

Comment on attachment 9002000 [details] [diff] [review] Part 1 revised [Approval Request Comment] See beta approval request comment 17, it answers most of the questions. If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a sec-moderate because it doesn't allow remote exploitation, but the maintenance service is involved, so it allows privilege escalation to SYSTEM. Fix Landed on Version: 63 nightly with uplift to 62 beta.

Comment on attachment 9002000 [details] [diff] [review] Part 1 revised Approved for ESR 60.2.

https://hg.mozilla.org/releases/mozilla-esr60/rev/a2b17d8a58fa https://hg.mozilla.org/releases/mozilla-esr60/rev/7af1843bbfb5 https://hg.mozilla.org/releases/mozilla-esr60/rev/b7a7d47e309c

I managed to reproduce the wow64log.dll file creation in C:\Windows\System32 using the steps provided in comment 0 and 61.0.1 build1 x86 (20180704003137), on Windows 10 x64. Also, the same STR didn't triggered the wow64log.dll creation anymore, using 60.2.0esr build2 x86 (20180903060751), 63.0b7 build1 x86 (20180917143811) and 62.0 build2 x86 (20180830143136). Are these enough to consider this bug as verified on the previous mentioned builds?

This fix wasn't uplifted to 61, only to ESR 60 and to 62. So yes, those results are exactly as expected.

(In reply to Matt Howell [:mhowell] from comment #39) > This fix wasn't uplifted to 61, only to ESR 60 and to 62. Yes, I am aware the fix wasn't uplifted to 61, that's why I tried (and managed) to reproduce the issue using this build. > So yes, those results are exactly as expected. Thank you for confirming the expected results. Based on this and on comment 38, I will mark the targeted builds as verified fixed.