USB vulnerabilities

Dmitry,

Hi, I am an engineer at Cisco Systems, and this summer we tasked some
interns with performing USB fuzzing. One of the interns, Anirudh Bagde,
was able to crash the USB stack due to an error in the iforce module and
discovered a buffer over-read in the usbtouchscreen module.  Please see
the attachment for details.

Thank you,
Rosie Hall

Hi Rosie,

On Thu, Jul 28, 2016 at 12:23:09PM -0400, roswest wrote:

Dmitry,

Hi, I am an engineer at Cisco Systems, and this summer we tasked some
interns with performing USB fuzzing. One of the interns, Anirudh Bagde,
was able to crash the USB stack due to an error in the iforce module and
discovered a buffer over-read in the usbtouchscreen module.  Please see
the attachment for details.

Thank you for the report. Any chance Anirudh could provide patches to
fix these issues as well?

Thanks!

-- 
Dmitry

Dmitry,

Tomorrow is his last day, but he is going to try to accomplish that.

Rosie

On Thu Jul 28 2016 13:48:48 GMT-0400 (EDT), Dmitry Torokhov wrote:

Hi Rosie,

On Thu, Jul 28, 2016 at 12:23:09PM -0400, roswest wrote:

quoted

Dmitry,

Hi, I am an engineer at Cisco Systems, and this summer we tasked some
interns with performing USB fuzzing. One of the interns, Anirudh Bagde,
was able to crash the USB stack due to an error in the iforce module and
discovered a buffer over-read in the usbtouchscreen module.  Please see
the attachment for details.

Thank you for the report. Any chance Anirudh could provide patches to
fix these issues as well?

Thanks!

Dmitry,

Attached are the patches Anirudh created.  Also, I have added him to the
thread if you have any questions or comments for him.

Rosie

On Thu Jul 28 2016 16:58:27 GMT-0400 (EDT), roswest wrote:

Dmitry,

Tomorrow is his last day, but he is going to try to accomplish that.

Rosie

On Thu Jul 28 2016 13:48:48 GMT-0400 (EDT), Dmitry Torokhov wrote:

quoted

Hi Rosie,

On Thu, Jul 28, 2016 at 12:23:09PM -0400, roswest wrote:

quoted

Dmitry,

Hi, I am an engineer at Cisco Systems, and this summer we tasked some
interns with performing USB fuzzing. One of the interns, Anirudh Bagde,
was able to crash the USB stack due to an error in the iforce module and
discovered a buffer over-read in the usbtouchscreen module.  Please see
the attachment for details.

Thank you for the report. Any chance Anirudh could provide patches to
fix these issues as well?

Thanks!

Hi Rosie, Anirudh,

On Fri, Jul 29, 2016 at 08:48:01PM -0400, Rosie Hall wrote:

Dmitry,

Attached are the patches Anirudh created.  Also, I have added him to the
thread if you have any questions or comments for him.

Rosie

...

The iforce patch looks good, but I need "Signed-off-by" from Anirudh for
me to apply it. Please see Documentation/SubmittingPatches. 

 
-static int nexio_read_data(struct usbtouch_usb *usbtouch, unsigned char *pkt)
+static int nexio_read_data(struct usbtouch_usb *usbtouch, unsigned char *pkt,
+			   unsigned int len)

I do not think we need to pass the length to the readers: we know what
protocol we are dealing with and we can simply use NEXIO_BUFSIZE.

We have more adjustments to x_len and data_len below, so maybe we
should move these new checks there as well?
 

+
 	if (data_len > 0xff)
 		data_len -= 0x100;
 	if (x_len > 0xff)

Thanks.

-- 
Dmitry

Hi Dmitry,

Thanks for the input. I have added my sign off to the iforce patch.
I'm sending it as an attachment since I don't have my email client
properly setup yet for inline patches. Let me know if there is
anything else that needs to be done.

For the touchscreen patch, I'd forgotten about NEXIO_BUFSIZE, using
that is a good alternative to changing all the reader functions.
However there is still the chance that the packet byte array is
smaller than NEXIO_BUFSIZE so there can still be a buffer overflow,
though much less significant than before, and still relatively
harmless. I will update my patch to use NEXIO_BUFSIZE if you think
that is preferrable. AS for the other two adjustments, I'm not
actually sure what purpose they serve, so I thought I should leave
them alone, but I'll try to make it more concise. I will send the
updated patch soon once I get everything setup to test the patch on my
computer.

Thanks,
Anirudh Bagde
 --
Anirudh Bagde


On Sat, Jul 30, 2016 at 9:14 PM, Dmitry Torokhov
[off-list ref] wrote:

Hi Rosie, Anirudh,

On Fri, Jul 29, 2016 at 08:48:01PM -0400, Rosie Hall wrote:

quoted

Dmitry,

Attached are the patches Anirudh created.  Also, I have added him to the
thread if you have any questions or comments for him.

Rosie

...

quoted

--- a/drivers/input/joystick/iforce/iforce-usb.c      2016-07-29 15:02:47.602630504 -0400
+++ b/drivers/input/joystick/iforce/iforce-usb.c      2016-07-29 15:02:32.946812336 -0400

@@ -135,12 +135,23 @@
 {
      struct usb_device *dev = interface_to_usbdev(intf);
      struct usb_host_interface *interface;
-     struct usb_endpoint_descriptor *epirq, *epout;
+     struct usb_endpoint_descriptor *epirq = NULL, *epout = NULL;
      struct iforce *iforce;
-     int err = -ENOMEM;
+     int i, err = -ENOMEM;

      interface = intf->cur_altsetting;

+     for (i = 0; i < interface->desc.bNumEndpoints; i++) {
+             if (!epirq &&
+                 usb_endpoint_dir_in(&interface->endpoint[i].desc))
+                     epirq = &interface->endpoint[i].desc;
+             if (!epout &&
+                 usb_endpoint_dir_out(&interface->endpoint[i].desc))
+                     epout = &interface->endpoint[i].desc;
+     }
+     if (!epirq || !epout)
+             return -ENODEV;
+
      epirq = &interface->endpoint[0].desc;
      epout = &interface->endpoint[1].desc;

The iforce patch looks good, but I need "Signed-off-by" from Anirudh for
me to apply it. Please see Documentation/SubmittingPatches.

quoted

-static int nexio_read_data(struct usbtouch_usb *usbtouch, unsigned char *pkt)
+static int nexio_read_data(struct usbtouch_usb *usbtouch, unsigned char *pkt,
+                        unsigned int len)

I do not think we need to pass the length to the readers: we know what
protocol we are dealing with and we can simply use NEXIO_BUFSIZE.

quoted

 {
      struct nexio_touch_packet *packet = (void *) pkt;
      struct nexio_priv *priv = usbtouch->priv;

@@ -977,6 +996,11 @@
      if ((pkt[0] & 0xe0) != 0xe0)
              return 0;

+     if (data_len > len)
+             data_len = len;
+     if (x_len + y_len > data_len)
+             return 0;

We have more adjustments to x_len and data_len below, so maybe we
should move these new checks there as well?

quoted

+
      if (data_len > 0xff)
              data_len -= 0x100;
      if (x_len > 0xff)

Thanks.

--
Dmitry