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

Author: ejohnstown

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
 };
@@ -6969,9 +6968,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
  * {
@@ -7361,24 +7378,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) !=
@@ -7597,6 +7612,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;
@@ -7605,13 +7626,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;
                 }
@@ -7639,21 +7661,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,
@@ -8006,14 +8013,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;
 
@@ -8221,6 +8224,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;
@@ -8567,17 +8575,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;
@@ -8773,16 +8776,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;
@@ -9034,15 +9032,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);
@@ -9202,6 +9195,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;
@@ -9590,17 +9589,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;
@@ -9739,6 +9733,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;
@@ -9828,6 +9827,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;