How insecure string copy functions happen in C calculations.c and how to fix it

How Insecure String Copy Functions Happen in C calculations.c and How to Fix It

Introduction

The src/calculations.c file is the computational heart of this CLI tool — it handles loading output entries, tracking conductivity values, and assembling structured data for downstream processing. Inside the add_loading_output() function, a single strncpy call at line 37 was responsible for copying a string label into the entry->type field of a context structure. On the surface, the code looked responsible: it used strncpy (not the famously dangerous strcpy) and even included a manual null-terminator assignment on the following line. But this two-step pattern contains a subtle, high-severity flaw that Semgrep caught and that a code reviewer could easily miss.

This post breaks down exactly why that pattern is dangerous, how an attacker with control over CLI arguments or input files could exploit it, and how a single-line snprintf replacement closes the vulnerability permanently.

The Vulnerability Explained

The Vulnerable Code

Here is the code as it existed before the fix, in src/calculations.c around line 37:

strncpy(entry->type, type, sizeof(entry->type) - 1);
entry->type[sizeof(entry->type) - 1] = '\0';

At first glance, this looks like a "safe" use of strncpy. The developer:
1. Used strncpy instead of strcpy (good instinct).
2. Passed sizeof(entry->type) - 1 as the size limit to avoid writing past the buffer.
3. Manually null-terminated the last byte of the buffer.

So what's wrong?

Why strncpy Is Still Dangerous Here

strncpy has two well-known failure modes:

1. No null-termination on exact or overflow fit: If the source string type is exactly sizeof(entry->type) - 1 characters or longer, strncpy fills the buffer completely but writes no null terminator. The manual entry->type[sizeof(entry->type) - 1] = '\0' on the next line patches this — but only if both lines always execute together, in the correct order, with no intervening logic.

2. Fragility of the two-step pattern: The correctness of this approach depends on two separate statements being kept in sync. If a future developer adds an early return, a conditional branch, or refactors the function, the null-termination line can be separated from the strncpy call, silently reintroducing the vulnerability.

3. strncpy pads with zeros on short strings: When the source is shorter than the count, strncpy zero-fills the remainder of the destination — a behavior that is almost never desired and can mask bugs in downstream code that relies on the buffer's content.

The Real-World Attack Scenario

This is a local CLI tool where the threat model explicitly notes that attackers can control command-line arguments or input files. The type parameter passed to add_loading_output() originates from parsed input data. Consider this scenario:

1. An attacker crafts a malicious input file where a loading type label is exactly the size of entry->type (e.g., 16 characters if the buffer is 16 bytes).
2. Due to a refactoring error that removed the manual null-termination line (easy to miss in a code review), entry->type is no longer null-terminated.
3. A subsequent strlen(entry->type), printf("%s", entry->type), or string comparison walks past the end of the buffer, reading adjacent memory.
4. Depending on what follows entry->type in the context_t struct, this could leak sensitive values, corrupt a length field, or — in the worst case — enable a write-what-where condition.

Even with the null-termination line present, the pattern is a maintenance trap: the next developer to touch this function may not realize both lines are a coupled safety mechanism.

The Fix

What Changed

The fix replaces the fragile two-line strncpy pattern with a single, atomic snprintf call:

Before:

strncpy(entry->type, type, sizeof(entry->type) - 1);
entry->type[sizeof(entry->type) - 1] = '\0';

After:

snprintf(entry->type, sizeof(entry->type), "%s", type);

Why This Fix Works

snprintf provides three critical guarantees that make it strictly superior here:

By passing sizeof(entry->type) (not sizeof(entry->type) - 1) as the size argument, snprintf writes at most sizeof(entry->type) - 1 characters of the format result and always places a null terminator in the final byte. This is guaranteed by the C standard for snprintf regardless of whether the source overflows the buffer.

The "%s" format specifier is intentional: it treats type as a plain string with no format interpretation, avoiding any secondary format-string injection risk that a naive snprintf(entry->type, sizeof(entry->type), type) would introduce.

Note on strcpy_s and strlcpy

The Semgrep rule suggests strcpy_s (C11 Annex K), but this is an optional extension not universally available. The PR title also mentions strlcpy, which is available on BSD/macOS but not in glibc by default. snprintf is the most portable safe alternative across all standard C environments, making it the right choice for a cross-platform CLI tool.

Prevention & Best Practices

1. Ban strcpy and strncpy at the Project Level

Add a Semgrep rule or compiler warning configuration that fails the build on any use of strcpy or strncpy. This makes the policy machine-enforced rather than relying on code review.

# .semgrep.yml
rules:
  - id: no-strcpy-strncpy
    patterns:
      - pattern: strcpy(...)
      - pattern: strncpy(...)
    message: "Use snprintf() or strlcpy() instead of strcpy/strncpy"
    severity: ERROR
    languages: [c, cpp]

2. Prefer snprintf for All String Copies Into Fixed Buffers

The pattern snprintf(dest, sizeof(dest), "%s", src) is the most portable safe string copy in C. Use it consistently.

3. Use strlcpy Where Available

On platforms with strlcpy (BSD, macOS, or via libbsd on Linux):

strlcpy(entry->type, type, sizeof(entry->type));

strlcpy always null-terminates and returns the length of the source string (not the destination), making truncation detection straightforward.

4. Validate Input Length Before Copying

For the add_loading_output() function specifically, consider asserting or logging when the type string is truncated:

if (strlen(type) >= sizeof(entry->type)) {
    // Log warning: type label truncated
}
snprintf(entry->type, sizeof(entry->type), "%s", type);

5. Relevant Standards

• CWE-120: Buffer Copy without Checking Size of Input ("Classic Buffer Overflow")
• CWE-676: Use of Potentially Dangerous Function
• OWASP: Buffer Overflow
• SEI CERT C: STR07-C — Use the bounds-checking interfaces for string manipulation

Key Takeaways

• The two-step strncpy + manual null-termination pattern in add_loading_output() is a maintenance trap: both lines must always execute together, and any future refactoring that separates them silently reintroduces the vulnerability.
• strncpy does not guarantee null-termination when the source string length equals or exceeds sizeof(dest) - 1 — the exact condition that matters most for security.
• snprintf(entry->type, sizeof(entry->type), "%s", type) is one line, always safe, always null-terminated, and requires no follow-up statement to be correct.
• The "%s" format specifier in the fix is not cosmetic — it prevents the type parameter from being interpreted as a format string, closing a secondary injection vector.
• Static analysis (Semgrep) caught this in production code that had already gone through human review, demonstrating why automated scanning is an essential layer of defense for C codebases.

How Orbis AppSec Detected This

• Source: The type parameter of add_loading_output() in src/calculations.c, which originates from parsed CLI arguments or input files controlled by the user.
• Sink: strncpy(entry->type, type, sizeof(entry->type) - 1) at src/calculations.c:37, where the user-influenced string is copied into a fixed-size struct field without atomic null-termination guarantees.
• Missing control: The two-line pattern relied on a manually written null-terminator that could be separated from the copy in future refactors; no single atomic operation ensured both bounds-safety and null-termination simultaneously.
• CWE: CWE-120 — Buffer Copy without Checking Size of Input ("Classic Buffer Overflow"), and CWE-676 — Use of Potentially Dangerous Function.
• Fix: Replaced the two-line strncpy + null-termination pattern with snprintf(entry->type, sizeof(entry->type), "%s", type), which atomically enforces bounds and guarantees null-termination per the C standard.

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

The vulnerability in src/calculations.c is a textbook example of why "almost safe" C string handling is still dangerous. The original strncpy usage was clearly written by someone thinking about safety — they chose strncpy over strcpy and added a null-terminator. But the two-step pattern is inherently fragile: it requires two lines of code to remain coupled forever, in a codebase that will continue to evolve. The snprintf replacement collapses that logic into a single, self-contained, standard-guaranteed operation.

For C developers, the lesson is clear: treat strncpy as a deprecated function for string copying purposes. Reach for snprintf(dest, sizeof(dest), "%s", src) as your default safe string copy idiom — it's portable, atomic, and leaves no room for the null-termination mistake that has caused countless real-world vulnerabilities.

References

• CWE-120: Buffer Copy without Checking Size of Input
• CWE-676: Use of Potentially Dangerous Function
• OWASP Buffer Overflow Vulnerability
• SEI CERT C Coding Standard: STR07-C
• cppreference: snprintf
• Semgrep rule: insecure-use-string-copy-fn
• fix: use bounded strlcpy/snprintf in calculations.c...