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@

7 files changed, 158 insertions, 74 deletions

diff --git a/usr.bin/ssh/clientloop.c b/usr.bin/ssh/clientloop.c
index 10aebd68d5e..7356ee2804e 100644
--- a/usr.bin/ssh/clientloop.c
+++ b/usr.bin/ssh/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
@@ -1490,7 +1490,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];
@@ -1605,10 +1605,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
@@ -1639,23 +1644,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/usr.bin/ssh/kex.c b/usr.bin/ssh/kex.c
index 86776420a76..1ee1e5d459d 100644
--- a/usr.bin/ssh/kex.c
+++ b/usr.bin/ssh/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.
  *
@@ -585,6 +585,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/usr.bin/ssh/kex.h b/usr.bin/ssh/kex.h
index b179d06dbfd..88a9084c1ec 100644
--- a/usr.bin/ssh/kex.h
+++ b/usr.bin/ssh/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.
@@ -165,6 +165,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/usr.bin/ssh/opacket.h b/usr.bin/ssh/opacket.h
index b90af80ce2a..9b82d32fc9d 100644
--- a/usr.bin/ssh/opacket.h
+++ b/usr.bin/ssh/opacket.h
@@ -124,8 +124,6 @@ void	 packet_read_expect(int expected_type);
 	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/usr.bin/ssh/packet.c b/usr.bin/ssh/packet.c
index 427e82ed354..288156b9a5a 100644
--- a/usr.bin/ssh/packet.c
+++ b/usr.bin/ssh/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
@@ -253,6 +253,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.
@@ -1018,6 +1026,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,
@@ -1221,35 +1274,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 */
@@ -1265,10 +1341,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;
@@ -1772,6 +1860,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;
 }
@@ -2248,25 +2343,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/usr.bin/ssh/packet.h b/usr.bin/ssh/packet.h
index 9f8d1380b12..e9887d0f5fc 100644
--- a/usr.bin/ssh/packet.h
+++ b/usr.bin/ssh/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>
@@ -72,6 +72,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);
@@ -131,7 +132,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/usr.bin/ssh/serverloop.c b/usr.bin/ssh/serverloop.c
index bd1de6e6022..19f67ccf858 100644
--- a/usr.bin/ssh/serverloop.c
+++ b/usr.bin/ssh/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
@@ -788,7 +788,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;
 
@@ -815,11 +815,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;
@@ -834,14 +834,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;