Unbounded strcpy() in FreezeProject/fs.c: How Four Lines Fixed a Critical Buffer Overflow

The Incident

The FreezeProject/src/fs.c file implements a custom in-memory and on-disk filesystem — the kind of low-level code that manages file metadata, allocates slots, mounts stored state from disk, and serializes everything back. It's exactly the sort of code that needs to handle adversarial input gracefully, because filenames come directly from user input at the shell level.

What we found instead was a custom strcpy() implementation being called at four separate locations to copy filenames into fixed-size files[i].name buffers — with zero bounds checking. This is a textbook, critical-severity stack/heap buffer overflow waiting to be triggered.

The Vulnerability Explained

The Custom strcpy() — Already a Red Flag

The file defines its own strcpy():

char* strcpy(char* dest, const char* src) {
    // ... unbounded byte-by-byte copy
    return dest;
}

Writing your own strcpy() in a security-sensitive context is almost always the wrong move. This implementation copies bytes from src to dest until it hits a null terminator — with no awareness of how large dest actually is. The moment src is longer than dest, you have undefined behavior: adjacent memory gets overwritten.

Four Vulnerable Call Sites

The dangerous function was called in four places, each copying a filename into a fixed-size buffer:

1. fs_alloc_slot() — line 57 (original)

static int fs_alloc_slot(const char* name) {
    for (int i = 0; i < MAX_FILES; i++) {
        if (!files[i].used) {
            strcpy(files[i].name, name);  // ← no length limit

2. fs_mount() — line 141 (original)

files[i].used = 1;
strcpy(files[i].name, meta->name);  // ← reading from disk, still dangerous

3. fs_create() — line 186 (original)

if (!files[i].used) {
    strcpy(files[i].name, name);  // ← user input flows here

4. fs_save() — line 327 (original)

struct fs_metadata* meta = (struct fs_metadata*)metadata_buf;
strcpy(meta->name, files[fd].name);  // ← serializing back to disk

The Attack Path

The PR description is explicit: user input from the shell flows directly into file creation functions that call strcpy. Here's what a concrete attack looks like:

1. An attacker interacts with the FreezeProject shell interface and issues a file creation command with a filename of, say, 512 characters: AAAAAAAAAA... (×512).
2. The shell passes this string to fs_create().
3. fs_create() calls strcpy(files[i].name, name), where files[i].name is a fixed-size buffer (bounded by MAX_FILENAME).
4. strcpy() happily copies all 512 bytes, blowing past the end of files[i].name and overwriting whatever sits adjacent in memory — potentially other file metadata entries, control data, or return addresses depending on the memory layout.
5. The result ranges from silent data corruption (other files' metadata is trashed) to a full crash or, in the worst case, exploitable control-flow hijacking.

The fs_mount() call site is particularly sneaky: even data being read back from disk gets passed through the unbounded copy. If an attacker can write a crafted disk image (or if a previous overflow corrupted the on-disk metadata), the overflow fires again at mount time.

The Fix

A Minimal, Purpose-Built Safe Copy Helper

The fix adds a single static helper function, safe_strncpy(), immediately after the existing strcpy() definition:

static void safe_strncpy(char* dest, const char* src, size_t n) {
    size_t i;
    for (i = 0; i + 1 < n && src[i]; i++) {
        dest[i] = src[i];
    }
    dest[i] = 0;
}

This is a clean, dependency-free bounded copy that:
- Copies at most n - 1 bytes from src to dest
- Always null-terminates dest (the dest[i] = 0 after the loop)
- Stops early if src is shorter than n (no over-read)

The i + 1 < n condition in the loop guard is the critical detail — it ensures there is always at least one byte of space remaining for the null terminator before the loop writes the next character.

Before and After at Each Call Site

fs_alloc_slot()

// Before
strcpy(files[i].name, name);

// After
safe_strncpy(files[i].name, name, MAX_FILENAME);

fs_mount()

// Before
strcpy(files[i].name, meta->name);

// After
safe_strncpy(files[i].name, meta->name, MAX_FILENAME);

fs_create()

// Before
strcpy(files[i].name, name);

// After
safe_strncpy(files[i].name, name, MAX_FILENAME);

fs_save()

// Before
strcpy(meta->name, files[fd].name);

// After
safe_strncpy(meta->name, files[fd].name, MAX_FILENAME);

Every call site now passes MAX_FILENAME as the size bound — the same constant that defines the buffer size. This creates a consistent, enforceable contract: no filename stored anywhere in the filesystem layer can ever exceed MAX_FILENAME - 1 bytes (plus null terminator).

Why All Four Sites Matter

It might be tempting to fix only the "obvious" user-input paths (fs_alloc_slot and fs_create) and leave the others. That would be a mistake:

• fs_mount() reads metadata from disk. If the disk image was created by a vulnerable version of this code (or crafted by an attacker), the meta->name field could contain an oversized string. Fixing the mount path closes this re-entry vector.
• fs_save() serializes files[fd].name back to disk. If a truncated-but-valid name is in memory, this ensures the on-disk representation is also safely bounded, maintaining consistency between the in-memory and on-disk representations.

Prevention & Best Practices

1. Never Use Unbounded String Copy Functions with User Input

strcpy(), strcat(), and gets() have no place in code that processes external input. In C, always reach for bounded alternatives:

Note that strncpy() from the standard library is also problematic — it does not guarantee null termination when the source is longer than n. The safe_strncpy() introduced here explicitly handles this.

2. Define and Enforce Buffer Size Constants Consistently

The fix works because MAX_FILENAME is used both to declare the buffer size and as the bound in safe_strncpy(). This consistency is essential. If the buffer size and the copy limit diverge, you get off-by-one errors or re-opened vulnerabilities. Consider:

#define MAX_FILENAME 256

typedef struct {
    char name[MAX_FILENAME];  // buffer defined with constant
    // ...
} file_entry_t;

// Copy also uses the same constant
safe_strncpy(entry.name, user_input, MAX_FILENAME);

3. Treat All External Data as Adversarial — Including Disk Data

The fs_mount() call site illustrates an important principle: data read from disk is external data. It may have been written by a different version of your code, a different machine, or a malicious actor. Never assume on-disk data is safe to copy without bounds checking.

4. Use Static Analysis and Compiler Warnings

Tools that can catch this class of issue early:
- -Wall -Wformat-overflow (GCC/Clang) — catches some format string and overflow issues
- AddressSanitizer (-fsanitize=address) — runtime detection of buffer overflows during testing
- cppcheck — static analysis that flags unsafe string function usage
- clang-tidy with bugprone-not-null-terminated-result — catches misuse of string functions

5. Relevant Standards

• CWE-120: Buffer Copy without Checking Size of Input ("Classic Buffer Overflow")
• CWE-676: Use of Potentially Dangerous Function (specifically flags strcpy)
• OWASP: Buffer Overflow
• SEI CERT C: STR31-C — Guarantee that storage for strings has sufficient space for character data and the null terminator

Key Takeaways

• Custom strcpy() implementations carry the same risks as the standard one — if you're writing your own byte-copy loop without a length parameter, you've just reinvented an unsafe function. The FreezeProject/src/fs.c custom strcpy() had no n parameter and no bounds awareness.

• All four call sites needed fixing, not just the most obvious one — the fs_mount() and fs_save() paths are easy to overlook, but both handle data that can be attacker-influenced, making them equally dangerous.

• safe_strncpy() always null-terminates — unlike strncpy() from the C standard library, the new helper guarantees a null terminator even when truncation occurs. This prevents a separate class of read-overrun bugs downstream.

• Buffer size constants must be shared between declaration and copy — using MAX_FILENAME in both the struct definition and the safe_strncpy() call ensures the two stay in sync when the constant is updated.

• Filesystem code is a high-value target — code that handles filenames, paths, and metadata is frequently exposed to user-controlled input. It deserves the same scrutiny as network-facing code.

Conclusion

This vulnerability in FreezeProject/src/fs.c is a clear example of how a seemingly simple custom helper function — an unbounded strcpy() — can create a critical security exposure when it's used at multiple points in security-sensitive code. The fix is elegant in its simplicity: an eight-line safe_strncpy() helper that enforces MAX_FILENAME at every copy site, ensuring that no amount of adversarial input can push data past the end of a filename buffer.

The broader lesson is one of discipline: in C, every string copy touching external input must be bounded, and the bound must match the actual destination buffer size. When you find yourself writing a custom string copy function, ask whether it has a length parameter — and if it doesn't, that's your first warning sign.

Security in low-level filesystem code isn't optional. The filenames that users provide are, by definition, attacker-controlled. Treat them accordingly.

This fix was identified and applied automatically by OrbisAI Security. Automated security scanning caught all four vulnerable call sites simultaneously, ensuring no partial fix left residual risk.