From ca5e2f8d505fd3bfac6f7c85b82f2740becbc0aa Mon Sep 17 00:00:00 2001 From: Jordi Boggiano Date: Tue, 5 Oct 2021 09:28:42 +0200 Subject: [PATCH] Fix escaping issues on Windows which could lead to command injection, fixes GHSA-frqg-7g38-6gcf --- CHANGELOG.md | 5 ++ src/Composer/Command/HomeCommand.php | 2 +- src/Composer/Util/ProcessExecutor.php | 47 ++++++------------- .../Test/Downloader/GitDownloaderTest.php | 2 +- tests/Composer/Test/Util/SvnTest.php | 2 +- 5 files changed, 22 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc6a92fc8..b19cfe345 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +### [1.10.23] 2021-10-05 + + * Security: Fixed command injection vulnerability on Windows (GHSA-frqg-7g38-6gcf / CVE-2021-41116) + ### [1.10.22] 2021-04-27 * Security: Fixed command injection vulnerability in HgDriver/HgDownloader and hardened other VCS drivers and downloaders (GHSA-h5h8-pc6h-jvvx / CVE-2021-29472) @@ -938,6 +942,7 @@ * Initial release +[1.10.23]: https://github.com/composer/composer/compare/1.10.22...1.10.23 [1.10.22]: https://github.com/composer/composer/compare/1.10.21...1.10.22 [1.10.21]: https://github.com/composer/composer/compare/1.10.20...1.10.21 [1.10.20]: https://github.com/composer/composer/compare/1.10.19...1.10.20 diff --git a/src/Composer/Command/HomeCommand.php b/src/Composer/Command/HomeCommand.php index b7d907066..c22128d46 100644 --- a/src/Composer/Command/HomeCommand.php +++ b/src/Composer/Command/HomeCommand.php @@ -129,7 +129,7 @@ EOT $process = new ProcessExecutor($this->getIO()); if (Platform::isWindows()) { - return $process->execute('start "web" explorer "' . $url . '"', $output); + return $process->execute('start "web" explorer ' . $url, $output); } $linux = $process->execute('which xdg-open', $output); diff --git a/src/Composer/Util/ProcessExecutor.php b/src/Composer/Util/ProcessExecutor.php index 658c2a136..7cb83e39b 100644 --- a/src/Composer/Util/ProcessExecutor.php +++ b/src/Composer/Util/ProcessExecutor.php @@ -155,7 +155,7 @@ class ProcessExecutor } /** - * Copy of ProcessUtils::escapeArgument() that is deprecated in Symfony 3.3 and removed in Symfony 4. + * Copy of Symfony's Process::escapeArgument() which is private * * @param string $argument * @@ -163,40 +163,21 @@ class ProcessExecutor */ private static function escapeArgument($argument) { - //Fix for PHP bug #43784 escapeshellarg removes % from given string - //Fix for PHP bug #49446 escapeshellarg doesn't work on Windows - //@see https://bugs.php.net/bug.php?id=43784 - //@see https://bugs.php.net/bug.php?id=49446 - if ('\\' === DIRECTORY_SEPARATOR) { - if ((string) $argument === '') { - return escapeshellarg($argument); - } - - $escapedArgument = ''; - $quote = false; - foreach (preg_split('/(")/', $argument, -1, PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE) as $part) { - if ('"' === $part) { - $escapedArgument .= '\\"'; - } elseif (self::isSurroundedBy($part, '%')) { - // Avoid environment variable expansion - $escapedArgument .= '^%"'.substr($part, 1, -1).'"^%'; - } else { - // escape trailing backslash - if ('\\' === substr($part, -1)) { - $part .= '\\'; - } - $quote = true; - $escapedArgument .= $part; - } - } - if ($quote) { - $escapedArgument = '"'.$escapedArgument.'"'; - } - - return $escapedArgument; + if ('' === $argument || null === $argument) { + return '""'; } + if ('\\' !== \DIRECTORY_SEPARATOR) { + return "'".str_replace("'", "'\\''", $argument)."'"; + } + if (false !== strpos($argument, "\0")) { + $argument = str_replace("\0", '?', $argument); + } + if (!preg_match('/[\/()%!^"<>&|\s]/', $argument)) { + return $argument; + } + $argument = preg_replace('/(\\\\+)$/', '$1$1', $argument); - return "'".str_replace("'", "'\\''", $argument)."'"; + return '"'.str_replace(array('"', '^', '%', '!', "\n"), array('""', '"^^"', '"^%"', '"^!"', '!LF!'), $argument).'"'; } private static function isSurroundedBy($arg, $char) diff --git a/tests/Composer/Test/Downloader/GitDownloaderTest.php b/tests/Composer/Test/Downloader/GitDownloaderTest.php index cd446a73b..3bc9686c5 100644 --- a/tests/Composer/Test/Downloader/GitDownloaderTest.php +++ b/tests/Composer/Test/Downloader/GitDownloaderTest.php @@ -524,7 +524,7 @@ composer https://github.com/old/url (push) public function testUpdateDoesntThrowsRuntimeExceptionIfGitCommandFailsAtFirstButIsAbleToRecover() { - $expectedFirstGitUpdateCommand = $this->winCompat("(git remote set-url composer -- '' && git rev-parse --quiet --verify 'ref^{commit}' || (git fetch composer && git fetch --tags composer)) && git remote set-url composer -- ''"); + $expectedFirstGitUpdateCommand = $this->winCompat("(git remote set-url composer -- \"\" && git rev-parse --quiet --verify 'ref^{commit}' || (git fetch composer && git fetch --tags composer)) && git remote set-url composer -- \"\""); $expectedSecondGitUpdateCommand = $this->winCompat("(git remote set-url composer -- 'https://github.com/composer/composer' && git rev-parse --quiet --verify 'ref^{commit}' || (git fetch composer && git fetch --tags composer)) && git remote set-url composer -- 'https://github.com/composer/composer'"); $packageMock = $this->getMockBuilder('Composer\Package\PackageInterface')->getMock(); diff --git a/tests/Composer/Test/Util/SvnTest.php b/tests/Composer/Test/Util/SvnTest.php index ada3818ae..8e1db31ec 100644 --- a/tests/Composer/Test/Util/SvnTest.php +++ b/tests/Composer/Test/Util/SvnTest.php @@ -47,7 +47,7 @@ class SvnTest extends TestCase return array( array('http://till:test@svn.example.org/', $this->getCmd(" --username 'till' --password 'test' ")), array('http://svn.apache.org/', ''), - array('svn://johndoe@example.org', $this->getCmd(" --username 'johndoe' --password '' ")), + array('svn://johndoe@example.org', $this->getCmd(" --username 'johndoe' --password \"\" ")), ); }