Skip to content

Fix Windows SFTP open flags and wolfsshd -D argument parsing(f8827) - #1173

Open
miyazakh wants to merge 1 commit into
wolfSSL:masterfrom
miyazakh:f8827_WindowsSFTP
Open

Fix Windows SFTP open flags and wolfsshd -D argument parsing(f8827)#1173
miyazakh wants to merge 1 commit into
wolfSSL:masterfrom
miyazakh:f8827_WindowsSFTP

Conversation

@miyazakh

Copy link
Copy Markdown
Contributor

Summary

  • wolfSSH_SFTP_RecvOpen()'s Windows (USE_WINDOWS_API) path built dwCreationDisposition by OR-ing together OPEN_EXISTING / CREATE_ALWAYS bits, but CreateFile()'s creation-disposition parameter is a single enumerated value, not a bitmask. TRUNC and EXCL were also never wired up (left under #if 0), and APPEND access was missing. Add SFTP_WinCreationDisp() to resolve the SFTP CREAT/EXCL/TRUNC flag combination to the one correct CreateFile() disposition, and OR in FILE_APPEND_DATA for WOLFSSH_FXF_APPEND.
  • wolfsshd's -D (foreground/no-daemon) argument check on Windows compared cmdArgs[i] with WSTRCMP, but cmdArgs entries come from CommandLineToArgvW and are wide strings. Comparing them as narrow char* data meant -D was never recognized. Use wcscmp() against L"-D" instead.
  • Extend the WOLFSSH_TEST_INTERNAL regression-test plumbing in wolfsftp.c/wolfsftp.h to build under USE_WINDOWS_API too (it was previously guarded out on Windows), except for wolfSSH_SFTP_TestInvalidateHeadFd(), which stays POSIX-only since it manipulates a raw fd and Windows tracks a HANDLE instead.
  • Add TestSftpWindowsOpenFlagMatrix() (tests/regress.c), which walks the RecvOpen CREAT/EXCL/TRUNC flag matrix on Windows and checks both the open result and the resulting file state for each case:
    • WRITE only, no CREAT, missing file -> fails, file not created
    • WRITE|CREAT, missing file -> creates it (OPEN_ALWAYS)
    • WRITE|CREAT, existing file -> opens without truncating
    • WRITE|CREAT|TRUNC, existing file -> truncates immediately
    • WRITE|CREAT|EXCL, existing file -> fails
    • WRITE|CREAT|EXCL, missing file -> succeeds
    • READ|WRITE|CREAT, missing file -> creates it

Testing

  • Built and ran the full test suite on Windows via MSYS2 MinGW64 (_WIN32 -> USE_WINDOWS_API):

    PASS: tests/api.test.exe
    PASS: tests/testsuite.test.exe
    PASS: tests/kex.test.exe
    PASS: tests/regress.test.exe
    PASS: tests/unit.test.exe
    

    tests/regress.test.exe includes the new TestSftpWindowsOpenFlagMatrix(), exercising the corrected CreateFile() disposition logic end to end.

  • scripts/external.test and scripts/fwd.test are skipped on Windows as expected (external network / Unix-only port forwarding).

Copilot AI lite review requested due to automatic review settings August 18, 2026 11:10
@miyazakh miyazakh self-assigned this Aug 18, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes Windows-specific behavior in wolfSSH’s SFTP server open handling and wolfsshd argument parsing, and adds Windows regression coverage to prevent regressions in the SFTP open-flag matrix.

Changes:

  • Correct Windows SFTP RecvOpen creation-disposition handling by mapping CREAT/EXCL/TRUNC to a single valid CreateFile() disposition and wiring APPEND access.
  • Fix Windows wolfsshd -D detection by comparing CommandLineToArgvW() wide arguments with wcscmp(L"-D").
  • Extend internal SFTP test plumbing to build under USE_WINDOWS_API and add a Windows open-flag matrix regression test.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
wolfssh/wolfsftp.h Enables SFTP internal test hooks on Windows (except POSIX-fd invalidation helper).
src/wolfsftp.c Adds SFTP_WinCreationDisp() and fixes Windows RecvOpen access/disposition handling; adjusts internal test hook gating for Windows.
tests/regress.c Refactors shared SFTP reply assertion helper to build on Windows and adds TestSftpWindowsOpenFlagMatrix().
apps/wolfsshd/wolfsshd.c Fixes -D parsing on Windows by using wcscmp() with wide string literals.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

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

Comment thread src/wolfsftp.c
Comment on lines +2680 to +2687
if (reason & WOLFSSH_FXF_READ)
desiredAccess |= GENERIC_READ;
creationDisp |= OPEN_EXISTING;
}
if (reason & WOLFSSH_FXF_WRITE) {
if (reason & WOLFSSH_FXF_WRITE)
desiredAccess |= GENERIC_WRITE;
if (reason & WOLFSSH_FXF_CREAT)
creationDisp |= CREATE_ALWAYS;
#if 0
if (reason & WOLFSSH_FXF_TRUNC)
creationDisp |= TRUNCATE_EXISTING;
if (reason & WOLFSSH_FXF_EXCL)
creationDisp |= CREATE_NEW;
if (reason & WOLFSSH_FXF_APPEND)
desiredAccess |= FILE_APPEND_DATA;
#endif
}
if (reason & WOLFSSH_FXF_APPEND)
desiredAccess |= FILE_APPEND_DATA;

creationDisp = SFTP_WinCreationDisp(reason);

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1173

Scan targets checked: wolfssh-bugs, wolfssh-src

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

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

Comment thread src/wolfsftp.c
#endif
}
if (reason & WOLFSSH_FXF_APPEND)
desiredAccess |= FILE_APPEND_DATA;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] APPEND without WRITE grants append-only access, making RecvWrite ignore the requested offset · API contract violations

For WOLFSSH_FXF_APPEND without WOLFSSH_FXF_WRITE, desiredAccess becomes FILE_APPEND_DATA alone. Windows then treats the handle as append-only and WriteFile in wolfSSH_SFTP_RecvWrite (src/wolfsftp.c:4438) ignores the OVERLAPPED offset, so every write lands at EOF. Pre-PR this combination failed to open at all (creationDisp was 0). Adjacent to known finding #8827, which concerns the disposition bitmask, not access rights.

Related known finding #8827 (similar but distinct): Both affect Windows RecvOpen/CreateFile flag handling, but this candidate sets append-only access rights, causing later WriteFile offsets to be ignored; #8827 corrupts the creation-disposition selection by ORing enum values. The causes and required patches differ.

Fix: Only OR in FILE_APPEND_DATA alongside GENERIC_WRITE, or reject APPEND requests that omit WOLFSSH_FXF_WRITE.

Comment thread tests/regress.c
idx = 0;
SftpPutU32(pathSz, pkt + idx); idx += UINT32_SZ;
WMEMCPY(pkt + idx, path, pathSz); idx += pathSz;
SftpPutU32(WOLFSSH_FXF_WRITE | WOLFSSH_FXF_CREAT | WOLFSSH_FXF_TRUNC,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] New Windows flag-matrix test never exercises TRUNCATE_EXISTING or the APPEND path · Missing edge-case coverage on a function the PR also changed

The matrix covers only CREATE_NEW, CREATE_ALWAYS, OPEN_ALWAYS and OPEN_EXISTING. TRUNC is always paired with CREAT, so SFTP_WinCreationDisp's TRUNCATE_EXISTING branch (src/wolfsftp.c:2346) is never reached, and the new WOLFSSH_FXF_APPENDFILE_APPEND_DATA line is untested.

Fix: Add cases for WRITE|TRUNC on an existing file and for WRITE|APPEND|CREAT, asserting the resulting file size/content.

Comment thread src/wolfsftp.c
}
else {
if (reason & WOLFSSH_FXF_TRUNC)
disp = TRUNCATE_EXISTING;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚪ [Info] TRUNCATE_EXISTING returned for read-only requests, which CreateFile rejects · Logic errors

READ|TRUNC (no WRITE, no CREAT) yields TRUNCATE_EXISTING while desiredAccess is only GENERIC_READ; CreateFile requires GENERIC_WRITE for that disposition and fails with ERROR_INVALID_PARAMETER. The POSIX branch accepts the same request via open(O_RDONLY|O_TRUNC).

Related known finding #8827 (similar but distinct): Both concern Windows CreateFile disposition selection for SFTP open flags, but this candidate selects TRUNCATE_EXISTING without required write access, while #8827 ORs incompatible disposition enums in RecvOpen. They are different operations, causes, and fixes.

Fix: Only select TRUNCATE_EXISTING when write access was requested, otherwise fall back to OPEN_EXISTING.

Comment thread src/wolfsftp.c
#endif
}
if (reason & WOLFSSH_FXF_APPEND)
desiredAccess |= FILE_APPEND_DATA;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] WOLFSSH_FXF_APPEND still has no effect on Windows · SSH protocol violations

FILE_APPEND_DATA is a no-op for the spec-mandated WRITE|APPEND combination: GENERIC_WRITE (line 2683) already grants FILE_WRITE_DATA, which disables Windows append-only enforcement, and wolfSSH_SFTP_RecvWrite positions every write via an explicit OVERLAPPED offset (line 4438). SSH_FXF_APPEND remains unimplemented on Windows while the POSIX path honors it via O_APPEND. Adjacent to known finding #8827, which faults the creationDisp OR-ing; this is the separately-added access-mask line and needs a different fix.

Related known finding #8827 (similar but distinct): Both are Windows SFTP RecvOpen flag-handling defects, but #8827 faults CreateFile disposition bitwise OR-ing while this finding faults APPEND access/explicit-offset write semantics. The root causes and required patches differ.

Fix: Track APPEND in the file-handle entry and have RecvWrite seek to end-of-file (FILE_END) for such handles instead of using the client offset.

Comment thread apps/wolfsshd/wolfsshd.c
if (WSTRCMP((char*)(cmdArgs[i]), "-D") == 0) {
/* cmdArgs entries are wide strings (CommandLineToArgvW); compare
* as such instead of reinterpreting as narrow char data. */
if (wcscmp(cmdArgs[i], L"-D") == 0) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] -D path pairs CommandLineToArgvW's argc with the caller's argv array · NULL pointer dereference

Recognizing -D here makes the previously dead else { argv = (char**)wargv; } branch live, so mygetopt runs with argc from CommandLineToArgvW (line 2854) against the CRT-parsed array from main. The two parsers are independent: a smaller count silently drops trailing options such as -f \<config> (falling back to the default sshd_config), a larger count makes mygetopt dereference argv[myoptind][0] past the array's NULL terminator (wolfssh/test.h:354).

Fix: Leave argc as the caller-supplied value when using wargv, and only overwrite it with cmdArgC in the branch that builds argv from cmdArgs.

Comment thread tests/regress.c
!defined(NO_FILESYSTEM)
/* Walks the RecvOpen CREAT/EXCL/TRUNC flag matrix on Windows, checking both
* the open result and the resulting file state for each case. */
static void TestSftpWindowsOpenFlagMatrix(void)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 [Low] New Windows flag matrix omits the two branches the PR adds · Missing edge-case coverage on a function the PR also changed

The matrix covers only CREAT-bearing combinations, leaving both newly added behaviors unexercised: WOLFSSH_FXF_APPEND (desiredAccess |= FILE_APPEND_DATA, wolfsftp.c:2685) and TRUNC without CREAT (the TRUNCATE_EXISTING branch of SFTP_WinCreationDisp, wolfsftp.c:2346). Neither branch would fail the suite if wrong.

Fix: Add cases for WRITE|APPEND (assert data lands at end-of-file) and WRITE|TRUNC on an existing file (assert size drops to 0).

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