Merge pull request #44010 from Ch3LL/2016.3.7_follow_up

Security Fixes for 2016.3.8

salt/crypt.py

@@ -577,6 +577,9 @@ class AsyncAuth(object):
                 log.warning('SaltReqTimeoutError: {0}'.format(e))
                 raise tornado.gen.Return('retry')
             raise SaltClientError('Attempt to authenticate with the salt master failed with timeout error')
+        if not isinstance(payload, dict):
+            log.error('Sign-in attempt failed: %s', payload)
+            raise tornado.gen.Return(False)
         if 'load' in payload:
             if 'ret' in payload['load']:
                 if not payload['load']['ret']:

salt/transport/tcp.py

@@ -499,6 +499,17 @@ class TCPReqServerChannel(salt.transport.mixins.auth.AESReqServerMixin, salt.tra
                 'payload and load must be a dict', header=header))
             raise tornado.gen.Return()
 
+        try:
+            id_ = payload['load'].get('id', '')
+            if '\0' in id_:
+                log.error('Payload contains an id with a null byte: %s', payload)
+                stream.send(self.serial.dumps('bad load: id contains a null byte'))
+                raise tornado.gen.Return()
+        except TypeError:
+            log.error('Payload contains non-string id: %s', payload)
+            stream.send(self.serial.dumps('bad load: id {0} is not a string'.format(id_)))
+            raise tornado.gen.Return()
+
         # intercept the "_auth" commands, since the main daemon shouldn't know
         # anything about our key auth
         if payload['enc'] == 'clear' and payload.get('load', {}).get('cmd') == '_auth':

salt/transport/zeromq.py

@@ -593,6 +593,17 @@ class ZeroMQReqServerChannel(salt.transport.mixins.auth.AESReqServerMixin, salt.
             stream.send(self.serial.dumps('payload and load must be a dict'))
             raise tornado.gen.Return()
 
+        try:
+            id_ = payload['load'].get('id', '')
+            if '\0' in id_:
+                log.error('Payload contains an id with a null byte: %s', payload)
+                stream.send(self.serial.dumps('bad load: id contains a null byte'))
+                raise tornado.gen.Return()
+        except TypeError:
+            log.error('Payload contains non-string id: %s', payload)
+            stream.send(self.serial.dumps('bad load: id {0} is not a string'.format(id_)))
+            raise tornado.gen.Return()
+
         # intercept the "_auth" commands, since the main daemon shouldn't know
         # anything about our key auth
         if payload['enc'] == 'clear' and payload.get('load', {}).get('cmd') == '_auth':

salt/utils/verify.py

@@ -485,22 +485,15 @@ def clean_path(root, path, subdir=False):
     return ''
 
 
-def clean_id(id_):
-    '''
-    Returns if the passed id is clean.
-    '''
-    if re.search(r'\.\.{sep}'.format(sep=os.sep), id_):
-        return False
-    return True
-
-
 def valid_id(opts, id_):
     '''
     Returns if the passed id is valid
     '''
     try:
-        return bool(clean_path(opts['pki_dir'], id_)) and clean_id(id_)
-    except (AttributeError, KeyError) as e:
+        if any(x in id_ for x in ('/', '\\', '\0')):
+            return False
+        return bool(clean_path(opts['pki_dir'], id_))
+    except (AttributeError, KeyError, TypeError):
         return False
 
 

tests/unit/utils/verify_test.py

@@ -60,6 +60,16 @@ class TestVerify(TestCase):
         opts = {'pki_dir': '/tmp/whatever'}
         self.assertFalse(valid_id(opts, None))
 
+    def test_valid_id_pathsep(self):
+        '''
+        Path separators in id should make it invalid
+        '''
+        opts = {'pki_dir': '/tmp/whatever'}
+        # We have to test both path separators because os.path.normpath will
+        # convert forward slashes to backslashes on Windows.
+        for pathsep in ('/', '\\'):
+            self.assertFalse(valid_id(opts, pathsep.join(('..', 'foobar'))))
+
     def test_zmq_verify(self):
         self.assertTrue(zmq_version())