Hardening: Block path traversal in package install via composer.json name#237
Hardening: Block path traversal in package install via composer.json name#237
Conversation
…tion path Agent-Logs-Url: https://github.com/wp-cli/package-command/sessions/159d5271-2365-4bfe-8f16-6fd715d1a133 Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Agent-Logs-Url: https://github.com/wp-cli/package-command/sessions/159d5271-2365-4bfe-8f16-6fd715d1a133 Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Agent-Logs-Url: https://github.com/wp-cli/package-command/sessions/d84b0dfc-3c57-40c3-82e8-fbe52598eb58 Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Code Review
This pull request introduces security measures to prevent path-traversal attacks during ZIP package installation by validating package names and verifying that destination paths remain within the designated directory. However, the implementation has a few critical issues: the use of ValidatingArrayLoader::hasPackageNamingError() will cause fatal errors on environments running Composer 1.x, and the is_child_path helper is currently ineffective against traversal because it fails to resolve '..' components and does not account for case-insensitive file systems.
…ng contents Agent-Logs-Url: https://github.com/wp-cli/package-command/sessions/7bc70ad5-73a0-45a9-b337-7be724bfffcb Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
| private static function is_child_path( $path, $parent_dir ) { | ||
| $normalized_path = rtrim( Path::normalize( $path ), '/' ) . '/'; | ||
| $normalized_parent = rtrim( Path::normalize( $parent_dir ), '/' ) . '/'; | ||
| return 0 === strpos( $normalized_path, $normalized_parent ); | ||
| } |
There was a problem hiding this comment.
The is_child_path helper has two significant limitations for a security guard:
- Lack of
..resolution:Path::normalize()in WP-CLI does not resolve..components. A path like/base/local/../will still be considered a child of/base/local/because the string starts with the parent path. This makes the check ineffective against the very traversal it aims to prevent if the input contains... - Case Sensitivity:
strpos()is case-sensitive. On Windows or macOS, this check might fail if the paths have inconsistent casing (e.g.,C:\vsc:\), leading to false positives.
Consider resolving .. components manually (since the path might not exist on disk) and using a case-insensitive comparison where appropriate.
There was a problem hiding this comment.
Fixed in ab278f6. Replaced Path::normalize() + strpos() with:
- A new
resolve_dot_segments()helper that manually walks the path segments and collapses..without touching the filesystem — so/base/local/../vendorcorrectly resolves to/base/vendor, which then fails the containment check. stripos()instead ofstrpos()on Windows (DIRECTORY_SEPARATOR === '\\') for case-insensitive path comparison.
…rison on Windows Agent-Logs-Url: https://github.com/wp-cli/package-command/sessions/c458f3cb-4cae-4b57-b796-a60a092eea37 Co-authored-by: swissspidy <841956+swissspidy@users.noreply.github.com>
A malicious ZIP package with
"name": ".."in itscomposer.jsoncould causewp package installto copy files outside the intended~/.wp-cli/packages/local/sandbox—landing payloads likevendor/autoload.phpat~/.wp-cli/packages/vendor/autoload.php, which WP-CLI loads on every invocation. The exploit fires before Composer validates the package name, so even a failed install leaves the payload on disk.Changes
src/Package_Command.phpPackage name validation (
get_package_name_and_version_from_dir_package()): validate thenameread fromcomposer.jsonagainst Composer's canonical naming regex viaValidatingArrayLoader::hasPackageNamingError()before any file operations. Names like..are rejected immediately.Path containment check (
install()): after computing the destination path, assert it stays withinlocal_dirusing a newis_child_path()helper. Violation throws anExceptioncaught by the existing cleanup handler.features/package-install.featureAdded a scenario that creates a ZIP with
"name": ".."and avendor/autoload.phppayload, then asserts the install fails with the expected error before any files are written.