Re: [EXT] wild pointer dereference in if_umb.c umb_decap()

[Gerhard Roth]

On Wed, 2025-06-04 at 19:11 +0200, Alexander Bluhm wrote:
> On Mon, May 26, 2025 at 01:05:51PM +0000, Gerhard Roth wrote:
> > Hi Pierre,
> > 
> > On Sun, 2025-05-25 at 12:44 +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=284920
> > > 
> > > The bug report mentions:
> > > 
> > > > When processing a message produced by a USB device, umb_decap()
> > > > says:
> > > > 
> > > > ptroff = UGETDW(hdr32->dwNdpIndex);
> > > > ...;
> > > > ptr16 = (struct ncm_pointer16 *)(buf + ptroff);
> > > > psig = UGETDW(ptr16->dwSignature);
> > > > 
> > > > But ptroff can be any 32-bit value, so the a broken or malicious USB
> > > > device can cause ptr16 to point outside the message buffer.
> > > 
> > > And:
> > > 
> > > > And later, umb_decap() pulls dlen and doff out of a message sent by
> > > > the USB device, and uses doff to form a pointer without a sanity
> > > > check:
> > > > 
> > > > dgram32 = (struct ncm_pointer32_dgram *)
> > > > (buf + ptroff + dgentryoff);
> > > > ...;
> > > > dlen = UGETDW(dgram32->dwDatagramLen);
> > > > doff = UGETDW(dgram32->dwDatagramIndex);
> > > > ...;
> > > > dp = buf + doff;
> > > > ...;
> > > > m = m_devget(dp, dlen, 0, ifp, NULL);
> > > > 
> > > > A malicious USB device could cause the wrong memory to be copied, or a
> > > > page fault.
> > > 
> > > From my reading of the current version of the umb(4) driver in OpenBSD,
> > > ISTM that you are vulnerable to this issue as well.
> > > 
> > > Thoughts?
> > > 
> > > HTH,
> > 
> > 64bit integers should prevent overlows in the check.
> > Also validate 'ptroff' before using it the first time.
> 
> I would prefer to have uint32_t variables for values coming from
> UGETW() and uint64_t for UGETDW().  Then I don't have to think about
> negative values and sign conversions in all the other comparions.
> 
> uint32_t ptrlen, dgentryoff;
> uint64_t blen, ptroff, dlen, doff;
> 
> bluhm
> 

Thanks for the review, updated diff below.


Index: sys/dev/usb/if_umb.c
===================================================================
RCS file: /cvs/src/sys/dev/usb/if_umb.c,v
diff -u -p -u -p -r1.61 if_umb.c
--- sys/dev/usb/if_umb.c        5 Jun 2025 08:22:25 -0000       1.61
+++ sys/dev/usb/if_umb.c        5 Jun 2025 11:54:00 -0000
@@ -2407,9 +2407,8 @@ umb_decap(struct umb_softc *sc, struct u
        struct ncm_pointer16_dgram *dgram16;
        struct ncm_pointer32_dgram *dgram32;
        uint32_t hsig, psig;
-       int      blen;
-       int      ptrlen, ptroff, dgentryoff;
-       uint32_t doff, dlen;
+       uint32_t ptrlen, dgentryoff;
+       uint64_t blen, ptroff, doff, dlen;
        struct mbuf_list ml = MBUF_LIST_INITIALIZER();
        struct mbuf *m;
 
@@ -2453,15 +2452,17 @@ umb_decap(struct umb_softc *sc, struct u
                goto fail;
        }
        if (blen != 0 && len < blen) {
-               DPRINTF("%s: bad NTB len (%d) for %d bytes of data\n",
+               DPRINTF("%s: bad NTB len (%llu) for %d bytes of data\n",
                    DEVNAM(sc), blen, len);
                goto fail;
        }
 
+       if (len < ptroff)
+               goto toosmall;
        ptr16 = (struct ncm_pointer16 *)(buf + ptroff);
        psig = UGETDW(ptr16->dwSignature);
        ptrlen = UGETW(ptr16->wLength);
-       if (len < ptrlen + ptroff)
+       if ((uint64_t)len < (uint64_t)ptrlen + ptroff)
                goto toosmall;
        if (!MBIM_NCM_NTH16_ISISG(psig) && !MBIM_NCM_NTH32_ISISG(psig)) {
                DPRINTF("%s: unsupported NCM pointer signature (0x%08x)\n",
@@ -2508,16 +2509,16 @@ umb_decap(struct umb_softc *sc, struct u
                /* Terminating zero entry */
                if (dlen == 0 || doff == 0)
                        break;
-               if (len < dlen + doff) {
+               if ((uint64_t)len < dlen + doff) {
                        /* Skip giant datagram but continue processing */
-                       DPRINTF("%s: datagram too large (%d @ off %d)\n",
+                       DPRINTF("%s: datagram too large (%llu @ off %llu)\n",
                            DEVNAM(sc), dlen, doff);
                        continue;
                }
 
                dp = buf + doff;
-               DPRINTFN(3, "%s: decap %d bytes\n", DEVNAM(sc), dlen);
-               m = m_devget(dp, dlen, sizeof(uint32_t));
+               DPRINTFN(3, "%s: decap %llu bytes\n", DEVNAM(sc), dlen);
+               m = m_devget(dp, (int)dlen, sizeof(uint32_t));
                if (m == NULL) {
                        ifp->if_iqdrops++;
                        continue;

smime.p7s
Description: S/MIME cryptographic signature