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

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Reed Loden [:reed]]

Attachment: 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 ([e-mail])

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

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

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

Attachment: possible fix - v2

Comment 4

[Joe Drew (not getting mail)]

This feels related to bug 286382.

Comment 5

[(no longer active)]

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.

Comment 6

[Reed Loden [:reed]]

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

Comment on attachment 458058 [details] [diff] [review]
possible fix - v2

New patch coming up soon.

Comment 8

[(no longer active)]

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

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

Not a blocker, but I'd take a patch.

Comment 11

[Daniel Veditz [:dveditz]]

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

[(no longer active)]

I tested to make sure that my patch for bug 286382 (attachment 459195 [details] [diff] [review]) fixes this bug.

Comment 13

[(no longer active)]

See bug 286382 comment 78.

Comment 14

[LegNeato]

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

[(no longer active)]

(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

[LegNeato]

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

[(no longer active)]

(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

[LegNeato]

Rob, do you think you could take a look? Thanks!

Comment 19

[Rob Arnold [:robarnold]]

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

I can fix the patch tonight. I've just been busy finishing school + preparing to move cross-country.

Comment 21

[Reed Loden [:reed]]

Attachment: patch - v3

Currently driving cross-country, but here's what I have right now. It's still untested.

Comment 22

[Rob Arnold [:robarnold]]

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

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

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

[LegNeato]

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

Comment 26

[LegNeato]

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

[(no longer active)]

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

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

http://hg.mozilla.org/mozilla-central/rev/df37bb06494a

Comment 30

[Reed Loden [:reed]]

Attachment: patch - v1 (1.9.2)

Comment 31

[Reed Loden [:reed]]

Attachment: patch - v1 (1.9.1)

Comment 32

[Reed Loden [:reed]]

Attachment: patch - v1 (1.9.1)

The real 1.9.1 patch.

Comment 33

[Rob Arnold [:robarnold]]

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

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

Attachment: patch - v2 (1.9.2)

Comment 36

[Daniel Veditz [:dveditz]]

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

Comment on attachment 466008 [details] [diff] [review]
patch - v2 (1.9.2)

Approved for 1.9.2.9, a=dveditz

Comment 38

[LegNeato]

Reed likely won't be available soon...dveditz can you land these as well?

Comment 39

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fe2f80c3a6b6
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1734e7184f54

Comment 40

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/de354bd1adde

Comment 41

[Daniel Veditz [:dveditz]]

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 - ex-MoCo]]

Dan, can you verify this for 1.9.1 since you're set up for it and know what you're doing already? :-)

Comment 44

[Daniel Veditz [:dveditz]]

I verified this on 1.9.1.12 as well.

Comment 45

[Daniel Veditz [:dveditz]]

Mitre has assigned CVE-2010-3131 to this.