Skip to content

Split SFTP confinement from the start path - #1167

Open
ejohnstown wants to merge 2 commits into
wolfSSL:masterfrom
ejohnstown:confine
Open

Split SFTP confinement from the start path#1167
ejohnstown wants to merge 2 commits into
wolfSSL:masterfrom
ejohnstown:confine

Conversation

@ejohnstown

@ejohnstownejohnstown commented Aug 13, 2026

Copy link
Copy Markdown
Contributor
  • Add wolfSSH_SFTP_SetConfinePath and a separate sftpConfinePath, so the start path only says where a session begins, and only an explicit confinement root rejects out-of-tree requests.
  • Have GetAndCleanPath take the WOLFSSH and enforce the confinement root, resolving relative requests against the start path.
  • Factor the shared canonicalize-and-store work out of wolfSSH_SFTP_SetDefaultPath into CanonicalizePath and StorePath.
  • Give the echoserver -D to opt a session into confinement; without it the -d home directory only says where SFTP starts.
  • Document both settings, and the symlink and TOCTOU caveats, once in wolfsftp.h; cover the split in api.c and regress.c.

Issue: ZD-22308

CopilotAI 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 PR separates SFTP “start path” (relative-path base) from an explicit confinement root, so sessions can start in a directory without implicitly being jailed there, and only an explicit confinement root rejects out-of-tree requests.

Changes:

  • Adds wolfSSH_SFTP_SetConfinePath() and ssh->sftpConfinePath, and updates path resolution to enforce confinement only when explicitly set.
  • Refactors SFTP path canonicalization into shared helpers (CanonicalizePath, StorePath) used by both default/start and confinement setters.
  • Updates echoserver (-D) and expands tests to cover the new split semantics and regression behavior.

Reviewed changes

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

Show a summary per file
FileDescription
wolfssh/wolfsftp.hDocuments the split between start path and confinement root; declares wolfSSH_SFTP_SetConfinePath().
wolfssh/internal.hAdds sftpConfinePath to session state alongside sftpDefaultPath.
src/wolfsftp.cImplements confinement root storage, refactors canonicalization, and updates request path cleaning to enforce confinement.
src/internal.cFrees sftpConfinePath in session teardown.
examples/echoserver/echoserver.cAdds -D flag to opt into confinement; otherwise -d only sets start path.
tests/regress.cUpdates regress tests to set both start path and confinement root where required.
tests/api.cAdds tests verifying “start path != confinement” and the independent setters’ behavior.

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

Comment threadwolfssh/wolfsftp.h

@wolfSSL-Fenrir-botwolfSSL-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 #1167

Scan targets checked:wolfssh-bugs, wolfssh-src

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

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

Comment threadtests/api.c
Comment threadsrc/wolfsftp.c
Comment threadsrc/wolfsftp.c
Comment threadtests/api.c
- Add wolfSSH_SFTP_SetConfinePath and a separate sftpConfinePath, so
the start path only says where a session begins, and only an
explicit confinement root rejects out-of-tree requests.
- Have GetAndCleanPath take the WOLFSSH and enforce the confinement
root, resolving relative requests against the start path.
- Factor the shared canonicalize-and-store work out of
wolfSSH_SFTP_SetDefaultPath into CanonicalizePath and StorePath.
- Give the echoserver -D to opt a session into confinement; without
it the -d home directory only says where SFTP starts.
- Document both settings, and the symlink and TOCTOU caveats, once in
wolfsftp.h, noting the confinement root itself is trusted.
- Cover the split in api.c, and in regress.c start a session in a
subdirectory of the confinement root: a sibling of the start
directory is reachable, anything above the root is not.
Issue: ZD-22308
- Chaining each path off root formats one 256 byte buffer into another,
which GCC cannot prove fits, failing every Linux build with
-Werror=format-truncation.
- Sizing cwd to leave room for the suffixes bounds the paths by the
declared sizes. A cwd too deep to fit fails WGETCWD and skips as
before.

@yosuke-wolfsslyosuke-wolfssl 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.

It looks good to me.
The jail root would be given by sftpConfinePath instead of sftpDefaultPath.
It's clearer design than previous one.
Let's give @JacobBarthelmeh a chance to review this as well

Sign up for freeto 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.

5 participants

@ejohnstown@yosuke-wolfssl@wolfSSL-Fenrir-bot@JacobBarthelmeh