SFTP file handle abstraction#875
Open
JacobBarthelmeh wants to merge 4 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
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.
Contributor
There was a problem hiding this comment.
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
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
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.
- 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.
- Add TestSftpForgedHandleRejected(): forged handles (raw fd bytes and fabricated IDs) aimed at a non-SFTP descriptor are refused by Recv Write/Read/FSetSTAT/Close, leaving the victim intact. - Open a real handle first as a positive control. - Add WOLFSSH_TEST_INTERNAL recvState accessors so the assertions live in regress.c.
- 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.
- Add TestSftpHandleNamespaceIsolation(): file and directory handles share one ID namespace and never collide. - File handlers (READ/WRITE/FSETSTAT) reject a directory handle; READDIR rejects a file handle. - Closing the file leaves the directory handle valid, so a close never crosses resource types.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ZD20562