Skip to content

CWE-676: Use of Potentially Dangerous Function - C

Overview

C and C++ carry a set of standard library functions that are dangerous by convention rather than by defect: strcpy(), strcat(), sprintf(), and system() are still current, standard API, and each has a documented way to use it safely. The danger is that the unsafe way to call them - an unbounded copy, a shell command built from untrusted text - is exactly as easy to write as the safe way, and nothing in the function signature stops it.

Common Vulnerable Patterns

Unbounded String Copies

// VULNERABLE - no destination capacity checked
char dest[64];
strcpy(dest, user_input);       // overflow if user_input is 64 bytes or longer
strcat(dest, more_input);       // overflow if the combined length exceeds dest's capacity
sprintf(dest, "%s", user_input); // no bound on the formatted output length

Why this is vulnerable: None of strcpy(), strcat(), or sprintf() take the destination buffer's size as an argument. Each writes however much input it's given, so the only bound is a check the caller wrote by hand before the call - easy to omit, and easy to invalidate later when a refactor changes the size that check assumed.

Shell Commands Built From Untrusted Text

// VULNERABLE - command injection
char cmd[256];
snprintf(cmd, sizeof(cmd), "cp %s %s.bak", filename, filename);
system(cmd);  // the shell parses filename, so it can inject additional commands

// Attacker-controlled filename: "a; rm -rf /"

Why this is vulnerable: system() hands the entire string to a shell, which interprets metacharacters (;, |, &, `, $()) as command separators and substitutions. Building that string by concatenating untrusted input means the attacker controls part of what the shell executes, not just the value of one argument.

Secure Patterns

Bounds-Checked String Handling

#include <stdio.h>
#include <string.h>

void build_value(const char *user_input, const char *more_input) {
    char dest[64];

    // bounded formatted output - always terminates, and reports truncation
    int n = snprintf(dest, sizeof(dest), "%s", user_input);
    if (n < 0 || (size_t)n >= sizeof(dest)) {
        // truncated: dest is still a valid string, but it is not the whole input
    }

    // strlcpy/strlcat null-terminate on every path (BSD, macOS, and glibc 2.38+)
    strlcpy(dest, user_input, sizeof(dest));
    strlcat(dest, more_input, sizeof(dest));

    // strncpy is bounded but NOT guaranteed to terminate - the explicit write is required
    strncpy(dest, user_input, sizeof(dest) - 1);
    dest[sizeof(dest) - 1] = '\0';
}

Why this works: Every replacement takes the destination's capacity as an explicit argument and stops writing at that limit, so oversized input is truncated instead of overflowing adjacent memory. snprintf() and strlcpy()/strlcat() write a terminator on every path including truncation, which is why they lead here; strlcpy/strlcat are available in glibc 2.38 and later as well as on the BSDs and macOS.

strncpy() is listed last because it is bounded without being terminating. It writes no terminator whenever the source is as long as or longer than its size argument - with sizeof(dest) - 1 passed above, that means any user_input of 63 bytes or more, not only one that fills the buffer exactly. Swapping strcpy for strncpy and stopping there converts a buffer overflow into an unterminated string, which is CWE-170 and satisfies the scanner either way, so the explicit terminator write is part of the fix rather than a nicety.

Truncation is also a decision, not a default. All three of these silently drop the tail of an oversized value, which is right for a log line and wrong for a path, a hostname, or anything compared against an allowlist later - check the return of snprintf/strlcpy against the buffer size and reject where a shortened value would mean something different from the original.

Parameterized Process Execution

#include <unistd.h>
#include <sys/wait.h>

int backup_file(const char *filename, const char *backup_name) {
    pid_t pid = fork();
    if (pid == -1) {
        return -1;                  // fork failed - there is no child to wait for
    }
    if (pid == 0) {
        // child process - arguments passed directly, never through a shell.
        // "--" ends cp's options, so a filename beginning with '-' stays a filename.
        char *args[] = {"cp", "--", (char *)filename, (char *)backup_name, NULL};
        execvp("cp", args);
        _exit(127);                 // only reached if execvp fails
    }

    int status;
    if (waitpid(pid, &status, 0) == -1) {
        return -1;
    }
    return (WIFEXITED(status) && WEXITSTATUS(status) == 0) ? 0 : -1;
}

Why this works: execvp() passes each argument to the target program directly, as an array entry - there's no shell in between to parse metacharacters in filename. A value such as a; rm -rf / arrives at cp as one argument: a strange filename, not a second command.

That closes command injection and nothing else, which is the part most often missed. The argument list controls how the shell reads the value; it says nothing about how cp reads it. Without the separator, a filename of --target-directory=/var/www or -r is an option rather than an operand, so the attacker who lost the shell can still change where the copy lands or what it recurses into. -- tells cp that everything after it is a file operand, whatever it starts with. Where the value is a path rather than an arbitrary string, resolve it against a known base directory as well, since execvp is equally happy to hand cp a path pointing anywhere on the filesystem.

execvp() also searches PATH, so it runs whichever cp the environment names. In a process whose environment an attacker can influence, use execv() with an absolute path, or sanitise PATH before the call.

Considerations

A dangerous function is not automatically a finding. system() invoked with a fixed string the application wrote, or strcpy into a buffer whose size is known at compile time and provably sufficient, is not attacker-reachable and does not need rewriting. What it does need is a comment at the call site saying why it is safe - otherwise the next reader either "fixes" working code or, worse, assumes a neighbouring call was reviewed to the same standard when it was not.

Testing

  • Compile with AddressSanitizer (-fsanitize=address) and fuzz string-handling call sites with inputs at, below, and above the destination's capacity.
  • For every replaced system()/popen() call, test with shell metacharacters (| & $() \) in the untrusted portion and confirm they have no special effect after the fix.
  • Test the same call with a filename that begins with a hyphen - --help, -r, or a real option of the program being run - and confirm it is treated as a file operand rather than a flag. This is the failure execvp() alone does not prevent, so it passes only where the -- separator is present; a suite that stops at metacharacters will report the argument-array rewrite as complete when it is half done.
  • Re-run static analysis (e.g. cppcheck, Clang static analyzer) to confirm no remaining unguarded calls to the banned functions.

Additional Resources