How Heap Buffer Overflows Happen in C++ ZIP Extraction and How to Fix Them

How Heap Buffer Overflows Happen in C++ ZIP Extraction and How to Fix Them

Summary

A critical heap buffer overflow was discovered and fixed in TKLiveSync/unzip.cpp, a production C++ file responsible for extracting ZIP archives in a Node.js native module. The vulnerable code used strcpy() to copy untrusted ZIP entry names into a PATH_MAX-sized heap buffer — a pattern that is guaranteed to overflow when processing a crafted archive with long filenames. The fix eliminates the unsafe buffer entirely, replaces dirname() with std::string operations, and introduces an entry-name validator that blocks both oversized names and path traversal attempts.

Introduction

The TKLiveSync/unzip.cpp file handles ZIP archive extraction for a Node.js native library. Its job is to iterate over every entry in a ZIP file, recreate the directory structure, and write file contents to a destination path. That sounds routine — but a flaw in how entry names were handled before directory creation created a textbook heap buffer overflow.

The offending pattern was on line 49, inside the unzip() function:

auto pathcopy = new char[PATH_MAX];
// ... later, inside the loop:
strcpy(pathcopy, name);
auto path = dirname(pathcopy);

pathcopy is allocated exactly PATH_MAX bytes on the heap — typically 4,096 bytes on Linux and 1,024 bytes on macOS. Then strcpy() copies name — the raw entry name from the ZIP archive — directly into that buffer with zero length checking.

The ZIP specification (PKWARE APPNOTE, section 4.4.17) allows entry names up to 65,535 bytes. Any attacker who can deliver a crafted ZIP file to a consumer of this library can trigger a heap overflow with a single entry whose name exceeds PATH_MAX.

The Vulnerability Explained

The Dangerous Code Path

Here is the vulnerable loop body as it existed before the fix:

auto pathcopy = new char[PATH_MAX];   // heap buffer, PATH_MAX bytes

for (zip_int64_t i = 0; i < num; i++) {
    zip_stat_index(z, i, ZIP_STAT_MTIME, &sb);
    auto name = sb.name;              // raw, untrusted ZIP entry name

    std::string assetFullname{ destination };
    assetFullname.append("/");
    assetFullname.append(name);

    strcpy(pathcopy, name);           // ← OVERFLOW HERE if len(name) >= PATH_MAX
    auto path = dirname(pathcopy);
    std::string dirFullname(destination);
    dirFullname.append("/");
    dirFullname.append(path);
    mkdir_rec(dirFullname.c_str());
    // ... file extraction continues
}

The problem is straightforward: strcpy() has no length parameter. It copies bytes from name into pathcopy until it hits a null terminator, regardless of how large pathcopy is. When name is 65,535 bytes (the ZIP spec maximum), strcpy() writes approximately 61,000 bytes past the end of a 4,096-byte heap allocation.

Why the ZIP Spec Makes This Worse

This isn't a theoretical edge case. The ZIP format stores filename length as a 16-bit unsigned integer in the local file header (offset 26), meaning any value from 0 to 65,535 is structurally valid. A malicious archive creator does not need to exploit any other vulnerability — they simply name a file with a 65,000-character string and package it into a .zip file. Tools like Python's zipfile module or raw binary construction make this trivial.

Exploitation Scenario

Consider this attack chain for a downstream Node.js application using this library:

1. An attacker crafts a ZIP archive containing one entry with a filename of 65,535 A characters.
2. The application receives this archive (via upload, download, or sync operation) and calls unzip().
3. strcpy(pathcopy, name) writes 65,535 bytes into a 4,096-byte heap buffer.
4. The overflow corrupts heap metadata and adjacent allocations.
5. Depending on the heap layout, this can cause a crash (denial of service) or, with careful heap grooming, enable arbitrary code execution.

Because pathcopy is allocated with new char[PATH_MAX] and never freed inside the loop, there is also a secondary memory leak — but the overflow is the critical issue.

Additional Risk: No Path Traversal Validation

The original code also lacked any check for path traversal sequences. An entry named ../../etc/passwd would pass straight through to assetFullname and dirFullname, potentially writing files outside the intended destination directory. The original dirname() call would silently process the .. components.

The Fix

The fix addresses both problems — the buffer overflow and the path traversal risk — with two coordinated changes.

Change 1: The is_safe_entry_name() Validator

A new static helper function is added before unzip():

// ZIP entry names are untrusted input: reject absolute paths and ".."
// components so extraction can never write outside `destination`.
static bool is_safe_entry_name(const char* name)
{
    if (name == nullptr || *name == '\0' || *name == '/')
        return false;

    for (const char* p = name; *p;) {
        const char* component = p;
        while (*p && *p != '/')
            p++;
        if (p - component == 2 && component[0] == '.' && component[1] == '.')
            return false;
        if (*p == '/')
            p++;
    }

    return true;
}

This function enforces three rules before any memory operation occurs:

• Null/empty rejection: nullptr and empty strings are invalid.
• Absolute path rejection: Names starting with / are rejected, preventing writes to absolute filesystem paths.
• .. component rejection: Each path component is checked character by character. Any component that is exactly .. causes the function to return false, blocking directory traversal.

The function is called at the top of the extraction loop, and entries that fail validation are simply skipped with continue — no memory is touched.

Change 2: Replacing strcpy/dirname with std::string

The pathcopy heap buffer and strcpy() call are removed entirely. Directory extraction now uses std::string::find_last_of():

Before:

auto pathcopy = new char[PATH_MAX];
// ...
strcpy(pathcopy, name);
auto path = dirname(pathcopy);
std::string dirFullname(destination);
dirFullname.append("/");
dirFullname.append(path);
mkdir_rec(dirFullname.c_str());

After:

std::string entryName{ name };
auto separator = entryName.find_last_of('/');

if (separator != std::string::npos) {
    std::string dirFullname{ destination };
    dirFullname.append("/");
    dirFullname.append(entryName.substr(0, separator));
    mkdir_rec(dirFullname.c_str());
}

std::string manages its own memory dynamically — it allocates exactly as much space as needed for the string content, regardless of length. There is no fixed-size buffer to overflow. The #include <libgen.h> header (which provided dirname()) is also removed, since it is no longer needed.

Why This Combination Works

The is_safe_entry_name() check runs before any string construction. By the time std::string entryName{ name } executes, the entry name has already been validated to be non-null, non-empty, non-absolute, and free of .. components. The std::string operations then handle arbitrary-length names safely because they manage heap allocation internally.

Key Takeaways

• strcpy(pathcopy, name) in unzip.cpp was the single line that made this critical. A PATH_MAX-sized allocation is not a safe bound for ZIP entry names, which the spec allows up to 65,535 bytes.
• ZIP entry names are untrusted input, always. The sb.name field in zip_stat comes directly from the archive file and must be validated before any memory operation — not after.
• Replacing dirname() with std::string::find_last_of('/') eliminates both the overflow and the dependency on <libgen.h>. The fix is strictly simpler code, not just safer code.
• Path traversal and buffer overflow often coexist in archive extraction code. The is_safe_entry_name() function addresses both in a single, readable validation pass.
• new char[PATH_MAX] allocated outside a loop but used inside it is a memory-safety red flag. If the buffer cannot be proven to be large enough for every possible iteration input, it should be replaced with a dynamically sized type.

How Orbis AppSec Detected This

• Source: The sb.name field returned by zip_stat_index() in unzip.cpp — raw, attacker-controlled ZIP entry name data read directly from the archive file.
• Sink: strcpy(pathcopy, name) at line 49 of TKLiveSync/unzip.cpp, where the untrusted name is copied into the fixed PATH_MAX-sized heap buffer pathcopy.
• Missing control: No length check was performed before the copy. The code did not compare strlen(name) against PATH_MAX, did not use a bounded copy function, and did not validate path components for traversal sequences.
• CWE: CWE-120 — Buffer Copy without Checking Size of Input ("Classic Buffer Overflow").
• Fix: The pathcopy buffer and strcpy() call were removed and replaced with std::string operations, and a new is_safe_entry_name() validator was added to reject unsafe entry names before any string manipulation.

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

A single strcpy() call in a ZIP extraction loop turned a routine file operation into a critical heap buffer overflow. The root cause was the mismatch between a PATH_MAX-sized allocation and the ZIP specification's 65,535-byte maximum entry name length — a gap large enough to drive a truck through. The fix is a model of defensive C++: validate untrusted input at the boundary with is_safe_entry_name(), then use std::string to eliminate fixed-size buffer concerns entirely.

If you maintain C or C++ code that processes archive files, treat every field in the archive metadata as hostile input. Validate lengths, reject traversal sequences, and prefer RAII string types over raw char arrays. These habits close an entire category of vulnerabilities before they ever reach a security scanner.