From c2ef7dbdb38a0a477394975097655c00adec97c4 Mon Sep 17 00:00:00 2001
From: Kirill Efimov <kirill@snyk.io>
Date: Thu, 19 Dec 2019 13:04:40 +0200
Subject: [PATCH] Update addArg() to improve escaping (fix #44)

---
 src/Command.php               | 19 +++++++++++++------
 tests/BlockingCommandTest.php |  2 +-
 tests/CommandTest.php         | 25 ++++++++++++++++++++-----
 3 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/src/Command.php b/src/Command.php
index 7b78fbb..7fe4380 100644
--- a/src/Command.php
+++ b/src/Command.php
@@ -272,7 +272,7 @@ public function getArgs()
      * @param string|array|null $value the optional argument value which will
      * get escaped if $escapeArgs is true.  An array can be passed to add more
      * than one value for a key, e.g. `addArg('--exclude',
-     * array('val1','val2'))` which will create the option `--exclude 'val1'
+     * array('val1','val2'))` which will create the option `'--exclude' 'val1'
      * 'val2'`.
      * @param bool|null $escape if set, this overrides the $escapeArgs setting
      * and enforces escaping/no escaping
@@ -288,18 +288,25 @@ public function addArg($key, $value = null, $escape = null)
             setlocale(LC_CTYPE, $this->locale);
         }
         if ($value === null) {
-            // Only escape single arguments if explicitely requested
-            $this->_args[] = $escape ? escapeshellarg($key) : $key;
+            $this->_args[] = $doEscape ? escapeshellarg($key) : $key;
         } else {
-            $separator = substr($key, -1)==='=' ? '' : ' ';
+            if (substr($key, -1) === '=') {
+                $separator = '=';
+                $argKey = substr($key, 0, -1);
+            } else {
+                $separator = ' ';
+                $argKey = $key;
+            }
+            $argKey = $doEscape ? escapeshellarg($argKey) : $argKey;
+
             if (is_array($value)) {
                 $params = array();
                 foreach ($value as $v) {
                     $params[] = $doEscape ? escapeshellarg($v) : $v;
                 }
-                $this->_args[] = $key . $separator.implode(' ',$params);
+                $this->_args[] = $argKey . $separator . implode(' ', $params);
             } else {
-                $this->_args[] = $key . $separator .
+                $this->_args[] = $argKey . $separator .
                     ($doEscape ? escapeshellarg($value) : $value);
             }
         }
diff --git a/tests/BlockingCommandTest.php b/tests/BlockingCommandTest.php
index 25ac7a2..838c98c 100644
--- a/tests/BlockingCommandTest.php
+++ b/tests/BlockingCommandTest.php
@@ -62,7 +62,7 @@ public function testCanRunCommandWithArguments()
         $command->nonBlockingMode = false;
         $command->addArg('-l');
         $command->addArg('-n');
-        $this->assertEquals("ls -l -n", $command->getExecCommand());
+        $this->assertEquals("ls '-l' '-n'", $command->getExecCommand());
         $this->assertFalse($command->getExecuted());
         $this->assertTrue($command->execute());
         $this->assertTrue($command->getExecuted());
diff --git a/tests/CommandTest.php b/tests/CommandTest.php
index 2797fcb..281e932 100644
--- a/tests/CommandTest.php
+++ b/tests/CommandTest.php
@@ -81,8 +81,8 @@ public function testCanAddArguments()
         $command->addArg('-b=', array('v4','v5','v6'));
         $command->addArg('-c', '');
         $command->addArg('some name', null, true);
-        $this->assertEquals("--arg1=x --a --a '中文字äüp' --a 'v'\''1' 'v2' 'v3' -b=v -b='v4' 'v5' 'v6' -c '' 'some name'", $command->getArgs());
-        $this->assertEquals("test --arg1=x --a --a '中文字äüp' --a 'v'\''1' 'v2' 'v3' -b=v -b='v4' 'v5' 'v6' -c '' 'some name'", $command->getExecCommand());
+        $this->assertEquals("--arg1=x '--a' '--a' '中文字äüp' '--a' 'v'\''1' 'v2' 'v3' -b=v '-b'='v4' 'v5' 'v6' '-c' '' 'some name'", $command->getArgs());
+        $this->assertEquals("test --arg1=x '--a' '--a' '中文字äüp' '--a' 'v'\''1' 'v2' 'v3' -b=v '-b'='v4' 'v5' 'v6' '-c' '' 'some name'", $command->getExecCommand());
     }
     public function testCanResetArguments()
     {
@@ -102,14 +102,29 @@ public function testCanDisableEscaping()
         $command->addArg('-b=','v', true);
         $command->addArg('-b=', array('v4','v5','v6'));
         $command->addArg('some name', null, true);
-        $this->assertEquals("--a --a v --a v1 v2 v3 -b='v' -b=v4 v5 v6 'some name'", $command->getArgs());
+        $this->assertEquals("--a --a v --a v1 v2 v3 '-b'='v' -b=v4 v5 v6 'some name'", $command->getArgs());
+    }
+    public function testCanPreventCommandInjection()
+    {
+        $command = new Command(array(
+            'command' => 'curl',
+        ));
+        $command->addArg('http://example.com --wrong-argument || echo "RCE 1"');
+        $this->assertEquals("'http://example.com --wrong-argument || echo \"RCE 1\"'", $command->getArgs());
+
+        $command = new Command(array(
+            'command' => 'curl',
+        ));
+        $command->addArg('http://example.com');
+        $command->addArg('--header foo --wrong-argument || echo "RCE 2" ||', 'bar');
+        $this->assertEquals("'http://example.com' '--header foo --wrong-argument || echo \"RCE 2\" ||' 'bar'", $command->getArgs());
     }
     public function testCanRunCommandWithArguments()
     {
         $command = new Command('ls');
         $command->addArg('-l');
         $command->addArg('-n');
-        $this->assertEquals("ls -l -n", $command->getExecCommand());
+        $this->assertEquals("ls '-l' '-n'", $command->getExecCommand());
         $this->assertFalse($command->getExecuted());
         $this->assertTrue($command->execute());
         $this->assertTrue($command->getExecuted());
@@ -163,7 +178,7 @@ public function testCanCastToString()
         $command = new Command('ls');
         $command->addArg('-l');
         $command->addArg('-n');
-        $this->assertEquals("ls -l -n", (string)$command);
+        $this->assertEquals("ls '-l' '-n'", (string)$command);
     }
 
     // Exec