and if SetDefaultDllDirectories function is not supported, it loads some DLLs (like UXTHEME.dll) from system folder. I didn't test that code in many systems. So if you know how to improve that code, please notify me.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think I know another way of increasing the DLL security: you can temporarily change the current directory to System32 before calling LoadLibrary(). This should work on any Windows version. However, if an attacker creates a .local file with SFX's name, the DLLs would be loaded from SFX directory anyway. (UPDATE: I didn't read MSDN carefully: .local files are ignored if a manifest is present.)
I also suggest some changes to DllSecur.c:
1) remove Windows version check (SetDefaultDllDirectories() could appear under older systems after some patch);
2) declare buf[] dynamically, or at least replace 100 with something like sizeof(g_Dlls)+2;
3) replace pos with len and eliminate the call to lstrlenW().
Last edit: Shell 2016-09-29
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
1) It's from NSIS source code. They suppose that we can't use SetDefaultDllDirectories() function at Vista (it works incorrectly). I didn't test it. I have no Vista now.
2) g_Dlls can be long list (for example, if we increase it to list of 100 dlls) . We don't need big buffer. And name of one DLL (it's standard DLL) can't be larger than 100 characters.
The main question about that code: Do we need to load these special DLLs, if SetDefaultDllDirectories() works correctly?
I suppose that we don't.
But NSIS still loads DLLs.
So what way is more correct?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I have XP (x86 and x64), Vista (x86) and Win7 (x64). If you provide something to test, I can check these OSes.
I think that no extra action is needed if SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32) succeeds. On the other hand, this function need not to be called if all DLLs are loaded with LoadLibraryEx() with the LOAD_LIBRARY_SEARCH_SYSTEM32 flag set, unless you want to know whether that flag has any effect.
Loading DLLs by the full path to System32 with the LOAD_WITH_ALTERED_SEARCH_PATH flag for is more universal since it works even on Win95. There is no need to load all the DLLs from one place; I think it is better to load all system DLLs with LoadLibraryEx() directly from where they are loaded now. If the DLL is absent from System32, LoadLibraryEx() will fail and we detect a damaged OS distribution.UPDATE: no, the search may continue in other directories unless disabled by SetDefaultDllDirectories().
Now my suggestions are as follows:
1) link the SFX statically only against known DLLs (such as KERNEL32);
2) load other system DLLs with a code similar to the following: HANDLE LoadSystemLibraryW(WCHAR *path)
{
// append path to the full path to System32
return LoadLibraryEx(buf,NULL,LOAD_WITH_ALTERED_SEARCH_PATH);
}
Last edit: Shell 2016-09-29
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I used my test DLL: http://7-zip.org/a/dlltest.7z
you need
1) run DebugView from sysinternals.
2) copy UXTHEME.dll from dlltest.7z to some folder with exe files
3) you can copy UXTHEME.dll to another files with any other "bad" dll names.
4) run exe file (for example, old exe installer 7z1602.exe and new 7z1603.exe)
5) if system loads dll (like UXTHEME.dll) from current folder, then you will see a name of that dll in DebugView. It means that dll attack is possible.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
There is BUG in 16.03 in that code.
Correct code for Vista check must be:
if (!GetVersionEx(&vi) || vi.dwMajorVersion != 6 || vi.dwMinorVersion != 0)
So 16.03 calls SetDefaultDllDirectories() at Vista too.
But NSIS writes that CoCreateInstance(&CLSID_ShellLink doesn't work in that case.
I use CoCreateInstance(&CLSID_ShellLink
to create links for start menu folder:
7-Zip File Manager.lnk
7-Zip Help.lnk
So can you check it again in Vista:
1) uninstall 7-Zip 16.03
2) check that there is no Start menu links.
3) install 7-Zip 16.03
4) check links in Start menu.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
No links. CoCreateInstance() is called 3 or 4 times and always returns E_INVALIDARG.
If you don't like the idea about loading DLLs from the absolute path without using SetDefaultDllDirectories() at all, try calling the latter function with all flags before CoCreateInstance(). If OLE32 cannot load necessary DLLs, this will help.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Actually I'll fix that bug with version check (for Vista).
And now I don't know why I need to change something else.
Or you think that some things work incorrectly?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I don't. I just think that, since everything works without SetDefaultDllDirectories() (see my experiment in ntsd), this call and version check are redundant.
Could you probably create a test version with two SetDefaultDllDirectories() calls - one before loading DLLs and one after (but before creating links)? Maybe this will make Vista happy.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
We don't know exact list of "bad" DLLs names.
Look, for example, winrar sfxs and installer. It contain some list of DLLs as I remember (search UXTHEME in UTF-16).
So if SetDefaultDllDirectories removes all attacks with any name of DLL, it's good to use it.
If we can't use SetDefaultDllDirectories, we preload "bad" DLLs from safe windows folder.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I didn't understand what a "bad" DLL is. I think that we can divide all the DLLs that an application uses into two categories: 1) Windows DLLs; 2) third-party DLLs (including application's own DLLs). An application always knows each DLL from cat. 2 it uses. It may not know the dependencies of the DLLs from cat. 1, but all such dependencies also fall in cat. 1 and thus should be present in System32.
It may require a thorough investigation to determine whether cat. 2 DLLs can be replaced by malware in a particular case. This means that you're right with taking extra precautions. However, if an application uses only cat. 1 DLLs, I think they can be safely loaded from System32 with a simple GetSystemDirectory()+LoadLibrary().
UPDATE: the above method is unsafe. According to MSDN,
If a DLL has dependencies, the system searches for the dependent DLLs as if they were loaded with just their module names. This is true even if the first DLL was loaded by specifying a full path.
So, Igor, you're right to call SetDefaultDllDirectories(). Perhaps, it is worthwhile to call SetDllDirectoryW(L"") (with empty path; this function was introduced with WinXP SP1) if SetDefaultDllDirectories() is not found - this will prevent the application from loading DLLs from the current directory (but not from its own directory).
Another thing is that, according to MSDN, the NSIS way also has a flaw: any dependency of DLLs from g_Dlls[] will be loaded from the current directory first. Please, consider this idea when writing your own security code.
Last edit: Shell 2016-09-30
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The problem that windows can load some "bad" DLLs from current folder instead of system folder. Actually I don't know exact list of such DLLs. Look NSIS source code (search uxtheme), that writes about DLLs for different systems.
I suppose, that if we use SetDefaultDllDirectories, we don't need to worry that that there is some new "bad" DLL that can be used in future version of windows (or after some windows update).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Yes, I understand that now. Please, consider my update in the previous post. To summarize, XP can be partially protected by SetDllDirectory(), and Vista can probably (I'll test that now) be protected by calling SetDefaultDllDirectories() and then reverting its action.
Last edit: Shell 2016-09-30
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Above I read something about using absolute paths, do you mean using driver letter, eg "C:\Windows\System32"? Afaik, it's a bad idea because Windows can run on other letters (at least in uncommon installations); should use method to get system folder.
I found some articles that may be helpful for this discussion.
Yes, you're right. The correct path is determined by calling GetSystemDirectory() rather than hardcoding System32 - I have mentioned that function above.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
If I understand .ZIP File Format Specification correctly, setup.exe is an invalid ZIP archive. Its End of central directory specifies the start of central directory at offset 0x3E0689A, but its actual offset is 0x3E1FC9E due to the SFX module. The difference 0x19404 matches neither SFX size, nor unpacked SFX size. It can be determined only by scanning the file for both the End of central directory signature and the Start of central directory signature. However, a ZIP archive may contain several Starts of central directory (which is unlikely but still correct).
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The behavior is undefined when LOAD_WITH_ALTERED_SEARCH_PATH flag is set, and lpFileName specifiies a relative path.
This value cannot be combined with any LOAD_LIBRARY_SEARCH flag.
On my OS (Vista x86 SP2), if any of the LOAD_LIBRARY_SEARCH flags is specified in SetDefaultDllDirectories(), a check is performed on the path. If the path is relative, LoadLibraryEx() fails. Unfortunately, the parameter to SetDefaultDllDirectories()must contain LOAD_LIBRARY_SEARCH flags (otherwise the function fails). Thus, OLE32.DLL thinks it cannot load SHELL32.DLL (it is actually loaded, but LoadLibraryEx() returns NULL before checking the loaded libraries).
Consider the following solution: static DWORD _BaseDefaultDllDirectories;
#define SetDefaultDllDirectories(flags) (_BaseDefaultDllDirectories=flags)
#define MyLoadLibrary(path) LoadLibraryEx(path,NULL,_BaseDefaultDllDirectories)
#define MyLoadLibraryEx(path,file,flags) LoadLibraryEx(path,NULL,_BaseDefaultDllDirectories | flags)
Now each DLL loaded from the application will be secured:
If this value is used, <...> is searched for the DLL and its dependencies. Directories in the standard search path are not searched.
On the other hand, each DLL loaded dynamically from other DLLs will NOT be secured. This is a flaw, but it resolves the issue with CoCreateInstance().
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
7-Zip 16.03 was released.
7-Zip for 32-bit Windows:
http://7-zip.org/a/7z1603.exe
or
http://7-zip.org/a/7z1603.msi
7-Zip for 64-bit Windows x64:
http://7-zip.org/a/7z1603-x64.exe
or
http://7-zip.org/a/7z1603-x64.msi
What's new after 7-Zip 16.02:
In to archive lzma1603.7z SFX module 7zS2.sfx have version 16.02. Do your needed update for this SFX?
Big thanks! I update my LAN installation.
Can we get around the problem by using the 32-bit version, or do we have to use the 64-bit?
Thankyou Igor for this update, I'm a big fan of 7-Zip.
Great, that's good for transmission.
The code that must protect against DLL preloading attack uses two things:
and if
SetDefaultDllDirectoriesfunction is not supported, it loads some DLLs (like UXTHEME.dll) from system folder. I didn't test that code in many systems. So if you know how to improve that code, please notify me.I think I know another way of increasing the DLL security: you can temporarily change the current directory to
System32before callingLoadLibrary(). This should work on any Windows version.However, if an attacker creates a(UPDATE: I didn't read MSDN carefully:.localfile with SFX's name, the DLLs would be loaded from SFX directory anyway..localfiles are ignored if a manifest is present.)I also suggest some changes to
DllSecur.c:1) remove Windows version check (
SetDefaultDllDirectories()could appear under older systems after some patch);2) declare
buf[]dynamically, or at least replace100with something likesizeof(g_Dlls)+2;3) replace
poswithlenand eliminate the call tolstrlenW().Last edit: Shell 2016-09-29
1) It's from NSIS source code. They suppose that we can't use SetDefaultDllDirectories() function at Vista (it works incorrectly). I didn't test it. I have no Vista now.
2) g_Dlls can be long list (for example, if we increase it to list of 100 dlls) . We don't need big buffer. And name of one DLL (it's standard DLL) can't be larger than 100 characters.
The main question about that code:
Do we need to load these special DLLs, if SetDefaultDllDirectories() works correctly?
I suppose that we don't.
But NSIS still loads DLLs.
So what way is more correct?
I have XP (x86 and x64), Vista (x86) and Win7 (x64). If you provide something to test, I can check these OSes.
I think that no extra action is needed if
SetDefaultDllDirectories(LOAD_LIBRARY_SEARCH_SYSTEM32)succeeds. On the other hand, this function need not to be called if all DLLs are loaded withLoadLibraryEx()with theLOAD_LIBRARY_SEARCH_SYSTEM32flag set, unless you want to know whether that flag has any effect.Loading DLLs by the full path to
System32with theLOAD_WITH_ALTERED_SEARCH_PATHflag for is more universal since it works even on Win95. There is no need to load all the DLLs from one place; I think it is better to load all system DLLs withLoadLibraryEx()directly from where they are loaded now. If the DLL is absent fromSystem32,LoadLibraryEx()will fail and we detect a damaged OS distribution.SetDefaultDllDirectories().Now my suggestions are as follows:
1) link the SFX statically only against known DLLs (such as
KERNEL32);2) load other system DLLs with a code similar to the following:
HANDLE LoadSystemLibraryW(WCHAR *path){
// append path to the full path to System32
return LoadLibraryEx(buf,NULL,LOAD_WITH_ALTERED_SEARCH_PATH);
}
Last edit: Shell 2016-09-29
I used my test DLL:
http://7-zip.org/a/dlltest.7z
you need
1) run DebugView from sysinternals.
2) copy UXTHEME.dll from dlltest.7z to some folder with exe files
3) you can copy UXTHEME.dll to another files with any other "bad" dll names.
4) run exe file (for example, old exe installer 7z1602.exe and new 7z1603.exe)
5) if system loads dll (like UXTHEME.dll) from current folder, then you will see a name of that dll in DebugView. It means that dll attack is possible.
Running tests on fully updated Windows Vista x86 SP2, elevated prompt.
R:\TEMP>ntsd -g 7z1602.exeModLoad: 00400000 0040e000 image00400000
ModLoad: 76fa0000 770c9000 ntdll.dll
ModLoad: 77120000 771fd000 C:\Windows\system32\kernel32.dll
Also loaded from system32: ole32, msvcrt, GDI32, USER32, ADVAPI32, RPCRT4, SHELL32, SHLWAPI, ShimEng, apphelp, IMM32, MSCTF, LPK, USP10
ModLoad: 74090000 7422e000 C:\Windows\WinSxS\x86_microsoft.windows.common-cont
rols_6595b64144ccf1df_6.0.6002.19373_none_5cbe60a608848a19\comctl32.dll
ModLoad: 10000000 10005000 R:\TEMP\UxTheme.dll
R:\TEMP\UxTheme.dll1
R:\TEMP>ntsd -g 7z1603.exeModLoad: 00400000 0040e000 image00400000
ModLoad: 76fa0000 770c9000 ntdll.dll
ModLoad: 77120000 771fd000 C:\Windows\system32\kernel32.dll
Also loaded from system32: ole32, msvcrt, GDI32, USER32, ADVAPI32, RPCRT4, SHELL32, SHLWAPI, ShimEng, apphelp, IMM32, MSCTF, LPK, USP10
ModLoad: 74090000 7422e000 C:\Windows\WinSxS\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.19373_none_5cbe60a608848a19\comctl32.dll
ModLoad: 73e60000 73e9f000 C:\Windows\system32\UxTheme.dll
Now I disable
SetDefaultDllDirectories():R:\TEMP>ntsd -g 7z1603.exeModLoad: 00400000 0040e000 image00400000
ModLoad: 76fa0000 770c9000 ntdll.dll
ModLoad: 77120000 771fd000 C:\Windows\system32\kernel32.dll
Also loaded from system32: ole32, msvcrt, GDI32, USER32, ADVAPI32, RPCRT4, SHELL32, SHLWAPI
0:000> a 405965
00405965 xor eax,eax // as if GetProcAddress() returned NULL
00405967
0:000> g
Loaded from system32: ShimEng, apphelp, IMM32, MSCTF, LPK, USP10
ModLoad: 74090000 7422e000 C:\Windows\WinSxS\x86_microsoft.windows.common-controls_6595b64144ccf1df_6.0.6002.19373_none_5cbe60a608848a19\comctl32.dll
ModLoad: 73e60000 73e9f000 C:\Windows\system32\UxTheme.dll
Also loaded from system32: USERENV, Secur32, SETUPAPI, OLEAUT32, APPHELP, PROPSYS, DWMAPI, OLEACC, CLBCATQ - i.e. whole g_Dlls[]
So,LoadLibraryEx()with absolute paths is sufficient.LoadLibrary()will also do as long as the DLL is present inSystem32.There is BUG in 16.03 in that code.
Correct code for Vista check must be:
So 16.03 calls SetDefaultDllDirectories() at Vista too.
But NSIS writes that
CoCreateInstance(&CLSID_ShellLinkdoesn't work in that case.I use
CoCreateInstance(&CLSID_ShellLinkto create links for start menu folder:
So can you check it again in Vista:
1) uninstall 7-Zip 16.03
2) check that there is no Start menu links.
3) install 7-Zip 16.03
4) check links in Start menu.
No links.
CoCreateInstance()is called 3 or 4 times and always returnsE_INVALIDARG.If you don't like the idea about loading DLLs from the absolute path without using
SetDefaultDllDirectories()at all, try calling the latter function with all flags beforeCoCreateInstance(). If OLE32 cannot load necessary DLLs, this will help.Actually I'll fix that bug with version check (for Vista).
And now I don't know why I need to change something else.
Or you think that some things work incorrectly?
I don't. I just think that, since everything works without
SetDefaultDllDirectories()(see my experiment inntsd), this call and version check are redundant.Could you probably create a test version with two
SetDefaultDllDirectories()calls - one before loading DLLs and one after (but before creating links)? Maybe this will make Vista happy.We don't know exact list of "bad" DLLs names.
Look, for example, winrar sfxs and installer. It contain some list of DLLs as I remember (search UXTHEME in UTF-16).
So if SetDefaultDllDirectories removes all attacks with any name of DLL, it's good to use it.
If we can't use SetDefaultDllDirectories, we preload "bad" DLLs from safe windows folder.
I didn't understand what a "bad" DLL is. I think that we can divide all the DLLs that an application uses into two categories: 1) Windows DLLs; 2) third-party DLLs (including application's own DLLs). An application always knows each DLL from cat. 2 it uses. It may not know the dependencies of the DLLs from cat. 1, but all such dependencies also fall in cat. 1 and thus should be present in System32.
It may require a thorough investigation to determine whether cat. 2 DLLs can be replaced by malware in a particular case. This means that you're right with taking extra precautions. However, if an application uses only cat. 1 DLLs, I think they can be safely loaded from System32 with a simple
GetSystemDirectory()+LoadLibrary().UPDATE: the above method is unsafe. According to MSDN,
So, Igor, you're right to call
SetDefaultDllDirectories(). Perhaps, it is worthwhile to callSetDllDirectoryW(L"")(with empty path; this function was introduced with WinXP SP1) ifSetDefaultDllDirectories()is not found - this will prevent the application from loading DLLs from the current directory (but not from its own directory).Another thing is that, according to MSDN, the NSIS way also has a flaw: any dependency of DLLs from
g_Dlls[]will be loaded from the current directory first. Please, consider this idea when writing your own security code.Last edit: Shell 2016-09-30
The problem that windows can load some "bad" DLLs from current folder instead of system folder. Actually I don't know exact list of such DLLs. Look NSIS source code (search uxtheme), that writes about DLLs for different systems.
I suppose, that if we use SetDefaultDllDirectories, we don't need to worry that that there is some new "bad" DLL that can be used in future version of windows (or after some windows update).
Yes, I understand that now. Please, consider my update in the previous post. To summarize, XP can be partially protected by
SetDllDirectory(),and Vista can probably (I'll test that now) be protected by calling.SetDefaultDllDirectories()and then reverting its actionLast edit: Shell 2016-09-30
Above I read something about using absolute paths, do you mean using driver letter, eg "C:\Windows\System32"? Afaik, it's a bad idea because Windows can run on other letters (at least in uncommon installations); should use method to get system folder.
I found some articles that may be helpful for this discussion.
https://msdn.microsoft.com/en-us/library/ff919712.aspx
https://blogs.technet.microsoft.com/srd/2010/08/23/more-information-about-the-dll-preloading-remote-attack-vector/ (Also see attached doc "Secure loading of libraries to prevent DLL Preloading.docx")
https://flexeracommunity.force.com/customer/articles/en_US/INFO/Best-Practices-to-Avoid-Windows-Setup-Launcher-Executable-Issues
http://wixtoolset.org/development/wips/5184-burn-clean-room/
https://www.firegiant.com/blog/2016/1/20/wix-v3.10.2-released/
http://www.binaryplanting.com/
Yes, you're right. The correct path is determined by calling
GetSystemDirectory()rather than hardcodingSystem32- I have mentioned that function above.При попытке распаковать инсталлятор, созданный InstallAnywhere, 7-zip не справляется. Unzip.exe (http://www.info-zip.org/pub/infozip/UnZip.html) же справляется.
Вот инсталлятор http://www.ups-software-download.com/winpower/data/Windows/Winpower_setup_Windows.zip
Можно ли как-нибудь заставить 7-zip распаковывать такие файлы напрямую?
If I understand .ZIP File Format Specification correctly,
setup.exeis an invalid ZIP archive. Its End of central directory specifies the start of central directory at offset 0x3E0689A, but its actual offset is 0x3E1FC9E due to the SFX module. The difference 0x19404 matches neither SFX size, nor unpacked SFX size. It can be determined only by scanning the file for both the End of central directory signature and the Start of central directory signature. However, a ZIP archive may contain several Starts of central directory (which is unlikely but still correct).I have identified the problem with
CoCreateInstance()on Vista. A very clever Microsoft programmer has made the following call inOLE32.DLL:According to MSDN:
On my OS (Vista x86 SP2), if any of the
LOAD_LIBRARY_SEARCHflags is specified inSetDefaultDllDirectories(), a check is performed on the path. If the path is relative,LoadLibraryEx()fails. Unfortunately, the parameter toSetDefaultDllDirectories()must containLOAD_LIBRARY_SEARCHflags (otherwise the function fails). Thus,OLE32.DLLthinks it cannot loadSHELL32.DLL(it is actually loaded, butLoadLibraryEx()returns NULL before checking the loaded libraries).Consider the following solution:
static DWORD _BaseDefaultDllDirectories;#define SetDefaultDllDirectories(flags) (_BaseDefaultDllDirectories=flags)
#define MyLoadLibrary(path) LoadLibraryEx(path,NULL,_BaseDefaultDllDirectories)
#define MyLoadLibraryEx(path,file,flags) LoadLibraryEx(path,NULL,_BaseDefaultDllDirectories | flags)
Now each DLL loaded from the application will be secured:
On the other hand, each DLL loaded dynamically from other DLLs will NOT be secured. This is a flaw, but it resolves the issue with
CoCreateInstance().