ssh: flush any queued output at teardown

wolfSSH_shutdown() flushes whatever a short send left in the output
buffer, not just a queued disconnect. A rejected auth's
USERAUTH_FAILURE has no channel, and DoChannelClose() retires the
channel as soon as the close is bundled, so neither had anything
left to carry the retry.

- The gate still refuses a flush once the peer has disconnected,
  unless our own disconnect is the thing queued.
- ssh.h documents the widened flush and the WS_WANT_WRITE a short
  one leaves wolfSSH_shutdown() returning.
- Tests cover the rejected auth with no channel and the close whose
  channel DoChannelClose() already retired.
- A flush that short-sends on a live channel leaves the teardown
  queued behind it, and the retry adds no second EOF or close.

Author: ejohnstown

src/ssh.c

@@ -1149,17 +1149,20 @@ static int SendAfterDisconnect(WOLFSSH* ssh)
 }
 
 
-/* A disconnect of ours left queued by a short send still has to reach the
+/* Whatever a short send left in the output buffer still has to reach the
  * peer, and flushing bytes that are already bundled is not the new traffic
- * RFC 4253 section 11.1 forbids. Only our own disconnect qualifies: a
- * disconnect from the peer leaves nothing queued but unrelated traffic,
- * which the session is over for. Call only after a NULL check of ssh. */
-static int FlushQueuedDisconnect(WOLFSSH* ssh)
+ * RFC 4253 section 11.1 forbids. Once the peer has disconnected, though,
+ * only our own queued disconnect still qualifies: anything else in there
+ * belongs to a session that is over. Call only after a NULL check of ssh. */
+static int FlushQueuedOutput(WOLFSSH* ssh)
 {
-    if (!ssh->disconnectTxd || !wolfSSH_OutputPending(ssh))
+    if (!wolfSSH_OutputPending(ssh))
         return 0;
 
-    WLOG(WS_LOG_DEBUG, "Flushing a disconnect left queued by a short send");
+    if (ssh->disconnected && !ssh->disconnectTxd)
+        return 0;
+
+    WLOG(WS_LOG_DEBUG, "Flushing output left queued by a short send");
     return 1;
 }
 
@@ -1175,9 +1178,11 @@ int wolfSSH_shutdown(WOLFSSH* ssh)
     if (ssh == NULL)
         ret = WS_BAD_ARGUMENT;
 
-    /* This is a teardown call, so a disconnect of ours left queued by a
-     * short send goes out here, with or without a channel to tear down. */
-    if (ret == WS_SUCCESS && FlushQueuedDisconnect(ssh))
+    /* This is a teardown call, so anything a short send left queued goes out
+     * here, with or without a channel to tear down. A rejected auth's
+     * USERAUTH_FAILURE has no channel, and a channel close is retired off
+     * the channel the moment it is bundled. */
+    if (ret == WS_SUCCESS && FlushQueuedOutput(ssh))
         flushRet = wolfSSH_SendPacket(ssh);
 
     if (ret == WS_SUCCESS && ssh->channelList == NULL)
@@ -1760,7 +1765,7 @@ int wolfSSH_SendDisconnect(WOLFSSH *ssh, word32 reason)
      * connection that is already over. A short send leaves the first one
      * queued, though, so that retry goes through. */
     if (SendAfterDisconnect(ssh)) {
-        if (FlushQueuedDisconnect(ssh))
+        if (FlushQueuedOutput(ssh))
             return wolfSSH_SendPacket(ssh);
         return WS_FATAL_ERROR;
     }

tests/regress.c

@@ -6308,6 +6308,222 @@ static void TestShutdownFlushesWithNoChannel(void)
 }
 
 
+/* Two packets can share one flush, so step over the first to reach the
+ * second: a length prefix and the bytes it counts. */
+static word32 NextPacketOffset(const byte* pkt, word32 sz)
+{
+    word32 packetLen;
+
+    AssertTrue(sz >= LENGTH_SZ);
+    packetLen = ReadUint32(pkt);
+    AssertTrue(sz - LENGTH_SZ >= packetLen);
+
+    return packetLen + LENGTH_SZ;
+}
+
+
+#ifndef NO_WOLFSSH_SERVER
+
+static int RejectUserAuthCb(byte authType, WS_UserAuthData* authData,
+        void* ctx)
+{
+    WOLFSSH_UNUSED(authType);
+    WOLFSSH_UNUSED(authData);
+    WOLFSSH_UNUSED(ctx);
+
+    return WOLFSSH_USERAUTH_REJECTED;
+}
+
+
+/* A rejected authentication owes the peer a USERAUTH_FAILURE, RFC 4252
+ * section 5.1. Below the auth-failure cap nothing queues a disconnect to
+ * drag the buffer out, and authentication never completed so there is no
+ * channel: teardown is the only thing left that can push the reply. */
+static void TestShutdownFlushesQueuedUserAuthFailure(void)
+{
+    ChannelOpenHarness harness;
+    byte in[128];
+    word32 inSz;
+
+    inSz = BuildUserAuthPasswordRequest("alice", "pw", in, sizeof(in));
+
+    InitUserAuthHarness(&harness, in, inSz);
+    wolfSSH_SetUserAuth(harness.ctx, RejectUserAuthCb);
+    wolfSSH_SetIOSend(harness.ctx, MemSendWantWrite);
+
+    /* The failure is bundled, then the socket refuses it. */
+    MemSendWantWriteCount = 1;
+    AssertIntEQ(DoReceive(harness.ssh), WS_FATAL_ERROR);
+    AssertIntEQ(harness.ssh->error, WS_USER_AUTH_E);
+    AssertIntEQ(harness.io.outSz, 0);
+    AssertTrue(wolfSSH_OutputPending(harness.ssh));
+
+    /* Neither of the two things that would have flushed it anyway. */
+    AssertFalse(harness.ssh->disconnectTxd);
+    AssertNull(harness.ssh->channelList);
+
+    /* The socket takes bytes now, so only the gate stands between the
+     * queued failure and the peer. */
+    AssertIntEQ(wolfSSH_shutdown(harness.ssh), WS_CHANNEL_CLOSED);
+    AssertFalse(wolfSSH_OutputPending(harness.ssh));
+
+    /* The failure, and nothing bundled behind it. */
+    AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz),
+            MSGID_USERAUTH_FAILURE);
+    AssertIntEQ(harness.io.outSz,
+            NextPacketOffset(harness.io.out, harness.io.outSz));
+
+    FreeChannelOpenHarness(&harness);
+}
+
+#endif /* !NO_WOLFSSH_SERVER */
+
+
+/* DoChannelClose() retires the channel once the close is bundled, so a close
+ * the socket refused is left with no channel to carry the retry. Teardown
+ * still owes the peer that close, RFC 4254 section 5.3. */
+static void TestShutdownFlushesQueuedChannelClose(void)
+{
+    WOLFSSH_CTX* ctx;
+    WOLFSSH* ssh;
+    MemIo io;
+    byte in[128];
+    byte out[512];
+    word32 inSz;
+    word32 channelId;
+    word32 closeOff;
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    AssertNotNull(ctx);
+
+    wolfSSH_SetIORecv(ctx, MemRecv);
+    wolfSSH_SetIOSend(ctx, MemSendWantWrite);
+
+    ssh = wolfSSH_new(ctx);
+    AssertNotNull(ssh);
+    AddSessionChannel(ssh);
+    channelId = ssh->channelList->channel;
+    /* Past userauth, or the message filter turns the close away and the
+     * wire checks below prove nothing. */
+    ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE;
+
+    inSz = BuildChannelClosePacket(channelId, in, sizeof(in));
+    MemIoInit(&io, in, inSz, out, sizeof(out));
+    wolfSSH_SetIOReadCtx(ssh, &io);
+    wolfSSH_SetIOWriteCtx(ssh, &io);
+
+    /* Our EOF and close answer the peer's close. Refuse both sends, or the
+     * second one carries the first out with it. */
+    MemSendWantWriteCount = 2;
+    AssertIntEQ(DoReceive(ssh), WS_CHANNEL_CLOSED);
+    AssertIntEQ(io.outSz, 0);
+    AssertTrue(wolfSSH_OutputPending(ssh));
+
+    /* The channel that would have retried the send is already gone. */
+    AssertNull(ssh->channelList);
+    AssertFalse(ssh->disconnectTxd);
+
+    AssertIntEQ(wolfSSH_shutdown(ssh), WS_CHANNEL_CLOSED);
+    AssertFalse(wolfSSH_OutputPending(ssh));
+
+    /* Both halves reached the peer, EOF ahead of the close, and nothing
+     * else: the two packets account for every byte written. */
+    AssertIntEQ(ParseMsgId(out, io.outSz), MSGID_CHANNEL_EOF);
+    closeOff = NextPacketOffset(out, io.outSz);
+    AssertIntEQ(ParseMsgId(out + closeOff, io.outSz - closeOff),
+            MSGID_CHANNEL_CLOSE);
+    AssertIntEQ(io.outSz,
+            closeOff + NextPacketOffset(out + closeOff, io.outSz - closeOff));
+
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+}
+
+
+/* The flush is no longer tied to a disconnect, so it can fire on a live
+ * session with the channel still listed and then short-send itself. The
+ * teardown sends bundle in behind bytes the socket has not taken, and the
+ * caller has to come back: the retry owes the peer the rest of the buffer
+ * and not a second copy of the EOF and close. */
+static void TestShutdownFlushShortSendsWithChannel(void)
+{
+    WOLFSSH_CTX* ctx;
+    WOLFSSH* ssh;
+    WOLFSSH_CHANNEL* channel;
+    MemIo io;
+    byte out[1024];
+    byte data[32];
+    word32 eofOff;
+    word32 reqOff;
+    word32 closeOff;
+
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    AssertNotNull(ctx);
+
+    wolfSSH_SetIORecv(ctx, MemRecv);
+    wolfSSH_SetIOSend(ctx, MemSendWantWrite);
+
+    ssh = wolfSSH_new(ctx);
+    AssertNotNull(ssh);
+    AddSessionChannel(ssh);
+    channel = ssh->channelList;
+    ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE;
+
+    MemIoInit(&io, NULL, 0, out, sizeof(out));
+    wolfSSH_SetIOReadCtx(ssh, &io);
+    wolfSSH_SetIOWriteCtx(ssh, &io);
+
+    /* Channel data the socket will not take, on a session that is neither
+     * disconnected nor out of channels. */
+    WMEMSET(data, 'x', sizeof(data));
+    MemSendWantWriteCount = 5;
+    AssertIntEQ(wolfSSH_stream_send(ssh, data, sizeof(data)),
+            (int)sizeof(data));
+    AssertIntEQ(wolfSSH_get_error(ssh), WS_WANT_WRITE);
+    AssertTrue(wolfSSH_OutputPending(ssh));
+    AssertFalse(ssh->disconnected);
+    AssertNotNull(ssh->channelList);
+
+    /* The flush and all three teardown sends are refused, so the unfinished
+     * flush owns the return and the whole teardown is still queued. */
+    AssertIntEQ(wolfSSH_shutdown(ssh), WS_WANT_WRITE);
+    AssertIntEQ(io.outSz, 0);
+    AssertTrue(wolfSSH_OutputPending(ssh));
+    AssertIntEQ(MemSendWantWriteCount, 0);
+
+    /* Bundled counts as sent, so the retry must not emit them again. The
+     * list comes first: a channel retired here would be freed, and the
+     * reads below it would be of freed memory. */
+    AssertNotNull(ssh->channelList);
+    AssertTrue(channel->eofTxd);
+    AssertTrue(channel->closeTxd);
+
+    /* The socket takes bytes now. The retry drains what was queued and
+     * lands on the wait for the peer's close, not on a stale want-write
+     * for a buffer that is already empty. */
+    AssertIntEQ(wolfSSH_shutdown(ssh), WS_FATAL_ERROR);
+    AssertIntEQ(wolfSSH_get_error(ssh), WS_WANT_READ);
+    AssertFalse(wolfSSH_OutputPending(ssh));
+
+    /* One copy of each, in order, accounting for every byte written. */
+    AssertIntEQ(ParseMsgId(out, io.outSz), MSGID_CHANNEL_DATA);
+    eofOff = NextPacketOffset(out, io.outSz);
+    AssertIntEQ(ParseMsgId(out + eofOff, io.outSz - eofOff),
+            MSGID_CHANNEL_EOF);
+    reqOff = eofOff + NextPacketOffset(out + eofOff, io.outSz - eofOff);
+    AssertIntEQ(ParseMsgId(out + reqOff, io.outSz - reqOff),
+            MSGID_CHANNEL_REQUEST);
+    closeOff = reqOff + NextPacketOffset(out + reqOff, io.outSz - reqOff);
+    AssertIntEQ(ParseMsgId(out + closeOff, io.outSz - closeOff),
+            MSGID_CHANNEL_CLOSE);
+    AssertIntEQ(io.outSz,
+            closeOff + NextPacketOffset(out + closeOff, io.outSz - closeOff));
+
+    wolfSSH_free(ssh);
+    wolfSSH_CTX_free(ctx);
+}
+
+
 /* The highwater callback is the application's, and its return propagates out
  * of wolfSSH_SendPacket(). A mark firing on the disconnect packet must not
  * fail a send that went out fine, whoever owns the callback. */
@@ -10966,6 +11182,11 @@ int main(int argc, char** argv)
     TestPeerDisconnectKeepsTrafficQueued(0);
     TestPeerDisconnectKeepsTrafficQueued(1);
     TestShutdownFlushesWithNoChannel();
+#ifndef NO_WOLFSSH_SERVER
+    TestShutdownFlushesQueuedUserAuthFailure();
+#endif
+    TestShutdownFlushesQueuedChannelClose();
+    TestShutdownFlushShortSendsWithChannel();
     TestQueuedDisconnectFlushes();
     TestShutdownFlushesQueuedDisconnect();
     TestShutdownKeepsFlushWantWrite();

wolfssh/ssh.h

@@ -688,6 +688,14 @@ WOLFSSH_API int wolfSSH_CTX_SetWindowPacketSize(WOLFSSH_CTX* ctx,
 
 WOLFSSH_API int wolfSSH_accept(WOLFSSH* ssh);
 WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh);
+/* Tears down the first channel in the list, and flushes whatever a short
+ * send left queued, not just a disconnect of ours: a rejected auth's
+ * USERAUTH_FAILURE, and a CHANNEL_CLOSE whose channel was retired the
+ * moment it was bundled, have nothing else left to carry the retry. That
+ * flush can be short too, so a WS_WANT_WRITE from here may be owed to it
+ * rather than to the teardown sends; either way the caller retries. Once
+ * the peer has disconnected, only our own queued disconnect still goes
+ * out, per the comment below. */
 WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh);
 /* A disconnect, sent or received, ends the session. Nothing more goes out:
  * wolfSSH_shutdown() above this comment, and every send call below it,