| Message ID | 20190415154503.6758-1-berrange@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb-mtp: fix ObjectInfo request handling | expand |
Daniel P. Berrangé <berrange@redhat.com> writes: > Two previous attempts to fix this due to GCC 9 highlighting > unaligned data access. My attempt: > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > And a previous one: > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > method handling the filename character set conversion. > > The 2nd patch in this series is a security flaw fix since the > code was not correctly validating guest provided data length. > > I've been unable to figure out how to exercise the codepath that > calls usb_mtp_write_metadata. At a guess, it looks like something > that should be called when writing to a file from a guest, but the > GNOME GVFS MTP driver doesn't provide write support. Using the > command line MTP tools "mtp-sendfile" command results in an > protocol error > > # mtp-sendfile foo eek.txt > libmtp version: 1.1.14 > The store is read only by default. Are you trying something like: -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? BTW, I also found a bug introduced by a recent commit which will return an incomplete transfer for smaller file sizes. > Device 0 (VID=46f4 and PID=0004) is UNKNOWN in libmtp v1.1.14. > Please report this VID/PID and the device model to the libmtp development team > PTP_ERROR_IO: failed to open session, trying again after resetting USB interface > LIBMTP libusb: Attempt to reset device > Sending foo to eek.txt > type: , 44 > Sending file... > > Error sending file. > Error 2: PTP Layer error 02ff: send_file_object_info(): Could not send object info. > Error 2: Error 02ff: PTP I/O Error > ERROR: Could not close session! > > And QEMU tracing show unexpected requests > > 26582@1555340076151600935 usb_mtp_command dev 4, code 0x9803, trans 0x18, args 0x11, 0xdc04, 0x0, 0x0, 0x0 > 26582@1555340076151619955 usb_mtp_xfer dev 4, ep 2, 20/20 > 26582@1555340076154138556 usb_mtp_data_in dev 4, trans 0x18, len 8 > 26582@1555340076154150689 usb_mtp_xfer dev 4, ep 1, 20/512 > 26582@1555340076156654311 usb_mtp_success dev 4, trans 0x18, args 0x0, 0x0 > 26582@1555340076156667764 usb_mtp_xfer dev 4, ep 1, 12/512 > 26582@1555340076159215930 usb_mtp_command dev 4, code 0x100c, trans 0x19, args 0x10001, 0xc, 0x0, 0x0, 0x0 > 26582@1555340076159229610 usb_mtp_xfer dev 4, ep 2, 20/20 > 26582@1555340076164166196 usb_mtp_stall dev 4, reason: awaiting data-out > 26582@1555340076167156367 usb_mtp_stall dev 4, reason: transaction inflight > 26582@1555340076170108336 usb_mtp_stall dev 4, reason: unknown control request > 26582@1555340076172606798 usb_mtp_stall dev 4, reason: unknown control request > > Perhaps a Windows guest can exercise this, but I don't have a modern > Windows install with MTP support. > > Thus this series is merely compile tested. > > Daniel P. Berrangé (3): > usb-mtp: fix string length for filename when writing metadata > usb-mtp: fix bounds check for guest provided filename > usb-mtp: fix alignment of access of ObjectInfo filename field > > hw/usb/dev-mtp.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-)
On Mon, Apr 15, 2019 at 12:52:41PM -0400, Bandan Das wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > Two previous attempts to fix this due to GCC 9 highlighting > > unaligned data access. My attempt: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > > > And a previous one: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > > method handling the filename character set conversion. > > > > The 2nd patch in this series is a security flaw fix since the > > code was not correctly validating guest provided data length. > > > > I've been unable to figure out how to exercise the codepath that > > calls usb_mtp_write_metadata. At a guess, it looks like something > > that should be called when writing to a file from a guest, but the > > GNOME GVFS MTP driver doesn't provide write support. Using the > > command line MTP tools "mtp-sendfile" command results in an > > protocol error > > > > # mtp-sendfile foo eek.txt > > libmtp version: 1.1.14 > > > > The store is read only by default. Are you trying something like: > -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? Ah ha, I didn't realize I had to enable write support explicitly. Will retry with that. Regards, Daniel
On 4/15/19 10:45 AM, Daniel P. Berrangé wrote: > Two previous attempts to fix this due to GCC 9 highlighting > unaligned data access. My attempt: > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > And a previous one: > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > method handling the filename character set conversion. > > The 2nd patch in this series is a security flaw fix since the > code was not correctly validating guest provided data length. Given that this is a security flaw, I've added this series to https://wiki.qemu.org/Planning/4.0 in case you're hoping to get it in -rc4.
On Mon, 15 Apr 2019 at 18:10, Eric Blake <eblake@redhat.com> wrote: > On 4/15/19 10:45 AM, Daniel P. Berrangé wrote: > > The 2nd patch in this series is a security flaw fix since the > > code was not correctly validating guest provided data length. > > Given that this is a security flaw, I've added this series to > https://wiki.qemu.org/Planning/4.0 in case you're hoping to get it in -rc4. What are the consequences of the flaw ? IIRC it's only one extra byte read? thanks -- PMM
On Mon, Apr 15, 2019 at 05:54:26PM +0100, Daniel P. Berrangé wrote: > On Mon, Apr 15, 2019 at 12:52:41PM -0400, Bandan Das wrote: > > Daniel P. Berrangé <berrange@redhat.com> writes: > > > > > Two previous attempts to fix this due to GCC 9 highlighting > > > unaligned data access. My attempt: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > > > > > And a previous one: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > > > > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > > > method handling the filename character set conversion. > > > > > > The 2nd patch in this series is a security flaw fix since the > > > code was not correctly validating guest provided data length. > > > > > > I've been unable to figure out how to exercise the codepath that > > > calls usb_mtp_write_metadata. At a guess, it looks like something > > > that should be called when writing to a file from a guest, but the > > > GNOME GVFS MTP driver doesn't provide write support. Using the > > > command line MTP tools "mtp-sendfile" command results in an > > > protocol error > > > > > > # mtp-sendfile foo eek.txt > > > libmtp version: 1.1.14 > > > > > > > The store is read only by default. Are you trying something like: > > -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? > > Ah ha, I didn't realize I had to enable write support explicitly. Will > retry with that. Even after setting readonly=false, I still can't get "mtp-sendfile" to succeed in a guest. Regards, Daniel
On Mon, Apr 15, 2019 at 06:18:01PM +0100, Peter Maydell wrote: > On Mon, 15 Apr 2019 at 18:10, Eric Blake <eblake@redhat.com> wrote: > > On 4/15/19 10:45 AM, Daniel P. Berrangé wrote: > > > The 2nd patch in this series is a security flaw fix since the > > > code was not correctly validating guest provided data length. > > > > Given that this is a security flaw, I've added this series to > > https://wiki.qemu.org/Planning/4.0 in case you're hoping to get it in -rc4. > > What are the consequences of the flaw ? IIRC it's only one > extra byte read? No, it is arbitrary extra bytes read. We have a USB packet N bytes in length, where N is supposed to match sizeof(ObjectInfo) + (ObjectInfo.length * 2) but we checked it against sizeof(ObjectInfo) + ObjectInfo.length As a result a malicious value for ObjectInfo.length can make QEMU can read ObjectInfo.length too many bytes when converting the string from UTF16 to UTF8. IOW, the returned UTF-8 string will end with whatever data is next on the heap. This in turn creates a filename on disk with this random data. I don't think this is a serious problem though, because if you have enabled write support you've already given the guest ermission to create arbitrary named files, so I don't see what they gain by trying to exploit this. We already outlaw "/" which is the main danger point in guest provided filenames. Regards, Daniel
On Mon, 15 Apr 2019 at 16:45, Daniel P. Berrangé <berrange@redhat.com> wrote: > > Two previous attempts to fix this due to GCC 9 highlighting > unaligned data access. My attempt: > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > And a previous one: > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > method handling the filename character set conversion. > > The 2nd patch in this series is a security flaw fix since the > code was not correctly validating guest provided data length. Given that we don't seem to be confident in this fix just now, and this is a read-only buffer overrun in a not-commonly-used feature that only happens if you explicitly enable write support, my current thought is that we should not try to put this into 4.0 (but instead treat it as we would a security issue that had occurred after we released 4.0). Opinions? Maybe we should just apply patch 2/3 for 4.0 ? thanks -- PMM
Daniel P. Berrangé <berrange@redhat.com> writes: ... >> > The store is read only by default. Are you trying something like: >> > -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? >> >> Ah ha, I didn't realize I had to enable write support explicitly. Will >> retry with that. > > Even after setting readonly=false, I still can't get "mtp-sendfile" > to succeed in a guest. > I posted a patch for a bug introduced by a recent commit that made smaller file sizes return back with a incomplete file transfer. https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02552.html > Regards, > Daniel
On Tue, Apr 16, 2019 at 12:10:16PM -0400, Bandan Das wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > ... > >> > The store is read only by default. Are you trying something like: > >> > -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? > >> > >> Ah ha, I didn't realize I had to enable write support explicitly. Will > >> retry with that. > > > > Even after setting readonly=false, I still can't get "mtp-sendfile" > > to succeed in a guest. > > > I posted a patch for a bug introduced by a recent commit that made smaller > file sizes return back with a incomplete file transfer. > > https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02552.html Yes, I applied that and didn't see any difference in behaviour Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Apr 16, 2019 at 12:10:16PM -0400, Bandan Das wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> ... >> >> > The store is read only by default. Are you trying something like: >> >> > -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? >> >> >> >> Ah ha, I didn't realize I had to enable write support explicitly. Will >> >> retry with that. >> > >> > Even after setting readonly=false, I still can't get "mtp-sendfile" >> > to succeed in a guest. >> > >> I posted a patch for a bug introduced by a recent commit that made smaller >> file sizes return back with a incomplete file transfer. >> >> https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02552.html > > Yes, I applied that and didn't see any difference in behaviour > Just noticed the error message you posted: Error sending file. Error 2: PTP Layer error 02ff: send_file_object_info(): Could not send object info. Error 2: Error 02ff: PTP I/O Error ERROR: Could not close session! I can't find usb-mtp sending a "I/O error" on an error condition for the objectinfo phase. It might be libmtp or even the command itself failing for some reason. For incomplete transfer, I just checked, it's spitting out the error message correctly as INCOMPLETE_FILE_TRANSFER. With libmtp version 1.13 on a FC24 guest, here's the output: $ mtp-sendfile test.txt test.img libmtp version: 1.1.13 Device 0 (VID=46f4 and PID=0004) is UNKNOWN in libmtp v1.1.13. Please report this VID/PID and the device model to the libmtp development team ignoring libusb_claim_interface() = -6PTP_ERROR_IO: failed to open session, trying again after resetting USB interface LIBMTP libusb: Attempt to reset device Sending test.txt to test.img type: txt, 44 Sending file... Progress: 322 of 322 (100%) New file ID: 7 What guest is this ? I can try to reproduce. > Regards, > Daniel
On Tue, Apr 16, 2019 at 12:45:04PM -0400, Bandan Das wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Tue, Apr 16, 2019 at 12:10:16PM -0400, Bandan Das wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > >> ... > >> >> > The store is read only by default. Are you trying something like: > >> >> > -device usb-mtp,rootdir=/code/mtpshare,readonly=false ? > >> >> > >> >> Ah ha, I didn't realize I had to enable write support explicitly. Will > >> >> retry with that. > >> > > >> > Even after setting readonly=false, I still can't get "mtp-sendfile" > >> > to succeed in a guest. > >> > > >> I posted a patch for a bug introduced by a recent commit that made smaller > >> file sizes return back with a incomplete file transfer. > >> > >> https://lists.gnu.org/archive/html/qemu-devel/2019-04/msg02552.html > > > > Yes, I applied that and didn't see any difference in behaviour > > > > Just noticed the error message you posted: > > Error sending file. > Error 2: PTP Layer error 02ff: send_file_object_info(): Could not send object info. > Error 2: Error 02ff: PTP I/O Error > ERROR: Could not close session! > > I can't find usb-mtp sending a "I/O error" on an error condition > for the objectinfo phase. It might be libmtp or even the command itself > failing for some reason. For incomplete transfer, I just checked, it's > spitting out the error message correctly as INCOMPLETE_FILE_TRANSFER. > > With libmtp version 1.13 on a FC24 guest, here's the output: > > $ mtp-sendfile test.txt test.img > libmtp version: 1.1.13 > > Device 0 (VID=46f4 and PID=0004) is UNKNOWN in libmtp v1.1.13. > Please report this VID/PID and the device model to the libmtp development team > ignoring libusb_claim_interface() = -6PTP_ERROR_IO: failed to open session, trying again after resetting USB interface > LIBMTP libusb: Attempt to reset device > Sending test.txt to test.img > type: txt, 44 > Sending file... > Progress: 322 of 322 (100%) > New file ID: 7 > > What guest is this ? I can try to reproduce. This was Fedora 26, so technically it is end of life, but then so is your F24 guest :-) Should really check with a modern guest like Fedora 29 or rawhide. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: >> I can't find usb-mtp sending a "I/O error" on an error condition >> for the objectinfo phase. It might be libmtp or even the command itself >> failing for some reason. For incomplete transfer, I just checked, it's >> spitting out the error message correctly as INCOMPLETE_FILE_TRANSFER. >> >> With libmtp version 1.13 on a FC24 guest, here's the output: >> >> $ mtp-sendfile test.txt test.img >> libmtp version: 1.1.13 >> >> Device 0 (VID=46f4 and PID=0004) is UNKNOWN in libmtp v1.1.13. >> Please report this VID/PID and the device model to the libmtp development team >> ignoring libusb_claim_interface() = -6PTP_ERROR_IO: failed to open session, trying again after resetting USB interface >> LIBMTP libusb: Attempt to reset device >> Sending test.txt to test.img >> type: txt, 44 >> Sending file... >> Progress: 322 of 322 (100%) >> New file ID: 7 >> >> What guest is this ? I can try to reproduce. > > This was Fedora 26, so technically it is end of life, but then so > is your F24 guest :-) > > Should really check with a modern guest like Fedora 29 or rawhide. > I had a F28 guest and here, mtp-sendfile appears to succeed but no file is written However, nautilus works fine, it doesn't appear like an issue with libmtp or Qemu. > Regards, > Daniel
On Tue, 16 Apr 2019 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Mon, 15 Apr 2019 at 16:45, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > Two previous attempts to fix this due to GCC 9 highlighting > > unaligned data access. My attempt: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > > > And a previous one: > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > > method handling the filename character set conversion. > > > > The 2nd patch in this series is a security flaw fix since the > > code was not correctly validating guest provided data length. > > Given that we don't seem to be confident in this fix just now, > and this is a read-only buffer overrun in a not-commonly-used > feature that only happens if you explicitly enable write support, > my current thought is that we should not try to put this into 4.0 > (but instead treat it as we would a security issue that had > occurred after we released 4.0). > > Opinions? Maybe we should just apply patch 2/3 for 4.0 ? Having thought a bit more I think I'd definitely like to apply just patch 2 for 4.0. Could people try to test that and confirm that it at least does not make the feature behave any worse? thanks -- PMM
On Tue, 16 Apr 2019 at 18:27, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 16 Apr 2019 at 14:35, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Mon, 15 Apr 2019 at 16:45, Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > Two previous attempts to fix this due to GCC 9 highlighting > > > unaligned data access. My attempt: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html > > > > > > And a previous one: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html > > > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html > > > > > > There are a number of bugs in the USB MTP usb_mtp_write_metadata > > > method handling the filename character set conversion. > > > > > > The 2nd patch in this series is a security flaw fix since the > > > code was not correctly validating guest provided data length. > > > > Given that we don't seem to be confident in this fix just now, > > and this is a read-only buffer overrun in a not-commonly-used > > feature that only happens if you explicitly enable write support, > > my current thought is that we should not try to put this into 4.0 > > (but instead treat it as we would a security issue that had > > occurred after we released 4.0). > > > > Opinions? Maybe we should just apply patch 2/3 for 4.0 ? > > Having thought a bit more I think I'd definitely like to apply > just patch 2 for 4.0. Could people try to test that and confirm > that it at least does not make the feature behave any worse? I've done a tentative merge test of patch 2, which is OK. I'd like to push that either today or tomorrow (uk time): objections? thanks -- PMM
On Tue, 16 Apr 2019 at 20:33, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 16 Apr 2019 at 18:27, Peter Maydell <peter.maydell@linaro.org> wrote: > > Having thought a bit more I think I'd definitely like to apply > > just patch 2 for 4.0. Could people try to test that and confirm > > that it at least does not make the feature behave any worse? > > I've done a tentative merge test of patch 2, which is OK. > I'd like to push that either today or tomorrow (uk time): > objections? ...now pushed. thanks -- PMM
On Tue, Apr 16, 2019 at 11:27:06PM +0100, Peter Maydell wrote: > On Tue, 16 Apr 2019 at 20:33, Peter Maydell <peter.maydell@linaro.org> wrote: > > > > On Tue, 16 Apr 2019 at 18:27, Peter Maydell <peter.maydell@linaro.org> wrote: > > > Having thought a bit more I think I'd definitely like to apply > > > just patch 2 for 4.0. Could people try to test that and confirm > > > that it at least does not make the feature behave any worse? > > > > I've done a tentative merge test of patch 2, which is OK. > > I'd like to push that either today or tomorrow (uk time): > > objections? > > ...now pushed. Added the other two to the (4.1) usb queue. thanks, Gerd
Two previous attempts to fix this due to GCC 9 highlighting unaligned data access. My attempt: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg07763.html And a previous one: https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg07923.html https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg00162.html There are a number of bugs in the USB MTP usb_mtp_write_metadata method handling the filename character set conversion. The 2nd patch in this series is a security flaw fix since the code was not correctly validating guest provided data length. I've been unable to figure out how to exercise the codepath that calls usb_mtp_write_metadata. At a guess, it looks like something that should be called when writing to a file from a guest, but the GNOME GVFS MTP driver doesn't provide write support. Using the command line MTP tools "mtp-sendfile" command results in an protocol error # mtp-sendfile foo eek.txt libmtp version: 1.1.14 Device 0 (VID=46f4 and PID=0004) is UNKNOWN in libmtp v1.1.14. Please report this VID/PID and the device model to the libmtp development team PTP_ERROR_IO: failed to open session, trying again after resetting USB interface LIBMTP libusb: Attempt to reset device Sending foo to eek.txt type: , 44 Sending file... Error sending file. Error 2: PTP Layer error 02ff: send_file_object_info(): Could not send object info. Error 2: Error 02ff: PTP I/O Error ERROR: Could not close session! And QEMU tracing show unexpected requests 26582@1555340076151600935 usb_mtp_command dev 4, code 0x9803, trans 0x18, args 0x11, 0xdc04, 0x0, 0x0, 0x0 26582@1555340076151619955 usb_mtp_xfer dev 4, ep 2, 20/20 26582@1555340076154138556 usb_mtp_data_in dev 4, trans 0x18, len 8 26582@1555340076154150689 usb_mtp_xfer dev 4, ep 1, 20/512 26582@1555340076156654311 usb_mtp_success dev 4, trans 0x18, args 0x0, 0x0 26582@1555340076156667764 usb_mtp_xfer dev 4, ep 1, 12/512 26582@1555340076159215930 usb_mtp_command dev 4, code 0x100c, trans 0x19, args 0x10001, 0xc, 0x0, 0x0, 0x0 26582@1555340076159229610 usb_mtp_xfer dev 4, ep 2, 20/20 26582@1555340076164166196 usb_mtp_stall dev 4, reason: awaiting data-out 26582@1555340076167156367 usb_mtp_stall dev 4, reason: transaction inflight 26582@1555340076170108336 usb_mtp_stall dev 4, reason: unknown control request 26582@1555340076172606798 usb_mtp_stall dev 4, reason: unknown control request Perhaps a Windows guest can exercise this, but I don't have a modern Windows install with MTP support. Thus this series is merely compile tested. Daniel P. Berrangé (3): usb-mtp: fix string length for filename when writing metadata usb-mtp: fix bounds check for guest provided filename usb-mtp: fix alignment of access of ObjectInfo filename field hw/usb/dev-mtp.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)