Re: revisiting the issue of hardening the USB enumeration parser

[Alan Stern <stern@rowland.harvard.edu>]

On Thu, May 16, 2024 at 03:48:41PM +0200, Oliver Neukum wrote:
> Hi,
> 
> you convinced me that my last attempt to look at the parser
> was fundamentally flawed. Instead I went top down from parsing
> the configuration down to endpoints. I found one major issue.
> 
> static int find_next_descriptor(unsigned char *buffer, int size,
>     int dt1, int dt2, int *num_skipped)
> {
>         struct usb_descriptor_header *h;
>         int n = 0;
>         unsigned char *buffer0 = buffer;
>         /* Find the next descriptor of type dt1 or dt2 */
>         while (size > 0) {
>                 h = (struct usb_descriptor_header *) buffer;
> 
>                 if (h->bDescriptorType == dt1 || h->bDescriptorType == dt2)
>                         break;
>                 buffer += h->bLength;
>                 size -= h->bLength;
>                 ++n;
>         }
>         /* Store the number of descriptors skipped and return the
>          * number of bytes skipped */
>         if (num_skipped)
>                 *num_skipped = n;
>         return buffer - buffer0;
> }
> 
> This is called from multiple sites on chains of descriptors.
> We do have a check for overflowing the buffer in the while statement.
> However, there is no guarantee for make progress. If at some point
> in the chain we arrive at a descriptor of neither type we are looking
> for and a bLength of 0, size will remain constant and the loop
> will go on forever.
> 
> AFAICT this is guarded nowhere outside the function against.

You didn't notice this code in usb_parse_configuration() (starting 
around line 659):

		header = (struct usb_descriptor_header *) buffer2;
		if ((header->bLength > size2) || (header->bLength < 2)) {
			dev_notice(ddev, "config %d has an invalid descriptor "
			    "of length %d, skipping remainder of the config\n",
			    cfgno, header->bLength);
			break;
		}

The inner parentheses in the "if" condition aren't necessary, but the 
second half of the condition protects against zero-length descriptors.

> So how about the attached patch?

It's not necessary.

Alan Stern