← Writeups

The bounds check that couldn't: a length-desync over-read in libnfs

28 Jul 2026 libnfsnfsheap-overflowcwe-125parsing

Advisory: Heap out-of-bounds read in libnfs NFSv4 GETATTR fattr4 decode

TL;DR: libnfs’s NFSv4 GETATTR decoder tracked “how many bytes are left in this reply” in a counter, checked that counter before every field read — and then, at two spots, forgot to subtract the field it had just consumed. The counter drifted above the real remaining length, so the bounds checks kept passing while the read pointer walked off the end of the reply buffer: a heap out-of-bounds read driven by a malicious NFS server. The fix is one line, repeated twice. Merged as sahlberg/libnfs#593.

This is a nice bug to study because the code looks defensive — it bounds-checks everything — and is wrong anyway. The defect isn’t a missing check; it’s a bookkeeping error that quietly defeats the checks that are there.

The setup

A GETATTR reply carries a packed fattr4 attribute blob. libnfs walks it in nfs_parse_attributes() (lib/nfs_v4.c), advancing a buf pointer and tracking the bytes remaining in len. Before each read it uses a macro, CHECK_GETATTR_BUF_SPACE(len, n), to confirm at least n bytes remain:

#define CHECK_GETATTR_BUF_SPACE(len, size) \
    if (len < size) { /* ... error out ... */ }

So the invariant the whole function relies on is simple: len always equals the number of bytes left starting at buf. Every advance of buf must be matched by an equal decrement of len. Break that pairing and every subsequent check is comparing against a lie.

The break

The Owner and Group attributes are variable-length strings: a 4-byte length slen, then slen bytes of name, then padding to a 4-byte boundary. Here’s the Owner block (Group is identical):

CHECK_GETATTR_BUF_SPACE(len, 4);
slen = ntohl(*(uint32_t *)(void *)buf);
if (slen < 0) { return -1; }
buf += 4;
len -= 4;                              /* length field: paired. good. */
pad = (4 - (slen & 0x03)) & 0x03;
CHECK_GETATTR_BUF_SPACE(len, slen);    /* checks slen against len... */
st->nfs_uid = nfs_get_ugid(nfs, buf, slen, 1);
buf += slen;                           /* advance past the name...    */
                                       /* ...but len is NOT reduced by slen!  <-- */
CHECK_GETATTR_BUF_SPACE(len, pad);
buf += pad;
len -= pad;                            /* pad: paired. */

Look at the buf += slen line. buf moves forward by the whole string, but len is only ever decremented by 4 (the length field) and pad — never by slen itself. After the Owner and Group blocks, len over-reports the real remaining bytes by slen_owner + slen_group.

From that point on the invariant is broken. Every later CHECK_GETATTR_BUF_SPACE is comparing the requested size against an inflated len, so the checks pass even when buf has already run past the end of the reply. The trailing fixed-width attributes — rdev, space-used, the atime/ctime/mtime timestamps — are then read straight off the end of the heap allocation. A server that sends long owner/group strings and a short tail controls exactly how far past the buffer those reads land.

Why it’s easy to miss

The tell is that len is decremented in three places in each block (-= 4, -= pad, and the missing one) — so it looks maintained. Reading top to bottom, your eye pattern-matches “yep, len is being kept up to date.” The bug is an absence next to two present decrements, which is exactly the kind of thing code review glides over. The other attribute paths in the same function decrement correctly, so the desync is local to these two string fields — and a normal server sends short, well-formed strings where the drift never grows enough to matter.

The fix

Restore the invariant: subtract the consumed string length, matching the sibling paths.

         st->nfs_uid = nfs_get_ugid(nfs, buf, slen, 1);
         buf += slen;
+        len -= slen;
         CHECK_GETATTR_BUF_SPACE(len, pad);

One line, applied to both the Owner and Group blocks. With it, len tracks the true remaining bytes again and the existing checks do their job.

Verification (the part that matters before submitting a fix upstream): built current master with AddressSanitizer and a crafted GETATTR reply — heap out-of-bounds read before the change, clean after — and confirmed a valid reply still decodes. Both directions, so the fix is doing what it claims and not just suppressing the symptom.

Takeaways