Bug 579593 (CVE-2010-3131)

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

RESOLVED FIXED in mozilla2.0b4

Status

()

Core
Widget: Win32
--
major
RESOLVED FIXED
7 years ago
a year ago

People

(Reporter: reed, Assigned: reed)

Tracking

({verified1.9.1, verified1.9.2})

unspecified
mozilla2.0b4
All
Windows XP
verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(blocking2.0 final+, blocking1.9.2 .9+, status1.9.2 .9-fixed, blocking1.9.1 .12+, status1.9.1 .12-fixed)

Details

(Whiteboard: [sg:critical] [not entirely local])

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 458030 [details]
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.
(Assignee)

Updated

7 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
blocking2.0: --- → ?
status1.9.1: --- → ?
status1.9.2: --- → ?
(Assignee)

Comment 1

7 years ago
Created attachment 458037 [details] [diff] [review]
possible fix - v1

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+
(Assignee)

Comment 3

7 years ago
Created attachment 458058 [details] [diff] [review]
possible fix - v2
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
Last Resolved: 7 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 286382
(Assignee)

Comment 6

7 years ago
(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 → ---
(Assignee)

Comment 7

7 years ago
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: - → ?
status1.9.1: ? → wanted
status1.9.2: ? → wanted
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]

Updated

7 years ago
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]

Updated

7 years ago
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
Last Resolved: 7 years ago7 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]

Comment 14

7 years ago
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)?

Updated

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

Comment 16

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

Updated

7 years ago
Assignee: ehsan → tellrob

Comment 18

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

Comment 20

7 years ago
I can fix the patch tonight. I've just been busy finishing school + preparing to move cross-country.
Assignee: tellrob → reed
Status: REOPENED → ASSIGNED
(Assignee)

Comment 21

7 years ago
Created attachment 465570 [details] [diff] [review]
patch - v3

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

Comment 23

7 years ago
Created attachment 465644 [details] [diff] [review]
patch - v4 (trunk)

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+

Comment 25

7 years ago
Note to verifiers: When verifying this bug it needs to be verified on multiple windows platforms.

Comment 26

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

Comment 29

7 years ago
http://hg.mozilla.org/mozilla-central/rev/df37bb06494a
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Attachment #465644 - Flags: approval1.9.2.9?
Attachment #465644 - Flags: approval1.9.1.12?
(Assignee)

Updated

7 years ago
Attachment #465644 - Flags: approval1.9.2.9?
Attachment #465644 - Flags: approval1.9.1.12?
(Assignee)

Updated

7 years ago
Attachment #465644 - Attachment description: patch - v4 → patch - v4 (trunk)
(Assignee)

Comment 30

7 years ago
Created attachment 465960 [details] [diff] [review]
patch - v1 (1.9.2)
Attachment #465960 - Flags: review?(tellrob)
(Assignee)

Comment 31

7 years ago
Created attachment 465961 [details] [diff] [review]
patch - v1 (1.9.1)
Attachment #465961 - Flags: review?(tellrob)
(Assignee)

Comment 32

7 years ago
Created attachment 465962 [details] [diff] [review]
patch - v1 (1.9.1)

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+
(Assignee)

Updated

7 years ago
Attachment #465962 - Flags: approval1.9.1.12?
(Assignee)

Comment 35

7 years ago
Created attachment 466008 [details] [diff] [review]
patch - v2 (1.9.2)
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+

Comment 38

7 years ago
Reed likely won't be available soon...dveditz can you land these as well?
Assignee: reed → dveditz
(Assignee)

Updated

7 years ago
Assignee: dveditz → reed
(Assignee)

Comment 39

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fe2f80c3a6b6
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1734e7184f54
status1.9.1: wanted → .12-fixed
status1.9.2: wanted → .9-fixed
(Assignee)

Comment 40

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/de354bd1adde
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? :-)
Keywords: verified1.9.2
(Assignee)

Updated

7 years ago
Duplicate of this bug: 589925
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
(Assignee)

Updated

7 years ago
Duplicate of this bug: 591577
Group: core-security

Updated

7 years ago
Duplicate of this bug: 593746
Flags: sec-bounty+
Duplicate of this bug: 1034713
Duplicate of this bug: 1283794
You need to log in before you can comment on or make changes to this bug.