[PATCH 03/17] shell32/autocomplete: Handle heap_alloc failure and avoid a potential buffer overflow

[Huw Davies]

On Wed, Sep 05, 2018 at 07:13:05PM +0300, Gabriel Ivăncescu wrote:
> Besides what's stated in the comments, the quickComplete format can have
> stuff like %1234s which can make the resulting string much larger, and
> that's handled via the while loop.
> 
> Signed-off-by: Gabriel Ivăncescu <gabrielopcode at gmail.com>
> ---
>  dlls/shell32/autocomplete.c | 59 +++++++++++++++++++++++++++++----------------
>  1 file changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/dlls/shell32/autocomplete.c b/dlls/shell32/autocomplete.c
> index 254884b..ec91474 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,27 @@ 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 *qc;
> +                        size_t sz = strlenW(This->quickComplete) + 1;
> +                        sz += max(strlenW(hwndText), 2);  /* %s is 2 chars */
> +
> +                        while ((qc = heap_alloc(sz * sizeof(WCHAR)))) {
> +                            /* Pass it 3 times to guard against crap like %*.*s, where the
> +                               width and precision are taken from extra args preceeding it,
> +                               so at least it won't crash or cause other vulnerabilities */

Do we have an app that actually passes crazy format strings?  What we
mainly care about is protecting against crazy user input strings, not app
provided strings, so handle the alloc failure by all means, but I'm not
sure the rest is really useful.


> +                            sel = snprintfW(qc, sz, This->quickComplete,
> +                                            hwndText, hwndText, hwndText);
> +                            if (sel >= 0 && sel < sz) {
> +                                SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)qc);
> +                                SendMessageW(hwnd, EM_SETSEL, 0, sel);
> +                                heap_free(qc);
> +                                break;
> +                            }
> +                            heap_free(qc);
> +                            if (sz > 0x800000U)  /* too large, give up */
> +                                break;
> +                            sz *= 2;
> +                        }
>                      }
>  
>                      ShowWindow(This->hwndListBox, SW_HIDE);
> @@ -177,14 +192,14 @@ static LRESULT APIENTRY ACEditSubclassProc(HWND hwnd, UINT uMsg, WPARAM wParam,
>                              SendMessageW(This->hwndListBox, LB_SETCURSEL, sel, 0);
>                              if (sel != -1) {
>                                  WCHAR *msg;
> -                                int len;
> -
> -                                len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
> -                                msg = heap_alloc((len + 1)*sizeof(WCHAR));
> -                                SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg);
> -                                SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg);
> -                                SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg));
> -                                heap_free(msg);
> +                                UINT len = SendMessageW(This->hwndListBox, LB_GETTEXTLEN, sel, 0);
> +
> +                                if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) {

You could just return on heap_alloc failure, that would keep the patch smaller.

> +                                    SendMessageW(This->hwndListBox, LB_GETTEXT, sel, (LPARAM)msg);
> +                                    SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)msg);
> +                                    SendMessageW(hwnd, EM_SETSEL, lstrlenW(msg), lstrlenW(msg));
> +                                    heap_free(msg);
> +                                }
>                              } else {
>                                  SendMessageW(hwnd, WM_SETTEXT, 0, (LPARAM)This->txtbackup);
>                                  SendMessageW(hwnd, EM_SETSEL, lstrlenW(This->txtbackup), lstrlenW(This->txtbackup));
> @@ -291,7 +306,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:
> @@ -303,12 +319,13 @@ 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));
> -            SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg);
> -            SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg);
> -            SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg));
> -            ShowWindow(hwnd, SW_HIDE);
> -            heap_free(msg);
> +            if ((msg = heap_alloc((len + 1)*sizeof(WCHAR)))) {

Again, a break on alloc failure would keep things simpler.

> +                SendMessageW(hwnd, LB_GETTEXT, sel, (LPARAM)msg);
> +                SendMessageW(This->hwndEdit, WM_SETTEXT, 0, (LPARAM)msg);
> +                SendMessageW(This->hwndEdit, EM_SETSEL, 0, lstrlenW(msg));
> +                ShowWindow(hwnd, SW_HIDE);
> +                heap_free(msg);
> +            }
>              break;
>          default:
>              return CallWindowProcW(This->wpOrigLBoxProc, hwnd, uMsg, wParam, lParam);
> -- 
> 1.9.1
> 
> 
>