Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions features/package-install.feature
Original file line number Diff line number Diff line change
Expand Up @@ -1417,6 +1417,29 @@ Feature: Install WP-CLI packages
"""
And STDOUT should be empty

Scenario: Reject a ZIP package whose composer.json name contains path-traversal components
Given an empty directory
And a create-malicious-package-zip.php file:
"""
<?php
$zip = new ZipArchive();
$zip->open( 'traversal.zip', ZipArchive::CREATE );
$zip->addFromString( 'traversal/composer.json', '{"name":"..","description":"path traversal test","type":"wp-cli-package"}' );
$zip->addFromString( 'traversal/vendor/autoload.php', '<?php /* COMPROMISED */ ?>' );
$zip->close();
"""
When I run `php create-malicious-package-zip.php`

And I try `wp package install traversal.zip`
Then the return code should be 1
And STDERR should contain:
"""
Error: Invalid package name '..':
"""
And STDOUT should be empty
Comment thread
swissspidy marked this conversation as resolved.
And the {PACKAGE_PATH}vendor/autoload.php file should not exist
And the {PACKAGE_PATH}local directory should not exist

@github-api
Scenario: Install package with --no-interaction fails fast on Git authentication errors
Given an empty directory
Expand Down
51 changes: 50 additions & 1 deletion src/Package_Command.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Composer\Installer;
use Composer\Json\JsonFile;
use Composer\Package\BasePackage;
use Composer\Package\Loader\ValidatingArrayLoader;
use Composer\Package\PackageInterface;
use Composer\Package\Version\VersionSelector;
use Composer\Repository;
Expand Down Expand Up @@ -302,6 +303,10 @@ public function install( $args, $assoc_args ) {
// Move to a location based on the package name
$local_dir = rtrim( WP_CLI::get_runner()->get_packages_dir_path(), '/' ) . '/local/';
$actual_dir_package = $local_dir . str_replace( '/', '-', $package_name );
// Guard against path traversal: ensure destination stays within local_dir.
if ( ! self::is_child_path( $actual_dir_package, $local_dir ) ) {
throw new Exception( 'Invalid package: resolved destination path escapes the packages directory.' );
}
Extractor::copy_overwrite_files( $dir_package, $actual_dir_package );
Extractor::rmdir( $dir_package );
// Behold, the extracted package
Expand Down Expand Up @@ -1182,13 +1187,57 @@ private static function get_package_name_and_version_from_dir_package( $dir_pack
WP_CLI::error( sprintf( "Invalid package: no name in composer.json file '%s'.", $composer_file ) );
}
$package_name = $composer_data['name'];
$version = self::DEFAULT_DEV_BRANCH_CONSTRAINTS;
$naming_error = ValidatingArrayLoader::hasPackageNamingError( $package_name );
if ( null !== $naming_error ) {
WP_CLI::error( sprintf( "Invalid package name '%s': %s", $package_name, $naming_error ) );
}
Comment thread
swissspidy marked this conversation as resolved.
$version = self::DEFAULT_DEV_BRANCH_CONSTRAINTS;
if ( ! empty( $composer_data['version'] ) ) {
$version = $composer_data['version'];
}
return [ $package_name, $version ];
}

/**
* Checks whether a path is inside (or equal to) a given parent directory.
*
* Resolves '.' and '..' segments without touching the filesystem so it
* works even when the paths do not exist yet. Uses a case-insensitive
* comparison on Windows where the filesystem is case-insensitive.
*
* @param string $path Path to test.
* @param string $parent_dir Parent directory to test against.
* @return bool True when $path is inside $parent_dir.
*/
private static function is_child_path( $path, $parent_dir ) {
$normalized_path = self::resolve_dot_segments( rtrim( str_replace( '\\', '/', $path ), '/' ) ) . '/';
$normalized_parent = self::resolve_dot_segments( rtrim( str_replace( '\\', '/', $parent_dir ), '/' ) ) . '/';
if ( DIRECTORY_SEPARATOR === '\\' ) {
return 0 === stripos( $normalized_path, $normalized_parent );
}
return 0 === strpos( $normalized_path, $normalized_parent );
}
Comment thread
swissspidy marked this conversation as resolved.
Comment on lines +1212 to +1219
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot

The is_child_path helper has two significant limitations for a security guard:

  1. 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 ...
  2. Case Sensitivity: strpos() is case-sensitive. On Windows or macOS, this check might fail if the paths have inconsistent casing (e.g., C:\ vs c:\), leading to false positives.

Consider resolving .. components manually (since the path might not exist on disk) and using a case-insensitive comparison where appropriate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ab278f6. Replaced Path::normalize() + strpos() with:

  1. A new resolve_dot_segments() helper that manually walks the path segments and collapses .. without touching the filesystem — so /base/local/../vendor correctly resolves to /base/vendor, which then fails the containment check.
  2. stripos() instead of strpos() on Windows (DIRECTORY_SEPARATOR === '\\') for case-insensitive path comparison.


/**
* Resolves '.' and '..' segments in a path without touching the filesystem.
*
* @param string $path Forward-slash path to resolve.
* @return string Resolved path.
*/
private static function resolve_dot_segments( $path ) {
$is_absolute = isset( $path[0] ) && '/' === $path[0];
$result = [];
foreach ( explode( '/', $path ) as $part ) {
if ( '..' === $part ) {
array_pop( $result );
} elseif ( '.' !== $part && '' !== $part ) {
$result[] = $part;
}
}
$resolved = implode( '/', $result );
return $is_absolute ? '/' . $resolved : $resolved;
}

/**
* Gets the WP-CLI packages composer.json object.
*/
Expand Down