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.

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.

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.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #875

Scan targets checked: wolfssh-bugs, wolfssh-src

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

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

JacobBarthelmeh · 2026-06-11T21:36:30Z

Reviewed and the changes look good to me. Thanks John!

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.

wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #875

Scan targets checked: wolfssh-bugs, wolfssh-src

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

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

yosuke-wolfssl

I confirmed this new file handle abstraction is more robust than my old one.
I like it.
The minor flaw I found is that Windows FSetSTAT assigns HANDLE to WFD (int) — type mismatch / handle truncation. But it's already addressed by Fenrir-bot.

Copilot

Pull request overview

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

+int wolfSSH_SFTP_TestInvalidateHeadFd(WOLFSSH* ssh)
+{
+    if (ssh == NULL || ssh->fileList == NULL) {
+        return WS_BAD_ARGUMENT;
+    }
+    WCLOSE(ssh->fs, ssh->fileList->fd);
+    return WS_SUCCESS;
+}


+        /* WS_BAD_FILE_E means the handle is not a directory handle, so fall
+         * back to the file-handle path below. WS_SUCCESS or any other error
+         * (including a genuine directory-close failure) is propagated as-is
+         * rather than being masked by the file path. */


wolfSSL-Fenrir-bot

Fenrir Automated Review — PR #875

Scan targets checked: wolfssh-bugs, wolfssh-src

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-06-12T23:25:21Z

+#ifdef USE_WINDOWS_API
+    HANDLE fd = NULL;
+#else
+    WFD fd = 0;


🟠 [Medium] RecvFSetSTAT uses WFD (int) for the handle on Windows, truncating the stored HANDLE · Incorrect sizeof/type usage

wolfSSH_SFTP_RecvFSetSTAT declares WFD fd (an int under USE_WINDOWS_API) and does fd = fileEntry->fd, but WS_FILE_LIST.fd is a HANDLE (void*) on Windows. Unlike RecvFSTAT, it was not given a HANDLE fd variant, so on non-WCE Windows the handle pointer is truncated and the assignment is a constraint violation (MSVC build error).

Fix: Declare fd as HANDLE under USE_WINDOWS_API (mirroring RecvFSTAT) so the stored handle type is preserved.

wolfSSL-Fenrir-bot · 2026-06-12T23:25:21Z

+    word32 ofst[2] = {0, 0};
+    const byte* reply;
+
+    const char ownedPath[]  = "wolfssh_poc_owned.tmp";


🔵 [Low] New SFTP tests create fixture files with fixed names in the CWD · Hardcoded paths, ports, or environment dependencies

The three new tests write fixtures (wolfssh_poc_owned.tmp, wolfssh_poc_victim.tmp, wolfssh_poc_ns.tmp, wolfssh_closefail.tmp) with fixed relative names into the process CWD, requiring a writable CWD and colliding if the binary runs concurrently in the same directory.

Fix: Derive fixture names from a unique per-run token (e.g. PID) or a TMPDIR-based temp directory.

- Track open SFTP file handles per session in a fileList, returning opaque session-scoped handle IDs instead of raw file descriptors. - Resolve and validate client-supplied handle IDs via FindFileHandle. - Free the handle list and close handles on error paths, including the Windows code paths. - Drop the old raw-fd SFTP_ValidateFileHandle/STOREHANDLE handle table and its tests, superseded by the per-session ID lookup.

- Merge fileIdCount and dirIdCount into a single handleIdCount. - File and directory handle IDs now share one namespace. - A close or other handle op cannot match the wrong resource type.

- Reject forged/raw-fd handles in Write/Read/FSetSTAT/FSTAT/Close - Isolate file vs directory handle-ID namespaces - Cover positive and forged FSTAT

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

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

Comment thread src/wolfsftp.c

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

wolfSSL-Fenrir-bot reviewed Apr 10, 2026

View reviewed changes

ejohnstown force-pushed the handles branch from 90dd328 to 6a3ac91 Compare June 10, 2026 19:31

ejohnstown requested review from Copilot and wolfSSL-Fenrir-bot June 11, 2026 15:42

Copilot started reviewing on behalf of ejohnstown June 11, 2026 15:42 View session

Copilot AI reviewed Jun 11, 2026

View reviewed changes

Comment thread src/wolfsftp.c

Comment thread src/wolfsftp.c

wolfSSL-Fenrir-bot reviewed Jun 11, 2026

View reviewed changes

ejohnstown force-pushed the handles branch from 6a3ac91 to bbca466 Compare June 11, 2026 17:35

ejohnstown requested a review from yosuke-wolfssl June 11, 2026 17:48

ejohnstown unassigned JacobBarthelmeh Jun 11, 2026

JacobBarthelmeh requested review from Copilot and wolfSSL-Fenrir-bot June 11, 2026 21:35

Copilot AI reviewed Jun 11, 2026

View reviewed changes

wolfSSL-Fenrir-bot reviewed Jun 11, 2026

View reviewed changes

Copilot started reviewing on behalf of JacobBarthelmeh June 11, 2026 22:06 View session

yosuke-wolfssl reviewed Jun 12, 2026

View reviewed changes

Comment thread tests/regress.c Outdated

ejohnstown force-pushed the handles branch 2 times, most recently from fe48ae0 to 53b848f Compare June 12, 2026 19:41

ejohnstown requested review from Copilot and wolfSSL-Fenrir-bot June 12, 2026 22:58

Copilot started reviewing on behalf of ejohnstown June 12, 2026 22:58 View session

Copilot AI reviewed Jun 12, 2026

View reviewed changes

wolfSSL-Fenrir-bot reviewed Jun 12, 2026

View reviewed changes

JacobBarthelmeh and others added 3 commits June 12, 2026 16:26

Share one ID counter for SFTP handles

a6570e0

- Merge fileIdCount and dirIdCount into a single handleIdCount. - File and directory handle IDs now share one namespace. - A close or other handle op cannot match the wrong resource type.

Test SFTP forged handles and namespace isolation

6645fee

- Reject forged/raw-fd handles in Write/Read/FSetSTAT/FSTAT/Close - Isolate file vs directory handle-ID namespaces - Cover positive and forged FSTAT

ejohnstown force-pushed the handles branch from 53b848f to 6645fee Compare June 12, 2026 23:29

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!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #875

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #875

Uh oh!

JacobBarthelmeh commented Jun 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

Fenrir Automated Review — PR #875

Uh oh!

yosuke-wolfssl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

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 Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

wolfSSL-Fenrir-bot Jun 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers