Skip to content

fix(security): fix stack buffer overflow in ZipFile and add resource path validation#188

Open
dcol91863 wants to merge 1 commit intopjasicek:masterfrom
dcol91863:fix/zipfile-buffer-overflow-and-path-traversal
Open

fix(security): fix stack buffer overflow in ZipFile and add resource path validation#188
dcol91863 wants to merge 1 commit intopjasicek:masterfrom
dcol91863:fix/zipfile-buffer-overflow-and-path-traversal

Conversation

@dcol91863
Copy link

Summary

This PR fixes two security issues in the asset-loading code that can be triggered by a malformed or malicious archive file.

  • Stack buffer overflow in ZipFile.cpp (two sites)
  • Directory traversal in ResourceCache

A prior PR (#185) proposed similar fixes but has not been merged. This PR reimplements them cleanly and independently.


Changes

OpenClaw/Engine/Resource/ZipFile.cpp

ZipFile::Init() — bounds check before memcpy

fh.fnameLen is a uint16 (0–65535) read directly from the untrusted ZIP central-directory. It was used as the memcpy count into a fixed 260-byte (_MAX_PATH) stack buffer with no validation. A crafted ASSETS.ZIP can overflow the stack.

Before:

char fileName[_MAX_PATH];
memcpy(fileName, pfh, fh.fnameLen);   // fh.fnameLen up to 65535 — stack overflow
fileName[fh.fnameLen] = 0;            // out-of-bounds write

After:

if (fh.fnameLen >= _MAX_PATH)
{
    success = false;
    break;
}
char fileName[_MAX_PATH];
memcpy(fileName, pfh, fh.fnameLen);
fileName[fh.fnameLen] = 0;

ZipFile::GetFilename() — eliminate intermediate stack buffer

Same root cause. Fixed by constructing the std::string directly from pointer + length:

Before:

char pszDest[_MAX_PATH];
memcpy(pszDest, m_papDir[i]->GetName(), m_papDir[i]->fnameLen);  // overflow
pszDest[m_papDir[i]->fnameLen] = '\0';                            // OOB write
fileName = pszDest;

After:

fileName = std::string(m_papDir[i]->GetName(), m_papDir[i]->fnameLen);

OpenClaw/Engine/Resource/ResourceCache.h + ResourceCache.cpp

Added IsValidResourcePath() which rejects paths containing .. components or embedded null bytes. Called at the top of VGetRawResourceSize() and VGetRawResource() in ResourceRezArchive.

bool IsValidResourcePath(const std::string& path)
{
    if (path.find("..") != std::string::npos)
        return false;
    if (path.find('\0') != std::string::npos)
        return false;
    return true;
}

Testing

  • Normal ASSETS.ZIP loads without regression.
  • A crafted ZIP with fnameLen = 0x01FF (511) now causes Init() to return false cleanly instead of crashing.
  • GetFilename() with the same crafted archive returns an empty string instead of overflowing.

…path validation

ZipFile.cpp — two buffer overflow sites:

1. ZipFile::Init(): fh.fnameLen is a uint16 read directly from the
   untrusted ZIP central-directory (max 65535). It was used as the
   memcpy count into a fixed 260-byte (_MAX_PATH) stack buffer with no
   bounds check. A malformed ASSETS.ZIP could overflow the stack frame.
   Fix: reject any entry whose fnameLen >= _MAX_PATH and fail the open.

2. ZipFile::GetFilename(): same root cause — fnameLen used to index
   past the end of a 260-byte stack buffer.
   Fix: construct the std::string directly from (ptr, len), removing
   the intermediate stack buffer entirely.

ResourceCache.h / ResourceCache.cpp — directory traversal:

Resource names were forwarded to the REZ archive API with no validation,
allowing a crafted archive to request paths containing ".." and escape
the resource root. Added IsValidResourcePath() which rejects paths
containing ".." components or embedded null bytes, and call it at the
top of VGetRawResourceSize() and VGetRawResource().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants