[Bug 46661] New: ISF_Desktop_fnGetDisplayNameOf function missing check for string variable This- >sPathTarget before copy it
https://bugs.winehq.org/show_bug.cgi?id=46661
Bug ID: 46661 Summary: ISF_Desktop_fnGetDisplayNameOf function missing check for string variable This->sPathTarget before copy it Product: Wine Version: 4.0-rc7 Hardware: x86 OS: Linux Status: UNCONFIRMED Severity: normal Priority: P2 Component: shell32 Assignee: wine-bugs@winehq.org Reporter: ossecurity@iscas.ac.cn Distribution: ---
ISF_Desktop_fnGetDisplayNameOf function miss the check for string variable This->sPathTarget before copy it.
Malware(virus etc.) can leverage process injection techniques to hook this WINAPI function(ISF_Desktop_fnGetDisplayNameOf) and tampered the string variable This->sPathTarget which may cause "NULL pointer dereference" and "buffer overflow" in shell32.dll.
583:static HRESULT WINAPI ISF_Desktop_fnGetDisplayNameOf (IShellFolder2 * iface, 584: LPCITEMIDLIST pidl, DWORD dwFlags, LPSTRRET strRet) 585:{ 586: IDesktopFolderImpl *This = impl_from_IShellFolder2(iface); ... 596: pszPath = CoTaskMemAlloc((MAX_PATH +1) * sizeof(WCHAR)); ... 600: if (_ILIsDesktop (pidl)) 601: { 602: if ((GET_SHGDN_RELATION (dwFlags) == SHGDN_NORMAL) && 603: (GET_SHGDN_FOR (dwFlags) & SHGDN_FORPARSING)) 604: strcpyW(pszPath, This->sPathTarget);//** missing check before copy **
We find a lot of similar code but not sure about its seriousness yet. Any comments are helpful.
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #1 from ossecurity ossecurity@iscas.ac.cn --- Function:ISF_Desktop_fnGetDisplayNameOf is in shfldr_desktop.c:583
Here is a List of similar bugs which need further examination:
key stmt/exp function (nearest)WINAPI for HOOK attack strcpyW(pszPath, This->sPathTarget); ISF_Desktop_fnGetDisplayNameOf ITSELF lstrcpynW(szPath, This->sPathTarget, MAX_PATH);ISF_Desktop_fnParseDisplayName ITSELF strlenW(font->name) get_outline_text_metrics GetTextMetricsW/A lstrcpynW(str, physdev->font->name, count); freetype_GetTextFace GetTextMetricsW/A n = strlenW(physdev->font->name) + 1; freetype_GetTextFace GetTextMetricsW/A
lstrcpynW(dst, es->text, count); EDIT_WM_GetText Dispatch/SendMessageW/A memcpy(buf, es->text + s, bufl * sizeof(WCHAR)); EDIT_EM_ReplaceSel Dispatch/SendMessageW/A
https://bugs.winehq.org/show_bug.cgi?id=46661
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |z.figura12@gmail.com Resolution|--- |INVALID Status|UNCONFIRMED |RESOLVED
--- Comment #2 from Zebediah Figura z.figura12@gmail.com --- sPathTarget can't be null at that point; it's allocated in the constructor. (Admittedly we don't check for allocation failure.)
Anyway, if an attacker is able to hook functions, there are much easier ways to cause problems than by changing an internal variable.
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #3 from ossecurity ossecurity@iscas.ac.cn --- Created attachment 63632 --> https://bugs.winehq.org/attachment.cgi?id=63632 errorlog of the bug in ISF_Desktop_fnGetDisplayNameOf
This hook program example, is to trigger a missing check of 'This->sPathTarget' in function 'ISF_Desktop_fnGetDisplayNameOf'. [https://bugs.winehq.org/show_bug.cgi?id=46661] It seems that this bug exists in all wine version till today(2019.2.19).
The errorlog.txt in it is generated under Debian Linux i386 environment and with wine-2.0.2
For details please refer README.txt in it.
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #4 from ossecurity ossecurity@iscas.ac.cn --- Created attachment 63633 --> https://bugs.winehq.org/attachment.cgi?id=63633 README file with instruction to trigger it.
This hook program example, is for a missing check of 'This->sPathTarget' in function 'ISF_Desktop_fnGetDisplayNameOf'. [https://bugs.winehq.org/show_bug.cgi?id=46661] It seems that this bug exists in all wine version till today(2019.2.19).
The errorlog.txt is generated under Debian Linux i386 environment and with wine-2.0.2.
Steps to run this test example.
1.Extract this zip file to (e.g~/example). 2*(optional).Put the explorer.exe into the bin directory(e.g ~/example/bin). Tip: you can replace it with your own. Find it from wine PREFIX directory. [e.g. ~/.wine/drive_c/windows/system32/explorer.exe ~/.wine/drive_c/windows/explorer.exe]
3.Change to that directory and run DoInjection.exe by wine $ cd ~/example/bin $ wine ./DoInjection.exe $ ^C (Ctrl+C stop it, but not stop wineserver) $ wine ./DoInjection.exe (the second run will trigger the bug)
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #5 from ossecurity ossecurity@iscas.ac.cn --- Created attachment 63634 --> https://bugs.winehq.org/attachment.cgi?id=63634 DoInjection.exe
DoInjection.exe is the main file of this test example. It requires MfcHookApi.dll and explorer.exe(this can be replaced by your own) to run.
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #6 from ossecurity ossecurity@iscas.ac.cn --- Created attachment 63635 --> https://bugs.winehq.org/attachment.cgi?id=63635 MfcHookApi.dll
MfcHookApi.dll is dll for process injection. It serves DoInjection.exe and is injected to process: explorer.exe(this can be replaced by your own).
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #7 from ossecurity ossecurity@iscas.ac.cn --- Created attachment 63636 --> https://bugs.winehq.org/attachment.cgi?id=63636 explorer.exe
This explorer.exe is from wine-2.0.2 which triggered the errorlog.txt.
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #8 from Zebediah Figura z.figura12@gmail.com --- There are much simpler ways to crash a program from a hook. To paraphrase Douglas Adams (and to pay a nod to Raymond Chen), you are already on the other side of the airtight hatchway.
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #9 from ossecurity ossecurity@iscas.ac.cn --- Hi, Zebediah
Thank you for your reply. I'm a novice about attack methods. What do you mean by much easier ways? Could you please provide some examples? Names or website links are all helpful for me.
By the way, I think the error happened in dll of wine, so it is different from bugs in win32 application. Is the error trigger place make any difference?
------------------------------------------ I upload a log file and a sample test. In this test case, we tamper the 'sPathTarget' to 'NULL', and trigger a 'NULL pointer dereference'. (buffer overflow can be triggered in a similar way but we not provide for the moment).
DoInjection.exe and MfcHookApi.dll are created by using classic injection technique. (The first technique summarized in this website [https://www.endgame.com/blog/technical-blog/ten-process-injection-techniques...])
Hope this can help, and thanks for your patience.
Ke Yang
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #10 from Zebediah Figura z.figura12@gmail.com --- (In reply to ossecurity from comment #9)
Hi, Zebediah
Thank you for your reply. I'm a novice about attack methods. What do you mean by much easier ways? Could you please provide some examples?
For example, in your hook, execute:
*((int *)NULL) = 0;
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #11 from ossecurity ossecurity@iscas.ac.cn --- Yes, that can cause application crash and this crash is a behaviour of Win32 application. However, the missing check in ISF_Desktop_fnGetDisplayNameOf is the behaviour of wine.
It will be clearer to judge this bug if we focus on the behaviour mismatch. As hooking is supported function in windows, a prepared Win32 Application(DoInjection.exe) doesn't crash in Windows(I verify it on Win 7), but it crash in wine. It seems Win7 has added sufficient checks(sanitizations or authority checks), however, wine doesn't.
Ke Yang
https://bugs.winehq.org/show_bug.cgi?id=46661
Zebediah Figura z.figura12@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED
--- Comment #12 from Zebediah Figura z.figura12@gmail.com --- (In reply to ossecurity from comment #11)
Yes, that can cause application crash and this crash is a behaviour of Win32 application. However, the missing check in ISF_Desktop_fnGetDisplayNameOf is the behaviour of wine.
It will be clearer to judge this bug if we focus on the behaviour mismatch. As hooking is supported function in windows, a prepared Win32 Application(DoInjection.exe) doesn't crash in Windows(I verify it on Win 7), but it crash in wine. It seems Win7 has added sufficient checks(sanitizations or authority checks), however, wine doesn't.
You're also assuming that Windows has the same struct layout as Wine, which it almost certainly doesn't. Only behaviour differences that affect real applications are worth fixing.
There is no real reason to check for NULL here. It doesn't matter whose "behaviour" the code is. The contract internal to the Wine code is that the variable is valid from the moment the struct is allocated, not that it is valid if and only if it is non-NULL.
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #13 from ossecurity ossecurity@iscas.ac.cn --- (In reply to Zebediah Figura from comment #12)
(In reply to ossecurity from comment #11)
Yes, that can cause application crash and this crash is a behaviour of Win32 application. However, the missing check in ISF_Desktop_fnGetDisplayNameOf is the behaviour of wine.
It will be clearer to judge this bug if we focus on the behaviour mismatch. As hooking is supported function in windows, a prepared Win32 Application(DoInjection.exe) doesn't crash in Windows(I verify it on Win 7), but it crashes in wine. It seems Win7 has added sufficient checks(sanitizations or authority checks), however, wine doesn't.
You're also assuming that Windows has the same struct layout as Wine, which it almost certainly doesn't. Only behaviour differences that affect real applications are worth fixing.
There is no real reason to check for NULL here. It doesn't matter whose "behaviour" the code is. The contract internal to the Wine code is that the variable is valid from the moment the struct is allocated, not that it is valid if and only if it is non-NULL.
Oh yes, I mix struct layout difference into behaviour difference. DoInjection.exe is really not as important as Word etc. The check consideration really has a conflict with the internal contract of wine you just say. When considering about fix and maintenance, you are right.
Maybe this kind of check for is important in other scene or software besides wine. I'll keep on researching.
Ke Yang
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #14 from ossecurity ossecurity@iscas.ac.cn --- Modern operating systems and libraries always perform validity checks of API parameters for safe computation. However, the data querying from the environment are paid little attention. As the API can be hooked, adverse can intercept the return value and parameter.(In reply to ossecurity from comment #13)
(In reply to Zebediah Figura from comment #12)
(In reply to ossecurity from comment #11)
Yes, that can cause application crash and this crash is a behaviour of Win32 application. However, the missing check in ISF_Desktop_fnGetDisplayNameOf is the behaviour of wine.
It will be clearer to judge this bug if we focus on the behaviour mismatch. As hooking is supported function in windows, a prepared Win32 Application(DoInjection.exe) doesn't crash in Windows(I verify it on Win 7), but it crashes in wine. It seems Win7 has added sufficient checks(sanitizations or authority checks), however, wine doesn't.
You're also assuming that Windows has the same struct layout as Wine, which it almost certainly doesn't. Only behaviour differences that affect real applications are worth fixing.
There is no real reason to check for NULL here. It doesn't matter whose "behaviour" the code is. The contract internal to the Wine code is that the variable is valid from the moment the struct is allocated, not that it is valid if and only if it is non-NULL.
Oh yes, I mix struct layout difference into behaviour difference. DoInjection.exe is really not as important as Word etc. The check consideration really has a conflict with the internal contract of wine you just say. When considering about fix and maintenance, you are right.
Maybe this kind of check for is important in other scene or software besides wine. I'll keep on researching.
I have different thought now.
What do you mean by real applications? The DoInjection.exe inject code in explorer.exe(this is real applications).
And I think the internal contract is wrong if it's just as you say. The variable can be changed(by a hooker) between construction and use.
Ke Yang
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #15 from ossecurity ossecurity@iscas.ac.cn --- (In reply to Zebediah Figura from comment #12)
(In reply to ossecurity from comment #11)
Yes, that can cause application crash and this crash is a behaviour of Win32 application. However, the missing check in ISF_Desktop_fnGetDisplayNameOf is the behaviour of wine.
It will be clearer to judge this bug if we focus on the behaviour mismatch. As hooking is supported function in windows, a prepared Win32 Application(DoInjection.exe) doesn't crash in Windows(I verify it on Win 7), but it crashes in wine. It seems Win7 has added sufficient checks(sanitizations or authority checks), however, wine doesn't.
You're also assuming that Windows has the same struct layout as Wine, which it almost certainly doesn't. Only behaviour differences that affect real applications are worth fixing.
There is no real reason to check for NULL here. It doesn't matter whose "behaviour" the code is. The contract internal to the Wine code is that the variable is valid from the moment the struct is allocated, not that it is valid if and only if it is non-NULL.
I have different thought now.
What do you mean by real applications? The DoInjection.exe inject code in explorer.exe(this is real applications).
And I think the internal contract is wrong if it's just as you say. The variable can be changed(by a hooker) between construction and use.
Ke Yang
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #16 from Nikolay Sivov bunglehead@gmail.com --- You can corrupt any memory at any point during method execution, there is no way to check for that.
There are many places that need improvement in shell32 or other modules, like fixed size buffer copies without length check. Case you are describing is not interesting.
https://bugs.winehq.org/show_bug.cgi?id=46661
--- Comment #17 from ossecurity ossecurity@iscas.ac.cn --- (In reply to Nikolay Sivov from comment #16)
You can corrupt any memory at any point during method execution, there is no way to check for that.
There are many places that need improvement in shell32 or other modules, like fixed size buffer copies without length check. Case you are describing is not interesting.
Only the data which can be accessed by WINAPI parameters and return values need to be checked. Other inner variables are safe.
-
wine-bugs＠winehq.org