286382 - [Windows] Many insecure uses of LoadLibrary (filename without path) (MSVR-10-0102)

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Steve Lord]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1; .NET CLR 1.1.4322)
Build Identifier: Not sure but from firefox-1.0.1-source.tar.bz2

When you load a windows DLL, you use LoadLibrary(). Normally you're looking 
for LoadLibrary("C:\Windows\System32\Thingy.dll"); or rather LoadLibrary
(string) where string is the path (calculated from the registry) with the DLL 
name appended to it. If you just LoadLibrary("foo.dll") with no path, most 
Windows versions check for the existence of foo.dll in the following order:

The directory from which the application loaded Current directory (normally 
c:\program files\mozilla firefox because of the Start in value) The system 
directory The 16-bit name for the system directory The Windows directory The 
directories listed in the PATH environment variable

XP and above uses a registry key, 
HKLM\System\CurrentControlSet\Control\Session Manager\SafeDllSearchMode which 
by default is set to 1. When set to 0, the same order as above is used. The 
default of 1 changes the order to the following:

The directory from which the application loaded The system directory The 16-
bit name for the system directory The Windows directory Current directory 
(normally c:\program files\mozilla firefox because of the Start in value) The 
directories listed in the PATH environment variable

To make things more complex, the SetDllDirectory function in Windows 2003 and 
XP SP1 modifies the search path used to locate DLLs for the application and 
affects all subsequent calls to the LoadLibrary and LoadLibraryEx functions by 
the application.

The impact of this issue is that anyone that can write to the one of the 
potential paths (preferably the directory from which the application loaded), 
say using a Java applet could write a trojan dll, then close the browser using 
Javascript for example. When the user next starts firefox and a situation 
occurs where the DLL is loaded, it'll search through the potential paths, find 
a trojan dll and the system could be compromised with the privileges of the 
user running Firefox.

This affects the following (AFAIK, although I've only tested it on Windows XP 
SP1):

Windows 98
Windows 98SE
Windows ME
Windows NT 4.0 (subject to file permissions)
Windows 2000
Windows XP
Windows XP SP1
Possibly XP SP2
Possibly Windows 2003

I don't know about XP SP2 and Windows 2003 - haven't tried and have heard 
conflicting information regarding DLL search order.


Reproducible: Always

Steps to Reproduce:
1. cd <path to source tree>
1. rgrep -n LoadLibrary ./ | grep \.DLL\" | grep -v PR_LoadLibrary
2. The second to last one specifies a full path starting with Y: and can be 
ignored, also I don't think the last one is used either.
3. Profit!

Actual Results:  
You should get a list of LoadLibrary calls that call DLLs directly, with paths 
and line numbers. These are the affected lines. My results tally up with what 
I've seen in the code and are included here:

./accessible/src/msaa/nsAccessNodeWrap.cpp:539:    gmUserLib =::LoadLibrary
("USER32.DLL");
./accessible/src/msaa/nsAccessibleWrap.cpp:134:    gmAccLib =::LoadLibrary
("OLEACC.DLL");  
./xpcom/base/nsStackFrameWin.cpp:87:    HMODULE module = ::LoadLibrary
("IMAGEHLP.DLL");
./xpcom/io/nsLocalFileWin.cpp:1717:    HINSTANCE hInst = LoadLibrary
("KERNEL32.DLL");
./xpcom/obsolete/nsFileSpecWin.cpp:683:    HINSTANCE hInst = LoadLibrary
("KERNEL32.DLL");
./xpinstall/src/nsInstallPatch.cpp:1183:	    hImageHelp = LoadLibrary
("IMAGEHLP.DLL");
./widget/src/windows/nsWindow.cpp:7096:    gmAccLib =::LoadLibrary
("OLEACC.DLL");  
./mailnews/addrbook/src/nsMapiAddressBook.cpp:68:    HMODULE libraryHandle = 
LoadLibrary("MAPI32.DLL") ;
./mailnews/addrbook/src/nsMapiAddressBook.cpp:75:        libraryHandle = 
LoadLibrary("MAPI32BAK.DLL") ;
./mailnews/import/outlook/src/MapiApi.cpp:150:	HINSTANCE	hInst 
= ::LoadLibrary( "MAPI32.DLL");
./mailnews/import/outlook/src/MapiApi.cpp:156:		hInst = ::LoadLibrary
( "MAPI32BAK.DLL");
./mailnews/mapi/old/tests/mapitest/mapiproc.cpp:177:	m_hInstMapi = 
LoadLibrary("Y:\\ns\\cmd\\winfe\\mapi\\MAPI.DLL");	
./mailnews/mapi/old/tests/mapitest/mapiproc.cpp:179:  m_hInstMapi = LoadLibrary
(".\\COMPONENTS\\MAPI32.DLL");

Expected Results:  
The correct way to use LoadLibrary is to work out the path to the directory of 
the DLL you want (use GetWindowsDirectory() or try getting %SYSTEMROOT% or %
SYSTEMDIRECTORY% environment variables). Given the architecture, probably the 
best way to handle this is to work out the SystemRoot, Windows Directory and 
any other paths you *want* to check, then write your own LoadLibrary Wrapper 
(lets call it PR_LoadLibrary). You then only need to prefix three characters 
to each affected instance of LoadLibrary. The PR_LoadLibrary function would 
basically go through each of the paths you want to check, in the order you 
specify, creating a new string containing the path to check and dll name then 
calling LoadLibrary with the string.

Comment 1

[Daniel Veditz [:dveditz]]

-> Product:Core

If a trojan can write to the application directory the bad guys win. The trojan
could easily install a plugin or XPCOM component to be loaded on next start.
Even if we didn't have an extensible architecture the application itself could
be replaced with a hacked copy.

If the trojan can write to the windows directory, again, it wins.

LoadLibrary() with no path is a special case that first looks for an
already-loaded module of the same name. This is especially appropriate for OS
modules that are running before any user-space programs run.

So the scope of the problem is much, much, smaller than first appears.

In older versions of windows without "SafeDLLSearchMode" the current
directory--if the attacker could somehow get your current directory
changed--maybe the non-loaded system .dll's could be inserted. On newer versions
with "SafeDLLSearchMode" only libraries not found in the system libraries could
be supplied by the current directory or any other spot on the path.

There are places that attempt to LoadLibrary() a system dll introduced in later
versions of windows, with fallbacks for old windows without those libraries.
These are potential targets for this attack, except on these old windows systems
(e.g. Win95 and early Win98) nothing is stopping the attacker from overwriting
the app itself or installing a plugin.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm sorry, but this has nothing to do with File Handling.  That component is
clearly documented to be about helper applications, content type guessing, etc,
NOT about interaction with OS file apis...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that we'll probably want one bug per module that does the wrong thing here,
with this bug being the tracker...

Comment 4

[Steve Lord]

The LoadLibrary bug is in the same class as LD_Preload and IFS style bugs 
under Unix - I agree that it's nothing to do with file handling (it's about 
trusting third-party compenents IMHO but call it anything you like), it's 
still an issue that needs to be fixed. I'd suggest adding something on the use 
of external libraries to the mozilla development guide. If you guys are short 
of time and want me to write something up for you then I'll be happy to do so.

My concern is whether this is a Firefox bug or a Mozilla bug and where else 
these things are found. Has anyone checked the other bits of source knocking 
around, or is this something I should do?

Regards,
Steve

Comment 5

[Steve Lord]

I noticed someone's changed the summary, does this mean that someone else has 
picked up the ball on this?

Comment 6

[(no longer active)]

The risk here is real.  A long time ago I saw a keylogger application which hooked into another application in this exact way.  (And yes, malicious applications can do this in many ways, but this is something which we can protect against, so I think we should.)

Here's the current list:

ehsanakhgari:~/moz/src [10:02:32]$ grep -rn LoadLibrary ./ | grep -i \.DLL\" | grep -v PR_LoadLibrary
./accessible/src/msaa/nsAccessibleWrap.cpp:169:    gmAccLib =::LoadLibraryW(L"OLEACC.DLL");
./accessible/src/msaa/nsAccessNodeWrap.cpp:561:    gmUserLib =::LoadLibraryW(L"USER32.DLL");
./browser/components/migration/src/nsIEProfileMigrator.cpp:934:  HMODULE pstoreDLL = ::LoadLibraryW(L"pstorec.dll");
./browser/components/migration/src/nsIEProfileMigrator.cpp:1257:  HMODULE pstoreDLL = ::LoadLibraryW(L"pstorec.dll");
./dom/src/geolocation/WinMobileLocationProvider.cpp:102:    mGPSInst = LoadLibraryW(L"gpsapi.dll");
./embedding/browser/activex/src/plugin/XPConnect.cpp:380:        HMODULE hlib = ::LoadLibraryW(L"xpcom.dll");
./extensions/auth/nsAuthSSPI.cpp:118:    sspi_lib = LoadLibraryW(L"secur32.dll");
./extensions/auth/nsAuthSSPI.cpp:120:        sspi_lib = LoadLibraryW(L"security.dll");
./extensions/metrics/src/nsLoadCollector.cpp:154:    sPSModule = LoadLibrary("psapi.dll");
./gfx/cairo/cairo/src/cairo-d2d-private.h:134:		GetProcAddress(LoadLibraryW(L"d2d1.dll"), "D2D1CreateFactory");
./gfx/cairo/cairo/src/cairo-d2d-private.h:179:		GetProcAddress(LoadLibraryA("d3d10_1.dll"), "D3D10CreateDevice1");
./gfx/cairo/cairo/src/cairo-dwrite-font.cpp:79:		GetProcAddress(LoadLibraryW(L"d2d1.dll"), "D2D1CreateFactory");
./gfx/cairo/cairo/src/cairo-dwrite-private.h:55:		GetProcAddress(LoadLibraryW(L"dwrite.dll"), "DWriteCreateFactory");
./gfx/layers/d3d9/LayerManagerD3D9.cpp:85:      GetProcAddress(LoadLibraryW(L"d3d9.dll"), "Direct3DCreate9");
./gfx/thebes/src/gfxGDIFontList.cpp:702:    HMODULE fontlib = LoadLibraryW(L"t2embed.dll");
./gfx/thebes/src/gfxWindowsPlatform.cpp:153:            GetProcAddress(LoadLibraryW(L"dwrite.dll"), "DWriteCreateFactory");
./intl/locale/src/windows/nsIWin32LocaleImpl.cpp:615:  sKernelDLL = LoadLibraryW(L"kernel32.dll");
./ipc/chromium/src/base/gfx/native_theme.cc:62:    : theme_dll_(LoadLibrary(L"uxtheme.dll")),
./ipc/chromium/src/base/pe_image_unittest.cc:144:  HMODULE module = LoadLibrary(L"advapi32.dll");
./ipc/chromium/src/base/pe_image_unittest.cc:190:  HMODULE module = LoadLibrary(L"advapi32.dll");
./ipc/chromium/src/third_party/libevent/evdns.c:2807:	if (!(handle = LoadLibrary("iphlpapi.dll"))) {
./modules/lib7z/LZMASDK/CPP/Windows/MemoryLock.cpp:71:  HMODULE hModule = LoadLibrary(TEXT("Advapi32.dll"));
./modules/libpr0n/decoders/icon/win/nsIconChannel.cpp:424:  HMODULE hShellDLL = ::LoadLibraryW(L"shell32.dll");
./netwerk/base/src/nsAutodialWin.cpp:690:        mhRASapi32 = ::LoadLibraryW(L"rasapi32.dll");
./netwerk/base/src/nsAutodialWin.cpp:739:        mhRASdlg = ::LoadLibraryW(L"rasdlg.dll");
./netwerk/system/win32/nsNotifyAddrListener.cpp:81:        sIPHelper = LoadLibraryW(L"iphlpapi.dll");
./netwerk/system/win32/nsNotifyAddrListener.cpp:100:        sNetshell = LoadLibraryW(L"Netshell.dll");
./netwerk/wifi/nsWifiScannerWin.cpp:177:  HINSTANCE hIpDLL = LoadLibraryW(L"Iphlpapi.dll");
./netwerk/wifi/nsWifiScannerWin.cpp:557:    HINSTANCE wlan_library = LoadLibrary("Wlanapi.dll");
./nsprpub/pr/src/md/windows/ntinrval.c:54:    HMODULE mmtimerlib = LoadLibraryW(L"mmtimer.dll");  /* XXX leaked! */
./nsprpub/pr/src/md/windows/w95sock.c:122:        libWinsock2 = LoadLibraryW(L"Ws2_32.dll");
./other-licenses/nsis/Contrib/nsProcess/Source/nsProcess.c:238:    if (hLib=LoadLibraryA("NTDLL.DLL"))
./other-licenses/nsis/Contrib/nsProcess/Source/nsProcess.c:325:    if (hLib=LoadLibraryA("KERNEL32.DLL"))
./security/nss/lib/freebl/win_rand.c:535:    hModule = LoadLibrary("advapi32.dll");
./toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.cpp:100:    gAdvAPIDLLInst = ::LoadLibrary("Advapi32.dll");
./toolkit/crashreporter/client/crashreporter_win.cpp:148:  LoadLibrary(L"riched20.dll");
./toolkit/crashreporter/client/crashreporter_win.cpp:331:  HMODULE themeDLL = LoadLibrary(L"uxtheme.dll");
./toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/minidump_generator.cc:233:    dbghelp_module_ = LoadLibrary(TEXT("dbghelp.dll"));
./toolkit/crashreporter/google-breakpad/src/client/windows/crash_generation/minidump_generator.cc:255:    rpcrt4_module_ = LoadLibrary(TEXT("rpcrt4.dll"));
./toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:248:    dbghelp_module_ = LoadLibrary(L"dbghelp.dll");
./toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:257:    rpcrt4_module_ = LoadLibrary(L"rpcrt4.dll");
./tools/trace-malloc/lib/nsDebugHelpWin32.cpp:73:    HMODULE module = ::LoadLibrary("DBGHELP.DLL");
./tools/trace-malloc/lib/nsDebugHelpWin32.cpp:115:  static DHWImportHooker gLoadLibraryW("Kernel32.dll", "LoadLibraryW",
./tools/trace-malloc/lib/nsDebugHelpWin32.cpp:123:  static DHWImportHooker gLoadLibraryExW("Kernel32.dll", "LoadLibraryExW",
./tools/trace-malloc/lib/nsDebugHelpWin32.cpp:131:  static DHWImportHooker gLoadLibraryA("Kernel32.dll", "LoadLibraryA",
./tools/trace-malloc/lib/nsDebugHelpWin32.cpp:139:  static DHWImportHooker gLoadLibraryExA("Kernel32.dll", "LoadLibraryExA",
./uriloader/exthandler/win/nsMIMEInfoWin.cpp:308:    hDll = ::LoadLibraryW(L"shell32.dll");
./widget/src/windows/nsAccelerometerWin.cpp:93:  HMODULE hSensorLib = LoadLibraryW(L"HTCSensorSDK.dll");
./widget/src/windows/nsAccelerometerWin.cpp:223:  HMODULE hSensorLib = LoadLibraryW(L"SamsungMobileSDK_1.dll");
./widget/src/windows/nsAccelerometerWin.cpp:311:  HMODULE hSensorLib = LoadLibraryW(L"axcon.dll");
./widget/src/windows/nsAccelerometerWin.cpp:413:  mLibrary = LoadLibraryW(L"sensor.dll");
./widget/src/windows/nsLookAndFeel.cpp:82:  gShell32DLLInst = LoadLibraryW(L"Shell32.dll");
./widget/src/windows/nsToolkit.cpp:222:      GetProcAddress(LoadLibraryW(L"user32.dll"), "SetProcessDPIAware");
./widget/src/windows/nsWindow.cpp:4544:        HINSTANCE library = LoadLibrary(L"HTCAPI.dll"); 
./widget/src/windows/nsWindow.cpp:6866:    sAccLib =::LoadLibraryW(L"OLEACC.DLL");
./xpcom/base/nsStackWalk.cpp:334:    HMODULE module = ::LoadLibraryW(L"DBGHELP.DLL");
./xpcom/base/nsStackWalk.cpp:336:        module = ::LoadLibraryW(L"IMAGEHLP.DLL");
./xpcom/io/SpecialSystemDirectory.cpp:131:    gShell32DLLInst = LoadLibraryW(L"shell32.dll");
./xpcom/threads/nsProcessCommon.cpp:485:    HMODULE kernelDLL = ::LoadLibraryW(L"kernel32.dll");

I'll provide patches for each module separately.

Comment 7

[(no longer active)]

Attachment: Part 1: a11y module

Comment 8

[chris hofmann]

It looks like there may be some risk on AMO hosted addons for this as well.

https://mxr.mozilla.org/addons/search?string=LoadLibrary has a lot of false positives and needs some filtering, but looks like there are also a few hits there.   I wonder how we should approach addon developers on this, and if there is a way to add checks for this to the verifier.  does the verifier run on .cpp files or is it more manual checks for those files and any binary components?

Comment 9

[(no longer active)]

Attachment: Part 2: profile migrator

Comment 10

[(no longer active)]

Attachment: Part 1: a11y module

Use the Unicode version of GetSystemDirectory.

Comment 11

[(no longer active)]

Does any of you guys know where gpsapi.dll should be located?  Can I just look for it in the path returned by GetSystemDirectory?

Comment 12

[(no longer active)]

Not bothering with embedding/browser/activex/src/plugin/XPConnect.cpp since we don't really support the activex plugin any more (right?)

Comment 13

[(no longer active)]

Attachment: Part 3: auth extension

Comment 14

[(no longer active)]

Attachment: Part 4: metrics extension

Comment 15

[(no longer active)]

Attachment: Part 5: cairo

Comment 16

[(no longer active)]

(In reply to comment #8)
> It looks like there may be some risk on AMO hosted addons for this as well.

This should be a better list, sans the python files:

<https://mxr.mozilla.org/addons/search?string=LoadLibrary&case=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons>

> https://mxr.mozilla.org/addons/search?string=LoadLibrary has a lot of false
> positives and needs some filtering, but looks like there are also a few hits
> there.   I wonder how we should approach addon developers on this, and if there
> is a way to add checks for this to the verifier.  does the verifier run on .cpp
> files or is it more manual checks for those files and any binary components?

I'm not sure, do we require addons with binary components to submit the source code to their binary components as well as the binary component itself?

Comment 17

[(no longer active)]

Attachment: Part 6: gfx layers

Comment 18

[(no longer active)]

Attachment: Part 7: thebes

Comment 19

[(no longer active)]

More to come tomorrow...

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Can't we just use a wrapper around LoadLibrary instead of doing the path concatenation by hand everywhere?

Comment 21

[Jeff Muizelaar [:jrmuizel]]

I also don't really understand what security problem this fixes. i.e. how do you get the dll into the dll search path?

And how would you deal with third-party libraries like d3d10core.dll calling LoadLibrary without a full path?

Comment 22

[Brendan Eich [:brendan]]

Comment on attachment 453640 [details] [diff] [review]
Part 4: metrics extension

Ducking, bsmedberg is better equipped.

/be

Comment 23

[timeless]

So, the use of MAX_PATH + something bothers me, since it violates the concept of maxpath...

each of these hopefully rarely hit codepaths does some implicit strlens in interesting ways, i wonder if relying on our directoryservice whose nsstrings already know the length would have advantages.

Some asides:
1. some of the code being patched is sketchy... calling GetProcAddress(null, ....) -- this isn't the fault of the patcher...
2. Classically the api pattern for LoadLibrary is that you were supposed to call FreeLibrary when you were done (or if you failed to get anything you wanted...) - as libraries are reference counted Load = +1, Free = -1. I think we might have decided that we don't want to call FreeLibrary (there are security concerns involved in letting go), however if we decided this, we didn't do a great job of informing our code/developers:
http://mxr.mozilla.org/mozilla-central/ident?i=FreeLibrary

Comment 24

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 453636 [details] [diff] [review]
Part 1: a11y module

r=me, looks ok, but I had similar thoughts to comment 20.

(Does GetSystemDirectoryW respect the registry setting for (un)safe loading?)

Comment 25

[(no longer active)]

(In reply to comment #21)
> I also don't really understand what security problem this fixes. i.e. how do
> you get the dll into the dll search path?

You can put it inside the application's directory.  This way you can run arbitrary code in Firefox without needing to invalidate the digital signature of the modules we ship.

> And how would you deal with third-party libraries like d3d10core.dll calling
> LoadLibrary without a full path?

You can't.  This bug is not about full protection against injected code into our address space, because such full protection does not exist.  Consider this as a first level of defense against attackers trying to get us run their code.

Comment 26

[(no longer active)]

(In reply to comment #20)
> Can't we just use a wrapper around LoadLibrary instead of doing the path
> concatenation by hand everywhere?

The usual suspect for such code is NSPR, but looking at the list of places where I need to do this, there are some places (such as cairo code) where I can't use NSPR.

Comment 27

[Jorge Villalobos [:jorgev] (he/him)]

I agree with comment #21. At least from the POV of an add-on, you can already write files anywhere, and it wouldn't be hard for a malicious add-on to replace any binaries and do all kinds of damage. (It would be very difficult for it to pass review on AMO, obviously)

For add-on authors that use LoadLibrary, we can recommend c-types. Can they use PR_LoadLibrary? Is that something that is exposed to binary components in add-ons?

Comment 28

[(no longer active)]

(In reply to comment #23)
> So, the use of MAX_PATH + something bothers me, since it violates the concept
> of maxpath...

Well, exceeding MAX_PATH is not really an issue here, because it cannot happen (otherwise, no application could open any system library unless they prefix the patch with "\\?\"), and worst comes to worst, it can cause LoadLibrary to fail, which the code already handles.

And in fact, not all of the Windows APIs enforce MAX_PATH under the hoods.  I'm not sure if LoadLibrary is one of them though.

> each of these hopefully rarely hit codepaths does some implicit strlens in
> interesting ways, i wonder if relying on our directoryservice whose nsstrings
> already know the length would have advantages.

What do you mean?  I've never used strlen in any of the patches...

And the directory service cannot be used in all of these cases.

> Some asides:
> 1. some of the code being patched is sketchy... calling GetProcAddress(null,
> ....) -- this isn't the fault of the patcher...

While this feels dirty, GetProcAddress returns null if the module handle passed to it is null.

> 2. Classically the api pattern for LoadLibrary is that you were supposed to
> call FreeLibrary when you were done (or if you failed to get anything you
> wanted...) - as libraries are reference counted Load = +1, Free = -1. I think
> we might have decided that we don't want to call FreeLibrary (there are
> security concerns involved in letting go), however if we decided this, we
> didn't do a great job of informing our code/developers:
> http://mxr.mozilla.org/mozilla-central/ident?i=FreeLibrary

I'm not sure what the security considerations of calling FreeLibrary are, but anyways, we can handle those cases in a separate bug.

Comment 29

[(no longer active)]

(In reply to comment #27)
> I agree with comment #21. At least from the POV of an add-on, you can already
> write files anywhere, and it wouldn't be hard for a malicious add-on to replace
> any binaries and do all kinds of damage. (It would be very difficult for it to
> pass review on AMO, obviously)

We are not protecting against add-ons here.

> For add-on authors that use LoadLibrary, we can recommend c-types. Can they use
> PR_LoadLibrary? Is that something that is exposed to binary components in
> add-ons?

They can use PR_LoadLibrary, but I'm not sure what the advantage would be.

We should also make sure that js-ctypes is safe against this type of attacks.

Comment 30

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I thought the attack was related to UNC paths for remote attacks, and that's why it was critical.  If you have to install local software in order to inject things here, then I really don't see why it's critical, or even above :low, in which case there are many more important things that we could be fixing first.

Can someone talk me through the sg:crit labelling, since I clearly do not get it? I thank you in advance for your indulgence!

Comment 31

[(no longer active)]

(In reply to comment #30)
> I thought the attack was related to UNC paths for remote attacks, and that's
> why it was critical.  If you have to install local software in order to inject
> things here, then I really don't see why it's critical, or even above :low, in
> which case there are many more important things that we could be fixing first.

Since most of these DLLs live in the system directory, here are the directories search before they're found:

With SafeDllSearchMode:
* The directory from which the application loaded.

Without SafeDllSearchMode:
* The directory from which the application loaded.
* The current directory.

So, if Firefox itself is being run from a UNC path, the attack DLLs could be loaded from the same UNC path.

> Can someone talk me through the sg:crit labelling, since I clearly do not get
> it? I thank you in advance for your indulgence!

It was marked as sg:crit by Dan Veditz, without any comments in the bug.  I may not be the best person to judge the security rating of bugs, and I definitely don't know the exact reason behind this being marked as sg:crit (I must say that the reason is not obvious.)

Comment 32

[Johnny Stenback (:jst)]

I don't have the details of this attack but I believe Jesse does.

Comment 33

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 453641 [details] [diff] [review]
Part 5: cairo

Here are some examples where libraries we don't control call LoadLibrary:

advapi32.dll -> ntmarta.dll
dxgi.dll -> nvwgf2um.dll
srvcli.dll -> cscapi.dll
gdi32.dll -> mscms.dll

Given this, and the fact that you need write access to the application directory, I don't really see the point of trying to do anything special here.

Comment 34

[(no longer active)]

(In reply to comment #33)
> (From update of attachment 453641 [details] [diff] [review])
> Here are some examples where libraries we don't control call LoadLibrary:
> 
> advapi32.dll -> ntmarta.dll
> dxgi.dll -> nvwgf2um.dll
> srvcli.dll -> cscapi.dll
> gdi32.dll -> mscms.dll

Hmm, for some reason Windows doesn't seem to try to load those DLLs from the application's directory.  I guess Windows should somehow be protecting against this for the DLLs loaded from its own code...

Comment 35

[(no longer active)]

Before I invest any more time in this, we should come to a conclusion on whether or not we want to fix this bug.

Jeff's review comment in comment 33 is actually suggesting that we should not fix this bug.  Even if the concern stated in that comment is real (which my investigation in comment 34 seems to suggest otherwise), I personally think we should fix this bug in order to reduce the attack surface, especially since that it does not have any negative performance downside (since Windows is going to do a lot more work on behalf of us if we don't specify the full path) and negligible code complexity downside (which can be addressed in each module by wrapping the common code in a helper function I guess).

The rest of the patches needed here are mostly repetitive and boring work (similar to the patches I've already submitted), so I don't want to invest time in doing that work if people are going to reject the patches based on the idea that this bug is not worth fixing.

I'll wait for drivers to comment here to express their views on whether we should fix this bug.

Comment 36

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The attack here is related to UNC paths and straightforward social engineering.  A brief discussion with Vlad and dveditz indicates that we can use the DLL blocklisting hook to suppress UNC loads, with an environment variable override for enterprises that have deployed Firefox in such a way that it runs from UNC (and then hopefully block outbound UNC at the firewall to avoid getting hit by this).

I look forward, greatly, to their elucidation of the aforementioned topics, and related discourse.

Comment 37

[Jesse Ruderman]

The attacks involve causing Firefox's "current directory" to change so it is not the application directory.  The researchers found several ways to do this.

I don't think UNC is the only attack vector, and using the DLL blocklisting hook to block this attack seems fragile.

Comment 38

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Fragile in what way?  Transitive loading of DLLs via plugins and similar are just as easy an attack (Flash testing for d2d via speculative LoadLibrary would not be shocking, f.e.) and having a central place to enforce policy seems like exactly what we want to avoid finding in dot release after dot release that we are loading another library.

Let's talk about the other attack vectors, though, and how mitigating each affects the severity of the umbrella issue.  Other than UNC, they require much more involved social engineering, AFAIK, but I would very much welcome more detail!

Comment 39

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I agree with Ehsan that when we're loading system DLLs, it makes sense for us to look only in the system directory. That's not only more robust, but probably also a tiny bit faster: it avoids the search of the application directory when the DLL is present, and avoids the search of lots of directories when the DLL is not present.

But I think we should avoid duplicating this code as much as possible, so we should put a shared (Windows-only) LoadSystemLibrary function somewhere, maybe in XPCOM, that most of our code can use.

One caveat: it's not clear to me whether LoadLibrary with an absolute path will search other directories if the file does not exist at that path. MSDN documentation is confusing and possibly contradictory. Someone should test to make sure that using LoadLibrary with an absolute path actually protects us against searching the current directory.

Comment 40

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

fwiw -- the function that handles a DLL load is here:

http://hg.mozilla.org/mozilla-central/file/800ef4b6087f/toolkit/xre/nsWindowsDllBlocklist.cpp#l71

that's where any UNC etc. blocking should go.

Comment 41

[chris hofmann]

also saw this page on msdn that talks about a "Safe DLL Search Mode"  that is *not* the default setting.  http://msdn.microsoft.com/en-us/library/ms682586%28VS.85%29.aspx   The page may have been recently updated.  Is that something we could use?

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No, that still searches the current directory if the DLL is not present elsewhere.

Comment 43

[(no longer active)]

(In reply to comment #39)
> But I think we should avoid duplicating this code as much as possible, so we
> should put a shared (Windows-only) LoadSystemLibrary function somewhere, maybe
> in XPCOM, that most of our code can use.

I thought about that, but I think any place that we choose for such a function is not going to be usable by some callers (for example cairo) which do not want to depend on any Mozilla libraries.

But I guess we could special case those callers.

> One caveat: it's not clear to me whether LoadLibrary with an absolute path will
> search other directories if the file does not exist at that path. MSDN
> documentation is confusing and possibly contradictory. Someone should test to
> make sure that using LoadLibrary with an absolute path actually protects us
> against searching the current directory.

Windows does not search at all when you pass an absolute path to LoadLibrary.  If the library cannot be found at that exact location, the call will fail.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #43)
> I thought about that, but I think any place that we choose for such a function
> is not going to be usable by some callers (for example cairo) which do not want
> to depend on any Mozilla libraries.

Sure, but we should share as much code as possible. We could have a separate helper function for cairo.

> Windows does not search at all when you pass an absolute path to LoadLibrary. 
> If the library cannot be found at that exact location, the call will fail.

Great.

Comment 45

[(no longer active)]

(In reply to comment #40)
> fwiw -- the function that handles a DLL load is here:
> 
> http://hg.mozilla.org/mozilla-central/file/800ef4b6087f/toolkit/xre/nsWindowsDllBlocklist.cpp#l71
> 
> that's where any UNC etc. blocking should go.

Wait.  Vlad, how about we hook LoadLibraryExW (which is the underlying beast behind all LoadLibrary APIs in kernel32.dll) and implement some kind of logic there?  It's not clear to me what that logic should be right now, but I guess we could have a list of DLLs which are shipped by the system somewhere, and try to only load those DLLs from the system directory?  *Or*, implement our own searching algorithm, by searching in the application directory for our known DLLs, then searching in the system directory, then in the windows directory, and then failing?  (The second solution seems better to me, but I need to think about it a bit more.)

Comment 46

[(no longer active)]

I guess the solution in comment 45 works, except for when we need to load component DLLs and stuff.  Does anyone know where those are loaded from?  And where in the code base?

Comment 47

[Jesse Ruderman]

Does the DLL blacklisting hook run early enough that no malicious code gets to run?  So far we have only used it as a stability measure and not a security measure.

Could we limit UNC loads to only loads that are in the same directory as Firefox itself, instead of making it a pref?  That way, enterprises wouldn't have to rely on firewalls blocking outbound connections and all internal machines being non-malicious.

Comment 48

[(no longer active)]

(In reply to comment #47)
> Does the DLL blacklisting hook run early enough that no malicious code gets to
> run?  So far we have only used it as a stability measure and not a security
> measure.

That code is run from <http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsWindowsWMain.cpp#94>, which is the first statement in wmain.  I _think_ it should be early enough, because by this time the only code run is some CRT code, as well as the global object constructors.  We could also call this function from a global ctor, but then we would basically have no idea when this is executed exactly.

> Could we limit UNC loads to only loads that are in the same directory as
> Firefox itself, instead of making it a pref?  That way, enterprises wouldn't
> have to rely on firewalls blocking outbound connections and all internal
> machines being non-malicious.

Are we assuming that we trust everything in the Firefox directory on a UNC share?

Comment 49

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We would also need to load from the user profile dir, and anywhere specified via registry key as a system-wide add-on.

I think that we should disable UNC loads, and allow an environment variable to permit them (not a pref; environment variables are a lot easier for admins, and don't have bootstrap problems with profiles not having been loaded yet, or mid-execution changes).

As a later item, the env var could contains a semicolon-separated list of permitted UNC paths.

Comment 50

[(no longer active)]

That should be easy to do inside the hook.

Does MOZ_LOAD_UNC_DLLS seem like a good name for the variable?  We can support loading DLLS from all UNC paths if that var is set for now, and later add support for specifying the allowed paths in the value for that variable (and optionally continuing to support the empty value as a blanket while-list).

Does this sound good?

Comment 51

[(no longer active)]

Attachment: Don't load DLLs from the current directory

So, the real risk here is loading DLLs from the current directory.

I was working on a patch which would mimic the Windows logic for loading DLLs and just would skip the current directory, but then I thought that a much simpler and robust method is to make sure we don't ask the OS to load DLLs which exist in the current directory.  This has the advantage that we won't have bugs in simulating the OS behavior, and we won't have bugs if future versions (or service packs) of Windows choose to change the algorithm.

Vlad, what do you think?  Am I insane?

Comment 52

[(no longer active)]

I had a very interesting discussion with jesse on this over IRC.  We think we might have a better solution: saving the current directory before calling ntdll's LdrLoadDll, setting the current directory to the system directory, calling ntdll's LdrLoadDll, and then restoring the current directory (so that we don't break things which possibly rely on the current directory).

I'm pasting the whole chat log here for people to read and comment on:

Jesse: hey, tell me about https://bugzilla.mozilla.org/show_bug.cgi?id=286382#c51
[8:31pm] Jesse: does it allow an attacker to cause firefox to *not* load a dll by choosing the working directory?
[8:32pm] Jesse: and does it break firefox if its current directory is its own directory?
[8:34pm] ehsan: what that patch does is that for dll loads where no path is specified, it checks to see if a dll with the same name exists in that directory
[8:34pm] ehsan: well, the current directory of the process
[8:34pm] ehsan: if such a dll exists, it just pretends that the requested dll cannot be found
[8:34pm] Jesse: hmmm
[8:34pm] Jesse: that means an attacker can deny the load of any dll
[8:34pm] Jesse: i don't like that much
[8:34pm] ehsan: hmm
[8:35pm] ehsan: yeah, I suppose that's possible
[8:35pm] Jesse: and also if the attacker does something weird like first saying the file exists and then later saying it doesn't exist, we lose
[8:35pm] Jesse: or the other way around
[8:35pm] ehsan: what do you mean?
[8:35pm] ehsan: we don't ask the attacker whether that dll exists!
[8:35pm] Jesse: what is the information this function has?
[8:36pm] ehsan: the name of the dll to be loaded, for the most part
[8:36pm] Jesse: just the string passed to LoadLibrary?
[8:37pm] Jesse: windows hasn't yet run its algorithm to find the dll?
[8:37pm] ehsan: hmm
[8:37pm] ehsan: I *think* the name is what is passed to LoadLibrary
[8:37pm] Jesse: how would we get the dll version if windows hadn't already found the file?
[8:38pm] ehsan: we look at the file on disk
[8:38pm] ehsan: that's the only way to figure out the version of any dll
[8:38pm] Jesse: so we must have the final filename after windows searches all the paths
[8:39pm] ehsan: no
[8:39pm] ehsan: we search for the dll using SearchPath
[8:39pm] ehsan: (which is not correct, btw)
[8:39pm] Jesse: oh man
[8:40pm] ehsan: I really hate to try to imitate windows' algorithm for searching to find a dll
[8:40pm] Jesse: i think we should do it the other way around: deny the load if it doesn't exist in one of the places we want it to be
[8:40pm] ehsan: that would be prone to a lot of problems
[8:40pm] Jesse: the order doesn't have to be exactly the same
[8:40pm] ehsan: it does
[8:40pm] Jesse: it does?
[8:40pm] ehsan: yep
[8:41pm] ehsan: because otherwise we might be looking at the wrong file
[8:41pm] ehsan: well
[8:41pm] ehsan: ok
[8:41pm] Jesse: all we care about in this case is whether windows will find a valid dll before it looks in the current directory
[8:41pm] ehsan: so SearchPath is *not* the algorithm that windows uses to load a dll
[8:42pm] ehsan: I'm not even sure why vlad used that
[8:42pm] ehsan: yeah, that's right
[8:42pm] Jesse: for checking the version of the dll we'd like to know exactly which dll will be found
[8:42pm] ehsan: yes, but that's not what we currently do
[8:42pm] ehsan: hmm
[8:43pm] ehsan: I guess our LdrLoadDLL is completely broken in that sense 
[8:43pm] Jesse: lol "Do not use the SearchPath function to retrieve a path to a DLL for a subsequent LoadLibrary call."
[8:43pm] ehsan: I just assumed that maybe there's a reason that vlad used SearchPath there that I may be missing
[8:43pm] Jesse: http://msdn.microsoft.com/en-us/library/ms684175(VS.85).aspx
[8:44pm] ehsan: where do you want me to look at in that page?
[8:45pm] Jesse: i was just saying where i got that quote
[8:45pm] ehsan: ah
[8:45pm] ehsan: yeah
[8:45pm] ehsan:
[8:45pm] ehsan: ms could not be more specific there 
[8:46pm] ehsan: so thinking about this a bit more, I guess we would have to immitate windows' algorithm for searching for a dll 
[8:46pm] ehsan: *maybe* there's an undocumented ntdll api which does that
[8:46pm] Jesse: it's not that painful, is it?
[8:46pm] Jesse: there are 4 places listed on http://msdn.microsoft.com/en-us/library/ms682586(VS.85).aspx
[8:46pm] ehsan: I need to look at the wine source tree to see if there is such a function
[8:46pm] Jesse: before "the current directory"
[8:47pm] Jesse: hehe yay for wine
[8:47pm] ehsan: well, there are at least 4 different algorithms in play there
[8:47pm] ehsan: maybe 5
[8:47pm] ehsan: or more
[8:47pm] ehsan: and windows sdk docs are not always great to specify what really happens
[8:48pm] ehsan: and these algorithms might change in future versions of windows
[8:48pm] ehsan: or service packs
[8:48pm] ehsan: or both 
[8:48pm] Jesse: hopefully!
[8:48pm] ehsan: depending on the nature of the change, I might agree or disagree with that "hopefully" 
[8:49pm] ehsan: my current patch is kind of aggressive
[8:49pm] ehsan: in that it just tries to err on the safe side
[8:49pm] ehsan: but it *is* possible for an attacker to be able to deny loading a dll
[8:50pm] ehsan: this should be (more or less) fine for LoadLibrary calls
[8:50pm] Jesse: i think your patch isn't actually safe
[8:50pm] ehsan: because the call site is supposed to handle the case where the dll is not found
[8:50pm] ehsan: oh, how so?
[8:50pm] Jesse: because an attacker can make our "does this file exist?" call return false and the subsequent "does this file exist?" call done by windows return true
[8:51pm] ehsan: how is he going to do that?
[8:51pm] Jesse: it's a network file system, he can do whatever he wants
[8:51pm] Jesse: unless windows caches aggressively and consistently
[8:51pm] ehsan: but that would require being able to time the calls
[8:52pm] ehsan: or having a debugger attached to firefox.exe 
[8:52pm] ehsan: I wouldn't count on windows' cache for anything!
[8:52pm] Jesse: or just keeping track of the number of calls
[8:52pm] ehsan: hmm
[8:52pm] Jesse: return false for the first call and true for the second call
[8:52pm] ehsan: so I guess a simple improvement could be for us to check if the dll is already loaded
[8:52pm] ehsan: using GetModuleHandle
[8:53pm] ehsan: I'm pretty sure that ntdll.dll does not use PathFileExists 
[8:53pm] ehsan: that's a shell api
[8:53pm] ehsan: I actually don't know how it checks for the file existence
[8:53pm] ehsan: but anyways, remember that we're assuming that the attacker cannot know when function X is called in firefox.exe
[8:54pm] ehsan: if they do, it means that they're already inside our address space
[8:54pm] Jesse: that's a bad assumption
[8:54pm] ehsan: in which case we're screwed no matter what
[8:54pm] ehsan: I don't think that's a bad assumption
[8:54pm] Jesse: or a misleading assumption
[8:54pm] ehsan: why is that?
[8:54pm] Jesse: an attacker can just return answers at random and have a 25% chance of winning
[8:54pm] Jesse: timing channel attacks
[8:55pm] ehsan: hmm
[8:55pm] ehsan: they can do that with deleting and writing the file over and over again, right?
[8:55pm] Jesse: sure. but this is all happening over a network, the attacker's "filesystem" might not even be an actual filesystem on an actual windows machine.
[8:56pm] ehsan: right
[8:56pm] ehsan: but in that case we're already screwed
[8:56pm] Jesse: no
[8:56pm] ehsan: maybe we should just block UNC dll loads no matter what?
[8:57pm] Jesse: we're not "already screwed" if we decided to load a png file from an attacker's fake filesystem
[8:57pm] ehsan:  I'm not talking about png files
[8:57pm] ehsan: I'm talking about the current implementation of LdrLoadDLL hook
[8:57pm] ehsan: they can be playing the same game with module versions, right?
[8:58pm] Jesse: yes
[8:58pm] Jesse: our current hook is not a security measure
[8:58pm] Jesse: so it's not a problem
[8:58pm] ehsan: hmm, I see your point...
[8:59pm] Jesse: i don't like "block UNC dll loads" either, because then you have to add an exception for many corporate networks and you're back to square one
[8:59pm] ehsan: if we have our own search algorithm, that should be prone to the exact same problem as well, right?
[8:59pm] Jesse: i think emulating the windows searching up to the "current directory" point would work
[9:00pm] Jesse: also we should be talking to microsoft if we can
[9:00pm] ehsan: no, it would break %PATH%
[9:00pm] ehsan: yeah, maybe that's what we should do
[9:00pm] Jesse: fuck? do we want %PATH% to work?
[9:00pm] ehsan: I think that's something that many things rely on
[9:00pm] Jesse: ok then we're just screwed
[9:00pm] Jesse: how about changing our working directory
[9:01pm] ehsan: hmm
[9:01pm] ehsan: wair
[9:01pm] ehsan: *wait
[9:01pm] ehsan: how about saving the current directory, set the current directory to the system directory, call windows' LdrLoadDll, and then restore the working directory?
[9:01pm] ehsan: that _should_ work, right?
[9:03pm] Jesse: as long as nothing else tries to change the working directory right then
[9:05pm] ehsan: like what?
[9:06pm] Jesse: ionno, another thread? lemme look at the acros presentation again
[9:06pm] ehsan: I _think_ all of the stuff in that presentation happen on the main thread
[9:10pm] Jesse: yeah looks like it


I don't think that I can write the patch until the summit, but if this solution feels sane, it should be easy to implement, so maybe someone else can pick it up during that time?  If not, I'll resume working on it after I'm back from vacation.

Comment 53

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 455024 [details] [diff] [review]
Don't load DLLs from the current directory


>+          // A DLL with the same name has been found in the current directory.
>+          // This is not safe, because the OS might end up that DLL, and we

we might end up that comment


Looks fine other than that, so I'll r+ it, but you can figure out whether you want to take this or figure out how to write the other patch you discussed above... However, I'm not sure I understand how this manages to not prevent us from loading, say, "xul.dll" from the app dir if you launch firefox from within the app dir (which people often do while developing, e.g. ./firefox.exe -P test).

Comment 54

[Jesse Ruderman]

Ted points out that SetDllDirectory("") removes the current directory from the search path.  That sounds like a solution.

http://msdn.microsoft.com/en-us/library/ms686203%28v=VS.85%29.aspx

Comment 55

[Jesse Ruderman]

Otherwise, I think we'd need to change the current directory, or emulate the search path algorithm and force the OS to load it based on the absolute path we find.

Comment 57

[(no longer active)]

(In reply to comment #54)
> Ted points out that SetDllDirectory("") removes the current directory from the
> search path.  That sounds like a solution.
> 
> http://msdn.microsoft.com/en-us/library/ms686203%28v=VS.85%29.aspx

This would be the ideal solution, except that this function is only available from XP SP1.  This means that if we start using it, it would not be possible to run Firefox on Windows 2000 and XP (without any service packs).

Comment 58

[(no longer active)]

Attachment: CWD guard

This is the patch to implement the current directory switching trick as described in comment 52.  I've submitted a try server build, and I'll verify it to make sure that it fixes the problem before asking for review on this patch.

Comment 59

[Rob Arnold [:robarnold]]

(In reply to comment #57)
> (In reply to comment #54)
> > Ted points out that SetDllDirectory("") removes the current directory from the
> > search path.  That sounds like a solution.
> > 
> > http://msdn.microsoft.com/en-us/library/ms686203%28v=VS.85%29.aspx
> 
> This would be the ideal solution, except that this function is only available
> from XP SP1.  This means that if we start using it, it would not be possible to
> run Firefox on Windows 2000 and XP (without any service packs).

Well, we did just drop Windows 2000/XP SP<2 support so that's ok. Alternatively you can ::LoadLibrary it (absolute path is still safe - right?).

Comment 60

[(no longer active)]

(In reply to comment #59)
> (In reply to comment #57)
> > (In reply to comment #54)
> > > Ted points out that SetDllDirectory("") removes the current directory from the
> > > search path.  That sounds like a solution.
> > > 
> > > http://msdn.microsoft.com/en-us/library/ms686203%28v=VS.85%29.aspx
> > 
> > This would be the ideal solution, except that this function is only available
> > from XP SP1.  This means that if we start using it, it would not be possible to
> > run Firefox on Windows 2000 and XP (without any service packs).
> 
> Well, we did just drop Windows 2000/XP SP<2 support so that's ok.

True, but calling that function using static linking means that the firefox.exe executable cannot even start.

> Alternatively you can ::LoadLibrary it (absolute path is still safe - right?).

Yes, but the other solution just felt simpler!  :-)

Comment 61

[(no longer active)]

Comment on attachment 458640 [details] [diff] [review]
CWD guard

FWIW, this patch didn't cut it.  Some DLLs are loaded off of nsToolkit::Startup which is called by xul.dll's DllMain (way before main is executed).  I have pushed another patch to the try server which I'll test tomorrow, and it all goes well, I'll post it on the bug.

Comment 63

[Justin Dolske [:Dolske]]

Bug 577589 adds the ability to load DLLs via ctypes, eg |ctypes.libraryName("foo")|. Looks to me like this will be made safe by this patch as well?

Comment 64

[(no longer active)]

(In reply to comment #63)
> Bug 577589 adds the ability to load DLLs via ctypes, eg
> |ctypes.libraryName("foo")|. Looks to me like this will be made safe by this
> patch as well?

Yes, once we have a patch that actually works!

Comment 65

[(no longer active)]

Attachment: Patch (v1)

OK, this patch actually works correctly.  I tested this using the test case attached to bug 579593.  The difference against attachment 458640 [details] [diff] [review] is that I initialize the LdrLoadDll hook at the start of xul.dll's DllMain, which is run way before wmain (given the fact that xul.dll is the first DLL in firefox.exe's import table, is the first code run in the program over which we have control).

Comment 66

[Daniel Veditz [:dveditz]]

optimistically hoping we can get this into the next release train.

Comment 67

[(no longer active)]

I have no idea how to port this to 1.9.1, because we don't have our DLL blocklisting infrastructure ported to that branch.

Comment 68

[(no longer active)]

vlad: ping?

Comment 69

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 459195 [details] [diff] [review]
Patch (v1)

Looks fine, but add a comment in patched_LdrLoadDll explaining why the guard is needed and what it does..

Comment 70

[(no longer active)]

Will do.

Vlad, did you also see comment 67?

Comment 71

[(no longer active)]

Attachment: Patch (v1)

With review comment addressed.

Comment 72

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

No idea about 1.9.1 either, though the dll blocklist stuff should drop in mostly as-is.

Comment 73

[(no longer active)]

Hmm, why did we not port the blocklist to 1.9.1 originally?  I guess for the purposes of this bug, we could just port the DLL interceptor code to 1.9.1, and have a patched_LdrLoadDll which only takes care of the current directory.  Would that work?

Comment 74

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/767706ccb547

Comment 75

[Benjamin Smedberg]

This patch is in the regression range for bug 585032, silverlight stopped working, and seems very likely to be related.

Comment 76

[Daniel Veditz [:dveditz]]

Comment on attachment 462602 [details] [diff] [review]
Patch (v1)

Assuming the regression report is correct (and would also happen on the branch) we don't want this patch in 1.9.2 -- some people do actually like Silverlight :-)

We do still very much want the fix so re-request approval (on a new patch?) when the regression is solved.

Comment 77

[Dave Townsend [:mossop]]

Backed out: http://hg.mozilla.org/mozilla-central/rev/d69b051e4097

Comment 78

[(no longer active)]

So, I tested the second most safe approach (calling SetDllDirectory("") at startup), and the silverlight plugin doesn't load either.  So I guess the Silverlight plugin somehow relies on loading DLLs form the current directory (which is unfortunate).

Can we get some contacts at Microsoft about this issue?  The way that things stand right now, I'm not sure if we can do anything further.

Also, this most certainly won't make it for the branch code freeze dates, so I'm undoing the blocker flag for those branches until we have a better idea on what we can do on our end to solve this problem.

Comment 79

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

I'll connect Ehsan with the silverlight team.

Comment 80

[(no longer active)]

(In reply to comment #79)
> I'll connect Ehsan with the silverlight team.

Thanks!

Comment 81

[Reed Loden [:reed]]

(In reply to comment #79)
> I'll connect Ehsan with the silverlight team.

Also, MSVR is happy to connect Windows engineers with any Mozilla developers working on this issue.

Comment 82

[(no longer active)]

(In reply to comment #81)
> (In reply to comment #79)
> > I'll connect Ehsan with the silverlight team.
> 
> Also, MSVR is happy to connect Windows engineers with any Mozilla developers
> working on this issue.

I may need those contacts as well, but let's see what the Silverlight team has to say first.

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are we still running Silverlight in the browser process? If we were running it OOP then presumably we could avoid changing anything in the plugin process.

Comment 84

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

The attack description includes DLLs loaded by plugins, IIRC, but that would be a way to at least protect the other cases.  I believe that Silverlight is out of process in 3.6.4+ by default.

(People make it run in-process so that some of the debugging tools work, though, in which case they would...not have working Silverlight. Hrm.)

Comment 85

[(no longer active)]

And I think we'd want this protection for plugin processes anyways...

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Of course we *want* it, but if we can't have it, we could still protect the browser.

Comment 87

[(no longer active)]

So, I have a version of this patch using SetDllDirectory instead of SetCurrentDirectory.  It does fix the problem with the Silverlight plugin, but the problem is that SetDllDirectory is only available in WinXP SP1 and above.

So, if we land that patch, users running Windows XP or 2k will still remain vulnerable to this type of attack.

Dan, would this be an acceptable fix?

I'm following up with the Silverlight and MSVR teams to see if there is a version of this API which we can use on XP and 2k, but I have a strong feeling that the answer is no, unfortunately.

Comment 88

[Daniel Veditz [:dveditz]]

So our choice is
 1) use SetDllDirectory() and leave folks on WinXP SP1 and less vulnerable
 2) use SetCurrentDirectory() and break Silverlight everywhere?

What about choice 1.5) 
  use SetDllDirectory() where available and
  break Silverlight on < WinXP SP1 by using SetCurrentDirectory() ?

Comment 89

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That sounds like a great choice.  WinXP SP1 should be incredibly widely deployed; does Silverlight even work without SP1?

Comment 90

[Ludovic Hirlimann [:Usul]]

(In reply to comment #89)
> That sounds like a great choice.  WinXP SP1 should be incredibly widely
> deployed; does Silverlight even work without SP1?

http://www.microsoft.com/silverlight/faq/ says that SP2 is required.

Comment 91

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Do we have any reason to believe that Silverlight is the only thing that's broken this way?  We had to back the patch out of trunk after less than 2 days, so unless we've done some analysis I'm not sure I share the confidence.

Microsoft is going to have to give people a structured way to solve this for Windows applications; they just can't expect everyone who builds Windows applications to write their own DLL-loading hook.  Ehsan and I are in discussions with them about what that might be, and how to handle pre-SP1 issues.

Comment 92

[(no longer active)]

We do not have any reason that Silverlight is the only plugin which breaks this way.  In fact, quite the contrary.  The code in question is very old, but digging back through our CVS history, I found out that it was added exactly in order to support plugins which link to other DLLs statically, which is why Silverlight failed in this case.

Comment 94

[timeless]

fwiw, changing CWD is totally unusable, it's an app global thing and thus subject to thread based races. If we actually don't trust random third party modules not to mess with us (AV? Audio? Video? LSP?) then it's not a winning proposition.

We could probably have our DLL loading hook ban the current working directory. We could also try to get the signing work finished (but last I played with that, it broke on w7x64 because ms hadn't signed some of its forwarder dlls...).

Comment 95

[(no longer active)]

(In reply to comment #94)
> We could probably have our DLL loading hook ban the current working directory.

Is that actually possible?  Our hook passes the request to the underlying Windows API...

> We could also try to get the signing work finished (but last I played with
> that, it broke on w7x64 because ms hadn't signed some of its forwarder
> dlls...).

I'm not sure what you mean there.

Comment 96

[(no longer active)]

Damon, we're still waiting to hear from MSVR on the best approach of addressing this issue.  The patches attached to this bug so far are not good enough.  :(

Comment 97

[Daniel Veditz [:dveditz]]

MS Advisory on the issue:
http://www.microsoft.com/technet/security/advisory/2269637.mspx

MS blog post on what to do about it, including links to a binary tool to prevent DLL loads from network locations:
http://blogs.technet.com/b/srd/archive/2010/08/23/more-information-about-dll-preloading-remote-attack-vector.aspx

Comment 98

[(no longer active)]

SetSearchPathMode seems like a good API addition, but unfortunately it's only available on Windows 7 by default (and on XP SP2 and above by applying an update).

Still waiting for the final word from MSVR.

Comment 99

[Johnny Stenback (:jst)]

Are we at a point where we need to poke MS for more info here, or has that already happened?

Comment 100

[chris hofmann]

Vulnerability Note VU#707943
http://www.kb.cert.org/vuls/id/707943

Comment 101

[(no longer active)]

(In reply to comment #99)
> Are we at a point where we need to poke MS for more info here, or has that
> already happened?

I just re-pinged my MS contacts.

Comment 102

[(no longer active)]

I have not heard back from MSVR on this yet, but it appears that there is no other solution except for using SetDllDirectory, which means that we cannot protect our users on WinXP without any SPs or Win2K, but it seems that there's no protection available on those platforms.

I'll attach a patch here which uses SetDllDirectory, and I'll make sure to load it dynamically so that firefox.exe can still be loaded on those platforms.

Comment 103

[Daniel Veditz [:dveditz]]

Ehsan is going to try pinging MS one more time, maybe they figured we solved the problem when we announced we fixed the specific dwmapi.dll case. In the meanwhile we'll proceed with the best approach we can figure out. At this point I don't mind leaving out old versions of windows--those users have other problems!--and relying on the newer SetDllDirectory() assuming it works.

Comment 104

[(no longer active)]

(In reply to comment #103)
> Ehsan is going to try pinging MS one more time, maybe they figured we solved
> the problem when we announced we fixed the specific dwmapi.dll case.

FWIW, I've already sent them another email.

> In the
> meanwhile we'll proceed with the best approach we can figure out. At this point
> I don't mind leaving out old versions of windows--those users have other
> problems!--and relying on the newer SetDllDirectory() assuming it works.

SetDllDirectory seems to work fine with Sillverlight...  I'm going to push to try to see what else needs taking care of, but I'll attach the patch here shortly anyways.

Comment 105

[(no longer active)]

Attachment: Use SetDllDirectory

Try to use SetDllDirectory if it's available.

Comment 106

[(no longer active)]

I've pushed the patch to the try server, and will update this bug with the results.

Comment 107

[(no longer active)]

Try builds available at:

<http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-21058b5fb974/>

Comment 108

[(no longer active)]

Try server runs were successful (sans intermittent oranges).

Comment 109

[(no longer active)]

Juan, in order to test this, you can use the builds available at <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-21058b5fb974/tryserver-win32/>.

This should mostly be tested with as many different plugins (and plugin versions) as possible.  Also, if you know of well-known extensions which include binary components, or the extensions that come with installed software (Skype, for example).

The general point here is to see whether anything that requires us to load a DLL is broken in these builds or not.

It would also be interesting to verify that this doesn't break Firefox in Windows XP without service packs and Windows 2k.

Thanks!

Comment 110

[Benjamin Smedberg]

Comment on attachment 477278 [details] [diff] [review]
Use SetDllDirectory

Per IRC discussion, I don't think we want the nsToolkit.cpp change, just nsWindowsWMain. r=me with that change.

Comment 111

[(no longer active)]

Attachment: Use SetDllDirectory

With comments addressed.

There was an email thread about the possibility of this making into beta7.  If that's desired, it'd be great if one of the drivers can bump the blocking flag on this bug.

Comment 112

[juan becerra [:juanb]]

We are tracking some of the testing here: http://etherpad.mozilla.com:9000/TryServerTesting

After a little testing, I found that playing videos in Netflix doesn't work on the tryserver build in comment #109. You get a "Thanks for Installing. Once installation is complete please restart your browser to watch this movie." Restarting the browser doesn't help.

But it plays the video on the latest trunk nighly. It's a 32bit Win7 machine with Silverlight 4.0.50826.0.

Comment 113

[(no longer active)]

(In reply to comment #112)
> After a little testing, I found that playing videos in Netflix doesn't work on
> the tryserver build in comment #109. You get a "Thanks for Installing. Once
> installation is complete please restart your browser to watch this movie."
> Restarting the browser doesn't help.
> 
> But it plays the video on the latest trunk nighly. It's a 32bit Win7 machine
> with Silverlight 4.0.50826.0.

Strange, because I explicitly tested Silverlight with this patch.  Do you see the same problem on other Silverlight-based sites as well?

Comment 114

[juan becerra [:juanb]]

This is happening with all the sites I tried, including www.bing.com/videos/browse www.nbcolympics.com memorabilia.hardrock.com and others.

Comment 115

[(no longer active)]

OK, in that case, I don't think that we can take this patch.  :(

Comment 116

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should figure out why it works for you and not Juan before we give up on this patch.

Comment 117

[(no longer active)]

Juan, can you please test http://silverlight.net/ and see if you can load the Silverlight animation using this build?

Comment 118

[(no longer active)]

Huh, OK, I was testing with my own locl build, which was loading the Silverlight plugin just fine.  Then, I tried the try server build and saw that it doesn't work.  I'm now trying to figure out why.

Comment 119

[(no longer active)]

Looking at the mozconfig used for try server opt builds <http://hg.mozilla.org/build/buildbot-configs/file/a296a19b5bfd/mozilla2/win32/tryserver/nightly/mozconfig>, I think the only real differences between these and my local build is either jemalloc or PGO.  That's scary...  Continuing to investigate.

Comment 120

[(no longer active)]

So, I did a local PGO build, and the Silverlight plugin was still working in that build.  Can someone help me with doing a build with this patch which only includes --enable-jemalloc in mozconfig?

Comment 121

[(no longer active)]

OK, I finally made some progress here.

I pushed a patch to the try server <http://hg.mozilla.org/try/rev/ceafaeca7e5e> to build Firefox with jemalloc disabled.  With this build, which is available here <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-ceafaeca7e5e/tryserver-win32/>, Silverlight seems to work.  So, jemalloc is coming into play for whatever reason.

I'll try to get my hands on a system which can build with jemalloc enabled tomorrow, in order to try to debug this problem.

Comment 122

[(no longer active)]

I did a local build with --enable-jemalloc, and the Silverlight plugin still works!  I'm really puzzled at this point, and I don't have a clear plan how to proceed.

Comment 123

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Try disabling PGO on the tryserver and see if that affects the problem?

Make sure you're using the same Visual Studio version etc as try server?

Maybe use filemon or something like, plus a debugger, it to monitor the failing Windows build and see how far it gets through the Silverlight startup process, to try to figure out how far it gets and where it fails?

Comment 124

[(no longer active)]

(In reply to comment #123)
> Try disabling PGO on the tryserver and see if that affects the problem?

I don't think that's possible, since the try server uses the make profiledbuild instead of make build to build PGO, and IIRC that's hardcoded and cannot be changed in mozconfigs.

Also, I did a local PGO build with jemalloc enabled, and Silverlight worked again.

> Make sure you're using the same Visual Studio version etc as try server?

I'm using 15.00.30729.01, and the try server build is using 14.00.50727.762 which I assume is VC8SP1.  I need to install that version of Visual Studio locally and give things a try using that version.

> Maybe use filemon or something like, plus a debugger, it to monitor the failing
> Windows build and see how far it gets through the Silverlight startup process,
> to try to figure out how far it gets and where it fails?

I'll give it a shot.  It seems that plugin-container is crashing.  I would have a much easier time if I could access the symbols for the try server build, but it seems that's not working.  I'll update here when I have more information.

Comment 125

[Johnny Stenback (:jst)]

(In reply to comment #124)
> (In reply to comment #123)
> > Try disabling PGO on the tryserver and see if that affects the problem?
> 
> I don't think that's possible, since the try server uses the make profiledbuild
> instead of make build to build PGO, and IIRC that's hardcoded and cannot be
> changed in mozconfigs.

Could you not simply find where in the build code it is hardcoded and change it there in a patch that you push to try?

Comment 126

[(no longer active)]

There is no way that this is going to make tomorrow's code freeze.  It needs quite a bit of baking time on nightlies.  Re-setting the blocking flags in the hope to get blocking for the next dot release.

Comment 127

[(no longer active)]

(In reply to comment #125)
> (In reply to comment #124)
> > (In reply to comment #123)
> > > Try disabling PGO on the tryserver and see if that affects the problem?
> > 
> > I don't think that's possible, since the try server uses the make profiledbuild
> > instead of make build to build PGO, and IIRC that's hardcoded and cannot be
> > changed in mozconfigs.
> 
> Could you not simply find where in the build code it is hardcoded and change it
> there in a patch that you push to try?

By hardcoded, I meant being hardcoded in the automation tools (i.e., they execute make profiledbuilds instead of make build, and there is nothing in mozilla-central which we can modifty to change that behavior.)

I'm CCing some releng people though to see if they know of any way to do non-PGO try opt builds.

Comment 128

[Armen [:armenzg]]

(In reply to comment #127)
> 
> By hardcoded, I meant being hardcoded in the automation tools (i.e., they
> execute make profiledbuilds instead of make build, and there is nothing in
> mozilla-central which we can modifty to change that behavior.)
>
For win32 it is hardcoded as Ehsan mentions:
http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#662
Developers don't have a way of triggering a normal build on the try server AFAIK.

Comment 129

[(no longer active)]

(In reply to comment #128)
> (In reply to comment #127)
> > 
> > By hardcoded, I meant being hardcoded in the automation tools (i.e., they
> > execute make profiledbuilds instead of make build, and there is nothing in
> > mozilla-central which we can modifty to change that behavior.)
> >
> For win32 it is hardcoded as Ehsan mentions:
> http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#662
> Developers don't have a way of triggering a normal build on the try server
> AFAIK.

Can releng do that manually (i.e. by remotely connecting to the build machines, edit factory.py and start a build?

Comment 130

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Submit with a patch that changes "make profiledbuild" just do the "make build" routine.  Should be a 9-character change, I believe.

Comment 131

[Armen [:armenzg]]

After a chat with Ehsan I will be providing him his build without PGO from one of the try slaves.

Comment 132

[Armen [:armenzg]]

(In reply to comment #129)
> 
> Can releng do that manually (i.e. by remotely connecting to the build machines,
> edit factory.py and start a build?
(In reply to comment #130)
> Submit with a patch that changes "make profiledbuild" just do the "make build"
> routine.  Should be a 9-character change, I believe.

FTR We can't really do the change locally and the change on the master would make *all* try builds to be non-PGO.

Comment 133

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

We should file a bug on this command-line switch not working any more, then:

dnl Provide a switch to disable PGO even when called via profiledbuild.
MOZ_ARG_DISABLE_BOOL(profile-guided-optimization,
[  --disable-profile-guided-optimization
                          Don't build with PGO even if called via make profiledbuild],
MOZ_PROFILE_GUIDED_OPTIMIZE_DISABLE=1,
MOZ_PROFILE_GUIDED_OPTIMIZE_DISABLE=)

This is the diff I was proposing that Ehsan use when he pushed:

diff --git a/client.mk b/client.mk
--- a/client.mk
+++ b/client.mk
@@ -210,5 +210,5 @@ else
 endif

-profiledbuild::
+Xprofiledbuild::
        $(MAKE) -f $(TOPSRCDIR)/client.mk build MOZ_PROFILE_GENERATE=1
        $(MAKE) -C $(PGO_OBJDIR) package
@@ -352,5 +352,5 @@ endif
 # Build it

-build::  $(OBJDIR)/Makefile $(OBJDIR)/config.status
+profiledbuild::  $(OBJDIR)/Makefile $(OBJDIR)/config.status
        $(MOZ_MAKE)

Would that not work?

Comment 134

[(no longer active)]

I'm positive that it would, actually.  I'll give it a try.

Comment 135

[Armen [:armenzg]]

Such a cheater! :P

That should fool our systems.

Comment 136

[(no longer active)]

Pushed: http://hg.mozilla.org/try/rev/c6870be9ecaf

Comment 137

[(no longer active)]

So, the try server build crashes in plugin-container here: <http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginModuleChild.cpp#220>, as it fails to null check mLibrary (or check the return value of LoadPlugin on line 196.  Next, I'll try to determine why this doesn't happen in my local builds.

Comment 138

[(no longer active)]

The non-pgo try server build also crashes the same way as comment 137...

Comment 139

[(no longer active)]

So, I found out that if I run firefox from the command line in my local build (using objdir/dist/bin/firefox.exe), it can successfully load the Silverlight plugin.  If I run it from Explorer, it crashes in the same way.  The same thing happens with try server builds: if I run them from the command line, they can load the Silverlight plugin successfully.  Otherwise, they crash.

Here's a theory.  I think whatever SetDllDirectory does behind the scenes might be inheriting to child processes in some specific situations.  We actually don't call SetDllDirectory in the child process at all with my patch (since plugin-container.exe uses a different main function), but the "DLL directory bit" might be inherited in the child process only if Firefox is executed from Explorer.

I have a plan to fix this.  We will call SetDllDirectory("") in plugin-container's main like we do for firefox.exe's.  When we actually try to load the plugin in the child process, we temporarily call SetDllDirecory(NULL) before calling LoadLibrary, and call SetDllDirectory("") when it returns.

I think this is safe, because as far as I can tell, by the time that we try to load the plugin DLL, there are no thread race conditions to worry about, so I'm kind of positive that this will work.  I'll create a patch and give it a shot locally.

Comment 140

[(no longer active)]

Attachment: Correctly handle plugin processes

This patch implements my idea in comment 139, and it works!

Comment 141

[Benjamin Smedberg]

Comment on attachment 479070 [details] [diff] [review]
Correctly handle plugin processes

damn, I hate plugins

Comment 142

[(no longer active)]

I'm planning to land this after the tree opens up to post-b7 blockers.

Comment 143

[Marcia Knous [:marcia]]

Ehsan: Would it be possible to get another tryserver build to test (since in Comment 137 you mentioned it was crashing in plugin container)?

Comment 144

[(no longer active)]

(In reply to comment #143)
> Ehsan: Would it be possible to get another tryserver build to test (since in
> Comment 137 you mentioned it was crashing in plugin container)?

Sure.  The build should appear here after a few hours: <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-b6b050a2d22a>

Comment 145

[(no longer active)]

The try server build is now available at <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-b6b050a2d22a/tryserver-win32/>.

Comment 146

[(no longer active)]

Marcia, Juan, did you get a chance to test the latest build?  Did you find any other problems with it?

Comment 147

[Marcia Knous [:marcia]]

Ehsan: Will test this today.

(In reply to comment #146)
> Marcia, Juan, did you get a chance to test the latest build?  Did you find any
> other problems with it?

Comment 148

[Marcia Knous [:marcia]]

Ehsan: Using the tryserver build in Comment 145, I am having some problems with the justin.tv site on Win XP and Win 7. For example,

1. Load http://www.justin.tv/hotbloodedgaming?utm_source=front_page&utm_medium=channel&utm_campaign=fp_promo_clicks#/w/444782448
2. The browser freezes and I have to force quite plugin-container.exe and then eventually force quit FF alltogether.
3. I get an error in Visual Studio "Unhandled exception at 0x7c90100b in plugin-container.exe: 0xC0000005: Access violation reading location 0x00000030"

I tried the same Justin TV link in the latest Minefield nightly and it loads fine.I am using Shockwave Flash 10.2 d161 which is the version of labs.adobe.com.

My initial pass at loading a few Silverlight sites was okay on this XP hardware, but on my Windows XP VM when I loaded the Silverlight showcase everything froze.

Juan said he was going to run this tryserver build and report back as well.

Comment 149

[(no longer active)]

Is this also a problem with the old try server build in this bug?  I'm interested to know if this is a regression from the changes that I made to the patch in this bug or not.

Comment 150

[juan becerra [:juanb]]

Older build from comment #109: justin.tv example (Flash) freezes the browser; Netflix not working.

Newer build from comment #145: justin.tv example also freezes browser; Netflix works now.

Latest nightly: justin.tv example works, as well as Netflix

Comment 151

[(no longer active)]

Juan, do you have the same version of Flash as Marcia?  Anyways, I'll investigate this more closely tomorrow when I'm at the office.

Comment 152

[juan becerra [:juanb]]

I was using Flash 10.1.85.3 (latest version listed http://www.adobe.com/software/flash/about/), which is different from the version Marcia was using.

Comment 153

[(no longer active)]

I can't reproduce the justin.tv problem on Windows 7 using either the try server build or my own local debug build.  I have Flash 10.1.85.3 installed, the same as Juan.

Juan, Marcia, can you share more information on how you encounter this problem?  Is it on a clean profile?  Can you take a screencast of exactly what you're doing please?  Thanks!

Comment 154

[(no longer active)]

Here's something else that would help me here as well.  Keep Process Monitor running, open Justin TV, let it run for a while and then close it.  Then inside Process Monitor, filter for plugin-container.exe events with Load Image as the value for the Operation column.  Here's what I get on the try server build: http://pastebin.mozilla.org/808283.  It would be interesting for me to compare a log coming from a nightly build against one coming from the try server build...

Comment 155

[juan becerra [:juanb]]

Using tryserverbuild from comment #145:
Screencast reproducing the problem: http://screencast.com/t/rjUIrfmC6R
And procmon log: http://pastebin.mozilla.org/808363

Comment 156

[(no longer active)]

Marcia, can you please try something?  In the try server build, disable all plugins except for Flash and try justin.tv again and see what happens?  If the hang occurs again, can you also pastebin a procmon log, but this time without Load Image filtered?  (ie, all messages from plugin-container)

Comment 157

[juan becerra [:juanb]]

Attachment: all plugin-container messages

All plugin-container messages, only Flash enabled in about:addons

Comment 158

[juan becerra [:juanb]]

Attachment: all plugin-container messages, complete

Earlier attachment was not complete. This one should be.

Comment 159

[(no longer active)]

I *just* managed to reproduce this locally!  More to come soon, hopefully.

Comment 160

[(no longer active)]

This is plugin-container's stack:

http://pastebin.mozilla.org/808577

And firefox.exe is spending 100% of the CPU cycles in what appears to be methodjit code:

http://pastebin.mozilla.org/808582

I tried to get the stacks several times, but it was different every time, but always in methodjit code.  We _seem_ to be in an infinite loop somewhere inside FinishExcessFrames (as its the lowest function on the stack that I can get to).  sayrer, do you have any ideas why this could be happening?

Comment 161

[(no longer active)]

The methodjit stuff made me think that maybe it's something which has been broken on m-c when I pushed that try server patch before, and has been fixed since then.  So I decided to give another try server build of mine a try.  This build <http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-a16f6190966c/tryserver-win32/> is based on a much more recent revision of mozilla-central, and includes the patches on this bug plus a bunch of other non-related patches.  Using this build I could not reproduce the problem.

Juan, Marcia, could you please try to see if you can reproduce this problem using this new try server build?

Comment 162

[(no longer active)]

Huh!  I might be on to something in comment 161!

I get the exact same problem on 2010-09-28's nightly as well: <http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2010-09-28-04-mozilla-central/>

Juan, Marcia, can you please confirm?  If this problem can't be reproduced with the try server build in comment 161, then this patch is ready to land as far as I can tell.

Comment 163

[juan becerra [:juanb]]

I am not able to reproduce the problem with the tryserver builds in comment #161, on the same environment.

Comment 164

[(no longer active)]

(In reply to comment #163)
> I am not able to reproduce the problem with the tryserver builds in comment
> #161, on the same environment.

Great!  Marcia, could you please confirm this as well?

Comment 165

[Marcia Knous [:marcia]]

the justin.tv site plays fine using the Build in Comment 161 on Win XP.

The only issue I would like to compare a bit more is the Silverlight performance issue that I see on this XP hardware, which is a bit older - it seems on the tryserver builds that Silverlight is not as snappy to load and sometimes hangs a bit. I know there is a known issue with the Silverlight Showcase site having a slow script dialog, but the performance on the latest nightly trunk seems a bit better than the tryserver. 


(In reply to comment #164)
> (In reply to comment #163)
> > I am not able to reproduce the problem with the tryserver builds in comment
> > #161, on the same environment.
> 
> Great!  Marcia, could you please confirm this as well?

Comment 166

[(no longer active)]

(In reply to comment #165)
> the justin.tv site plays fine using the Build in Comment 161 on Win XP.
> 
> The only issue I would like to compare a bit more is the Silverlight
> performance issue that I see on this XP hardware, which is a bit older - it
> seems on the tryserver builds that Silverlight is not as snappy to load and
> sometimes hangs a bit. I know there is a known issue with the Silverlight
> Showcase site having a slow script dialog, but the performance on the latest
> nightly trunk seems a bit better than the tryserver. 

OK.  I would seriously problem any performance change with this patch (as we only change the way that DLLs are loaded by the OS) but I'll wait for more test results from you before proceeding.  Thanks!

Comment 167

[Marcia Knous [:marcia]]

Ehsan: I checked on another Win XP laptop with the same version of Silverlight, and I am not seeing the issue there. I loaded several demos from the Silverlight showcase and everything seems okay. justin.tv also plays fine on that machine as well.

Comment 168

[(no longer active)]

Great!  In that case, this is *finally* ready to land!  :-)

Comment 169

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6f304a4dfd4d

bsmedberg: I'm really interested in the following days to see if you see any regressions on loading plugins in Core::Plugins...

Comment 170

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/240c3df5a779

Comment 171

[(no longer active)]

I backed out the patch from 1.9.2 for now until we find a solution for bug 603679:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/757565bc3924

Comment 172

[Daniel Veditz [:dveditz]]

We still want this on branches, but I guess we need it to work on trunk first

Comment 173

[LegNeato]

Comment on attachment 479070 [details] [diff] [review]
Correctly handle plugin processes

I think we will pass on this for branches until it has baked/shipped in a FF4.0
beta. Rescinding approvals and setting blocking flags appropriately.

Comment 174

[(no longer active)]

One challenge here is how to address this on 1.9.1, without causing problems such as bug 603679.  Since we don't have OPP on that branch, we can't use the trick in bug 603679 comment 42.

Comment 175

[Daniel Veditz [:dveditz]]

Realistically we're not going to resolve the plugin issues safely in time to help the 1.9.1 branch -- marking "wontfix". For the 1.9.2 branch we still want this, but since we're still finding new broken plugins on trunk we should continue to wait and watch.

(In reply to comment #174)
> One challenge here is how to address this on 1.9.1, without causing problems
> such as bug 603679.  Since we don't have OPP on that branch, we can't use the
> trick in bug 603679 comment 42.

Does the plugin involved have be out of process, or is it enough to have OOPP code? Shockwave still runs in-process on the 1.9.2 branch so do we have the same problem?

Comment 176

[(no longer active)]

(In reply to comment #175)
> Realistically we're not going to resolve the plugin issues safely in time to
> help the 1.9.1 branch -- marking "wontfix". For the 1.9.2 branch we still want
> this, but since we're still finding new broken plugins on trunk we should
> continue to wait and watch.
> 
> (In reply to comment #174)
> > One challenge here is how to address this on 1.9.1, without causing problems
> > such as bug 603679.  Since we don't have OPP on that branch, we can't use the
> > trick in bug 603679 comment 42.
> 
> Does the plugin involved have be out of process, or is it enough to have OOPP
> code? Shockwave still runs in-process on the 1.9.2 branch so do we have the
> same problem?

So the current situation is that for *OOP* plugins (such as shockwave on trunk) we can whitelist them if they prove to be broken, the same as we did for shockwave on trunk.  For in-process plugins (such as shockwave on 1.9.2), there is no solution that I'm aware of right now, so those plugins will be broken until a fix has been shipped by plugin vendors.

Here's what should happen if we decide to take this fix for 1.9.2.  For shockwave, we can make it OOP, but we have to get QA testing to make sure that nothing is broken with it living out of process.  We should also get QA testing on basically all of the major plugins on a 1.9.2 build containing this patch, and act on any breakage on a case by case basis.

We should also consider the possibility of there being some plugins that are not installed in our QA lab computers, which might break on the next 1.9.2.x release with this patch, and we should consider whether the benefits of this security fix outweigh the risk of breaking such plugins.

What are your thoughts?