Bug 642469 (CVE-2011-2980)

Binary planting potential in ThinkPadSensor::Startup

RESOLVED FIXED in mozilla6

Status

()

Core
Document Navigation
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: bsterne, Assigned: Ehsan)

Tracking

Trunk
mozilla6
x86
Mac OS X
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(status2.0 wanted, blocking1.9.2 .20+, status1.9.2 .20-fixed, status1.9.1 unaffected)

Details

(Whiteboard: [sg:high] remote exploit possible, but browser needs to be shut down first [qa-examined-192][qa-needs-STR])

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 2

6 years ago
Created attachment 520214 [details]
Bug analysis

Please review the attached bug analysis from the reporter for good details of the issue.
Can you please attach the sensor.dll file mentioned in the article too, so that I can try to reproduce it?
(Reporter)

Comment 4

6 years ago
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.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
Created attachment 521032 [details] [diff] [review]
General fix: sanitize the environment variable PATH by doing the environment string expansion upfront
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #521032 - Flags: review?(benjamin)
Created attachment 521033 [details] [diff] [review]
General fix: sanitize the environment variable PATH by doing the environment string expansion upfront

The file picker change was irrelevant...
Attachment #521032 - Attachment is obsolete: true
Attachment #521032 - Flags: review?(benjamin)
Attachment #521033 - Flags: review?(benjamin)
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?
Actually, 3.5 doesn't seem to load sensor.dll or axcon.dll, so it's not affected by this bug.
blocking1.9.1: ? → ---
status1.9.1: --- → unaffected
(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.
Don't think this is required for a branch.
blocking2.0: ? → -
(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.
I do still need to know where sensor.dll is expected to be found on a Thinkpad system.
status1.9.2: --- → wanted
status2.0: --- → wanted
Whiteboard: [sg:high] remote exploit possible, but browser needs to be shut down first

Comment 16

6 years ago
On my machine it is in C:\Windows\System32\Sensor.DLL
Blocks: 644737
(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?
blocking1.9.2: ? → .18+
blocking2.0: - → Macaw

Comment 18

6 years ago
(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?

Comment 20

6 years ago
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...
bsmedberg: ping?

Comment 23

6 years ago
I will check to see if Kev has any contacts.

Updated

6 years ago
blocking2.0: Macaw+ → ---
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.
Created attachment 525858 [details] [diff] [review]
General fix: sanitize the environment variable PATH by doing the environment string expansion upfront

Addressed the review comments.
Attachment #521033 - Attachment is obsolete: true
Attachment #521033 - Flags: review?(benjamin)
Attachment #525858 - Flags: review?(benjamin)
Attachment #525858 - Flags: review?(benjamin) → review+
http://hg.mozilla.org/mozilla-central/rev/183d5a2357de

Still need information from Lenovo before I can write the branch patch.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
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...
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!
(Please test with both Firefox 3.6 and 4)

Comment 32

6 years ago
(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.
blocking1.9.2: .18+ → .19+
Created attachment 546865 [details] [diff] [review]
1.9.2 patch
Attachment #546865 - Flags: review?(jmathies)

Updated

6 years ago
Attachment #546865 - Flags: review?(jmathies) → review+
Attachment #546865 - Flags: approval1.9.2.20?

Updated

6 years ago
Attachment #546865 - Flags: approval1.9.2.20? → approval1.9.2.20+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/ca877d69b6ed
status1.9.2: wanted → .20-fixed
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?
Whiteboard: [sg:high] remote exploit possible, but browser needs to be shut down first → [sg:high] remote exploit possible, but browser needs to be shut down first [qa-examined-192]
(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.
Alias: CVE-2011-2980
(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?
blocking1.9.2: .20+ → .21+
status1.9.2: .20-fixed → .21-fixed
Whiteboard: [sg:high] remote exploit possible, but browser needs to be shut down first [qa-examined-192] → [sg:high] remote exploit possible, but browser needs to be shut down first [qa-examined-192][qa-needs-STR]
(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...
Group: core-security
blocking1.9.2: .23+ → .20+
status1.9.2: .23-fixed → .20-fixed
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.