salt/fix-cve-2020-25592-and-add-tests-bsc-1178319.patch

From e7514afcba4f57c5cb8599f561fcefdcc3db7314 Mon Sep 17 00:00:00 2001
From: "Daniel A. Wozniak" <dwozniak@saltstack.com>
Date: Wed, 16 Sep 2020 00:25:10 +0000
Subject: [PATCH] Fix CVE-2020-25592 and add tests (bsc#1178319)

Properly validate eauth credentials and tokens on SSH calls made by Salt API

(bsc#1178319) (bsc#1178362) (bsc#1178361) (CVE-2020-25592) (CVE-2020-17490) (CVE-2020-16846)
---
 salt/client/ssh/shell.py                |  26 ++-
 salt/modules/tls.py                     |  18 +-
 salt/netapi/__init__.py                 |  67 ++++++
 tests/integration/netapi/test_client.py | 296 +++++++++++++++++++++++-
 4 files changed, 388 insertions(+), 19 deletions(-)

diff --git a/salt/client/ssh/shell.py b/salt/client/ssh/shell.py
index bd55c514ee..27aba7b382 100644
--- a/salt/client/ssh/shell.py
+++ b/salt/client/ssh/shell.py
@@ -8,6 +8,7 @@ from __future__ import absolute_import, print_function, unicode_literals
 import re
 import os
 import sys
+import shlex
 import time
 import logging
 import subprocess
@@ -43,10 +44,10 @@ def gen_key(path):
     '''
     Generate a key for use with salt-ssh
     '''
-    cmd = 'ssh-keygen -P "" -f {0} -t rsa -q'.format(path)
+    cmd = ["ssh-keygen", "-P", '""', "-f", path, "-t", "rsa", "-q"]
     if not os.path.isdir(os.path.dirname(path)):
         os.makedirs(os.path.dirname(path))
-    subprocess.call(cmd, shell=True)
+    subprocess.call(cmd)
 
 
 def gen_shell(opts, **kwargs):
@@ -289,8 +290,7 @@ class Shell(object):
         '''
         try:
             proc = salt.utils.nb_popen.NonBlockingPopen(
-                cmd,
-                shell=True,
+                self._split_cmd(cmd),
                 stderr=subprocess.PIPE,
                 stdout=subprocess.PIPE,
             )
@@ -369,6 +369,21 @@ class Shell(object):
 
         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
@@ -378,8 +393,7 @@ class Shell(object):
             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,
diff --git a/salt/modules/tls.py b/salt/modules/tls.py
index af845621a3..116b5fe379 100644
--- a/salt/modules/tls.py
+++ b/salt/modules/tls.py
@@ -798,12 +798,13 @@ def create_ca(ca_name,
             if old_key.strip() == keycontent.strip():
                 write_key = False
             else:
-                log.info('Saving old CA ssl key in %s', bck)
-                with salt.utils.files.fopen(bck, 'w') as bckf:
+                log.info('Saving old CA ssl key in {0}'.format(bck))
+                fp = os.open(bck, os.O_CREAT | os.O_RDWR, 0o600)
+                with os.fdopen(fp, 'w') as bckf:
                     bckf.write(old_key)
-                    os.chmod(bck, 0o600)
     if write_key:
-        with salt.utils.files.fopen(ca_keyp, 'wb') as ca_key:
+        fp = os.open(ca_keyp, os.O_CREAT | os.O_RDWR, 0o600)
+        with os.fdopen(fp, 'wb') as ca_key:
             ca_key.write(salt.utils.stringutils.to_bytes(keycontent))
 
     with salt.utils.files.fopen(certp, 'wb') as ca_crt:
@@ -1115,9 +1116,9 @@ def create_csr(ca_name,
     req.sign(key, salt.utils.stringutils.to_str(digest))
 
     # Write private key and request
-    with salt.utils.files.fopen('{0}/{1}.key'.format(csr_path,
-                                                     csr_filename),
-                                'wb+') as priv_key:
+    priv_keyp = '{0}/{1}.key'.format(csr_path, csr_filename)
+    fp = os.open(priv_keyp, os.O_CREAT | os.O_RDWR, 0o600)
+    with os.fdopen(fp, 'wb+') as priv_key:
         priv_key.write(
             salt.utils.stringutils.to_bytes(
                 OpenSSL.crypto.dump_privatekey(OpenSSL.crypto.FILETYPE_PEM,
@@ -1266,7 +1267,8 @@ def create_self_signed_cert(tls_dir='tls',
     priv_key_path = '{0}/{1}/certs/{2}.key'.format(cert_base_path(),
                                                    tls_dir,
                                                    cert_filename)
-    with salt.utils.files.fopen(priv_key_path, 'wb+') as priv_key:
+    fp = os.open(priv_key_path, os.O_CREAT | os.O_RDWR, 0o600)
+    with os.fdopen(fp, 'wb+') as priv_key:
         priv_key.write(
             salt.utils.stringutils.to_bytes(
                 OpenSSL.crypto.dump_privatekey(OpenSSL.crypto.FILETYPE_PEM,
diff --git a/salt/netapi/__init__.py b/salt/netapi/__init__.py
index 31a24bb420..4e5b6b093a 100644
--- a/salt/netapi/__init__.py
+++ b/salt/netapi/__init__.py
@@ -3,24 +3,36 @@
 Make api awesomeness
 '''
 from __future__ import absolute_import, print_function, unicode_literals
+
+import copy
+
 # Import Python libs
 import inspect
+import logging
 import os
 
 # Import Salt libs
 import salt.log  # pylint: disable=W0611
+import salt.auth
 import salt.client
 import salt.config
+import salt.daemons.masterapi
 import salt.runner
 import salt.syspaths
 import salt.wheel
 import salt.utils.args
 import salt.client.ssh.client
 import salt.exceptions
+import salt.utils.args
+import salt.utils.minions
+import salt.wheel
+from salt.defaults import DEFAULT_TARGET_DELIM
 
 # Import third party libs
 from salt.ext import six
 
+log = logging.getLogger(__name__)
+
 
 class NetapiClient(object):
     '''
@@ -34,6 +46,15 @@ class NetapiClient(object):
 
     def __init__(self, opts):
         self.opts = opts
+        apiopts = copy.deepcopy(self.opts)
+        apiopts["enable_ssh_minions"] = True
+        apiopts["cachedir"] = os.path.join(opts["cachedir"], "saltapi")
+        if not os.path.exists(apiopts["cachedir"]):
+            os.makedirs(apiopts["cachedir"])
+        self.resolver = salt.auth.Resolver(apiopts)
+        self.loadauth = salt.auth.LoadAuth(apiopts)
+        self.key = salt.daemons.masterapi.access_keys(apiopts)
+        self.ckminions = salt.utils.minions.CkMinions(apiopts)
 
     def _is_master_running(self):
         '''
@@ -55,6 +76,49 @@ class NetapiClient(object):
             self.opts['sock_dir'],
             ipc_file))
 
+    def _prep_auth_info(self, clear_load):
+        sensitive_load_keys = []
+        key = None
+        if "token" in clear_load:
+            auth_type = "token"
+            err_name = "TokenAuthenticationError"
+            sensitive_load_keys = ["token"]
+            return auth_type, err_name, key, sensitive_load_keys
+        elif "eauth" in clear_load:
+            auth_type = "eauth"
+            err_name = "EauthAuthenticationError"
+            sensitive_load_keys = ["username", "password"]
+            return auth_type, err_name, key, sensitive_load_keys
+        raise salt.exceptions.EauthAuthenticationError(
+            "No authentication credentials given"
+        )
+
+    def _authorize_ssh(self, low):
+        auth_type, err_name, key, sensitive_load_keys = self._prep_auth_info(low)
+        auth_check = self.loadauth.check_authentication(low, auth_type, key=key)
+        auth_list = auth_check.get("auth_list", [])
+        error = auth_check.get("error")
+        if error:
+            raise salt.exceptions.EauthAuthenticationError(error)
+        delimiter = low.get("kwargs", {}).get("delimiter", DEFAULT_TARGET_DELIM)
+        _res = self.ckminions.check_minions(
+            low["tgt"], low.get("tgt_type", "glob"), delimiter
+        )
+        minions = _res.get("minions", list())
+        missing = _res.get("missing", list())
+        authorized = self.ckminions.auth_check(
+            auth_list,
+            low["fun"],
+            low.get("arg", []),
+            low["tgt"],
+            low.get("tgt_type", "glob"),
+            minions=minions,
+        )
+        if not authorized:
+            raise salt.exceptions.EauthAuthenticationError(
+                "Authorization error occurred."
+            )
+
     def run(self, low):
         '''
         Execute the specified function in the specified client by passing the
@@ -80,6 +144,9 @@ class NetapiClient(object):
             raise salt.exceptions.EauthAuthenticationError(
                     'Raw shell option not allowed.')
 
+        if low['client'] == 'ssh':
+            self._authorize_ssh(low)
+
         l_fun = getattr(self, low['client'])
         f_call = salt.utils.args.format_call(l_fun, low)
         return l_fun(*f_call.get('args', ()), **f_call.get('kwargs', {}))
diff --git a/tests/integration/netapi/test_client.py b/tests/integration/netapi/test_client.py
index 08030f31ec..b99bdfe313 100644
--- a/tests/integration/netapi/test_client.py
+++ b/tests/integration/netapi/test_client.py
@@ -1,26 +1,30 @@
 # encoding: utf-8
-
 # Import Python libs
 from __future__ import absolute_import, print_function, unicode_literals
+import copy
 import logging
 import os
 import time
 
+import salt.config
+import salt.netapi
+import salt.utils.files
+import salt.utils.platform
+import salt.utils.pycrypto
+
 # Import Salt Testing libs
 from tests.support.paths import TMP_CONF_DIR, TMP
 from tests.support.runtests import RUNTIME_VARS
 from tests.support.unit import TestCase, skipIf
 from tests.support.mock import patch
-from tests.support.case import SSHCase
+from tests.support.case import ModuleCase, SSHCase
+from salt.exceptions import EauthAuthenticationError
 from tests.support.helpers import (
     Webserver,
     SaveRequestsPostHandler,
     requires_sshd_server
 )
 
-# Import Salt libs
-import salt.config
-import salt.netapi
 
 from salt.exceptions import (
     EauthAuthenticationError
@@ -174,6 +178,10 @@ class NetapiSSHClientTest(SSHCase):
         '''
         opts = salt.config.client_config(os.path.join(TMP_CONF_DIR, 'master'))
         self.netapi = salt.netapi.NetapiClient(opts)
+        opts = salt.config.client_config(os.path.join(TMP_CONF_DIR, "master"))
+        naopts = copy.deepcopy(opts)
+        naopts["ignore_host_keys"] = True
+        self.netapi = salt.netapi.NetapiClient(naopts)
 
         self.priv_file = os.path.join(RUNTIME_VARS.TMP_CONF_DIR, 'key_test')
         self.rosters = os.path.join(RUNTIME_VARS.TMP_CONF_DIR)
@@ -271,3 +279,281 @@ class NetapiSSHClientTest(SSHCase):
 
         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
+
+    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",
+            "ssh_priv": "aaa|id>{} #".format(path),
+            "fun": "test.ping",
+            "eauth": "auto",
+            "username": "saltdev_auto",
+            "password": "saltdev",
+        }
+        ret = self.netapi.run(low)
+        self.assertFalse(os.path.exists(path))
+
+    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),
+            "roster_file": "/tmp/salt-tests-tmpdir/config/roaster",
+            "rosters": "/",
+            "fun": "test.ping",
+            "eauth": "auto",
+            "username": "saltdev_auto",
+            "password": "saltdev",
+        }
+        ret = self.netapi.run(low)
+        self.assertFalse(os.path.exists(path))
+
+    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": "test.ping",
+            "eauth": "auto",
+            "username": "saltdev_auto",
+            "password": "saltdev",
+            "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))
+
+    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": "test.ping",
+            "eauth": "auto",
+            "username": "saltdev_auto",
+            "password": "saltdev",
+            "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))
+
+    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": "test.ping",
+            "roster_file": "/tmp/salt-tests-tmpdir/config/roaster",
+            "rosters": "/",
+            "ssh_remote_port_forwards": "hhhhh|id>{} #, lol".format(path),
+            "eauth": "auto",
+            "username": "saltdev_auto",
+            "password": "saltdev",
+        }
+        ret = self.netapi.run(low)
+        self.assertFalse(os.path.exists(path))
+
+
+@requires_sshd_server
+class NetapiSSHClientAuthTest(SSHCase):
+
+    USERA = "saltdev"
+    USERA_PWD = "saltdev"
+
+    def setUp(self):
+        """
+        Set up a NetapiClient instance
+        """
+        opts = salt.config.client_config(os.path.join(TMP_CONF_DIR, "master"))
+        naopts = copy.deepcopy(opts)
+        naopts["ignore_host_keys"] = True
+        self.netapi = salt.netapi.NetapiClient(naopts)
+
+        self.priv_file = os.path.join(RUNTIME_VARS.TMP_CONF_DIR, "key_test")
+        self.rosters = os.path.join(RUNTIME_VARS.TMP_CONF_DIR)
+        # Initialize salt-ssh
+        self.run_function("test.ping")
+        self.mod_case = ModuleCase()
+        try:
+            add_user = self.mod_case.run_function(
+                "user.add", [self.USERA], createhome=False
+            )
+            self.assertTrue(add_user)
+            if salt.utils.platform.is_darwin():
+                hashed_password = self.USERA_PWD
+            else:
+                hashed_password = salt.utils.pycrypto.gen_hash(password=self.USERA_PWD)
+            add_pwd = self.mod_case.run_function(
+                "shadow.set_password", [self.USERA, hashed_password],
+            )
+            self.assertTrue(add_pwd)
+        except AssertionError:
+            self.mod_case.run_function("user.delete", [self.USERA], remove=True)
+            self.skipTest("Could not add user or password, skipping test")
+
+    def tearDown(self):
+        del self.netapi
+        self.mod_case.run_function("user.delete", [self.USERA], remove=True)
+
+    @classmethod
+    def setUpClass(cls):
+        cls.post_webserver = Webserver(handler=SaveRequestsPostHandler)
+        cls.post_webserver.start()
+        cls.post_web_root = cls.post_webserver.web_root
+        cls.post_web_handler = cls.post_webserver.handler
+
+    @classmethod
+    def tearDownClass(cls):
+        cls.post_webserver.stop()
+        del cls.post_webserver
+
+    def test_ssh_auth_bypass(self):
+        """
+        CVE-2020-25592 - Bogus eauth raises exception.
+        """
+        low = {
+            "roster": "cache",
+            "client": "ssh",
+            "tgt": "127.0.0.1",
+            "renderer": "cheetah",
+            "fun": "test.ping",
+            "roster_file": "/tmp/salt-tests-tmpdir/config/roaster",
+            "rosters": "/",
+            "eauth": "xx",
+        }
+        with self.assertRaises(salt.exceptions.EauthAuthenticationError):
+            ret = self.netapi.run(low)
+
+    def test_ssh_auth_valid(self):
+        """
+        CVE-2020-25592 - Valid eauth works as expected.
+        """
+        low = {
+            "client": "ssh",
+            "tgt": "localhost",
+            "fun": "test.ping",
+            "roster_file": "roster",
+            "rosters": [self.rosters],
+            "ssh_priv": self.priv_file,
+            "eauth": "pam",
+            "username": "saltdev",
+            "password": "saltdev",
+        }
+        ret = self.netapi.run(low)
+        assert "localhost" in ret
+        assert ret["localhost"]["return"] is True
+
+    def test_ssh_auth_invalid(self):
+        """
+        CVE-2020-25592 - Wrong password raises exception.
+        """
+        low = {
+            "client": "ssh",
+            "tgt": "localhost",
+            "fun": "test.ping",
+            "roster_file": "roster",
+            "rosters": [self.rosters],
+            "ssh_priv": self.priv_file,
+            "eauth": "pam",
+            "username": "saltdev",
+            "password": "notvalidpassword",
+        }
+        with self.assertRaises(salt.exceptions.EauthAuthenticationError):
+            ret = self.netapi.run(low)
+
+    def test_ssh_auth_invalid_acl(self):
+        """
+        CVE-2020-25592 - Eauth ACL enforced.
+        """
+        low = {
+            "client": "ssh",
+            "tgt": "localhost",
+            "fun": "at.at",
+            "args": ["12:05am", "echo foo"],
+            "roster_file": "roster",
+            "rosters": [self.rosters],
+            "ssh_priv": self.priv_file,
+            "eauth": "pam",
+            "username": "saltdev",
+            "password": "notvalidpassword",
+        }
+        with self.assertRaises(salt.exceptions.EauthAuthenticationError):
+            ret = self.netapi.run(low)
+
+    def test_ssh_auth_token(self):
+        """
+        CVE-2020-25592 - Eauth tokens work as expected.
+        """
+        low = {
+            "eauth": "pam",
+            "username": "saltdev",
+            "password": "saltdev",
+        }
+        ret = self.netapi.loadauth.mk_token(low)
+        assert "token" in ret and ret["token"]
+        low = {
+            "client": "ssh",
+            "tgt": "localhost",
+            "fun": "test.ping",
+            "roster_file": "roster",
+            "rosters": [self.rosters],
+            "ssh_priv": self.priv_file,
+            "token": ret["token"],
+        }
+        ret = self.netapi.run(low)
+        assert "localhost" in ret
+        assert ret["localhost"]["return"] is True
-- 
2.28.0