File security-fixes-cve-2017-14695-and-cve-2017-1469... of Package salt

Overview Repositories Revisions Requests Users Attributes Meta

File security-fixes-cve-2017-14695-and-cve-2017-14696.patch of Package salt (Revision 3a7c2d45577744db966838fdc07c2570)

Currently displaying revision 3a7c2d45577744db966838fdc07c2570 , Show latest

From b7ef927ef4a2548217750ee26c09c649733001fd Mon Sep 17 00:00:00 2001
From: Erik Johnson <palehose@gmail.com>
Date: Fri, 25 Aug 2017 14:15:58 -0500
Subject: [PATCH] Security fixes: CVE-2017-14695 and CVE-2017-14696

* Don't allow path separators in minion ID
* Do not allow IDs with null bytes in decoded payloads
---
 salt/crypt.py                   |   3 ++
 salt/transport/tcp.py           | 109 ++++++++++++++++++++++++----------------
 salt/transport/zeromq.py        |  11 ++++
 salt/utils/verify.py            |  15 ++----
 tests/unit/utils/verify_test.py |  10 ++++
 5 files changed, 94 insertions(+), 54 deletions(-)

diff --git a/salt/crypt.py b/salt/crypt.py
index 6658e46919..fbff832d0d 100644
--- a/salt/crypt.py
+++ b/salt/crypt.py
@@ -576,6 +576,9 @@ class AsyncAuth(object):
                 raise tornado.gen.Return('retry')
             else:
                 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']:
diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py
index 29ef0746d5..cf047a4744 100644
--- a/salt/transport/tcp.py
+++ b/salt/transport/tcp.py
@@ -611,49 +611,72 @@ class TCPReqServerChannel(salt.transport.mixins.auth.AESReqServerMixin, salt.tra
         Handle incoming messages from underylying tcp streams
         '''
         try:
-            payload = self._decode_payload(payload)
-        except Exception:
-            stream.write(salt.transport.frame.frame_msg('bad load', header=header))
-            raise tornado.gen.Return()
-
-        # TODO helper functions to normalize payload?
-        if not isinstance(payload, dict) or not isinstance(payload.get('load'), dict):
-            yield stream.write(salt.transport.frame.frame_msg(
-                'payload and load must be a dict', header=header))
-            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':
-            yield stream.write(salt.transport.frame.frame_msg(
-                self._auth(payload['load']), header=header))
-            raise tornado.gen.Return()
-
-        # TODO: test
-        try:
-            ret, req_opts = yield self.payload_handler(payload)
-        except Exception as e:
-            # always attempt to return an error to the minion
-            stream.write('Some exception handling minion payload')
-            log.error('Some exception handling a payload from minion', exc_info=True)
-            stream.close()
-            raise tornado.gen.Return()
-
-        req_fun = req_opts.get('fun', 'send')
-        if req_fun == 'send_clear':
-            stream.write(salt.transport.frame.frame_msg(ret, header=header))
-        elif req_fun == 'send':
-            stream.write(salt.transport.frame.frame_msg(self.crypticle.dumps(ret), header=header))
-        elif req_fun == 'send_private':
-            stream.write(salt.transport.frame.frame_msg(self._encrypt_private(ret,
-                                                         req_opts['key'],
-                                                         req_opts['tgt'],
-                                                         ), header=header))
-        else:
-            log.error('Unknown req_fun {0}'.format(req_fun))
-            # always attempt to return an error to the minion
-            stream.write('Server-side exception handling payload')
-            stream.close()
+            try:
+                payload = self._decode_payload(payload)
+            except Exception:
+                stream.write(salt.transport.frame.frame_msg('bad load', header=header))
+                raise tornado.gen.Return()
+
+            # TODO helper functions to normalize payload?
+            if not isinstance(payload, dict) or not isinstance(payload.get('load'), dict):
+                yield stream.write(salt.transport.frame.frame_msg(
+                    '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':
+                yield stream.write(salt.transport.frame.frame_msg(
+                    self._auth(payload['load']), header=header))
+                raise tornado.gen.Return()
+
+            # TODO: test
+            try:
+                ret, req_opts = yield self.payload_handler(payload)
+            except Exception as e:
+                # always attempt to return an error to the minion
+                stream.write('Some exception handling minion payload')
+                log.error('Some exception handling a payload from minion', exc_info=True)
+                stream.close()
+                raise tornado.gen.Return()
+
+            req_fun = req_opts.get('fun', 'send')
+            if req_fun == 'send_clear':
+                stream.write(salt.transport.frame.frame_msg(ret, header=header))
+            elif req_fun == 'send':
+                stream.write(salt.transport.frame.frame_msg(self.crypticle.dumps(ret), header=header))
+            elif req_fun == 'send_private':
+                stream.write(salt.transport.frame.frame_msg(self._encrypt_private(ret,
+                                                             req_opts['key'],
+                                                             req_opts['tgt'],
+                                                             ), header=header))
+            else:
+                log.error('Unknown req_fun {0}'.format(req_fun))
+                # always attempt to return an error to the minion
+                stream.write('Server-side exception handling payload')
+                stream.close()
+        except tornado.gen.Return:
+            raise
+        except tornado.iostream.StreamClosedError:
+            # Stream was closed. This could happen if the remote side
+            # closed the connection on its end (eg in a timeout or shutdown
+            # situation).
+            log.error('Connection was unexpectedly closed', exc_info=True)
+        except Exception as exc:  # pylint: disable=broad-except
+            # Absorb any other exceptions
+            log.error('Unexpected exception occurred: {0}'.format(exc), exc_info=True)
+
         raise tornado.gen.Return()
 
 
diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py
index 48bece8344..58106120ba 100644
--- a/salt/transport/zeromq.py
+++ b/salt/transport/zeromq.py
@@ -605,6 +605,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':
diff --git a/salt/utils/verify.py b/salt/utils/verify.py
index 8690f6fb61..45581f02ce 100644
--- a/salt/utils/verify.py
+++ b/salt/utils/verify.py
@@ -481,22 +481,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
 
 
diff --git a/tests/unit/utils/verify_test.py b/tests/unit/utils/verify_test.py
index 7e60f886d0..c3fa373290 100644
--- a/tests/unit/utils/verify_test.py
+++ b/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())
 
-- 
2.14.2

Places

File security-fixes-cve-2017-14695-and-cve-2017-14696.patch of Package salt (Revision 3a7c2d45577744db966838fdc07c2570)

Places