On Sat, Sep 08, 2018 at 02:50:47PM +0300, Gabriel Ivăncescu wrote:
The quickComplete format can have more than one % argument, or stuff like %*.* or %1234s, which can be exploited since the format string can be read from the registry, so handle it manually instead of using sprintf.
Signed-off-by: Gabriel Ivăncescu gabrielopcode@gmail.com
Supersedes series starting with 150711.
v3: Use a helper function and handle %% and also split it to multiple patches and fix the embarrassing mistake. And for the entire patch series, I've moved the "txtbackup never NULL" patch to the IACList::Expand patch which will come later (because then it will hopefully make more sense!).
Lastly, please see comments on patch 7/10 in the series for more information, hopefully that's okay now.
dlls/shell32/autocomplete.c | 44 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 38 insertions(+), 6 deletions(-)
diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c index 4ec7387..39df45e 100644 --- a/dlls/shell32/autocomplete.c +++ b/dlls/shell32/autocomplete.c @@ -93,6 +93,34 @@ static inline IAutoCompleteImpl *impl_from_IAutoCompleteDropDown(IAutoCompleteDr return CONTAINING_RECORD(iface, IAutoCompleteImpl, IAutoCompleteDropDown_iface); }
+static void format_quick_complete(WCHAR *dst, const WCHAR *qc, const WCHAR *str, size_t str_len) +{
- /* Replace the first %s directly without using snprintf, to avoid
exploits since the format string can be retrieved from the registry */- while (*qc != '\0')
- {
if (qc[0] == '%'){if (qc[1] == 's'){memcpy(dst, str, str_len * sizeof(WCHAR));dst += str_len;qc += 2;while (*qc != '\0'){if (qc[0] == '%' && qc[1] == '%')qc++;*dst++ = *qc++;}
This inner loop to process %% is ugly. Just do the processing of %s in this block. If you want to make sure you only do it once then set a flag.
break;}qc += (qc[1] == '%');}*dst++ = *qc++;- }
- *dst = '\0';
+}
static void destroy_autocomplete_object(IAutoCompleteImpl *ac) { ac->hwndEdit = NULL; @@ -109,7 +137,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 +165,16 @@ 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);
WCHAR *buf;size_t len = strlenW(hwndText);size_t sz = strlenW(This->quickComplete) + 1 + len;if ((buf = heap_alloc(sz * sizeof(WCHAR)))){format_quick_complete(buf, This->quickComplete, hwndText, len);SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)buf);SendMessageW(hwnd, EM_SETSEL, 0, sz-1);
Again, let's put spaces around binary operators (here the minus op) in new code. This applies other patches in this series too.
However it would be nicer to have format_quick_complete return the length so that you can call EM_SETSEL with the correct length rather than an over-estimate.
heap_free(buf);} } ShowWindow(This->hwndListBox, SW_HIDE);-- 1.9.1