Bugzilla@Mozilla – Bug 579593
Windows XP "dwmapi.dll" Dll-load Vulnerability (FG-VD-10-022)
Last modified: 2011-05-20 12:00:02 PDT
Summon comment box
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.
Created attachment 458037 [details] [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 on attachment 458037 [details] [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.
Created attachment 458058 [details] [review] possible fix - v2
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. *** This bug has been marked as a duplicate of bug 286382 ***
(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 on attachment 458058 [details] [review] possible fix - v2 New patch coming up soon.
(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.
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.
I tested to make sure that my patch for bug 286382 (attachment 459195 [details] [review]) fixes this bug.
See bug 286382 comment 78.
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)?
(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?
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.
Created attachment 465570 [details] [review] patch - v3 Currently driving cross-country, but here's what I have right now. It's still untested.
Comment on attachment 465570 [details] [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.
Created attachment 465644 [details] [review] patch - v4 (trunk) Hitting the road again now, so won't be able to look at this again until tonight.
Comment on attachment 465644 [details] [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.
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
Created attachment 465960 [details] [review] patch - v1 (1.9.2)
Created attachment 465961 [details] [review] patch - v1 (1.9.1)
Created attachment 465962 [details] [review] patch - v1 (1.9.1) The real 1.9.1 patch.
Comment on attachment 465960 [details] [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 on attachment 465962 [details] [review] patch - v1 (1.9.1) I don't think we have the infrastructure in place to wrap here so this looks good.
Created attachment 466008 [details] [review] patch - v2 (1.9.2)
Comment on attachment 465962 [details] [review] patch - v1 (1.9.1) Approved for 1.9.1.12, a=dveditz
Comment on attachment 466008 [details] [review] patch - v2 (1.9.2) Approved for 1.9.2.9, a=dveditz
Reed likely won't be available soon...dveditz can you land these as well?
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/fe2f80c3a6b6 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/1734e7184f54
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? :-)
*** Bug 589925 has been marked as a duplicate of this bug. ***
I verified this on 1.9.1.12 as well.
Mitre has assigned CVE-2010-3131 to this.
*** Bug 591577 has been marked as a duplicate of this bug. ***
*** Bug 593746 has been marked as a duplicate of this bug. ***