Last Comment Bug 579593 - (CVE-2010-3131) Windows XP "dwmapi.dll" Dll-load Vulnerability (FG-VD-10-022)
(CVE-2010-3131)
: Windows XP "dwmapi.dll" Dll-load Vulnerability (FG-VD-10-022)
Status: RESOLVED FIXED
[sg:critical] [not entirely local]
: verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: unspecified
: All Windows XP
: -- major (vote)
: mozilla2.0b4
Assigned To: Reed Loden [:reed] (use needinfo?)
:
Mentors:
: 589925 591577 593746 1034713 1283794 (view as bug list)
Depends on: 286382
Blocks:
  Show dependency treegraph
 
Reported: 2010-07-16 18:29 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2016-07-01 03:27 PDT (History)
20 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
final+
.9+
.9-fixed
.12+
.12-fixed


Attachments
PoC (10.36 KB, application/x-msdos-program)
2010-07-16 18:29 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details
possible fix - v1 (5.57 KB, patch)
2010-07-16 18:52 PDT, Reed Loden [:reed] (use needinfo?)
tellrob: review+
Details | Diff | Splinter Review
possible fix - v2 (9.46 KB, patch)
2010-07-17 00:51 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details | Diff | Splinter Review
patch - v3 (9.55 KB, patch)
2010-08-12 23:51 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details | Diff | Splinter Review
patch - v4 (trunk) (8.34 KB, patch)
2010-08-13 07:01 PDT, Reed Loden [:reed] (use needinfo?)
tellrob: review+
Details | Diff | Splinter Review
patch - v1 (1.9.2) (8.50 KB, patch)
2010-08-14 00:27 PDT, Reed Loden [:reed] (use needinfo?)
tellrob: review+
Details | Diff | Splinter Review
patch - v1 (1.9.1) (8.50 KB, patch)
2010-08-14 00:27 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details | Diff | Splinter Review
patch - v1 (1.9.1) (3.83 KB, patch)
2010-08-14 00:28 PDT, Reed Loden [:reed] (use needinfo?)
tellrob: review+
dveditz: approval1.9.1.12+
Details | Diff | Splinter Review
patch - v2 (1.9.2) (8.61 KB, patch)
2010-08-14 09:26 PDT, Reed Loden [:reed] (use needinfo?)
reed: review+
dveditz: approval1.9.2.9+
Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2010-07-16 18:29:58 PDT
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.
Comment 1 Reed Loden [:reed] (use needinfo?) 2010-07-16 18:52:05 PDT
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.
Comment 2 Rob Arnold [:robarnold] 2010-07-16 23:39:05 PDT
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.
Comment 3 Reed Loden [:reed] (use needinfo?) 2010-07-17 00:51:44 PDT
Created attachment 458058 [details] [diff] [review]
possible fix - v2
Comment 4 Joe Drew (not getting mail) 2010-07-19 10:54:36 PDT
This feels related to bug 286382.
Comment 5 :Ehsan Akhgari 2010-07-19 14:55:41 PDT
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.

*** This bug has been marked as a duplicate of bug 286382 ***
Comment 6 Reed Loden [:reed] (use needinfo?) 2010-07-19 17:18:38 PDT
(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.
Comment 7 Reed Loden [:reed] (use needinfo?) 2010-07-19 17:19:06 PDT
Comment on attachment 458058 [details] [diff] [review]
possible fix - v2

New patch coming up soon.
Comment 8 :Ehsan Akhgari 2010-07-19 21:18:32 PDT
(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.
Comment 9 Rob Arnold [:robarnold] 2010-07-19 21:33:33 PDT
(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.
Comment 10 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-07-21 08:19:56 PDT
Not a blocker, but I'd take a patch.
Comment 11 Daniel Veditz [:dveditz] 2010-07-21 11:03:55 PDT
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.
Comment 12 :Ehsan Akhgari 2010-07-21 16:23:38 PDT
I tested to make sure that my patch for bug 286382 (attachment 459195 [details] [diff] [review]) fixes this bug.
Comment 13 :Ehsan Akhgari 2010-08-09 12:58:55 PDT
See bug 286382 comment 78.
Comment 14 christian 2010-08-11 10:18:22 PDT
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)?
Comment 15 :Ehsan Akhgari 2010-08-11 10:56:10 PDT
(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 christian 2010-08-11 11:12:51 PDT
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.
Comment 17 :Ehsan Akhgari 2010-08-11 12:20:24 PDT
(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?
Comment 18 christian 2010-08-11 12:24:25 PDT
Rob, do you think you could take a look? Thanks!
Comment 19 Rob Arnold [:robarnold] 2010-08-11 15:03:17 PDT
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.
Comment 20 Reed Loden [:reed] (use needinfo?) 2010-08-11 15:09:34 PDT
I can fix the patch tonight. I've just been busy finishing school + preparing to move cross-country.
Comment 21 Reed Loden [:reed] (use needinfo?) 2010-08-12 23:51:04 PDT
Created attachment 465570 [details] [diff] [review]
patch - v3

Currently driving cross-country, but here's what I have right now. It's still untested.
Comment 22 Rob Arnold [:robarnold] 2010-08-13 00:23:37 PDT
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.
Comment 23 Reed Loden [:reed] (use needinfo?) 2010-08-13 07:01:21 PDT
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.
Comment 24 Rob Arnold [:robarnold] 2010-08-13 08:57:12 PDT
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.
Comment 25 christian 2010-08-13 10:39:55 PDT
Note to verifiers: When verifying this bug it needs to be verified on multiple windows platforms.
Comment 26 christian 2010-08-13 13:32:21 PDT
Reed said he will not be able to land this today. If anyone cc'd wants to pick this up, that would be great!
Comment 27 :Ehsan Akhgari 2010-08-13 13:50:46 PDT
(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?
Comment 28 Rob Arnold [:robarnold] 2010-08-13 13:58:11 PDT
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.
Comment 29 Reed Loden [:reed] (use needinfo?) 2010-08-14 00:07:52 PDT
http://hg.mozilla.org/mozilla-central/rev/df37bb06494a
Comment 30 Reed Loden [:reed] (use needinfo?) 2010-08-14 00:27:32 PDT
Created attachment 465960 [details] [diff] [review]
patch - v1 (1.9.2)
Comment 31 Reed Loden [:reed] (use needinfo?) 2010-08-14 00:27:58 PDT
Created attachment 465961 [details] [diff] [review]
patch - v1 (1.9.1)
Comment 32 Reed Loden [:reed] (use needinfo?) 2010-08-14 00:28:52 PDT
Created attachment 465962 [details] [diff] [review]
patch - v1 (1.9.1)

The real 1.9.1 patch.
Comment 33 Rob Arnold [:robarnold] 2010-08-14 08:06:48 PDT
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.
Comment 34 Rob Arnold [:robarnold] 2010-08-14 08:08:04 PDT
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.
Comment 35 Reed Loden [:reed] (use needinfo?) 2010-08-14 09:26:07 PDT
Created attachment 466008 [details] [diff] [review]
patch - v2 (1.9.2)
Comment 36 Daniel Veditz [:dveditz] 2010-08-14 23:28:45 PDT
Comment on attachment 465962 [details] [diff] [review]
patch - v1 (1.9.1)

Approved for 1.9.1.12, a=dveditz
Comment 37 Daniel Veditz [:dveditz] 2010-08-14 23:29:15 PDT
Comment on attachment 466008 [details] [diff] [review]
patch - v2 (1.9.2)

Approved for 1.9.2.9, a=dveditz
Comment 38 christian 2010-08-16 14:15:22 PDT
Reed likely won't be available soon...dveditz can you land these as well?
Comment 40 Reed Loden [:reed] (use needinfo?) 2010-08-16 22:51:45 PDT
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/de354bd1adde
Comment 41 Daniel Veditz [:dveditz] 2010-08-23 10:22:45 PDT
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).
Comment 42 Al Billings [:abillings] 2010-08-23 14:04:38 PDT
Dan, can you verify this for 1.9.1 since you're set up for it and know what you're doing already? :-)
Comment 43 Reed Loden [:reed] (use needinfo?) 2010-08-23 15:32:25 PDT
*** Bug 589925 has been marked as a duplicate of this bug. ***
Comment 44 Daniel Veditz [:dveditz] 2010-08-24 17:43:10 PDT
I verified this on 1.9.1.12 as well.
Comment 45 Daniel Veditz [:dveditz] 2010-08-26 12:47:00 PDT
Mitre has assigned CVE-2010-3131 to this.
Comment 46 Reed Loden [:reed] (use needinfo?) 2010-08-28 01:25:23 PDT
*** Bug 591577 has been marked as a duplicate of this bug. ***
Comment 47 Tim (fmdeveloper) 2011-02-05 16:10:03 PST
*** Bug 593746 has been marked as a duplicate of this bug. ***
Comment 51 Daniel Veditz [:dveditz] 2014-07-09 13:32:34 PDT
*** Bug 1034713 has been marked as a duplicate of this bug. ***
Comment 52 Wayne Mery (:wsmwk, NI for questions) 2016-07-01 03:27:12 PDT
*** Bug 1283794 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.