Repository navigation
sftp: answer every refused server request - #1372
ejohnstown wants to merge 4 commits into
Conversation
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
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.
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. A request's attribute flag bits outside the v3 set are rejected, which draft-ietf-secsh-filexfer-02 section 5 calls a protocol error; reply parsing still ignores them. - 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
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
ff52359 to
533bece
Compare
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: Max
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