From 1b1ad6f269c9c9206c76f02180202bbca364c0cd Mon Sep 17 00:00:00 2001 From: David Date: Wed, 18 Feb 2026 12:27:11 +0000 Subject: [PATCH] fix(security): fix stack buffer overflow in ZipFile and add resource path validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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(). --- OpenClaw/Engine/Resource/ResourceCache.cpp | 28 ++++++++++++++++++++++ OpenClaw/Engine/Resource/ResourceCache.h | 4 ++++ OpenClaw/Engine/Resource/ZipFile.cpp | 24 ++++++++++++++----- 3 files changed, 50 insertions(+), 6 deletions(-) diff --git a/OpenClaw/Engine/Resource/ResourceCache.cpp b/OpenClaw/Engine/Resource/ResourceCache.cpp index 2abff8d2a3..f6315778bc 100644 --- a/OpenClaw/Engine/Resource/ResourceCache.cpp +++ b/OpenClaw/Engine/Resource/ResourceCache.cpp @@ -1,11 +1,27 @@ #include #include +#include #include #include "ResourceCache.h" #include "../Util/StringUtil.h" +// -------------------------------------------------------------------------- +// Function: IsValidResourcePath +// Purpose: Reject resource paths that contain ".." components or +// embedded null bytes, blocking directory traversal attacks +// via crafted archive filenames. +// -------------------------------------------------------------------------- +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; +} + // // Resource::Resource // @@ -51,6 +67,12 @@ bool ResourceRezArchive::VOpen() int32 ResourceRezArchive::VGetRawResourceSize(Resource* r) { + if (!IsValidResourcePath(r->GetName())) + { + LOG_ERROR("Blocked unsafe resource path: " + r->GetName()); + return -1; + } + RezFile* rezFile = WAP_GetRezFileFromRezArchive(_rezArchive, r->GetName().c_str()); if (rezFile == NULL) { @@ -64,6 +86,12 @@ int32 ResourceRezArchive::VGetRawResourceSize(Resource* r) // Remark: outBuffer is valid heap pointer int32 ResourceRezArchive::VGetRawResource(Resource* r, char* outBuffer) { + if (!IsValidResourcePath(r->GetName())) + { + LOG_ERROR("Blocked unsafe resource path: " + r->GetName()); + return -1; + } + assert(outBuffer != NULL); RezFile* rezFile = WAP_GetRezFileFromRezArchive(_rezArchive, r->GetName().c_str()); diff --git a/OpenClaw/Engine/Resource/ResourceCache.h b/OpenClaw/Engine/Resource/ResourceCache.h index 9a62b36ed0..7f0effe854 100644 --- a/OpenClaw/Engine/Resource/ResourceCache.h +++ b/OpenClaw/Engine/Resource/ResourceCache.h @@ -159,6 +159,10 @@ typedef std::list> ResourceHandleList; typedef std::list> ResourceLoaderList; typedef std::map> ResourceHandleMap; +// Returns false if the path contains ".." components or embedded null bytes, +// which could be used to escape the resource archive root (directory traversal). +bool IsValidResourcePath(const std::string& path); + class ResourceCache { public: diff --git a/OpenClaw/Engine/Resource/ZipFile.cpp b/OpenClaw/Engine/Resource/ZipFile.cpp index f90a891790..0b9720b1d6 100644 --- a/OpenClaw/Engine/Resource/ZipFile.cpp +++ b/OpenClaw/Engine/Resource/ZipFile.cpp @@ -9,7 +9,7 @@ // // This program is distributed in the hope that it will be useful, // but WITHOUT ANY WARRANTY; without even the implied warranty of -// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See // http://www.gnu.org/licenses/lgpl-3.0.txt for more details. // // You should have received a copy of the GNU Lesser GPL v3 @@ -190,6 +190,16 @@ bool ZipFile::Init(const std::string &resFileName) { pfh += sizeof(fh); + // Security fix: reject ZIP entries with filenames that would overflow + // a _MAX_PATH-sized buffer. fnameLen is a uint16 (max 65535) read + // directly from the archive, so a malformed ZIP could previously + // cause a stack-buffer overflow here. + if (fh.fnameLen >= _MAX_PATH) + { + success = false; + break; + } + char fileName[_MAX_PATH]; memcpy(fileName, pfh, fh.fnameLen); fileName[fh.fnameLen] = 0; @@ -273,7 +283,7 @@ std::vector ZipFile::GetAllFilesInDirectory(const std::string& dir // -------------------------------------------------------------------------- // Function: End // Purpose: Finish the object -// Parameters: +// Parameters: // -------------------------------------------------------------------------- void ZipFile::End() { @@ -292,10 +302,12 @@ std::string ZipFile::GetFilename(int i) const std::string fileName = ""; if (i >= 0 && i < m_nEntries) { - char pszDest[_MAX_PATH]; - memcpy(pszDest, m_papDir[i]->GetName(), m_papDir[i]->fnameLen); - pszDest[m_papDir[i]->fnameLen] = '\0'; - fileName = pszDest; + // Security fix: use std::string constructor with explicit length instead + // of copying into a fixed _MAX_PATH stack buffer. The old code wrote + // pszDest[fnameLen] = '\0' which was an out-of-bounds write whenever + // fnameLen >= _MAX_PATH (260), since fnameLen is a uint16 read from the + // untrusted ZIP archive and can be up to 65535. + fileName = std::string(m_papDir[i]->GetName(), m_papDir[i]->fnameLen); if (fileName.size() > 0 && fileName[0] != '/') {