Repository navigation
sftp: answer every refused server request - #1372
ejohnstown wants to merge 4 commits into
Conversation
The SFTP server handlers queue an SSH_FXP_STATUS for a request they refuse, so the client is never left waiting on its request id. SFTP_ParseAttributes_buffer rejects attribute flag bits outside the v3 set, which draft-ietf-secsh-filexfer-02 section 5 calls a protocol error. - OPEN, OPENDIR, MKDIR, REALPATH and REMOVE answer FAILURE for a path that does not resolve, such as one of WOLFSSH_MAX_FILENAME bytes - FSTAT and READDIR answer FAILURE for a wrong-size or unknown handle - READ clamps a length over WOLFSSH_MAX_SFTP_RW to a short read, as OpenSSH's sftp-server does and filexfer-02 section 3 allows - OPEN, MKDIR, SETSTAT and FSETSTAT answer BAD_MESSAGE for attributes that do not parse - Add TestSftpFailedRequestsAnswered and a REALPATH test hook Issue: F-17050, F-17051, F-17076, F-17077
There was a problem hiding this comment.
🟡 Changes recommended
The Windows OPEN branch still bypasses attribute validation, allowing malformed requests to reach WS_CreateFileA() and potentially create or truncate files instead of returning BAD_MESSAGE.
1 open issue
What changed in this PR
This PR improves SFTP server error handling so refused requests receive status replies rather than leaving clients waiting.
Changes:
- Adds replies for malformed requests, invalid paths, unknown handles, and unsupported attribute flags.
- Clamps oversized READ requests to short reads and corrects failure logging.
- Adds regression coverage and a REALPATH test hook.
| File | Description |
|---|---|
wolfssh/wolfsftp.h |
Declares the REALPATH test hook. |
tests/regress.c |
Tests refusal replies, malformed requests, and oversized reads. |
src/wolfsftp.c |
Updates status handling, attribute validation, read limits, and logging. |
Review effort: Balanced
RecvRemove keeps the return of WREMOVE, and the Nucleus debug block compares that against the NUF_* codes instead of ret, which still held WS_SUCCESS there. Issue: F-17052
A server handler that cannot parse its request, because a field runs past the end of the packet, queues an SSH_FX_BAD_MESSAGE status for the request id through SFTP_SendBadMessage instead of returning silently. - Covers REALPATH, OPEN, OPENDIR, MKDIR, RMDIR, REMOVE, READ, WRITE, READDIR, STAT, LSTAT, FSTAT, SETSTAT, FSETSTAT and CLOSE - The WRITE handle and RENAME path failures now say BAD_MESSAGE, not FAILURE - CLOSE reads its handle length with GetSize - TestSftpFailedRequestsAnswered sends truncated packets to each handler
RecvRMDIR answers a path GetAndCleanPath cannot resolve with FAILURE straight away, as RecvMKDIR does, so the failure log never prints the unfilled dir buffer. - TestSftpFailedRequestsAnswered sends RMDIR an over-long path
6084397 to
ff52359
Compare
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1372
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 3 of 3 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I checked whether removing the length caps in CLOSE, READDIR and REMOVE could allow out-of-bounds reads. They can't. CLOSE and READDIR still reject any handle whose size isn't WOLFSSH_HANDLE_ID_SZ. REMOVE passes the name to GetAndCleanPath, which rejects anything of WOLFSSH_MAX_FILENAME or longer before copying it.
Extended reasoning...
The PR changes error handling across most SFTP server request handlers in src/wolfsftp.c, adds a test hook to the public header and adds regression tests. One reason not to approve is the confirmed inline finding about the shared attribute parser, which client-side parsing also uses. The other is an open Copilot thread at src/wolfsftp.c:2830 that only the PR author resolved.
| /* flag bits outside the v3 set are a protocol error (filexfer-02 5) */ | ||
| if ((atr->flags & ~(word32)(WOLFSSH_FILEATRB_SIZE | WOLFSSH_FILEATRB_UIDGID | ||
| | WOLFSSH_FILEATRB_PERM | WOLFSSH_FILEATRB_TIME | ||
| | WOLFSSH_FILEATRB_EXT)) != 0) { | ||
| WLOG(WS_LOG_SFTP, "Unsupported attribute flags 0x%08X", atr->flags); | ||
| return WS_BUFFER_E; | ||
| } |
There was a problem hiding this comment.
🟡 (optional) wolfSSH SFTP clients now reject a whole server reply when an attribute block sets any flag bit outside the v3 set. Affected replies are READDIR/REALPATH name lists and STAT/LSTAT results. The new flags check at src/wolfsftp.c:7261 sits in the shared SFTP_ParseAttributes_buffer, so it also runs on client-side parsing at :7662 and :8198. Fix: apply the v3-flag check only on the server request paths (MKDIR, OPEN, SETSTAT, FSETSTAT), for example through a strict flag on the parser. Leave client reply parsing as lenient as the base, which ignored unknown bits.
Why this was flagged
The condition is a server that sets a flag bit other than SIZE/UIDGID/PERM/TIME/EXT in a v3 ATTRS or NAME entry. I could not enumerate such servers from this checkout. The PR describes the check as a server request-validation change. But SFTP_ParseAttributes_buffer (src/wolfsftp.c:7249) is also called by the client's NAME-list parser at src/wolfsftp.c:7662 and by the client's STAT/LSTAT reply handler at src/wolfsftp.c:8198. On the base branch, unknown bits were ignored and the defined fields parsed normally. After this change the parser returns WS_BUFFER_E, so the client's whole directory listing (wolfSSH_SFTPNAME_list_free plus an error) or its stat call fails. Before, it would have succeeded. No caller-side guard limits the new check to server requests.
Verification: The new check at src/wolfsftp.c:7261-7267 is in the shared parser SFTP_ParseAttributes_buffer. Line 7662 is the NAME-list parser used for READDIR and REALPATH replies. At 7673-7676 the whole list is then freed and NULL is returned, so one bad entry drops the entire directory listing. On the base, unknown bits were ignored and the defined fields still parsed.
The SFTP server sends an SSH_FXP_STATUS for every request it refuses,
so a client is never left waiting on a request id.
attributes that do not parse or carry flags outside the v3 set;
WRITE and RENAME parse failures change from FAILURE to BAD_MESSAGE
OpenSSH's sftp-server does
checks the right result
Issue: F-17050, F-17051, F-17052, F-17076, F-17077