SFTP file handle abstraction by JacobBarthelmeh · Pull Request #875 · wolfSSL/wolfssh

JacobBarthelmeh · 2026-02-03T23:07:19Z

ZD20562

Copilot

Pull request overview

This pull request introduces a new file handle abstraction for the SFTP subsystem, replacing the old conditional WOLFSSH_STOREHANDLE mechanism with an always-on tracking system. Instead of sending raw file descriptors or handles to SFTP clients, the implementation now generates unique 8-byte IDs that are tracked internally in a linked list (WS_FILE_LIST), improving security and portability.

Changes:

Removed old WOLFSSH_STOREHANDLE conditional compilation and associated functions (SFTP_AddHandleNode, SFTP_RemoveHandleNode, SFTP_GetHandleNode, SFTP_FreeHandles)
Added new file handle tracking system with WS_FILE_LIST structure and helper functions (SFTP_AddFileHandle, SFTP_FindFileHandle, SFTP_RemoveFileHandle, SFTP_FreeAllFileHandles)
Unified RecvClose implementation across platforms (previously had separate Unix and Windows versions)
Updated all file operations (Open, Read, Write, Close, FSTAT, FSetSTAT) to use 8-byte handle IDs instead of raw file descriptors

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 14 comments.

File	Description
wolfssh/wolfsftp.h	Removed declarations for old WOLFSSH_STOREHANDLE functions (SFTP_AddHandleNode, SFTP_RemoveHandleNode)
wolfssh/internal.h	Added WOLFSSH_HANDLE_ID_SZ constant, WS_FILE_LIST typedef, and fileIdCount/fileList members to WOLFSSH struct, removed conditional WOLFSSH_STOREHANDLE handleList member
src/wolfsftp.c	Implemented new file handle tracking system, updated all file operations to use 8-byte IDs, unified cross-platform RecvClose implementation, removed old WOLFSSH_STOREHANDLE code, fixed code style (else statement formatting)

Comments suppressed due to low confidence (1)

src/wolfsftp.c:2184

When SFTP_CreatePacket fails on line 2176, the file is closed (lines 2178-2182), but the file handle is not removed from the tracking list. This creates the same issue as with the malloc failure: the handle remains in ssh->fileList with an invalid file descriptor. SFTP_RemoveFileHandle should be called to clean up the tracking list before returning WS_FATAL_ERROR.

        if (SFTP_CreatePacket(ssh, WOLFSSH_FTP_HANDLE, out, outSz,
            idFlat, sizeof(idFlat)) != WS_SUCCESS) {
        #ifdef MICROCHIP_MPLAB_HARMONY
            WFCLOSE(ssh->fs, &fd);
        #else
            WCLOSE(ssh->fs, fd);
        #endif
            return WS_FATAL_ERROR;
        }

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/wolfsftp.c

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-04T19:21:25Z

src/wolfsftp.c

@@ -2151,28 +2163,34 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
            ret = WS_FATAL_ERROR;
        }


Resource leak: If SFTP_AddFileHandle fails but wolfSSH_SFTP_CreateStatus succeeds (returns WS_SIZE_ONLY), the file descriptor is not closed before returning WS_FATAL_ERROR. The file should be closed unconditionally after SFTP_AddFileHandle failure, similar to how it's handled in the Windows version at line 2322.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #875

Scan targets checked: wolfssh-bugs, wolfssh-compliance, wolfssh-consttime, wolfssh-defaults, wolfssh-mutation, wolfssh-proptest, wolfssh-src, wolfssh-zeroize

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

wolfSSL-Fenrir-bot · 2026-04-10T16:30:21Z

src/wolfsftp.c

        return WS_BUFFER_E;
    }

-#ifndef NO_WOLFSSH_DIR
-    /* check if is a handle for a directory */
-    if (sz == (sizeof(word32) * 2)) {
-        ret = wolfSSH_SFTP_RecvCloseDir(ssh, data + idx, sz);
-    }
-    if (ret != WS_SUCCESS) {
-#endif /* NO_WOLFSSH_DIR */
-    if (sz == sizeof(WFD)) {
-        WMEMSET((byte*)&fd, 0, sizeof(WFD));
-        WMEMCPY((byte*)&fd, data + idx, sz);
-
-#ifdef MICROCHIP_MPLAB_HARMONY
-        ret = WFCLOSE(ssh->fs, &fd);
-#else
-        ret = WCLOSE(ssh->fs, fd);
-#endif
-    #ifdef WOLFSSH_STOREHANDLE
-        if (SFTP_RemoveHandleNode(ssh, data + idx, sz) != WS_SUCCESS) {
-            WLOG(WS_LOG_SFTP, "Unable to remove handle from list");
-            ret = WS_FATAL_ERROR;
-        }
-    #endif
-    }
-    else {
-        ret = WS_FATAL_ERROR;
-    }
-#ifndef NO_WOLFSSH_DIR
-    }
-#endif
-
-    if (ret < 0) {
-        WLOG(WS_LOG_SFTP, "Error closing file");
-        res = err;
+    /* Validate file handle size - must be 8 bytes for tracked handles */
+    if (sz != WOLFSSH_HANDLE_ID_SZ) {
+        WLOG(WS_LOG_SFTP, "Invalid handle size - expected 8 bytes");
        ret = WS_BAD_FILE_E;
    }
    else {
-        res  = suc;
-        type = WOLFSSH_FTP_OK;
-        ret  = WS_SUCCESS;
-    }
+        word32 handle[2] = {0, 0};

-    if (wolfSSH_SFTP_CreateStatus(ssh, type, reqId, res, "English", NULL,
-                &outSz) != WS_SIZE_ONLY) {
-        return WS_FATAL_ERROR;
-    }
-    out = (byte*)WMALLOC(outSz, ssh->ctx->heap, DYNTYPE_BUFFER);
-    if (out == NULL) {
-        return WS_MEMORY_E;
-    }
-    if (wolfSSH_SFTP_CreateStatus(ssh, type, reqId, res, "English", out,
-                &outSz) != WS_SUCCESS) {
-        WFREE(out, ssh->ctx->heap, DYNTYPE_BUFFER);
-        return WS_FATAL_ERROR;
-    }
-
-    /* set send out buffer, "out" is taken by ssh  */
-    wolfSSH_SFTP_RecvSetSend(ssh, out, outSz);
-    return ret;
-}
-#else /* USE_WINDOWS_API */
-{
-    HANDLE fd;
-    word32 sz;
-    word32 idx  = 0;
-    int    ret = WS_FATAL_ERROR;
-
-    byte* out = NULL;
-    word32 outSz = 0;
-
-    char* res = NULL;
-    char  suc[] = "Closed File";
-    char  err[] = "Close File Error";
-    byte  type = WOLFSSH_FTP_FAILURE;
-
-    if (ssh == NULL) {
-        return WS_BAD_ARGUMENT;
-    }
-
-    WLOG(WS_LOG_SFTP, "Receiving WOLFSSH_FTP_CLOSE");
-
-    if (maxSz < UINT32_SZ) {
-        /* not enough for an ato32 call */
-        return WS_BUFFER_E;
-    }
-
-    /* get file handle */
-    ato32(data + idx, &sz); idx += UINT32_SZ;
-    if (sz + idx > maxSz || sz > WOLFSSH_MAX_HANDLE) {
-        return WS_BUFFER_E;
-    }
+        ato32(data + idx, &handle[0]);
+        ato32(data + idx + UINT32_SZ, &handle[1]);

+        /* First check if it's a directory handle */
 #ifndef NO_WOLFSSH_DIR
-    /* check if is a handle for a directory */
-    if (sz == (sizeof(word32) * 2)) {
        ret = wolfSSH_SFTP_RecvCloseDir(ssh, data + idx, sz);
-    }
-    if (ret != WS_SUCCESS) {
-#endif /* NO_WOLFSSH_DIR */
-    if (sz == sizeof(HANDLE)) {
-        WMEMSET((byte*)&fd, 0, sizeof(HANDLE));
-        WMEMCPY((byte*)&fd, data + idx, sz);
-        CloseHandle(fd);
-        ret = WS_SUCCESS;
-    #ifdef WOLFSSH_STOREHANDLE
-        if (SFTP_RemoveHandleNode(ssh, data + idx, sz) != WS_SUCCESS) {
-            WLOG(WS_LOG_SFTP, "Unable to remove handle from list");
-            ret = WS_FATAL_ERROR;
+        if (ret == WS_SUCCESS) {
+            /* It was a directory handle */
        }
-    #endif
-    }
-    else {
-        ret = WS_FATAL_ERROR;
-    }
+        else {
+#endif /* NO_WOLFSSH_DIR */
+            WS_FILE_LIST* fileNode = NULL;
+
+            fileNode = SFTP_FindFileHandle(ssh, handle);
+            if (fileNode != NULL) {
+#ifdef MICROCHIP_MPLAB_HARMONY
+                ret = WFCLOSE(ssh->fs, &fileNode->fd);
+#elif defined(USE_WINDOWS_API)
+                /* Close the file and remove from tracking list */
+                ret = (CloseHandle(fileNode->fd) != 0) ? WS_SUCCESS : WS_INVALID_STATE_E;
+#else
+                ret = WCLOSE(ssh->fs, fileNode->fd);
+#endif
+                if (ret >= 0) {
+                    ret = SFTP_RemoveFileHandle(ssh, handle);
+                }
+            }
+            else {
+                WLOG(WS_LOG_SFTP, "Invalid handle - not found in session");
+                ret = WS_BAD_FILE_E;
+            }
 #ifndef NO_WOLFSSH_DIR
+        }
+#endif /* NO_WOLFSSH_DIR */
    }
-#endif

    if (ret < 0) {
        WLOG(WS_LOG_SFTP, "Error closing file");


⚠️ [High] File and directory handle ID namespace collision causes wrong-resource close
Category: Roundtrip property violations

The new fileIdCount[2] counter (in wolfssh/internal.h:899) and the pre-existing dirIdCount[2] counter (in wolfssh/internal.h:896) both start at [0,0] and increment independently. Both produce 8-byte (WOLFSSH_HANDLE_ID_SZ) opaque handles sent to the client. In wolfSSH_SFTP_RecvClose, the code unconditionally tries wolfSSH_SFTP_RecvCloseDir first, and only falls through to file handle close on failure:

ret = wolfSSH_SFTP_RecvCloseDir(ssh, data + idx, sz); if (ret == WS_SUCCESS) { /* It was a directory handle */ } else { /* try file close */ }

Since the first directory opened gets ID [0,0] and the first file opened also gets ID [0,0], a close request for the file handle [0,0] will match the directory entry in RecvCloseDir instead, closing the wrong resource. This is a reliable bug in any SFTP session where both files and directories are opened (the standard SFTP workflow). The mismatch means: (1) the directory is unexpectedly closed, (2) the file handle leaks and cannot be closed by the client until session teardown via SFTP_FreeAllFileHandles.

/* Both counters start at [0,0] in struct WOLFSSH */ word32 dirIdCount[2]; /* internal.h:896 */ word32 fileIdCount[2]; /* internal.h:899 */ /* RecvOpen assigns file IDs from fileIdCount */ id[0] = ssh->fileIdCount[0]; id[1] = ssh->fileIdCount[1]; AddAssign64(ssh->fileIdCount, 1); /* RecvClose tries directory first */ ret = wolfSSH_SFTP_RecvCloseDir(ssh, data + idx, sz); if (ret == WS_SUCCESS) { /* directory closed */ } else { /* only then try file handle */ }

Recommendation: Use a single shared counter for both directory and file handle IDs to guarantee uniqueness across both namespaces. Alternatively, prefix or tag the ID space (e.g., set the high bit for file handles vs directory handles) so RecvCloseDir cannot match a file handle ID. The simplest fix is to replace the separate fileIdCount and dirIdCount with one shared handleIdCount used by both RecvOpen and RecvOpenDir.

wolfSSL-Fenrir-bot · 2026-04-10T16:30:21Z

src/wolfsftp.c


    /* get file handle */
    ato32(data + idx, &sz); idx += UINT32_SZ;
-    if (sz + idx > maxSz || sz > WOLFSSH_MAX_HANDLE || sz != sizeof(WFD)) {
+    if (sz + idx > maxSz || sz > WOLFSSH_MAX_HANDLE) {
        return WS_BUFFER_E;
    }
-    WMEMSET((byte*)&fd, 0, sizeof(WFD));
-    WMEMCPY((byte*)&fd, data + idx, sz); idx += sz;
+
+    /* Validate file handle size - must be 8 bytes for tracked handles */
+    if (sz != WOLFSSH_HANDLE_ID_SZ) {
+        WLOG(WS_LOG_SFTP, "Invalid file handle size - expected 8 bytes");
+        if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId,
+                "Invalid file handle", "English", NULL, &outSz) != WS_SIZE_ONLY) {
+            return WS_FATAL_ERROR;
+        }
+        res = err;
+        type = WOLFSSH_FTP_FAILURE;
+        ret = WS_BAD_FILE_E;
+    }
+    else {
+        word32 handle[2] = {0, 0};
+        WS_FILE_LIST* fileEntry = NULL;
+
+        ato32(data + idx, &handle[0]);
+        ato32(data + idx + UINT32_SZ, &handle[1]);
+
+        /* Find the file handle in our tracking list */
+        fileEntry = SFTP_FindFileHandle(ssh, handle);
+        if (fileEntry == NULL) {
+            WLOG(WS_LOG_SFTP, "Invalid file handle - not found in session");
+            if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId,
+                    "Invalid file handle", "English", NULL, &outSz) != WS_SIZE_ONLY) {
+                return WS_FATAL_ERROR;
+            }
+            res = err;
+            type = WOLFSSH_FTP_FAILURE;
+            ret = WS_BAD_FILE_E;
+        }
+        else {
+            fd = fileEntry->fd;
+            ret = WS_SUCCESS;
+        }
+        idx += sz;
+    }

    /* get offset into file */
    ato32(data + idx, &ofst[1]); idx += UINT32_SZ;


🔹 [Low] RecvRead (non-Windows) continues parsing at wrong buffer offset after handle validation failure
Category: Parser robustness gaps

In the non-Windows wolfSSH_SFTP_RecvRead, when handle validation fails (sz != WOLFSSH_HANDLE_ID_SZ or SFTP_FindFileHandle returns NULL), idx is only advanced by sz inside the else branch (success path). In the error path, idx is NOT advanced past the handle bytes. However, the subsequent offset parsing is unconditional:

/* get offset into file */ ato32(data + idx, &ofst[1]); idx += UINT32_SZ; ato32(data + idx, &ofst[0]); idx += UINT32_SZ;

This reads offset values from the wrong buffer position (the handle bytes instead of offset bytes). While the actual WPREAD call is guarded by ret == WS_SUCCESS, the sz variable is overwritten by ato32(data + idx, &sz) from an incorrect position, and this sz controls the subsequent buffer allocation size. The allocation is bounded by the maxSz check, so this is not directly exploitable, but it represents a parsing inconsistency that could interact with future code changes. The Windows variant of RecvRead correctly gates all subsequent parsing inside if (ret == WS_SUCCESS), showing the intended pattern.

if (sz != WOLFSSH_HANDLE_ID_SZ) { /* error path - idx NOT advanced */ ret = WS_BAD_FILE_E; } else { /* success path */ idx += sz; /* only here is idx advanced */ } /* unconditional - reads from wrong offset on error */ ato32(data + idx, &ofst[1]); idx += UINT32_SZ; ato32(data + idx, &ofst[0]); idx += UINT32_SZ;

Recommendation: Gate the offset and length parsing behind if (ret == WS_SUCCESS), matching the pattern used in the Windows variant of RecvRead (around line 4158) and in RecvWrite. Move idx += sz outside the else branch (or before the if/else) so that idx is always advanced past the handle data regardless of validation result.

JacobBarthelmeh added 2 commits February 3, 2026 15:55

keep a list of file handles open per session

ad92290

node handle refactor

d192f8e

JacobBarthelmeh requested a review from Copilot February 3, 2026 23:07

JacobBarthelmeh self-assigned this Feb 3, 2026

Copilot started reviewing on behalf of JacobBarthelmeh February 3, 2026 23:07 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

Copilot AI reviewed Feb 4, 2026

View reviewed changes

clean up handles list in error cases and adjustments for windows cases

90dd328

JacobBarthelmeh requested a review from Copilot February 4, 2026 18:44

Copilot started reviewing on behalf of JacobBarthelmeh February 4, 2026 19:04 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

padelsbach requested a review from wolfSSL-Fenrir-bot April 10, 2026 15:58

wolfSSL-Fenrir-bot reviewed Apr 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SFTP file handle abstraction#875

SFTP file handle abstraction#875
JacobBarthelmeh wants to merge 3 commits intowolfSSL:masterfrom
JacobBarthelmeh:handles

JacobBarthelmeh commented Feb 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 4, 2026

Uh oh!

wolfSSL-Fenrir-bot left a comment

Uh oh!

wolfSSL-Fenrir-bot Apr 10, 2026

Uh oh!

wolfSSL-Fenrir-bot Apr 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -2151,28 +2163,34 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz)
		ret = WS_FATAL_ERROR;
		}

Conversation

JacobBarthelmeh commented Feb 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #875

Uh oh!

wolfSSL-Fenrir-bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Fenrir-bot Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants