Skip to content

Harden a file handle validation and add a unit test#997

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/sftpFileHandle
Open

Harden a file handle validation and add a unit test#997
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/sftpFileHandle

Conversation

@yosuke-wolfssl
Copy link
Copy Markdown
Contributor

This PR extends the existing handleList infrastructure to validate the requested handle during SFTP operations.
The target functions:

  • wolfSSH_SFTP_RecvRead
  • wolfSSH_SFTP_RecvWrite
  • wolfSSH_SFTP_RecvFSetSTAT
  • wolfSSH_SFTP_RecvClose
  • wolfSSH_SFTP_RecvFSTAT

These functions do the validation of the requested handle using new helper SFTP_ValidateFileHandle.
SFTP_GetHandleNode, SFTP_AddHandleNode, SFTP_RemoveHandleNode, and SFTP_FreeHandles are not wrapped with WOLFSSH_STOREHANDLE anymore because these are used by this validation logic.
But, WOLFSSH_STOREHANDLE still exists for the original purpose: WOLFSSH_STOREHANDLE gates only name = cur->name in RecvFSTAT. The settings.h auto-defines for FATFS / Nucleus / Harmony / MQX remain meaningful because those platforms pass the filename to their fstat equivalents.

Also, this PR add a unit test for validation helper.

Addressed by f_4232, f_4343, f_4346, f_4349

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 2, 2026
Copilot AI review requested due to automatic review settings June 2, 2026 05:29
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens SFTP file-handle handling on the server. A new helper SFTP_ValidateFileHandle checks both the size of an incoming handle and its presence in the per-session handleList, and is now invoked from RecvRead, RecvWrite, RecvClose, RecvFSTAT, and RecvFSetSTAT. The handle table is no longer gated on WOLFSSH_STOREHANDLE (that macro now only controls whether RecvFSTAT looks up the cached filename), so all server SFTP builds get this validation. A new unit test TestSftpValidateFileHandle covers the helper.

Changes:

  • Add SFTP_ValidateFileHandle / SFTP_ValidateFileHandle_ex and call them from the read/write/close/fstat/fsetstat handlers.
  • Always compile WS_HANDLE_LIST, handleList, and the SFTP_*HandleNode helpers; restrict WOLFSSH_STOREHANDLE to gating only the cached-name use in RecvFSTAT.
  • Add wolfSSH_TestSftpValidateFileHandle test wrapper and TestSftpValidateFileHandle regression test, and re-gate the existing handle test on !NO_WOLFSSH_SERVER instead of WOLFSSH_STOREHANDLE.

Reviewed changes

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

File Description
wolfssh/internal.h Move WS_HANDLE_LIST definition out of WOLFSSH_STOREHANDLE and unconditionally include handleList in WOLFSSH.
wolfssh/wolfsftp.h Constify name in SFTP_AddHandleNode; declare wolfSSH_TestSftpValidateFileHandle for unit tests.
src/wolfsftp.c Introduce validation helpers, replace ad-hoc size checks in SFTP receive paths, and remove WOLFSSH_STOREHANDLE gating around handle-list bookkeeping.
tests/regress.c Re-gate existing SFTP handle test, add TestSftpValidateFileHandle, and register it in main.

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

Comment thread tests/regress.c Outdated
Copy link
Copy Markdown

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #997

Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
1 finding(s) posted as inline comments (see file-level comments below)

Medium (1)

Windows handle cleanup uses CRT close

File: src/wolfsftp.c:4538
Function: SFTP_FreeHandles
Category: API contract violations

SFTP_FreeHandles() closes stored Windows HANDLE values through WCLOSE, which maps to _close(int), so abandoned SFTP file handles are not closed correctly on Windows.

Recommendation: Add a USE_WINDOWS_API branch that copies a HANDLE from cur->handle and calls CloseHandle.

Referenced code: src/wolfsftp.c:4538-4542 (5 lines)


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

Comment thread tests/regress.c
Comment thread tests/regress.c
Copy link
Copy Markdown

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #997

Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 1

Medium (1)

Windows cleanup closes stored HANDLEs with the wrong API

File: src/wolfsftp.c:4562
Function: SFTP_FreeHandles
Category: API contract violations

On Windows, RecvOpen stores HANDLE values in handleList, but SFTP_FreeHandles() closes entries as WFD through WCLOSE. Session cleanup leaks open Windows file handles and can pass truncated handle bits to _close.

Recommendation: Add a USE_WINDOWS_API branch that calls CloseHandle(*(HANDLE*)cur->handle) for stored Windows handles.

Referenced code: src/wolfsftp.c:4562-4566 (5 lines)


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

@yosuke-wolfssl yosuke-wolfssl force-pushed the fix/sftpFileHandle branch 2 times, most recently from f4d7522 to 1e44a52 Compare June 3, 2026 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants