option parsing Reply-To: Bcc: Subject: Re: [PATCH 3/4] net: tftp: use bounded string compare for OACK option parsing Reply-To: In-Reply-To: <20260910192538.GO1764417@bill-the-cat>
On Thu, Sep 10, 2026 at 01:25:38PM -0600, Tom Rini wrote: > On Wed, Sep 09, 2026 at 12:20:15PM -0700, Sriram Sriram wrote: > > > From: Drew Kluemke <[email protected]> > > > > The TFTP OACK handler matches option names in the received packet with > > strcasecmp(), which scans until a NUL byte. A malformed or truncated > > OACK packet may not contain a NUL within the received length, causing > > an out-of-bounds read past the packet buffer. > > > > Replace each strcasecmp() with a strncasecmp() bounded to the option > > name length plus an explicit check that the name is NUL-terminated at > > the expected position. For the "windowsize" option, whose name and > > value extend beyond the loop's "i + 8 < len" guard, add an explicit > > "i + 11 < len" bound before the comparison. > > > > Signed-off-by: Drew Kluemke <[email protected]> > > Signed-off-by: Sriram Sriram <[email protected]> > > --- > > net/tftp.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > Does the spec/RFC ensure that we're being given a NULL terminated > string here? It certainly makes sense to be using strncasecmp here, > but since we're a size constrained bootloader, we do need to find the > right balance between defense-in-depth and potential exposure. Yes. RFC 2347 defines the OACK as +-------+---~~---+---+---~~---+---+---~~---+---+---~~---+---+ | opc | opt1 | 0 | value1 | 0 | optN | 0 | valueN | 0 | +-------+---~~---+---+---~~---+---+---~~---+---+---~~---+---+ so every option name and value is a NUL terminated netascii string, and the last byte of a well-formed OACK is always a NUL. What we do not have is any guarantee that the packet on the wire is well-formed -- nothing in tftp_handler() checks that before the strcasecmp()s run. On exposure: it is real but small. strcasecmp() bails at the first mismatching byte, so we only read past the packet when it ends part-way through something that matches an option name, and the over-read is then at most strlen("windowsize") + 1 == 11 bytes. Those bytes are normally stale data in the same 1536 byte net_rx_packets[] buffer (the receive buffers are one contiguous pool, net_rx_packets[i] = net_tx_packet + (i + 1) * PKTSIZE_ALIGN), so we only leave the allocation for a near-MTU OACK landing in the last rx buffer. The nastier case is the dectoul() right after a match: if the name and its NUL end exactly at the end of the packet, dectoul() starts past the end and runs until it hits a non-digit byte. v1 of this patch does not address that at all. So I think the balance you are asking about argues for a smaller patch than the one I sent. Since the RFC guarantees the trailing NUL, we can check for it once instead of bounding four comparisons: case TFTP_OACK: /* * RFC 2347 requires every option name and value to be * NUL terminated, so a well-formed OACK always ends in * a NUL. Verifying that once keeps the option parsing * below in bounds. */ if (!len || pkt[len - 1] != '\0') return; That leaves the existing strcasecmp()/dectoul() code untouched, covers the dectoul() calls as well as the name comparisons, and is cheaper. net/tftp.o .text on am335x_evm_defconfig (arm-linux-gnueabi-, with CONFIG_TFTP_TSIZE=y and CONFIG_TFTP_WINDOWSIZE=1): 3222 unpatched 3246 v1, strncasecmp (+24) 3238 trailing NUL check (+16) The one behaviour change is that a non-conforming OACK is dropped rather than half parsed, so we fall back to the retry path instead of ACKing with whatever the loop happened to pick up. I will send that as v2 unless you would rather not spend the 16 bytes at all, in which case I will drop this patch -- the other three in the series are independent of it. Thanks, Sriram
