642469 - (CVE-2011-2980) Binary planting potential in ThinkPadSensor::Startup

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Brandon Sterne (:bsterne)]

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.

Comment 1

[(no longer active)]

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.

Comment 2

[Brandon Sterne (:bsterne)]

Attachment: Bug analysis

Please review the attached bug analysis from the reporter for good details of the issue.

Comment 3

[(no longer active)]

Can you please attach the sensor.dll file mentioned in the article too, so that I can try to reproduce it?

Comment 4

[Brandon Sterne (:bsterne)]

Attachment: 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.

Comment 5

[(no longer active)]

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.

Comment 6

[(no longer active)]

Attachment: General fix: sanitize the environment variable PATH by doing the environment string expansion upfront

Comment 7

[(no longer active)]

Attachment: General fix: sanitize the environment variable PATH by doing the environment string expansion upfront

The file picker change was irrelevant...

Comment 8

[(no longer active)]

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?

Comment 9

[(no longer active)]

Actually, 3.5 doesn't seem to load sensor.dll or axcon.dll, so it's not affected by this bug.

Comment 10

[Makoto Kato [:m_kato]]

(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.

Comment 11

[Mike Beltzner [:beltzner, not reading bugmail]]

Don't think this is required for a branch.

Comment 12

[(no longer active)]

(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?

Comment 13

[Makoto Kato [:m_kato]]

(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).

Comment 14

[(no longer active)]

(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.

Comment 15

[(no longer active)]

I do still need to know where sensor.dll is expected to be found on a Thinkpad system.

Comment 16

[Jesper Kristensen]

On my machine it is in C:\Windows\System32\Sensor.DLL

Comment 17

[(no longer active)]

(In reply to comment #16)
> On my machine it is in C:\Windows\System32\Sensor.DLL

But are we sure that this is where the DLL is found on every Thinkpad machine?

Comment 18

[Jesper Kristensen]

(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.

Comment 19

[(no longer active)]

(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?

Comment 20

[Mitja Kolsek]

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.

Comment 21

[(no longer active)]

(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 22

[(no longer active)]

bsmedberg: ping?

Comment 23

[JP Rosevear [:jpr]]

I will check to see if Kev has any contacts.

Comment 24

[Benjamin Smedberg]

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.

Comment 25

[(no longer active)]

Attachment: General fix: sanitize the environment variable PATH by doing the environment string expansion upfront

Addressed the review comments.

Comment 26

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/183d5a2357de

Still need information from Lenovo before I can write the branch patch.

Comment 29

[(no longer active)]

Kev: any updates here?  The code freeze for 1.9.2.18 is this monday, and if we don't hear back from Lenovo in time, this is going to miss the release...

Comment 30

[(no longer active)]

Here is what we've heard from Lenovo:

<quote>
Here is the newest update on Lenovo ThinkVantage Password Manager issue.

Lenovo fix the problem, please see the patch links are attached below.

1.) Lenovo guy will take care of lenovo forum (post the patch on it)
2.) Due to the resource issue, they will leave this patch as it is now here. We will assist them to work new patch using the solution as Key suggested, to use dlls that are not built against Firefox, and to use JS Ctypes to access the functions in those librariesfrom Firefox.

Thanks and best regards,
Hong

------------------------------
--------------------------------------------------------------
win7 http://www-307.ibm.com/pc/support/site.wss/document.do?sitestyle=lenovo&lndocid=MIGR-77138
Xp/Vista http://www-307.ibm.com/pc/support/site.wss/document.do?sitestyle=lenovo&lndocid=MIGR-77137

Please note that firefox patch only works for the latest version of Client Security Solution(or called Password manager).
Win7 CSS/Password manager
http://www-307.ibm.com/pc/support/site.wss/document.do?sitestyle=lenovo&lndocid=MIGR-73785
XP CSS/Password Manager
http://www-307.ibm.com/pc/support/site.wss/document.do?sitestyle=lenovo&lndocid=MIGR-69958
</quote>

I'm not sure if these patches address the problem at all, though.  Jesper, can you please try installing these patches on your Lenovo machine and see if it fixes the problem?  Thanks!

Comment 31

[(no longer active)]

(Please test with both Firefox 3.6 and 4)

Comment 32

[Jesper Kristensen]

(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.

Comment 33

[(no longer active)]

Attachment: 1.9.2 patch

Comment 34

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ca877d69b6ed

Comment 35

[Al Billings [:abillings - ex-MoCo]]

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?

Comment 36

[(no longer active)]

(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.

Comment 37

[Al Billings [:abillings - ex-MoCo]]

(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?

Comment 38

[(no longer active)]

(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...

Comment 39

[Raymond Forbes[:rforbes]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719