Merge pull request saltstack#2 from saltstack/security-fixes

Security fix 2019.2.4

doc/topics/hardening.rst

@@ -57,6 +57,10 @@ Salt hardening tips
   particularly sensitive minions. There is also :ref:`salt-ssh` or the
   :mod:`modules.sudo <salt.modules.sudo>` if you need to further restrict
   a minion.
+- Monitor specific security releated log messages. Salt ``salt-master`` logs
+  attempts to access methods which are not exposed to network clients. These log
+  messages are logged at the ``error`` log level and start with ``Requested
+  method not exposed``.
 
 .. _salt-users: https://groups.google.com/forum/#!forum/salt-users
 .. _salt-announce: https://groups.google.com/forum/#!forum/salt-announce

doc/topics/releases/2019.2.4.rst

@@ -0,0 +1,24 @@
+===========================
+Salt 2019.2.4 Release Notes
+===========================
+
+Version 2019.2.4 is a CVE-fix release for :ref:`2019.2.0 <release-2019-2-0>`.
+
+Security Fix
+============
+
+**CVE-2020-11651** 
+
+An issue was discovered in SaltStack Salt before 2019.2.4 and 3000 before 3000.2.
+The salt-master process ClearFuncs class does not properly validate
+method calls. This allows a remote user to access some methods without
+authentication. These methods can be used to retrieve user tokens from
+the salt master and/or run arbitrary commands on salt minions.
+
+
+**CVE-2020-11652** 
+
+An issue was discovered in SaltStack Salt before 2019.2.4 and 3000 before 3000.2.
+The salt-master process ClearFuncs class allows access to some methods
+that improperly sanitize paths. These methods allow arbitrary
+directory access to authenticated users.

salt/master.py

@@ -1088,12 +1088,13 @@ def _handle_clear(self, load):
         '''
         log.trace('Clear payload received with command %s', load['cmd'])
         cmd = load['cmd']
-        if cmd.startswith('__'):
-            return False
+        method = self.clear_funcs.get_method(cmd)
+        if not method:
+            return {}, {'fun': 'send_clear'}
         if self.opts['master_stats']:
             start = time.time()
             self.stats[cmd]['runs'] += 1
-        ret = getattr(self.clear_funcs, cmd)(load), {'fun': 'send_clear'}
+        ret = method(load), {'fun': 'send_clear'}
         if self.opts['master_stats']:
             self._post_stats(start, cmd)
         return ret
@@ -1111,8 +1112,9 @@ def _handle_aes(self, data):
             return {}
         cmd = data['cmd']
         log.trace('AES payload received with command %s', data['cmd'])
-        if cmd.startswith('__'):
-            return False
+        method = self.aes_funcs.get_method(cmd)
+        if not method:
+            return {}, {'fun': 'send'}
         if self.opts['master_stats']:
             start = time.time()
             self.stats[cmd]['runs'] += 1
@@ -1143,13 +1145,44 @@ def run(self):
         self.__bind()
 
 
+class TransportMethods(object):
+    '''
+    Expose methods to the transport layer, methods with their names found in
+    the class attribute 'expose_methods' will be exposed to the transport layer
+    via 'get_method'.
+    '''
+
+    expose_methods = ()
+
+    def get_method(self, name):
+        '''
+        Get a method which should be exposed to the transport layer
+        '''
+        if name in self.expose_methods:
+            try:
+                return getattr(self, name)
+            except AttributeError:
+                log.error("Requested method not exposed: %s", name)
+        else:
+            log.error("Requested method not exposed: %s", name)
+
+
 # TODO: rename? No longer tied to "AES", just "encrypted" or "private" requests
-class AESFuncs(object):
+class AESFuncs(TransportMethods):
     '''
     Set up functions that are available when the load is encrypted with AES
     '''
-    # The AES Functions:
-    #
+
+    expose_methods = (
+        'verify_minion', '_master_tops', '_ext_nodes', '_master_opts',
+        '_mine_get', '_mine', '_mine_delete', '_mine_flush', '_file_recv',
+        '_pillar', '_minion_event', '_handle_minion_event', '_return',
+        '_syndic_return', '_minion_runner', 'pub_ret', 'minion_pub',
+        'minion_publish', 'revoke_auth', 'run_func', '_serve_file',
+        '_file_find', '_file_hash', '_file_find_and_stat', '_file_list',
+        '_file_list_emptydirs', '_dir_list', '_symlink_list', '_file_envs',
+    )
+
     def __init__(self, opts):
         '''
         Create a new AESFuncs
@@ -1863,11 +1896,18 @@ def run_func(self, func, load):
         return ret, {'fun': 'send'}
 
 
-class ClearFuncs(object):
+class ClearFuncs(TransportMethods):
     '''
     Set up functions that are safe to execute when commands sent to the master
     without encryption and authentication
     '''
+
+    # These methods will be exposed to the transport layer by
+    # MWorker._handle_clear
+    expose_methods = (
+        'ping', 'publish', 'get_token', 'mk_token', 'wheel', 'runner',
+    )
+
     # The ClearFuncs object encapsulates the functions that can be executed in
     # the clear:
     # publish (The publish from the LocalClient)

salt/tokens/localfs.py

@@ -12,6 +12,7 @@
 
 import salt.utils.files
 import salt.utils.path
+import salt.utils.verify
 import salt.payload
 
 from salt.ext import six
@@ -61,6 +62,8 @@ def get_token(opts, tok):
     :returns: Token data if successful. Empty dict if failed.
     '''
     t_path = os.path.join(opts['token_dir'], tok)
+    if not salt.utils.verify.clean_path(opts['token_dir'], t_path):
+        return {}
     if not os.path.isfile(t_path):
         return {}
     serial = salt.payload.Serial(opts)

salt/utils/verify.py

@@ -31,6 +31,7 @@
 import salt.utils.path
 import salt.utils.platform
 import salt.utils.user
+import salt.ext.six
 
 log = logging.getLogger(__name__)
 
@@ -495,23 +496,69 @@ def check_max_open_files(opts):
     log.log(level=level, msg=msg)
 
 
+def _realpath_darwin(path):
+    base = ''
+    for part in path.split(os.path.sep)[1:]:
+        if base != '':
+            if os.path.islink(os.path.sep.join([base, part])):
+                base = os.readlink(os.path.sep.join([base, part]))
+            else:
+                base = os.path.abspath(os.path.sep.join([base, part]))
+        else:
+            base = os.path.abspath(os.path.sep.join([base, part]))
+    return base
+
+
+def _realpath_windows(path):
+    base = ''
+    for part in path.split(os.path.sep):
+        if base != '':
+            try:
+                part = os.readlink(os.path.sep.join([base, part]))
+                base = os.path.abspath(part)
+            except OSError:
+                base = os.path.abspath(os.path.sep.join([base, part]))
+        else:
+            base = part
+    return base
+
+
+def _realpath(path):
+    '''
+    Cross platform realpath method. On Windows when python 3, this method
+    uses the os.readlink method to resolve any filesystem links. On Windows
+    when python 2, this method is a no-op. All other platforms and version use
+    os.realpath
+    '''
+    if salt.utils.platform.is_darwin():
+        return _realpath_darwin(path)
+    elif salt.utils.platform.is_windows():
+        if salt.ext.six.PY3:
+            return _realpath_windows(path)
+        else:
+            return path
+    return os.path.realpath(path)
+
+
 def clean_path(root, path, subdir=False):
     '''
     Accepts the root the path needs to be under and verifies that the path is
     under said root. Pass in subdir=True if the path can result in a
     subdirectory of the root instead of having to reside directly in the root
     '''
-    if not os.path.isabs(root):
+    real_root = _realpath(root)
+    if not os.path.isabs(real_root):
         return ''
     if not os.path.isabs(path):
         path = os.path.join(root, path)
     path = os.path.normpath(path)
+    real_path = _realpath(path)
     if subdir:
-        if path.startswith(root):
-            return path
+        if real_path.startswith(real_root):
+            return real_path
     else:
-        if os.path.dirname(path) == os.path.normpath(root):
-            return path
+        if os.path.dirname(real_path) == os.path.normpath(real_root):
+            return real_path
     return ''
 
 

salt/wheel/config.py

@@ -75,13 +75,19 @@ def update_config(file_name, yaml_contents):
     dir_path = os.path.join(__opts__['config_dir'],
                             os.path.dirname(__opts__['default_include']))
     try:
-        yaml_out = salt.utils.yaml.safe_dump(yaml_contents, default_flow_style=False)
+        yaml_out = salt.utils.yaml.safe_dump(
+            yaml_contents,
+            default_flow_style=False,
+        )
 
         if not os.path.exists(dir_path):
             log.debug('Creating directory %s', dir_path)
             os.makedirs(dir_path, 0o755)
 
         file_path = os.path.join(dir_path, file_name)
+        if not salt.utils.verify.clean_path(dir_path, file_path):
+            return 'Invalid path'
+
         with salt.utils.files.fopen(file_path, 'w') as fp_:
             fp_.write(yaml_out)
 

salt/wheel/file_roots.py

@@ -25,6 +25,8 @@ def find(path, saltenv='base'):
         return ret
     for root in __opts__['file_roots'][saltenv]:
         full = os.path.join(root, path)
+        if not salt.utils.verify.clean_path(root, full):
+            continue
         if os.path.isfile(full):
             # Add it to the dict
             with salt.utils.files.fopen(full, 'rb') as fp_:
@@ -107,7 +109,10 @@ def write(data, path, saltenv='base', index=0):
     if os.path.isabs(path):
         return ('The path passed in {0} is not relative to the environment '
                 '{1}').format(path, saltenv)
-    dest = os.path.join(__opts__['file_roots'][saltenv][index], path)
+    root = __opts__['file_roots'][saltenv][index]
+    dest = os.path.join(root, path)
+    if not salt.utils.verify.clean_path(root, dest, subdir=True):
+        return 'Invalid path: {}'.format(path)
     dest_dir = os.path.dirname(dest)
     if not os.path.isdir(dest_dir):
         os.makedirs(dest_dir)