[RFC]How else could a malicious device sabotage endpoints for usbnet

[RFC]How else could a malicious device sabotage endpoints for usbnet

[Oliver Neukum]

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

Hi,

I have checked for type, direction and number of endpoints.
But I keep thinking that I have overlooked a way to make broken
endpoint descriptors. Any suggestions?

    Regards
        Oliver


[-- Attachment #2: 0001-usbnet-sanity-check-for-endpoint-types.patch --]
[-- Type: text/x-patch, Size: 1967 bytes --]

From 853e421630f82fb3b7005ad0b294c091a064ac39 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Thu, 18 Nov 2021 18:15:03 +0100
Subject: [PATCH] usbnet: sanity check for endpoint types

A malicious device can pretend to be a device with a known
configuration of endpoints yet present endpoints of the wrong type
or too few or none at all.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/usbnet.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index 9a6450f796dc..b1f93810a6f3 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -91,6 +91,31 @@ static const char * const usbnet_event_names[] = {
 	[EVENT_NO_IP_ALIGN]	   = "EVENT_NO_IP_ALIGN",
 };
 
+bool usbnet_validate_endpoints(struct usbnet *dev, struct usb_interface *intf, const struct driver_info *info)
+{
+	struct usb_host_interface *alt = intf->cur_altsetting;
+	struct usb_host_endpoint *e;
+	int num_endpoints = alt->desc.bNumEndpoints;
+
+	if (info->in > num_endpoints)
+		return false;
+	e = alt->endpoint + info->in;
+	if (!e)
+		return false;
+	if (!usb_endpoint_is_bulk_in(&e->desc))
+		return false;
+
+	if (info->out > num_endpoints)
+		return false;
+	e = alt->endpoint + info->out;
+	if (!e)
+		return false;
+	if (!usb_endpoint_is_bulk_out(&e->desc))
+		return false;
+
+	return true;
+}
+
 /* handles CDC Ethernet and many other network "bulk data" interfaces */
 int usbnet_get_endpoints(struct usbnet *dev, struct usb_interface *intf)
 {
@@ -1772,6 +1797,8 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod)
 	} else if (!info->in || !info->out)
 		status = usbnet_get_endpoints (dev, udev);
 	else {
+		if (!usbnet_validate_endpoints(dev, udev, info))
+			goto out3;
 		dev->in = usb_rcvbulkpipe (xdev, info->in);
 		dev->out = usb_sndbulkpipe (xdev, info->out);
 		if (!(info->flags & FLAG_NO_SETINT))
-- 
2.26.2

Re: [RFC]How else could a malicious device sabotage endpoints for usbnet

[Greg KH]

On Thu, Dec 09, 2021 at 04:33:29PM +0100, Oliver Neukum wrote:
> Hi,
> 
> I have checked for type, direction and number of endpoints.
> But I keep thinking that I have overlooked a way to make broken
> endpoint descriptors. Any suggestions?
> 
>     Regards
>         Oliver
> 

> >From 853e421630f82fb3b7005ad0b294c091a064ac39 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oneukum@suse.com>
> Date: Thu, 18 Nov 2021 18:15:03 +0100
> Subject: [PATCH] usbnet: sanity check for endpoint types
> 
> A malicious device can pretend to be a device with a known
> configuration of endpoints yet present endpoints of the wrong type
> or too few or none at all.
> 
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>  drivers/net/usb/usbnet.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index 9a6450f796dc..b1f93810a6f3 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -91,6 +91,31 @@ static const char * const usbnet_event_names[] = {
>  	[EVENT_NO_IP_ALIGN]	   = "EVENT_NO_IP_ALIGN",
>  };
>  
> +bool usbnet_validate_endpoints(struct usbnet *dev, struct usb_interface *intf, const struct driver_info *info)
> +{
> +	struct usb_host_interface *alt = intf->cur_altsetting;
> +	struct usb_host_endpoint *e;
> +	int num_endpoints = alt->desc.bNumEndpoints;
> +
> +	if (info->in > num_endpoints)
> +		return false;
> +	e = alt->endpoint + info->in;
> +	if (!e)
> +		return false;
> +	if (!usb_endpoint_is_bulk_in(&e->desc))
> +		return false;
> +
> +	if (info->out > num_endpoints)
> +		return false;
> +	e = alt->endpoint + info->out;
> +	if (!e)
> +		return false;
> +	if (!usb_endpoint_is_bulk_out(&e->desc))
> +		return false;
> +
> +	return true;

Why not use usb_find_common_endpoints() and/or the other helper
functions instead? that's what they were created for.

thanks,

greg k-h

Re: [RFC]How else could a malicious device sabotage endpoints for usbnet

[Oliver Neukum]

On 09.12.21 16:47, Greg KH wrote:
>
> Why not use usb_find_common_endpoints() and/or the other helper
> functions instead? that's what they were created for.

Hi,

which one would I use? In this case I already know the endpoints
to be verified.

    Regards
        Oliver

Re: [RFC]How else could a malicious device sabotage endpoints for usbnet

[Greg KH]

On Wed, Dec 15, 2021 at 03:47:55PM +0100, Oliver Neukum wrote:
> 
> On 09.12.21 16:47, Greg KH wrote:
> >
> > Why not use usb_find_common_endpoints() and/or the other helper
> > functions instead? that's what they were created for.
> 
> Hi,
> 
> which one would I use? In this case I already know the endpoints
> to be verified.

I have no context here so I have no idea, sorry.

greg k-h

Re: [RFC]How else could a malicious device sabotage endpoints for usbnet

[Oliver Neukum]

On 15.12.21 15:57, Greg KH wrote:
> On Wed, Dec 15, 2021 at 03:47:55PM +0100, Oliver Neukum wrote:
>> On 09.12.21 16:47, Greg KH wrote:
>>> Why not use usb_find_common_endpoints() and/or the other helper
>>> functions instead? that's what they were created for.
>> Hi,
>>
>> which one would I use? In this case I already know the endpoints
>> to be verified.
> I have no context here so I have no idea, sorry.

usbnet has three ways to match the endpoints

1) the subdriver provides a method

2) a heuristic to find the endpoints is used (that should be converted
to the new API)

3) they are given nummerically by the subdriver

It turns out that #3 needs to verify the endpoints against malicious
devices.
So the following questions

a) should that verification go into usbcore
b) what possible ways for a malicious device to spoof us can you come
up with

    Regards
        Oliver

Re: [RFC]How else could a malicious device sabotage endpoints for usbnet

[Greg KH]

On Thu, Dec 16, 2021 at 11:16:26AM +0100, Oliver Neukum wrote:
> 
> On 15.12.21 15:57, Greg KH wrote:
> > On Wed, Dec 15, 2021 at 03:47:55PM +0100, Oliver Neukum wrote:
> >> On 09.12.21 16:47, Greg KH wrote:
> >>> Why not use usb_find_common_endpoints() and/or the other helper
> >>> functions instead? that's what they were created for.
> >> Hi,
> >>
> >> which one would I use? In this case I already know the endpoints
> >> to be verified.
> > I have no context here so I have no idea, sorry.
> 
> usbnet has three ways to match the endpoints
> 
> 1) the subdriver provides a method
> 
> 2) a heuristic to find the endpoints is used (that should be converted
> to the new API)
> 
> 3) they are given nummerically by the subdriver
> 
> It turns out that #3 needs to verify the endpoints against malicious
> devices.
> So the following questions
> 
> a) should that verification go into usbcore

the usb_find_common_endpoints() functions are in the usbcore for drivers
to use for this type of problem.

> b) what possible ways for a malicious device to spoof us can you come
> up with

Start with:
	- invalid endpoint sizes and types
	- invalid data being sent on valid endpoint types
and you will catch almost all possible errors.

thanks,

greg k-h

Re: [RFC]How else could a malicious device sabotage endpoints for usbnet

[Oliver Neukum]

On 21.12.21 08:54, Greg KH wrote:
> On Thu, Dec 16, 2021 at 11:16:26AM +0100, Oliver Neukum wrote:
>>
>> 2) a heuristic to find the endpoints is used (that should be converted
>> to the new API)
>>
>> 3) they are given nummerically by the subdriver
>>
>> It turns out that #3 needs to verify the endpoints against malicious
>> devices.
>> So the following questions
>>
>> a) should that verification go into usbcore
> the usb_find_common_endpoints() functions are in the usbcore for drivers
> to use for this type of problem.
That API insist on finding the endpoints. It is a heuristic, so we need
to have a fallback in case it fails.
>> b) what possible ways for a malicious device to spoof us can you come
>> up with
> Start with:
> 	- invalid endpoint sizes and types
> 	- invalid data being sent on valid endpoint types
> and you will catch almost all possible errors.
>
OK. But I still need a way to do verification _only_.

    Regards
        Oliver