Skip to content

Commit 527e019

Browse files
yosuke-wolfsslejohnstown
authored andcommitted
internal: set the keying flag once the KEX init reaches the transport
- SendKexInit() takes ssh->txFlushCount before SendPacketFlush() and sets WOLFSSH_SELF_IS_KEYING when SendPacketDelivered() reports the packet away, in place of setting it before the packet is built. - PurgePacket() runs on the same decision rather than on the send's return. - SendKexInit() runs HighwaterCheck() itself, after the flag is set, in place of taking it from wolfSSH_SendPacket(). - internal.h describes the flag as set once the KEX init is sent or queued. - tests/unit.c covers a KEX init the transport rejects, one it refuses with a would-block, and one it takes whole behind a highwater callback that fails or reads the keying flag; the first two also check whether the packet is left owed. - HwTestCb() also records wolfSSH_RekeyPending() for the session its HwTestCtx names. - FailHighwater(), s_sendRefusals and RefuseThenResetIoSend() move beside the other shared test callbacks and gain WS_MAYBE_UNUSED, so the client-side test compiles without the server.
1 parent 84b360a commit 527e019

3 files changed

Lines changed: 140 additions & 32 deletions

File tree

‎src/internal.c‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15199,6 +15199,7 @@ int SendKexInit(WOLFSSH* ssh)
1519915199
macAlgoNamesSz = 0, noneNamesSz = 0;
1520015200

1520115201
int ret = WS_SUCCESS;
15202+
int delivered = 0;
1520215203

1520315204
WLOG(WS_LOG_DEBUG, "Entering SendKexInit()");
1520415205

@@ -15225,8 +15226,6 @@ int SendKexInit(WOLFSSH* ssh)
1522515226
}
1522615227

1522715228
if (ret == WS_SUCCESS) {
15228-
/* Set self is keying flag since we started sending the KEX init msg */
15229-
ssh->isKeying |= WOLFSSH_SELF_IS_KEYING;
1523015229
if (ssh->handshake == NULL) {
1523115230
ssh->handshake = HandshakeInfoNew(ssh->ctx->heap);
1523215231
if (ssh->handshake == NULL) {
@@ -15354,11 +15353,23 @@ int SendKexInit(WOLFSSH* ssh)
1535415353
}
1535515354

1535615355
if (ret == WS_SUCCESS) {
15357-
ret = wolfSSH_SendPacket(ssh);
15356+
word32 flushes = ssh->txFlushCount;
15357+
15358+
ret = SendPacketFlush(ssh);
15359+
delivered = SendPacketDelivered(ssh, flushes, ret);
1535815360
}
1535915361

15360-
if (ret != WS_WANT_WRITE && ret != WS_SUCCESS)
15362+
if (delivered) {
15363+
/* Set self is keying flag once the KEX init is sent or queued, before
15364+
* HighwaterCheck() can run a callback that reads it. */
15365+
ssh->isKeying |= WOLFSSH_SELF_IS_KEYING;
15366+
}
15367+
else {
1536115368
PurgePacket(ssh);
15369+
}
15370+
15371+
if (ret == WS_SUCCESS)
15372+
ret = HighwaterCheck(ssh, WOLFSSH_HWSIDE_TRANSMIT);
1536215373

1536315374
WLOG(WS_LOG_DEBUG, "Leaving SendKexInit(), ret = %d", ret);
1536415375
return ret;

‎tests/unit.c‎

Lines changed: 124 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -4160,10 +4160,13 @@ static int test_ChannelPutData(void)
41604160
return result;
41614161
}
41624162

4163-
/* Counter callback for test_MsgHighwater. Records each invocation without
4164-
* triggering wolfSSH_TriggerKeyExchange (which needs a live session). */
4163+
/* Counter callback for the highwater tests. Records each invocation, and the
4164+
* keying state of ssh when one is set, without triggering
4165+
* wolfSSH_TriggerKeyExchange (which needs a live session). */
41654166
typedef struct HwTestCtx {
4167+
WOLFSSH* ssh;
41664168
int count;
4169+
int keying;
41674170
byte lastSide;
41684171
} HwTestCtx;
41694172

@@ -4173,6 +4176,7 @@ static int HwTestCb(byte side, void* ctx)
41734176
if (hc != NULL) {
41744177
hc->count++;
41754178
hc->lastSide = side;
4179+
hc->keying = wolfSSH_RekeyPending(hc->ssh);
41764180
}
41774181
return WS_SUCCESS;
41784182
}
@@ -4618,6 +4622,13 @@ static WS_MAYBE_UNUSED int OobIoSend(WOLFSSH* ssh, void* buf, word32 sz,
46184622
return (int)sz + 1;
46194623
}
46204624

4625+
/* Reports an error whenever the byte or message highwater mark fires. */
4626+
static WS_MAYBE_UNUSED int FailHighwater(byte side, void* ctx)
4627+
{
4628+
(void)side; (void)ctx;
4629+
return WS_FATAL_ERROR;
4630+
}
4631+
46214632
static int test_DoChannelExtendedData_overflow(void)
46224633
{
46234634
WOLFSSH_CTX* ctx = NULL;
@@ -4907,6 +4918,24 @@ static WS_MAYBE_UNUSED int PacketIoRecv(WOLFSSH* ssh, void* buf, word32 sz, void
49074918
return (int)n;
49084919
}
49094920

4921+
/* Write budget for the IOSend mocks: that many writes are refused with a
4922+
* would-block before the mock acts. */
4923+
static int s_sendRefusals = 0;
4924+
4925+
/* Refuses the first s_sendRefusals writes with a would-block, taking no bytes,
4926+
* then resets the socket. */
4927+
static WS_MAYBE_UNUSED int RefuseThenResetIoSend(WOLFSSH* ssh, void* buf,
4928+
word32 sz, void* ctx)
4929+
{
4930+
(void)ssh; (void)buf; (void)sz; (void)ctx;
4931+
4932+
if (s_sendRefusals > 0) {
4933+
s_sendRefusals--;
4934+
return WS_CBIO_ERR_WANT_WRITE;
4935+
}
4936+
return WS_CBIO_ERR_CONN_RST;
4937+
}
4938+
49104939
/* Builds a plaintext CHANNEL_EXTENDED_DATA (stderr) SSH packet addressed to
49114940
* channelId, carrying 10 bytes of payload set to fill, into pkt (needs 32
49124941
* bytes) and returns its size. A bare session negotiates no cipher
@@ -5758,13 +5787,6 @@ static int test_ChannelExtDataBufferGrowth(void)
57585787

57595788
#ifndef NO_WOLFSSH_SERVER
57605789

5761-
/* Fires once the message highwater mark is crossed and reports an error. */
5762-
static int FailHighwater(byte side, void* ctx)
5763-
{
5764-
(void)side; (void)ctx;
5765-
return WS_FATAL_ERROR;
5766-
}
5767-
57685790
/* wolfSSH_SendPacket() runs the highwater check after the packet is on the wire
57695791
* and returns the highwater callback's status, so a failing callback makes a
57705792
* delivered WINDOW_ADJUST look like a failed send. Credit re-parked then is
@@ -7927,7 +7949,6 @@ static int test_StreamReadEofOtherChannel(void)
79277949

79287950
static byte s_sentBuf[512];
79297951
static word32 s_sentSz = 0;
7930-
static int s_sendRefusals = 0;
79317952

79327953
/* Refuses the first s_sendRefusals writes with a would-block, then takes
79337954
* everything and keeps a copy of what reached the transport. */
@@ -7949,23 +7970,6 @@ static int RefuseThenCaptureIoSend(WOLFSSH* ssh, void* buf, word32 sz,
79497970
}
79507971

79517972

7952-
/* Refuses the sends DoChannelClose() makes, then resets the socket under the
7953-
* worker's flush. */
7954-
static int RefuseThenResetIoSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
7955-
{
7956-
WOLFSSH_UNUSED(ssh);
7957-
WOLFSSH_UNUSED(buf);
7958-
WOLFSSH_UNUSED(sz);
7959-
WOLFSSH_UNUSED(ctx);
7960-
7961-
if (s_sendRefusals > 0) {
7962-
s_sendRefusals--;
7963-
return WS_CBIO_ERR_WANT_WRITE;
7964-
}
7965-
return WS_CBIO_ERR_CONN_RST;
7966-
}
7967-
7968-
79697973
/* DoPacket() consumes the peer's CHANNEL_CLOSE whatever DoChannelClose()
79707974
* returns, so the reply gets one chance to be built. A blocked socket must not
79717975
* cost it: the EOF and the close both have to be bundled, and the channel
@@ -8809,6 +8813,94 @@ static int test_TriggerKeyExchangeKeepsError(void)
88098813
wolfSSH_CTX_free(ctx);
88108814
return result;
88118815
}
8816+
8817+
8818+
/* Covers a KEX init the transport rejects, one it refuses with a
8819+
* would-block, and one it takes whole with a highwater callback that fails
8820+
* or reads the keying flag. */
8821+
static int test_KexInitSendAwayGatesKeying(void)
8822+
{
8823+
WOLFSSH_CTX* ctx = NULL;
8824+
WOLFSSH* ssh = NULL;
8825+
HwTestCtx hc;
8826+
int result = 0;
8827+
int ret;
8828+
8829+
ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
8830+
if (ctx == NULL)
8831+
return -1925;
8832+
/* No refusals, so the first write resets the socket. */
8833+
s_sendRefusals = 0;
8834+
wolfSSH_SetIOSend(ctx, RefuseThenResetIoSend);
8835+
8836+
ssh = wolfSSH_new(ctx);
8837+
if (ssh == NULL) { result = -1926; goto done; }
8838+
8839+
ret = wolfSSH_TriggerKeyExchange(ssh);
8840+
if (ret != WS_SOCKET_ERROR_E) { result = -1927; goto done; }
8841+
if (wolfSSH_RekeyPending(ssh)) { result = -1928; goto done; }
8842+
if (wolfSSH_OutputPending(ssh)) { result = -1929; goto done; }
8843+
8844+
wolfSSH_free(ssh);
8845+
ssh = NULL;
8846+
8847+
/* One refusal leaves the packet framed and owed instead. */
8848+
s_sendRefusals = 1;
8849+
ssh = wolfSSH_new(ctx);
8850+
if (ssh == NULL) { result = -1930; goto done; }
8851+
8852+
ret = wolfSSH_TriggerKeyExchange(ssh);
8853+
if (ret != WS_WANT_WRITE) { result = -1931; goto done; }
8854+
if (!wolfSSH_RekeyPending(ssh)) { result = -1932; goto done; }
8855+
if (!wolfSSH_OutputPending(ssh)) { result = -1933; goto done; }
8856+
8857+
wolfSSH_free(ssh);
8858+
ssh = NULL;
8859+
wolfSSH_CTX_free(ctx);
8860+
ctx = NULL;
8861+
8862+
/* The transport takes the whole packet and the highwater callback then
8863+
* fails, so the error arrives with the KEX init already sent. */
8864+
ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
8865+
if (ctx == NULL) { result = -1934; goto done; }
8866+
wolfSSH_SetIOSend(ctx, DiscardIoSend);
8867+
wolfSSH_SetHighwaterCb(ctx, 1, FailHighwater);
8868+
8869+
ssh = wolfSSH_new(ctx);
8870+
if (ssh == NULL) { result = -1935; goto done; }
8871+
8872+
ret = wolfSSH_TriggerKeyExchange(ssh);
8873+
if (ret == WS_SUCCESS) { result = -1936; goto done; }
8874+
if (!wolfSSH_RekeyPending(ssh)) { result = -1937; goto done; }
8875+
8876+
wolfSSH_free(ssh);
8877+
ssh = NULL;
8878+
wolfSSH_CTX_free(ctx);
8879+
ctx = NULL;
8880+
8881+
/* The highwater callback runs from the KEX init's own flush, with the
8882+
* packet already on the wire. */
8883+
ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
8884+
if (ctx == NULL) { result = -1938; goto done; }
8885+
wolfSSH_SetIOSend(ctx, DiscardIoSend);
8886+
wolfSSH_SetHighwaterCb(ctx, 1, HwTestCb);
8887+
8888+
ssh = wolfSSH_new(ctx);
8889+
if (ssh == NULL) { result = -1939; goto done; }
8890+
WMEMSET(&hc, 0, sizeof(hc));
8891+
hc.ssh = ssh;
8892+
wolfSSH_SetHighwaterCtx(ssh, &hc);
8893+
8894+
ret = wolfSSH_TriggerKeyExchange(ssh);
8895+
if (ret != WS_SUCCESS) { result = -1940; goto done; }
8896+
if (hc.count != 1 || !hc.keying) { result = -1941; goto done; }
8897+
8898+
done:
8899+
s_sendRefusals = 0;
8900+
wolfSSH_free(ssh);
8901+
wolfSSH_CTX_free(ctx);
8902+
return result;
8903+
}
88128904
#endif /* NO_WOLFSSH_CLIENT */
88138905

88148906

@@ -23099,6 +23191,11 @@ int wolfSSH_UnitTest(int argc, char** argv)
2309923191
printf("TriggerKeyExchangeKeepsError: %s\n",
2310023192
(unitResult == 0 ? "SUCCESS" : "FAILED"));
2310123193
testResult = testResult || unitResult;
23194+
23195+
unitResult = test_KexInitSendAwayGatesKeying();
23196+
printf("KexInitSendAwayGatesKeying: %s\n",
23197+
(unitResult == 0 ? "SUCCESS" : "FAILED"));
23198+
testResult = testResult || unitResult;
2310223199
#endif
2310323200

2310423201

‎wolfssh/internal.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -780,7 +780,7 @@ enum NameIdType {
780780
#define WOLFSSH_PROTOID_LIMIT 255
781781

782782
/* Keep track of keying state for both sides of the connection.
783-
* WOLFSSH_SELF_IS_KEYING gets set on sending KEX init and
783+
* WOLFSSH_SELF_IS_KEYING gets set once the KEX init is sent or queued and
784784
* WOLFSSH_PEER_IS_KEYING gets set on receiving KEX init */
785785
#define WOLFSSH_PEER_IS_KEYING 0x01
786786
#define WOLFSSH_SELF_IS_KEYING 0x02

0 commit comments

Comments
 (0)