Support OpenSSH delayed compression without a rekey.

The problem with OpenSSH delayed compression is that the spec has a
race condition. Compression is enabled when the server sends
USERAUTH_SUCCESS. In the server->client direction, that's fine: the
USERAUTH_SUCCESS packet is not itself compressed, and the next packet
in the same direction is. But in the client->server direction, this
specification relies on there being a moment of half-duplex in the
connection: the client can't send any outgoing packet _after_ whatever
userauth packet the USERAUTH_SUCCESS was a response to, and _before_
finding out whether the response is USERAUTH_SUCCESS or something
else. If it emitted, say, an SSH_MSG_IGNORE or initiated a rekey
(perhaps due to a timeout), then that might cross in the network with
USERAUTH_SUCCESS and the server wouldn't be able to know whether to
treat it as compressed.

My previous solution was to note the presence of delayed compression
options in the server KEXINIT, but not to negotiate them in the
initial key exchange. Instead, we conduct the userauth exchange with
compression="none", and then once userauth has concluded, we trigger
an immediate rekey in which we do accept delayed compression methods -
because of course by that time they're no different from the non-
delayed versions. And that means compression is enabled by the
bidirectional NEWKEYS exchange, which lacks that race condition.

I think OpenSSH itself gets away with this because its layer structure
is structure so as to never send any such asynchronous transport-layer
message in the middle of userauth. Ours is not. But my cunning plan is
that now that my BPP abstraction includes a queue of packets to be
sent and a callback that processes that queue on to the output raw
data bufchain, it's possible to make that callback terminate early, to
leave any dangerous transport-layer messages unsent while we wait for
a userauth response.

Specifically: if we've negotiated a delayed compression method and not
yet seen USERAUTH_SUCCESS, then ssh2_bpp_handle_output will emit all
packets from its queue up to and including the last one in the
userauth type-code range, and keep back any further ones. The idea is
that _if_ that last userauth message was one that might provoke
USERAUTH_SUCCESS, we don't want to send any difficult things after it;
if it's not (e.g. it's in the middle of some ongoing userauth process
like k-i or GSS) then the userauth layer will know that, and will emit
some further userauth packet on its own initiative which will clue us
in that it's OK to release everything up to and including that one.

(So in particular it wasn't even necessary to forbid _all_ transport-
layer packets during userauth. I could have done that by reordering
the output queue - packets in that queue haven't been assigned their
sequence numbers yet, so that would have been safe - but it's more
elegant not to have to.)

One particular case we do have to be careful about is not trying to
initiate a _rekey_ during userauth, if delayed compression is in the
offing. That's because when we start rekeying, ssh2transport stops
sending any higher-layer packets at all, to discourage servers from
trying to ignore the KEXINIT and press on regardless - you don't get
your higher-layer replies until you actually respond to the
lower-layer interrupt. But in this case, if ssh2transport sent a
KEXINIT, which ssh2bpp kept back in the queue to avoid a delayed
compression race and would only send if another userauth packet
followed it, which ssh2transport would never pass on to ssh2bpp's
output queue, there'd be a complete protocol deadlock. So instead I
defer any attempt to start a rekey until after userauth finishes
(using the existing system for starting a deferred rekey at that
moment, which was previously used for the _old_ delayed-compression
strategy, and still has to be here anyway for GSSAPI purposes).

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;
+        }
     }
 }

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);

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);