Mitja Kolsek reported this today as a follow up to CVE-2010-3131.
-----
Our colleague noticed that one of the binary planting issues we reported to you last year during the "binary planting sweep" hasn't been fixed yet - or it has subsequently returned to the code. I have attached our report so you don't have to dig through the archives.
The test was done on Firefox 3.6.15. Soon-to-be-released Firefox 4 mitigates this issue to some extent by calling SetDllDirectory(""), but due to a bug in Windows, this mitigation is not fully effective (see
http://blog.acrossecurity.com/2010/10/breaking-setdlldirectory-protection.html). We confirmed that a "problematic" PATH entry, as described in this blog post, makes FF4 again vulnerable to binary planting through sensor.dll.
-----
A quick mxr search for sensor.dll turns up:
PRBool
ThinkPadSensor::Startup()
{
mLibrary = LoadLibraryW(L"sensor.dll");
if (!mLibrary)
return PR_FALSE;
gShockproofGetAccelerometerData = (ShockproofGetAccelerometerData)
GetProcAddress(mLibrary, "ShockproofGetAccelerometerData");
if (!gShockproofGetAccelerometerData) {
FreeLibrary(mLibrary);
mLibrary = nsnull;
return PR_FALSE;
}
return PR_TRUE;
}
I'm copying Ehsan since he fixed a more recent version of one of these.

Hmm, is this a theoretical concern, or is there a real exploit which causes us to load sensor.dll this way? I stepped into patched_LdrLoadDll, and it seems like the filePath variable passed to it contains the expanded versions of the environment string variables, so at the first investigation, we don't seem to be vulnerable to this attach in 4.0.

Created attachment 520316[details]
Reference files
(In reply to comment #3)
> Can you please attach the sensor.dll file mentioned in the article too, so that
> I can try to reproduce it?
Attached, as requested.

Hmm, I can reproduce on trunk with a directory named %CommonProgramFiles%. Thanks, Microsoft :(
OK, I have a fix plan here, but so far I have been unable to trigger this bug in my local builds for some reason.
I think we should address this at two levels. The first level would be to address this bug in general for Firefox 4 and Firefox Next. The other level would be to switch to loading these DLLs from absolute paths for branches.
Nominating for .x on 2.0, and also on branches.

The second part of the fix which is more useful to 3.6 is for us to load sensor.dll and axcon.dll from full paths.
I need to know the correct path name to those DLLs on Thinkpad and Toshiba systems. Can you guys help me with that, please?

(In reply to comment #8)
> The second part of the fix which is more useful to 3.6 is for us to load
> sensor.dll and axcon.dll from full paths.
>
> I need to know the correct path name to those DLLs on Thinkpad and Toshiba
> systems. Can you guys help me with that, please?
\windows\axcon.dll. But this code is for Windows Mobile.

(In reply to comment #10)
> (In reply to comment #8)
> > The second part of the fix which is more useful to 3.6 is for us to load
> > sensor.dll and axcon.dll from full paths.
> >
> > I need to know the correct path name to those DLLs on Thinkpad and Toshiba
> > systems. Can you guys help me with that, please?
>
> \windows\axcon.dll. But this code is for Windows Mobile.
So can we just remove the code responsible for loading it?

(In reply to comment #12)
> So can we just remove the code responsible for loading it?
We don't release for WinMo version, so you can remove this with HTC code and Samsung code for WinMo devices. If we develop Windows Phone 7 version, we need another implementation for Phone 7 (Phone 7 has accelerometer API on framework).

(In reply to comment #13)
> (In reply to comment #12)
> > So can we just remove the code responsible for loading it?
>
> We don't release for WinMo version, so you can remove this with HTC code and
> Samsung code for WinMo devices. If we develop Windows Phone 7 version, we need
> another implementation for Phone 7 (Phone 7 has accelerometer API on
> framework).
OK, I filed bug 644743 for that.

(In reply to comment #17)
> But are we sure that this is where the DLL is found on every Thinkpad machine?
I don't think we can be. The API exposed by the DLL is not officially documented, so I guess the location of the DLL is also not documented.

(In reply to comment #18)
> (In reply to comment #17)
> > But are we sure that this is where the DLL is found on every Thinkpad machine?
>
> I don't think we can be. The API exposed by the DLL is not officially
> documented, so I guess the location of the DLL is also not documented.
Can we reach out to Lenovo and ask them about it?

If LoadLibraryW(L"sensor.dll") worked well on computers where sensor.dll existed, this DLL must have been in one of the following locations:
1) Home path of firefox.exe
2) System32 folder
3) System folder
4) Windows folder
5) Current working directory (only in an attack)
6) One of the directories in %PATH%
If it's impossible to get a reliable answer to the DLL's exact location, there are alternative solutions:
a) Calling SetDllDirectory("") before the LoadLibrary call, and SetDllDirectory(NULL) immediately afterwards. This will remove the CWD from the search order, which resolves the vulnerability, but it will create a tiny time frame where other threads will be affected by the SetDllDirectory call (it changes the search order for the entire process). With thousands of Firefox add-ons out there, there may be a few that do the DLL loading in a weird but functional way by first changing the CWD to the location of the library and then relying on LoadLibrary to find the DLL - in the off chance that such loading would occur exactly between the two SetDllDirectory calls, it would fail.
b) Similar to a), but instead of calling SetDllDirectory, changing the CWD to some safe location immediately before the LoadLibrary call and restoring it immediately after. This could have a similar effect on other threads relying on the CWD during the time frame.
Neither of these should break the loading of sensor.dll on computers where it currently works.
c) If sensor.dll is not found in %PATH%, it means it is either in firefox's home folder (which it isn't or you guys would know it) or in one of the three system folders. If so, three LoadLibrary calls with full paths to Windows\System32\sensor.dll, Windows\System\sensor.dll and Windows\sensor.dll (using functions GetSystemDirectory and GetWindowsDirectory) should do the trick with minimal overhead. However, if sensor.dll should happen to be in %PATH%, this would be a dangerous situation for all applications using it (presumably Firefox is not the only one) as they will likely have the same kind of binary planting bug. In this case the vendor should be contacted and advised against having the DLL found in %PATH%, but I see no good solution for this particular bug in Firefox.

(In reply to comment #20)
> If LoadLibraryW(L"sensor.dll") worked well on computers where sensor.dll
> existed, this DLL must have been in one of the following locations:
>
> 1) Home path of firefox.exe
> 2) System32 folder
> 3) System folder
> 4) Windows folder
> 5) Current working directory (only in an attack)
> 6) One of the directories in %PATH%
Yes. But I don't want to make guesses, I just want to load it from the correct directory, and fail otherwise.
> If it's impossible to get a reliable answer to the DLL's exact location, there
> are alternative solutions:
>
> a) Calling SetDllDirectory("") before the LoadLibrary call, and
> SetDllDirectory(NULL) immediately afterwards.
We already have this protection in place. This does not protect against this bug though because of the bug in expansion of environment variables in Windows.
> b) Similar to a), but instead of calling SetDllDirectory, changing the CWD to
> some safe location immediately before the LoadLibrary call and restoring it
> immediately after.
I've gave this a shot, but it's not possible to get it to work correctly because of race conditions that will happen.
Really, for branch, the correct fix is to just attempt to load this DLL from the directory in which we know it exists, and fail otherwise.
CCing JP to see if he can help me in outreaching to Lenovo...

Comment on attachment 521033[details][diff][review]
General fix: sanitize the environment variable PATH by doing the environment string expansion upfront
Is there any particular reason you're using getenv/putenv instead of GetEnvironmentVariable/PutEnvironmentVariable in Windows-specific code? with _putenv we need to leak the data, but with SetEnvironmentVariable we don't (and we don't have to do the PATH= string-fu either.

(In reply to comment #30)
That does not sound related at all to this. It mentions "Lenovo ThinkVantage Password Manager" and "Client Security Solution", which is a product from Lenovo containing amongst other things a Firefox Add-on.
My understanding of this bug is that it is about loading a DLL from an unspecified location from within Firefox. The DLL belongs to an unrelated Lenovo product named "ThinkVantage Active Protection System". From what I understand from this bug, Firefox is vulnerable if APS is not installed on the system. My understanding is that PCs not made by Lenovo would be vulnerable because of this bug, and I do not think a patch from Lenovo could do anything here.
Of course I can try to test it, but I really cannot see the point of it, as it seems totally unrelated.
This seems to be fixed from Firefox 4. If the remaining fix is Firefox 3.6 only, can't we just assume that the location is always C:\Windows\System32\Sensor.DLL ? If we guess wrong, we risk that Firefox 3.6 users will not get orientation support even if they have orientation capable hardware.

I'm trying to understand the repro case for this bug for 1.9.2 verification. Reading through all the comments here, it isn't clear to me how to reproduce this issue on my Windows XP system. Can anyone give the actual STR?

(In reply to Ehsan Akhgari [:ehsan] from comment #36)
> (In reply to comment #35)
> > I'm trying to understand the repro case for this bug for 1.9.2 verification.
> > Reading through all the comments here, it isn't clear to me how to reproduce
> > this issue on my Windows XP system. Can anyone give the actual STR?
>
> This post
> <http://blog.acrossecurity.com/2010/10/breaking-setdlldirectory-protection.
> html> has the STR that I used to reproduce the bug.
I've read through this. It looks like you need ActivePython installed for their example but I see no STR to reproduce the exploit in the blog post beyond their discussion of the overall issue. They launch from some sort of demo page in their video using iTunes and Safari. Do you have a test script or something similar?

(In reply to comment #37)
> (In reply to Ehsan Akhgari [:ehsan] from comment #36)
> > (In reply to comment #35)
> > > I'm trying to understand the repro case for this bug for 1.9.2 verification.
> > > Reading through all the comments here, it isn't clear to me how to reproduce
> > > this issue on my Windows XP system. Can anyone give the actual STR?
> >
> > This post
> > <http://blog.acrossecurity.com/2010/10/breaking-setdlldirectory-protection.
> > html> has the STR that I used to reproduce the bug.
>
> I've read through this. It looks like you need ActivePython installed for their
> example but I see no STR to reproduce the exploit in the blog post beyond their
> discussion of the overall issue. They launch from some sort of demo page in
> their video using iTunes and Safari. Do you have a test script or something
> similar?
Unfortunately not. Back when I worked on this bug, I came up with a rough STR based on that post, but I've lost the profile I did this on since then... IIRC what mattered was setting the PATH env variable to include %CommonProgramFiles%, and making sure that the directory from which the html file is opened contains a directory called %CommonProgramFiles% with a sensor.dll file inside it...