[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