fix: add buffer-length check in unzip.cpp - #429
Conversation
Automated security fix generated by OrbisAI Security
📝 WalkthroughWalkthroughThe ZIP extraction code validates entry names before processing. It rejects unsafe paths and extracts parent directories with ChangesZIP extraction safety and 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
Entry names from sync.zip are untrusted, yet unzip() appended them verbatim to the destination before mkdir/fopen. Names containing ".." components or absolute paths escaped the LiveSync directory, letting a crafted archive create or overwrite files outside it (ZipSlip). Validate each entry with is_safe_entry_name() and skip entries that are absolute or contain a ".." path component. Legitimate archives extract unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
TKLiveSync/unzip.cpp (1)
28-47: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd regression tests for the path-validation boundary.
Cover nested safe names, absolute names,
..components, empty names, and names longer thanPATH_MAX. Run the long-name case under AddressSanitizer to protect the heap-overflow fix from regression.🤖 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 28 - 47, Add regression tests targeting is_safe_entry_name for nested safe paths, absolute paths, parent-directory components, empty names, and names exceeding PATH_MAX. Verify each boundary case is accepted or rejected as intended, and execute the overlong-name test with AddressSanitizer enabled to detect heap overflows.
🤖 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.
Nitpick comments:
In `@TKLiveSync/unzip.cpp`:
- Around line 28-47: Add regression tests targeting is_safe_entry_name for
nested safe paths, absolute paths, parent-directory components, empty names, and
names exceeding PATH_MAX. Verify each boundary case is accepted or rejected as
intended, and execute the overlong-name test with AddressSanitizer enabled to
detect heap overflows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 76e5ed03-cd5b-44ba-8d2e-a51171124d75
📒 Files selected for processing (1)
TKLiveSync/unzip.cpp
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