Finding and rewriting a vulnerable C program

The program

Here is the shape this question always takes, in words before code. A function declares two eight-byte character arrays, str1 and str2, and an integer flag valid initialised to false. It calls next_tag(str1) to load str1 with a tag the program expects. It then calls gets(str2) to read a line from standard input. Finally it compares the two with strncmp(str1, str2, 8) and sets valid to true if they match.

Read that description and locate the defect before reading on. It is on one line.

Why it is vulnerable — the precise statement

The defect is gets(str2), and the reason is structural rather than incidental: gets() cannot be given a bound. Look at its signature — char *gets(char *s) — and notice there is no second parameter. There is nowhere to state how large s is, and the function has no other way to find out, because in C an array decays to a bare pointer the moment it is passed. So gets() does the only thing it can: it reads characters until it meets a newline or end of file, and writes every one of them into memory starting at s.

str2 holds seven characters plus a terminating NUL. Any input longer than that writes past the end of the array and into whatever the compiler laid out next. That is the definition of a buffer overflow, and it is unavoidable here — not a matter of the caller being careless, but of the callee being unable to be careful.

What lies next matters, and this program has two distinct exploitation paths.

Path 1 — corrupt the neighbour. str1 is adjacent. A sixteen-character input fills str2 with the first eight characters and overwrites str1 with the next eight. If the attacker supplies the same eight characters twice, str1 and str2 now hold identical bytes, strncmp returns 0, and valid becomes true — without the attacker ever knowing what the expected tag was. Notice what did not happen: no return address was touched, no machine code was injected, and no address was guessed. This is why the defences of the previous lesson are irrelevant to this path.

Path 2 — corrupt the control data. Keep writing. Past the two arrays sit the saved frame pointer and then the return address. An input long enough reaches them, and an attacker who knows the layout can divert execution when the function returns.

The order matters when you write the answer: the adjacent-variable path is the easier attack and the one this program uniquely invites, and a full-marks answer names it first.

The rewrite

The minimum fix is to replace the unbounded read with a bounded one:

fgets(str2, sizeof str2, stdin)

fgets takes the destination size as its second argument and will never write more than that many bytes including the terminator. Three follow-ups turn a passing answer into a full-marks one.

One — fgets keeps the newline. Unlike gets, fgets stores the terminating newline in the buffer when there is room for it. Compare without stripping it and a correct tag fails to match. Strip it with str2[strcspn(str2, "\n")] = '\0'; which finds the newline if there is one and the terminator if there is not.

Two — sizeof only works where the array is. sizeof str2 yields 8 inside the function that declared str2, because there the name refers to an actual array. Pass str2 to another function and the parameter is a pointer; sizeof there yields the size of a pointer — 4 or 8 — which is a coincidence waiting to become a bug. Wherever a buffer crosses a function boundary, its size must cross with it as an explicit argument.

Three — next_tag is unbounded too. It is handed str1 and no size, so it has precisely the same defect as gets. Fixing only the input call leaves the second hole open. Change its contract to int next_tag(char *dst, size_t dstlen) and honour the length inside.

One more improvement worth a line: compare using the buffers' real length rather than a literal 8. A hard-coded length silently becomes wrong the day someone changes the declaration, and sizeof str1 cannot.