How stack buffer overflow happens in C PMenu_Do_Update() and how to fix it

How Stack Buffer Overflow Happens in C PMenu_Do_Update() and How to Fix It

Summary

A stack buffer overflow in PMenu_Do_Update() within src/menu/menu.c allowed repeated unchecked sprintf() calls to overflow a fixed 1400-byte stack buffer when menu entries contained long text strings. The fix replaces every sprintf(string + strlen(string), ...) call with snprintf(string + len, sizeof(string) - len, ...), tracking remaining buffer space and preventing writes beyond the buffer boundary.

Introduction

The src/menu/menu.c file handles in-game player menus — team selection, inventory display, and similar UI elements. Its PMenu_Do_Update() function is responsible for building the layout string that gets sent to the client to render the menu. But a subtle, dangerous pattern lurked inside: the function accumulated formatted strings into a single 1400-byte stack buffer using repeated sprintf() calls, with no tracking of how much space remained after each write.

This is a textbook instance of CWE-120 — a classic buffer overflow — and it's particularly dangerous because the overflow target is on the stack. An attacker who can control the text of a menu entry (for example, by setting a long player name that appears in a team selection menu) could overflow the buffer, corrupt adjacent stack memory, and potentially hijack control flow.

The Vulnerability Explained

The Dangerous Pattern in PMenu_Do_Update()

Inside PMenu_Do_Update(), a 1400-byte stack buffer named string is declared, and the function builds a UI layout string by appending formatted chunks to it in a loop:

// VULNERABLE CODE — before the fix
strcpy(string, "xv 32 yv 8 picn inventory ");

for (i = 0, p = hnd->entries; i < hnd->num; i++, p++)
{
    // ... text processing ...

    sprintf(string + strlen(string), "yv %d ", 32 + i * 8);

    // ... alignment logic ...

    sprintf(string + strlen(string), "xv %d ", x - ((hnd->cur == i) ? 8 : 0));

    if (hnd->cur == i)
    {
        sprintf(string + strlen(string), "string2 \"\x0d%s\" ", t);
    }
    else if (alt)
    {
        sprintf(string + strlen(string), "string2 \"%s\" ", t);
    }
    else
    {
        sprintf(string + strlen(string), "string \"%s\" ", t);
    }
}

There are several compounding problems here:

1. sprintf() has no size limit. It will write as many bytes as the format string and arguments produce, regardless of how much space remains in string.
2. strlen(string) is used to find the "end" of the buffer, but this only tells you where to start writing — not how many bytes are left before the buffer ends.
3. The loop runs once per menu entry. Each iteration appends multiple formatted strings. With enough entries, or with sufficiently long entry text (like a player name), the total easily exceeds 1400 bytes.
4. The buffer is on the stack. Overflowing it overwrites the function's return address and other local variables, which is the classic setup for a stack smashing attack.

The Attack Scenario

Consider a team selection menu that displays player names. If an attacker sets their player name to a 200-character string, each iteration of the loop appends something like:

string "AAAAAAAAAA...AAA (200 chars)" 

That's roughly 210 bytes per menu entry. With just 7 players in the menu, the buffer is already at ~1470 bytes — past the 1400-byte boundary. The overflow begins writing into adjacent stack memory, corrupting the saved frame pointer and return address of PMenu_Do_Update().

When the function returns, execution jumps to attacker-controlled data. On modern systems with stack canaries and ASLR this is harder to exploit reliably, but it still causes crashes (denial of service) and may be exploitable in environments without those mitigations.

The Fix

The fix introduces a single size_t len variable to track how many bytes have been written so far, and replaces every sprintf() call with snprintf() that passes sizeof(string) - len as the size limit.

Before and After

Before (line 175, vulnerable):

strcpy(string, "xv 32 yv 8 picn inventory ");

After (safe):

len = snprintf(string, sizeof(string), "xv 32 yv 8 picn inventory ");

Before (line 192, vulnerable):

sprintf(string + strlen(string), "yv %d ", 32 + i * 8);

After (safe):

len += snprintf(string + len, sizeof(string) - len, "yv %d ", 32 + i * 8);

Before (lines 210, 214, 218, vulnerable):

sprintf(string + strlen(string), "string2 \"\x0d%s\" ", t);
sprintf(string + strlen(string), "string2 \"%s\" ", t);
sprintf(string + strlen(string), "string \"%s\" ", t);

After (safe):

len += snprintf(string + len, sizeof(string) - len, "string2 \"\x0d%s\" ", t);
len += snprintf(string + len, sizeof(string) - len, "string2 \"%s\" ", t);
len += snprintf(string + len, sizeof(string) - len, "string \"%s\" ", t);

Why This Works

snprintf(dest, n, fmt, ...) guarantees it will write at most n - 1 bytes plus a null terminator. By passing sizeof(string) - len as n, each call knows exactly how many bytes remain in the buffer. Once len reaches or exceeds sizeof(string), subsequent calls receive 0 or a very small n and write nothing (or just a null terminator), safely truncating the output instead of overflowing.

The len variable accumulates the total written bytes, making strlen(string) calls unnecessary — this also eliminates the O(n²) behavior of the original loop, which called strlen() on an ever-growing string on every iteration.

The Additional Fix in PMenu_Open()

The diff also hardens PMenu_Open():

// BEFORE
hnd->entries = malloc(sizeof(pmenu_t) * num);
memcpy(hnd->entries, entries, sizeof(pmenu_t) * num);

// AFTER
if (num <= 0)
{
    free(hnd);
    return NULL;
}
hnd->entries = calloc((size_t)num, sizeof(pmenu_t));
memcpy(hnd->entries, entries, sizeof(pmenu_t) * (size_t)num);

This adds a guard against zero or negative num values (which would cause malloc(0) or a large allocation due to integer wrap), and switches to calloc() which zero-initializes memory, preventing use of uninitialized data.

Prevention & Best Practices

1. Never Use sprintf() for Buffer Accumulation

sprintf() is inherently unsafe when writing into a fixed-size buffer. Always use snprintf() and always pass the remaining buffer size, not the total size:

// Wrong — passes total size, ignores already-written bytes
snprintf(buf + offset, sizeof(buf), ...);

// Correct — passes remaining size
snprintf(buf + offset, sizeof(buf) - offset, ...);

2. Track Written Bytes, Not String Length

Using strlen(buf) to find the write position is O(n) and doesn't tell you the remaining space. Maintain a len or offset variable from the start:

size_t len = 0;
len += snprintf(buf + len, sizeof(buf) - len, "part1 %s", s1);
len += snprintf(buf + len, sizeof(buf) - len, "part2 %d", n);

3. Check snprintf() Return Values for Truncation

snprintf() returns the number of bytes that would have been written if the buffer were large enough. If the return value equals or exceeds the size limit, truncation occurred:

int written = snprintf(buf + len, sizeof(buf) - len, fmt, arg);
if (written < 0 || (size_t)written >= sizeof(buf) - len) {
    // Handle truncation — log, error out, or resize buffer
}
len += (size_t)written;

4. Use Compiler and Linker Hardening

Enable stack protection features during compilation:
- -fstack-protector-strong — adds stack canaries to detect overflow before return
- -D_FORTIFY_SOURCE=2 — enables compile-time and runtime checks on sprintf, strcpy, etc.
- -Wformat-overflow — GCC warning for statically detectable format string overflows

5. Static Analysis Tools

• Semgrep: Rules for sprintf into fixed buffers — https://semgrep.dev/r?q=sprintf
• Clang Static Analyzer: Detects buffer overflows in C/C++
• Coverity: Enterprise-grade detection of CWE-120 patterns
• AddressSanitizer (ASan): Runtime detection — compile with -fsanitize=address

Key Takeaways

• sprintf(buf + strlen(buf), ...) is a red flag in any C codebase. It tells you the developer is accumulating into a buffer but not tracking remaining space. Search your codebase for this pattern.
• In PMenu_Do_Update(), the overflow was loop-amplified. Each menu entry added multiple sprintf() calls, meaning the overflow risk scaled with the number of entries and the length of player names — both attacker-influenced values.
• Switching to snprintf() alone isn't enough. You must pass sizeof(buffer) - current_offset as the size, not sizeof(buffer). The fix correctly uses sizeof(string) - len on every call.
• calloc() over malloc() for entry arrays. The PMenu_Open() fix also switched to calloc(), ensuring zero-initialized memory and guarding against use of uninitialized pointer fields in menu entries.
• The num <= 0 guard in PMenu_Open() closes a related integer handling bug. Without it, a zero or negative entry count could produce a zero-size allocation or integer overflow in the sizeof(pmenu_t) * num multiplication.

How Orbis AppSec Detected This

• Source: Menu entry text strings (p->text) populated from player-controlled data such as player names in team selection menus
• Sink: sprintf(string + strlen(string), "string \"%s\" ", t) at src/menu/menu.c:218 (and related calls at lines 172, 192, 210, 214)
• Missing control: No remaining-buffer-size tracking; sprintf() used instead of snprintf(), allowing unbounded writes into the 1400-byte stack buffer string
• CWE: CWE-120 — Buffer Copy without Checking Size of Input ("Classic Buffer Overflow")
• Fix: Introduced size_t len to track bytes written and replaced all sprintf() calls with snprintf(string + len, sizeof(string) - len, ...) to enforce the buffer boundary

Orbis AppSec automatically detected this vulnerability and opened a pull request with the fix. Try Orbis AppSec on your repositories to find and fix issues like this automatically.

Conclusion

The buffer overflow in PMenu_Do_Update() is a clear example of how a common, decades-old C pattern — appending to a string buffer with sprintf() — becomes dangerous the moment user-controlled data enters the picture. The fix is straightforward: one size_t len variable and a consistent switch to snprintf() with sizeof(string) - len. The cost of the fix is minimal; the cost of leaving it unfixed could be a crashed server or, in a less hardened environment, arbitrary code execution triggered by a long player name.

When writing C code that builds strings incrementally, always ask: do I know exactly how many bytes remain in this buffer at every write? If the answer is no, it's time to introduce a length tracker and switch to snprintf().

References

• CWE-120: Buffer Copy without Checking Size of Input
• OWASP Buffer Overflow Cheat Sheet
• GNU C Library — snprintf documentation
• Semgrep rules for sprintf
• fix: add buffer-length check in menu.c