usb-mtp: two bugfixes (one security fix).

PATCH In review Gerd Hoffmann 4 participants 6 messages Dec 3, 2018

Share & tools

Gerd Hoffmann

author

2018-12-03 10:10 UTC #0 lore raw

v3 cover usb-mtp: two bugfixes (one security fix).

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(-)

Signature · 1 line

2.9.3

Gerd Hoffmann

author

2018-12-03 10:10 UTC #1 lore raw

PATCH v3 1/2 usb-mtp: fix utf16_to_str

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 <[email protected]>

Signed-off-by Gerd Hoffmann <[email protected]>

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

hw/usb/dev-mtp.c +12 -6

index 00a3691bae..0f6a9702ef 100644
@@ -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) {

Signature · 1 line

2.9.3

Peter Maydell

Reviewed-by on v3 1/2

2018-12-03 14:59 UTC #2 lore raw

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

Quoted 7 lines · 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).

Quoted 5 lines

Reported-by Michael Hanselmann <[email protected]>
Signed-off-by Gerd Hoffmann <[email protected]>
---
 hw/usb/dev-mtp.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Reviewed-by Peter Maydell <[email protected]>

thanks
-- PMM

Philippe Mathieu-Daudé

Reviewed-by on v3 1/2

2018-12-03 16:12 UTC #3 lore raw

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

In reply to Gerd Hoffmann

Quoted 8 lines · 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 <[email protected]>
Signed-off-by Gerd Hoffmann <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Quoted 55 lines

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

hw/usb/dev-mtp.c +12 -6

index 00a3691bae..0f6a9702ef 100644
@@ -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) {

Markus Armbruster

Reviewed-by on v3 1/2

2018-12-03 18:10 UTC #4 lore raw

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

In reply to Gerd Hoffmann

Quoted 21 lines · 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 <[email protected]>
Signed-off-by Gerd Hoffmann <[email protected]>
---
 hw/usb/dev-mtp.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
hw/usb/dev-mtp.c +0 -1
index 00a3691bae..0f6a9702ef 100644
@@ -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)

hw/usb/dev-mtp.c static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p) · line 1603

Quoted 10 lines

hw/usb/dev-mtp.c +5 -2

+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 <[email protected]>

Quoted 13 lines

hw/usb/dev-mtp.c +5 -1

         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 */

[...]

Gerd Hoffmann

author

2018-12-03 10:10 UTC #5 lore raw

PATCH v3 2/2 usb-mtp: outlaw slashes in filenames

In reply to 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 <[email protected]>

Signed-off-by Gerd Hoffmann <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

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

hw/usb/dev-mtp.c +6 -0

index 0f6a9702ef..100b7171f4 100644
@@ -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;

Signature · 1 line

2.9.3