upstream commit

refactor activation of rekeying

This makes automatic rekeying internal to the packet code (previously
the server and client loops needed to assist). In doing to it makes
application of rekey limits more accurate by accounting for packets
about to be sent as well as packets queued during rekeying events
themselves.

Based on a patch from dtucker@ which was in turn based on a patch
Aleksander Adamowski in bz#2521; ok markus@

Upstream-ID: a441227fd64f9739850ca97b4cf794202860fcd8

7 files changed, 158 insertions, 74 deletions

diff --git a/clientloop.c b/clientloop.c
index f0a08f23..9820455c 100644
--- a/clientloop.c
+++ b/clientloop.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: clientloop.c,v 1.283 2016/02/01 21:18:17 millert Exp $ */
+/* $OpenBSD: clientloop.c,v 1.284 2016/02/08 10:57:07 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1502,7 +1502,7 @@ client_loop(int have_pty, int escape_char_arg, int ssh2_chan_id)
 {
 	fd_set *readset = NULL, *writeset = NULL;
 	double start_time, total_time;
-	int r, max_fd = 0, max_fd2 = 0, len, rekeying = 0;
+	int r, max_fd = 0, max_fd2 = 0, len;
 	u_int64_t ibytes, obytes;
 	u_int nalloc = 0;
 	char buf[100];
@@ -1617,10 +1617,15 @@ client_loop(int have_pty, int escape_char_arg, int ssh2_chan_id)
 		if (compat20 && session_closed && !channel_still_open())
 			break;
 
-		rekeying = (active_state->kex != NULL && !active_state->kex->done);
-
-		if (rekeying) {
+		if (ssh_packet_is_rekeying(active_state)) {
 			debug("rekeying in progress");
+		} else if (need_rekeying) {
+			/* manual rekey request */
+			debug("need rekeying");
+			if ((r = kex_start_rekex(active_state)) != 0)
+				fatal("%s: kex_start_rekex: %s", __func__,
+				    ssh_err(r));
+			need_rekeying = 0;
 		} else {
 			/*
 			 * Make packets of buffered stdin data, and buffer
@@ -1651,23 +1656,14 @@ client_loop(int have_pty, int escape_char_arg, int ssh2_chan_id)
 		 */
 		max_fd2 = max_fd;
 		client_wait_until_can_do_something(&readset, &writeset,
-		    &max_fd2, &nalloc, rekeying);
+		    &max_fd2, &nalloc, ssh_packet_is_rekeying(active_state));
 
 		if (quit_pending)
 			break;
 
 		/* Do channel operations unless rekeying in progress. */
-		if (!rekeying) {
+		if (!ssh_packet_is_rekeying(active_state))
 			channel_after_select(readset, writeset);
-			if (need_rekeying || packet_need_rekeying()) {
-				debug("need rekeying");
-				active_state->kex->done = 0;
-				if ((r = kex_send_kexinit(active_state)) != 0)
-					fatal("%s: kex_send_kexinit: %s",
-					    __func__, ssh_err(r));
-				need_rekeying = 0;
-			}
-		}
 
 		/* Buffer input from the connection.  */
 		client_process_net_input(readset);
diff --git a/kex.c b/kex.c
index 335b789f..d371f47c 100644
--- a/kex.c
+++ b/kex.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: kex.c,v 1.116 2016/01/14 16:17:39 markus Exp $ */
+/* $OpenBSD: kex.c,v 1.117 2016/02/08 10:57:07 djm Exp $ */
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
  *
@@ -606,6 +606,25 @@ kex_setup(struct ssh *ssh, char *proposal[PROPOSAL_MAX])
 	return 0;
 }
 
+/*
+ * Request key re-exchange, returns 0 on success or a ssherr.h error
+ * code otherwise. Must not be called if KEX is incomplete or in-progress.
+ */
+int
+kex_start_rekex(struct ssh *ssh)
+{
+	if (ssh->kex == NULL) {
+		error("%s: no kex", __func__);
+		return SSH_ERR_INTERNAL_ERROR;
+	}
+	if (ssh->kex->done == 0) {
+		error("%s: requested twice", __func__);
+		return SSH_ERR_INTERNAL_ERROR;
+	}
+	ssh->kex->done = 0;
+	return kex_send_kexinit(ssh);
+}
+
 static int
 choose_enc(struct sshenc *enc, char *client, char *server)
 {
diff --git a/kex.h b/kex.h
index 24d4aa15..1c589660 100644
--- a/kex.h
+++ b/kex.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: kex.h,v 1.75 2016/01/14 16:17:39 markus Exp $ */
+/* $OpenBSD: kex.h,v 1.76 2016/02/08 10:57:07 djm Exp $ */
 
 /*
  * Copyright (c) 2000, 2001 Markus Friedl.  All rights reserved.
@@ -179,6 +179,7 @@ int	 kex_input_ext_info(int, u_int32_t, void *);
 int	 kex_derive_keys(struct ssh *, u_char *, u_int, const struct sshbuf *);
 int	 kex_derive_keys_bn(struct ssh *, u_char *, u_int, const BIGNUM *);
 int	 kex_send_newkeys(struct ssh *);
+int	 kex_start_rekex(struct ssh *);
 
 int	 kexdh_client(struct ssh *);
 int	 kexdh_server(struct ssh *);
diff --git a/opacket.h b/opacket.h
index b14b6769..c26ade44 100644
--- a/opacket.h
+++ b/opacket.h
@@ -125,8 +125,6 @@ void	packet_disconnect(const char *, ...)
 	sshpkt_add_padding(active_state, (pad))
 #define packet_send_ignore(nbytes) \
 	ssh_packet_send_ignore(active_state, (nbytes))
-#define packet_need_rekeying() \
-	ssh_packet_need_rekeying(active_state)
 #define packet_set_server() \
 	ssh_packet_set_server(active_state)
 #define packet_set_authenticated() \
diff --git a/packet.c b/packet.c
index f7e5bc78..7ddebeb7 100644
--- a/packet.c
+++ b/packet.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: packet.c,v 1.227 2016/02/04 23:43:48 djm Exp $ */
+/* $OpenBSD: packet.c,v 1.228 2016/02/08 10:57:07 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -259,6 +259,14 @@ ssh_alloc_session_state(void)
 	return NULL;
 }
 
+/* Returns nonzero if rekeying is in progress */
+int
+ssh_packet_is_rekeying(struct ssh *ssh)
+{
+	return ssh->state->rekeying ||
+	    (ssh->kex != NULL && ssh->kex->done == 0);
+}
+
 /*
  * Sets the descriptors used for communication.  Disables encryption until
  * packet_set_encryption_key is called.
@@ -1029,6 +1037,51 @@ ssh_set_newkeys(struct ssh *ssh, int mode)
 	return 0;
 }
 
+#define MAX_PACKETS	(1U<<31)
+static int
+ssh_packet_need_rekeying(struct ssh *ssh, u_int outbound_packet_len)
+{
+	struct session_state *state = ssh->state;
+	u_int32_t out_blocks;
+
+	/* XXX client can't cope with rekeying pre-auth */
+	if (!state->after_authentication)
+		return 0;
+
+	/* Haven't keyed yet or KEX in progress. */
+	if (ssh->kex == NULL || ssh_packet_is_rekeying(ssh))
+		return 0;
+
+	/* Peer can't rekey */
+	if (ssh->compat & SSH_BUG_NOREKEY)
+		return 0;
+
+	/*
+	 * Permit one packet in or out per rekey - this allows us to
+	 * make progress when rekey limits are very small.
+	 */
+	if (state->p_send.packets == 0 && state->p_read.packets == 0)
+		return 0;
+
+	/* Time-based rekeying */
+	if (state->rekey_interval != 0 &&
+	    state->rekey_time + state->rekey_interval <= monotime())
+		return 1;
+
+	/* Always rekey when MAX_PACKETS sent in either direction */
+	if (state->p_send.packets > MAX_PACKETS ||
+	    state->p_read.packets > MAX_PACKETS)
+		return 1;
+
+	/* Rekey after (cipher-specific) maxiumum blocks */
+	out_blocks = roundup(outbound_packet_len,
+	    state->newkeys[MODE_OUT]->enc.block_size);
+	return (state->max_blocks_out &&
+	    (state->p_send.blocks + out_blocks > state->max_blocks_out)) ||
+	    (state->max_blocks_in &&
+	    (state->p_read.blocks > state->max_blocks_in));
+}
+
 /*
  * Delayed compression for SSH2 is enabled after authentication:
  * This happens on the server side after a SSH2_MSG_USERAUTH_SUCCESS is sent,
@@ -1232,35 +1285,58 @@ ssh_packet_send2_wrapped(struct ssh *ssh)
 	return r;
 }
 
+/* returns non-zero if the specified packet type is usec by KEX */
+static int
+ssh_packet_type_is_kex(u_char type)
+{
+	return
+	    type >= SSH2_MSG_TRANSPORT_MIN &&
+	    type <= SSH2_MSG_TRANSPORT_MAX &&
+	    type != SSH2_MSG_SERVICE_REQUEST &&
+	    type != SSH2_MSG_SERVICE_ACCEPT &&
+	    type != SSH2_MSG_EXT_INFO;
+}
+
 int
 ssh_packet_send2(struct ssh *ssh)
 {
 	struct session_state *state = ssh->state;
 	struct packet *p;
 	u_char type;
-	int r;
+	int r, need_rekey;
 
+	if (sshbuf_len(state->outgoing_packet) < 6)
+		return SSH_ERR_INTERNAL_ERROR;
 	type = sshbuf_ptr(state->outgoing_packet)[5];
+	need_rekey = !ssh_packet_type_is_kex(type) &&
+	    ssh_packet_need_rekeying(ssh, sshbuf_len(state->outgoing_packet));
 
-	/* during rekeying we can only send key exchange messages */
-	if (state->rekeying) {
-		if ((type < SSH2_MSG_TRANSPORT_MIN) ||
-		    (type > SSH2_MSG_TRANSPORT_MAX) ||
-		    (type == SSH2_MSG_SERVICE_REQUEST) ||
-		    (type == SSH2_MSG_SERVICE_ACCEPT) ||
-		    (type == SSH2_MSG_EXT_INFO)) {
-			debug("enqueue packet: %u", type);
-			p = calloc(1, sizeof(*p));
-			if (p == NULL)
-				return SSH_ERR_ALLOC_FAIL;
-			p->type = type;
-			p->payload = state->outgoing_packet;
-			TAILQ_INSERT_TAIL(&state->outgoing, p, next);
-			state->outgoing_packet = sshbuf_new();
-			if (state->outgoing_packet == NULL)
-				return SSH_ERR_ALLOC_FAIL;
-			return 0;
+	/*
+	 * During rekeying we can only send key exchange messages.
+	 * Queue everything else.
+	 */
+	if ((need_rekey || state->rekeying) && !ssh_packet_type_is_kex(type)) {
+		if (need_rekey)
+			debug3("%s: rekex triggered", __func__);
+		debug("enqueue packet: %u", type);
+		p = calloc(1, sizeof(*p));
+		if (p == NULL)
+			return SSH_ERR_ALLOC_FAIL;
+		p->type = type;
+		p->payload = state->outgoing_packet;
+		TAILQ_INSERT_TAIL(&state->outgoing, p, next);
+		state->outgoing_packet = sshbuf_new();
+		if (state->outgoing_packet == NULL)
+			return SSH_ERR_ALLOC_FAIL;
+		if (need_rekey) {
+			/*
+			 * This packet triggered a rekey, so send the
+			 * KEXINIT now.
+			 * NB. reenters this function via kex_start_rekex().
+			 */
+			return kex_start_rekex(ssh);
 		}
+		return 0;
 	}
 
 	/* rekeying starts with sending KEXINIT */
@@ -1276,10 +1352,22 @@ ssh_packet_send2(struct ssh *ssh)
 		state->rekey_time = monotime();
 		while ((p = TAILQ_FIRST(&state->outgoing))) {
 			type = p->type;
+			/*
+			 * If this packet triggers a rekex, then skip the
+			 * remaining packets in the queue for now.
+			 * NB. re-enters this function via kex_start_rekex.
+			 */
+			if (ssh_packet_need_rekeying(ssh,
+			    sshbuf_len(p->payload))) {
+				debug3("%s: queued packet triggered rekex",
+				    __func__);
+				return kex_start_rekex(ssh);
+			}
 			debug("dequeue packet: %u", type);
 			sshbuf_free(state->outgoing_packet);
 			state->outgoing_packet = p->payload;
 			TAILQ_REMOVE(&state->outgoing, p, next);
+			memset(p, 0, sizeof(*p));
 			free(p);
 			if ((r = ssh_packet_send2_wrapped(ssh)) != 0)
 				return r;
@@ -1784,6 +1872,13 @@ ssh_packet_read_poll2(struct ssh *ssh, u_char *typep, u_int32_t *seqnr_p)
 #endif
 	/* reset for next packet */
 	state->packlen = 0;
+
+	/* do we need to rekey? */
+	if (ssh_packet_need_rekeying(ssh, 0)) {
+		debug3("%s: rekex triggered", __func__);
+		if ((r = kex_start_rekex(ssh)) != 0)
+			return r;
+	}
  out:
 	return r;
 }
@@ -2268,25 +2363,6 @@ ssh_packet_send_ignore(struct ssh *ssh, int nbytes)
 	}
 }
 
-#define MAX_PACKETS	(1U<<31)
-int
-ssh_packet_need_rekeying(struct ssh *ssh)
-{
-	struct session_state *state = ssh->state;
-
-	if (ssh->compat & SSH_BUG_NOREKEY)
-		return 0;
-	return
-	    (state->p_send.packets > MAX_PACKETS) ||
-	    (state->p_read.packets > MAX_PACKETS) ||
-	    (state->max_blocks_out &&
-	        (state->p_send.blocks > state->max_blocks_out)) ||
-	    (state->max_blocks_in &&
-	        (state->p_read.blocks > state->max_blocks_in)) ||
-	    (state->rekey_interval != 0 && state->rekey_time +
-		 state->rekey_interval <= monotime());
-}
-
 void
 ssh_packet_set_rekey_limits(struct ssh *ssh, u_int64_t bytes, time_t seconds)
 {
diff --git a/packet.h b/packet.h
index 62302747..28516a55 100644
--- a/packet.h
+++ b/packet.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: packet.h,v 1.69 2016/01/29 02:54:45 dtucker Exp $ */
+/* $OpenBSD: packet.h,v 1.70 2016/02/08 10:57:07 djm Exp $ */
 
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
@@ -86,6 +86,7 @@ int      ssh_packet_get_connection_in(struct ssh *);
 int      ssh_packet_get_connection_out(struct ssh *);
 void     ssh_packet_close(struct ssh *);
 void	 ssh_packet_set_encryption_key(struct ssh *, const u_char *, u_int, int);
+int	 ssh_packet_is_rekeying(struct ssh *);
 void     ssh_packet_set_protocol_flags(struct ssh *, u_int);
 u_int	 ssh_packet_get_protocol_flags(struct ssh *);
 int      ssh_packet_start_compression(struct ssh *, int);
@@ -145,7 +146,6 @@ int	 ssh_packet_set_state(struct ssh *, struct sshbuf *);
 const char *ssh_remote_ipaddr(struct ssh *);
 int	 ssh_remote_port(struct ssh *);
 
-int	 ssh_packet_need_rekeying(struct ssh *);
 void	 ssh_packet_set_rekey_limits(struct ssh *, u_int64_t, time_t);
 time_t	 ssh_packet_get_rekey_timeout(struct ssh *);
 
diff --git a/serverloop.c b/serverloop.c
index 47bc168b..80d1db54 100644
--- a/serverloop.c
+++ b/serverloop.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: serverloop.c,v 1.181 2016/01/14 16:17:40 markus Exp $ */
+/* $OpenBSD: serverloop.c,v 1.182 2016/02/08 10:57:07 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -820,7 +820,7 @@ void
 server_loop2(Authctxt *authctxt)
 {
 	fd_set *readset = NULL, *writeset = NULL;
-	int rekeying = 0, max_fd;
+	int max_fd;
 	u_int nalloc = 0;
 	u_int64_t rekey_timeout_ms = 0;
 
@@ -847,11 +847,11 @@ server_loop2(Authctxt *authctxt)
 	for (;;) {
 		process_buffered_input_packets();
 
-		rekeying = (active_state->kex != NULL && !active_state->kex->done);
-
-		if (!rekeying && packet_not_very_much_data_to_write())
+		if (!ssh_packet_is_rekeying(active_state) &&
+		    packet_not_very_much_data_to_write())
 			channel_output_poll();
-		if (options.rekey_interval > 0 && compat20 && !rekeying)
+		if (options.rekey_interval > 0 && compat20 &&
+		    !ssh_packet_is_rekeying(active_state))
 			rekey_timeout_ms = packet_get_rekey_timeout() * 1000;
 		else
 			rekey_timeout_ms = 0;
@@ -866,14 +866,8 @@ server_loop2(Authctxt *authctxt)
 		}
 
 		collect_children();
-		if (!rekeying) {
+		if (!ssh_packet_is_rekeying(active_state))
 			channel_after_select(readset, writeset);
-			if (packet_need_rekeying()) {
-				debug("need rekeying");
-				active_state->kex->done = 0;
-				kex_send_kexinit(active_state);
-			}
-		}
 		process_input(readset);
 		if (connection_closed)
 			break;