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@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 avoidexploits 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