Cole Munz

build log · embedded and firmware security

An overread GNU cpio didn't have (yet)

2026-08-29

GNU cpio reads tar archives too, and its tar-reading code lives in src/tar.c. A ustar header's uname and gname fields are 32 bytes, fixed size, and the format does not require them to be NUL-terminated inside a conforming archive. read_in_tar_header passed those 32 bytes straight to getuidbyname and getgidbyname, which hand them to getpwnam/getgrnam as C strings. Feed it a header where uname, gname, and the bytes after them in the 512-byte record are all non-NUL, and the lookup walks off the end of a stack buffer looking for a terminator that isn't there. Reachable with an ordinary cpio -itv on a crafted archive.

I sent a small patch: copy the fields into local buffers sized one byte larger than the record field, terminate the copy, look those up instead of the raw pointer into the header. Ten lines changed, under the FSF's tiny-change threshold, so no copyright assignment paperwork was needed to send it.

The rejection

Maintainer Sergey Poznyakoff wrote back and pushed back on the framing, not the fix: uname and gname are NUL-terminated per POSIX for a conforming ustar archive, so in his reading there was no bug to fix, just defense against something the format already rules out.

He is right about that. POSIX ustar does define the fields that way for an archive that follows the spec. The mistake would have been arguing the POSIX point, because I would have lost it and been correct to lose it.

The reframe

The patch was never about conforming archives. It was about what cpio does when it is handed one that doesn't conform, and cpio is exactly the kind of tool that gets pointed at archives nobody has vouched for. I checked HEAD before replying, because a rejection you haven't re-verified is just a maintainer's opinion, not a fact: the read side was still unguarded, unchanged from what I'd found. Then I went looking for how Sergey himself had handled the identical shape of problem before, and found it: commit e8931bebb9e1, his own fix to the write side of the same code, bounding a strcpy into tar_hdr->uname/tar_hdr->gname with a helper that truncates to the field size:

static void
tarnamecpy (char *buf, char const *name, size_t size)
{
  strncpy (buf, name, size-1);
  buf[size-1] = 0;
}

That fix does not lean on POSIX either. It bounds a copy against malformed or oversized input because bounding it costs nothing and leaving it unbounded crashes. The read side had the mirror-image problem and none of the mirror-image fix. I wrote back conceding the POSIX point exactly as he'd stated it, then reframed around malformed input, and pointed at his own write-side commit as precedent for treating this as worth fixing regardless of what a conforming archive guarantees.

What came back

Sergey conceded: "You are right." He installed his own fix rather than mine, and it's the better one: instead of my fixed-size copy-and-terminate, he uses memchr to look for a NUL within the field and ignores the name entirely if none is found, rather than silently truncating a name that happens to fill the field. Same bound, cleaner behavior on the edge case. That's commit e5bb73f8c2f3, landed 2026-08-29.

What I keep relearning

A maintainer's rejection deserves the same scrutiny as their suggestions. Sergey wasn't wrong about POSIX, and if I'd argued that point I'd have been arguing against someone who was right. The actual disagreement was one level up, about which inputs the fix needed to survive, and the maintainer's own commit history was the evidence for that, not mine.