From 5b91bb584a0dd543def8507df6b391f3b3921556 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 13 Aug 2026 11:39:33 -0700 Subject: [PATCH 1/2] Split SFTP confinement from the start path - 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 --- examples/echoserver/echoserver.c | 23 ++- src/internal.c | 4 + src/wolfsftp.c | 324 ++++++++++++++++--------------- tests/api.c | 169 +++++++++++++++- tests/regress.c | 135 +++++++++++++ wolfssh/internal.h | 3 +- wolfssh/wolfsftp.h | 39 ++++ 7 files changed, 539 insertions(+), 158 deletions(-) diff --git a/examples/echoserver/echoserver.c b/examples/echoserver/echoserver.c index 082fad462..2dd03dc8c 100644 --- a/examples/echoserver/echoserver.c +++ b/examples/echoserver/echoserver.c @@ -2893,9 +2893,12 @@ static int wsUserAuth(byte authType, * * @param ssh WOLFSSH object to update * @param defaultSftpPath command line provided default SFTP path + * @param confine when set, also confine the session to that path, + * rather than only starting it there * @return 0 for success or error code */ -static int SetDefaultSftpPath(WOLFSSH* ssh, const char* defaultSftpPath) +static int SetDefaultSftpPath(WOLFSSH* ssh, const char* defaultSftpPath, + int confine) { char path[WOLFSSH_MAX_FILENAME]; char realPath[WOLFSSH_MAX_FILENAME]; @@ -2936,6 +2939,12 @@ static int SetDefaultSftpPath(WOLFSSH* ssh, const char* defaultSftpPath) ret = wolfSSH_SFTP_SetDefaultPath(ssh, realPath); } + /* the echoserver does not drop privileges, so -D is the only thing that + * bounds a session */ + if (ret == WS_SUCCESS && confine) { + ret = wolfSSH_SFTP_SetConfinePath(ssh, realPath); + } + return ret; } #endif @@ -2956,6 +2965,8 @@ static void ShowUsage(void) printf(" -N use non-blocking sockets\n"); #ifdef WOLFSSH_SFTP printf(" -d set the home directory for SFTP connections\n"); + printf(" -D confine SFTP connections to the home directory," + " rather than only starting them there\n"); #endif printf(" -j load in a SSH public key to accept from peer\n" " (user assumed in comment)\n"); @@ -3091,6 +3102,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) word16 port = wolfSshPort; char* readyFile = NULL; const char* defaultSftpPath = NULL; + int confineSftpPath = 0; char nonBlock = 0; #ifndef NO_FILESYSTEM char* userPubKey = NULL; @@ -3108,7 +3120,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) #endif if (argc > 0) { - const char* optlist = "?1a:d:efEp:R:Ni:j:i:I:J:K:P:k:b:x:m:c:s:G:H"; + const char* optlist = "?1a:d:DefEp:R:Ni:j:i:I:J:K:P:k:b:x:m:c:s:G:H"; myoptind = 0; while ((ch = mygetopt(argc, argv, optlist)) != -1) { switch (ch) { @@ -3171,6 +3183,10 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) defaultSftpPath = myoptarg; break; + case 'D': + confineSftpPath = 1; + break; + #ifndef NO_FILESYSTEM case 'j': userPubKey = myoptarg; @@ -3739,7 +3755,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) } #ifdef WOLFSSH_SFTP - if (SetDefaultSftpPath(ssh, defaultSftpPath) != 0) { + if (SetDefaultSftpPath(ssh, defaultSftpPath, confineSftpPath) != 0) { ES_ERROR("Couldn't store default sftp path.\n"); } #endif @@ -3838,6 +3854,7 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args) #endif (void)defaultSftpPath; + (void)confineSftpPath; WOLFSSL_RETURN_FROM_THREAD(0); } diff --git a/src/internal.c b/src/internal.c index 3e2c555cd..6cfc967ff 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1791,6 +1791,10 @@ void SshResourceFree(WOLFSSH* ssh, void* heap) WFREE(ssh->sftpDefaultPath, heap, DYNTYPE_STRING); ssh->sftpDefaultPath = NULL; } + if (ssh->sftpConfinePath) { + WFREE(ssh->sftpConfinePath, heap, DYNTYPE_STRING); + ssh->sftpConfinePath = NULL; + } #endif #ifdef WOLFSSH_TERM if (ssh->modes) { diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 1e2c1e1bc..86c30208a 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1488,7 +1488,8 @@ static int wolfSSH_SFTP_RecvRealPath(WOLFSSH* ssh, int reqId, byte* data, r[rSz] = '\0'; WLOG(WS_LOG_SFTP, "Real Path Request = %s", r); - /* If the default path isn't set, try to get it. */ + /* If the start path isn't set, try to get it. Only the start path - a + * first REALPATH must not confine a session the server left unconfined. */ if (ssh->sftpDefaultPath == NULL) { char wd[WOLFSSH_MAX_FILENAME]; @@ -1516,7 +1517,7 @@ static int wolfSSH_SFTP_RecvRealPath(WOLFSSH* ssh, int reqId, byte* data, } } - /* If the default path still isn't set, send error to peer. */ + /* If the start path still isn't set, send error to peer. */ if (ssh->sftpDefaultPath == NULL) { WLOG(WS_LOG_SFTP, "Unable to get current working directory"); if (wolfSSH_SFTP_CreateStatus(ssh, WOLFSSH_FTP_FAILURE, reqId, @@ -1938,18 +1939,22 @@ int wolfSSH_SFTP_CreateStatus(WOLFSSH* ssh, word32 status, word32 reqId, * the source path value, copy the path from the data stream into a local * array and use that as the source. * - * @param defaultPath pointer to the defaultPath - * @param data input data stream at the location of the path name - * @param sz size of the path name in bytes - * @param s destination buffer for the Real Path - * @param sSz size of s in bytes - * @return 0 for success or negative error code + * A relative request resolves against the session's start path; the result + * must stay inside the confinement root, when one is set (see wolfsftp.h). + * + * @param ssh session supplying the start path and the confinement root + * @param data input data stream at the location of the path name + * @param sz size of the path name in bytes + * @param s destination buffer for the Real Path + * @param sSz size of s in bytes + * @return 0 for success or negative error code */ -static int GetAndCleanPath(const char* defaultPath, +static int GetAndCleanPath(WOLFSSH* ssh, const byte* data, word32 sz, char* s, word32 sSz) { int ret; - word32 dpLen = 0; + word32 cpLen = 0; + const char* confinePath = ssh->sftpConfinePath; char r[WOLFSSH_MAX_FILENAME]; if (sz >= sizeof r) @@ -1957,33 +1962,32 @@ static int GetAndCleanPath(const char* defaultPath, WMEMCPY(r, data, sz); r[sz] = '\0'; - ret = wolfSSH_RealPath(defaultPath, r, s, sSz); - if (ret == WS_SUCCESS && defaultPath != NULL) { - /* defaultPath is stored in canonical form by - * wolfSSH_SFTP_SetDefaultPath, so a direct prefix compare against the - * canonical resolved request path enforces confinement. */ - dpLen = (word32)WSTRLEN(defaultPath); + ret = wolfSSH_RealPath(ssh->sftpDefaultPath, r, s, sSz); + if (ret == WS_SUCCESS && confinePath != NULL) { + /* both sides are canonical - the root by SetConfinePath, the request + * by the RealPath call above - so a prefix compare confines it */ + cpLen = (word32)WSTRLEN(confinePath); /* strip trailing separator(s), but keep a lone "/" as-is */ - while (dpLen > 1 && WOLFSSH_SFTP_IS_DELIM(defaultPath[dpLen - 1])) { - dpLen--; - } - if (dpLen > 1) { - /* resolved path must equal the default path or be within its - * subtree. On Windows the filesystem is case-insensitive and the - * default path is canonicalized to GetCurrentDirectoryA()'s case, - * so compare case-insensitively there to avoid rejecting valid - * in-jail requests that differ only in case. */ + while (cpLen > 1 && WOLFSSH_SFTP_IS_DELIM(confinePath[cpLen - 1])) { + cpLen--; + } + if (cpLen > 1) { + /* the resolved path must be the root itself or sit under it. + * Windows paths are case-insensitive and the root keeps + * GetCurrentDirectoryA()'s case, so compare without case there + * rather than reject in-jail requests over case alone. */ #ifdef USE_WINDOWS_API - if (WSTRNCASECMP(s, defaultPath, dpLen) != 0 || + if (WSTRNCASECMP(s, confinePath, cpLen) != 0 || #else - if (WSTRNCMP(s, defaultPath, dpLen) != 0 || + if (WSTRNCMP(s, confinePath, cpLen) != 0 || #endif - (s[dpLen] != '\0' && !WOLFSSH_SFTP_IS_DELIM(s[dpLen]))) { + (s[cpLen] != '\0' && !WOLFSSH_SFTP_IS_DELIM(s[cpLen]))) { ret = WS_PERMISSIONS; } } else { - /* default path is "/" - only absolute paths are accepted */ + /* root is "/", the whole filesystem: unconfined, but still only + * absolute paths are accepted */ if (s[0] == '\0' || !WOLFSSH_SFTP_IS_DELIM(s[0])) { ret = WS_PERMISSIONS; } @@ -1991,27 +1995,24 @@ static int GetAndCleanPath(const char* defaultPath, } #ifdef WOLFSSH_HAVE_SYMLINK - if (ret == WS_SUCCESS && defaultPath != NULL && dpLen > 1) { - /* Defense in depth: the prefix check above only proves the normalized - * path string stays under the jail. Because wolfSSH_RealPath does not - * resolve symlinks, an in-jail symlink pointing outside the jail would - * pass that check and then be followed by the file operation, escaping - * confinement. Walk every path component below the jail root and - * reject if any existing component is a link (in-jail links are - * rejected too, which is the conservative, safe choice). A - * not-yet-created leaf is left to the operation so creates still - * work. */ + if (ret == WS_SUCCESS && confinePath != NULL && cpLen > 1) { + /* Defense in depth: the prefix check only proves the path string + * stays under the jail, and wolfSSH_RealPath does not resolve + * symlinks, so an in-jail link pointing out would pass it and then be + * followed. Reject any existing component below the root that is a + * link, in-jail targets included; a not-yet-created leaf is left to + * the operation so creates still work. */ word32 i; word32 sLen = (word32)WSTRLEN(s); char saved; - for (i = dpLen; i <= sLen && ret == WS_SUCCESS; i++) { + for (i = cpLen; i <= sLen && ret == WS_SUCCESS; i++) { /* act only at a component boundary (a separator) or the leaf */ if (i != sLen && !WOLFSSH_SFTP_IS_DELIM(s[i])) { continue; } /* the jail root itself is trusted; only inspect inside the jail */ - if (i <= dpLen) { + if (i <= cpLen) { continue; } saved = s[i]; @@ -2085,8 +2086,7 @@ int wolfSSH_SFTP_RecvRMDIR(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, - str, strSz, dir, sizeof(dir)); + ret = GetAndCleanPath(ssh, str, strSz, dir, sizeof(dir)); if (ret == 0) { #ifndef USE_WINDOWS_API @@ -2153,8 +2153,7 @@ int wolfSSH_SFTP_RecvMKDIR(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, - str, strSz, dir, sizeof(dir)); + ret = GetAndCleanPath(ssh, str, strSz, dir, sizeof(dir)); if (ret != WS_SUCCESS && ret != WS_PERMISSIONS) { return ret; } @@ -2381,8 +2380,7 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) goto cleanup; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, - str, strSz, dir, sizeof(dir)); + ret = GetAndCleanPath(ssh, str, strSz, dir, sizeof(dir)); if (ret == WS_PERMISSIONS) { WLOG(WS_LOG_SFTP, "Creating path for file to open failed"); rc = SFTP_SendStatus(ssh, WOLFSSH_FTP_PERMISSION, reqId, per); @@ -2612,8 +2610,7 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) goto cleanup; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, - str, strSz, dir, sizeof(dir)); + ret = GetAndCleanPath(ssh, str, strSz, dir, sizeof(dir)); if (ret == WS_PERMISSIONS) { WLOG(WS_LOG_SFTP, "Creating path for file to open failed"); rc = SFTP_SendStatus(ssh, WOLFSSH_FTP_PERMISSION, reqId, per); @@ -2833,8 +2830,7 @@ int wolfSSH_SFTP_RecvOpenDir(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, - str, strSz, dir, sizeof(dir)); + ret = GetAndCleanPath(ssh, str, strSz, dir, sizeof(dir)); if (ret == WS_PERMISSIONS) { rc = SFTP_SendStatus(ssh, WOLFSSH_FTP_PERMISSION, reqId, per); return (rc == WS_SUCCESS) ? WS_BAD_FILE_E : rc; @@ -2961,11 +2957,11 @@ int wolfSSH_SFTP_RecvOpenDir(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - /* Resolve and confine the peer supplied path to sftpDefaultPath, the same - * way the POSIX branch does, so an absolute or UNC path cannot escape the - * configured root. When no default path is set this only normalizes the - * path, preserving the "/" drive listing special case below. */ - ret = GetAndCleanPath(ssh->sftpDefaultPath, data + idx, sz, + /* Resolve and confine the peer supplied path the same way the POSIX branch + * does, so an absolute or UNC path cannot escape the configured root. When + * the session is unconfined this only normalizes the path, preserving the + * "/" drive listing special case below. */ + ret = GetAndCleanPath(ssh, data + idx, sz, clean, sizeof(clean)); if (ret == WS_PERMISSIONS) { rc = SFTP_SendStatus(ssh, WOLFSSH_FTP_PERMISSION, reqId, per); @@ -4854,7 +4850,7 @@ int wolfSSH_SFTP_RecvRemove(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, str, strSz, + ret = GetAndCleanPath(ssh, str, strSz, name, sizeof(name)); if (ret == WS_SUCCESS) { @@ -4935,7 +4931,7 @@ int wolfSSH_SFTP_RecvRename(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) ret = WS_BUFFER_E; } if (ret == WS_SUCCESS) { - ret = GetAndCleanPath(ssh->sftpDefaultPath, str, strSz, + ret = GetAndCleanPath(ssh, str, strSz, old, sizeof(old)); } if (ret == WS_SUCCESS) { @@ -4945,7 +4941,7 @@ int wolfSSH_SFTP_RecvRename(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) } } if (ret == WS_SUCCESS) { - ret = GetAndCleanPath(ssh->sftpDefaultPath, str, strSz, + ret = GetAndCleanPath(ssh, str, strSz, name, sizeof(name)); } @@ -5894,7 +5890,7 @@ int wolfSSH_SFTP_RecvSTAT(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) } /* try to get file attributes and send back to client */ - ret = GetAndCleanPath(ssh->sftpDefaultPath, str, sz, name, sizeof(name)); + ret = GetAndCleanPath(ssh, str, sz, name, sizeof(name)); if (ret < 0) { if (ret == WS_PERMISSIONS) { statusType = WOLFSSH_FTP_PERMISSION; @@ -5985,8 +5981,7 @@ int wolfSSH_SFTP_RecvLSTAT(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, - str, strSz, name, sizeof(name)); + ret = GetAndCleanPath(ssh, str, strSz, name, sizeof(name)); if (ret < 0) { if (ret == WS_PERMISSIONS) { statusType = WOLFSSH_FTP_PERMISSION; @@ -6200,7 +6195,7 @@ int wolfSSH_SFTP_RecvSetSTAT(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) return WS_BUFFER_E; } - ret = GetAndCleanPath(ssh->sftpDefaultPath, str, strSz, name, sizeof(name)); + ret = GetAndCleanPath(ssh, str, strSz, name, sizeof(name)); if (ret != WS_SUCCESS) { if (ret == WS_PERMISSIONS) { type = WOLFSSH_FTP_PERMISSION; @@ -8985,120 +8980,143 @@ int wolfSSH_SFTP_Close(WOLFSSH* ssh, byte* handle, word32 handleSz) } -/* Sets the default path that SFTP will start a user in. - * - * Setting a default path other than "/" also confines the session to that - * subtree: requests resolving outside it are rejected with WS_PERMISSIONS - * (see GetAndCleanPath). Because paths are resolved lexically and the result - * cannot prove a link stays in-jail, confinement deliberately rejects ALL - * symbolic links encountered below the default path - including links whose - * target is itself inside the jail (e.g. "current -> releases/v3"). Deploy - * served trees without symlinks, or build with WOLFSSH_NO_SYMLINK_CHECK to - * disable the link check (which also removes the symlink-escape protection). - * - * Note this link check is best-effort, not a hard security boundary: it is a - * time-of-check/time-of-use (TOCTOU) check. GetAndCleanPath inspects each - * path component, then the operation acts on the same path by name in a later, - * separate call. An attacker able to write inside the jail concurrently and - * as the same user the server runs file operations as could swap a validated - * component for a symlink in that window and escape. wolfSSH services a single - * SFTP session's requests serially, so a session cannot race its own - * check-then-use; this requires a separate concurrent writer. For hostile - * multi-tenant deployments, confine the session with an OS-level mechanism - * (e.g. chroot and dropped privileges) and treat this check as defense in - * depth: it reliably blocks static (non-racing) in-jail symlinks but cannot - * close the concurrent-swap race portably (the *at/O_NOFOLLOW primitives the - * full fix needs do not exist across all supported filesystems). - * - * path NULL-terminated string specifying the default/base path - * the SFTP session should start in. If path is NULL, the - * existing default path (if any) is left unchanged. +/* Canonicalize path into out, which must be at least WOLFSSH_MAX_FILENAME + * bytes. Both paths are stored canonical so the confinement check stays a + * plain prefix compare. A relative path is resolved against the working + * directory; canonicalizing e.g. "." lexically would collapse it to "/" and + * leave confinement effectively disabled. * * returns WS_SUCCESS on success */ -int wolfSSH_SFTP_SetDefaultPath(WOLFSSH* ssh, const char* path) +static int CanonicalizePath(WOLFSSH* ssh, const char* path, + char* out, word32 outSz) { int ret = WS_SUCCESS; - word32 canonSz; - const char* canon = NULL; - char* newPath = NULL; char in[WOLFSSH_MAX_FILENAME]; char cwd[WOLFSSH_MAX_FILENAME]; - char real[WOLFSSH_MAX_FILENAME]; #ifdef USE_WINDOWS_API DWORD cwdLen; #endif - if (ssh == NULL) - return WS_BAD_ARGUMENT; + /* only the WGETCWD ports that take a filesystem handle use ssh */ + WOLFSSH_UNUSED(ssh); - if (path != NULL) { - /* Store the default path in canonical form so the SFTP confinement - * check (GetAndCleanPath) can compare it directly against canonicalized - * request paths without re-canonicalizing per request. A relative path - * is resolved against the current working directory; a purely lexical - * canonicalization of e.g. "." would collapse to "/" and leave - * confinement effectively disabled. */ - if (WSTRLEN(path) >= sizeof(in)) { - return WS_BUFFER_E; - } - WSTRNCPY(in, path, sizeof(in)); - in[sizeof(in) - 1] = '\0'; + if (WSTRLEN(path) >= sizeof(in)) { + return WS_BUFFER_E; + } + WSTRNCPY(in, path, sizeof(in)); + in[sizeof(in) - 1] = '\0'; - if (!WOLFSSH_SFTP_IS_DELIM(in[0]) && - !WOLFSSH_SFTP_IS_WINPATH((word32)WSTRLEN(in), in)) { - /* relative: resolve against the canonicalized working directory */ + if (!WOLFSSH_SFTP_IS_DELIM(in[0]) && + !WOLFSSH_SFTP_IS_WINPATH((word32)WSTRLEN(in), in)) { + /* relative: resolve against the canonicalized working directory */ #ifdef WOLFSSH_ZEPHYR - WSTRNCPY(cwd, CONFIG_WOLFSSH_SFTP_DEFAULT_DIR, sizeof cwd); + WSTRNCPY(cwd, CONFIG_WOLFSSH_SFTP_DEFAULT_DIR, sizeof cwd); #elif !defined(USE_WINDOWS_API) - if (WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1) == NULL) { - ret = WS_INVALID_PATH_E; - } + if (WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1) == NULL) { + ret = WS_INVALID_PATH_E; + } #else - /* GetCurrentDirectoryA returns the number of chars written on - * success, or the required size (including the NUL) when the - * buffer is too small; treat zero or an over-long result as a - * failure so a truncated cwd is never canonicalized. */ - cwdLen = GetCurrentDirectoryA(sizeof(cwd) - 1, cwd); - if (cwdLen == 0 || cwdLen >= (DWORD)(sizeof(cwd) - 1)) { - ret = WS_INVALID_PATH_E; - } -#endif - if (ret == WS_SUCCESS) { - cwd[sizeof(cwd) - 1] = '\0'; - ret = wolfSSH_RealPath(NULL, cwd, real, sizeof(real)); - } - if (ret == WS_SUCCESS) { - ret = wolfSSH_RealPath(real, in, cwd, sizeof(cwd)); - canon = cwd; - } + /* GetCurrentDirectoryA returns the chars written, or the size + * needed (with the NUL) when the buffer is too small; treat zero or + * an over-long result as failure so a truncated cwd is never used */ + cwdLen = GetCurrentDirectoryA(sizeof(cwd) - 1, cwd); + if (cwdLen == 0 || cwdLen >= (DWORD)(sizeof(cwd) - 1)) { + ret = WS_INVALID_PATH_E; } - else { - ret = wolfSSH_RealPath(NULL, in, real, sizeof(real)); - canon = real; +#endif + if (ret == WS_SUCCESS) { + cwd[sizeof(cwd) - 1] = '\0'; + ret = wolfSSH_RealPath(NULL, cwd, out, outSz); } - if (ret == WS_SUCCESS) { - /* Allocate and populate the replacement first, then swap it in, - * freeing the previous path only on success. A failed allocation - * must leave the existing confinement base path intact rather than - * clear it (repeated calls, e.g. wolfsshd setting "/" then the - * user's home dir, also do not leak). */ - canonSz = (word32)WSTRLEN(canon) + 1; - newPath = (char*)WMALLOC(canonSz, ssh->ctx->heap, DYNTYPE_STRING); - if (newPath == NULL) { - ssh->error = WS_MEMORY_E; - ret = WS_FATAL_ERROR; + /* move the canonical cwd out of out, so out can take the + * relative path resolved against it */ + if (WSTRLEN(out) >= sizeof(cwd)) { + ret = WS_BUFFER_E; } else { - WSTRNCPY(newPath, canon, canonSz); - if (ssh->sftpDefaultPath != NULL) { - WFREE(ssh->sftpDefaultPath, ssh->ctx->heap, DYNTYPE_STRING); - } - ssh->sftpDefaultPath = newPath; + WSTRNCPY(cwd, out, sizeof(cwd)); + ret = wolfSSH_RealPath(cwd, in, out, outSz); } } } + else { + ret = wolfSSH_RealPath(NULL, in, out, outSz); + } + + return ret; +} + + +/* Replaces the path at *dst with a copy of canon. + * + * Builds the replacement before swapping it in and freeing the old one, so a + * failed allocation leaves the existing path intact rather than clearing it. + * Repeated calls, e.g. wolfsshd setting "/" then the user's home dir, do not + * leak. + * + * returns WS_SUCCESS on success + */ +static int StorePath(WOLFSSH* ssh, char** dst, const char* canon) +{ + word32 canonSz; + char* newPath; + + canonSz = (word32)WSTRLEN(canon) + 1; + newPath = (char*)WMALLOC(canonSz, ssh->ctx->heap, DYNTYPE_STRING); + if (newPath == NULL) { + ssh->error = WS_MEMORY_E; + return WS_FATAL_ERROR; + } + WSTRNCPY(newPath, canon, canonSz); + if (*dst != NULL) { + WFREE(*dst, ssh->ctx->heap, DYNTYPE_STRING); + } + *dst = newPath; + + return WS_SUCCESS; +} + + +/* Confines an SFTP session to a subtree, leaving the start path alone. + * See wolfssh/wolfsftp.h for the contract. */ +int wolfSSH_SFTP_SetConfinePath(WOLFSSH* ssh, const char* path) +{ + int ret; + char canon[WOLFSSH_MAX_FILENAME]; + + if (ssh == NULL) + return WS_BAD_ARGUMENT; + if (path == NULL) + return WS_SUCCESS; + + ret = CanonicalizePath(ssh, path, canon, sizeof(canon)); + if (ret == WS_SUCCESS) { + ret = StorePath(ssh, &ssh->sftpConfinePath, canon); + } + + return ret; +} + + +/* Sets the directory an SFTP session starts in. Confinement is separate; see + * wolfssh/wolfsftp.h for the contract. */ +int wolfSSH_SFTP_SetDefaultPath(WOLFSSH* ssh, const char* path) +{ + int ret; + char canon[WOLFSSH_MAX_FILENAME]; + + if (ssh == NULL) + return WS_BAD_ARGUMENT; + if (path == NULL) + return WS_SUCCESS; + + ret = CanonicalizePath(ssh, path, canon, sizeof(canon)); + if (ret == WS_SUCCESS) { + ret = StorePath(ssh, &ssh->sftpDefaultPath, canon); + } + return ret; } diff --git a/tests/api.c b/tests/api.c index 8a61ea2ee..840e22ec7 100644 --- a/tests/api.c +++ b/tests/api.c @@ -4376,6 +4376,7 @@ static void test_wolfSSH_SFTP_Confinement(void) argsCount = 0; args[argsCount++] = "."; args[argsCount++] = "-1"; + args[argsCount++] = "-D"; /* confine to the working directory */ args[argsCount++] = "-p"; args[argsCount++] = "0"; ser.argv = (char**)args; @@ -4589,6 +4590,165 @@ static void test_wolfSSH_SFTP_Confinement(void) } +/* A session given only a start path is NOT confined to it: the wolfsshd + * arrangement, where the OS bounds access instead. The echoserver runs + * without -D, so out-of-tree paths must be reachable - the inverse of + * test_wolfSSH_SFTP_Confinement, proving GetAndCleanPath enforces the + * confinement root and not the start path. */ +static void test_wolfSSH_SFTP_StartPathNotConfined(void) +{ +/* Staging the out-of-tree fixtures needs mkdtemp()/fopen(), as in + * test_wolfSSH_SFTP_Confinement, so this is hosted POSIX only. */ +#if !defined(WOLFSSH_ZEPHYR) && !defined(USE_WINDOWS_API) + func_args ser; + tcp_ready ready; + int argsCount; + WS_SOCKET_T clientFd; + const char* args[10]; + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + THREAD_TYPE serThread; + WS_SFTPNAME* ls = NULL; + WS_SFTP_FILEATRB atr; + int ret; + char curDir[] = "."; + char outRoot[] = "/tmp/wolfssh_startpath_XXXXXX"; + char outFile[WOLFSSH_MAX_FILENAME]; + char outDir[WOLFSSH_MAX_FILENAME]; + char startCwd[WOLFSSH_MAX_FILENAME]; + WFILE* fp = NULL; + + AssertNotNull(mkdtemp(outRoot)); + + /* A temp root inside the start directory (the test process's cwd) would + * make the paths below in-tree, proving nothing; skip instead. */ + WMEMSET(startCwd, 0, sizeof(startCwd)); + if (WGETCWD(NULL, startCwd, sizeof(startCwd) - 1) != NULL) { + size_t startLen = WSTRLEN(startCwd); + if (WSTRLEN(outRoot) >= startLen && + WSTRNCMP(outRoot, startCwd, startLen) == 0) { + WRMDIR(NULL, outRoot); + return; + } + } + + WSNPRINTF(outFile, sizeof(outFile), "%s/real_file", outRoot); + WSNPRINTF(outDir, sizeof(outDir), "%s/real_dir", outRoot); + AssertIntEQ(WFOPEN(NULL, &fp, outFile, "wb"), 0); + AssertNotNull(fp); + WFCLOSE(NULL, fp); + AssertIntEQ(WMKDIR(NULL, outDir, 0755), 0); + + WMEMSET(&ser, 0, sizeof(func_args)); + argsCount = 0; + args[argsCount++] = "."; + args[argsCount++] = "-1"; + args[argsCount++] = "-p"; /* no -D, so the session is unconfined */ + args[argsCount++] = "0"; + ser.argv = (char**)args; + ser.argc = argsCount; + ser.signal = &ready; + InitTcpReady(ser.signal); + ThreadStart(echoserver_test, (void*)&ser, &serThread); + WaitTcpReady(&ready); + + sftp_client_connect(&ctx, &ssh, ready.port); + AssertNotNull(ctx); + AssertNotNull(ssh); + + /* the session starts in the working directory */ + ls = wolfSSH_SFTP_LS(ssh, curDir); + AssertNotNull(ls); + wolfSSH_SFTPNAME_list_free(ls); + ls = NULL; + + /* and can still reach outside it: listing and stat both succeed */ + ls = wolfSSH_SFTP_LS(ssh, outDir); + AssertNotNull(ls); + wolfSSH_SFTPNAME_list_free(ls); + ls = NULL; + + WMEMSET(&atr, 0, sizeof(atr)); + AssertIntEQ(wolfSSH_SFTP_STAT(ssh, outFile, &atr), WS_SUCCESS); + + /* Drain any pending rekey before shutdown. */ + while (wolfSSH_get_error(ssh) == WS_REKEYING) + wolfSSH_worker(ssh, NULL); + + ret = AbsorbBenignReset(ssh, wolfSSH_shutdown(ssh)); +#if DEFAULT_HIGHWATER_MARK < 8000 + if (ret == WS_REKEYING) { + ret = WS_SUCCESS; + } +#endif + AssertIntEQ(ret, WS_SUCCESS); + clientFd = wolfSSH_get_fd(ssh); + WCLOSESOCKET(clientFd); + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + ThreadJoin(serThread); + FreeTcpReady(&ready); + + WREMOVE(NULL, outFile); + WRMDIR(NULL, outDir); + WRMDIR(NULL, outRoot); +#endif /* !WOLFSSH_ZEPHYR && !USE_WINDOWS_API */ +} + + +/* The start path and the confinement root are stored independently: setting + * one must not disturb the other, and the default path does not confine. */ +static void test_wolfSSH_SFTP_SetConfinePath(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + char longPath[WOLFSSH_MAX_FILENAME + 4]; + + AssertNotNull(ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL)); + AssertNotNull(ssh = wolfSSH_new(ctx)); + + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(NULL, "/"), WS_BAD_ARGUMENT); + + /* a root that does not fit the working buffer is rejected up front and + * leaves the session unconfined */ + WMEMSET(longPath, 'a', sizeof(longPath)); + longPath[0] = '/'; + longPath[WOLFSSH_MAX_FILENAME + 1] = '\0'; /* length == MAX_FILENAME + 1 */ + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, longPath), WS_BUFFER_E); + AssertNull(ssh->sftpConfinePath); + AssertNull(ssh->sftpDefaultPath); + + /* a NULL path leaves both settings alone */ + AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, NULL), WS_SUCCESS); + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, NULL), WS_SUCCESS); + AssertNull(ssh->sftpDefaultPath); + AssertNull(ssh->sftpConfinePath); + + /* a start path on its own does not confine the session */ + AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, "/tmp/../tmp/start"), + WS_SUCCESS); + AssertStrEQ(ssh->sftpDefaultPath, "/tmp/start"); + AssertNull(ssh->sftpConfinePath); + + /* a confinement root on its own does not move the start path */ + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, "/tmp/./jail"), WS_SUCCESS); + AssertStrEQ(ssh->sftpConfinePath, "/tmp/jail"); + AssertStrEQ(ssh->sftpDefaultPath, "/tmp/start"); + + /* each is replaceable without touching the other */ + AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, "/tmp/jail/sub"), WS_SUCCESS); + AssertStrEQ(ssh->sftpDefaultPath, "/tmp/jail/sub"); + AssertStrEQ(ssh->sftpConfinePath, "/tmp/jail"); + + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, "/var/jail2"), WS_SUCCESS); + AssertStrEQ(ssh->sftpConfinePath, "/var/jail2"); + AssertStrEQ(ssh->sftpDefaultPath, "/tmp/jail/sub"); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + /* Direct unit coverage for wolfSSH_SFTP_SetDefaultPath, exercising the new * canonicalization and error branches that test_wolfSSH_SFTP_Confinement only * reaches indirectly (it always passes an already-absolute realpath): @@ -4624,12 +4784,15 @@ static void test_wolfSSH_SFTP_SetDefaultPath(void) longPath[WOLFSSH_MAX_FILENAME + 1] = '\0'; /* length == MAX_FILENAME + 1 */ AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, longPath), WS_BUFFER_E); AssertNull(ssh->sftpDefaultPath); + AssertNull(ssh->sftpConfinePath); - /* An absolute path is stored in lexically canonical form */ + /* An absolute path is stored in lexically canonical form as the start + * path, and does not confine the session */ AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, "/tmp/../tmp/sdp"), WS_SUCCESS); AssertNotNull(ssh->sftpDefaultPath); AssertStrEQ(ssh->sftpDefaultPath, "/tmp/sdp"); + AssertNull(ssh->sftpConfinePath); /* A repeated call frees the previous path (no leak) and stores the new * one - the wolfsshd "/" then home-dir sequence */ @@ -4715,6 +4878,8 @@ static void test_wolfSSH_SFTP_PartialSend(void) { ; } static void test_wolfSSH_SFTP_ReKey(void) { ; } static void test_wolfSSH_SFTP_ReKey_NonBlock(void) { ; } static void test_wolfSSH_SFTP_Confinement(void) { ; } +static void test_wolfSSH_SFTP_StartPathNotConfined(void) { ; } +static void test_wolfSSH_SFTP_SetConfinePath(void) { ; } static void test_wolfSSH_SFTP_SetDefaultPath(void) { ; } static void test_wolfSSH_SFTP_SaveOfst(void) { ; } #endif /* WOLFSSH_SFTP && !NO_WOLFSSH_CLIENT && !SINGLE_THREADED */ @@ -6564,6 +6729,8 @@ int wolfSSH_ApiTest(int argc, char** argv) test_wolfSSH_SFTP_ReKey(); test_wolfSSH_SFTP_ReKey_NonBlock(); test_wolfSSH_SFTP_Confinement(); + test_wolfSSH_SFTP_StartPathNotConfined(); + test_wolfSSH_SFTP_SetConfinePath(); test_wolfSSH_SFTP_SetDefaultPath(); test_wolfSSH_SFTP_SaveOfst(); diff --git a/tests/regress.c b/tests/regress.c index f601c61d0..5484b0b24 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2900,6 +2900,7 @@ static void TestSftpForgedHandleRejected(void) WMEMSET(cwd, 0, sizeof(cwd)); AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1)); AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, cwd), WS_SUCCESS); + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, cwd), WS_SUCCESS); /* ---- positive control: legitimately open a file over SFTP ---- * RecvOpen assigns the first handle the per-session id {0,0}. */ @@ -3085,6 +3086,7 @@ static void TestSftpHandleNamespaceIsolation(void) WMEMSET(cwd, 0, sizeof(cwd)); AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1)); AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, cwd), WS_SUCCESS); + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, cwd), WS_SUCCESS); /* open a directory -> first id from the shared counter */ idx = 0; @@ -3236,6 +3238,7 @@ static void TestSftpHandleLimit(void) WMEMSET(cwd, 0, sizeof(cwd)); AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1)); AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, cwd), WS_SUCCESS); + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, cwd), WS_SUCCESS); /* open the cap's worth of handles against one file; all must succeed */ for (i = 0; i < WOLFSSH_MAX_SFTP_HANDLES; i++) { @@ -3369,6 +3372,7 @@ static void TestSftpDirHandleLimit(void) WMEMSET(cwd, 0, sizeof(cwd)); AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1)); AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, cwd), WS_SUCCESS); + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, cwd), WS_SUCCESS); /* open the cap's worth of handles on "."; all must succeed */ for (i = 0; i < WOLFSSH_MAX_SFTP_HANDLES; i++) { @@ -3462,6 +3466,7 @@ static void TestSftpCloseFailureRemovesHandle(void) WMEMSET(cwd, 0, sizeof(cwd)); AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1)); AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, cwd), WS_SUCCESS); + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, cwd), WS_SUCCESS); idx = 0; SftpPutU32((word32)WSTRLEN(path), pkt + idx); idx += UINT32_SZ; @@ -3501,6 +3506,134 @@ static void TestSftpCloseFailureRemovesHandle(void) wolfSSH_CTX_free(ctx); } +/* Sends an FXP_STAT for path and returns the handler's return code. */ +static int SftpStatPath(WOLFSSH* ssh, int reqId, const char* path) +{ + byte pkt[WOLFSSH_MAX_FILENAME + UINT32_SZ]; + word32 idx = 0; + word32 sz = (word32)WSTRLEN(path); + + SftpPutU32(sz, pkt); idx += UINT32_SZ; + WMEMCPY(pkt + idx, path, sz); idx += sz; + + return wolfSSH_SFTP_RecvSTAT(ssh, reqId, pkt, idx); +} + +/* An accepted STAT answers with FXP_ATTRS carrying the same request id. */ +static void AssertSftpAttrsReply(WOLFSSH* ssh, int reqId) +{ + const byte* reply; + word32 replySz; + + reply = wolfSSH_SFTP_TestRecvReply(ssh, &replySz); + AssertNotNull(reply); + AssertTrue(replySz >= WOLFSSH_SFTP_HEADER + UINT32_SZ); + AssertIntEQ(reply[LENGTH_SZ], WOLFSSH_FTP_ATTRS); + AssertIntEQ((int)SftpGetU32(reply + LENGTH_SZ + MSG_ID_SZ), reqId); +} + +/* The start path and the confinement root are separate settings: a relative + * request resolves against the start path, while the jail boundary is the + * confinement root. Start the session in a subdirectory of the root and + * confirm requests are judged against the root - a sibling of the start + * directory is reachable, anything above the root is not. The tests that pass + * the same directory to both setters cannot tell the two apart, so they would + * still pass if confinement went back to following the start path. */ +static void TestSftpStartPathInsideConfineRoot(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + int rid = 500; + char cwd[WOLFSSH_MAX_FILENAME]; + char root[WOLFSSH_MAX_FILENAME]; + char start[WOLFSSH_MAX_FILENAME]; + char sibling[WOLFSSH_MAX_FILENAME]; + char startFile[WOLFSSH_MAX_FILENAME]; + char sibFile[WOLFSSH_MAX_FILENAME]; + char nearMiss[WOLFSSH_MAX_FILENAME]; + WFILE* fp = NULL; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AssertIntEQ(wolfSSH_SFTP_TestRecvStateInit(ssh), WS_SUCCESS); + + WMEMSET(cwd, 0, sizeof(cwd)); + AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1)); + + /* the fixture paths below hang off the working directory; skip rather + * than test truncated paths if they would not fit */ + if (WSTRLEN(cwd) + 64 >= WOLFSSH_MAX_FILENAME) { + wolfSSH_SFTP_TestRecvStateFree(ssh); + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return; + } + + /* unique per-process fixture names (see TestSftpForgedHandleRejected) */ + WSNPRINTF(root, sizeof(root), "%s/wolfssh_confine_%d", cwd, (int)getpid()); + WSNPRINTF(start, sizeof(start), "%s/start", root); + WSNPRINTF(sibling, sizeof(sibling), "%s/sibling", root); + WSNPRINTF(startFile, sizeof(startFile), "%s/start_file", start); + WSNPRINTF(sibFile, sizeof(sibFile), "%s/sib_file", sibling); + WSNPRINTF(nearMiss, sizeof(nearMiss), "%s_evil", root); + + AssertIntEQ(WMKDIR(ssh->fs, root, 0755), 0); + AssertIntEQ(WMKDIR(ssh->fs, start, 0755), 0); + AssertIntEQ(WMKDIR(ssh->fs, sibling, 0755), 0); + AssertIntEQ(WFOPEN(ssh->fs, &fp, startFile, "wb"), 0); + AssertNotNull(fp); + WFCLOSE(ssh->fs, fp); + AssertIntEQ(WFOPEN(ssh->fs, &fp, sibFile, "wb"), 0); + AssertNotNull(fp); + WFCLOSE(ssh->fs, fp); + + /* start deeper than the jail: the session opens in start, the boundary + * stays at root */ + AssertIntEQ(wolfSSH_SFTP_SetDefaultPath(ssh, start), WS_SUCCESS); + AssertIntEQ(wolfSSH_SFTP_SetConfinePath(ssh, root), WS_SUCCESS); + + /* a relative request resolves against the start path: start_file exists + * only there, not at the root */ + AssertIntEQ(SftpStatPath(ssh, ++rid, "start_file"), WS_SUCCESS); + AssertSftpAttrsReply(ssh, rid); + + /* a sibling of the start directory is inside the root, so it is reachable + * both by absolute path and by climbing out of the start directory */ + AssertIntEQ(SftpStatPath(ssh, ++rid, sibFile), WS_SUCCESS); + AssertSftpAttrsReply(ssh, rid); + AssertIntEQ(SftpStatPath(ssh, ++rid, "../sibling/sib_file"), WS_SUCCESS); + AssertSftpAttrsReply(ssh, rid); + + /* the root itself is in bounds, reached relatively or absolutely */ + AssertIntEQ(SftpStatPath(ssh, ++rid, ".."), WS_SUCCESS); + AssertSftpAttrsReply(ssh, rid); + AssertIntEQ(SftpStatPath(ssh, ++rid, root), WS_SUCCESS); + AssertSftpAttrsReply(ssh, rid); + + /* above the root is out of bounds, however it is spelled */ + AssertIntEQ(SftpStatPath(ssh, ++rid, "../.."), WS_BAD_FILE_E); + AssertSftpStatusReply(ssh, rid, WOLFSSH_FTP_PERMISSION); + AssertIntEQ(SftpStatPath(ssh, ++rid, cwd), WS_BAD_FILE_E); + AssertSftpStatusReply(ssh, rid, WOLFSSH_FTP_PERMISSION); + AssertIntEQ(SftpStatPath(ssh, ++rid, "/"), WS_BAD_FILE_E); + AssertSftpStatusReply(ssh, rid, WOLFSSH_FTP_PERMISSION); + + /* a sibling of the root sharing its string prefix is not under it */ + AssertIntEQ(SftpStatPath(ssh, ++rid, nearMiss), WS_BAD_FILE_E); + AssertSftpStatusReply(ssh, rid, WOLFSSH_FTP_PERMISSION); + + (void)WREMOVE(ssh->fs, startFile); + (void)WREMOVE(ssh->fs, sibFile); + (void)WRMDIR(ssh->fs, start); + (void)WRMDIR(ssh->fs, sibling); + (void)WRMDIR(ssh->fs, root); + wolfSSH_SFTP_TestRecvStateFree(ssh); + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + #endif /* !NO_WOLFSSH_SERVER && !USE_WINDOWS_API && !NO_FILESYSTEM */ #if defined(WOLFSSL_NUCLEUS) && !defined(NO_WOLFSSH_MKTIME) @@ -5960,6 +6093,8 @@ int main(int argc, char** argv) #endif /* a failed close still drops the handle from the tracking list */ TestSftpCloseFailureRemovesHandle(); + /* confinement follows the confine root, not the start path */ + TestSftpStartPathInsideConfineRoot(); #endif #if defined(WOLFSSL_NUCLEUS) && !defined(NO_WOLFSSH_MKTIME) TestNucleusMonthConversion(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index e0f516bcd..f4c12e927 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1145,7 +1145,8 @@ struct WOLFSSH { byte realState; byte sftpInt; SFTP_OFST sftpOfst[WOLFSSH_MAX_SFTPOFST]; - char* sftpDefaultPath; + char* sftpDefaultPath; /* where a session starts, base for relative paths */ + char* sftpConfinePath; /* jail root, NULL or "/" for unconfined */ #ifndef NO_WOLFSSH_DIR WS_DIR_LIST* dirList; #endif diff --git a/wolfssh/wolfsftp.h b/wolfssh/wolfsftp.h index 72daf10e7..a7ae54669 100644 --- a/wolfssh/wolfsftp.h +++ b/wolfssh/wolfsftp.h @@ -214,7 +214,46 @@ WOLFSSH_LOCAL WS_SFTPNAME* wolfSSH_SFTP_ReadDir(WOLFSSH* ssh, byte* handle, word32 handleSz); WOLFSSH_LOCAL int wolfSSH_SFTP_OpenDir(WOLFSSH* ssh, byte* buf, word32 bufSz); +/* An SFTP session has two independent path settings: + * + * start path where the session begins and what relative requests resolve + * against. Grants and denies nothing. + * Set with wolfSSH_SFTP_SetDefaultPath. + * confinement the root the session is jailed to. Requests resolving + * outside it are rejected with WS_PERMISSIONS. No root, or a + * root of "/", leaves the session unconfined. + * Set with wolfSSH_SFTP_SetConfinePath. + * + * Keeping them separate lets a server start a session deep inside a jail + * (start /srv/data/user7, confine to /srv/data), confine without moving where + * the session opens, or do neither and let the OS bound access instead, as + * wolfsshd does by dropping to the authenticated user. + * + * For both, path is NULL-terminated, a NULL path leaves the current setting + * unchanged, and WS_SUCCESS is returned on success. + * + * Paths are resolved lexically, which cannot prove a link stays in-jail, so + * confinement rejects ALL symbolic links below the root - including ones + * pointing back inside it (e.g. "current -> releases/v3"). Serve trees without + * symlinks, or build with WOLFSSH_NO_SYMLINK_CHECK to drop the check, and the + * escape protection with it. + * + * The root itself is trusted and never checked, so a root reached through a + * symbolic link is as wide as that link's target. Give a root the server + * controls, with no symlink components. + * + * That link check is defense in depth, not a boundary: it is a TOCTOU check, + * inspecting each component before the operation later acts on the path by + * name. A concurrent writer inside the jail, running as the user the server + * does file operations as, could swap a checked component for a symlink in + * that window; a session cannot race itself, since wolfSSH serves one + * session's requests serially. The check reliably blocks static in-jail + * symlinks, but closing the race portably needs *at/O_NOFOLLOW primitives some + * supported filesystems lack. Hostile multi-tenant deployments want an + * OS-level jail (chroot plus dropped privileges). + */ WOLFSSH_API int wolfSSH_SFTP_SetDefaultPath(WOLFSSH* ssh, const char* path); +WOLFSSH_API int wolfSSH_SFTP_SetConfinePath(WOLFSSH* ssh, const char* path); WOLFSSH_API WS_SFTPNAME* wolfSSH_SFTP_RealPath(WOLFSSH* ssh, char* dir); WOLFSSH_API int wolfSSH_SFTP_Close(WOLFSSH* ssh, byte* handle, word32 handleSz); WOLFSSH_API int wolfSSH_SFTP_Open(WOLFSSH* ssh, char* dir, word32 reason, From b82a5ef94e5d1d9dde9c1f2c2d90d31d5c50d30e Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 13 Aug 2026 15:27:32 -0700 Subject: [PATCH 2/2] Build the confinement test fixture paths from the working directory - 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. --- tests/regress.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/tests/regress.c b/tests/regress.c index 5484b0b24..09c5a16b5 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3544,7 +3544,8 @@ static void TestSftpStartPathInsideConfineRoot(void) WOLFSSH_CTX* ctx; WOLFSSH* ssh; int rid = 500; - char cwd[WOLFSSH_MAX_FILENAME]; + /* short enough that any fixture suffix below still fits a full path */ + char cwd[WOLFSSH_MAX_FILENAME - 64]; char root[WOLFSSH_MAX_FILENAME]; char start[WOLFSSH_MAX_FILENAME]; char sibling[WOLFSSH_MAX_FILENAME]; @@ -3560,24 +3561,30 @@ static void TestSftpStartPathInsideConfineRoot(void) AssertIntEQ(wolfSSH_SFTP_TestRecvStateInit(ssh), WS_SUCCESS); WMEMSET(cwd, 0, sizeof(cwd)); - AssertNotNull(WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1)); /* the fixture paths below hang off the working directory; skip rather - * than test truncated paths if they would not fit */ - if (WSTRLEN(cwd) + 64 >= WOLFSSH_MAX_FILENAME) { + * than test truncated paths if it is too deep to leave room for them */ + if (WGETCWD(ssh->fs, cwd, sizeof(cwd) - 1) == NULL) { wolfSSH_SFTP_TestRecvStateFree(ssh); wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); return; } - /* unique per-process fixture names (see TestSftpForgedHandleRejected) */ - WSNPRINTF(root, sizeof(root), "%s/wolfssh_confine_%d", cwd, (int)getpid()); - WSNPRINTF(start, sizeof(start), "%s/start", root); - WSNPRINTF(sibling, sizeof(sibling), "%s/sibling", root); - WSNPRINTF(startFile, sizeof(startFile), "%s/start_file", start); - WSNPRINTF(sibFile, sizeof(sibFile), "%s/sib_file", sibling); - WSNPRINTF(nearMiss, sizeof(nearMiss), "%s_evil", root); + /* unique per-process fixture names (see TestSftpForgedHandleRejected), + * each spelled out from cwd so the lengths stay provably in bounds */ + #define CONFINE_ROOT "%s/wolfssh_confine_%d" + WSNPRINTF(root, sizeof(root), CONFINE_ROOT, cwd, (int)getpid()); + WSNPRINTF(start, sizeof(start), CONFINE_ROOT "/start", cwd, (int)getpid()); + WSNPRINTF(sibling, sizeof(sibling), CONFINE_ROOT "/sibling", + cwd, (int)getpid()); + WSNPRINTF(startFile, sizeof(startFile), CONFINE_ROOT "/start/start_file", + cwd, (int)getpid()); + WSNPRINTF(sibFile, sizeof(sibFile), CONFINE_ROOT "/sibling/sib_file", + cwd, (int)getpid()); + WSNPRINTF(nearMiss, sizeof(nearMiss), CONFINE_ROOT "_evil", + cwd, (int)getpid()); + #undef CONFINE_ROOT AssertIntEQ(WMKDIR(ssh->fs, root, 0755), 0); AssertIntEQ(WMKDIR(ssh->fs, start, 0755), 0);