File fix-cve-2020-25592-and-add-tests-bsc-1178319.patch of Package salt.16872
From 7e2c41e02a21350433dbe239b7d54ec28bcb099b 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
