fix: add buffer-length check in unzip.cpp - #429
Conversation
Automated security fix generated by OrbisAI Security
📝 WalkthroughWalkthroughThe ZIP extraction code removes an unused header and temporary buffer. It extracts parent directories with ChangesZIP directory handling
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you, target main branch instead of dev on this PR. |
done. |
Replace heap-allocated PATH_MAX buffer + strcpy/dirname with std::string find_last_of to avoid silent truncation of long ZIP entry names that could cause the directory path to diverge from assetFullname. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@TKLiveSync/unzip.cpp`:
- Around line 47-55: Validate each archive entry name in unzip() before
constructing directory paths or opening files: reject absolute paths and any
path component exactly equal to "..". Treat invalid names as failed entries and
skip further processing, ensuring mkdir_rec() and fopen() are never called for
them.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 441a7f98-d430-4747-bfef-2104aa141350
📒 Files selected for processing (1)
TKLiveSync/unzip.cpp
| 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()); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files relevant to unzip:"
git ls-files | rg '(^|/)unzip\.(cpp|h)$|TKLiveSync/TKLiveSync\.m|TKLiveSync/TKLiveSync\.mm$' || true
echo
echo "unzip.cpp outline:"
ast-grep outline TKLiveSync/unzip.cpp || true
echo
echo "unzip.cpp relevant lines:"
cat -n TKLiveSync/unzip.cpp | sed -n '1,120p'
echo
echo "unzip.h:"
cat -n TKLiveSync/unzip.h | sed -n '1,140p'
echo
echo "Path traversal behavior model for current construction:"
python3 - <<'PY'
from pathlib import PurePosixPath
def simulate(destination, name):
separator = name.rfind("/")
if separator != -1:
dirFullname = destination + "/" + name[:separator]
else:
dirFullname = destination
assetFullname = destination + "/" + name
return dirFullname, assetFullname, PurePosixPath(assetFullname)
destination = "/Users/me/Application Support/LiveSync"
examples = ["../outside.txt", "../../outside.txt", "/tmp/outside.txt", "normal.txt", "subdir/../escape.txt"]
for name in examples:
df, af, pp = simulate(destination, name)
print(f"name={name!r}")
print(f" constructed_asset_fullname={af!r}')
print(f" posix_absolute={pp.is_absolute()}")
print(f" canonical_resolved_without_sysfs={pp.resolve() if str(pp).startswith('/') else 'depends on cwd'}")
print(f" startsWithDestinationCanonical=False")
PYRepository: NativeScript/ios
Length of output: 3777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import PurePosixPath
def simulate(destination, name):
separator = name.rfind("/")
if separator != -1:
dirFullname = destination + "/" + name[:separator]
else:
dirFullname = destination
assetFullname = destination + "/" + name
return dirFullname, assetFullname, PurePosixPath(assetFullname)
destination = "/Users/me/Application Support/LiveSync"
examples = [
"../outside.txt",
"../../outside.txt",
"/tmp/outside.txt",
"normal.txt",
"subdir/../escape.txt",
"foo/../../escape.txt",
"foo/bar/../../../escape.txt",
]
for name in examples:
df, af, pp = simulate(destination, name)
print(f"name={name!r}")
print(f" constructed_asset_fullname={af!r}")
print(f" posix_absolute={pp.is_absolute()}")
print(f" starts_with_traversal={name.startswith(('../', '/'))}")
PY
echo
echo "Search for unzip calls and LiveSync archive handling:"
rg -n "unzip\\(|sync\.zip|LiveSync" TKLiveSyncRepository: NativeScript/ios
Length of output: 2779
🌐 Web query:
libzip zip_get_name zip files ZIP filenames unsafe absolute paths parent directory traversal
💡 Result:
The use of filenames retrieved from ZIP archives—via functions like libzip's zip_get_name—poses a significant risk of directory traversal attacks [1][2][3]. This is not a vulnerability in libzip itself, but rather a standard security risk inherent in the ZIP file format, which allows entry names to contain absolute paths (e.g., /etc/passwd) or parent directory traversal sequences (e.g.,../../../) [1][4][3]. When an application uses these filenames to construct local file paths on the filesystem, it can lead to arbitrary file creation or overwriting outside the intended destination directory [1][2][5][3]. This class of vulnerability is commonly referred to as a ZipSlip attack [3]. To mitigate this risk, developers must treat any filename obtained from a ZIP archive as untrusted input [2][3]. You must validate and sanitize these paths before using them in filesystem operations [1][3]. Essential security practices include: 1. Canonicalization: Convert the target destination path and the extracted filename into their canonical forms to resolve any traversal sequences [3]. 2. Validation: After canonicalization, verify that the resulting path still resides within the intended destination directory (e.g., by checking if the canonical path starts with the prefix of the safe destination directory) [3]. 3. Disallow Absolute Paths: Explicitly check for and reject filenames that are absolute paths or attempt to escape the designated extraction root [1][3]. libzip provides the raw, unmodified names as stored in the archive [6][7]. It is the responsibility of the application developer to perform the necessary path normalization and security checks before opening or creating files based on these names [7][3].
Citations:
- 1: Directory traversal vulnerability from libzip nim-lang/zip#54
- 2: Potential path traversal when using filename from Zip archive find-sec-bugs/find-sec-bugs#514
- 3: https://developer.android.com/privacy-and-security/risks/zip-path-traversal
- 4: https://fuzze.rs/fuzz/libzip
- 5: https://github.com/kuba--/zip/issues/123
- 6: https://libzip.org/documentation/zip_get_name.html
- 7: https://man.archlinux.org/man/libzip.3.en.txt
Path Traversal (CWE-22): Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal')
Reachability: External
Treat archive entry names as untrusted before writing files.
sb.name is appended to both the directory path and assetFullname unchanged. Entries with .., /../, or absolute paths such as /tmp/outside.txt make macOS resolve paths back toward filesystem root for later segments, so unzip() can create files or overwrite contents outside the LiveSync destination. Reject absolute names and any .. path component before calling mkdir_rec() or fopen(), and fail the entry on invalid names.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TKLiveSync/unzip.cpp` around lines 47 - 55, Validate each archive entry name
in unzip() before constructing directory paths or opening files: reject absolute
paths and any path component exactly equal to "..". Treat invalid names as
failed entries and skip further processing, ensuring mkdir_rec() and fopen() are
never called for them.
2 participants
Summary
Fix critical severity security issue in
TKLiveSync/unzip.cpp.Vulnerability
V-001TKLiveSync/unzip.cpp:49Description: A PATH_MAX-sized heap buffer (pathcopy) receives ZIP entry names via strcpy() without bounds checking. ZIP specification allows entry names up to 65535 bytes, far exceeding typical PATH_MAX values (4096 or 1024). This creates a classic buffer overflow where crafted long filenames overflow the heap buffer.
Evidence
Exploitation scenario: Attacker creates a ZIP archive with an entry name longer than PATH_MAX bytes.
Scanner confirmation: multi_agent_ai rule
V-001flagged this pattern.Production code: This file is in the production codebase, not test-only code.
Threat Model Context
This is a Node.js library - vulnerabilities affect downstream consumers who use this package.
Changes
TKLiveSync/unzip.cppBehavior Preservation
The change is scoped to 1 file on the vulnerable path; it only tightens handling of untrusted input and leaves valid inputs unaffected.
Automated security fix by OrbisAI Security
Summary by CodeRabbit
Bug Fixes
Refactor