Skip to content

Commit 50aa10d

Browse files
ejohnstownejohnstown
authored
Merge pull request #793 from JacobBarthelmeh/rekey
sanity checks on message types during rekey
2 parents 6ea3f5c + 4862400 commit 50aa10d

8 files changed

Lines changed: 210 additions & 49 deletions

File tree

‎examples/client/client.c‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,9 @@ static THREAD_RET readInput(void* in)
342342
ret = wolfSSH_stream_send(args->ssh, buf, sz);
343343
wc_UnLockMutex(&args->lock);
344344
if (ret <= 0) {
345+
if (ret == WS_REKEYING) {
346+
continue;
347+
}
345348
fprintf(stderr, "Couldn't send data\n");
346349
return THREAD_RET_SUCCESS;
347350
}
@@ -472,8 +475,16 @@ static THREAD_RET readPeer(void* in)
472475
continue;
473476
}
474477
#endif /* WOLFSSH_AGENT */
478+
else if (ret == WS_REKEYING) {
479+
wolfSSH_worker(args->ssh, NULL);
480+
ret = 0;
481+
}
475482
}
476483
else if (ret != WS_EOF) {
484+
if (ret == 0) {
485+
bytes = 0;
486+
continue;
487+
}
477488
err_sys("Stream read failed.");
478489
}
479490
}

‎examples/echoserver/echoserver.c‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1416,8 +1416,11 @@ static int sftp_worker(thread_ctx_t* threadCtx)
14161416
}
14171417
else if (ret < 0) {
14181418
error = wolfSSH_get_error(ssh);
1419-
if (error == WS_EOF)
1419+
if (error == WS_EOF) {
1420+
/* shutdown is happening, clear peek error */
1421+
ret = 0;
14201422
break;
1423+
}
14211424
}
14221425

14231426
if (ret == WS_FATAL_ERROR && error == 0) {

‎examples/sftpclient/sftpclient.c‎

Lines changed: 80 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -747,6 +747,13 @@ static int doCmds(func_args* args)
747747

748748
/* check directory is valid */
749749
do {
750+
while (ret == WS_REKEYING || ssh->error == WS_REKEYING) {
751+
ret = wolfSSH_worker(ssh, NULL);
752+
if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) {
753+
ret = wolfSSH_get_error(ssh);
754+
}
755+
}
756+
750757
ret = wolfSSH_SFTP_STAT(ssh, pt, &atrb);
751758
err = wolfSSH_get_error(ssh);
752759
} while ((err == WS_WANT_READ || err == WS_WANT_WRITE)
@@ -828,6 +835,13 @@ static int doCmds(func_args* args)
828835

829836
/* update permissions */
830837
do {
838+
while (ret == WS_REKEYING || ssh->error == WS_REKEYING) {
839+
ret = wolfSSH_worker(ssh, NULL);
840+
if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) {
841+
ret = wolfSSH_get_error(ssh);
842+
}
843+
}
844+
831845
ret = wolfSSH_SFTP_CHMOD(ssh, pt, mode);
832846
err = wolfSSH_get_error(ssh);
833847
} while ((err == WS_WANT_READ || err == WS_WANT_WRITE)
@@ -878,6 +892,13 @@ static int doCmds(func_args* args)
878892
}
879893

880894
do {
895+
while (ret == WS_REKEYING || ssh->error == WS_REKEYING) {
896+
ret = wolfSSH_worker(ssh, NULL);
897+
if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) {
898+
ret = wolfSSH_get_error(ssh);
899+
}
900+
}
901+
881902
ret = wolfSSH_SFTP_RMDIR(ssh, pt);
882903
err = wolfSSH_get_error(ssh);
883904
} while ((err == WS_WANT_READ || err == WS_WANT_WRITE)
@@ -924,6 +945,13 @@ static int doCmds(func_args* args)
924945
}
925946

926947
do {
948+
while (ret == WS_REKEYING || ssh->error == WS_REKEYING) {
949+
ret = wolfSSH_worker(ssh, NULL);
950+
if (ret != WS_SUCCESS && ret == WS_FATAL_ERROR) {
951+
ret = wolfSSH_get_error(ssh);
952+
}
953+
}
954+
927955
ret = wolfSSH_SFTP_Remove(ssh, pt);
928956
err = wolfSSH_get_error(ssh);
929957
} while ((err == WS_WANT_READ || err == WS_WANT_WRITE)
@@ -1119,7 +1147,7 @@ static int doCmds(func_args* args)
11191147
/* alternate main loop for the autopilot get/receive */
11201148
static int doAutopilot(int cmd, char* local, char* remote)
11211149
{
1122-
int err;
1150+
int err = 0;
11231151
int ret = WS_SUCCESS;
11241152
char fullpath[128] = ".";
11251153
WS_SFTPNAME* name = NULL;
@@ -1156,6 +1184,12 @@ static int doAutopilot(int cmd, char* local, char* remote)
11561184
}
11571185

11581186
do {
1187+
if (err == WS_REKEYING || err == WS_WINDOW_FULL) { /* handle rekeying state */
1188+
do {
1189+
ret = wolfSSH_worker(ssh, NULL);
1190+
} while (ret == WS_REKEYING);
1191+
}
1192+
11591193
if (cmd == AUTOPILOT_PUT) {
11601194
ret = wolfSSH_SFTP_Put(ssh, local, fullpath, 0, NULL);
11611195
}
@@ -1164,7 +1198,8 @@ static int doAutopilot(int cmd, char* local, char* remote)
11641198
}
11651199
err = wolfSSH_get_error(ssh);
11661200
} while ((err == WS_WANT_READ || err == WS_WANT_WRITE ||
1167-
err == WS_CHAN_RXD || err == WS_REKEYING) &&
1201+
err == WS_CHAN_RXD || err == WS_REKEYING ||
1202+
err == WS_WINDOW_FULL) &&
11681203
ret == WS_FATAL_ERROR);
11691204

11701205
if (ret != WS_SUCCESS) {
@@ -1452,14 +1487,52 @@ THREAD_RETURN WOLFSSH_THREAD sftpclient_test(void* args)
14521487

14531488
WFREE(workingDir, NULL, DYNAMIC_TYPE_TMP_BUFFER);
14541489
if (ret == WS_SUCCESS) {
1455-
if (wolfSSH_shutdown(ssh) != WS_SUCCESS) {
1456-
int rc;
1457-
rc = wolfSSH_get_error(ssh);
1490+
int err;
1491+
ret = wolfSSH_shutdown(ssh);
1492+
1493+
/* peer hung up, stop trying to shutdown */
1494+
if (ret == WS_SOCKET_ERROR_E) {
1495+
ret = 0;
1496+
}
1497+
1498+
err = wolfSSH_get_error(ssh);
1499+
if (err != WS_SOCKET_ERROR_E &&
1500+
(err == WS_WANT_READ || err == WS_WANT_WRITE)) {
1501+
int maxAttempt = 10; /* make 10 attempts max before giving up */
1502+
int attempt;
1503+
1504+
for (attempt = 0; attempt < maxAttempt; attempt++) {
1505+
ret = wolfSSH_worker(ssh, NULL);
1506+
err = wolfSSH_get_error(ssh);
1507+
1508+
/* peer successfully closed down gracefully */
1509+
if (ret == WS_CHANNEL_CLOSED) {
1510+
ret = 0;
1511+
break;
1512+
}
1513+
1514+
/* peer hung up, stop shutdown */
1515+
if (ret == WS_SOCKET_ERROR_E) {
1516+
ret = 0;
1517+
break;
1518+
}
1519+
1520+
if (err == WS_WANT_READ || err == WS_WANT_WRITE) {
1521+
/* Wanting read or wanting write. Clear ret. */
1522+
ret = 0;
1523+
}
1524+
else {
1525+
break;
1526+
}
1527+
}
14581528

1459-
if (rc != WS_SOCKET_ERROR_E && rc != WS_EOF)
1460-
printf("error with wolfSSH_shutdown()\n");
1529+
if (attempt == maxAttempt) {
1530+
printf("SFTP client gave up on gracefull shutdown,"
1531+
"closing the socket\n");
1532+
}
14611533
}
14621534
}
1535+
14631536
WCLOSESOCKET(sockFd);
14641537
wolfSSH_free(ssh);
14651538
wolfSSH_CTX_free(ctx);

‎src/internal.c‎

Lines changed: 54 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,40 @@ static void HandshakeInfoFree(HandshakeInfo* hs, void* heap)
595595
}
596596

597597

598+
/* RFC 4253 section 7.1, Once having sent SSH_MSG_KEXINIT the only messages
599+
* that can be sent are 1-19 (except SSH_MSG_SERVICE_REQUEST and
600+
* SSH_MSG_SERVICE_ACCEPT), 20-29 (except SSH_MSG_KEXINIT again), and 30-49
601+
*/
602+
INLINE static int IsMessageAllowedKeying(WOLFSSH *ssh, byte msg)
603+
{
604+
if (ssh->isKeying == 0) {
605+
return 1;
606+
}
607+
608+
/* case of service request or accept in 1-19 */
609+
if (msg == MSGID_SERVICE_REQUEST || msg == MSGID_SERVICE_ACCEPT) {
610+
WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by during rekeying", msg);
611+
ssh->error = WS_REKEYING;
612+
return 0;
613+
}
614+
615+
/* case of resending SSH_MSG_KEXINIT */
616+
if (msg == MSGID_KEXINIT) {
617+
WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by during rekeying", msg);
618+
ssh->error = WS_REKEYING;
619+
return 0;
620+
}
621+
622+
/* case where message id greater than 49 */
623+
if (msg >= MSGID_USERAUTH_REQUEST) {
624+
WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by during rekeying", msg);
625+
ssh->error = WS_REKEYING;
626+
return 0;
627+
}
628+
return 1;
629+
}
630+
631+
598632
#ifndef NO_WOLFSSH_SERVER
599633
INLINE static int IsMessageAllowedServer(WOLFSSH *ssh, byte msg)
600634
{
@@ -673,8 +707,14 @@ INLINE static int IsMessageAllowedClient(WOLFSSH *ssh, byte msg)
673707
#endif /* NO_WOLFSSH_CLIENT */
674708

675709

676-
INLINE static int IsMessageAllowed(WOLFSSH *ssh, byte msg)
710+
/* 'state' argument is for if trying to send a message or receive one.
711+
* Returns 1 if allowed 0 if not allowed. */
712+
INLINE static int IsMessageAllowed(WOLFSSH *ssh, byte msg, byte state)
677713
{
714+
if (state == WS_MSG_SEND && !IsMessageAllowedKeying(ssh, msg)) {
715+
return 0;
716+
}
717+
678718
#ifndef NO_WOLFSSH_SERVER
679719
if (ssh->ctx->side == WOLFSSH_ENDPOINT_SERVER) {
680720
return IsMessageAllowedServer(ssh, msg);
@@ -5905,7 +5945,6 @@ static int DoNewKeys(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
59055945
HandshakeInfoFree(ssh->handshake, ssh->ctx->heap);
59065946
ssh->handshake = NULL;
59075947
WLOG(WS_LOG_DEBUG, "Keying completed");
5908-
59095948
if (ssh->ctx->keyingCompletionCb)
59105949
ssh->ctx->keyingCompletionCb(ssh->keyingCompletionCtx);
59115950
}
@@ -9322,7 +9361,7 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed)
93229361
return WS_OVERFLOW_E;
93239362
}
93249363

9325-
if (!IsMessageAllowed(ssh, msg)) {
9364+
if (!IsMessageAllowed(ssh, msg, WS_MSG_RECV)) {
93269365
return WS_MSGID_NOT_ALLOWED_E;
93279366
}
93289367

@@ -15662,6 +15701,12 @@ int SendChannelEof(WOLFSSH* ssh, word32 peerChannelId)
1566215701
if (ssh == NULL)
1566315702
ret = WS_BAD_ARGUMENT;
1566415703

15704+
if (ret == WS_SUCCESS) {
15705+
if (!IsMessageAllowed(ssh, MSGID_CHANNEL_EOF, WS_MSG_SEND)) {
15706+
ret = WS_MSGID_NOT_ALLOWED_E;
15707+
}
15708+
}
15709+
1566515710
if (ret == WS_SUCCESS) {
1566615711
channel = ChannelFind(ssh, peerChannelId, WS_CHANNEL_ID_PEER);
1566715712
if (channel == NULL)
@@ -16090,6 +16135,12 @@ int SendChannelWindowAdjust(WOLFSSH* ssh, word32 channelId,
1609016135
if (ssh == NULL)
1609116136
ret = WS_BAD_ARGUMENT;
1609216137

16138+
if (ret == WS_SUCCESS) {
16139+
if (!IsMessageAllowed(ssh, MSGID_CHANNEL_WINDOW_ADJUST, WS_MSG_SEND)) {
16140+
ret = WS_MSGID_NOT_ALLOWED_E;
16141+
}
16142+
}
16143+
1609316144
channel = ChannelFind(ssh, channelId, WS_CHANNEL_ID_SELF);
1609416145
if (channel == NULL) {
1609516146
WLOG(WS_LOG_DEBUG, "Invalid channel");

‎src/ssh.c‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1135,6 +1135,11 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz)
11351135
return WS_ERROR;
11361136
}
11371137

1138+
if (ssh->isKeying) {
1139+
ssh->error = WS_REKEYING;
1140+
return WS_FATAL_ERROR;
1141+
}
1142+
11381143
inputBuffer = &ssh->channelList->inputBuffer;
11391144
ssh->error = WS_SUCCESS;
11401145

@@ -1164,7 +1169,7 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz)
11641169
}
11651170

11661171
/* update internal input buffer based on data read */
1167-
if (ret == WS_SUCCESS) {
1172+
if (ret == WS_SUCCESS && !ssh->isKeying) {
11681173
int n;
11691174

11701175
n = min(bufSz, inputBuffer->length - inputBuffer->idx);
@@ -1196,7 +1201,7 @@ int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz)
11961201

11971202
if (ssh->isKeying) {
11981203
ssh->error = WS_REKEYING;
1199-
return WS_REKEYING;
1204+
return WS_FATAL_ERROR;
12001205
}
12011206

12021207
bytesTxd = SendChannelData(ssh, ssh->channelList->channel, buf, bufSz);
@@ -2901,6 +2906,11 @@ int wolfSSH_ChannelRead(WOLFSSH_CHANNEL* channel, byte* buf, word32 bufSz)
29012906
if (channel == NULL || buf == NULL || bufSz == 0)
29022907
return WS_BAD_ARGUMENT;
29032908

2909+
if (channel->ssh->isKeying) {
2910+
channel->ssh->error = WS_REKEYING;
2911+
return WS_REKEYING;
2912+
}
2913+
29042914
bufSz = _ChannelRead(channel, buf, bufSz);
29052915

29062916
WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_ChannelRead(), bytesRxd = %d",

0 commit comments

Comments
 (0)