Attachment 2783 Details for Bug 2521 – refactor rekeying logic

[patch] refactor rekeying logic

bz2521.diff (text/plain), 11.73 KB, created by Damien Miller on 2016-01-29 17:06:21 AEDT

(hide)

Description:

Filename:

MIME Type:

Creator: Damien Miller

Created: 2016-01-29 17:06:21 AEDT

Size: 11.73 KB

patch

obsolete

>diff --git a/clientloop.c b/clientloop.c
>index 80395a1..35a839b 100644
>--- a/clientloop.c
>+++ b/clientloop.c
>@@ -1491,7 +1491,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];
>@@ -1606,10 +1606,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
>@@ -1640,23 +1645,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 8677642..06622a5 100644
>--- a/kex.c
>+++ b/kex.c
>@@ -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/kex.h b/kex.h
>index b179d06..374bc6a 100644
>--- a/kex.h
>+++ b/kex.h
>@@ -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/opacket.h b/opacket.h
>index b90af80..9b82d32 100644
>--- a/opacket.h
>+++ b/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/packet.c b/packet.c
>index a3b3aae..a15fde1 100644
>--- a/packet.c
>+++ b/packet.c
>@@ -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.
>@@ -1016,6 +1024,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_IN]->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,
>@@ -1219,35 +1272,59 @@ 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 */
>@@ -1263,10 +1340,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;
>@@ -1770,6 +1859,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;
> }
>@@ -2246,25 +2342,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 9f8d138..40661cd 100644
>--- a/packet.h
>+++ b/packet.h
>@@ -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/serverloop.c b/serverloop.c
>index bd1de6e..8ef7d2a 100644
>--- a/serverloop.c
>+++ b/serverloop.c
>@@ -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;

Actions: View | Diff

Attachments on bug 2521: 2778 | 2779 | 2780 | 2783