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

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

[Gerd Hoffmann]

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

-- 
2.9.3

[Qemu-devel] [PATCH for-3.1 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 (hansmi.ch)
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/dev-mtp.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
index 00a3691bae..fbe1ace035 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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) {
-- 
2.9.3

Re: [Qemu-devel] [PATCH for-3.1 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 (hansmi.ch)
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  hw/usb/dev-mtp.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
> index 00a3691bae..fbe1ace035 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -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;
>  }

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.

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

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

[Bandan Das]

Markus Armbruster <armbru@redhat.com> writes:

> 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 (hansmi.ch)
>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>> ---
>>  hw/usb/dev-mtp.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c
>> index 00a3691bae..fbe1ace035 100644
>> --- a/hw/usb/dev-mtp.c
>> +++ b/hw/usb/dev-mtp.c
>> @@ -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;
>>  }
>
> 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.
>

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

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

[Qemu-devel] [PATCH for-3.1 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 (hansmi.ch)
Signed-off-by: Gerd Hoffmann <kraxel@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 fbe1ace035..87eeac1084 100644
--- a/hw/usb/dev-mtp.c
+++ b/hw/usb/dev-mtp.c
@@ -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;
-- 
2.9.3

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

[Philippe Mathieu-Daudé]

On 30/11/18 12:12, Gerd Hoffmann wrote:
> 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).

> Signed-off-by: Gerd Hoffmann <kraxel@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 fbe1ace035..87eeac1084 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -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é <philmd@redhat.com>

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

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

[Eric Blake]

On 11/30/18 1:08 PM, Philippe Mathieu-Daudé wrote:
> On 30/11/18 12:12, Gerd Hoffmann wrote:
>> 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?

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

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

[Philippe Mathieu-Daudé]

On 30/11/18 20:58, Eric Blake wrote:
> On 11/30/18 1:08 PM, Philippe Mathieu-Daudé wrote:
>> On 30/11/18 12:12, Gerd Hoffmann wrote:
>>> 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?
> 

I did not notice hehe :)

Per the gpg key: Michael Hanselmann <public@hansmi.ch>
Per git commits: Michael Hanselmann <hansmi@vshn.ch>

Cc'ed him so he can decide/confirm.

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

[Michael Hanselmann]

[-- Attachment #1: Type: text/plain, Size: 903 bytes --]

On 01.12.18 12:55, Philippe Mathieu-Daudé wrote:
> On 30/11/18 20:58, Eric Blake wrote:
>> On 11/30/18 1:08 PM, Philippe Mathieu-Daudé wrote:
>>> On 30/11/18 12:12, Gerd Hoffmann wrote:
>>>> 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?
>>
> 
> I did not notice hehe :)
> 
> Per the gpg key: Michael Hanselmann <public@hansmi.ch>
> Per git commits: Michael Hanselmann <hansmi@vshn.ch>

It'd be <public@hansmi.ch> for this one. Thanks for asking!

Best regards,
Michael

-- 
https://hansmi.ch/


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bandan Das]

Gerd Hoffmann <kraxel@redhat.com> writes:

> 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 <kraxel@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 fbe1ace035..87eeac1084 100644
> --- a/hw/usb/dev-mtp.c
> +++ b/hw/usb/dev-mtp.c
> @@ -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;

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

Bandan