File security-fixes-cve-2017-14695-and-cve-2017-14696.patch of Package salt.6622
From e09f59e2863f5b5aa19b5ec08f4a9e128b3048b8 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.13.6
