[PATCH v2 03/11] shell32/autocomplete: Handle heap_alloc failure and fix a vulnerability by avoiding the use of sprintf
Huw Davies
huw at codeweavers.com
Fri Sep 7 04:02:16 CDT 2018
On Thu, Sep 06, 2018 at 09:26:13PM +0300, Gabriel Ivăncescu wrote:
> The quickComplete format can have stuff like %1234s or %*.* or more than
> one format argument, which can be exploited since the format string can be
> read from the registry.
>
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
> ---
> dlls/shell32/autocomplete.c | 42 +++++++++++++++++++++++++++++++-----------
> 1 file changed, 31 insertions(+), 11 deletions(-)
>
> diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
> index 4ec7387..22f1a61 100644
> --- a/dlls/shell32/autocomplete.c
> +++ b/dlls/shell32/autocomplete.c
> @@ -109,7 +109,6 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> IAutoCompleteImpl *This = GetPropW(hwnd, autocomplete_propertyW);
> HRESULT hr;
> WCHAR hwndText[255];
> - WCHAR *hwndQCText;
> RECT r;
> BOOL control, filled, displayall = FALSE;
> int cpt, height, sel;
> @@ -138,11 +137,30 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> /* If quickComplete is set and control is pressed, replace the string */
> control = GetKeyState(VK_CONTROL) & 0x8000;
> if (control && This->quickComplete) {
> - hwndQCText = heap_alloc((lstrlenW(This->quickComplete)+lstrlenW(hwndText))*sizeof(WCHAR));
> - sel = sprintfW(hwndQCText, This->quickComplete, hwndText);
> - SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)hwndQCText);
> - SendMessageW(hwnd, EM_SETSEL, 0, sel);
> - heap_free(hwndQCText);
I thought we'd agreed to split this into two patches?
> + WCHAR *buf;
> + size_t len = strlenW(hwndText);
> + size_t sz = strlenW(This->quickComplete) + 1;
> + sz += max(len, 2); /* %s is 2 chars */
> +
> + /* Replace the first %s directly without using snprintf, to avoid
> + exploits since the format string can be retrieved from the registry */
> + if ((buf = heap_alloc(sz * sizeof(WCHAR))))
> + {
> + WCHAR *qc = This->quickComplete, *dst = buf;
> + do
> + {
> + if (qc[0] == '%' && qc[1] == 's')
> + {
> + memcpy(dst, hwndText, len * sizeof(WCHAR));
> + strcpyW(dst + len, qc + 2);
> + break;
> + }
> + *dst++ = *qc++;
> + } while (qc[-1] != '\0');
Moving the sprintf replacement to a helper function would be good. Also,
it should probably cope with unescaping "%%".
> + SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)buf);
> + SendMessageW(hwnd, EM_SETSEL, 0, sz-1);
> + heap_free(buf);
> + }
> }
>
> ShowWindow(This->hwndListBox, SW_HIDE);
> @@ -177,10 +195,10 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> SendMessageW(This->hwndListBox, LB_SETCURSEL, sel, 0);
> if (sel != -1) {
> WCHAR *msg;
> - int len;
> + UINT len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
>
> - len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
> - msg = heap_alloc((len + 1)*sizeof(WCHAR));
> + if ((msg = heap_alloc((len + 1)*sizeof(WCHAR))))
> + return 0;
Err, this suggests two thing to me:
1. You're not testing properly.
2. You're not reviewing you patches properly.
In addition, spaces around the '*' would be nice.
> SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg);
> SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg);
> SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg));
> @@ -287,7 +305,8 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> {
> IAutoCompleteImpl *This = (IAutoCompleteImpl *)GetWindowLongPtrW(hwnd, GWLP_USERDATA);
> WCHAR *msg;
> - int sel, len;
> + UINT len;
> + int sel;
>
> switch (uMsg) {
> case WM_MOUSEMOVE:
> @@ -299,7 +318,8 @@ static LRESULT APIENTRY ACLBoxSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
> if (sel < 0)
> break;
> len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
> - msg = heap_alloc((len + 1)*sizeof(WCHAR));
> + if ((msg = heap_alloc((len + 1)*sizeof(WCHAR))))
> + break;
Same issue here.
> SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg);
> SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg);
> SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg));
> --
> 1.9.1
>
>
>
More information about the wine-devel
mailing list