Fix CVE-2020-16846

Stop calling Popen with shell=True to prevent shell injection attacks on
the netapi salt-ssh client.

changelog/cve-2020-16846.fixed

@@ -0,0 +1 @@
+CVE-2020-16804 - Prevent shell injections in netapi ssh client

salt/client/ssh/shell.py

@@ -9,6 +9,7 @@
 
 # Import python libs
 import re
+import shlex
 import subprocess
 import sys
 import time
@@ -31,22 +32,15 @@
 RSTR_RE = re.compile(r"(?:^|\r?\n)" + RSTR + r"(?:\r?\n|$)")
 
 
-class NoPasswdError(Exception):
-    pass
-
-
-class KeyAcceptError(Exception):
-    pass
-
-
 def gen_key(path):
     """
     Generate a key for use with salt-ssh
     """
-    cmd = 'ssh-keygen -P "" -f {0} -t rsa -q'.format(path)
-    if not os.path.isdir(os.path.dirname(path)):
+    cmd = ["ssh-keygen", "-P", '""', "-f", path, "-t", "rsa", "-q"]
+    dirname = os.path.dirname(path)
+    if dirname and not os.path.isdir(dirname):
         os.makedirs(os.path.dirname(path))
-    subprocess.call(cmd, shell=True)
+    subprocess.call(cmd)
 
 
 def gen_shell(opts, **kwargs):
@@ -278,27 +272,13 @@ def _cmd_str(self, cmd, ssh="ssh"):
 
         return " ".join(command)
 
-    def _old_run_cmd(self, cmd):
-        """
-        Cleanly execute the command string
-        """
-        try:
-            proc = subprocess.Popen(
-                cmd, shell=True, stderr=subprocess.PIPE, stdout=subprocess.PIPE,
-            )
-
-            data = proc.communicate()
-            return data[0], data[1], proc.returncode
-        except Exception:  # pylint: disable=broad-except
-            return ("local", "Unknown Error", None)
-
     def _run_nb_cmd(self, cmd):
         """
         cmd iterator
         """
         try:
             proc = salt.utils.nb_popen.NonBlockingPopen(
-                cmd, shell=True, stderr=subprocess.PIPE, stdout=subprocess.PIPE,
+                self._split_cmd(cmd), stderr=subprocess.PIPE, stdout=subprocess.PIPE,
             )
             while True:
                 time.sleep(0.1)
@@ -375,6 +355,21 @@ def send(self, local, remote, makedirs=False):
 
         return self._run_cmd(cmd)
 
+    def _split_cmd(self, cmd):
+        """
+        Split a command string so that it is suitable to pass to Popen without
+        shell=True. This prevents shell injection attacks in the options passed
+        to ssh or some other command.
+        """
+        try:
+            ssh_part, cmd_part = cmd.split("/bin/sh")
+        except ValueError:
+            cmd_lst = shlex.split(cmd)
+        else:
+            cmd_lst = shlex.split(ssh_part)
+            cmd_lst.append("/bin/sh {}".format(cmd_part))
+        return cmd_lst
+
     def _run_cmd(self, cmd, key_accept=False, passwd_retries=3):
         """
         Execute a shell command via VT. This is blocking and assumes that ssh
@@ -384,8 +379,7 @@ def _run_cmd(self, cmd, key_accept=False, passwd_retries=3):
             return "", "No command or passphrase", 245
 
         term = salt.utils.vt.Terminal(
-            cmd,
-            shell=True,
+            self._split_cmd(cmd),
             log_stdout=True,
             log_stdout_level="trace",
             log_stderr=True,

tests/integration/__init__.py

@@ -1319,12 +1319,13 @@ def _exit_ssh(self):
             except OSError as exc:
                 if exc.errno != 3:
                     raise
-            with salt.utils.files.fopen(self.sshd_pidfile) as fhr:
-                try:
-                    os.kill(int(fhr.read()), signal.SIGKILL)
-                except OSError as exc:
-                    if exc.errno != 3:
-                        raise
+            if os.path.exists(self.sshd_pidfile):
+                with salt.utils.files.fopen(self.sshd_pidfile) as fhr:
+                    try:
+                        os.kill(int(fhr.read()), signal.SIGKILL)
+                    except OSError as exc:
+                        if exc.errno != 3:
+                            raise
 
     def _exit_mockbin(self):
         path = os.environ.get("PATH", "")

tests/integration/netapi/test_client.py

@@ -8,6 +8,7 @@
 import pytest
 import salt.config
 import salt.netapi
+import salt.utils.files
 from salt.exceptions import EauthAuthenticationError
 from tests.support.case import SSHCase
 from tests.support.helpers import (
@@ -274,3 +275,125 @@ def test_ssh_authenticated_raw_shell_disabled(self):
 
         self.assertEqual(ret, None)
         self.assertFalse(os.path.exists("badfile.txt"))
+
+    @staticmethod
+    def cleanup_file(path):
+        try:
+            os.remove(path)
+        except OSError:
+            pass
+
+    @staticmethod
+    def cleanup_dir(path):
+        try:
+            salt.utils.files.rm_rf(path)
+        except OSError:
+            pass
+
+    @slowTest
+    def test_shell_inject_ssh_priv(self):
+        """
+        Verify CVE-2020-16846 for ssh_priv variable
+        """
+        # ZDI-CAN-11143
+        path = "/tmp/test-11143"
+        self.addCleanup(self.cleanup_file, path)
+        self.addCleanup(self.cleanup_file, "aaa")
+        self.addCleanup(self.cleanup_file, "aaa.pub")
+        self.addCleanup(self.cleanup_dir, "aaa|id>")
+        low = {
+            "roster": "cache",
+            "client": "ssh",
+            "tgt": "www.zerodayinitiative.com",
+            "fun": "xx",
+            "eauth": "xx",
+            "ssh_priv": "aaa|id>{} #".format(path),
+        }
+        ret = self.netapi.run(low)
+        self.assertFalse(os.path.exists(path))
+
+    @slowTest
+    def test_shell_inject_tgt(self):
+        """
+        Verify CVE-2020-16846 for tgt variable
+        """
+        # ZDI-CAN-11167
+        path = "/tmp/test-11167"
+        self.addCleanup(self.cleanup_file, path)
+        low = {
+            "roster": "cache",
+            "client": "ssh",
+            "tgt": "root|id>{} #@127.0.0.1".format(path),
+            "fun": "xx",
+            "eauth": "xx",
+            "roster_file": "/tmp/salt-tests-tmpdir/config/roaster",
+            "rosters": "/",
+        }
+        ret = self.netapi.run(low)
+        self.assertFalse(os.path.exists(path))
+
+    @slowTest
+    def test_shell_inject_ssh_options(self):
+        """
+        Verify CVE-2020-16846 for ssh_options
+        """
+        # ZDI-CAN-11169
+        path = "/tmp/test-11169"
+        self.addCleanup(self.cleanup_file, path)
+        low = {
+            "roster": "cache",
+            "client": "ssh",
+            "tgt": "127.0.0.1",
+            "renderer": "cheetah",
+            "fun": "xx",
+            "eauth": "xx",
+            "roster_file": "/tmp/salt-tests-tmpdir/config/roaster",
+            "rosters": "/",
+            "ssh_options": ["|id>{} #".format(path), "lol"],
+        }
+        ret = self.netapi.run(low)
+        self.assertFalse(os.path.exists(path))
+
+    @slowTest
+    def test_shell_inject_ssh_port(self):
+        """
+        Verify CVE-2020-16846 for ssh_port variable
+        """
+        # ZDI-CAN-11172
+        path = "/tmp/test-11172"
+        self.addCleanup(self.cleanup_file, path)
+        low = {
+            "roster": "cache",
+            "client": "ssh",
+            "tgt": "127.0.0.1",
+            "renderer": "cheetah",
+            "fun": "xx",
+            "eauth": "xx",
+            "roster_file": "/tmp/salt-tests-tmpdir/config/roaster",
+            "rosters": "/",
+            "ssh_port": "hhhhh|id>{} #".format(path),
+        }
+        ret = self.netapi.run(low)
+        self.assertFalse(os.path.exists(path))
+
+    @slowTest
+    def test_shell_inject_remote_port_forwards(self):
+        """
+        Verify CVE-2020-16846 for remote_port_forwards variable
+        """
+        # ZDI-CAN-11173
+        path = "/tmp/test-1173"
+        self.addCleanup(self.cleanup_file, path)
+        low = {
+            "roster": "cache",
+            "client": "ssh",
+            "tgt": "127.0.0.1",
+            "renderer": "cheetah",
+            "fun": "xx",
+            "eauth": "xx",
+            "roster_file": "/tmp/salt-tests-tmpdir/config/roaster",
+            "rosters": "/",
+            "ssh_remote_port_forwards": "hhhhh|id>{} #, lol".format(path),
+        }
+        ret = self.netapi.run(low)
+        self.assertFalse(os.path.exists(path))