Fix CVE-2020-11652

Sanitize paths in ClearFuncs methods provided by salt-master. This
ensures we do not allow access to un-intended files and directories.

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.path.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)

tests/integration/master/test_clear_funcs.py

@@ -126,3 +126,185 @@ def test_pub_not_allowed(self):
                 ret_evt = raw
                 break
         assert not os.path.exists(self.tmpfile), 'Evil file created'
+
+
+class ClearFuncsConfigTest(TestCase):
+
+    def setUp(self):
+        master_opts = AdaptedConfigurationTestCaseMixin.get_config('master')
+        self.conf_dir = os.path.dirname(master_opts['conf_file'])
+        master = '127.0.0.1'
+        ret_port = 64506
+        user = keyuser()
+        keyfile = '.{}_key'.format(user)
+        keypath = os.path.join(RUNTIME_VARS.TMP, 'rootdir', 'cache', keyfile)
+
+        with salt.utils.files.fopen(keypath) as keyfd:
+            self.key = keyfd.read()
+
+        self.minion_config = {
+            'transport': 'zeromq',
+            'pki_dir': '/tmp',
+            'id': 'root',
+            'master_ip': master,
+            'master_port': ret_port,
+            'auth_timeout': 5,
+            'auth_tries': 1,
+            'master_uri': 'tcp://{0}:{1}'.format(master, ret_port)
+        }
+
+    def tearDown(self):
+        try:
+            os.remove(os.path.join(self.conf_dir, 'evil.conf'))
+        except OSError:
+            pass
+        delattr(self, 'conf_dir')
+        delattr(self, 'key')
+        delattr(self, 'minion_config')
+
+    def test_clearfuncs_config(self):
+        clear_channel = salt.transport.client.ReqChannel.factory(
+            self.minion_config, crypt='clear')
+
+        msg = {
+           'key': self.key,
+           'cmd': 'wheel',
+           'fun': 'config.update_config',
+           'file_name': '../evil',
+           'yaml_contents': 'win',
+        }
+        ret = clear_channel.send(msg, timeout=5)
+        assert not os.path.exists(os.path.join(self.conf_dir, 'evil.conf')), \
+            'Wrote file via directory traversal'
+
+
+class ClearFuncsFileRoots(TestCase):
+
+    def setUp(self):
+        self.master_opts = AdaptedConfigurationTestCaseMixin.get_config('master')
+        self.target_dir = os.path.dirname(
+            self.master_opts['file_roots']['base'][0]
+        )
+        master = '127.0.0.1'
+        ret_port = 64506
+        user = keyuser()
+        self.keyfile = '.{}_key'.format(user)
+        keypath = os.path.join(RUNTIME_VARS.TMP, 'rootdir', 'cache', self.keyfile)
+
+        with salt.utils.files.fopen(keypath) as keyfd:
+            self.key = keyfd.read()
+
+        self.minion_config = {
+            'transport': 'zeromq',
+            'pki_dir': '/tmp',
+            'id': 'root',
+            'master_ip': master,
+            'master_port': ret_port,
+            'auth_timeout': 5,
+            'auth_tries': 1,
+            'master_uri': 'tcp://{0}:{1}'.format(master, ret_port)
+        }
+
+    def tearDown(self):
+        try:
+            os.remove(os.path.join(self.target_dir, 'pwn.txt'))
+        except OSError:
+            pass
+        delattr(self, 'master_opts')
+        delattr(self, 'target_dir')
+        delattr(self, 'keyfile')
+        delattr(self, 'key')
+        delattr(self, 'minion_config')
+
+    def test_fileroots_write(self):
+        clear_channel = salt.transport.client.ReqChannel.factory(
+            self.minion_config, crypt='clear')
+
+        msg = {
+            'key': self.key,
+            'cmd': 'wheel',
+            'fun': 'file_roots.write',
+            'data': 'win',
+            'path': os.path.join('..', 'pwn.txt'),
+            'saltenv': 'base',
+        }
+        ret = clear_channel.send(msg, timeout=5)
+        assert not os.path.exists(os.path.join(self.target_dir, 'pwn.txt')), \
+            'Wrote file via directory traversal'
+
+    def test_fileroots_read(self):
+        rootdir = self.master_opts['root_dir']
+        fileroot = self.master_opts['file_roots']['base'][0]
+        # If this asserion fails the test may need to be re-written
+        assert os.path.dirname(os.path.dirname(rootdir)) == os.path.dirname(fileroot)
+        clear_channel = salt.transport.client.ReqChannel.factory(
+            self.minion_config, crypt='clear')
+        readpath = os.path.join(
+            '..',
+            'salt-tests-tmpdir',
+            'rootdir',
+            'cache',
+            self.keyfile,
+        )
+        msg = {
+            'key': self.key,
+            'cmd': 'wheel',
+            'fun': 'file_roots.read',
+            'path': os.path.join(
+                '..',
+                'salt-tests-tmpdir',
+                'rootdir',
+                'cache',
+                self.keyfile,
+            ),
+            'saltenv': 'base',
+        }
+
+        ret = clear_channel.send(msg, timeout=5)
+        try:
+            # When vulnerable this assertion will fail.
+            assert list(ret['data']['return'][0].items())[0][1] != self.key, \
+                'Read file via directory traversal'
+        except IndexError:
+            pass
+        # If the vulnerability is fixed, no data will be returned.
+        assert ret['data']['return'] == []
+
+
+class ClearFuncsTokenTest(TestCase):
+
+    def setUp(self):
+        self.master_opts = AdaptedConfigurationTestCaseMixin.get_config('master')
+        master = '127.0.0.1'
+        ret_port = 64506
+        self.minion_config = {
+            'transport': 'zeromq',
+            'pki_dir': '/tmp',
+            'id': 'root',
+            'master_ip': master,
+            'master_port': ret_port,
+            'auth_timeout': 5,
+            'auth_tries': 1,
+            'master_uri': 'tcp://{0}:{1}'.format(master, ret_port)
+        }
+
+    def tearDown(self):
+        delattr(self, 'master_opts')
+        delattr(self, 'minion_config')
+
+    def test_token(self):
+        tokensdir = os.path.join(
+            self.master_opts['root_dir'],
+            self.master_opts['cachedir'],
+            'tokens'
+        )
+        assert os.path.exists(tokensdir), tokensdir
+        clear_channel = salt.transport.client.ReqChannel.factory(
+            self.minion_config, crypt='clear')
+        msg = {
+            'arg': [],
+            'cmd': 'get_token',
+            'token': os.path.join('..', 'minions', 'minion', 'data.p'),
+        }
+        ret = clear_channel.send(msg, timeout=5)
+        assert 'pillar' not in ret, 'Read minion data via directory traversal'

tests/unit/test_module_names.py

@@ -133,6 +133,7 @@ def test_module_name_source_match(self):
             'integration.logging.test_jid_logging',
             'integration.logging.handlers.test_logstash_mod',
             'integration.master.test_event_return',
+            'integration.master.test_clear_funcs',
             'integration.minion.test_blackout',
             'integration.minion.test_executor',
             'integration.minion.test_minion_cache',