2022-11-23 13:28:38

by Greg Kroah-Hartman

[permalink] [raw]
Subject: [PATCH] USB: disable all RNDIS protocol drivers

The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
any system that uses it with untrusted hosts or devices. Because the
protocol is impossible to make secure, just disable all rndis drivers to
prevent anyone from using them again.

Windows only needed this for XP and newer systems, Windows systems older
than that can use the normal USB class protocols instead, which do not
have these problems.

Android has had this disabled for many years so there should not be any
real systems that still need this.

Cc: "David S. Miller" <[email protected]>
Cc: Eric Dumazet <[email protected]>
Cc: Jakub Kicinski <[email protected]>
Cc: Paolo Abeni <[email protected]>
Cc: Kalle Valo <[email protected]>
Cc: Oleksij Rempel <[email protected]>
Cc: "Maciej Żenczykowski" <[email protected]>
Cc: Neil Armstrong <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Andrzej Pietrasiewicz <[email protected]>
Cc: Jacopo Mondi <[email protected]>
Cc: "Łukasz Stelmach" <[email protected]>
Cc: Laurent Pinchart <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Reported-by: Ilja Van Sprundel <[email protected]>
Reported-by: Joseph Tartaro <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
Note, I'll submit patches removing the individual drivers for later, but
that is more complex as unwinding the interaction between the CDC
networking and RNDIS drivers is tricky. For now, let's just disable all
of this code as it is not secure.

I can take this through the USB tree if the networking maintainers have
no objection. I thought I had done this months ago, when the last round
of "there are bugs in the protocol!" reports happened at the end of
2021, but forgot to do so, my fault.

drivers/net/usb/Kconfig | 1 +
drivers/net/wireless/Kconfig | 1 +
drivers/usb/gadget/Kconfig | 4 +---
drivers/usb/gadget/legacy/Kconfig | 3 +++
4 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
index 4402eedb3d1a..83f9c0632642 100644
--- a/drivers/net/usb/Kconfig
+++ b/drivers/net/usb/Kconfig
@@ -401,6 +401,7 @@ config USB_NET_MCS7830
config USB_NET_RNDIS_HOST
tristate "Host for RNDIS and ActiveSync devices"
depends on USB_USBNET
+ depends on BROKEN
select USB_NET_CDCETHER
help
This option enables hosting "Remote NDIS" USB networking links,
diff --git a/drivers/net/wireless/Kconfig b/drivers/net/wireless/Kconfig
index cb1c15012dd0..f162b25123d7 100644
--- a/drivers/net/wireless/Kconfig
+++ b/drivers/net/wireless/Kconfig
@@ -81,6 +81,7 @@ config USB_NET_RNDIS_WLAN
tristate "Wireless RNDIS USB support"
depends on USB
depends on CFG80211
+ depends on BROKEN
select USB_NET_DRIVERS
select USB_USBNET
select USB_NET_CDCETHER
diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
index 4fa2ddf322b4..2c99d4313064 100644
--- a/drivers/usb/gadget/Kconfig
+++ b/drivers/usb/gadget/Kconfig
@@ -183,9 +183,6 @@ config USB_F_EEM
config USB_F_SUBSET
tristate

-config USB_F_RNDIS
- tristate
-
config USB_F_MASS_STORAGE
tristate

@@ -297,6 +294,7 @@ config USB_CONFIGFS_RNDIS
bool "RNDIS"
depends on USB_CONFIGFS
depends on NET
+ depends on BROKEN
select USB_U_ETHER
select USB_F_RNDIS
help
diff --git a/drivers/usb/gadget/legacy/Kconfig b/drivers/usb/gadget/legacy/Kconfig
index 0a7b382fbe27..03d6da63edf7 100644
--- a/drivers/usb/gadget/legacy/Kconfig
+++ b/drivers/usb/gadget/legacy/Kconfig
@@ -153,6 +153,7 @@ config USB_ETH
config USB_ETH_RNDIS
bool "RNDIS support"
depends on USB_ETH
+ depends on BROKEN
select USB_LIBCOMPOSITE
select USB_F_RNDIS
default y
@@ -247,6 +248,7 @@ config USB_FUNCTIONFS_ETH
config USB_FUNCTIONFS_RNDIS
bool "Include configuration with RNDIS (Ethernet)"
depends on USB_FUNCTIONFS && NET
+ depends on BROKEN
select USB_U_ETHER
select USB_F_RNDIS
help
@@ -427,6 +429,7 @@ config USB_G_MULTI
config USB_G_MULTI_RNDIS
bool "RNDIS + CDC Serial + Storage configuration"
depends on USB_G_MULTI
+ depends on BROKEN
select USB_F_RNDIS
default y
help
--
2.38.1


2022-11-23 14:25:02

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, 2022-11-23 at 13:46 +0100, Greg Kroah-Hartman wrote:
> The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> any system that uses it with untrusted hosts or devices. Because the
> protocol is impossible to make secure, just disable all rndis drivers to
> prevent anyone from using them again.
>

Not that I mind disabling these, but is there any more detail available
on this pretty broad claim? :)

johannes

2022-11-23 15:09:00

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, Nov 23, 2022 at 03:20:36PM +0100, Johannes Berg wrote:
> On Wed, 2022-11-23 at 13:46 +0100, Greg Kroah-Hartman wrote:
> > The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> > any system that uses it with untrusted hosts or devices. Because the
> > protocol is impossible to make secure, just disable all rndis drivers to
> > prevent anyone from using them again.
> >
>
> Not that I mind disabling these, but is there any more detail available
> on this pretty broad claim? :)

I don't want to get into specifics in public any more than the above.

The protocol was never designed to be used with untrusted devices. It
was created, and we implemented support for it, when we trusted USB
devices that we plugged into our systems, AND we trusted the systems we
plugged our USB devices into. So at the time, it kind of made sense to
create this, and the USB protocol class support that replaced it had not
yet been released.

As designed, it really can not work at all if you do not trust either
the host or the device, due to the way the protocol works. And I can't
see how it could be fixed if you wish to remain compliant with the
protocol (i.e. still work with Windows XP systems.)

Today, with untrusted hosts and devices, it's time to just retire this
protcol. As I mentioned in the patch comments, Android disabled this
many years ago in their devices, with no loss of functionality.

thanks,

greg k-h

2022-11-23 15:26:02

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

Greg Kroah-Hartman <[email protected]> writes:

> The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> any system that uses it with untrusted hosts or devices. Because the
> protocol is impossible to make secure, just disable all rndis drivers to
> prevent anyone from using them again.
>
> Windows only needed this for XP and newer systems, Windows systems older
> than that can use the normal USB class protocols instead, which do not
> have these problems.
>
> Android has had this disabled for many years so there should not be any
> real systems that still need this.
>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: Oleksij Rempel <[email protected]>
> Cc: "Maciej Żenczykowski" <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Andrzej Pietrasiewicz <[email protected]>
> Cc: Jacopo Mondi <[email protected]>
> Cc: "Łukasz Stelmach" <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Reported-by: Ilja Van Sprundel <[email protected]>
> Reported-by: Joseph Tartaro <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> Note, I'll submit patches removing the individual drivers for later, but
> that is more complex as unwinding the interaction between the CDC
> networking and RNDIS drivers is tricky. For now, let's just disable all
> of this code as it is not secure.
>
> I can take this through the USB tree if the networking maintainers have
> no objection. I thought I had done this months ago, when the last round
> of "there are bugs in the protocol!" reports happened at the end of
> 2021, but forgot to do so, my fault.
>
> drivers/net/usb/Kconfig | 1 +
> drivers/net/wireless/Kconfig | 1 +

For wireless:

Acked-by: Kalle Valo <[email protected]>

Feel free to take this via your tree.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

2022-11-23 15:44:16

by Nicolas Cavallari

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On 23/11/2022 13:46, Greg Kroah-Hartman wrote:
> The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> any system that uses it with untrusted hosts or devices. Because the
> protocol is impossible to make secure, just disable all rndis drivers to
> prevent anyone from using them again.
>
> Windows only needed this for XP and newer systems, Windows systems older
> than that can use the normal USB class protocols instead, which do not
> have these problems.
>
> Android has had this disabled for many years so there should not be any
> real systems that still need this.

I kind of disagree here. I have seen plenty of android devices that only
support rndis for connection sharing, including my android 11 phone
released in Q3 2020. I suspect the qualcomm's BSP still enable it by
default.

There are also probably cellular dongles that uses rndis by default.
Maybe ask the ModemManager people ?

I'm also curious if reimplementing it in userspace would solve the
security problem.

2022-11-23 16:01:34

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, Nov 23, 2022 at 04:40:33PM +0100, Nicolas Cavallari wrote:
> On 23/11/2022 13:46, Greg Kroah-Hartman wrote:
> > The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> > any system that uses it with untrusted hosts or devices. Because the
> > protocol is impossible to make secure, just disable all rndis drivers to
> > prevent anyone from using them again.
> >
> > Windows only needed this for XP and newer systems, Windows systems older
> > than that can use the normal USB class protocols instead, which do not
> > have these problems.
> >
> > Android has had this disabled for many years so there should not be any
> > real systems that still need this.
>
> I kind of disagree here. I have seen plenty of android devices that only
> support rndis for connection sharing, including my android 11 phone released
> in Q3 2020. I suspect the qualcomm's BSP still enable it by default.

Qualcomm should not have it enabled, and if they do, they are adding
code that Google says should not be enabled, and so Qualcom is
responsible for supporting that mess. Good luck to them.

> There are also probably cellular dongles that uses rndis by default. Maybe
> ask the ModemManager people ?

That would be very very sad if it were the case, as they are totally
unsafe.

> I'm also curious if reimplementing it in userspace would solve the security
> problem.

The kernel would be happier, as all of the buffer overflows that are
possible would only be happening in userspace. But I doubt any library
or userspace code that interacts with the protocol would really enjoy
it.

thanks,

greg k-h

2022-11-23 16:36:33

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, 2022-11-23 at 16:05 +0100, Greg Kroah-Hartman wrote:
> On Wed, Nov 23, 2022 at 03:20:36PM +0100, Johannes Berg wrote:
> > On Wed, 2022-11-23 at 13:46 +0100, Greg Kroah-Hartman wrote:
> > > The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> > > any system that uses it with untrusted hosts or devices. Because the
> > > protocol is impossible to make secure, just disable all rndis drivers to
> > > prevent anyone from using them again.
> > >
> >
> > Not that I mind disabling these, but is there any more detail available
> > on this pretty broad claim? :)
>
> I don't want to get into specifics in public any more than the above.

Fair.

> The protocol was never designed to be used with untrusted devices. It
> was created, and we implemented support for it, when we trusted USB
> devices that we plugged into our systems, AND we trusted the systems we
> plugged our USB devices into. So at the time, it kind of made sense to
> create this, and the USB protocol class support that replaced it had not
> yet been released.
>
> As designed, it really can not work at all if you do not trust either
> the host or the device, due to the way the protocol works. And I can't
> see how it could be fixed if you wish to remain compliant with the
> protocol (i.e. still work with Windows XP systems.)

I guess I just don't see how a USB-based protocol can be fundamentally
insecure (to the host), when the host is always in control over messages
and parses their content etc.?

I can see this with e.g. firewire which must allow DMA access, and now
with Thunderbolt we have the same and ended up with boltd, but USB?

> Today, with untrusted hosts and devices, it's time to just retire this
> protcol. As I mentioned in the patch comments, Android disabled this
> many years ago in their devices, with no loss of functionality.

I'm not sure Android counts that much, FWIW, at least for WiFi there
really is no good reason to plug in a USB WiFi dongle into an Android
phone, and quick googling shows that e.g. Android TV may - depending on
build - support/permit RNDIS Ethernet?

Anyway, there was probably exactly one RNDIS WiFi dongle from Broadcom
(for some kind of console IIRC), so it's not a huge loss. Just having
issues with the blanket statement that a USB protocol can be designed as
inscure :)

johannes

2022-11-23 18:31:04

by Jakub Kicinski

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, 23 Nov 2022 13:46:20 +0100 Greg Kroah-Hartman wrote:
> I can take this through the USB tree if the networking maintainers have
> no objection.

Acked-by: Jakub Kicinski <[email protected]>

2022-11-23 20:39:44

by Maciej Żenczykowski

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, Nov 23, 2022 at 4:46 AM Greg Kroah-Hartman
<[email protected]> wrote:
>
> The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> any system that uses it with untrusted hosts or devices. Because the
> protocol is impossible to make secure, just disable all rndis drivers to
> prevent anyone from using them again.
>
> Windows only needed this for XP and newer systems, Windows systems older
> than that can use the normal USB class protocols instead, which do not
> have these problems.
>
> Android has had this disabled for many years so there should not be any
> real systems that still need this.
>
> Cc: "David S. Miller" <[email protected]>
> Cc: Eric Dumazet <[email protected]>
> Cc: Jakub Kicinski <[email protected]>
> Cc: Paolo Abeni <[email protected]>
> Cc: Kalle Valo <[email protected]>
> Cc: Oleksij Rempel <[email protected]>
> Cc: "Maciej Żenczykowski" <[email protected]>
> Cc: Neil Armstrong <[email protected]>
> Cc: Mauro Carvalho Chehab <[email protected]>
> Cc: Andrzej Pietrasiewicz <[email protected]>
> Cc: Jacopo Mondi <[email protected]>
> Cc: "Łukasz Stelmach" <[email protected]>
> Cc: Laurent Pinchart <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Reported-by: Ilja Van Sprundel <[email protected]>
> Reported-by: Joseph Tartaro <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> Note, I'll submit patches removing the individual drivers for later, but
> that is more complex as unwinding the interaction between the CDC
> networking and RNDIS drivers is tricky. For now, let's just disable all
> of this code as it is not secure.
>
> I can take this through the USB tree if the networking maintainers have
> no objection. I thought I had done this months ago, when the last round
> of "there are bugs in the protocol!" reports happened at the end of
> 2021, but forgot to do so, my fault.
>
> drivers/net/usb/Kconfig | 1 +
> drivers/net/wireless/Kconfig | 1 +
> drivers/usb/gadget/Kconfig | 4 +---
> drivers/usb/gadget/legacy/Kconfig | 3 +++
> 4 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index 4402eedb3d1a..83f9c0632642 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
> @@ -401,6 +401,7 @@ config USB_NET_MCS7830
> config USB_NET_RNDIS_HOST
> tristate "Host for RNDIS and ActiveSync devices"
> depends on USB_USBNET
> + depends on BROKEN
> select USB_NET_CDCETHER
> help
> This option enables hosting "Remote NDIS" USB networking links,

NACK.

I'm perfectly okay with disabling the gadget (guest/client/device)
side rndis drivers.
New devices (ie. phones) moving to newer kernels should simply be
switching to the NCM gadget drivers.
Especially since AFAICT this won't land until 6.2 and thus will
presumably not be in the 6.1 LTS and thus won't even end up in next
year's Android 14/U,
and instead will only be present on the absolutely freshest Android
15/V devices launching near the end of 2024 (or really in early 2025).
Additionally the gadget side upstream RNDIS implementation simply
isn't used by some chipset vendors - like Qualcomm (which AFAIK uses
an out of tree driver to provide rndis gadget with IPA hardware
offload acceleration).

However, AFAICT this patch is also disabling *HOST* side RNDIS driver support.

ie. the RNDIS driver you'd use on a Linux laptop to usb tether off of
an Android phone.

AFAICT this will break usb tethering off of the *vast* majority of
Android phones - likely including most of those currently being
manufactured and sold.

The only Android phones I'm actually aware of that have switched to
NCM instead of RNDIS for usb tethering are Google Pixel 6+ (ie.
6/6pro/6a/7/7pro).
Though it's possible there might be some relatively new hardware from
other phone vendors that also uses NCM - I don't track this that
closely...
I do know Android 13/T doesn't require phones to use NCM for
tethering, and I've not heard of any plans to change that with Android
14/U either...

Note that NCM isn't natively supported by Windows <10 and it required
a fair bit of 'guts' on our side to drop support for usb tethering
Windows 8.1 devices prior to Win 8.1 EOL (which is only this coming
January).

Yes, AFAICT, this patch as currently written will break usb tethering
off of a Google Pixel ../3/4/5,
and I'd assume any and all qualcomm chipset derived devices, etc...

ie. most likely the first of these two and possibly the second are required:
CONFIG_USB_NET_RNDIS_HOST=m
CONFIG_USB_NET_RNDIS_WLAN=m

(AFAIK the rndis host side driver is also used by various cell dongles
and portable cell hotspots)

[I also don't understand the commit description where it talks about
Windows XP - how is XP relevant? AFAIK the issue is with Win<10 not
WinXP]

> diff --git a/drivers/net/wireless/Kconfig b/drivers/net/wireless/Kconfig
> index cb1c15012dd0..f162b25123d7 100644
> --- a/drivers/net/wireless/Kconfig
> +++ b/drivers/net/wireless/Kconfig
> @@ -81,6 +81,7 @@ config USB_NET_RNDIS_WLAN
> tristate "Wireless RNDIS USB support"
> depends on USB
> depends on CFG80211
> + depends on BROKEN
> select USB_NET_DRIVERS
> select USB_USBNET
> select USB_NET_CDCETHER
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 4fa2ddf322b4..2c99d4313064 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -183,9 +183,6 @@ config USB_F_EEM
> config USB_F_SUBSET
> tristate
>
> -config USB_F_RNDIS
> - tristate
> -
> config USB_F_MASS_STORAGE
> tristate
>
> @@ -297,6 +294,7 @@ config USB_CONFIGFS_RNDIS
> bool "RNDIS"
> depends on USB_CONFIGFS
> depends on NET
> + depends on BROKEN
> select USB_U_ETHER
> select USB_F_RNDIS
> help
> diff --git a/drivers/usb/gadget/legacy/Kconfig b/drivers/usb/gadget/legacy/Kconfig
> index 0a7b382fbe27..03d6da63edf7 100644
> --- a/drivers/usb/gadget/legacy/Kconfig
> +++ b/drivers/usb/gadget/legacy/Kconfig
> @@ -153,6 +153,7 @@ config USB_ETH
> config USB_ETH_RNDIS
> bool "RNDIS support"
> depends on USB_ETH
> + depends on BROKEN
> select USB_LIBCOMPOSITE
> select USB_F_RNDIS
> default y
> @@ -247,6 +248,7 @@ config USB_FUNCTIONFS_ETH
> config USB_FUNCTIONFS_RNDIS
> bool "Include configuration with RNDIS (Ethernet)"
> depends on USB_FUNCTIONFS && NET
> + depends on BROKEN
> select USB_U_ETHER
> select USB_F_RNDIS
> help
> @@ -427,6 +429,7 @@ config USB_G_MULTI
> config USB_G_MULTI_RNDIS
> bool "RNDIS + CDC Serial + Storage configuration"
> depends on USB_G_MULTI
> + depends on BROKEN
> select USB_F_RNDIS
> default y
> help
> --
> 2.38.1
>
Maciej Żenczykowski, Kernel Networking Developer @ Google

2022-11-24 01:16:44

by Lars Melin

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On 11/23/2022 22:40, Nicolas Cavallari wrote:
> There are also probably cellular dongles that uses rndis by default.

Yes, there is a whole bunch of them and new ones are still coming out.
Some USB dongle mfgr prefer to implement RNDIS instead of MBIM because
the same dongle can then be used for both old and new WIN versions.
I do agree that the RNDIS protocol is crap but removing RNDIS_HOST will
be a regression for many linux users.

/Lars

2022-11-29 22:50:46

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, 2022-11-23 at 16:40 +0100, Nicolas Cavallari wrote:
> On 23/11/2022 13:46, Greg Kroah-Hartman wrote:
> > The Microsoft RNDIS protocol is, as designed, insecure and
> > vulnerable on
> > any system that uses it with untrusted hosts or devices.  Because
> > the
> > protocol is impossible to make secure, just disable all rndis
> > drivers to
> > prevent anyone from using them again.
> >
> > Windows only needed this for XP and newer systems, Windows systems
> > older
> > than that can use the normal USB class protocols instead, which do
> > not
> > have these problems.
> >
> > Android has had this disabled for many years so there should not be
> > any
> > real systems that still need this.
>
> I kind of disagree here. I have seen plenty of android devices that
> only
> support rndis for connection sharing, including my android 11 phone
> released in Q3 2020. I suspect the qualcomm's BSP still enable it by
> default.
>
> There are also probably cellular dongles that uses rndis by default.
> Maybe ask the ModemManager people ?

Yes, there are.

Another class of WWAN dongles presented as USB RNDIS to the host, had
an onboard DHCP server, and "bridged" that (for lack of a better term)
to the WWAN. And like a home router exposed HTTP based management on
192.168.1.1 to control the WWAN stuff.

https://openwrt.org/docs/guide-user/network/wan/wwan/ethernetoverusb_rndis

RE Wifi, (echoing Johannes) there was one Broadcom chipset, but a bunch
of devices used it. I have some though I don't actively use them. But
they still work...

Dan

>
> I'm also curious if reimplementing it in userspace would solve the
> security problem.
>

2023-01-10 22:49:57

by James Hilliard

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Tue, Jan 10, 2023 at 3:32 PM Johannes Berg <[email protected]> wrote:
>
> On Wed, 2022-11-23 at 16:05 +0100, Greg Kroah-Hartman wrote:
> > On Wed, Nov 23, 2022 at 03:20:36PM +0100, Johannes Berg wrote:
> > > On Wed, 2022-11-23 at 13:46 +0100, Greg Kroah-Hartman wrote:
> > > > The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> > > > any system that uses it with untrusted hosts or devices. Because the
> > > > protocol is impossible to make secure, just disable all rndis drivers to
> > > > prevent anyone from using them again.
> > > >
> > >
> > > Not that I mind disabling these, but is there any more detail available
> > > on this pretty broad claim? :)
> >
> > I don't want to get into specifics in public any more than the above.
>
> Fair.

I would guess it's related to?:
https://github.com/torvalds/linux/commit/c7dd13805f8b8fc1ce3b6d40f6aff47e66b72ad2

>
> > The protocol was never designed to be used with untrusted devices. It
> > was created, and we implemented support for it, when we trusted USB
> > devices that we plugged into our systems, AND we trusted the systems we
> > plugged our USB devices into. So at the time, it kind of made sense to
> > create this, and the USB protocol class support that replaced it had not
> > yet been released.
> >
> > As designed, it really can not work at all if you do not trust either
> > the host or the device, due to the way the protocol works. And I can't
> > see how it could be fixed if you wish to remain compliant with the
> > protocol (i.e. still work with Windows XP systems.)

Can it be fixed in a way that most RNDIS based modems devices like
RNDIS based android tethering work with Linux based hosts still?

>
> I guess I just don't see how a USB-based protocol can be fundamentally
> insecure (to the host), when the host is always in control over messages
> and parses their content etc.?
>
> I can see this with e.g. firewire which must allow DMA access, and now
> with Thunderbolt we have the same and ended up with boltd, but USB?
>
> > Today, with untrusted hosts and devices, it's time to just retire this
> > protcol. As I mentioned in the patch comments, Android disabled this
> > many years ago in their devices, with no loss of functionality.
>
> I'm not sure Android counts that much, FWIW, at least for WiFi there
> really is no good reason to plug in a USB WiFi dongle into an Android
> phone, and quick googling shows that e.g. Android TV may - depending on
> build - support/permit RNDIS Ethernet?
>
> Anyway, there was probably exactly one RNDIS WiFi dongle from Broadcom
> (for some kind of console IIRC), so it's not a huge loss. Just having
> issues with the blanket statement that a USB protocol can be designed as
> inscure :)
>
> johannes
>

2023-01-11 13:49:14

by Jan Engelhardt

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers


On Wednesday 2022-11-23 13:46, Greg Kroah-Hartman wrote:
>
>The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
>any system that uses it with untrusted hosts or devices. Because the
>protocol is impossible to make secure, just disable all rndis drivers to
>prevent anyone from using them again.
>
>Windows only needed this for XP and newer systems, Windows systems older
>than that can use the normal USB class protocols instead, which do not
>have these problems.


In other news, someone just proposed adding "RNDIS" things to UEFI, so
now the security problem is added right back into machines but at
another layer?!

https://edk2.groups.io/g/devel/topic/patch_1_3/95531719

2023-01-11 15:02:12

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, Jan 11, 2023 at 02:38:04PM +0100, Jan Engelhardt wrote:
>
> On Wednesday 2022-11-23 13:46, Greg Kroah-Hartman wrote:
> >
> >The Microsoft RNDIS protocol is, as designed, insecure and vulnerable on
> >any system that uses it with untrusted hosts or devices. Because the
> >protocol is impossible to make secure, just disable all rndis drivers to
> >prevent anyone from using them again.
> >
> >Windows only needed this for XP and newer systems, Windows systems older
> >than that can use the normal USB class protocols instead, which do not
> >have these problems.
>
>
> In other news, someone just proposed adding "RNDIS" things to UEFI, so
> now the security problem is added right back into machines but at
> another layer?!
>
> https://edk2.groups.io/g/devel/topic/patch_1_3/95531719

I guess systems that use this will always have to trust that the device
plugged into them is "trusted". Seems like an easy way to get access to
a "locked down" system if you ever need it :)

{sigh}

greg k-h

2023-07-03 21:12:50

by Enrico Mioso

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

Hi all!!

I think the rndis_host USB driver might emit a warning in the dmesg, but disabling the driver wouldn't be a good idea.
The TP-Link MR6400 V1 LTE modem and also some ZTE modems integrated in routers do use this protocol.

We may also distinguish between these cases and devices you might plug in - as they pose different risk levels.

Enrico

2023-07-04 06:50:14

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Mon, Jul 03, 2023 at 11:11:57PM +0200, Enrico Mioso wrote:
> Hi all!!
>
> I think the rndis_host USB driver might emit a warning in the dmesg, but disabling the driver wouldn't be a good idea.
> The TP-Link MR6400 V1 LTE modem and also some ZTE modems integrated in routers do use this protocol.
>
> We may also distinguish between these cases and devices you might plug in - as they pose different risk levels.

Again, you have to fully trust the other side of an RNDIS connection,
any hints on how to have the kernel determine that?

thanks,

greg k-h

2023-07-12 09:26:02

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers



On 04.07.23 08:47, Greg Kroah-Hartman wrote:
> On Mon, Jul 03, 2023 at 11:11:57PM +0200, Enrico Mioso wrote:
>> Hi all!!
>>
>> I think the rndis_host USB driver might emit a warning in the dmesg, but disabling the driver wouldn't be a good idea.
>> The TP-Link MR6400 V1 LTE modem and also some ZTE modems integrated in routers do use this protocol.
>>
>> We may also distinguish between these cases and devices you might plug in - as they pose different risk levels.
>
> Again, you have to fully trust the other side of an RNDIS connection,
> any hints on how to have the kernel determine that?

Greg,

it is a network protocol. So this statement is kind of odd.
Are you saying that there are RNDIS messages that cannot be verified
for some reason, that still cannot be disclosed?

Regards
Oliver

2023-07-12 13:04:57

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, 2023-07-12 at 11:22 +0200, Oliver Neukum wrote:
>
> On 04.07.23 08:47, Greg Kroah-Hartman wrote:
> > On Mon, Jul 03, 2023 at 11:11:57PM +0200, Enrico Mioso wrote:
> > > Hi all!!
> > >
> > > I think the rndis_host USB driver might emit a warning in the dmesg, but disabling the driver wouldn't be a good idea.
> > > The TP-Link MR6400 V1 LTE modem and also some ZTE modems integrated in routers do use this protocol.
> > >
> > > We may also distinguish between these cases and devices you might plug in - as they pose different risk levels.
> >
> > Again, you have to fully trust the other side of an RNDIS connection,
> > any hints on how to have the kernel determine that?

> it is a network protocol. So this statement is kind of odd.
> Are you saying that there are RNDIS messages that cannot be verified
> for some reason, that still cannot be disclosed?

Agree, it's also just a USB device, so no special trickery with DMA,
shared buffers, etc.

I mean, yeah, the RNDIS code is really old and almost certainly has a
severe lack of input validation, but that still doesn't mean it's
fundamentally impossible.

johannes

2023-07-12 16:46:52

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Wed, Jul 12, 2023 at 03:00:55PM +0200, Johannes Berg wrote:
> On Wed, 2023-07-12 at 11:22 +0200, Oliver Neukum wrote:
> >
> > On 04.07.23 08:47, Greg Kroah-Hartman wrote:
> > > On Mon, Jul 03, 2023 at 11:11:57PM +0200, Enrico Mioso wrote:
> > > > Hi all!!
> > > >
> > > > I think the rndis_host USB driver might emit a warning in the dmesg, but disabling the driver wouldn't be a good idea.
> > > > The TP-Link MR6400 V1 LTE modem and also some ZTE modems integrated in routers do use this protocol.
> > > >
> > > > We may also distinguish between these cases and devices you might plug in - as they pose different risk levels.
> > >
> > > Again, you have to fully trust the other side of an RNDIS connection,
> > > any hints on how to have the kernel determine that?
>
> > it is a network protocol. So this statement is kind of odd.
> > Are you saying that there are RNDIS messages that cannot be verified
> > for some reason, that still cannot be disclosed?
>
> Agree, it's also just a USB device, so no special trickery with DMA,
> shared buffers, etc.
>
> I mean, yeah, the RNDIS code is really old and almost certainly has a
> severe lack of input validation, but that still doesn't mean it's
> fundamentally impossible.

You all are going to make me have to write some exploits aren't you...

Ok, I'll put it on my todo list and do it before submitting this patch
again.

thanks,

greg k-h

2023-07-13 05:29:21

by Mauro Carvalho Chehab

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

Em Tue, 4 Jul 2023 07:47:31 +0100
Greg Kroah-Hartman <[email protected]> escreveu:

> On Mon, Jul 03, 2023 at 11:11:57PM +0200, Enrico Mioso wrote:
> > Hi all!!
> >
> > I think the rndis_host USB driver might emit a warning in the dmesg, but disabling the driver wouldn't be a good idea.
> > The TP-Link MR6400 V1 LTE modem and also some ZTE modems integrated in routers do use this protocol.
> >
> > We may also distinguish between these cases and devices you might plug in - as they pose different risk levels.
>
> Again, you have to fully trust the other side of an RNDIS connection,
> any hints on how to have the kernel determine that?

Kernel may not know but the user does.

See, when doing a security risk assessment, one needs to evaluate the
risks, the costs to implement mitigation issues, and the measures that
will be taken. Sometimes, the measure is to just accept the risk, as
either the chances to actually happen on a particular scenario is
very unlikely, and/or the costs to mitigate are too high.

In any case, it should not be up to Kernel developers to do risk
assessment, as this has to be checked case by case.

For instance I usually disable several the security options on my
slow test devices, as the risk to run untrusted code on them
while I'm testing a new Kernel I just built is close to zero
and doesn't pay off the the extra hours I'll be wasting otherwise.

In the specific case of untrusted USB devices, the risk of having
USB untrusted sticks connected to my desktop machine is very low,
and if a criminal breaks into my house to be close enough to plug an
USB device, I would have a lot more to be concerned than just my PC.

Granted, the risk is higher on laptops and mobile devices, but
still it might be acceptable on some use cases.

Maybe a compromise would be to add a modprobe parameter and/or
a Kconfig option to allow enabling RDNIS host and RDNIS gadget
support at the security options to let the user select what
kind of risks he's willing to take.

Thanks,
Mauro

2023-07-13 08:48:46

by Oliver Neukum

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers



On 13.07.23 07:34, Greg Kroah-Hartman wrote:
> On Thu, Jul 13, 2023 at 02:28:26AM +0200, Johannes Berg wrote:
>> On Wed, 2023-07-12 at 18:39 +0200, Greg Kroah-Hartman wrote:

Hi,

>> All we said is that your statement of "RNDIS is fundamentally unfixable"
>> doesn't make a lot of sense. If this were the case, all USB drivers
>> would have to "trust the other side" as well, right?
>
> No, well, yes. See the zillion patches we have had to apply to the
> kernel over the years when someone decided that "usb devices are not to
> be trusted" that syzbot has helped find :)

Well, there are protocols that are in a sense unfixable. Like,
hypothetical example, you allow the execution of postscript code.
Hence it is kind of important to keep that distinction.

Yes, our attitude here is inconsistent. With the advent of Thunderbolt
we should have gone through all PCI drivers and audited them for things
malicious devices can do.
However, we can wait for Pandora for the purpose of this discussion.

> It's not a DMA issue here, it's a "the protocol allows for buffer
> overflows and does not seem to be able to be verified to prevent this"
> from what I remember (it's been a year since I looked at this last,
> details are hazy.) At the time, I didn't see a way that it could be
> fixed, hence this patch.

That makes sort of sense, but still leaves us with the option of verifying
each memcopy for being within allowed buffers.

Now, by no means let me stop you from getting into your supervillain outfit
and write exploits. But just telling us the rest of the issues would do, though
not as well.

Regards
Oliver

2023-07-13 12:46:34

by Johannes Berg

[permalink] [raw]
Subject: Re: [PATCH] USB: disable all RNDIS protocol drivers

On Thu, 2023-07-13 at 07:34 +0200, Greg Kroah-Hartman wrote:
> I wasn't trying to be glib here, sorry if it came across that way. I'll
> blame the heat...

No worries.

> > All we said is that your statement of "RNDIS is fundamentally unfixable"
> > doesn't make a lot of sense. If this were the case, all USB drivers
> > would have to "trust the other side" as well, right?
>
> No, well, yes. See the zillion patches we have had to apply to the
> kernel over the years when someone decided that "usb devices are not to
> be trusted" that syzbot has helped find :)

Sure, I'm well aware of that. But that's also exactly my point - nowhere
has anyone previously suggested that the protocol for any of those
devices is fundamentally broken and the drivers should be removed. We've
fixed those things and moved on.

I can even understand the initial reaction of "oh hey this ancient thing
is probably not used any more, let's just remove it", but even that's a
different reasoning, along the lines of "this has bugs and nobody needs
it". Though that nobody uses it has in fact been proven wrong, which is
pretty much why we're have this discussion at all.

> It's not a DMA issue here, it's a "the protocol allows for buffer
> overflows and does not seem to be able to be verified to prevent this"
> from what I remember (it's been a year since I looked at this last,
> details are hazy.)

If you s/be able to be verified/be verified in the code/ I entirely
believe it, in fact I think it's quite likely given the age of the code
and all. It's just that not being _able_ to verify it seems questionable
to me (and you haven't given any reasons), given that it's USB and you
always have a full buffer in hand when processing it, at a time where
the device can no longer modify it (IOW no TOCTTOU issues either.)

(As an aside, I've wondered about TOCTTOU with PCI, given that IOMMUs
can and will do lazy unmap ... but that's a different discussion.)


> At the time, I didn't see a way that it could be
> fixed, hence this patch.

Yeah I mean, the code isn't great, even if it's not _that_ much, but all
the likely() and things in there don't make it easy to read, and the
buffer size handling seems not immediately clear to me. So I probably
couldn't fix it quickly either, though I haven't even seen the reports.
Maciej seems to think it's fixable, at least. And yeah, we'd want to
actually review/audit that, I suppose.


So if you'd have said something like

Let's disable the RNDIS driver(s) because there are known exploits
there, nobody really knows how to fix this, and we need a short-term
solution until the issues are public and somebody steps up to fix and
maintain it.

I'd have much less of a problem with that. That's not _great_, but at
least it's honest and realistic. That could give us some time and maybe
then we can get the bug reports public once it's no longer an immediate
threat for all kernels, and go about fixing it with more time, maybe
eventually backporting fixes and reverting the disablement etc.

I guess this is why secret bug reports suck so much :-)

Thanks,
johannes