On Mon, May 26, 2025 at 02:26:55PM +0000, Gerhard Roth wrote:
> On Mon, 2025-05-26 at 15:46 +0200, Claudio Jeker wrote:
> > On Mon, May 26, 2025 at 01:02:20PM +0000, Gerhard Roth wrote:
> > > Hi Pierre,
> > >
> > > thanks for your input. Your findings are correct. We shouldn't
> > > trust the values provided by a black-box USB device.
> > >
> > >
> > > On Fri, 2025-05-23 at 18:49 +0200, Pierre Pronchery wrote:
> > > > Dear OpenBSD team,
> > > >
> > > > I would like to bring your attention to the following bug report from
> > > > FreeBSD, where I have ported and imported the umb(4) driver recently:
> > > > https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=284904
> > > >
> > > > The bug report mentions:
> > > >
> > > > > in_len2mask(mask, len) will write as many as len/8 bytes:
> > > > >
> > > > > for (i = 0; i < len / 8; i++)
> > > > > p[i] = 0xff;
> > > > >
> > > > > len comes from a ipv4elem.prefixlen in a MBIM_CID_IP_CONFIGURATION
> > > > > message from the USB device, and can be any uint32_t value. So a
> > > > > broken
> > > > > or malicious USB device can cause a buffer overflow.
> > > >
> > > > I think that in reality, len comes from the network, which would make
> > > > the
> > > > issue marginally worse.
> > > >
> > > > Can you confirm the bug on your side, and would you have any suggestion
> > > > as to how to fix it properly?
> > > >
> > > > I would suggest to make len an unsigned value in umb_decode_cid() and
> > > > subsequent calls (infolen is unsigned in the first place there) but more
> > > > importantly, to verify that prefixlen is at most 32.
> > > >
> > > > Does that make sense?
> > > >
> > > > HTH,
> > > > --
> > > > khorben
> > >
> > > I would propose to limit 'prefixlen' to 32.
> > >
> > > Gerhard
> > >
> > > Index: sys/dev/usb/if_umb.c
> > > ===================================================================
> > > RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> > > diff -u -p -u -p -r1.59 if_umb.c
> > > --- sys/dev/usb/if_umb.c 8 Aug 2024 05:10:00 -0000 1.59
> > > +++ sys/dev/usb/if_umb.c 26 May 2025 11:37:15 -0000
> > > @@ -1838,7 +1838,8 @@ umb_add_inet_config(struct umb_softc *sc
> > > sin = &ifra.ifra_mask;
> > > sin->sin_family = AF_INET;
> > > sin->sin_len = sizeof (*sin);
> > > - in_len2mask(&sin->sin_addr, prefixlen);
> > > + in_len2mask(&sin->sin_addr,
> > > + MIN(prefixlen, sizeof (struct in_addr) * NBBY));
> > >
> > > rv = in_ioctl(SIOCAIFADDR, (caddr_t)&ifra, ifp, 1);
> > > if (rv != 0) {
> > >
> > >
> > >
> >
> > Please do not use NBBY and instead use 8 or just hardcode 32.
>
> Fine, updated patch below.
Maybe it would be better to fail hard in that case and return an error.
Diff below is OK claudio@
> Index: sys/dev/usb/if_umb.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
> diff -u -p -u -p -r1.59 if_umb.c
> --- sys/dev/usb/if_umb.c 8 Aug 2024 05:10:00 -0000 1.59
> +++ sys/dev/usb/if_umb.c 26 May 2025 14:25:34 -0000
> @@ -1838,7 +1838,8 @@ umb_add_inet_config(struct umb_softc *sc
> sin = &ifra.ifra_mask;
> sin->sin_family = AF_INET;
> sin->sin_len = sizeof (*sin);
> - in_len2mask(&sin->sin_addr, prefixlen);
> + in_len2mask(&sin->sin_addr,
> + MIN(prefixlen, sizeof (struct in_addr) * 8));
>
> rv = in_ioctl(SIOCAIFADDR, (caddr_t)&ifra, ifp, 1);
> if (rv != 0) {
>
>
--
:wq Claudio
