diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 0bc1f1dbd..5715062a3 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -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 }; @@ -6961,9 +6960,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 * { @@ -7353,24 +7370,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) != @@ -7589,6 +7604,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; @@ -7597,13 +7618,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; } @@ -7631,21 +7653,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, @@ -7998,14 +8005,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; @@ -8213,6 +8216,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; @@ -8559,17 +8567,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; @@ -8765,16 +8768,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; @@ -9026,15 +9024,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); @@ -9194,6 +9187,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; @@ -9582,17 +9581,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; @@ -9731,6 +9725,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; @@ -9820,6 +9819,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; diff --git a/tests/unit.c b/tests/unit.c index 82adcd5e6..4e68510bb 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -23768,7 +23768,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; @@ -24058,6 +24058,161 @@ 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 only) the right id on a HANDLE. 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) { + 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) + 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 @@ -26240,6 +26395,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(); diff --git a/wolfssh/error.h b/wolfssh/error.h index a7ee92111..a587027eb 100644 --- a/wolfssh/error.h +++ b/wolfssh/error.h @@ -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 */