Disconnect outranks a stuck rekey

wolfSSH_stream_peek() and wolfSSH_stream_read() test isKeying before
disconnected, so a peer that rekeys then disconnects wedges both: only
NEWKEYS clears isKeying and none is coming. Callers spin on WS_REKEYING
and never get the buffered data. Both gates and read's copy step now
defer to disconnected. Covered by TestDisconnectOutranksRekey.

Issue: F-8837

Author: ejohnstown

src/ssh.c

@@ -1257,7 +1257,10 @@ int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz)
         return WS_BAD_ARGUMENT;
     }
 
-    if (ssh->isKeying) {
+    /* A rekey the peer abandoned with a disconnect never completes, since
+     * only NEWKEYS clears the flag. Report the dead session instead, or the
+     * caller turns the crank forever. */
+    if (ssh->isKeying && !ssh->disconnected) {
         ssh->error = WS_REKEYING;
         return WS_REKEYING;
     }
@@ -1324,7 +1327,9 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz)
         return WS_ERROR;
     }
 
-    if (ssh->isKeying) {
+    /* See wolfSSH_stream_peek(): a disconnect ends a rekey that can no
+     * longer finish, so it outranks it here too. */
+    if (ssh->isKeying && !ssh->disconnected) {
         ssh->error = WS_REKEYING;
         return WS_FATAL_ERROR;
     }
@@ -1374,8 +1379,11 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz)
         }
     }
 
-    /* update internal input buffer based on data read */
-    if (ret == WS_SUCCESS && !ssh->isKeying) {
+    /* update internal input buffer based on data read. DoReceive() above may
+     * have started a rekey, which holds the copy back -- unless a disconnect
+     * came with it, since then the rekey never finishes and the buffered data
+     * would never be handed back. */
+    if (ret == WS_SUCCESS && (!ssh->isKeying || ssh->disconnected)) {
         int n;
 
         n = min(bufSz, inputBuffer->length - inputBuffer->idx);

tests/regress.c

@@ -3734,6 +3734,80 @@ static void TestShutdownFlushesQueuedDisconnect(void)
 }
 
 
+/* A rekey that the peer abandons with a DISCONNECT never completes: only
+ * NEWKEYS clears isKeying, and nothing more arrives. The read calls test
+ * isKeying first, so they report WS_REKEYING forever and the caller's
+ * "keep turning the crank" branch spins for the life of the connection.
+ * A dead session outranks a rekey that can no longer finish. */
+static void TestDisconnectOutranksRekey(void)
+{
+    WOLFSSH_CTX* ctx;
+    WOLFSSH* ssh;
+    WOLFSSH_CHANNEL* channel;
+    MemIo io;
+    byte in[128];
+    byte out[256];
+    byte payload[32];
+    byte data[64];
+    word32 inSz;
+    int ret;
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    AssertNotNull(ctx);
+
+    wolfSSH_SetIORecv(ctx, MemRecv);
+    wolfSSH_SetIOSend(ctx, MemSend);
+
+    ssh = wolfSSH_new(ctx);
+    AssertNotNull(ssh);
+    AddSessionChannel(ssh);
+    channel = ssh->channelList;
+    ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE;
+
+    /* Channel data that arrived before the rekey started. */
+    WMEMSET(payload, 'a', sizeof(payload));
+    AssertIntEQ(ChannelPutData(channel, payload, sizeof(payload)), WS_SUCCESS);
+
+    /* The peer's KEXINIT, the way DoKexInit records it. */
+    ssh->isKeying |= WOLFSSH_PEER_IS_KEYING;
+
+    /* DISCONNECT is a transport-generic message, so the rekey filter in
+     * IsMessageAllowed() lets it through. */
+    inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION,
+            in, sizeof(in));
+    MemIoInit(&io, in, inSz, out, sizeof(out));
+    wolfSSH_SetIOReadCtx(ssh, &io);
+    wolfSSH_SetIOWriteCtx(ssh, &io);
+
+    AssertIntEQ(DoReceive(ssh), WS_FATAL_ERROR);
+    AssertTrue(ssh->disconnected);
+    /* The rekey is stuck: no NEWKEYS is ever coming. */
+    AssertTrue(ssh->isKeying != 0);
+    io.outSz = 0;
+
+    /* What arrived before the disconnect is still the caller's. */
+    ret = wolfSSH_stream_peek(ssh, data, sizeof(data));
+    AssertIntEQ(ret, (int)sizeof(payload));
+    ret = wolfSSH_stream_read(ssh, data, sizeof(data));
+    AssertIntEQ(ret, (int)sizeof(payload));
+
+    /* Drained, so both report the disconnect instead of a rekey that will
+     * never finish. */
+    ret = wolfSSH_stream_peek(ssh, data, sizeof(data));
+    AssertIntEQ(ret, WS_FATAL_ERROR);
+    AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT);
+    ret = wolfSSH_stream_read(ssh, data, sizeof(data));
+    AssertIntEQ(ret, WS_FATAL_ERROR);
+    AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT);
+
+    /* The drain stayed quiet, as it does outside a rekey. */
+    AssertIntEQ(io.outSz, 0);
+
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+}
+
+
 /* A flush that is itself short owns ssh->error. Callers gate their retry on
  * WS_WANT_WRITE, so the disconnect gate must not overwrite it. */
 static void TestShutdownKeepsFlushWantWrite(void)
@@ -7567,6 +7641,7 @@ int main(int argc, char** argv)
     TestQueuedDisconnectFlushes();
     TestShutdownFlushesQueuedDisconnect();
     TestShutdownKeepsFlushWantWrite();
+    TestDisconnectOutranksRekey();
 #if defined(WOLFSSH_TERM) && !defined(NO_FILESYSTEM)
     TestTerminalResizeBlockedAfterDisconnect();
 #endif