Menu ▾ ▴

#1125 Code execution / Privilege escalation problems with NSIS installers

2.0 Series
closed-accepted
nobody
None
5
2016-10-10
2015-11-24
A. Heinecke
No

Hi,

We at gpg4win.org are happily using NSIS to create our Installers for over a
decade now. So first let me thank you for all the work providing and
maintaining NSIS.

It was recently brought to our attention that the Gpg4win installer could be
used to execute arbitrary code with Administrative privileges when an attacker
has control over the Folder in which the installer is placed or can modify the
temp directory. By either placing system DLL's like shfolder.dll or version.dll into the same directory as the installer or by modifing temporary extracted plugins an attacker could cause code to be executed. (And for Elevated installers this could also be used as a privilege escaltion attack). I've outlined the problems causing this below. As they are somewhat related I've decided not to split them up in different bugreports.

While this may be a general problem there are ways to prevent this and we would like to fix it as we see it a serious problem of our installer.

Attached patches try to mitigate this (in the patches.zip file as sourceforge only lets me attach one file). The patches are against 2.46 as provided by Debian GNU/Linux (as we needed fixes for this version and I'll also report a Bug with debian about this) but I'll port them to trunk if you want me to.
I just wanted to get opinions on our analysis and patches first. Patches tested on debian wheezy and debian jessie compiling NISIS with mingw. Minimum supported version for the code used is Windows XP (which I hope is ok)

We saw three problems:

Problem 1:
Use of LoadLibrary in NSIS. LoadLibrary by default looks in the current
directory of the process and into the Folder of the Executable. This is bad
(especially in the regular case that users execute our installer from the
downloads folder) because an attacker can just drop a dll in that folder and
it will be loaded with high privileges. To try it out just place a fake
shfolder.dll (That brings up a message box in it's DllMain) in the same
directory as an NSIS installer. This is also a Problem for LoadLibraryEx for
plugins because their dependencies are also loaded from the executables
directory first.

Solution: Using SetDefaultDllDirectories we can globally change the behavior
of LoadLibraryEx so that it only looks into the System32 folder and into any
directories explicitly added with AddDllDirectories.

While this is only available on Windows Vista and later with updates installed
I think that it is Ok to use for NSIS as it basically means that we won't
lower the security on maintained Windows Systems and privilege escalation is
not that relevant on Windows XP as it does not have the User Account Control.

Patch: 1_restrict_dll_directories.patch

Problem 2:
Plugins are extracted in the temporary folder and can be overwritten without
elevated privileges and the uninstaller is copied into a temporary directory
before it is executed. This also gives an attacker the chance to modify an
uninstaller.

Solution:
If the NSIS installer runs with elevated privileges create the temporary
directory in a way that only elevated users can write into it.

Patch: 2_secure_temp_directory.patch

Problem 3:
Even without delayload / direct loading Windows prefers DLL's from the
executables directory if they are not in the KnownDLL's [2] list. For NSIS
this is problematic for Version.dll

Solution:
No implicit linking against Version.dll instead we use wrapper functions that
resolve the dependency at runtime after we restricted the DLL Search order as
outlined for Problem 1.

Patch: 3_do_not_link_version_dll.patch

With these three patches I was no longer able to modify the Created Temporary
directory as a normal user, and I could execute the installer from a folder
where all libraries names from a Windows 7 System32 pointed to "fake"
libraries.

Uff, Long Text ;-)

Thanks for reading,
Andre

1: https://msdn.microsoft.com/en-us/library/windows/desktop/hh310515%28v=vs.85%29.aspx

2: http://blogs.msdn.com/b/larryosterman/archive/2004/07/19/187752.aspx

1 Attachments
patches.zip

Discussion

1 2 > >> (Page 1 of 2)

  • Anders

    Anders - 2015-11-24

    The way we load SHFOLDER has already been changed in trunk, expect a new release in a couple of weeks.

    We will take a look at your patch ASAP and decide what needs to be done...

     

  • A. Heinecke

    A. Heinecke - 2015-11-24

    Thanks for your quick reply.

    Btw. for testing I've just compiled the following code snippet as a dll with:
    i686-w64-mingw32-gcc -shared attacker.c -o attacker.dll

    And created a test directory with all DLL Names from system32 replaced by that dll. This gave me either the message box or an error that symbols could not be found in the dll when I executed an NSIS installer from that directory.

    Code for the attacker.c:

    #include <windows.h>

int WINAPI
DllMain (HINSTANCE hinst, DWORD reason, LPVOID reserved)
{
  MessageBox (NULL, "Vulnerable against current directory attack",
              "",MB_ICONINFORMATION|MB_OK);
}

    Batch code to create the testdirectory:

    mkdir c:\testdir
FORFILES /p %SystemRoot%\system32 /m *.dll /c "cmd /c copy <path to>\attacker.dll c:\testdir\@file"
     

  • Anders

    Anders - 2015-11-25

    I made version.dll delay-loaded now. (Version.dll used to be on the knowndlls list so I hope you are not suggesting that every API outside kernel32 needs to use GetProcAddress to be future-proof?)

    SetDefaultDllDirectories was already on our mind but I'll try to speed up the testing required to figure out if it breaks anything.

    Your secure temp directory patch might be problematic. What about installers running as SYSTEM? Our minumum supported OS here would be NT4 so you would have to use the older security APIs as well.

     

  • A. Heinecke

    A. Heinecke - 2015-11-25

    I made version.dll delay-loaded now. (Version.dll used to
    be on the knowndlls list so I hope you are not suggesting that
    every API outside kernel32 needs to use GetProcAddress to be future-proof?)

    I did some more research and Windows apparently treats known dll's as a performance thing ( https://technet.microsoft.com/en-us/magazine/2007.09.windowsconfidential.aspx )
    and changes them regularly so I guess one can't really rely on Known DLL's. The article
    mentioned also mentions that version.dll was removed from Known DLL's in Vista.

    There appears to be no real solution for this, so I guess testing / updating
    and avoiding directly linking unknonwn DLLs is the only option. I find it also
    disturbing that I can't find a complete list on "Safe" Dlls. As the dependencies
    of Known DLLs are apparently also treated as Known DLLs. sigh

    SetDefaultDllDirectories was already on our mind but I'll try to speed
    up the testing required to figure out if it breaks anything.

    Great. This already mitigates the most serious problems created by
    attacking dll's in someones Download folder.

    Your secure temp directory patch might be problematic. What about
    nstallers running as SYSTEM?

    I think this should work. My undestanding is that SYSTEM can always do things even if its not part of the ACL directly. Have to test it though to be sure.

    Our minumum supported OS here would be NT4 so you would have to use the older
    security APIs as well.

    I was a bit concerned about that. Can you point me in the right direction
    of what API to use? I don't know what would work both on Windows Vista and Later and
    on NT 4. It's also hard to find out with MSDN as they appear to use "Windows XP"
    as the minimum Supported version for everything. (Like CreateDirectory
    https://msdn.microsoft.com/en-us/library/windows/desktop/aa363855%28v=vs.85%29.aspx )
    Surely this works on NT4 even if MSDN says otherwise? I mean the code used it before..

    Alternatively we could do the GetProcAddress dance for the functions used in IsElevated
    and CreateRestrictedDirectory. Would that be ok?

    An alternative approach to fix this problem could be to keep the handle for plugins and the uninstaller open after writing / copying so that no one can modify them. But I decided against this as it was a more invasive change as DLL's are just handled as normal files currently and the Uninstaller is only copied with CopyFile.

     

    • Anders

      Anders - 2015-11-25

      You cannot trust version information on MSDN. A lot of the security functions can be looked up @ http://wayback.archive.org/web/20021213123126/http://msdn.microsoft.com/library/en-us/security/security/authorization_functions.asp?FRAME=true#basic_access_control_functions but just by looking at the patch I suspect that TRUSTEE_IS_WELL_KNOWN_GROUP is not supported on NT4 and maybe not on 2000 but as long as we don't change the ACLs on these old systems it should be OK. The functions themselves probably have to be delay loaded because I'm guessing they don't exist on Win95, I can take a look tomorrow.

      My main concern is that SYSTEM needs explicit access but it is easy enough to add one more ACE to the ACL after verifying that it actually fails if you don't do it.

      Another problem is that locking down the final directory for the uninstaller might break non-admin uninstallers so we probably need to use a different directory name when elevated.

      Using the TokenElevation class to check if you are elevated is technically broken and you really should check if the integrity level is >= 0x3000 but that is not really a problem here. (Even the Administrator prefix on console windows gets this wrong but it is such a corner case that you are not going to run into it by accident)

       

      Last edit: Anders 2015-11-25

  • A. Heinecke

    A. Heinecke - 2015-11-26

    You cannot trust version information on MSDN.
    ...
    The functions themselves probably have to be delay loaded because I'm guessing they > don't exist on Win95, I can take a look tomorrow.

    Thanks for the pointer to wayback archive. But it would be great if you could do that I don't have experience developing for Windows Versions < XP and don't have a test setup for it.

    My main concern is that SYSTEM needs explicit access but it is easy enough to add one > more ACE to the ACL after verifying that it actually fails if you don't do it.

    I've tested it. Running gpg4win-2.3.0 installer (which was was built with an nsis with the patches here) as System works. The temp folder is created in c:\windows\temp and is writable by the system account even if the ACL only gives access to administrators. See attached screenshot.

    Another problem is that locking down the final directory for the uninstaller might break > non-admin uninstallers so we probably need to use a different directory name when
    elevated.

    I don't understand that. If the uninstaller is non-admin the IsElevated check should fail and no locking should be done.

    Using the TokenElevation class to check if you are elevated is technically broken and
    you really should check if the integrity level is >= 0x3000 but that is not really a
    problem here. (Even the Administrator prefix on console windows gets this wrong but it
    is such a corner case that you are not going to run into it by accident)

    The only problem I had with it in the past was that if I tried to drop privileges through SAFER API the token was still reported as elevated even if it did not have the integrity level for it. I did not think that was a problem for this usecase.
    But yes might be and more compatibility is a good thing. In the past I've checked first for
    TokenElevation and additionally for the integrity_level to determine if we are really running elevated.
    Wout a check for only the integrity_level be enough?

     
    Thumbnail
    install-as-system.png

    • Anders

      Anders - 2015-11-26

      I don't understand that. If the uninstaller is non-admin the IsElevated check should fail and no locking should be done.

      1) Elevated uninstaller runs and creates a locked down ~nsu.tmp
      2) A unrelated non-elevated uninstaller runs and is unable to write to ~nsu.tmp

      Wout a check for only the integrity_level be enough?

      Checking the level requires more code and in our case we really only care about being a member of the administrators group. If we use CheckTokenMembership to make sure we have a non-deny ACE for that group we would be secure on 2000/XP as well. (SYSTEM seems to be a member of the administrators group so that is why you can write without a specific ACE)

       

  • Anders

    Anders - 2015-11-27

    SetDefaultDllDirectories seems to work but with some minor compatibility concerns.

    Another bonus is that it really reduces the need to set the ACL on the temp directory.

    /*
cl /DD2 /FeD2 /O1 /LD testplugin.cpp /link kernel32.lib user32.lib&&move /Y D2.dll D2_.dll
cl /DD1 /FeD1 /O1 /LD testplugin.cpp /link kernel32.lib user32.lib&&move /Y D1.dll D1_.dll
cl /DTP /FeTestPlugin /O1 /LD testplugin.cpp /link kernel32.lib user32.lib D1.lib
makensis testplugin.cpp
*/
#if 0
!if 0
#else /* BEGIN CXX */
#include <windows.h>
#if defined(TP)
EXTERN_C __declspec(dllimport) void Dep_1(HWND hwnd, ...);
EXTERN_C __declspec(dllexport) void __cdecl TestFunc(HWND hwnd, ...)
{
    HMODULE hMod = LoadLibraryA("D2");
    DWORD gle = GetLastError();
    Dep_1(hwnd);
    if (hMod)
    {
        FARPROC fp = GetProcAddress(hMod, "Dep_2");
        if (fp) ((void(__cdecl*)(HWND, ...))fp)(hwnd), gle = 0; else gle = GetLastError();
        FreeLibrary(hMod);
    }
    if (gle) { CHAR b[99]; wsprintfA(b, "GLE=%u", gle); MessageBoxA(hwnd, b, "D2 failed", 0); }
}
#elif defined(D1)
EXTERN_C __declspec(dllexport) void Dep_1(HWND hwnd, ...) { MessageBoxA(hwnd, "Hello from Dep_1", 0, 0); }
#elif defined(D2)
EXTERN_C __declspec(dllexport) void Dep_2(HWND hwnd, ...) { MessageBoxA(hwnd, "Hello from Dep_2", 0, 0); }
#else
#error Define TP, D1 or D2 please
#endif
#endif /* END CXX */
#if 0 
!else /* BEGIN NSIS */
OutFile TestPluginDep.exe
RequestExecutionLevel user
Section
!addplugindir "."
InitPluginsDir
File "/oname=$PluginsDir\D1.dll" "D1_.dll" /* Underscore'd to prevent loading from appdir during testing */
File "/oname=$PluginsDir\D2.dll" "D2_.dll"
TestPlugin::TestFunc
SetOutPath $PluginsDir /* D2 should now be found in older versions. SetDefaultDllDirectories breaks it. */
System::Call 'KERNEL32::AddDllDirectory(w "$PluginsDir")' /* AddDllDirectory can fix it if desired. */
TestPlugin::TestFunc 
SetOutPath $Temp
SectionEnd
!endif /* END NSIS */
#endif /* EOF */
     

    Last edit: Anders 2016-10-10

  • Anders

    Anders - 2015-12-07

    All 3 issues have now been fixed and I'm backporting to v2 now...

     

  • A. Heinecke

    A. Heinecke - 2015-12-08

    Great! From looking at your changes I would also say that you've fixed the Problems. Thanks for your work on this. I'll test the v2 version once you have backported the fixes.

     

  • Gauri Bhave

    Gauri Bhave - 2015-12-22

    Hi,
    I have tried the latest version NSIS 2.49 released on Dec16 and the issue is fixed for shfolder.dll. But the issue is still present with uxtheme.dll. i.e the uxtheme.dll(dummy dll) gets loaded from current folder from where I run the installer (e.g downloads folder). I have attached the screen shot.

    Can you tell me if you are going to fix the issue and when?

    Will appretiate your reply.

    Thanks
    Gauri

     
    Thumbnail
    uxtheme.JPG

    • Eric Lawrence

      Eric Lawrence - 2015-12-22

      @Guari-- Can you please share your executable, and information about your script? Are you using any extensions? I'm not able to reproduce your report using either the NSIS2.49 installer itself nor with my setup program built by 2.49. I'm using a fake UXTheme.dll which shows a messagebox on module load, and I've also verified using WinDBG that the correct module is loading.

      ModLoad: 71850000 7193d000   C:\WINDOWS\SysWOW64\uxtheme.dll
       

    • Anders

      Anders - 2015-12-22

      In the future it would be helpful if you tell us the version of Windows you are using.

      Just to be clear, NSIS does not load UXTheme.dll, Windows does. NSIS calls OleInitialize and on Windows 8 the call stack looks like this:

      USER32!NtUserCreateWindowEx
USER32!VerNtUserCreateWindowEx+0x238
USER32!CreateWindowInternal+0x26b
USER32!CreateWindowExW+0x37
combase!InitMainThreadWnd+0x48
combase!ThreadFirstInitialize+0x18e
combase!_CoInitializeEx+0x18e
combase!CoInitializeEx+0x42
OLE32!OleInitializeEx+0x13
WARNING: Stack unwind information not available. Following frames may be wrong.
image00400000+0x3242

      and when returning from kernel we have

      ntdll!LdrLoadDll+0xc6
KERNELBASE!LoadLibraryExW+0x142
USER32!__ClientLoadLibrary+0x75
ntdll!KiUserCallbackDispatcher+0x2e

      but here on Windows 8 LoadLibrary is called with a full path like Eric Lawrence also points out.

      We will try to come up with a workaround but we cannot preload every .dll in system32 just because some internal function in Windows might load a .dll behind our back.

       

  • Gauri Bhave

    Gauri Bhave - 2015-12-23

    Hi Eric and Anders,

    Thank you for your prompt reply.

    I am using windows 7.

    Attached is the installer on which I can reproduce the issue. Also attached is the script. This is a sample from the NSIS installer.

     
    AdvSplash Test.exe
    Example.nsi

  • Gauri Bhave

    Gauri Bhave - 2015-12-23

    Hi,

    If I set the execution level to admin using
    RequestExecutionLevel admin I do not face the issue.

    Thanks
    Gauri

     

  • Eric Lawrence

    Eric Lawrence - 2015-12-23

    Interestingly, there's a comment on the Dynamic Link Library Security article on MSDN (https://msdn.microsoft.com/en-us/library/windows/desktop/ff919712(v=vs.85).aspx) as follows:

    It's good to use SetSearchPathMode() even if you don't use SearchPath()
    Some Windows APIs follow the "do not use" practice of using SearchPath() and passing the result to LoadLibrary() (e.g., CryptAcquireContext() on XP, or ExtractIconEx() on XP through Win7) so you may want to call SetSearchPathMode() to avoid this potential source of issue even if your own code avoids this behavior.

    This KB (http://support.microsoft.com/kb/2389418) notes:
    An application that loads third-party plugins and that cannot force the plugins to use a qualified path for its LoadLibrary calls should call SetDllDirectory(“”) to remove CWD and then call SetDllDirectory(“plugin install location”) to add the plugin install directory to the DLL search path.

     

    • Anders

      Anders - 2015-12-23

      The current directory is not the only issue, we also need to avoid the application directory and SetDllDirectory does not do that AFAIK. Another problem with SetDllDirectory is that it also affects new processes spawned with CreateProcess and can cause them to fail!

      To just fix CWD issues you could just call SetCurrentDirectory($sysdir) I guess. Not foolproof but better than nothing.

       

  • Anders

    Anders - 2015-12-23

    It seems Vista (SP2) pulls in USERENV.dll so we might have to preload that as well:

    Address    Stack      Procedure / arguments                           
0012F1E8   76C4BF11   kernel32.LoadLibraryA                           
0012F1EC   76F4A380     FileName = "USERENV.dll"
0012F22C   76C3EE43   ? SHELL32.__delayLoadHelper2                    
0012F240   76C3FE9A   ? SHELL32._imp_load__GetUserProfileDirectoryW   
0012F258   76C3FE44   ? SHELL32.kfapi::GetUserProfileDir              
0012F290   76C58C80   ? SHELL32.kfapi::CFixedFolderLoader::GetPath    
0012F34C   76C58B39   ? SHELL32.kfapi::CFolderPathResolver::GetPath   
0012F3E0   76C40E8E   ? SHELL32.kfapi::CFolderCache::GetPath          
0012F454   76C40D43   ? SHELL32.kfapi::CFolderIDListBuilder::GetIDList
0012F4F4   76C703DD   ? SHELL32.kfapi::CFolderCache::GetIDList        
0012F52C   76C7034F   ? SHELL32.kfapi::CKFFacade::GetFolderIDList     
0012F54C   76C765BE   ? SHELL32.SHGetKnownFolderIDList_Internal       
0012F5A0   76C768CE   SHELL32.SHILAliasTranslate                      
0012F5FC   76C6E4B7   Includes SHELL32.76C768CE                       
0012F664   76C661E3   Includes SHELL32.76C6E4B7                       
0012F6B0   76D686D9   SHELL32.SHParseDisplayName                      
0012F8F8   76D6235E   SHELL32.SHGetFileInfoW                          
0012FDE0   004032A3   SHELL32.SHGetFileInfoA                          
????????   ????????   Test.<ModuleEntryPoint>+93
     

  • keeely

    keeely - 2015-12-30

    A commendable effort from Anders in trying to get on top of this, kudos!

    2.5 is a big improvement, but still tries to load wow64log.dll on Windows XP 64-bit.

    OS Name: Microsoft(R) Windows(R) XP Professional x64 Edition
    OS Version: 5.2.3790 Service Pack 1 Build 3790
    OS Manufacturer: Microsoft Corporation
    OS Configuration: Standalone Workstation
    OS Build Type: Uniprocessor Free

    Edit: It may be impossible to prevent this, as I've just tried to create my own minimal EXE that doesn't link to anything (not even kernel32) and wow64log.dll still gets loaded from the stub directory.

     

    Last edit: keeely 2015-12-30

  • San

    San - 2016-01-21

    Just wondering if there is a way to avoid insecure loading of system.dll which is called by nsis during installation. I am using v2.50

     

    • Anders

      Anders - 2016-01-21

      Windows version? System.dll only links to kernel32, user32 and ole32 and NSIS already loads those before System.dll is loaded.

      If you are talking about the dlls loaded by system.dll then you can do something like this:

      Section
System::Call '$SysDir\user32.dll::wsprintf(t.r0, t "%d", i 666)?c'
MessageBox MB_OK $0
SectionEnd

      Please clarify, are you talking about system.dll itself or dlls loaded by system.dll?

       

  • San

    San - 2016-01-22

    Thanks Andres for your inputs.I am talking about System.dll itself on Windows.
    With a test NSIS installer, I could see below DLL lookup in ProcMon:
    Install.exe 1896 QueryOpen C:\Users\test\AppData\Local\Temp\nseD2F6.tmp\System.dll NAME NOT FOUND
    System.dll could potentially be replaced with any other dll which will get loaded.

     

    • Anders

      Anders - 2016-01-23

      To find this folder you already need running code on the system and we change the folder permissions if you are admin so it should not be possible to elevate your privilege by dropping in a evil .dll.

       
1 2 > >> (Page 1 of 2)

Log in to post a comment.