Status

()

The Mozilla Toolkit is a set of APIs, built on top of Gecko, which provide advanced services to XUL applications. These services include Profile Management, Chrome Registration, Browsing History, Extension and Theme Management, Application Update Service, and Safe Mode. (More info)

Created attachment 8704556[details]
Exploit source code
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151223140742
Steps to reproduce:
A combination of vulnerabilities in updater.cpp allows to overwrite arbitrary files, including protected files in protected folders (such as c:\windows), allowing privilege escalation (for example, by overwriting one of the many executables that are scheduled to run periodically with SYSTEM privileges).
The main vulnerability is that files extracted by ArchiveReader are not locked for writing and can be overwritten while the update is running.
Since a software-update command can be run in any location by unprivileged users, an attacker can choose to run it from a controlled location and overwrite files extracted by updater.exe. By overwriting files as they are being extracted, the attacker can force updater.exe to overwrite any file. I explain the procedure below (after "Steps to reproduce").
The updater will only accept to overwrite files that are in the update location and its subfolders (the updater rejects any path that is not relative, or that contains ".."). But it doesn't check if the path contains junctions, allowing to circumvent this protection. Unlike symlinks, junctions can be created by unprivileged users.
Using a combination of both these vulnerabilities allow to overwrite any file in any protected location.
This is all theoretical, so I wrote an exploit in order to prove the seriousness of this vulnerability. This has been tested successfully on Windows 7 and Windows 10 (both 64 bits) with Firefox 43.0.2 and 43.0.3 (32 bits).
Steps to reproduce:
* Download the FirefoxExploit.zip project and unzip it.
* If necessary, modify any of the configuration parameters in main.cpp (junctionName, fileToOverwrite, and defaultMarPatchFileToOverwrite). In the next steps I'll assume you're using the default values.
* Compile the project (I used VS2013, any recent version should be alright). You get FirefoxExploit.exe.
* From a privileged account, create a file in c:\windows named exploitpoc.txt and put some text in it (e.g. "Original content"). This is the file that will be overwritten as a proof of concept.
* Download the latest mar file (firefox-43.0.2-43.0.3.partial.mar: http://releases.mozilla.org/pub/firefox/releases/43.0.3/update/win32/en-US/firefox-43.0.2-43.0.3.partial.mar , replace en-US by the correct language).
* Optionally, create a limited user account on the computer you are going to run the exploit on (this is to prove the privilege escalation) and log in with it.
* Place FirefoxExploit.exe and the mar file in a directory. I'll assume it's c:\firefox_pe in the next steps.
* Create a file named exploitpocnew.txt in c:\firefox_pe. Put any content in it (e.g. "Exploit successful"); it will replace c:\windows\exploitpoc.txt after the exploit is successful.
* Open a cmd shell. Go to c:\firefox_pe.
* Create a junction named "exploit" which points to c:\windows : mklink /J exploit c:\windows
* Execute the exploit: FirefoxExploit.exe exploitpocnew.txt
* Open c:\windows\exploitpoc.txt . It now contains "Exploit successful".
Below is an explanation of how the exploit works. For complete details, refer to the source code (mainly main.cpp).
The exploit will first prepare some things:
* Detect the Firefox installation directory, copy updater.exe from it into c:\firefox_pe (so that the maintenance service accepts to run the update from this location).
* Open the mar file, make sure the version is correct (downgrade is refused), make sure it contains the files needed.
* Read the updatev3.manifest and the original patch from the mar file, so that it can detect later when updater.exe has decompressed enough data from them.
* Generate (using bsdiff) the rogue patch file that will overwrite the legitimate patch.
Then, it will start the update by launching the maintenance service with the appropriate parameters. As soon as it is launched, it will continually monitor the c:\firefox_pe\updating directory, where updater.exe puts files extracted from the mar archive.
The first step of the exploit is to continually check for the presence of update.manifest (extracted by updater.exe). As soon as it is here, it is continually read until enough data is written to it. It is then overwritten with our rogue manifest, which contains a single PatchFile action with the file we want to overwrite as a destination.
The second step is to continually check for the presence of 0.patch (this is updater.exe extracting the legitimate patch file from the mar). Again, as soon as enough data is written to it, it is overwritten with our rogue patch. At this point, it is still being written to by updater.exe, so it is repeatedly truncated to the size of our rogue patch.
Then, updater.exe will apply our rogue patch to the file we want to overwrite. The exploit is successful; you can check that c:\windows\exploitpoc.txt contains the content of exploitpocnew.txt ("Exploit successful").
I'll be happy to provide any help needed to run the exploit successfully. I strongly advise to run the exploit on a multi-core machine; the race condition is much more likely to succeed if it can run concurrently to updater.exe since it has busy loops.

Notes:
The last time we had a similar bug it was decided it would be too difficult to determine consistently if we are dealing with junction points.
Ideally the maintenance service will never be used with locations where non-admin users have write access.
If there is a decent way to consistently determine either of these two conditions (there might be other conditions we could check) then that would be a good approach to fixing this bug.

There might be a different direction we could address this one from. This exploit is all about overwriting files while the update is in the process of being extracted from the MAR; if we could prevent those overwrite operations from succeeding, maybe by manipulating the permissions on the temporary files or by holding handles to them until we're done with them, then the actual location where all this is happening wouldn't matter, right?

Created attachment 8736489[details][diff][review]
Patch
This patch works by making the directory the MAR is unpacked into inaccessible to non-admin users when the maintenance service is being used, so that the exploit code is unable to perform its "in-flight" edits to files in there. This seems to render the exploit ineffective (it hangs forever on my machines).
I've left the more general issue of what to do about junctions/links for another day (again). When I looked into that angle, it wasn't clear how to differentiate the malicious kind of junction point from the useful kind. I also couldn't find a way to tell junctions apart from volume mount points from within the Windows API, and I'm not in a hurry to risk problems for configurations using those.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #9)
> I have to say that setting restrictive permissions to try to fix these types
> of issues always scares me since it could possibly lock out part of the
> process.
That's definitely a concern. I thought that limiting this to the service would prevent locking anything out, because the rest of the process would also be under the service and would have the same permissions.
The other ideas I had for solutions from a similar angle were to either apply the permissions on each individual file that we extract/patch instead of the whole directory, or to leave the permissions alone but hold open a non-sharing handle on each file until we're finished with it. I didn't attempt either of those because they both would require major surgery on the ArchiveReader and probably on libmar, but I still think they're both plausible.

Created attachment 8736961[details][diff][review]
Patch attempt 3
Thought I might try actually creating the directory before working with it, we'll see how that goes. Also corrected the error logging.
Side note: I am considerably weirded out by the fact that the first two patches, which just did not work, could still consistently pass local test runs. I won't be too surprised if this one also fails on try.

I killed your pending Win7 and WinXP xpcshell tests on that latest try push, because the four runs on Win8 left us with four dead machines from having files they were unable to remove in subsequent test runs.

Created attachment 8738209[details][diff][review]
Patch attempt 4
All right, I'm trying this one last time, adding a class with a destructor to make sure that the new directory the patch creates always gets deleted. I'll keep an eye on the try push and if it doesn't go well I'll probably abandon this strategy entirely; if it's that hard to get right, it's just not a good idea.

Created attachment 8740136[details][diff][review]
New Patch
Completely different approach from the previous patch, and also different from the other ideas I had before. This patch works by calling LockFile on each bsdiff patch after the file is created but before any data is extracted into it from the MAR. This ensures that only the updater process is able to read or write the patch file until after the locking handle is closed.
For this fix to be effective, the file stream has to be kept open between the extraction (or prepare) and applying (or execute) phases, so that the lock is never released across both phases; most of the code changes in this patch are just implementing that change.

Created attachment 8740455[details][diff][review]
Amended patch
I made all the changes, except for these two comments:
> The msdn page only states what is filesystems are supported on
> Win 8 / in 2012. It would be a good thing to check what is supported on Win XP.
I tried calling LockFile on XP SP3, and it worked as intended. I think the table that's on MSDN about Win8/2012 is trying to explain how new features introduced in those version are supported by previously existing API's.
> UnlockFile should be called as well otherwise we have to rely on the system
> which may not have unlocked the file in time when launching Firefox after an
> update.
The file gets unlocked when the handle is closed, which, since it's an AutoFile, should always happen either at the end of PatchFile::Execute or (in error cases) at the end of DoUpdate.

(In reply to Matt Howell [:mhowell] from comment #23)
> Created attachment 8740455[details][diff][review]
> Amended patch
>
> I made all the changes, except for these two comments:
>
> > The msdn page only states what is filesystems are supported on
> > Win 8 / in 2012. It would be a good thing to check what is supported on Win XP.
>
> I tried calling LockFile on XP SP3, and it worked as intended. I think the
> table that's on MSDN about Win8/2012 is trying to explain how new features
> introduced in those version are supported by previously existing API's.
I'm somewhat concerned that it might not be supported on all filesystems in earlier versions of windows which might be why they called this out for Win 8 here. I'll look into it.
>
> > UnlockFile should be called as well otherwise we have to rely on the system
> > which may not have unlocked the file in time when launching Firefox after an
> > update.
>
> The file gets unlocked when the handle is closed, which, since it's an
> AutoFile, should always happen either at the end of PatchFile::Execute or
> (in error cases) at the end of DoUpdate.
From
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365202%28v=vs.85%29.aspx
"If a process terminates with a portion of a file locked or closes a file that has outstanding locks, the locks are unlocked by the operating system. However, the time it takes for the operating system to unlock these locks depends upon available system resources. Therefore, it is recommended that your process explicitly unlock all files it has locked when it terminates. If this is not done, access to these files may be denied if the operating system has not yet unlocked them."
In my experience, these types of warnings are best heeded. Also, the page for LockFile doesn't state that the file is unlocked when the file handle is closed.

Created attachment 8740636[details][diff][review]
Further amended patch
Added calls to UnlockFile.
I'm highly confident about LockFile being supported on Windows < 8. I tried it on XP and it worked, the SDK headers don't have it behind any version checks, and the specific features MSDN calls out for Win8/2012 support are all new features in Win8/2012, so it seems like the only purpose of that table is to specifically show support for the new features. I also found a Stack Overflow answer from the Vista cycle (http://stackoverflow.com/questions/853805/locking-files-using-c-on-windows) which mentions LockFile.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #26)
> > #ifdef XP_WIN
> >+ // Lock the patch file, so it can't be messed with between
> >+ // when we're done creating it and when we go to apply it.
> >+ if (!LockFile((HANDLE)_get_osfhandle(fileno(mPatchStream)), 0, 0, -1, -1)) {
> >+ LOG(("Couldn't lock patch file: %d", GetLastError()));
> >+ return WRITE_ERROR;
> What do you think about adding a new error code for this?
Seems like a good plan. How about LOCK_ERROR_PATCH_FILE, code 73?
I'll make the other changes as well.

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #29)
> Could a reworked version of this exploit be used for the AddFile case?
> Specifically the code in
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/
> updater.cpp#1310
>
> If so, please file a followup security bug for it.
You could get the file overwriting behavior if you beat the updater in a race, because fopen() on Windows enables sharing. But I don't think you get the privilege elevation (and therefore the security issue) because AddFile::Execute extracts the file in place, so you have to have permissions to write there anyway.

The patch files are extracted under <path to install dir>\updating so if it possible for patch files I think it should be possible for add files as well which backup the existing file and then extracts on top of the existing file.

Comment on attachment 8741422[details][diff][review]
Patch
[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily, I think. Any exploit would require already having local unprivileged code execution, and even then the procedure is not exactly simple or obvious (see bug description).
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
I do not believe so.
Which older supported branches are affected by this flaw?
All
If not all supported branches, which bug introduced the flaw?
N/A
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I don't; they would be trivial to create and not at all risky, in my opinion.
How likely is this patch to cause regressions; how much testing does it need?
Regressions seem unlikely since the change is very simple and a green try run exists (this code already has test coverage).

We're too late in the release cycle to take it this release. We'd want at least a couple of betas. I'm giving this sec-approval+ to check into trunk on May 10. Please check this in there then.
We'll want it nominated for Aurora, Beta, and ESR45 at that time as well so it can go in everywhere else following trunk.

(In reply to Daniel Veditz [:dveditz] from comment #38)
> I imagine this code hasn't changed much-- does this patch apply cleanly to
> ESR-45 and Beta? If so request approval on this patch, if not we'll need
> munged versions first.
The patch does apply cleanly to both ESR45 and beta. I'll make those requests.

Comment on attachment 8741422[details][diff][review]
Patch
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
N/A; this is a sec-high bug.
User impact if declined:
Exposure to sec-high vulnerability.
Fix Landed on Version:
Nightly 49. This request is for ESR45 and beta 47.
Risk to taking this patch (and alternatives if risky):
Low; patch passed existing testing and has been running for a week on Nightly with no reported issues.
String or UUID changes made by this patch:
None.