[PATCH v3 01/10] shell32/autocomplete: Fix a vulnerability by avoiding the use of snprintf

[Huw Davies]

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 at 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
> 
> 
>