From 344ec3aec5d8d32a1e6d6ccc8028c6951ebce5d1 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Sat, 22 Sep 2018 11:09:15 +0100 Subject: [PATCH] Restructure SSH-1 compression again. Having redesigned it a few days ago in commit 562cdd4df, I'm changing it again, this time to fix a potential race condition on the _output_ side: the last change was intended to cope with a server sending an asynchronous message like IGNORE immediately after enabling compression, and this one fixes the case in which _we_ happen to decide to send an IGNORE while a compression request is still pending. I couldn't fix this until after the BPP was reorganised to have an explicit output queue of packets, but now it does, I can simply defer processing that queue on to the output raw-data bufchain if we're waiting for a compression request to be answered. Once it is answered, the BPP can release any pending packets. --- ssh.c | 9 ++++++- ssh1bpp.c | 71 +++++++++++++++++++++++++++++++++++++++---------------- 2 files changed, 59 insertions(+), 21 deletions(-) diff --git a/ssh.c b/ssh.c index 605c8e8a..94dbb064 100644 --- a/ssh.c +++ b/ssh.c @@ -3125,7 +3125,6 @@ static void do_ssh1_connection(void *vctx) pkt = ssh_bpp_new_pktout(ssh->bpp, SSH1_CMSG_REQUEST_COMPRESSION); put_uint32(pkt, 6); /* gzip compression level */ ssh_pkt_write(ssh, pkt); - ssh1_bpp_requested_compression(ssh->bpp); crMaybeWaitUntilV((pktin = pq_pop(&ssh->pq_ssh1_connection)) != NULL); if (pktin->type != SSH1_SMSG_SUCCESS && pktin->type != SSH1_SMSG_FAILURE) { @@ -3134,6 +3133,14 @@ static void do_ssh1_connection(void *vctx) } else if (pktin->type == SSH1_SMSG_FAILURE) { c_write_str(ssh, "Server refused to compress\r\n"); } else { + /* + * We don't have to actually do anything here: the SSH-1 + * BPP will take care of automatically starting the + * compression, by recognising our outgoing request packet + * and the success response. (Horrible, but it's the + * easiest way to avoid race conditions if other packets + * cross in transit.) + */ logevent("Started zlib (RFC1950) compression"); } } diff --git a/ssh1bpp.c b/ssh1bpp.c index 36e03b5d..4b775929 100644 --- a/ssh1bpp.c +++ b/ssh1bpp.c @@ -86,15 +86,6 @@ void ssh1_bpp_new_cipher(BinaryPacketProtocol *bpp, } } -void ssh1_bpp_requested_compression(BinaryPacketProtocol *bpp) -{ - struct ssh1_bpp_state *s; - assert(bpp->vt == &ssh1_bpp_vtable); - s = FROMFIELD(bpp, struct ssh1_bpp_state, bpp); - - s->pending_compression_request = TRUE; -} - #define BPP_READ(ptr, len) do \ { \ crMaybeWaitUntilV(bufchain_try_fetch_consume( \ @@ -212,21 +203,39 @@ static void ssh1_bpp_handle_input(BinaryPacketProtocol *bpp) int type = s->pktin->type; s->pktin = NULL; - if (type == SSH1_MSG_DISCONNECT) + switch (type) { + case SSH1_MSG_DISCONNECT: s->bpp.seen_disconnect = TRUE; + break; - if (type == SSH1_SMSG_SUCCESS && s->pending_compression_request) { - assert(!s->compctx); - assert(!s->decompctx); + case SSH1_SMSG_SUCCESS: + case SSH1_SMSG_FAILURE: + if (s->pending_compression_request) { + /* + * This is the response to + * SSH1_CMSG_REQUEST_COMPRESSION. + */ + if (type == SSH1_SMSG_SUCCESS) { + /* + * If the response was positive, start + * compression. + */ + assert(!s->compctx); + assert(!s->decompctx); - s->compctx = ssh_compressor_new(&ssh_zlib); - s->decompctx = ssh_decompressor_new(&ssh_zlib); + s->compctx = ssh_compressor_new(&ssh_zlib); + s->decompctx = ssh_decompressor_new(&ssh_zlib); + } - s->pending_compression_request = FALSE; - } - - if (type == SSH1_SMSG_FAILURE && s->pending_compression_request) { - s->pending_compression_request = FALSE; + /* + * Either way, cancel the pending flag, and + * schedule a run of our output side in case we + * had any packets queued up in the meantime. + */ + s->pending_compression_request = FALSE; + queue_idempotent_callback(&s->bpp.ic_out_pq); + } + break; } } } @@ -300,9 +309,31 @@ static void ssh1_bpp_handle_output(BinaryPacketProtocol *bpp) struct ssh1_bpp_state *s = FROMFIELD(bpp, struct ssh1_bpp_state, bpp); PktOut *pkt; + if (s->pending_compression_request) { + /* + * Don't send any output packets while we're awaiting a + * response to SSH1_CMSG_REQUEST_COMPRESSION, because if they + * cross over in transit with the responding SSH1_CMSG_SUCCESS + * then the other end could decode them with the wrong + * compression settings. + */ + return; + } + while ((pkt = pq_pop(&s->bpp.out_pq)) != NULL) { + int type = pkt->type; ssh1_bpp_format_packet(s, pkt); ssh_free_pktout(pkt); + + if (type == SSH1_CMSG_REQUEST_COMPRESSION) { + /* + * When we see the actual compression request go past, set + * the pending flag, and stop processing packets this + * time. + */ + s->pending_compression_request = TRUE; + break; + } } }