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

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

Introduction

In CodeBaseServer2020, a critical buffer overflow vulnerability was lurking in the file handling logic. The u4name.c file—responsible for processing database file paths and names—used unsafe string operations that could allow an attacker to supply a specially crafted filename exceeding the buffer size, leading to heap corruption and potentially remote code execution.

The vulnerability centered on three specific lines (408, 433, and 444) where the code used c4strcpy() (a wrapper around the notoriously unsafe strcpy() function) to copy user-influenced filenames directly into fixed-size destination buffers without any bounds checking. This is the textbook definition of a classic buffer overflow—and it was sitting in production code.

The Vulnerability Explained

What's the Problem?

The vulnerability stems from a fundamental misunderstanding of how string length validation should work. Let's look at the vulnerable code pattern:

// VULNERABLE CODE (line 408 area)
if ( c4strlen( outName + c4strlen( inputName ) ) > outNameLen )
    return -1 ;
c4strcat( outName, inputName ) ;

At first glance, this looks like it's trying to validate buffer length. But there's a critical flaw in the logic. The code calculates c4strlen(outName + c4strlen(inputName)), which means:

1. It gets the length of inputName
2. It adds that offset to the outName pointer
3. It measures the string length from that new position

This is mathematically incorrect. If inputName is 100 characters long and outName is already 50 characters long, the code would check the string length starting 100 bytes into outName's memory—potentially reading past the buffer entirely or checking the wrong memory region.

The correct calculation should be:

// CORRECT VALIDATION
if ( c4strlen( outName ) + c4strlen( inputName ) > outNameLen )
    return -1 ;

The Attack Scenario

Imagine an attacker connecting to the CodeBase server and requesting to open a database file with a path like:

/path/to/db/AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA...
(512+ 'A' characters total, exceeding the 256-byte outName buffer)

The vulnerable validation logic would fail to catch this oversized input because it's checking the wrong memory location. The subsequent c4strcat() call would then copy the entire oversized filename into the fixed 256-byte outName buffer, overflowing into adjacent heap memory.

An attacker could craft the payload to:
- Overwrite function pointers or return addresses
- Corrupt the heap metadata
- Execute arbitrary code with the privileges of the CodeBase server process

Why This Matters

This vulnerability affects any code path that processes filenames through the u4nameCreateMultiDirectories() function, which is called when users open or create database files. Since database paths are often controlled or influenced by user input (through network protocols, APIs, or configuration files), this creates a direct attack surface for remote exploitation.

The Fix

The fix is surgical and precise. The PR changed line 425 in the vulnerable bounds check:

- if ( c4strlen( outName + c4strlen( inputName ) ) > outNameLen )
+ if ( c4strlen( outName ) + c4strlen( inputName ) > outNameLen )

This single character change (removing the + operator placement) transforms the logic from incorrect pointer arithmetic to correct length addition.

Before the fix:

// Checking string length from an incorrect memory position
if ( c4strlen( outName + c4strlen( inputName ) ) > outNameLen )
    return -1 ;

After the fix:

// Correctly checking if combined lengths exceed buffer capacity
if ( c4strlen( outName ) + c4strlen( inputName ) > outNameLen )
    return -1 ;

Now the validation correctly:
1. Gets the current length of outName
2. Gets the length of inputName
3. Adds them together
4. Compares against the actual buffer size (outNameLen)
5. Returns an error (-1) if concatenation would overflow

The fix ensures that the security invariant "Buffer reads never exceed the declared length" is maintained.

Verification & Testing

The PR includes a comprehensive regression test that validates the fix against multiple attack payloads:

const char *payloads[] = {
    /* Exact exploit: 2x buffer size - should be rejected */
    "AAAAAAAAAAAAAAAA...(512 chars)...AAAAAAAAAAAAAAAA",
    /* Boundary: exactly at buffer limit - should be rejected */
    "BBBBBBBBBBBBBBBB...(256 chars)...BBBBBBBBBBBBBBBB",
    /* Valid input: normal filename - should succeed */
    "testfile.dat"
};

The test verifies that:
- Oversized inputs are rejected with error code -1
- Boundary conditions are handled correctly
- Valid inputs still work as expected
- Stack canaries remain intact (no corruption)

Prevention & Best Practices

1. Avoid strcpy() Entirely

Never use strcpy(), strcat(), or their thin wrappers in production code. These functions have no bounds checking and are fundamentally unsafe.

2. Use Length-Safe Alternatives

Replace unsafe functions with:
- strncpy() - copy with explicit length limit
- strlcpy() - safer BSD variant (copies at most n-1 bytes)
- snprintf() - formatted output with size control

Example:

// SAFE: Explicitly limit copy length
strncpy(outName, inputName, outNameLen - 1);
outName[outNameLen - 1] = '\0';  // Ensure null termination

3. Always Validate Before Operations

Before any string concatenation or copy, validate that both source and destination lengths are compatible:

// SAFE: Validate before concatenation
if (strlen(outName) + strlen(inputName) + 1 > outNameLen) {
    return -1;  // Error: would overflow
}
strcat(outName, inputName);

4. Use Static Analysis Tools

Deploy tools that detect unsafe string operations:
- Semgrep: Catches strcpy/strcat patterns
- Clang Static Analyzer: Detects buffer overflow risks
- Orbis AppSec: Identifies tainted user input flowing to unsafe sinks

5. Implement Security Invariants

Define and enforce security properties in your code:

// INVARIANT: outName buffer must never be written beyond outNameLen bytes
// INVARIANT: All string operations must be validated before execution

6. Code Review Checklist for String Operations

When reviewing C code, ask:
- [ ] Is strcpy/strcat/sprintf used? (Flag for replacement)
- [ ] Are buffer lengths checked before copy operations?
- [ ] Is the length calculation mathematically correct?
- [ ] Are off-by-one errors possible?
- [ ] Is user input flowing to this function?

Key Takeaways

• The arithmetic error was subtle but critical: Changing c4strlen(outName + c4strlen(inputName)) to c4strlen(outName) + c4strlen(inputName) fixed the vulnerability by checking the correct memory region.

• Wrapper functions don't inherently fix unsafe patterns: The c4strcpy() wrapper provided no additional safety—bounds checking must be explicit in the calling code.

• User-controlled filenames are a direct attack vector: Any code that processes filenames from network input, APIs, or configuration files should be treated as untrusted and validated rigorously.

• This vulnerability affected multiple locations: Lines 408, 433, 444, 653, 655, 1324, and 1329 in u4name.c used similar patterns, requiring comprehensive code review across the entire file.

• Regression testing prevents re-introduction: The test suite now guards against buffer overflow regressions by validating payloads 2x the buffer size, boundary conditions, and valid inputs.

How Orbis AppSec Detected This

Source: User-controlled filenames and paths passed to u4nameCreateMultiDirectories() via database file open operations

Sink: The vulnerable bounds check at u4name.c:425 and subsequent c4strcat() call that performs unbounded string concatenation

Missing control: Incorrect length validation logic that failed to properly calculate whether concatenated strings would exceed the outNameLen buffer capacity

CWE: CWE-120 - Buffer Copy without Checking Size of Input ('Classic Buffer Overflow')

Fix: Corrected the length validation arithmetic from c4strlen(outName + c4strlen(inputName)) to c4strlen(outName) + c4strlen(inputName) to properly check if combined string lengths would exceed buffer limits

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

Buffer overflow vulnerabilities in C string operations remain one of the most dangerous and exploitable security flaws decades after their discovery. The u4name.c vulnerability demonstrates that even simple, well-intentioned validation logic can fail if the underlying arithmetic is incorrect.

The fix—changing a single operator placement—prevented a critical security breach that could have allowed remote attackers to execute arbitrary code on CodeBase servers worldwide. This incident reinforces the importance of:

1. Never trusting user input with unsafe string functions
2. Using length-safe alternatives like strncpy() and snprintf()
3. Implementing automated security scanning to catch these patterns before they reach production
4. Comprehensive regression testing to prevent re-introduction of fixed vulnerabilities

For developers working with C code, especially code that processes external input, treat every strcpy(), strcat(), and sprintf() call as a potential security incident waiting to happen. Replace them proactively, validate inputs rigorously, and use tools like Orbis AppSec to catch these issues automatically.

References

• CWE-120: Buffer Copy without Checking Size of Input
• OWASP: Buffer Overflow
• C String Handling - OWASP Cheat Sheet
• GNU C Library - String and Array Utilities
• Semgrep: strcpy detection rule
• fix: add buffer-length check in u4name.c