How unsafe buffer copying happens in C credential storage and how to fix it

How unsafe buffer copying happens in C credential storage and how to fix it

Introduction

In the production codebase of an AFP (Apple Filing Protocol) server implementation, a critical vulnerability was discovered in lib/server.c at lines 39–40. The afp_server_complete_connection() function was using memcpy() to copy username and password credentials into fixed-size buffers without proper bounds checking or null-termination guarantees. This is a classic buffer overflow pattern that could allow attackers to trigger out-of-bounds memory reads by supplying crafted credentials through a malicious AFP server.

The specific problematic code was:

memcpy(server->username, username, sizeof(server->username));
memcpy(server->password, password, sizeof(server->password));

This pattern is dangerous because memcpy() blindly copies the number of bytes specified as the third argument, regardless of whether the source data actually contains that many bytes. If the source string is shorter than the destination buffer size, memcpy() will read past the end of the source string, leaking adjacent memory. Additionally, memcpy() doesn't guarantee null-termination, so if the source exactly fills the buffer, the resulting string won't be null-terminated—causing undefined behavior in any function that expects a C string.

The Vulnerability Explained

What Went Wrong

The vulnerable code in lib/server.c:39-40 makes two critical assumptions that are not guaranteed:

1. Assumption 1: The source buffer (username and password parameters) is at least as long as the destination buffer size.
2. Assumption 2: The destination buffer will be properly null-terminated after the copy.

Neither assumption is safe. Here's why:

The Vulnerable Code:

memcpy(server->username, username, sizeof(server->username));
memcpy(server->password, password, sizeof(server->password));

If sizeof(server->username) is 256 bytes (a typical size for credential buffers), but the username parameter points to a 10-byte string in memory, memcpy() will read 256 bytes starting from the username pointer. This causes an out-of-bounds read, pulling data from memory that follows the username string—potentially sensitive data like API keys, tokens, or other credentials stored nearby.

Attack Scenario

Consider this exploitation scenario:

1. An attacker controls a malicious AFP server or provides crafted input to the client.
2. The attacker supplies a short username string (e.g., "admin" = 5 bytes) but the code calls memcpy(..., 256).
3. memcpy() reads 256 bytes from the username pointer, including memory beyond the string boundary.
4. Adjacent memory contents (heap metadata, other credentials, or sensitive data) are copied into server->username.
5. If the application logs or returns this buffer, the attacker has leaked sensitive information.
6. Alternatively, if the buffer is used in string operations without proper length checks, it could trigger a buffer over-read in downstream code.

Real-World Impact

In this AFP server context, credentials are core to authentication. If an attacker can:
- Leak memory via out-of-bounds reads, they might discover other users' credentials or session tokens.
- Trigger a crash by reading from unmapped memory, they achieve denial of service.
- Corrupt the heap by triggering memory protection violations, they could escalate to arbitrary code execution.

The PR description notes this was confirmed exploitable by the security scanner, meaning the vulnerability was not theoretical—it could be reliably triggered.

The Fix

What Changed

The fix replaces the dangerous memcpy() calls with strlcpy(), a bounds-safe string copy function:

Before:

memcpy(server->username, username, sizeof(server->username));
memcpy(server->password, password, sizeof(server->password));

After:

strlcpy(server->username, username, sizeof(server->username));
strlcpy(server->password, password, sizeof(server->password));

Why This Fix Works

strlcpy() is specifically designed for safe string copying. It:

1. Enforces bounds checking: It will copy at most sizeof(server->username) - 1 bytes from the source.
2. Guarantees null-termination: It always appends a null byte (\0) to the destination, ensuring the result is a valid C string.
3. Prevents out-of-bounds reads: It stops reading from the source at the first null terminator or after copying the maximum allowed bytes—whichever comes first.
4. Returns the intended length: It returns the length of the source string, allowing the caller to detect truncation if needed.

Security Properties Restored:

• ✅ No out-of-bounds reads: strlcpy() will never read past the source string's null terminator.
• ✅ Always null-terminated: The destination buffer is guaranteed to end with \0, even if truncated.
• ✅ Predictable behavior: The function behaves identically regardless of input length.

Example

If username = "admin" (5 bytes + null terminator) and sizeof(server->username) = 256:

• Old code (memcpy): Copies 256 bytes, reading far past the "admin" string and leaking adjacent memory.
• New code (strlcpy): Copies the 5-byte string "admin" and appends a null terminator, leaving the rest of the 256-byte buffer unchanged.

Prevention & Best Practices

1. Never use memcpy() for string handling

memcpy() is for raw binary data. For strings, use:
- strlcpy() — Preferred on BSD, macOS, and modern systems. Guaranteed null-termination.
- strncpy() — Available everywhere but requires manual null-termination.
- snprintf() — Flexible, well-defined, and safe.

2. Validate input length before copying

Even with strlcpy(), validate that input meets your expectations:

if (strlen(username) > 255) {
    log_error("Username too long");
    return -1;
}
strlcpy(server->username, username, sizeof(server->username));

3. Use static analysis tools

Tools like Clang Static Analyzer, Coverity, and Semgrep can flag dangerous patterns:
- memcpy() with sizeof() on buffers
- Missing null-termination checks
- Unsafe string functions

4. Apply defense in depth

• Use AddressSanitizer (ASan) during testing to catch out-of-bounds accesses at runtime.
• Use Valgrind to detect memory errors in your credential handling code.
• Implement fuzzing with AFL or libFuzzer to test credential parsing with malformed input.

5. Encrypt credentials at rest

The PR description notes that PBKDF2 is available in dependencies but not used. Beyond fixing the buffer overflow, credentials should be encrypted before storage:

// Pseudocode: encrypt credentials before storing
unsigned char *encrypted = pbkdf2_encrypt(password, salt, iterations);
strlcpy(server->password_encrypted, (char*)encrypted, sizeof(server->password_encrypted));

6. Follow OWASP Secure Coding Guidelines

• OWASP: Buffer Overflow
• OWASP: Credential Storage Cheat Sheet

Key Takeaways

• Never use memcpy() to copy strings: It doesn't perform bounds checking or null-termination. Use strlcpy() instead.
• The sizeof(destination) pattern is dangerous: When used as the length argument to memcpy() for strings, it assumes the source is at least that long—a false assumption.
• Out-of-bounds reads are exploitable: Even if they don't crash immediately, they can leak sensitive data from adjacent memory, especially critical in credential-handling code.
• The AFP server's credential copying was confirmed exploitable: This wasn't a theoretical issue—attackers could trigger it with malicious input.
• Bounds-safe functions like strlcpy() are non-negotiable for credential storage: They eliminate entire classes of buffer overflow vulnerabilities.

How Orbis AppSec Detected This

Source: Credentials supplied via AFP protocol input (username and password parameters to afp_server_complete_connection())

Sink: Unsafe memcpy() calls at lib/server.c:39-40 copying into fixed-size buffers (server->username and server->password)

Missing Control: No bounds validation on source data length; no guarantee of null-termination after copy; no use of bounds-safe string functions

CWE: CWE-119 (Improper Restriction of Operations within the Bounds of a Memory Buffer) and CWE-120 (Buffer Copy without Checking Size of Input)

Fix: Replaced memcpy(server->username, username, sizeof(server->username)) with strlcpy(server->username, username, sizeof(server->username)) to enforce bounds checking and automatic null-termination. The same change was applied to the password field.

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 credential handling code are among the most critical security issues in C applications. The fix in this PR—replacing memcpy() with strlcpy()—is a straightforward but essential change that eliminates out-of-bounds reads and ensures proper null-termination.

For developers maintaining C code that handles sensitive data like credentials, the key lesson is simple: use bounds-safe functions by default. Don't assume input lengths; don't rely on implicit null-termination. Tools like strlcpy(), combined with static analysis and runtime testing, make it possible to write secure credential handling code.

If you're working on similar authentication or credential storage code, audit it now for the same pattern. This vulnerability is both common and easily fixed—but the impact of leaving it unfixed is severe.

References

• CWE-119: Improper Restriction of Operations within the Bounds of a Memory Buffer
• CWE-120: Buffer Copy without Checking Size of Input
• OWASP: Buffer Overflow
• OWASP: Credential Storage Cheat Sheet
• strlcpy() Manual Page
• Semgrep: Unsafe memcpy Rule
• fix: add bounds check before memcpy in server.c