Merge pull request #45 from Kirill89/master

Update addArg() to improve escaping
mikehaertl · Dec 20, 2019 · 8d98d85 · 8d98d85
2 parents 6c6f44c + c2ef7db
commit 8d98d85
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 12 deletions.
diff --git 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
@@ -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
@@ -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