Merge pull request #35413 from cachedout/issue_35296

Resolve path issues with cp.push

salt/master.py

@@ -1061,10 +1061,9 @@ class AESFuncs(object):
         '''
         if any(key not in load for key in ('id', 'path', 'loc')):
             return False
-        if not self.opts['file_recv'] or os.path.isabs(load['path']):
+        if not isinstance(load['path'], list):
             return False
-        if os.path.isabs(load['path']) or '../' in load['path']:
-            # Can overwrite master files!!
+        if not self.opts['file_recv']:
             return False
         if not salt.utils.verify.valid_id(self.opts, load['id']):
             return False
@@ -1100,18 +1099,28 @@ class AESFuncs(object):
             )
             return {}
         load.pop('tok')
-        # Normalize Windows paths
-        normpath = load['path']
-        if ':' in normpath:
-            # make sure double backslashes are normalized
-            normpath = normpath.replace('\\', '/')
-            normpath = os.path.normpath(normpath)
+
+        # Path normalization should have been done by the sending
+        # minion but we can't guarantee it. Re-do it here.
+        normpath = os.path.normpath(os.path.join(*load['path']))
+
+        # Ensure that this safety check is done after the path
+        # have been normalized.
+        if os.path.isabs(normpath) or '../' in load['path']:
+            # Can overwrite master files!!
+            return False
+
         cpath = os.path.join(
             self.opts['cachedir'],
             'minions',
             load['id'],
             'files',
             normpath)
+        # One last safety check here
+        if not os.path.normpath(cpath).startswith(self.opts['cachedir']):
+            log.warning('Attempt to write received file outside of master cache '
+                        'directory! Requested file write: {0}. Access denied.'.format(cpath))
+            return False
         cdir = os.path.dirname(cpath)
         if not os.path.isdir(cdir):
             try:

salt/modules/cp.py

@@ -782,9 +782,19 @@ def push(path, keep_symlinks=False, upload_path=None):
         load_path = upload_path.lstrip(os.sep)
     else:
         load_path = path.lstrip(os.sep)
+    # Normalize the path. This does not eliminate
+    # the possibility that relative entries will still be present
+    load_path_normal = os.path.normpath(load_path)
+
+    # If this is Windows and a drive letter is present, remove it
+    load_path_split_drive = os.path.splitdrive(load_path_normal)[1:]
+
+    # Finally, split the remaining path into a list for delivery to the master
+    load_path_list = os.path.split(load_path_split_drive)
+
     load = {'cmd': '_file_recv',
             'id': __opts__['id'],
-            'path': load_path,
+            'path': load_path_list,
             'tok': auth.gen_token('salt')}
     channel = salt.transport.Channel.factory(__opts__)
     with salt.utils.fopen(path, 'rb') as fp_:

tests/integration/files/conf/master

@@ -10,6 +10,7 @@ sock_dir: .salt-unix
 open_mode: True
 syndic_master: localhost
 fileserver_list_cache_time: 0
+file_recv: True
 pillar_opts: True
 peer:
   '.*':

tests/integration/modules/cp.py

@@ -4,6 +4,7 @@
 from __future__ import absolute_import
 import os
 import hashlib
+import tempfile
 
 # Import Salt Testing libs
 from salttesting.helpers import ensure_in_syspath
@@ -322,6 +323,22 @@ class CPModuleTest(integration.ModuleCase):
         finally:
             os.unlink(tgt)
 
+    def test_push(self):
+        log_to_xfer = os.path.join(tempfile.gettempdir(), 'salt-runtests.log')
+        try:
+            self.run_function('cp.push', log_to_xfer)
+            tgt_cache_file = os.path.join(
+                 integration.TMP,
+                'master-minion-root',
+                'cache',
+                'minions',
+                'minion',
+                'files',
+                tempfile.gettempdir(),
+                'salt-runtests.log')
+            self.assertTrue(os.path.isfile(tgt_cache_file), 'File was not cached on the master')
+        finally:
+            os.unlink(tgt_cache_file)
 
 if __name__ == '__main__':
     from integration import run_tests