How buffer overflow happens in C dcraw_lz.c nikon_3700() and how to fix it

Vulnerability at a glance: CWE-120 buffer overflow in nikon_3700() — two unchecked strcpy() calls copy camera metadata into 64-byte buffers. Fixed by replacing with bounds-checked strncpy() plus explicit null-termination.

Summary

A critical buffer overflow vulnerability was found in lightcrafts/coprocesses/dcraw/dcraw_lz.c at line 1334. The nikon_3700() function used strcpy() to copy camera make and model strings from a lookup table into fixed-size 64-byte character arrays — with zero bounds checking. A crafted RAW image file containing oversized make/model metadata fields could trigger memory corruption, potentially leading to arbitrary code execution. The fix replaces both strcpy() calls with strncpy() bounded to the buffer size, followed by explicit null-termination.

Introduction

The lightcrafts/coprocesses/dcraw/dcraw_lz.c file is a C implementation of the widely-used dcraw RAW image decoder, responsible for parsing and demosaicing RAW photographs from digital cameras. Deep inside this decoder, the nikon_3700() function handles a quirk specific to early Nikon 3700-era cameras: it reads a bitmask from the image's binary data, looks it up in a hardcoded table of known camera configurations, and copies the matching make and model strings into global character buffers.

That copy operation — two innocuous-looking lines — contained a critical flaw:

strcpy (make,  table[i].make );
strcpy (model, table[i].model);

The make and model buffers are fixed-size arrays (64 bytes each, as is standard in dcraw). strcpy() doesn't know or care about that limit. If any string in the lookup table — or more dangerously, any string injected via a crafted RAW file — exceeds 63 characters, bytes spill past the buffer boundary and overwrite adjacent memory.

The Vulnerability Explained

What strcpy() Actually Does

strcpy(dest, src) copies bytes from src to dest, one at a time, until it encounters a null byte (\0). It performs no length check. The C standard explicitly states this is undefined behavior when the source is longer than the destination — but undefined behavior in C often means "overwrite whatever is next in memory."

The Specific Vulnerable Code

Here is the exact vulnerable code from nikon_3700() at line 1334:

// VULNERABLE - no bounds checking
void CLASS nikon_3700()
{
  // ...
  for (i=0; i < sizeof table / sizeof *table; i++)
    if (bits == table[i].bits) {
      strcpy (make,  table[i].make );   // ← CWE-120: no length check
      strcpy (model, table[i].model);   // ← CWE-120: no length check
    }
}

The make and model variables are char arrays declared elsewhere in dcraw, typically as:

char make[64], model[64];

That means strcpy() is writing into a 64-byte slot. If table[i].make or table[i].model contains a string of 64 characters or more (including the null terminator, that means anything ≥ 64 characters in length), the write overflows the buffer.

How Could This Be Exploited?

dcraw processes RAW image files — binary files whose metadata fields, including camera make and model, are read from disk and parsed. An attacker who can supply a crafted RAW file to an application using this library has direct influence over the content that flows into these strcpy() calls.

Attack scenario:

1. An attacker crafts a Nikon RAW file whose embedded metadata causes bits to match one of the entries in table[].
2. The corresponding table[i].make or table[i].model string is longer than 63 bytes.
3. strcpy() faithfully copies all of it, writing past the end of make[64] or model[64].
4. Depending on memory layout (stack vs. heap), this overwrites adjacent variables, return addresses, or heap metadata.
5. With careful crafting, an attacker can redirect execution — achieving arbitrary code execution in the context of the application processing the image.

Applications that batch-process user-uploaded RAW files (photo editors, media servers, cloud photo libraries) are particularly exposed, since they process attacker-controlled files without user interaction.

Real-World Impact for This Component

LightZone, the application containing this code, is a photo editing tool that processes RAW files from a wide variety of cameras. Any workflow that opens untrusted RAW files — including plugins, batch importers, or web-facing upload endpoints — could be exploited via this path. Because dcraw is a widely forked library, variants of this vulnerability exist in many image-processing codebases.

The Fix

The fix is surgical and correct. Both strcpy() calls are replaced with strncpy() bounded to one less than the buffer size, immediately followed by explicit null-termination:

Before (Vulnerable)

strcpy (make,  table[i].make );
strcpy (model, table[i].model);

After (Fixed)

strncpy (make,  table[i].make,  sizeof make  - 1); make[sizeof make   - 1] = '\0';
strncpy (model, table[i].model, sizeof model - 1); model[sizeof model - 1] = '\0';

Why Each Part of the Fix Matters

strncpy(..., sizeof make - 1)
The third argument to strncpy() caps the number of bytes written. Using sizeof make - 1 (i.e., 63) means at most 63 bytes of the source string are copied, leaving room for the null terminator. Critically, sizeof make is evaluated at compile time from the actual buffer declaration — it's not a magic number that could drift out of sync.

make[sizeof make - 1] = '\0'
This is the part many developers forget. strncpy() does not guarantee null-termination when the source string is longer than the limit — it simply stops copying and leaves the remaining destination bytes as-is (or zero-filled, depending on the implementation). Explicitly setting the last byte to '\0' ensures the buffer is always a valid C string, regardless of input length.

sizeof make vs. a hardcoded 64
Using sizeof make instead of 64 is a defensive coding practice: if the buffer size ever changes in a refactor, the bounds check automatically updates. Hardcoded sizes are a maintenance hazard.

This two-part pattern — strncpy + explicit null-termination — is the canonical safe replacement for strcpy in fixed-size buffer scenarios in C.

Prevention & Best Practices

1. Never Use strcpy() with External Input

strcpy() has no place in modern C code that handles user-controlled or file-derived data. Treat it as a code smell during review.

2. Use strlcpy() Where Available

strlcpy(dest, src, size) always null-terminates and returns the length of the source string, making truncation detection easy. It's available on BSD systems and macOS natively, and via libbsd on Linux.

// Even cleaner than strncpy + manual null-term
strlcpy(make,  table[i].make,  sizeof make);
strlcpy(model, table[i].model, sizeof model);

3. Validate at the Source

When reading metadata from files, validate field lengths before copying:

if (strlen(table[i].make) >= sizeof make) {
    // Log warning, skip or truncate intentionally
}

4. Enable Compiler Hardening Flags

Modern compilers can catch many buffer overflow patterns:

gcc -Wall -Wextra -Wformat-security -D_FORTIFY_SOURCE=2 -fstack-protector-strong

_FORTIFY_SOURCE=2 replaces strcpy() with a checked version at compile time when the destination buffer size is statically known.

5. Use Static Analysis

Tools that can detect this class of vulnerability:

• Semgrep: c.lang.security.insecure-use-strcpy
• cppcheck: --enable=warning flags unsafe string functions
• Coverity / CodeQL: Taint analysis from file I/O to string copy sinks
• clang-analyzer: alpha.security.ArrayBound

6. CWE and OWASP References

• CWE-120: Buffer Copy without Checking Size of Input ("Classic Buffer Overflow")
• OWASP: Buffer Overflow — describes stack and heap corruption risks
• SEI CERT C: STR31-C — Guarantee sufficient storage for strings

Key Takeaways

• strcpy() into a fixed buffer is always a vulnerability when the source is externally influenced — in nikon_3700(), the source is ultimately derived from RAW file metadata, making this directly exploitable via crafted files.
• strncpy() alone is not sufficient — you must follow it with dest[sizeof(dest) - 1] = '\0' to guarantee null-termination. The fix in this PR correctly does both.
• Use sizeof buffer not a hardcoded integer — strncpy(make, src, sizeof make - 1) is safer than strncpy(make, src, 63) because it stays correct if the buffer is ever resized.
• RAW image parsers are high-value attack surfaces — they process binary data from untrusted files, often in batch workflows, making memory corruption bugs in this layer particularly dangerous.
• Compiler hardening flags (_FORTIFY_SOURCE=2, -fstack-protector-strong) provide a defense-in-depth layer but are not a substitute for fixing the root cause in source code.

How Orbis AppSec Detected This

• Source: Camera make/model strings read from a hardcoded lookup table in nikon_3700(), indexed by bits derived from RAW image binary data (dp[8] and dp[20]), making the effective source attacker-controlled via crafted RAW files.
• Sink: strcpy(make, table[i].make) and strcpy(model, table[i].model) at lightcrafts/coprocesses/dcraw/dcraw_lz.c:1334–1335, writing into fixed 64-byte character arrays with no length check.
• Missing control: No bounds check on the length of table[i].make or table[i].model before the copy; no use of length-limited string copy functions.
• CWE: CWE-120 — Buffer Copy without Checking Size of Input.
• Fix: Both strcpy() calls replaced with strncpy() bounded to sizeof(buffer) - 1, immediately followed by explicit null-termination of the last byte.

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 buffer overflow in nikon_3700() is a textbook example of why strcpy() is considered dangerous in C: it's a one-line mistake that creates a critical, exploitable vulnerability. The fix is equally concise — two lines of strncpy() with explicit null-termination — but the consequences of leaving it unfixed in a RAW image parser could be severe, from memory corruption to full arbitrary code execution via a crafted photo file.

For developers working in C or C++ on file-parsing code: treat every strcpy() as a code review flag. Ask "what is the maximum length of the source, and is it bounded by the destination?" If the answer is "I'm not sure," it's time to switch to a length-limited alternative. The pattern strncpy(dest, src, sizeof(dest) - 1); dest[sizeof(dest) - 1] = '\0'; is your safe default until you can adopt strlcpy() or a higher-level string abstraction.

References

• CWE-120: Buffer Copy without Checking Size of Input
• OWASP: Buffer Overflow
• SEI CERT C STR31-C: Guarantee sufficient storage for strings
• GNU C Library: strncpy documentation
• Semgrep rules: strcpy unsafe usage
• fix: add buffer-length check in dcraw_lz.c