WolfSSL
/
wolfssh
mirror of https://github.com/wolfSSL/wolfssh.git
1
0
Fork 0
Code Issues Releases Wiki Activity

sftp: check the request ID on every client reply 

SFTP_CheckReqId() matches each reply header against the outstanding
request ID, advances the ID once on a match, and sets
WS_SFTP_BAD_REQ_ID on a mismatch. Every client response handler calls
it before acting on the reply, including the STATUS-only ones.

- DoName and GetHandle set WS_SFTP_BAD_REQ_TYPE on an unexpected
  reply type and free their state
- SetSTAT, Close, Remove and RMDIR now check the reply ID
- correct the WS_SFTP_BAD_REQ_TYPE comment in error.h
- test each request against a matched, misnumbered and mistyped reply

Issue: F-10547, F-13909, F-10566
pull/1347/merge
John Safranek 2026-10-21 09:18:17 -07:00 committed by philljj
parent 6b990c3b12
commit a879502632
3 changed files with 233 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

126
src/wolfsftp.c
View File

@ -375,7 +375,6 @@ enum WS_SFTP_GET_HANDLE_STATE_ID {
STATE_GET_HANDLE_INIT,
STATE_GET_HANDLE_GET_HEADER,
STATE_GET_HANDLE_DO_STATUS,
STATE_GET_HANDLE_CHECK_REQ_ID,
STATE_GET_HANDLE_READ,
STATE_GET_HANDLE_CLEANUP
};
@ -7060,9 +7059,27 @@ int SendPacketType(WOLFSSH* ssh, byte type, byte* buf, word32 bufSz)
}
/* Check that a response answers the outstanding request. A match advances
* the request ID for the next request; a mismatch sets WS_SFTP_BAD_REQ_ID.
*
* returns WS_SUCCESS on a match
*/
static int SFTP_CheckReqId(WOLFSSH* ssh, word32 reqId)
{
if (reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "Unexpected request ID %u, expected %u",
reqId, ssh->reqId);
ssh->error = WS_SFTP_BAD_REQ_ID;
return WS_FATAL_ERROR;
}
ssh->reqId++;
return WS_SUCCESS;
}
/* process a status packet
*
* assuming that request ID has already been read
* assuming that request ID has already been read and checked
*
* structure of status packet is as follows
* {
@ -7444,24 +7461,22 @@ static WS_SFTPNAME* wolfSSH_SFTP_DoName(WOLFSSH* ssh)
return NULL;
}
if (reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "unexpected ID");
if (SFTP_CheckReqId(ssh, reqId) != WS_SUCCESS) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_NAME);
return NULL;
}
if (type != WOLFSSH_FTP_NAME) {
WLOG(WS_LOG_SFTP, "Unexpected packet type %d", type);
/* check for status msg */
if (type == WOLFSSH_FTP_STATUS) {
state->state = SFTP_NAME_DO_STATUS;
}
else {
return NULL;
}
if (type == WOLFSSH_FTP_NAME) {
state->state = SFTP_NAME_GET_PACKET;
}
else if (type == WOLFSSH_FTP_STATUS) {
state->state = SFTP_NAME_DO_STATUS;
}
else {
ssh->reqId += 1;
state->state = SFTP_NAME_GET_PACKET;
WLOG(WS_LOG_SFTP, "Unexpected packet type %d", type);
ssh->error = WS_SFTP_BAD_REQ_TYPE;
wolfSSH_SFTP_ClearState(ssh, STATE_ID_NAME);
return NULL;
}
if (wolfSSH_SFTP_buffer_create(ssh, &state->buffer, maxSz) !=
@ -7681,6 +7696,12 @@ static int wolfSSH_SFTP_GetHandle(WOLFSSH* ssh, byte* handle, word32* handleSz)
}
}
if (SFTP_CheckReqId(ssh, handleState->reqId) != WS_SUCCESS) {
handleState->state = STATE_GET_HANDLE_CLEANUP;
ret = WS_FATAL_ERROR;
continue;
}
if (wolfSSH_SFTP_buffer_create(ssh, &handleState->buffer, ret) !=
WS_SUCCESS) {
handleState->state = STATE_GET_HANDLE_CLEANUP;
@ -7689,13 +7710,14 @@ static int wolfSSH_SFTP_GetHandle(WOLFSSH* ssh, byte* handle, word32* handleSz)
}
if (type == WOLFSSH_FTP_HANDLE)
handleState->state = STATE_GET_HANDLE_CHECK_REQ_ID;
handleState->state = STATE_GET_HANDLE_READ;
else if (type == WOLFSSH_FTP_STATUS) {
handleState->state = STATE_GET_HANDLE_DO_STATUS;
}
else {
WLOG(WS_LOG_SFTP,
"Unexpected packet type with getting handle");
ssh->error = WS_SFTP_BAD_REQ_TYPE;
handleState->state = STATE_GET_HANDLE_CLEANUP;
ret = WS_FATAL_ERROR;
}
@ -7723,21 +7745,6 @@ static int wolfSSH_SFTP_GetHandle(WOLFSSH* ssh, byte* handle, word32* handleSz)
handleState->state = STATE_GET_HANDLE_CLEANUP;
continue;
case STATE_GET_HANDLE_CHECK_REQ_ID:
WLOG(WS_LOG_SFTP, "SFTP GET HANDLE STATE: CHECK_REQ_ID");
/* @TODO packets do not need to be in order, may need
* mechanism to handle out of order ID's? */
if (handleState->reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "Unexpected ID");
ret = WS_FATAL_ERROR;
handleState->state = STATE_GET_HANDLE_CLEANUP;
continue;
}
ssh->reqId++;
handleState->state = STATE_GET_HANDLE_READ;
FALL_THROUGH;
case STATE_GET_HANDLE_READ:
WLOG(WS_LOG_SFTP, "SFTP GET HANDLE STATE: READ");
ret = wolfSSH_SFTP_buffer_read(ssh, &handleState->buffer,
@ -8090,14 +8097,10 @@ static int SFTP_STAT(WOLFSSH* ssh, char* dir, WS_SFTP_FILEATRB* atr, byte type)
FALL_THROUGH;
case STATE_LSTAT_CHECK_REQ_ID:
/* check request ID */
if (state->reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "Bad request ID received");
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_LSTAT);
return WS_FATAL_ERROR;
}
else {
ssh->reqId++;
}
state->state = STATE_LSTAT_PARSE_REPLY;
FALL_THROUGH;
@ -8306,6 +8309,11 @@ int wolfSSH_SFTP_SetSTAT(WOLFSSH* ssh, char* dir, WS_SFTP_FILEATRB* atr)
return WS_FATAL_ERROR;
}
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
ret = WS_FATAL_ERROR;
break;
}
if (type != WOLFSSH_FTP_STATUS) {
WLOG(WS_LOG_SFTP, "Unexpected packet type %d", type);
ret = WS_FATAL_ERROR;
@ -8654,17 +8662,12 @@ int wolfSSH_SFTP_SendWritePacket(WOLFSSH* ssh, byte* handle, word32 handleSz,
continue;
}
/* check request ID */
if (state->reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "Bad request ID received");
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
ret = WS_FATAL_ERROR;
ssh->error = WS_SFTP_BAD_REQ_ID;
state->state = STATE_SEND_WRITE_CLEANUP;
continue;
}
ssh->reqId++;
if (type != WOLFSSH_FTP_STATUS) {
WLOG(WS_LOG_SFTP, "Unexpected packet type");
ret = WS_FATAL_ERROR;
@ -8870,16 +8873,11 @@ int wolfSSH_SFTP_SendReadPacket(WOLFSSH* ssh, byte* handle, word32 handleSz,
case STATE_SEND_READ_CHECK_REQ_ID:
WLOG(WS_LOG_SFTP, "SFTP SEND_READ STATE: CHECK_REQ_ID");
/* check request ID */
if (state->reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "Bad request ID received");
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
ret = WS_FATAL_ERROR;
state->state = STATE_SEND_READ_CLEANUP;
continue;
}
else {
ssh->reqId++;
}
if (state->type == WOLFSSH_FTP_DATA)
state->state = STATE_SEND_READ_FTP_DATA;
@ -9131,15 +9129,10 @@ int wolfSSH_SFTP_MKDIR(WOLFSSH* ssh, char* dir, WS_SFTP_FILEATRB* atr)
return WS_FATAL_ERROR;
}
/* check request ID */
if (state->reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "Bad request ID received");
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_MKDIR);
return WS_FATAL_ERROR;
}
else {
ssh->reqId++;
}
if (wolfSSH_SFTP_buffer_create(ssh, &state->buffer, ret) != 0) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_MKDIR);
@ -9300,6 +9293,12 @@ int wolfSSH_SFTP_Close(WOLFSSH* ssh, byte* handle, word32 handleSz)
continue;
}
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
ret = WS_FATAL_ERROR;
state->state = STATE_CLOSE_CLEANUP;
continue;
}
if (wolfSSH_SFTP_buffer_create(ssh, &state->buffer, ret) != 0) {
ret = WS_MEMORY_E;
state->state = STATE_CLOSE_CLEANUP;
@ -9688,17 +9687,12 @@ int wolfSSH_SFTP_Rename(WOLFSSH* ssh, const char* old, const char* nw)
continue;
}
/* check request ID */
if (state->reqId != ssh->reqId) {
WLOG(WS_LOG_SFTP, "Bad request ID received");
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
ret = WS_FATAL_ERROR;
ssh->error = WS_SFTP_BAD_REQ_ID;
state->state = STATE_RENAME_CLEANUP;
continue;
}
ssh->reqId++;
if (type != WOLFSSH_FTP_STATUS) {
WLOG(WS_LOG_SFTP, "Unexpected packet type");
ret = WS_FATAL_ERROR;
@ -9838,6 +9832,11 @@ int wolfSSH_SFTP_Remove(WOLFSSH* ssh, char* f)
return WS_FATAL_ERROR;
}
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_RM);
return WS_FATAL_ERROR;
}
if (wolfSSH_SFTP_buffer_create(ssh, &state->buffer, ret) != 0) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_RM);
return WS_FATAL_ERROR;
@ -9928,6 +9927,11 @@ int wolfSSH_SFTP_RMDIR(WOLFSSH* ssh, char* dir)
}
return WS_FATAL_ERROR;
}
if (SFTP_CheckReqId(ssh, state->reqId) != WS_SUCCESS) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_RMDIR);
return WS_FATAL_ERROR;
}
if (wolfSSH_SFTP_buffer_create(ssh, &state->buffer, ret) != 0) {
wolfSSH_SFTP_ClearState(ssh, STATE_ID_RMDIR);
return WS_MEMORY_E;

168
tests/unit.c
View File

@ -24358,7 +24358,7 @@ static int SftpGetCleanupCase(int mode)
}
ret = wolfSSH_SFTP_Get(ssh, from, to, 0, NULL);
if (ret == WS_SUCCESS || ssh->getState != NULL ||
ssh->closeState != NULL || ssh->reqId != id + 1) {
ssh->closeState != NULL || ssh->reqId != id + 2) {
result = -1118;
}
goto done;
@ -24648,6 +24648,168 @@ static int test_SftpPutReadError(void)
return 0;
}
enum SftpReplyIdOp {
REPLY_ID_REMOVE,
REPLY_ID_RMDIR,
REPLY_ID_CLOSE,
REPLY_ID_SETSTAT,
REPLY_ID_OPEN,
REPLY_ID_REALPATH,
REPLY_ID_OP_COUNT
};
enum SftpReplyIdMode {
REPLY_ID_MATCH,
REPLY_ID_WRONG_ID,
REPLY_ID_WRONG_TYPE
};
/* Answer one client request with a reply carrying the request id, a wrong
* id, or (REALPATH and OPEN only) the right id on a wrong message type. A
* matched reply advances the request id once; a mismatch fails with a
* specific error and frees the request state. */
static int SftpReplyIdCase(int op, int mode)
{
WOLFSSH_CTX* ctx = NULL;
WOLFSSH* ssh = NULL;
WS_SFTP_FILEATRB atr;
WS_SFTPNAME* name = NULL;
byte reply[WOLFSSH_SFTP_HEADER + UINT32_SZ * 3];
byte handle[WOLFSSH_MAX_HANDLE];
word32 handleSz = sizeof(handle);
word32 sz;
word32 id;
word32 status = WOLFSSH_FTP_OK;
int ret;
int failed;
int stateLeft;
int result = 0;
ret = SftpClientNewSession(&ctx, &ssh);
if (ret != 0) {
result = ret;
goto done;
}
ssh->sftpState = SFTP_DONE;
ssh->channelList->peerWindowSz = 65536;
ssh->channelList->peerMaxPacketSz = 32768;
id = ssh->reqId;
if (op == REPLY_ID_REMOVE) {
/* REMOVE checks the file with an LSTAT first */
sz = SftpBuildReply(reply, sizeof(reply), WOLFSSH_FTP_ATTRS, id,
UINT32_SZ);
PutU32BE(reply + WOLFSSH_SFTP_HEADER, 0);
if (wolfSSH_TestChannelPutData(ssh->channelList, reply, sz) != 0) {
result = -1200;
goto done;
}
id++;
}
if (op == REPLY_ID_OPEN || op == REPLY_ID_REALPATH)
status = WOLFSSH_FTP_FAILURE;
if (mode == REPLY_ID_WRONG_TYPE && op == REPLY_ID_OPEN) {
sz = SftpBuildReply(reply, sizeof(reply), WOLFSSH_FTP_ATTRS, id,
UINT32_SZ);
PutU32BE(reply + WOLFSSH_SFTP_HEADER, 0);
ret = wolfSSH_TestChannelPutData(ssh->channelList, reply, sz);
}
else if (mode == REPLY_ID_WRONG_TYPE) {
sz = SftpBuildReply(reply, sizeof(reply), WOLFSSH_FTP_HANDLE, id,
UINT32_SZ + 1);
PutU32BE(reply + WOLFSSH_SFTP_HEADER, 1);
reply[WOLFSSH_SFTP_HEADER + UINT32_SZ] = 'h';
ret = wolfSSH_TestChannelPutData(ssh->channelList, reply, sz);
}
else {
ret = SftpGetStatus(ssh, mode == REPLY_ID_WRONG_ID ? id + 1 : id,
status);
}
if (ret != 0) {
result = -1201;
goto done;
}
WMEMSET(&atr, 0, sizeof(atr));
switch (op) {
case REPLY_ID_REMOVE:
ret = wolfSSH_SFTP_Remove(ssh, (char*)"f");
stateLeft = ssh->rmState != NULL;
break;
case REPLY_ID_RMDIR:
ret = wolfSSH_SFTP_RMDIR(ssh, (char*)"d");
stateLeft = ssh->rmdirState != NULL;
break;
case REPLY_ID_CLOSE:
handle[0] = 'h';
ret = wolfSSH_SFTP_Close(ssh, handle, 1);
stateLeft = ssh->closeState != NULL;
break;
case REPLY_ID_SETSTAT:
ret = wolfSSH_SFTP_SetSTAT(ssh, (char*)"f", &atr);
stateLeft = ssh->setatrState != NULL;
break;
case REPLY_ID_OPEN:
ret = wolfSSH_SFTP_Open(ssh, (char*)"f", WOLFSSH_FXF_READ, NULL,
handle, &handleSz);
stateLeft = ssh->openState != NULL ||
ssh->getHandleState != NULL;
break;
default:
name = wolfSSH_SFTP_RealPath(ssh, (char*)"p");
ret = name == NULL ? WS_FATAL_ERROR : WS_SUCCESS;
/* realState 0 is SFTP_REAL_SEND_PACKET, ready for a new call */
stateLeft = ssh->nameState != NULL || ssh->realState != 0;
wolfSSH_SFTPNAME_list_free(name);
break;
}
if (stateLeft) {
result = -1202;
}
else if (mode == REPLY_ID_MATCH) {
failed = status != WOLFSSH_FTP_OK;
if ((failed ? ret == WS_SUCCESS : ret != WS_SUCCESS) ||
wolfSSH_get_error(ssh) == WS_SFTP_BAD_REQ_ID ||
ssh->reqId != id + 1) {
result = -1203;
}
}
else if (ret == WS_SUCCESS || wolfSSH_get_error(ssh) !=
(mode == REPLY_ID_WRONG_ID ?
WS_SFTP_BAD_REQ_ID : WS_SFTP_BAD_REQ_TYPE)) {
result = -1204;
}
else if (ssh->reqId != (mode == REPLY_ID_WRONG_ID ? id : id + 1)) {
result = -1205;
}
done:
wolfSSH_free(ssh);
wolfSSH_CTX_free(ctx);
return result;
}
static int test_SftpReplyId(void)
{
int op;
int mode;
int ret;
for (op = 0; op < REPLY_ID_OP_COUNT; op++) {
for (mode = REPLY_ID_MATCH; mode <= REPLY_ID_WRONG_TYPE; mode++) {
if (mode == REPLY_ID_WRONG_TYPE && op != REPLY_ID_REALPATH &&
op != REPLY_ID_OPEN)
continue;
ret = SftpReplyIdCase(op, mode);
if (ret != 0) {
printf("SFTP reply id op %d mode %d: %d\n", op, mode, ret);
return ret;
}
}
}
return 0;
}
#endif /* WOLFSSH_TEST_SFTP_PUT */
/* Builds an SFTP DATA reply in "out": the 9 byte header carrying the request
@ -26860,6 +27022,10 @@ int wolfSSH_UnitTest(int argc, char** argv)
printf("SftpClientPutWriteStatusFail: %s\n",
(unitResult == 0 ? "SUCCESS" : "FAILED"));
testResult = testResult || unitResult;
unitResult = test_SftpReplyId();
printf("SftpReplyId: %s\n", unitResult == 0 ? "SUCCESS" : "FAILED");
testResult = testResult || unitResult;
#endif
unitResult = test_SftpSendReadPacketSplit();

2
wolfssh/error.h
View File

@ -99,7 +99,7 @@ enum WS_ErrorCodes {
WS_CHAN_RXD = -1057, /* Status that channel data received. */
WS_INVALID_EXTDATA = -1058, /* invalid Channel Extended Data Type */
WS_SFTP_BAD_REQ_ID = -1060, /* SFTP Bad request ID */
WS_SFTP_BAD_REQ_TYPE = -1061, /* SFTP Bad request ID */
WS_SFTP_BAD_REQ_TYPE = -1061, /* SFTP Bad request response type */
WS_SFTP_STATUS_NOT_OK = -1062, /* SFTP Status not OK */
WS_SFTP_FILE_DNE = -1063, /* SFTP File Does Not Exist */
WS_SIZE_ONLY = -1064, /* Only getting size of buffer needed */