Introduction
In a containerized MUD (Multi-User Dungeon) server, we discovered a critical buffer overflow vulnerability in src/mcp.c at line 1384. The mcp_frame_process_input() function processes MCP (MUD Client Protocol) messages from connected clients, copying network-sourced input into a fixed-size output buffer. The function used strncpy() to perform these copies, but critically failed to guarantee null-termination when input exceeded buffer capacity—a textbook CWE-120 buffer overflow.
This vulnerability is remotely exploitable. An attacker needs only network access to the MUD server to send crafted MCP protocol messages with oversized payloads, triggering a buffer overflow that could lead to memory corruption or arbitrary code execution.
The Vulnerability Explained
The vulnerable code in mcp_frame_process_input() handles three different types of MCP protocol input, but all three code paths shared the same dangerous pattern. Here's what the original code looked like:
int
mcp_frame_process_input(McpFrame *mfr, const char *linein, char *outbuf,
int bufsize)
{
if (!strncasecmp(linein, MCP_MESG_PREFIX, 3)) {
/* treat it as an out-of-band message, and parse it. */
if (mfr->enabled || !strncasecmp(MCP_INIT_MESG, linein + 3, 4)) {
if (!mcp_internal_parse(mfr, linein + 3)) {
strncpy(outbuf, linein, bufsize); // ⚠️ VULNERABLE
outbuf[bufsize - 1] = '\0';
return 1;
}
return 0;
} else {
strncpy(outbuf, linein, bufsize); // ⚠️ VULNERABLE
outbuf[bufsize - 1] = '\0';
return 1;
}
} else if (mfr->enabled && !strncasecmp(linein, MCP_QUOTE_PREFIX, 3)) {
/* It's quoted in-band data. Strip the quoting. */
strncpy(outbuf, linein + 3, bufsize); // ⚠️ VULNERABLE
} else {
/* It's in-band data. Return it raw. */
strncpy(outbuf, linein, bufsize); // ⚠️ VULNERABLE
}
outbuf[bufsize - 1] = '\0';
return 1;
}
The Problem with strncpy()
The code uses strncpy(outbuf, linein, bufsize) to copy network input into the output buffer. While strncpy() limits the copy to bufsize bytes, it has a critical flaw: it only null-terminates the destination if the source string is shorter than the size parameter.
If linein contains 100 bytes but bufsize is 64, strncpy() will copy exactly 64 bytes without adding a null terminator. The manual null-termination outbuf[bufsize - 1] = '\0' on some paths attempts to fix this, but:
- It's inconsistent: The third code path (
linein + 3) doesn't immediately null-terminate - It truncates valid data: Setting
outbuf[bufsize - 1] = '\0'overwrites the last byte, even for valid input - It's error-prone: Easy to forget in one of the multiple code paths
Exploitation Scenario
An attacker connects to the MUD server and sends a crafted MCP message:
#$#mcp version: 2.1 to: 2.1[640 bytes of 'A' characters]
When mcp_frame_process_input() processes this message with bufsize = 64:
- The function enters the first
ifbranch (matches MCP_MESG_PREFIX) mcp_internal_parse()fails, entering the error handling pathstrncpy(outbuf, linein, 64)copies 64 bytes without null-termination- The manual
outbuf[bufsize - 1] = '\0'tries to fix it, but... - Any code that later uses
outbufwith string functions likestrlen()orstrcpy()will read past the buffer end looking for a null terminator - This can leak memory contents, corrupt adjacent data structures, or enable control-flow hijacking
The third code path is even worse—it lacks the immediate null-termination, making buffer overread more likely.
Why This Matters
This isn't a theoretical vulnerability. The MCP protocol is used in production MUD servers with real network exposure. The containerized deployment means an attacker who gains code execution could potentially escape the container or pivot to other services. With a CRITICAL severity rating and CWE-120 classification, this represents a high-priority security risk.
The Fix
The fix makes three specific changes to eliminate the buffer overflow:
1. Add Buffer Size Validation
int
mcp_frame_process_input(McpFrame *mfr, const char *linein, char *outbuf,
int bufsize)
{
if (bufsize <= 0) // ✅ NEW: Reject invalid buffer sizes
return 1;
// ... rest of function
}
This upfront check prevents any processing when the buffer size is invalid, defending against caller errors.
2. Replace strncpy() with snprintf()
The core fix replaces all four strncpy() calls with snprintf():
Before:
strncpy(outbuf, linein, bufsize);
outbuf[bufsize - 1] = '\0';
After:
snprintf(outbuf, bufsize, "%s", linein);
This single change provides multiple security improvements:
- Always null-terminates:
snprintf()guarantees a null terminator, even when truncating - Respects buffer boundaries: Will write at most
bufsize - 1characters plus null terminator - Simpler code: No manual null-termination needed, reducing error potential
- Consistent behavior: Same safety guarantees across all code paths
3. Remove Manual Null-Termination
// REMOVED: outbuf[bufsize - 1] = '\0';
Since snprintf() handles null-termination correctly, the manual assignments are no longer needed and could actually introduce bugs by overwriting valid data.
Complete Fix Comparison
Here's the complete before/after for one code path:
Before (vulnerable):
} else {
strncpy(outbuf, linein, bufsize);
}
outbuf[bufsize - 1] = '\0';
return 1;
After (secure):
} else {
snprintf(outbuf, bufsize, "%s", linein);
}
return 1;
The fix is cleaner, safer, and eliminates the entire class of strncpy() null-termination bugs.
Prevention & Best Practices
1. Never Use Unsafe String Functions
Avoid these functions in new C code:
- strcpy() - no bounds checking at all
- strncpy() - doesn't guarantee null-termination
- strcat() - no bounds checking
- gets() - buffer overflow by design (removed from C11)
Use these instead:
- snprintf() - always null-terminates, respects bounds
- strlcpy() / strlcat() - BSD functions with better semantics (if available)
- memcpy() with explicit size checks - for binary data
2. Validate Input Sizes Before Processing
if (bufsize <= 0 || bufsize > MAX_BUFFER_SIZE)
return ERROR;
Always validate buffer sizes at function entry, especially for network-facing code.
3. Use Static Analysis Tools
Configure your build system to run static analyzers:
- Semgrep: Detects unsafe string function usage
- Clang Static Analyzer: Finds buffer overflows and null-termination issues
- Coverity: Enterprise-grade analysis for C/C++
- CodeQL: GitHub's semantic code analysis
4. Implement Regression Tests
The fix includes a comprehensive regression test using the Check framework:
START_TEST(test_buffer_overflow_protection)
{
const int BUFSIZE = 64;
const int CANARY_SIZE = 16;
/* Test with 10x oversized payload */
char payload_10x[640];
memset(payload_10x, 'A', sizeof(payload_10x) - 1);
payload_10x[sizeof(payload_10x) - 1] = '\0';
/* Allocate buffer with canary bytes to detect overflow */
char *buf = malloc(BUFSIZE + CANARY_SIZE);
memset(buf + BUFSIZE, 0xDE, CANARY_SIZE); /* Canary pattern */
mcp_frame_process_input(NULL, payload_10x, buf, BUFSIZE);
/* Verify canary bytes are untouched */
for (int j = 0; j < CANARY_SIZE; j++) {
ck_assert_msg((unsigned char)buf[BUFSIZE + j] == 0xDE,
"Buffer overflow detected at canary byte %d", j);
}
}
END_TEST
This test verifies the security invariant: Buffer reads/writes never exceed the declared bufsize. It tests with payloads at 10x, 2x, and normal sizes to catch regressions.
5. Follow OWASP Secure Coding Practices
- Input validation: Validate all network input before processing
- Least privilege: Run network services with minimal permissions
- Defense in depth: Combine multiple security controls (bounds checking + ASLR + stack canaries)
- Security testing: Include fuzzing and penetration testing for network protocols
Key Takeaways
- strncpy() is not a safe replacement for strcpy(): The lack of guaranteed null-termination makes it nearly as dangerous as
strcpy()when handling variable-length input - Network protocol parsers require extra scrutiny: The
mcp_frame_process_input()function processes untrusted network data, making buffer overflow vulnerabilities remotely exploitable without authentication - snprintf() is the modern standard: For any string formatting or copying in C,
snprintf()provides bounds checking and null-termination guarantees that eliminate entire vulnerability classes - Manual null-termination is error-prone: The pattern
strncpy(); buf[size-1] = '\0';appeared four times in this function, but was inconsistently applied—proof that manual memory management is fragile - Regression tests preserve security fixes: The included test with canary bytes ensures future code changes can't reintroduce this buffer overflow vulnerability
How Orbis AppSec Detected This
- Source: Network-sourced input parameter
lineinfrom connected MCP clients - Sink:
strncpy(outbuf, linein, bufsize)calls insrc/mcp.c:1384and surrounding code paths - Missing control: No guarantee of null-termination when
strlen(linein) >= bufsize, allowing buffer overread in subsequent string operations - CWE: CWE-120 (Buffer Copy without Checking Size of Input)
- Fix: Replaced all
strncpy()calls withsnprintf()for guaranteed null-termination and added upfront buffer size validation
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
This buffer overflow in mcp_frame_process_input() demonstrates why C string handling requires extreme care, especially in network-facing code. The vulnerability was remotely exploitable with no authentication required—an attacker needed only to send an oversized MCP protocol message to trigger memory corruption.
The fix is straightforward: replace strncpy() with snprintf() and add input validation. But the lesson is broader: legacy C code often contains these patterns, and they represent serious security risks in production systems. Regular security audits, static analysis, and automated tools like Orbis AppSec are essential for finding and fixing these vulnerabilities before attackers exploit them.
When writing C code, always prefer modern, bounds-checked APIs over legacy string functions. Your future self—and your users—will thank you.