How stack buffer overflow happens in C memcpy() with caller-controlled length and how to fix it

Introduction

In the GDI/Comdlg32.cpp file, which provides wrapper functions for Windows common dialog operations, a critical stack buffer overflow vulnerability lurked in four seemingly innocent memcpy() calls. The functions comdlg_GetOpenFileNameA, comdlg_GetOpenFileNameW, comdlg_GetSaveFileNameA, and comdlg_GetSaveFileNameW all shared the same dangerous pattern: they trusted a caller-supplied size field to determine how many bytes to copy onto the stack.

At lines 57, 80, 103, and 126, the code performed:

memcpy(&OpenFile, lpOpenFile, lpOpenFile->lStructSize);

This single line is a textbook example of why you should never trust caller-controlled data for memory operations. The lStructSize field in the OPENFILENAME structure is meant to indicate the structure's size for versioning purposes, but nothing stops a malicious caller from setting it to an arbitrarily large value.

The Vulnerability Explained

What Made This Code Dangerous

The vulnerable pattern appeared in the comdlg_GetOpenFileNameA function at line 57:

BOOL WINAPI comdlg_GetOpenFileNameA(LPOPENFILENAMEA lpOpenFile)
{
    if (lpOpenFile && (lpOpenFile->Flags & OFN_ENABLEHOOK))
    {
        OPENFILENAMEA OpenFile = {};
        memcpy(&OpenFile, lpOpenFile, lpOpenFile->lStructSize);  // VULNERABLE!
        UpdateOpenFileNameStruct(OpenFile);
        return GetOpenFileName(&OpenFile);
    }
    // ...
}

Here's the problem: OpenFile is a stack-allocated structure with a fixed size determined at compile time by sizeof(OPENFILENAMEA). However, the memcpy() call uses lpOpenFile->lStructSize as the copy length—a value completely controlled by the caller.

Attack Scenario

An attacker could craft a malicious OPENFILENAME structure like this:

// Attacker's code
std::vector<uint8_t> malicious_buffer(0x1000, 0x41);  // 4KB of 'A's
OPENFILENAMEA* crafted = reinterpret_cast<OPENFILENAMEA*>(malicious_buffer.data());
crafted->lStructSize = 0x1000;  // Claim the structure is 4KB
crafted->Flags = OFN_ENABLEHOOK;  // Trigger the vulnerable code path

// Embed shellcode or ROP gadgets at the right offset to overwrite return address
// ...

comdlg_GetOpenFileNameA(crafted);  // BOOM - stack smash

When memcpy() executes with a 4KB length but only a ~100-byte destination buffer, it writes far beyond OpenFile, overwriting:
- Other local variables
- Saved frame pointer
- Return address (critical for exploitation)
- Potentially reaching into adjacent stack frames

This is a classic stack smashing attack that can lead to arbitrary code execution.

Why This Pattern Repeated Four Times

The same vulnerable pattern existed in all four dialog wrapper functions:
- comdlg_GetOpenFileNameA (line 57) - ANSI version of file open
- comdlg_GetOpenFileNameW (line 80) - Unicode version of file open
- comdlg_GetSaveFileNameA (line 103) - ANSI version of file save
- comdlg_GetSaveFileNameW (line 126) - Unicode version of file save

Copy-paste programming propagated the vulnerability across all variants of the dialog functions.

The Fix

The fix is elegant in its simplicity: clamp the copy length to never exceed the destination buffer size.

Before (Vulnerable)

memcpy(&OpenFile, lpOpenFile, lpOpenFile->lStructSize);

After (Fixed)

memcpy(&OpenFile, lpOpenFile, min(lpOpenFile->lStructSize, sizeof(OpenFile)));

The min() macro ensures that even if lStructSize is set to a billion bytes, the actual copy will never exceed sizeof(OpenFile)—the exact size of the stack-allocated destination buffer.

Applied Across All Four Locations

The fix was applied consistently to all vulnerable call sites:

Line 57 (GetOpenFileNameA):

-       memcpy(&OpenFile, lpOpenFile, lpOpenFile->lStructSize);
+       memcpy(&OpenFile, lpOpenFile, min(lpOpenFile->lStructSize, sizeof(OpenFile)));

Line 80 (GetOpenFileNameW):

-       memcpy(&OpenFile, lpOpenFile, lpOpenFile->lStructSize);
+       memcpy(&OpenFile, lpOpenFile, min(lpOpenFile->lStructSize, sizeof(OpenFile)));

Line 103 (GetSaveFileNameA):

-       memcpy(&OpenFile, lpOpenFile, lpOpenFile->lStructSize);
+       memcpy(&OpenFile, lpOpenFile, min(lpOpenFile->lStructSize, sizeof(OpenFile)));

Line 126 (GetSaveFileNameW):

-       memcpy(&OpenFile, lpOpenFile, lpOpenFile->lStructSize);
+       memcpy(&OpenFile, lpOpenFile, min(lpOpenFile->lStructSize, sizeof(OpenFile)));

Why This Fix Works

The security invariant is now enforced: buffer writes never exceed the declared destination size. Even with a maliciously crafted lStructSize of 0xFFFFFFFF, the copy is bounded to exactly sizeof(OPENFILENAME) bytes—safe for the stack-allocated buffer.

Key Takeaways

• Never use lStructSize or similar caller-controlled fields directly as memcpy() lengths — always clamp to sizeof(destination)
• The min() pattern is your friend — min(user_length, sizeof(buffer)) is a defensive programming essential
• Copy-paste vulnerabilities multiply — when fixing one instance, search for the same pattern throughout the codebase (this fix addressed 4 locations)
• Stack-allocated buffers are especially dangerous — overflow can directly corrupt return addresses, enabling RCE
• Regression tests should encode security invariants — the added test suite verifies the fix with adversarial inputs including sizeof(OPENFILENAME) * 10 and 0xFFFF

How Orbis AppSec Detected This

• Source: The lpOpenFile->lStructSize field in caller-provided OPENFILENAME structures
• Sink: memcpy(&OpenFile, lpOpenFile, lpOpenFile->lStructSize) at lines 57, 80, 103, and 126 in GDI/Comdlg32.cpp
• Missing control: No validation that lStructSize does not exceed sizeof(OpenFile) before the copy operation
• CWE: CWE-120 (Buffer Copy without Checking Size of Input)
• Fix: Added min(lpOpenFile->lStructSize, sizeof(OpenFile)) to clamp the copy length to the destination buffer size

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

This vulnerability demonstrates a fundamental truth in systems programming: never trust external data for memory operations. The lStructSize field, while seemingly innocuous as a versioning mechanism, became an attack vector when used directly as a memcpy() length.

The fix—a simple min() call—is both minimal and complete. It preserves functionality for legitimate callers while neutralizing malicious inputs. More importantly, the accompanying regression test suite ensures this security property is maintained as the code evolves.

When working with C/C++ code that handles external structures, always ask: "What happens if this size field is larger than I expect?" If the answer involves writing past buffer boundaries, you've found a vulnerability waiting to happen.