CVE-2022-24828: Improper Neutralization of Argument Delimiters in a Command ('Argument Injection') (CWE-88)
value: Composer is a dependency manager for the PHP programming language. Integrators using Composer code to call `VcsDriver::getFileContent` can have a code injection vulnerability if the user can control the `$file` or `$identifier` argument. This leads to a vulnerability on packagist.org for example where the composer.json's `readme` field can be used as a vector for injecting parameters into hg/Mercurial via the `$file` argument, or git via the `$identifier` argument if you allow arbitrary data there (Packagist does not, but maybe other integrators do). Composer itself should not be affected by the vulnerability as it does not call `getFileContent` with arbitrary data into `$file`/`$identifier`. To the best of our knowledge this was not abused, and the vulnerability has been patched on packagist.org and Private Packagist within a day of the vulnerability report.
CWE description: The product constructs a string for a command to be executed by a separate component
in another control sphere, but it does not properly delimit the
intended arguments, options, or switches within that command string.
CVSS v3 base score: 8.8
Published: 2022-04-13T21:15Z
Repository: https://github.com/composer/composer
Language: PHP
Files changed: src/Composer/Repository/Vcs/GitDriver.php, src/Composer/Repository/Vcs/HgDriver.php, tests/Composer/Test/Repository/Vcs/GitDriverTest.php, tests/Composer/Test/Repository/Vcs/HgDriverTest.php
Commit date: 2022-04-13 15:54:58 +0200
Commit message: Merge pull request from GHSA-x7cr-6qr6-2hh6

* GitDriver: filter branch names starting with a - character

* GitDriver: getFileContent prevent identifiers starting with a -

* HgDriver: prevent invalid identifiers and prevent file from running commands

* HgDriver: filter branches starting with a - character
Diff: diff --git a/src/Composer/Repository/Vcs/GitDriver.php b/src/Composer/Repository/Vcs/GitDriver.php
index b91508acd..039f9052f 100644
--- a/src/Composer/Repository/Vcs/GitDriver.php
+++ b/src/Composer/Repository/Vcs/GitDriver.php
@@ -136,10 +136,14 @@ public function getDist($identifier)
     /**
      * @inheritDoc
      */
     public function getFileContent($file, $identifier)
     {
+        if (isset($identifier[0]) && $identifier[0] === '-') {
+            throw new \RuntimeException('Invalid git identifier detected. Identifier must not start with a -, given: ' . $identifier);
+        }
+
         $resource = sprintf('%s:%s', ProcessExecutor::escape($identifier), ProcessExecutor::escape($file));
         $this->process->execute(sprintf('git show %s', $resource), $content, $this->repoDir);
 
         if (!trim($content)) {
             return null;
@@ -189,11 +193,11 @@ public function getBranches()
             $branches = array();
 
             $this->process->execute('git branch --no-color --no-abbrev -v', $output, $this->repoDir);
             foreach ($this->process->splitLines($output) as $branch) {
                 if ($branch && !Preg::isMatch('{^ *[^/]+/HEAD }', $branch)) {
-                    if (Preg::isMatch('{^(?:\* )? *(\S+) *([a-f0-9]+)(?: .*)?$}', $branch, $match)) {
+                    if (Preg::isMatch('{^(?:\* )? *(\S+) *([a-f0-9]+)(?: .*)?$}', $branch, $match) && $match[1][0] !== '-') {
                         $branches[$match[1]] = $match[2];
                     }
                 }
             }
 
diff --git a/src/Composer/Repository/Vcs/HgDriver.php b/src/Composer/Repository/Vcs/HgDriver.php
index 73bc426e5..f9f8064ba 100644
--- a/src/Composer/Repository/Vcs/HgDriver.php
+++ b/src/Composer/Repository/Vcs/HgDriver.php
@@ -124,11 +124,15 @@ public function getDist($identifier)
     /**
      * @inheritDoc
      */
     public function getFileContent($file, $identifier)
     {
-        $resource = sprintf('hg cat -r %s %s', ProcessExecutor::escape($identifier), ProcessExecutor::escape($file));
+        if (isset($identifier[0]) && $identifier[0] === '-') {
+            throw new \RuntimeException('Invalid hg identifier detected. Identifier must not start with a -, given: ' . $identifier);
+        }
+
+        $resource = sprintf('hg cat -r %s -- %s', ProcessExecutor::escape($identifier), ProcessExecutor::escape($file));
         $this->process->execute($resource, $content, $this->repoDir);
 
         if (!trim($content)) {
             return null;
         }
@@ -184,18 +188,18 @@ public function getBranches()
             $branches = array();
             $bookmarks = array();
 
             $this->process->execute('hg branches', $output, $this->repoDir);
             foreach ($this->process->splitLines($output) as $branch) {
-                if ($branch && Preg::isMatch('(^([^\s]+)\s+\d+:([a-f0-9]+))', $branch, $match)) {
+                if ($branch && Preg::isMatch('(^([^\s]+)\s+\d+:([a-f0-9]+))', $branch, $match) && $match[1][0] !== '-') {
                     $branches[$match[1]] = $match[2];
                 }
             }
 
             $this->process->execute('hg bookmarks', $output, $this->repoDir);
             foreach ($this->process->splitLines($output) as $branch) {
-                if ($branch && Preg::isMatch('(^(?:[\s*]*)([^\s]+)\s+\d+:(.*)$)', $branch, $match)) {
+                if ($branch && Preg::isMatch('(^(?:[\s*]*)([^\s]+)\s+\d+:(.*)$)', $branch, $match) && $match[1][0] !== '-') {
                     $bookmarks[$match[1]] = $match[2];
                 }
             }
 
             // Branches will have preference over bookmarks
diff --git a/tests/Composer/Test/Repository/Vcs/GitDriverTest.php b/tests/Composer/Test/Repository/Vcs/GitDriverTest.php
new file mode 100644
index 000000000..c8cfdab0a
--- /dev/null
+++ b/tests/Composer/Test/Repository/Vcs/GitDriverTest.php
@@ -0,0 +1,81 @@
+<?php
+
+namespace Composer\Test\Repository\Vcs;
+
+use Composer\Config;
+use Composer\Repository\Vcs\GitDriver;
+use Composer\Test\Mock\ProcessExecutorMock;
+use Composer\Test\TestCase;
+
+class GitDriverTest extends TestCase
+{
+    /** @var Config */
+    private $config;
+    /** @var string */
+    private $home;
+
+    public function setUp()
+    {
+        $this->home = self::getUniqueTmpDirectory();
+        $this->config = new Config();
+        $this->config->merge(array(
+            'config' => array(
+                'home' => $this->home,
+            ),
+        ));
+    }
+
+    public function testGetBranchesFilterInvalidBranchNames()
+    {
+        $process = new ProcessExecutorMock;
+        $io = $this->getMockBuilder('Composer\IO\IOInterface')->getMock();
+
+        $driver = new GitDriver(array('url' => 'https://example.org/acme.git'), $io, $this->config, $this->getMockBuilder('Composer\Util\HttpDownloader')->disableOriginalConstructor()->getMock(), $process);
+        $this->setRepoDir($driver, $this->home);
+
+        // Branches starting with a - character are not valid git branches names
+        // Still assert that they get filtered to prevent issues later on
+        $stdout = <<<GIT
+* main 089681446ba44d6d9004350192486f2ceb4eaa06 commit
+  2.2  12681446ba44d6d9004350192486f2ceb4eaa06 commit
+  -h   089681446ba44d6d9004350192486f2ceb4eaa06 commit
+GIT;
+
+        $process
+            ->expects(array(array(
+                'cmd' => 'git branch --no-color --no-abbrev -v',
+                'stdout' => $stdout,
+            )));
+
+        $branches = $driver->getBranches();
+        $this->assertSame(array(
+            'main' => '089681446ba44d6d9004350192486f2ceb4eaa06',
+            '2.2' => '12681446ba44d6d9004350192486f2ceb4eaa06',
+        ), $branches);
+    }
+
+    public function testFileGetContentInvalidIdentifier()
+    {
+        $this->expectException('\RuntimeException');
+
+        $process = new ProcessExecutorMock;
+        $io = $this->getMockBuilder('Composer\IO\IOInterface')->getMock();
+        $driver = new GitDriver(array('url' => 'https://example.org/acme.git'), $io, $this->config, $this->getMockBuilder('Composer\Util\HttpDownloader')->disableOriginalConstructor()->getMock(), $process);
+
+        $this->assertNull($driver->getFileContent('file.txt', 'h'));
+
+        $driver->getFileContent('file.txt', '-h');
+    }
+
+    /**
+     * @param GitDriver $driver
+     * @param string $path
+     */
+    private function setRepoDir($driver, $path)
+    {
+        $reflectionClass = new \ReflectionClass($driver);
+        $reflectionProperty = $reflectionClass->getProperty('repoDir');
+        $reflectionProperty->setAccessible(true);
+        $reflectionProperty->setValue($driver, $path);
+    }
+}
diff --git a/tests/Composer/Test/Repository/Vcs/HgDriverTest.php b/tests/Composer/Test/Repository/Vcs/HgDriverTest.php
index 2fcd27bdf..356a28c48 100644
--- a/tests/Composer/Test/Repository/Vcs/HgDriverTest.php
+++ b/tests/Composer/Test/Repository/Vcs/HgDriverTest.php
@@ -11,10 +11,11 @@
  */
 
 namespace Composer\Test\Repository\Vcs;
 
 use Composer\Repository\Vcs\HgDriver;
+use Composer\Test\Mock\ProcessExecutorMock;
 use Composer\Test\TestCase;
 use Composer\Util\Filesystem;
 use Composer\Config;
 
 class HgDriverTest extends TestCase
@@ -64,6 +65,51 @@ public function supportsDataProvider()
             array('ssh://user@bitbucket.org/user/repo'),
             array('https://bitbucket.org/user/repo'),
             array('https://user@bitbucket.org/user/repo'),
         );
     }
+
+    public function testGetBranchesFilterInvalidBranchNames()
+    {
+        $process = new ProcessExecutorMock;
+
+        $driver = new HgDriver(array('url' => 'https://example.org/acme.git'), $this->io, $this->config, $this->getMockBuilder('Composer\Util\HttpDownloader')->disableOriginalConstructor()->getMock(), $process);
+
+        $stdout = <<<HG_BRANCHES
+default 1:dbf6c8acb640
+--help  1:dbf6c8acb640
+HG_BRANCHES;
+
+        $stdout1 = <<<HG_BOOKMARKS
+help    1:dbf6c8acb641
+--help  1:dbf6c8acb641
+
+HG_BOOKMARKS;
+
+        $process
+            ->expects(array(array(
+                'cmd' => 'hg branches',
+                'stdout' => $stdout,
+            ), array(
+                'cmd' => 'hg bookmarks',
+                'stdout' => $stdout1,
+            )));
+
+        $branches = $driver->getBranches();
+        $this->assertSame(array(
+            'help' => 'dbf6c8acb641',
+            'default' => 'dbf6c8acb640',
+        ), $branches);
+    }
+
+    public function testFileGetContentInvalidIdentifier()
+    {
+        $this->expectException('\RuntimeException');
+
+        $process = new ProcessExecutorMock;
+        $driver = new HgDriver(array('url' => 'https://example.org/acme.git'), $this->io, $this->config, $this->getMockBuilder('Composer\Util\HttpDownloader')->disableOriginalConstructor()->getMock(), $process);
+
+        $this->assertNull($driver->getFileContent('file.txt', 'h'));
+
+        $driver->getFileContent('file.txt', '-h');
+    }
 }

Vulnerable code:                     if (Preg::isMatch('{^(?:\* )? *(\S+) *([a-f0-9]+)(?: .*)?$}', $branch, $match)) {
        $resource = sprintf('hg cat -r %s %s', ProcessExecutor::escape($identifier), ProcessExecutor::escape($file));
                if ($branch && Preg::isMatch('(^([^\s]+)\s+\d+:([a-f0-9]+))', $branch, $match)) {
                if ($branch && Preg::isMatch('(^(?:[\s*]*)([^\s]+)\s+\d+:(.*)$)', $branch, $match)) {
Fixed code:         if (isset($identifier[0]) && $identifier[0] === '-') {
            throw new \RuntimeException('Invalid git identifier detected. Identifier must not start with a -, given: ' . $identifier);
        }

                    if (Preg::isMatch('{^(?:\* )? *(\S+) *([a-f0-9]+)(?: .*)?$}', $branch, $match) && $match[1][0] !== '-') {
        if (isset($identifier[0]) && $identifier[0] === '-') {
            throw new \RuntimeException('Invalid hg identifier detected. Identifier must not start with a -, given: ' . $identifier);
        }

        $resource = sprintf('hg cat -r %s -- %s', ProcessExecutor::escape($identifier), ProcessExecutor::escape($file));
                if ($branch && Preg::isMatch('(^([^\s]+)\s+\d+:([a-f0-9]+))', $branch, $match) && $match[1][0] !== '-') {
                if ($branch && Preg::isMatch('(^(?:[\s*]*)([^\s]+)\s+\d+:(.*)$)', $branch, $match) && $match[1][0] !== '-') {
<?php

namespace Composer\Test\Repository\Vcs;

use Composer\Config;
use Composer\Repository\Vcs\GitDriver;
use Composer\Test\Mock\ProcessExecutorMock;
use Composer\Test\TestCase;

class GitDriverTest extends TestCase
{
    /** @var Config */
    private $config;
    /** @var string */
    private $home;

    public function setUp()
    {
        $this->home = self::getUniqueTmpDirectory();
        $this->config = new Config();
        $this->config->merge(array(
            'config' => array(
                'home' => $this->home,
            ),
        ));
    }

    public function testGetBranchesFilterInvalidBranchNames()
    {
        $process = new ProcessExecutorMock;
        $io = $this->getMockBuilder('Composer\IO\IOInterface')->getMock();

        $driver = new GitDriver(array('url' => 'https://example.org/acme.git'), $io, $this->config, $this->getMockBuilder('Composer\Util\HttpDownloader')->disableOriginalConstructor()->getMock(), $process);
        $this->setRepoDir($driver, $this->home);

        // Branches starting with a - character are not valid git branches names
        // Still assert that they get filtered to prevent issues later on
        $stdout = <<<GIT
* main 089681446ba44d6d9004350192486f2ceb4eaa06 commit
  2.2  12681446ba44d6d9004350192486f2ceb4eaa06 commit
  -h   089681446ba44d6d9004350192486f2ceb4eaa06 commit
GIT;

        $process
            ->expects(array(array(
                'cmd' => 'git branch --no-color --no-abbrev -v',
                'stdout' => $stdout,
            )));

        $branches = $driver->getBranches();
        $this->assertSame(array(
            'main' => '089681446ba44d6d9004350192486f2ceb4eaa06',
            '2.2' => '12681446ba44d6d9004350192486f2ceb4eaa06',
        ), $branches);
    }

    public function testFileGetContentInvalidIdentifier()
    {
        $this->expectException('\RuntimeException');

        $process = new ProcessExecutorMock;
        $io = $this->getMockBuilder('Composer\IO\IOInterface')->getMock();
        $driver = new GitDriver(array('url' => 'https://example.org/acme.git'), $io, $this->config, $this->getMockBuilder('Composer\Util\HttpDownloader')->disableOriginalConstructor()->getMock(), $process);

        $this->assertNull($driver->getFileContent('file.txt', 'h'));

        $driver->getFileContent('file.txt', '-h');
    }

    /**
     * @param GitDriver $driver
     * @param string $path
     */
    private function setRepoDir($driver, $path)
    {
        $reflectionClass = new \ReflectionClass($driver);
        $reflectionProperty = $reflectionClass->getProperty('repoDir');
        $reflectionProperty->setAccessible(true);
        $reflectionProperty->setValue($driver, $path);
    }
}
use Composer\Test\Mock\ProcessExecutorMock;

    public function testGetBranchesFilterInvalidBranchNames()
    {
        $process = new ProcessExecutorMock;

        $driver = new HgDriver(array('url' => 'https://example.org/acme.git'), $this->io, $this->config, $this->getMockBuilder('Composer\Util\HttpDownloader')->disableOriginalConstructor()->getMock(), $process);

        $stdout = <<<HG_BRANCHES
default 1:dbf6c8acb640
--help  1:dbf6c8acb640
HG_BRANCHES;

        $stdout1 = <<<HG_BOOKMARKS
help    1:dbf6c8acb641
--help  1:dbf6c8acb641

HG_BOOKMARKS;

        $process
            ->expects(array(array(
                'cmd' => 'hg branches',
                'stdout' => $stdout,
            ), array(
                'cmd' => 'hg bookmarks',
                'stdout' => $stdout1,
            )));

        $branches = $driver->getBranches();
        $this->assertSame(array(
            'help' => 'dbf6c8acb641',
            'default' => 'dbf6c8acb640',
        ), $branches);
    }

    public function testFileGetContentInvalidIdentifier()
    {
        $this->expectException('\RuntimeException');

        $process = new ProcessExecutorMock;
        $driver = new HgDriver(array('url' => 'https://example.org/acme.git'), $this->io, $this->config, $this->getMockBuilder('Composer\Util\HttpDownloader')->disableOriginalConstructor()->getMock(), $process);

        $this->assertNull($driver->getFileContent('file.txt', 'h'));

        $driver->getFileContent('file.txt', '-h');
    }