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 the sbin/restore/tape.c file, a critical buffer overflow vulnerability was lurking in plain sight. The setinput() function at line 179 used the notoriously unsafe strcpy() function to copy a user-controlled source parameter directly into a fixed-size buffer named magtape. This function handles input from command-line arguments (via the -f flag) and environment variables (like TAPE), making it an attractive attack surface. The problem: strcpy() has no concept of buffer boundaries—it copies bytes until it encounters a null terminator, regardless of how much space is available. An attacker controlling the input could provide a string longer than the buffer could hold, overwriting adjacent memory and potentially executing arbitrary code.

The Vulnerability Explained

Let's look at the vulnerable code:

void setinput(char *source)
{
    // ... validation logic ...
    (void) strcpy(magtape, source);  // Line 179: VULNERABLE
}

The magtape buffer is declared as a fixed-size array (typically char magtape[BUFSIZ]), but strcpy() trusts that the input will fit. It doesn't. Here's why this is dangerous:

The Problem:
- strcpy() copies the entire string pointed to by source into magtape
- There's no length check — the function doesn't know (or care) how large magtape is
- If source is longer than BUFSIZ bytes, the overflow silently corrupts memory beyond the buffer boundary

Example Attack Scenario:

An attacker could invoke the restore utility like this:

restore -r -f $(python -c 'print("A"*10000)')

This passes a 10,000-character string as the source parameter. When setinput() calls strcpy(), it copies all 10,000 characters into the magtape buffer, overflowing by approximately 9,900 bytes (depending on BUFSIZ). The overflow corrupts:
- Stack variables in the same function
- Return addresses on the stack
- Heap metadata and adjacent allocations
- Other process memory

Real-World Impact:

For a restore utility (which handles tape backup operations), this overflow could:
1. Crash the application — causing data loss during restore operations
2. Corrupt critical data structures — affecting the integrity of restored files
3. Enable privilege escalation — if the restore utility runs as root (common for backup tools), the attacker gains root code execution
4. Bypass security checks — overwrite validation flags or security tokens in memory

The vulnerability is "likely exploitable" because the attacker controls the input directly via command-line arguments, and the overflow target (the return address) is on the stack—a classic exploitation vector.

The Fix

The fix is surgical and direct: replace strcpy() with strlcpy(), which enforces a maximum copy length:

- (void) strcpy(magtape, source);
+ (void) strlcpy(magtape, source, sizeof(magtape));

What Changed:

• Old: strcpy(magtape, source) — copies unbounded
• New: strlcpy(magtape, source, sizeof(magtape)) — copies at most sizeof(magtape) - 1 bytes (reserving 1 byte for the null terminator)

Why This Works:

strlcpy() is a length-safe string function that:
1. Accepts a maximum length parameter (sizeof(magtape))
2. Copies at most length - 1 bytes from the source
3. Always null-terminates the destination string
4. Returns the length of the source string (allowing the caller to detect truncation if needed)

Now, if an attacker passes a 10,000-character string, strlcpy() safely copies only BUFSIZ - 1 characters and null-terminates the result. The rest of the string is silently truncated—no overflow, no memory corruption.

Security Invariant Enforced:

The PR description specifies: "Buffer reads never exceed the declared length." The strlcpy() fix enforces this invariant at the API level. Every call to strlcpy() with sizeof(magtape) guarantees that at most sizeof(magtape) bytes are written.

Additional Locations

The PR notes that similar unsafe patterns exist elsewhere in the same file:

sbin/restore/tape.c:356
sbin/restore/tape.c:360
sbin/restore/tape.c:394
sbin/restore/tape.c:843

These should be reviewed and fixed using the same strlcpy() pattern to prevent related vulnerabilities.

Prevention & Best Practices

For C developers, follow these rules:

1. Never use strcpy() or strcat() — They have no bounds checking and are impossible to use safely. Treat them as deprecated.

2. Use length-safe alternatives:
   - strlcpy() and strlcat() (BSD-style, available on most Unix systems)
   - strncpy() with careful null-termination (more error-prone than strlcpy())
   - snprintf() for formatted string copying

3. Always specify buffer size:
   ```c
   // Good
   strlcpy(buffer, source, sizeof(buffer));

// Risky (easy to get wrong)
strncpy(buffer, source, sizeof(buffer) - 1);
buffer[sizeof(buffer) - 1] = '\0';
```

1. Enable compiler warnings:
   bash gcc -Wall -Wextra -Wformat=2 -Wformat-security
   Modern compilers warn about strcpy() usage.

2. Use static analysis tools to catch this pattern automatically:
   - Clang Static Analyzer — detects strcpy() with tainted input
   - Coverity — enterprise-grade buffer overflow detection
   - Orbis AppSec — AI-powered scanning that caught this issue

3. Reference Standards:
   - CWE-120: Buffer Copy without Checking Size of Input — https://cwe.mitre.org/data/definitions/120.html
   - OWASP: Buffer Overflow — https://owasp.org/www-community/attacks/Buffer_Overflow
   - SEI CERT C: STR31-C: Guarantee that storage for strings has sufficient space for character data and the null terminator — https://wiki.sei.cmu.edu/confluence/display/c/STR31-C

Key Takeaways

• strcpy() is fundamentally unsafe — It cannot be used securely with untrusted input, period. Replace every instance in your codebase.
• strlcpy() enforces bounds at the API level — The function itself prevents overflow, not external validation. This is defense in depth.
• Command-line arguments are untrusted input — Even if the restore utility is a local tool, attackers can craft malicious arguments. Treat argv[] as hostile.
• Similar patterns in the same file are likely vulnerable — The PR identified four additional locations (lines 356, 360, 394, 843) that use similar unsafe patterns and should be audited.
• Automated detection works — The Orbis multi_agent_ai scanner flagged this pattern automatically, demonstrating that static analysis can catch these issues before they reach production.

How Orbis AppSec Detected This

Source: User-controlled input from command-line arguments (-f flag) and the TAPE environment variable, passed to the setinput() function via the source parameter.

Sink: The unsafe strcpy(magtape, source) call at line 179 in sbin/restore/tape.c, which copies the tainted input into a fixed-size buffer without bounds checking.

Missing Control: No length validation before the copy operation. The magtape buffer size is fixed (typically BUFSIZ), but strcpy() doesn't enforce this constraint.

CWE: CWE-120 (Buffer Copy without Checking Size of Input) — a classic buffer overflow vulnerability.

Fix: Replace strcpy(magtape, source) with strlcpy(magtape, source, sizeof(magtape)) to enforce a maximum copy length equal to the buffer size.

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.

Regression Testing

The PR includes a comprehensive regression test that guards against future regressions:

START_TEST(test_setinput_buffer_overflow)
{
    // Test with payloads of varying lengths
    const char *payloads[] = {
        "A",                    // Valid: short input
        "AAA...AAAA",          // Valid: exactly BUFSIZ-1 bytes
        "AAA...AAAAA"          // Invalid: exceeds BUFSIZ by 1
    };

    for (int i = 0; i < num_payloads; i++) {
        pid_t pid = fork();
        if (pid == 0) {
            setinput(payloads[i]);
            _exit(0);
        } else {
            int status;
            waitpid(pid, &status, 0);
            // Ensure child didn't crash (e.g., SIGSEGV)
            ck_assert_msg(!WIFSIGNALED(status), 
                "Buffer overflow detected with payload length %zu", 
                strlen(payloads[i]));
        }
    }
}
END_TEST

This test:
- Runs setinput() in a child process with various payload sizes
- Verifies that oversized input doesn't crash the program
- Guards against regressions if someone accidentally reverts the fix

Conclusion

Buffer overflow vulnerabilities in C are among the most dangerous security issues—they've enabled countless real-world exploits, from the Morris Worm to modern privilege escalation attacks. The fix in this case is simple but critical: replace the unsafe strcpy() with the length-safe strlcpy().

This vulnerability demonstrates why:
1. Never trust input — Even from command-line arguments you think you control
2. Use language features that enforce safety — strlcpy() makes overflow impossible by design
3. Automate security scanning — Manual code review missed this; automated tools caught it immediately
4. Test security invariants — The regression test ensures the invariant ("Buffer reads never exceed the declared length") is maintained

As you review your own C codebase, search for every instance of strcpy(), strcat(), gets(), and sprintf(). Replace them with their length-safe equivalents. Your future self—and your security team—will thank you.

References

• CWE-120: Buffer Copy without Checking Size of Input — https://cwe.mitre.org/data/definitions/120.html
• OWASP: Buffer Overflow — https://owasp.org/www-community/attacks/Buffer_Overflow
• SEI CERT C: STR31-C — https://wiki.sei.cmu.edu/confluence/display/c/STR31-C
• strlcpy() Manual Page — https://man.openbsd.org/strlcpy
• Semgrep Rule: strcpy() Detection — https://semgrep.dev/r?q=strcpy
• GitHub PR: fix: add buffer-length check in tape.c