netdev - Re: [PATCH net-next 2/5] usbnet: ipheth: remove extraneous rx URB length check

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4730d2a2-00ee-4b80-a251-7591967b5f90@pen.gy>
Date: Sun, 8 Sep 2024 01:21:17 +0200
From: Foster Snowhill <forst@....gy>
To: Simon Horman <horms@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, Georgi Valkov <gvalkov@...il.com>,
 Oliver Neukum <oneukum@...e.com>, netdev@...r.kernel.org,
 linux-usb@...r.kernel.org
Subject: Re: [PATCH net-next 2/5] usbnet: ipheth: remove extraneous rx URB
 length check

Hello Simon,

Thank you very much for the feedback, and apologies for the delay.

On 2024-08-09 12:16, Simon Horman wrote:
> I am slightly concerned what happens if a frame that does not match the
> slightly stricter check in this patch, is now passed to
> dev->rcvbulk_callback().
> 
> I see that observations have been made that this does not happen.  But is
> there no was to inject malicious packets, or for something to malfunction?

Specifically for this patchset, in my opinion it shouldn't have had any
effect on dev->rcvbulk_callback(). The first thing that both of the
callbacks do is check the length of the incoming URB, to make sure they fit
the padding (for legacy callback) or the entirety of NTH16+NDP16 (for NCM).

However your message did prompt me to look at the code again with fresh
eyes, especially at the NCM implementation, and there is definitely need
for improvement in handling malicious URBs. I've submitted a patch a few
minutes ago [1] to address the issues I noticed.

What I think happened is I had two conflicting ideas in my head, one was
"make this generic enough to support arbitrary NCM header length and
location", and on the other hand "iOS has a very specific header size,
don't reimplement CDC NCM which already has a proper driver in cdc_ncm".
The implementation ended up somewhere in between, and resulted in code
that is susceptible to being negatively affected by malicious URBs.

With the latest patch [1] I decided that I should limit the NCM
implementation in ipheth to the iOS-specific URB payload format, and error
on anything else to be on the safe side. If there is ever need to make it
more generic (e.g. if iOS suddenly changes the URB payload structure),
a better idea could be to somehow reuse the existing code in the cdc_ncm
driver, which is a lot more careful in parsing incoming data. That would
possibly involve converting ipheth to use the usbnet framework.

[1]: https://lore.kernel.org/netdev/20240907230108.978355-1-forst@pen.gy/

Cheers,
Foster.