Ori.livneh has uploaded a new change for review.
https://gerrit.wikimedia.org/r/234700
Change subject: harden ssh-agent-proxy
......................................................................
harden ssh-agent-proxy
Prior to this patch, ssh-agent-proxy only read the parts of each incoming
message that it thought it cared about: the request code, and (in the case of
signing requests) the public key blob. This is potentially insecure because a
malicious client could potentially trick the proxy into forwarding a request
that would otherwise get filtered by appending it to a legitimate request and
deliberately under-reporting the message length. (Similar to the Heartbleed
bug.) To protect against this, we make ssh-agent-proxy read and validate the
entire message and its length against the agent protocol spec before forwarding
it to the proxy.
Change-Id: Ia00ccafcd0ab8a6856be56e276616bceb0ae9d74
---
M modules/keyholder/files/ssh-agent-proxy
1 file changed, 41 insertions(+), 23 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/operations/puppet
refs/changes/00/234700/1
diff --git a/modules/keyholder/files/ssh-agent-proxy
b/modules/keyholder/files/ssh-agent-proxy
index df7b447..757a012 100644
--- a/modules/keyholder/files/ssh-agent-proxy
+++ b/modules/keyholder/files/ssh-agent-proxy
@@ -72,39 +72,47 @@
SSH2_AGENTC_SIGN_REQUEST = 13
SSH_AGENTC_REQUEST_RSA_IDENTITIES = 1
SSH_AGENT_FAILURE = 5
+SSH_AGENT_OLD_SIGNATURE = 1
s_message_header = struct.Struct('!LB')
+s_flags = struct.Struct('!L')
s_ucred = struct.Struct('2Ii')
+err = type('SshAgentProtocolError', (IOError,), {})
def unpack_variable_length_string(buffer, offset=0):
"""Read a variable-length string from a buffer. The first 4 bytes are the
big-endian unsigned long representing the length of the string."""
- size = struct.unpack_from('!L', buffer, offset)
- string, = struct.unpack_from('xxxx%ds' % size, buffer, offset)
- return string
+ fmt = 'xxxx%ds' % struct.unpack_from('!L', buffer, offset)
+ string, = struct.unpack_from(fmt, buffer, offset)
+ return string, offset + struct.calcsize(fmt)
-def get_key_permissions(path):
+def get_key_perms(path):
"""Recursively walk `path`, loading YAML configuration files."""
- key_permissions = {}
+ key_perms = {}
for fname in glob.glob(os.path.join(path, '*.y*ml')):
with open(fname) as yml:
for group, keys in yaml.safe_load(yml).items():
for key in keys:
key = key.replace(':', '')
- key_permissions.setdefault(key, set()).add(group)
- return key_permissions
+ key_perms.setdefault(key, set()).add(group)
+ return key_perms
class SshAgentProxyServer(socketserver.ThreadingUnixStreamServer):
"""A threaded server that listens on a UNIX domain socket and handles
requests by filtering them and proxying them to a backend SSH agent."""
- def __init__(self, server_address, agent_address, key_permissions):
+ def __init__(self, server_address, agent_address, key_perms):
super().__init__(server_address, SshAgentProxyHandler)
self.agent_address = agent_address
- self.key_permissions = key_permissions
+ self.key_perms = key_perms
+
+ def handle_error(self, request, client_address):
+ super().handle_error(request, client_address)
+ exc_type, exc_value, exc_traceback = sys.exc_info()
+ syslog.syslog(syslog.LOG_NOTICE, '[%s] %s' % (exc_type, exc_value))
class SshAgentProxyHandler(socketserver.BaseRequestHandler):
@@ -150,35 +158,45 @@
"""Read data from client and send to backend SSH agent."""
code, message = self.recv_message(self.request)
- if code == SSH2_AGENTC_REQUEST_IDENTITIES:
- return self.send_message(self.backend, code, message)
+ if not code:
+ return
- if code == SSH_AGENTC_REQUEST_RSA_IDENTITIES:
- return self.send_message(self.backend, code, message)
+ if code in (SSH2_AGENTC_REQUEST_IDENTITIES,
+ SSH_AGENTC_REQUEST_RSA_IDENTITIES):
+ if message:
+ raise err('Trailing bytes')
+ return self.send_message(self.backend, code)
if code == SSH2_AGENTC_SIGN_REQUEST:
- key = unpack_variable_length_string(message)
- digest = hashlib.md5(key).hexdigest()
+ key_blob, *_ = self.parse_sign_request(message)
+ key_digest = hashlib.md5(key_blob).hexdigest()
user, groups = self.get_peer_credentials(self.request)
- if groups & self.server.key_permissions.get(digest, set()):
- syslog.syslog(syslog.LOG_INFO, 'Allowing signing request '
- 'from user %s using key %s.' % (user, digest))
+ if groups & self.server.key_perms.get(key_digest, set()):
return self.send_message(self.backend, code, message)
- syslog.syslog(syslog.LOG_NOTICE, 'Denying signing request '
- 'from user %s using key %s.' % (user, digest))
return self.send_message(self.request, SSH_AGENT_FAILURE)
def handle(self):
"""Handle a new client connection by shuttling data between the client
and the backend."""
- syslog.syslog('New connection from %s.' % self.client_address)
while 1:
rlist, *_ = select.select((self.backend, self.request), (), (), 1)
if self.backend in rlist:
self.handle_backend()
if self.request in rlist:
self.handle_client_request()
+
+ def parse_sign_request(self, message):
+ """Parse the payload of an SSH2_AGENTC_SIGN_REQUEST into its
+ constituent parts: a key blob, data, and a uint32 flag."""
+ key_blob, offset = unpack_variable_length_string(message)
+ data, offset = unpack_variable_length_string(message, offset)
+ flags, = s_flags.unpack_from(message, offset)
+ if offset + s_flags.size != len(message):
+ raise err('SSH2_AGENTC_SIGN_REQUEST: Trailing bytes')
+ if flags != 0 and flags != SSH_AGENT_OLD_SIGNATURE:
+ raise err('SSH2_AGENTC_SIGN_REQUEST: Bad flags 0x%X' % flags)
+ return key_blob, data, flags
ap = argparse.ArgumentParser(description='filtering proxy for ssh-agent')
@@ -199,6 +217,6 @@
)
args = ap.parse_args()
-key_perms = get_key_permissions(args.auth_dir)
+perms = get_key_perms(args.auth_dir)
syslog.openlog(logoption=syslog.LOG_PID, facility=syslog.LOG_AUTH)
-SshAgentProxyServer(args.bind, args.connect, key_perms).serve_forever()
+SshAgentProxyServer(args.bind, args.connect, perms).serve_forever()
--
To view, visit https://gerrit.wikimedia.org/r/234700
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia00ccafcd0ab8a6856be56e276616bceb0ae9d74
Gerrit-PatchSet: 1
Gerrit-Project: operations/puppet
Gerrit-Branch: production
Gerrit-Owner: Ori.livneh <o...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
