diff --git a/ssh2bpp.c b/ssh2bpp.c index 67745278..681c7d44 100644 --- a/ssh2bpp.c +++ b/ssh2bpp.c @@ -14,6 +14,7 @@ struct ssh2_bpp_direction { ssh2_cipher *cipher; ssh2_mac *mac; int etm_mode; + const struct ssh_compression_alg *pending_compression; }; struct ssh2_bpp_state { @@ -34,6 +35,7 @@ struct ssh2_bpp_state { ssh_compressor *out_comp; int pending_newkeys; + int pending_compression, seen_userauth_success; BinaryPacketProtocol bpp; }; @@ -90,7 +92,7 @@ void ssh2_bpp_new_outgoing_crypto( BinaryPacketProtocol *bpp, const struct ssh2_cipheralg *cipher, const void *ckey, const void *iv, const struct ssh2_macalg *mac, int etm_mode, const void *mac_key, - const struct ssh_compression_alg *compression) + const struct ssh_compression_alg *compression, int delayed_compression) { struct ssh2_bpp_state *s; assert(bpp->vt == &ssh2_bpp_vtable); @@ -134,20 +136,31 @@ void ssh2_bpp_new_outgoing_crypto( s->out.mac = NULL; } - /* 'compression' is always non-NULL, because no compression is - * indicated by ssh_comp_none. But this setup call may return a - * null out_comp. */ - s->out_comp = ssh_compressor_new(compression); - if (s->out_comp) - bpp_logevent(("Initialised %s compression", - ssh_compressor_alg(s->out_comp)->text_name)); + if (delayed_compression && !s->seen_userauth_success) { + s->out.pending_compression = compression; + s->out_comp = NULL; + + bpp_logevent(("Will enable %s compression after user authentication", + s->out.pending_compression->text_name)); + } else { + s->out.pending_compression = NULL; + + /* 'compression' is always non-NULL, because no compression is + * indicated by ssh_comp_none. But this setup call may return a + * null out_comp. */ + s->out_comp = ssh_compressor_new(compression); + + if (s->out_comp) + bpp_logevent(("Initialised %s compression", + ssh_compressor_alg(s->out_comp)->text_name)); + } } void ssh2_bpp_new_incoming_crypto( BinaryPacketProtocol *bpp, const struct ssh2_cipheralg *cipher, const void *ckey, const void *iv, const struct ssh2_macalg *mac, int etm_mode, const void *mac_key, - const struct ssh_compression_alg *compression) + const struct ssh_compression_alg *compression, int delayed_compression) { struct ssh2_bpp_state *s; assert(bpp->vt == &ssh2_bpp_vtable); @@ -185,19 +198,39 @@ void ssh2_bpp_new_incoming_crypto( s->in.mac = NULL; } - /* 'compression' is always non-NULL, because no compression is - * indicated by ssh_comp_none. But this setup call may return a - * null in_decomp. */ - s->in_decomp = ssh_decompressor_new(compression); - if (s->in_decomp) - bpp_logevent(("Initialised %s decompression", - ssh_decompressor_alg(s->in_decomp)->text_name)); + if (delayed_compression && !s->seen_userauth_success) { + s->in.pending_compression = compression; + s->in_decomp = NULL; + + bpp_logevent(("Will enable %s decompression after user authentication", + s->in.pending_compression->text_name)); + } else { + s->in.pending_compression = NULL; + + /* 'compression' is always non-NULL, because no compression is + * indicated by ssh_comp_none. But this setup call may return a + * null in_decomp. */ + s->in_decomp = ssh_decompressor_new(compression); + + if (s->in_decomp) + bpp_logevent(("Initialised %s decompression", + ssh_decompressor_alg(s->in_decomp)->text_name)); + } /* Clear the pending_newkeys flag, so that handle_input below will * start consuming the input data again. */ s->pending_newkeys = FALSE; } +int ssh2_bpp_rekey_inadvisable(BinaryPacketProtocol *bpp) +{ + struct ssh2_bpp_state *s; + assert(bpp->vt == &ssh2_bpp_vtable); + s = container_of(bpp, struct ssh2_bpp_state, bpp); + + return s->pending_compression; +} + #define BPP_READ(ptr, len) do \ { \ crMaybeWaitUntilV(s->bpp.input_eof || \ @@ -207,6 +240,8 @@ void ssh2_bpp_new_incoming_crypto( goto eof; \ } while (0) +#define userauth_range(pkttype) ((unsigned)((pkttype) - 50) < 20) + static void ssh2_bpp_handle_input(BinaryPacketProtocol *bpp) { struct ssh2_bpp_state *s = container_of(bpp, struct ssh2_bpp_state, bpp); @@ -537,6 +572,74 @@ static void ssh2_bpp_handle_input(BinaryPacketProtocol *bpp) */ s->pending_newkeys = TRUE; crWaitUntilV(!s->pending_newkeys); + continue; + } + + if (s->pending_compression && userauth_range(type)) { + /* + * Another one: if we were configured with OpenSSH's + * deferred compression which is triggered on receipt + * of USERAUTH_SUCCESS, and we're currently + * anticipating the next packet perhaps _being_ + * USERAUTH_SUCCESS, then we do some special handling. + */ + + if (type == SSH2_MSG_USERAUTH_SUCCESS) { + /* + * Success! This is the moment to turn on + * compression. + */ + s->pending_compression = FALSE; + s->in_decomp = + ssh_decompressor_new(s->in.pending_compression); + s->out_comp = + ssh_compressor_new(s->out.pending_compression); + s->in.pending_compression = NULL; + s->out.pending_compression = NULL; + + if (s->out_comp) + bpp_logevent(("Initialised delayed %s compression", + ssh_compressor_alg( + s->out_comp)->text_name)); + if (s->in_decomp) + bpp_logevent(("Initialised delayed %s decompression", + ssh_decompressor_alg( + s->in_decomp)->text_name)); + + /* + * Also, since we will have temporarily disabled + * output queue processing (for fear of having + * some asynchronous thing like an IGNORE message + * cross in transit with USERAUTH_SUCCESS coming + * the other way, leaving its compresssion status + * in doubt), we should schedule a run of the + * output queue now, to release any pending + * packets. + */ + queue_idempotent_callback(&s->bpp.ic_out_pq); + } else { + /* + * This message indicates that we're not about to + * see USERAUTH_SUCCESS (i.e. turn on compression) + * just yet, so we turn off the outgoing packet + * blockage and release any queued output packets, + * so that we can make another attempt to + * authenticate. + */ + s->pending_compression = FALSE; + queue_idempotent_callback(&s->bpp.ic_out_pq); + } + } + + if (type == SSH2_MSG_USERAUTH_SUCCESS) { + /* + * Whether or not we were doing delayed compression in + * _this_ set of crypto parameters, we should set a + * flag indicating that we're now authenticated, so + * that a delayed compression method enabled in any + * future rekey will be treated as un-delayed. + */ + s->seen_userauth_success = TRUE; } } } @@ -734,6 +837,31 @@ static void ssh2_bpp_handle_output(BinaryPacketProtocol *bpp) { struct ssh2_bpp_state *s = container_of(bpp, struct ssh2_bpp_state, bpp); PktOut *pkt; + int n_userauth; + + /* + * Count the userauth packets in the queue. + */ + n_userauth = 0; + for (pkt = pq_first(&s->bpp.out_pq); pkt != NULL; + pkt = pq_next(&s->bpp.out_pq, pkt)) + if (userauth_range(pkt->type)) + n_userauth++; + + if (s->pending_compression && !n_userauth) { + /* + * We're currently blocked from sending any outgoing packets + * until the other end tells us whether we're going to have to + * enable compression or not. + * + * If our end has pushed a userauth packet on the queue, that + * must mean it knows that a USERAUTH_SUCCESS is not + * immediately forthcoming, so we unblock ourselves and send + * up to and including that packet. But in this if statement, + * there aren't any, so we're still blocked. + */ + return; + } if (s->cbc_ignore_workaround) { /* @@ -761,7 +889,23 @@ static void ssh2_bpp_handle_output(BinaryPacketProtocol *bpp) } while ((pkt = pq_pop(&s->bpp.out_pq)) != NULL) { + if (userauth_range(pkt->type)) + n_userauth--; + ssh2_bpp_format_packet(s, pkt); ssh_free_pktout(pkt); + + if (n_userauth == 0 && + (s->out.pending_compression || s->in.pending_compression)) { + /* + * This is the last userauth packet in the queue, so + * unless our side decides to send another one in future, + * we have to assume will potentially provoke + * USERAUTH_SUCCESS. Block (non-userauth) outgoing packets + * until we see the reply. + */ + s->pending_compression = TRUE; + return; + } } } diff --git a/ssh2transport.c b/ssh2transport.c index 60626403..a058f752 100644 --- a/ssh2transport.c +++ b/ssh2transport.c @@ -49,7 +49,10 @@ struct kexinit_algorithm { const struct ssh2_macalg *mac; int etm; } mac; - const struct ssh_compression_alg *comp; + struct { + const struct ssh_compression_alg *comp; + int delayed; + } comp; } u; }; @@ -206,6 +209,7 @@ struct ssh2_transport_state { const struct ssh2_macalg *mac; int etm_mode; const struct ssh_compression_alg *comp; + int comp_delayed; } in, out; ptrlen hostkeydata, sigdata; char *keystr, *fingerprint; @@ -223,8 +227,6 @@ struct ssh2_transport_state { int n_preferred_ciphers; const struct ssh2_ciphers *preferred_ciphers[CIPHER_MAX]; const struct ssh_compression_alg *preferred_comp; - int userauth_succeeded; /* for delayed compression */ - int pending_compression; int got_session_id; int dlgret; int guessok; @@ -614,8 +616,6 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) s->in.comp = s->out.comp = NULL; s->got_session_id = FALSE; - s->userauth_succeeded = FALSE; - s->pending_compression = FALSE; s->need_gss_transient_hostkey = FALSE; s->warned_about_no_gss_transient_hostkey = FALSE; @@ -935,22 +935,23 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) assert(lenof(compressions) > 1); /* Prefer non-delayed versions */ alg = ssh2_kexinit_addalg(s->kexlists[j], s->preferred_comp->name); - alg->u.comp = s->preferred_comp; - /* We don't even list delayed versions of algorithms until - * they're allowed to be used, to avoid a race. See the end of - * this function. */ - if (s->userauth_succeeded && s->preferred_comp->delayed_name) { + alg->u.comp.comp = s->preferred_comp; + alg->u.comp.delayed = FALSE; + if (s->preferred_comp->delayed_name) { alg = ssh2_kexinit_addalg(s->kexlists[j], s->preferred_comp->delayed_name); - alg->u.comp = s->preferred_comp; + alg->u.comp.comp = s->preferred_comp; + alg->u.comp.delayed = TRUE; } for (i = 0; i < lenof(compressions); i++) { const struct ssh_compression_alg *c = compressions[i]; alg = ssh2_kexinit_addalg(s->kexlists[j], c->name); - alg->u.comp = c; - if (s->userauth_succeeded && c->delayed_name) { + alg->u.comp.comp = c; + alg->u.comp.delayed = FALSE; + if (c->delayed_name) { alg = ssh2_kexinit_addalg(s->kexlists[j], c->delayed_name); - alg->u.comp = c; + alg->u.comp.comp = c; + alg->u.comp.delayed = TRUE; } } } @@ -1006,6 +1007,7 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) s->in.cipher = s->out.cipher = NULL; s->in.mac = s->out.mac = NULL; s->in.comp = s->out.comp = NULL; + s->in.comp_delayed = s->out.comp_delayed = FALSE; s->warn_kex = s->warn_hk = FALSE; s->warn_cscipher = s->warn_sccipher = FALSE; @@ -1076,21 +1078,14 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) s->in.mac = alg->u.mac.mac; s->in.etm_mode = alg->u.mac.etm; } else if (i == KEXLIST_CSCOMP) { - s->out.comp = alg->u.comp; + s->out.comp = alg->u.comp.comp; + s->out.comp_delayed = alg->u.comp.delayed; } else if (i == KEXLIST_SCCOMP) { - s->in.comp = alg->u.comp; + s->in.comp = alg->u.comp.comp; + s->in.comp_delayed = alg->u.comp.delayed; } goto matched; } - - /* Set a flag if there's a delayed compression option - * available for a compression method that we just - * failed to select the immediate version of. */ - s->pending_compression = ( - (i == KEXLIST_CSCOMP || i == KEXLIST_SCCOMP) && - in_commasep_string(alg->u.comp->delayed_name, - str.ptr, str.len) && - !s->userauth_succeeded); } ssh_sw_abort(s->ppl.ssh, "Couldn't agree a %s (available: %.*s)", kexlist_descr[i], PTRLEN_PRINTF(str)); @@ -1127,10 +1122,6 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) } } - if (s->pending_compression) { - ppl_logevent(("Server supports delayed compression; " - "will try this later")); - } get_string(pktin); /* client->server language */ get_string(pktin); /* server->client language */ s->ignorepkt = get_bool(pktin) && !s->guessok; @@ -2146,7 +2137,7 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) s->ppl.bpp, s->out.cipher, cipher_key->u, cipher_iv->u, s->out.mac, s->out.etm_mode, mac_key->u, - s->out.comp); + s->out.comp, s->out.comp_delayed); strbuf_free(cipher_key); strbuf_free(cipher_iv); @@ -2201,7 +2192,7 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) s->ppl.bpp, s->in.cipher, cipher_key->u, cipher_iv->u, s->in.mac, s->in.etm_mode, mac_key->u, - s->in.comp); + s->in.comp, s->in.comp_delayed); strbuf_free(cipher_key); strbuf_free(cipher_iv); @@ -2288,41 +2279,17 @@ static void ssh2_transport_process_queue(PacketProtocolLayer *ppl) if (s->rekey_class == RK_POST_USERAUTH) { /* - * userauth has seen a USERAUTH_SUCCEEDED. For a couple of - * reasons, this may be the moment to do an immediate - * rekey with different parameters. But it may not; so - * here we turn that rekey class into either RK_NONE or - * RK_NORMAL. + * userauth has seen a USERAUTH_SUCCESS. This may be the + * moment to do an immediate rekey with different + * parameters. But it may not; so here we turn that rekey + * class into either RK_NONE or RK_NORMAL. * - * One is to turn on delayed compression. We do this by a - * rekey to work around a protocol design bug: - * draft-miller-secsh-compression-delayed-00 says that you - * negotiate delayed compression in the first key - * exchange, and both sides start compressing when the - * server has sent USERAUTH_SUCCESS. This has a race - * condition -- the server can't know when the client has - * seen it, and thus which incoming packets it should - * treat as compressed. - * - * Instead, we do the initial key exchange without - * offering the delayed methods, but note if the server - * offers them; when we get here, if a delayed method was - * available that was higher on our list than what we got, - * we initiate a rekey in which we _do_ list the delayed - * methods (and hopefully get it as a result). Subsequent - * rekeys will do the same. - * - * Another reason for a rekey at this point is if we've - * done a GSS key exchange and don't have anything in our + * Currently the only reason for this is if we've done a + * GSS key exchange and don't have anything in our * transient hostkey cache, in which case we should make * an attempt to populate the cache now. */ - assert(!s->userauth_succeeded); /* should only happen once */ - s->userauth_succeeded = TRUE; - if (s->pending_compression) { - s->rekey_reason = "enabling delayed compression"; - s->rekey_class = RK_NORMAL; - } else if (s->need_gss_transient_hostkey) { + if (s->need_gss_transient_hostkey) { s->rekey_reason = "populating transient host key cache"; s->rekey_class = RK_NORMAL; } else { @@ -2908,7 +2875,7 @@ static void ssh2_transport_reconfigure(PacketProtocolLayer *ppl, Conf *conf) s->conf = conf_copy(conf); if (rekey_reason) { - if (!s->kex_in_progress) { + if (!s->kex_in_progress && !ssh2_bpp_rekey_inadvisable(s->ppl.bpp)) { s->rekey_reason = rekey_reason; s->rekey_class = RK_NORMAL; queue_idempotent_callback(&s->ppl.ic_process_queue); diff --git a/sshbpp.h b/sshbpp.h index 3914eeb3..e5de4dbc 100644 --- a/sshbpp.h +++ b/sshbpp.h @@ -103,12 +103,23 @@ void ssh2_bpp_new_outgoing_crypto( BinaryPacketProtocol *bpp, const struct ssh2_cipheralg *cipher, const void *ckey, const void *iv, const struct ssh2_macalg *mac, int etm_mode, const void *mac_key, - const struct ssh_compression_alg *compression); + const struct ssh_compression_alg *compression, int delayed_compression); void ssh2_bpp_new_incoming_crypto( BinaryPacketProtocol *bpp, const struct ssh2_cipheralg *cipher, const void *ckey, const void *iv, const struct ssh2_macalg *mac, int etm_mode, const void *mac_key, - const struct ssh_compression_alg *compression); + const struct ssh_compression_alg *compression, int delayed_compression); + +/* + * A query method specific to the interface between ssh2transport and + * ssh2bpp. If true, it indicates that we're potentially in the + * race-condition-prone part of delayed compression setup and so + * asynchronous outgoing transport-layer packets are currently not + * being sent, which means in particular that it would be a bad idea + * to start a rekey because then we'd stop responding to anything + * _other_ than transport-layer packets and deadlock the protocol. + */ +int ssh2_bpp_rekey_inadvisable(BinaryPacketProtocol *bpp); BinaryPacketProtocol *ssh2_bare_bpp_new(Frontend *frontend);