[Qemu-devel] [PATCH for-3.1 v3 0/2] usb-mtp: two bugfixes (one security fix).

[Qemu-devel] [PATCH for-3.1 v3 0/2] usb-mtp: two bugfixes (one security fix).

[Gerd Hoffmann]

v3:
 - add missing RES_ prefix to response code.
v2:
 - add comment about the (preexisting) issue noted by armbru.
 - change error code as suggested by bsd.
 - update reporter email address.

Gerd Hoffmann (2):
  usb-mtp: fix utf16_to_str
  usb-mtp: outlaw slashes in filenames

 hw/usb/dev-mtp.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH for-3.1 v3 1/2] usb-mtp: fix utf16_to_str

[Gerd Hoffmann]

Make utf16_to_str return an allocated string.  Remove the assumtion that
the number of string bytes equals the number of utf16 chars (which is
only true for ascii chars).  Instead call wcstombs twice, once to figure
the storage size and once for the actual conversion (as suggested by the
wcstombs manpage).

Reported-by: Michael Hanselmann <public@hansmi.ch>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 00a3691bae..0f6a9702ef 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1593,17 +1593,23 @@ static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p)
     fprintf(stderr, "%s\n", __func__);
 }
 
-static void utf16_to_str(uint8_t len, uint16_t *arr, char *name)
+static char *utf16_to_str(uint8_t len, uint16_t *arr)
 {
-    int count;
-    wchar_t *wstr = g_new0(wchar_t, len);
+    wchar_t *wstr = g_new0(wchar_t, len + 1);
+    int count, dlen;
+    char *dest;
 
     for (count = 0; count < len; count++) {
+        /* FIXME: not working for surrogate pairs */
         wstr[count] = (wchar_t)arr[count];
     }
+    wstr[count] = 0;
 
-    wcstombs(name, wstr, len);
+    dlen = wcstombs(NULL, wstr, 0) + 1;
+    dest = g_malloc(dlen);
+    wcstombs(dest, wstr, dlen);
     g_free(wstr);
+    return dest;
 }
 
 /* Wrapper around write, returns 0 on failure */
@@ -1703,7 +1709,7 @@ static void usb_mtp_write_metadata(MTPState *s)
 {
     MTPData *d = s->data_out;
     ObjectInfo *dataset = (ObjectInfo *)d->data;
-    char *filename = g_new0(char, dataset->length);
+    char *filename;
     MTPObject *o;
     MTPObject *p = usb_mtp_object_lookup(s, s->dataset.parent_handle);
     uint32_t next_handle = s->next_handle;
@@ -1711,7 +1717,7 @@ static void usb_mtp_write_metadata(MTPState *s)
     assert(!s->write_pending);
     assert(p != NULL);
 
-    utf16_to_str(dataset->length, dataset->filename, filename);
+    filename = utf16_to_str(dataset->length, dataset->filename);
 
     o = usb_mtp_object_lookup_name(p, filename, dataset->length);
     if (o != NULL) {
-- 
2.9.3

[Qemu-devel] [PATCH for-3.1 v3 2/2] usb-mtp: outlaw slashes in filenames

[Gerd Hoffmann]

Slash is unix directory separator, so they are not allowed in filenames.
Note this also stops the classic escape via "../".

Fixes: CVE-2018-16867
Reported-by: Michael Hanselmann <public@hansmi.ch>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/usb/dev-mtp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 0f6a9702ef..100b7171f4 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -1719,6 +1719,12 @@ static void usb_mtp_write_metadata(MTPState *s)
 
     filename = utf16_to_str(dataset->length, dataset->filename);
 
+    if (strchr(filename, '/')) {
+        usb_mtp_queue_result(s, RES_PARAMETER_NOT_SUPPORTED, d->trans,
+                             0, 0, 0, 0);
+        return;
+    }
+
     o = usb_mtp_object_lookup_name(p, filename, dataset->length);
     if (o != NULL) {
         next_handle = o->handle;
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-3.1 v3 1/2] usb-mtp: fix utf16_to_str

[Peter Maydell]

On Mon, 3 Dec 2018 at 10:15, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Make utf16_to_str return an allocated string.  Remove the assumtion that
> the number of string bytes equals the number of utf16 chars (which is
> only true for ascii chars).  Instead call wcstombs twice, once to figure
> the storage size and once for the actual conversion (as suggested by the
> wcstombs manpage).
>
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH for-3.1 v3 1/2] usb-mtp: fix utf16_to_str

[Philippe Mathieu-Daudé]

On 3/12/18 11:10, Gerd Hoffmann wrote:
> Make utf16_to_str return an allocated string.  Remove the assumtion that
> the number of string bytes equals the number of utf16 chars (which is
> only true for ascii chars).  Instead call wcstombs twice, once to figure
> the storage size and once for the actual conversion (as suggested by the
> wcstombs manpage).
> 
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  hw/usb/dev-mtp.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 00a3691bae..0f6a9702ef 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1593,17 +1593,23 @@ static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p)
>      fprintf(stderr, "%s\n", __func__);
>  }
>  
> -static void utf16_to_str(uint8_t len, uint16_t *arr, char *name)
> +static char *utf16_to_str(uint8_t len, uint16_t *arr)
>  {
> -    int count;
> -    wchar_t *wstr = g_new0(wchar_t, len);
> +    wchar_t *wstr = g_new0(wchar_t, len + 1);
> +    int count, dlen;
> +    char *dest;
>  
>      for (count = 0; count < len; count++) {
> +        /* FIXME: not working for surrogate pairs */
>          wstr[count] = (wchar_t)arr[count];
>      }
> +    wstr[count] = 0;
>  
> -    wcstombs(name, wstr, len);
> +    dlen = wcstombs(NULL, wstr, 0) + 1;
> +    dest = g_malloc(dlen);
> +    wcstombs(dest, wstr, dlen);
>      g_free(wstr);
> +    return dest;
>  }
>  
>  /* Wrapper around write, returns 0 on failure */
> @@ -1703,7 +1709,7 @@ static void usb_mtp_write_metadata(MTPState *s)
>  {
>      MTPData *d = s->data_out;
>      ObjectInfo *dataset = (ObjectInfo *)d->data;
> -    char *filename = g_new0(char, dataset->length);
> +    char *filename;
>      MTPObject *o;
>      MTPObject *p = usb_mtp_object_lookup(s, s->dataset.parent_handle);
>      uint32_t next_handle = s->next_handle;
> @@ -1711,7 +1717,7 @@ static void usb_mtp_write_metadata(MTPState *s)
>      assert(!s->write_pending);
>      assert(p != NULL);
>  
> -    utf16_to_str(dataset->length, dataset->filename, filename);
> +    filename = utf16_to_str(dataset->length, dataset->filename);
>  
>      o = usb_mtp_object_lookup_name(p, filename, dataset->length);
>      if (o != NULL) {
> 

Re: [Qemu-devel] [PATCH for-3.1 v3 1/2] usb-mtp: fix utf16_to_str

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Make utf16_to_str return an allocated string.  Remove the assumtion that
> the number of string bytes equals the number of utf16 chars (which is
> only true for ascii chars).  Instead call wcstombs twice, once to figure
> the storage size and once for the actual conversion (as suggested by the
> wcstombs manpage).
>
> Reported-by: Michael Hanselmann <public@hansmi.ch>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 00a3691bae..0f6a9702ef 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -1593,17 +1593,23 @@ static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p)
>      fprintf(stderr, "%s\n", __func__);
>  }
>  
> -static void utf16_to_str(uint8_t len, uint16_t *arr, char *name)
> +static char *utf16_to_str(uint8_t len, uint16_t *arr)
>  {
> -    int count;
> -    wchar_t *wstr = g_new0(wchar_t, len);
> +    wchar_t *wstr = g_new0(wchar_t, len + 1);
> +    int count, dlen;
> +    char *dest;
>  
>      for (count = 0; count < len; count++) {
> +        /* FIXME: not working for surrogate pairs */

Please mention the FIXME in the commit message.

With that:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

>          wstr[count] = (wchar_t)arr[count];
>      }
> +    wstr[count] = 0;
>  
> -    wcstombs(name, wstr, len);
> +    dlen = wcstombs(NULL, wstr, 0) + 1;
> +    dest = g_malloc(dlen);
> +    wcstombs(dest, wstr, dlen);
>      g_free(wstr);
> +    return dest;
>  }
>  
>  /* Wrapper around write, returns 0 on failure */
[...]