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

PATCH Superseded Gerd Hoffmann 6 participants 10 messages Nov 30, 2018 – Dec 1, 2018

lore

Share & tools

lore

Gerd Hoffmann

author

2018-11-30 11:12 UTC #0 lore raw

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

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

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

Signature · 1 line

2.9.3

Gerd Hoffmann

author

2018-11-30 11:12 UTC #1 lore raw

PATCH 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 (hansmi.ch)

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

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

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

index 00a3691bae..fbe1ace035 100644
@@ -1593,17 +1593,22 @@ 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++) {
         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 +1708,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 +1716,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

Markus Armbruster

2018-11-30 13:14 UTC #2 lore raw

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

Quoted 31 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 (hansmi.ch)
Signed-off-by Gerd Hoffmann <[email protected]>
---
 hw/usb/dev-mtp.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
hw/usb/dev-mtp.c +4 -3
index 00a3691bae..fbe1ace035 100644
@@ -1593,17 +1593,22 @@ 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++) {
         wstr[count] = (wchar_t)arr[count];

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

Quoted 10 lines

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

     }
+    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;
 }

Preexisting: casting uint16_t to wchar_t is iffy for at least two reasons:

* When wchar_t is UCS-4, things fall apart for surrogate pairs. For instance, the surrogate pair

      uint16_t arr = { 0xD834, 0xDD1E };

should map to the single wchar_t 0x1D11E.

* wchar_t needn't even be Unicode. It might well be on any host we care for, but are you *sure*?

I guess g_utf16_to_utf8() would be differently wrong: it converts to UTF-8, but we need to convert to the current locale's multibyte string.

Quoted 20 lines

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

 /* Wrapper around write, returns 0 on failure */

@@ -1703,7 +1708,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 +1716,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) {

Bandan Das

2018-11-30 19:58 UTC #3 lore raw

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

Quoted 52 lines · Markus Armbruster

Quoted 31 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 (hansmi.ch)
Signed-off-by Gerd Hoffmann <[email protected]>
---
 hw/usb/dev-mtp.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)
hw/usb/dev-mtp.c +4 -3
index 00a3691bae..fbe1ace035 100644
@@ -1593,17 +1593,22 @@ 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++) {
         wstr[count] = (wchar_t)arr[count];
hw/usb/dev-mtp.c static void usb_mtp_cancel_packet(USBDevice *dev, USBPacket *p) · line 1612
Quoted 10 lines
hw/usb/dev-mtp.c +5 -1
     }
+    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;
 }
Preexisting: casting uint16_t to wchar_t is iffy for at least two reasons:
* When wchar_t is UCS-4, things fall apart for surrogate pairs. For instance, the surrogate pair
      uint16_t arr = { 0xD834, 0xDD1E };

Quoted 8 lines

should map to the single wchar_t 0x1D11E.
* wchar_t needn't even be Unicode. It might well be on any host we care for, but are you *sure*?
I guess g_utf16_to_utf8() would be differently wrong: it converts to UTF-8, but we need to convert to the current locale's multibyte string.

I couldn't find an existing function that I could safely reuse which was my first preference. I will take a look at how to make this function better, maybe, even see what other MTP implementations are doing in this regard.

Bandan

Quoted 20 lines

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

 /* Wrapper around write, returns 0 on failure */

@@ -1703,7 +1708,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 +1716,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) {

Gerd Hoffmann

author

2018-11-30 11:12 UTC #4 lore raw

PATCH 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 (hansmi.ch)

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

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

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

index fbe1ace035..87eeac1084 100644
@@ -1718,6 +1718,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_INVALID_PARAMETER, 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

Philippe Mathieu-Daudé

Reviewed-by on v1 2/2

2018-11-30 19:09 UTC #5 lore raw

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

Quoted 5 lines · 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 (hansmi.ch)

It's common for scripts to match '<email>', can you write this one as Michael Hanselmann <hansmi.ch>?

(Same suggestion applies for patch 1/2).

Quoted 10 lines

Signed-off-by Gerd Hoffmann <[email protected]>
---
 hw/usb/dev-mtp.c | 6 ++++++
 1 file changed, 6 insertions(+)
hw/usb/dev-mtp.c +0 -0
index fbe1ace035..87eeac1084 100644

hw/usb/dev-mtp.c static void usb_mtp_write_metadata(MTPState *s) · line 1725

Quoted 8 lines

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

@@ -1718,6 +1718,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_INVALID_PARAMETER, d->trans,
+                             0, 0, 0, 0);
+        return;
+    }

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

Quoted 5 lines

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

+
     o = usb_mtp_object_lookup_name(p, filename, dataset->length);
     if (o != NULL) {
         next_handle = o->handle;

Eric Blake

2018-11-30 19:58 UTC #6 lore raw

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

Quoted 9 lines · Philippe Mathieu-Daudé

Quoted 5 lines · 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 (hansmi.ch)
It's common for scripts to match '<email>', can you write this one as Michael Hanselmann <hansmi.ch>?

That's not an email address, though. Do we have an email for Michael, or just a username?

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Philippe Mathieu-Daudé

2018-12-01 11:55 UTC #7 lore raw

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

Quoted 5 lines · Eric Blake

Quoted 4 lines · Philippe Mathieu-Daudé
Quoted 3 lines · Gerd Hoffmann
Slash is unix directory separator, so they are not allowed in filenames. Note this also stops the classic escape via "../".

Quoted 9 lines

Quoted 5 lines
Quoted 2 lines
Fixes CVE-2018-16867
Reported-by Michael Hanselmann (hansmi.ch)
It's common for scripts to match '<email>', can you write this one as Michael Hanselmann <hansmi.ch>?
That's not an email address, though. Do we have an email for Michael, or just a username?

I did not notice hehe :)

Per the gpg key: Michael Hanselmann <[email protected]> Per git commits: Michael Hanselmann <[email protected]>

Cc'ed him so he can decide/confirm.

Michael Hanselmann

2018-12-01 13:50 UTC #8 lore raw

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

Quoted 12 lines · Philippe Mathieu-Daudé

Quoted 11 lines · Eric Blake
Quoted 9 lines · Philippe Mathieu-Daudé
Quoted 5 lines · 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 (hansmi.ch)
It's common for scripts to match '<email>', can you write this one as Michael Hanselmann <hansmi.ch>?

Quoted 8 lines

Quoted 3 lines
That's not an email address, though. Do we have an email for Michael, or just a username?
I did not notice hehe :)
Per the gpg key: Michael Hanselmann <[email protected]> Per git commits: Michael Hanselmann <[email protected]>

It'd be <[email protected]> for this one. Thanks for asking!

Best regards,
Michael

Signature · 1 line

https://hansmi.ch/

Bandan Das

2018-11-30 20:08 UTC #9 lore raw

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

In reply to Gerd Hoffmann

Quoted 17 lines · 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 (hansmi.ch)
Signed-off-by Gerd Hoffmann <[email protected]>
---
 hw/usb/dev-mtp.c | 6 ++++++
 1 file changed, 6 insertions(+)
hw/usb/dev-mtp.c +0 -0
index fbe1ace035..87eeac1084 100644
@@ -1718,6 +1718,12 @@ static void usb_mtp_write_metadata(MTPState *s)
 
     filename = utf16_to_str(dataset->length, dataset->filename);

hw/usb/dev-mtp.c static void usb_mtp_write_metadata(MTPState *s) · line 1729

Quoted 10 lines

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

 
+    if (strchr(filename, '/')) {
+        usb_mtp_queue_result(s, RES_INVALID_PARAMETER, d->trans,
+                             0, 0, 0, 0);
+        return;
+    }
+
     o = usb_mtp_object_lookup_name(p, filename, dataset->length);
     if (o != NULL) {
         next_handle = o->handle;

Should we return PARAMETER_NOT_SUPPORTED instead of INVALID_PARAMETER ? That's a valid response code for SendObjectInfo acc to the spec.

Bandan