Closed Bug 579593 (CVE-2010-3131) Opened 14 years ago Closed 14 years ago

Windows XP "dwmapi.dll" Dll-load Vulnerability (FG-VD-10-022)

Categories

(Core :: Widget: Win32, defect)

All
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla2.0b4
Tracking Status
blocking2.0 --- final+
blocking1.9.2 --- .9+
status1.9.2 --- .9-fixed
blocking1.9.1 --- .12+
status1.9.1 --- .12-fixed

People

(Reporter: reed, Assigned: reed)

References

Details

(Keywords: verified1.9.1, verified1.9.2, Whiteboard: [sg:critical] [not entirely local])

Attachments

(4 files, 5 obsolete files)

Attached file PoC
Haifei Li of Fortinet's FortiGuard Labs submitted the following advisory to security@. The video is too large to attach, so if you need it, contact me OOB.

==============================================================================

//NOTE: The following vulnerability information is a part of the pending-released paper from the author regarding the Dll-load attack research. Additional information (PoC, reproduce video) is attached as well.

//We recommend you fix this issue as soon as possible since we are going the release our paper including all the details of this vulnerability in the near future. If you have any concerns let us know, we would like to work with you resolve this issue.

by Haifei Li of Fortinet's FortiGuard Labs (hfli@fortinet.com)

2. Mozilla Firefox on Windows XP "dwmapi.dll" Dll-load Vulnerability

Confirmed Affected Conditions:
Mozilla Firefox 3.6.6 on Windows XP SP3

Description:
When opening a html file with Firefox directly (by double-clicking on the file, for example, Firefox is the default web browser), Firefox will try to load a dll named "dwmapi.dll" in the system. This "dwmapi.dll" is a system dll on Windows editions after Windows XP such as Windows Vista and Windows 7. However, on Windows XP there is no such a dll. Thus, if the attacker put a malicious "dwmapi.dll" at the same directory of the html file, malicious code will be executed when opening the html file by Firefox.

This dll-loading process is done by LoadLibrary in “xul.dll” as the following codes:

.text:10253717          push    	offset s_Dwmapi_dll 	; "dwmapi.dll"
.text:1025371C          call    	edi ; LoadLibraryW

Thus, it follows the SSO. The following detailed search order also shows the point (in the case that “dwmapi.dll” does not exist in the system, “C:\firefox” is the CWD):

C:\Program Files\Mozilla Firefox\dwmapi.dll		NOT FOUND
C:\WINDOWS\system32\dwmapi.dll			NOT FOUND
C:\WINDOWS\system\dwmapi.dll				NOT FOUND
C:\WINDOWS\dwmapi.dll					NOT FOUND
C:\firefox\dwmapi.dll						NOT FOUND
C:\Program Files\Mozilla Firefox\dwmapi.dll		NOT FOUND
C:\WINDOWS\system32\dwmapi.dll			NOT FOUND
C:\WINDOWS\dwmapi.dll					NOT FOUND
C:\WINDOWS\System32\Wbem\dwmapi.dll		NOT FOUND

On the development side:
We could infer that in the developing/testing process of Firefox, the vendor used highly-version Windows as their testing environment, they did not realize that their loading-dll could be non-exist on other platforms.

This case suggests that vendor should test their product on all the platforms that their product could run on.

A middle course is that putting the system dll in the application directory when installing, however, this requires that if the system dll is updated by Microsoft for security problems in future, the 3rd party vendor should also update the dll with Microsoft to ensure the safety of their product.
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → ?
Attached patch possible fix - v1 (obsolete) — Splinter Review
Here's a possible fix (including code clean-up).

The problem is here:
131 #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN
132    sDwmDLL = ::LoadLibraryW(kDwmLibraryName);

What that is supposed to do is only load that library if the machine is Vista or later. Instead, it loads it for all windows versions because our build machine meets that ifdef req. A build-time check will not work for this. It has to be a runtime check.

To fix it, I encapsulated the ::LoadLibraryW() call in an if(sVistaOrLater) check.
Assignee: nobody → reed
Status: NEW → ASSIGNED
Attachment #458037 - Flags: review?(tellrob)
Comment on attachment 458037 [details] [diff] [review]
possible fix - v1

>diff --git a/widget/src/windows/nsUXThemeData.cpp b/widget/src/windows/nsUXThemeData.cpp
>--- a/widget/src/windows/nsUXThemeData.cpp
>+++ b/widget/src/windows/nsUXThemeData.cpp
>@@ -12,18 +12,18 @@
>  * Software distributed under the License is distributed on an "AS IS" basis,
>  * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>  * for the specific language governing rights and limitations under the
>  * License.
>  *
>  * The Original Code is the Mozilla browser.
>  *
>  * The Initial Developer of the Original Code is
>- * Netscape Communications Corporation.
>- * Portions created by the Initial Developer are Copyright (C) 1999
>+ * the Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2008
>  * the Initial Developer. All Rights Reserved.

No need to change this here - parts of the file contents may actually be copyright 1999 Netscape Communication Corporation. This was originally taken from nsNativeThemeWin and other bits.

>   ::ZeroMemory(sThemes, sizeof(sThemes));
>   NS_ASSERTION(!sThemeDLL, "nsUXThemeData being initialized twice!");
>+
>+  PRInt32 version = nsWindow::GetWindowsVersion();
>+  sIsXPOrLater = version >= WINXP_VERSION;
>+  sIsVistaOrLater = version >= VISTA_VERSION;
>+
>   sThemeDLL = ::LoadLibraryW(kThemeLibraryName);
>-  if (sThemeDLL) {
>+  if(sThemeDLL) {

Don't change the style here.

Also, if we are to be consistent then we need to also insert a similar check here for sIsXPOrLater otherwise Windows 2000 is vulnerable to the same sort of bug.

>-  if (sIsXPOrLater) {
>+  if(sIsXPOrLater) {

Same style comment applies here.

r=me with those changes.
Attachment #458037 - Flags: review?(tellrob) → review+
Attached patch possible fix - v2 (obsolete) — Splinter Review
Attachment #458037 - Attachment is obsolete: true
Attachment #458058 - Flags: review?(tellrob)
This feels related to bug 286382.
This is in fact a dupe of bug 286382.  I don't think it's worth taking this patch on its own.  We'll fix the issue globally in that bug.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
(In reply to comment #5)
> This is in fact a dupe of bug 286382.  I don't think it's worth taking this
> patch on its own.  We'll fix the issue globally in that bug.

Uh, no. We shouldn't even be *trying* to load the library on certain OSes.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment on attachment 458058 [details] [diff] [review]
possible fix - v2

New patch coming up soon.
Attachment #458058 - Attachment is obsolete: true
Attachment #458058 - Flags: review?(tellrob)
(In reply to comment #6)
> (In reply to comment #5)
> > This is in fact a dupe of bug 286382.  I don't think it's worth taking this
> > patch on its own.  We'll fix the issue globally in that bug.
> 
> Uh, no. We shouldn't even be *trying* to load the library on certain OSes.

Why?  The whole purpose of loading a library and handling the failure case is that version-based checks are prone to mistakes and failures.
(In reply to comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > This is in fact a dupe of bug 286382.  I don't think it's worth taking this
> > > patch on its own.  We'll fix the issue globally in that bug.
> > 
> > Uh, no. We shouldn't even be *trying* to load the library on certain OSes.
> 
> Why?  The whole purpose of loading a library and handling the failure case is
> that version-based checks are prone to mistakes and failures.

I'm not CC'd on the other bug so I don't know what it is or how it solves this one, but I don't see why this fix is bad. We have not tested and do not support loading these DLLs on versions of Windows prior to the ones checked for in the conditions. All this does is codify our assumptions about what platforms we support.
Not a blocker, but I'd take a patch.
blocking2.0: ? → -
This is not entirely a local problem, it can be exploited remotely through UNC paths (this specific .dll was called out by researchers from ACROS as a specific case of bug 286382 which is why we upped the severity of that bug). Renominating as a 2.0 blocker.
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .8+
blocking2.0: - → ?
Depends on: 286382
Whiteboard: [sg:moderate local] → [sg:critical] not entirely local
blocking2.0: ? → final+
I tested to make sure that my patch for bug 286382 (attachment 459195 [details] [diff] [review]) fixes this bug.
Whiteboard: [sg:critical] not entirely local → [sg:critical] [not entirely local] [will be fixed by bug 286382]
Whiteboard: [sg:critical] [not entirely local] [will be fixed by bug 286382] → [sg:critical] [not entirely local] [will be fixed by bug 286382][critsmash:investigating]
Whiteboard: [sg:critical] [not entirely local] [will be fixed by bug 286382][critsmash:investigating] → [sg:critical] [not entirely local] [will be fixed by bug 286382][critsmash:patch]
Assignee: reed → ehsan
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical] [not entirely local] [will be fixed by bug 286382][critsmash:patch] → [sg:critical] [not entirely local] [fixed by bug 286382]
Target Milestone: --- → mozilla2.0b4
See bug 286382 comment 78.
Status: RESOLVED → REOPENED
blocking1.9.1: .12+ → ?
blocking1.9.2: .9+ → ?
Resolution: FIXED → ---
Whiteboard: [sg:critical] [not entirely local] [fixed by bug 286382] → [sg:critical] [not entirely local]
We'd like this spot-fix for 1.9.2.9 and 1.9.1.12. Do you think this can come in by code-freeze (tomorrow @ 11:59 PST)?
blocking1.9.1: ? → .12+
blocking1.9.2: ? → .9+
(In reply to comment #14)
> We'd like this spot-fix for 1.9.2.9 and 1.9.1.12. Do you think this can come in
> by code-freeze (tomorrow @ 11:59 PST)?

Bug 286382 has been making progress, but it won't make it to tomorrow's freeze for sure (unless a miracle happens, but I don't believe in miracles).
Right, and we don't want to take bug 286382 yet. Dan suggested we take the spot-fix in this bug if it is safe and helps.
(In reply to comment #16)
> Right, and we don't want to take bug 286382 yet. Dan suggested we take the
> spot-fix in this bug if it is safe and helps.

Yes, it should be safe, although I won't have enough time to make it happen by tomorrow night, I'm afraid.  Maybe Rob can pick it up again?
Assignee: ehsan → tellrob
Rob, do you think you could take a look? Thanks!
I can't really make any guarantees about producing a patch by tomorrow. I'm currently "on fire" for another project. Reed pretty much had it but it didn't compile so if someone takes that patch, I can review it assuming that they've checked it builds and doesn't obviously break things on Vista/7.
I can fix the patch tonight. I've just been busy finishing school + preparing to move cross-country.
Assignee: tellrob → reed
Status: REOPENED → ASSIGNED
Attached patch patch - v3 (obsolete) — Splinter Review
Currently driving cross-country, but here's what I have right now. It's still untested.
Attachment #465570 - Flags: review?(tellrob)
Comment on attachment 465570 [details] [diff] [review]
patch - v3

This looks correct but I don't have a build handy to verify that claim. Nit:

-  HMODULE hTheme = ::LoadLibraryW(kThemeLibraryName);

can just turn into
HMODULE hTheme = nsUXThemeData::GetThemeDLL();

so there's less noise in the patch.

Also doesn't apply cleanly to 1.9.2 but I think only minor changes would be needed.
Hitting the road again now, so won't be able to look at this again until tonight.
Attachment #465570 - Attachment is obsolete: true
Attachment #465644 - Flags: review?(tellrob)
Attachment #465570 - Flags: review?(tellrob)
Comment on attachment 465644 [details] [diff] [review]
patch - v4 (trunk)

This appears to work on my m-c build on Windows 7. I don't have an XP machine handy to check if things look good there but I expect they will.
Attachment #465644 - Flags: review?(tellrob) → review+
Note to verifiers: When verifying this bug it needs to be verified on multiple windows platforms.
Reed said he will not be able to land this today. If anyone cc'd wants to pick this up, that would be great!
(In reply to comment #26)
> Reed said he will not be able to land this today. If anyone cc'd wants to pick
> this up, that would be great!

Is this actually ready to land?
It needs versions for 1.9.2 and 1.9.1 because the patch fails to apply there. This is ready for m-c though.
http://hg.mozilla.org/mozilla-central/rev/df37bb06494a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Attachment #465644 - Flags: approval1.9.2.9?
Attachment #465644 - Flags: approval1.9.1.12?
Attachment #465644 - Flags: approval1.9.2.9?
Attachment #465644 - Flags: approval1.9.1.12?
Attachment #465644 - Attachment description: patch - v4 → patch - v4 (trunk)
Attached patch patch - v1 (1.9.2) (obsolete) — Splinter Review
Attachment #465960 - Flags: review?(tellrob)
Attached patch patch - v1 (1.9.1) (obsolete) — Splinter Review
Attachment #465961 - Flags: review?(tellrob)
The real 1.9.1 patch.
Attachment #465961 - Attachment is obsolete: true
Attachment #465962 - Flags: review?(tellrob)
Attachment #465961 - Flags: review?(tellrob)
Comment on attachment 465960 [details] [diff] [review]
patch - v1 (1.9.2)

Good but wrap the declaration and definition in a #ifdef MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_LONGHORN as done in bug 587322.
Attachment #465960 - Flags: review?(tellrob) → review+
Comment on attachment 465962 [details] [diff] [review]
patch - v1 (1.9.1)

I don't think we have the infrastructure in place to wrap here so this looks good.
Attachment #465962 - Flags: review?(tellrob) → review+
Attachment #465962 - Flags: approval1.9.1.12?
Attachment #465960 - Attachment is obsolete: true
Attachment #466008 - Flags: review+
Attachment #466008 - Flags: approval1.9.2.9?
Comment on attachment 465962 [details] [diff] [review]
patch - v1 (1.9.1)

Approved for 1.9.1.12, a=dveditz
Attachment #465962 - Flags: approval1.9.1.12? → approval1.9.1.12+
Comment on attachment 466008 [details] [diff] [review]
patch - v2 (1.9.2)

Approved for 1.9.2.9, a=dveditz
Attachment #466008 - Flags: approval1.9.2.9? → approval1.9.2.9+
Reed likely won't be available soon...dveditz can you land these as well?
Assignee: reed → dveditz
Assignee: dveditz → reed
I verified this for 1.9.2 (8/22 nightly), dwmapi.dll is no longer loaded on WinXP SP3   (used "Process Monitor" from Microsoft, formerly SysInternals).
Dan, can you verify this for 1.9.1 since you're set up for it and know what you're doing already? :-)
I verified this on 1.9.1.12 as well.
Keywords: verified1.9.1
Mitre has assigned CVE-2010-3131 to this.
Alias: CVE-2010-3131
Group: core-security
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.