From 6a0e68c6d647f6db592484ba3ab22d79a2d4c681 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Wed, 10 Jun 2026 09:46:04 +0900 Subject: [PATCH] wolfsftp: reject symlink leaf in SFTP_RecvOpen --- src/wolfsftp.c | 53 +++++++++++++++++++++++++++++++++++++++++------ tests/api.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/port.h | 9 ++++++++ 3 files changed, 112 insertions(+), 6 deletions(-) diff --git a/src/wolfsftp.c b/src/wolfsftp.c index 33e4686aa..1abe9c01d 100644 --- a/src/wolfsftp.c +++ b/src/wolfsftp.c @@ -1816,6 +1816,22 @@ static int SFTP_IsSymlink(const char* path) #endif /* WOLFSSH_HAVE_SYMLINK */ +/* Length of the confining prefix of an SFTP default path: its length with any + * trailing separators stripped (a lone "/" is kept). A result > 1 means the + * session is confined to something deeper than the filesystem root. Defined + * once here so the confinement gate stays consistent between GetAndCleanPath + * and SFTP_SessionConfined. Caller must pass a non-NULL path. */ +static word32 SFTP_ConfinedPathLen(const char* defaultPath) +{ + word32 dpLen = (word32)WSTRLEN(defaultPath); + + while (dpLen > 1 && WOLFSSH_SFTP_IS_DELIM(defaultPath[dpLen - 1])) { + dpLen--; + } + return dpLen; +} + + /* * This is a wrapper around the function wolfSSH_RealPath. Since it modifies * the source path value, copy the path from the data stream into a local @@ -1845,11 +1861,7 @@ static int GetAndCleanPath(const char* defaultPath, /* 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); - /* strip trailing separator(s), but keep a lone "/" as-is */ - while (dpLen > 1 && WOLFSSH_SFTP_IS_DELIM(defaultPath[dpLen - 1])) { - dpLen--; - } + dpLen = SFTP_ConfinedPathLen(defaultPath); if (dpLen > 1) { /* resolved path must equal the default path or be within its * subtree. On Windows the filesystem is case-insensitive and the @@ -1911,6 +1923,22 @@ static int GetAndCleanPath(const char* defaultPath, } +#ifndef USE_WINDOWS_API +/* Returns 1 when the session is confined to a default path deeper than the + * filesystem root. This mirrors the gate GetAndCleanPath uses for its + * per-component symlink check, so the open-time symlink defenses stay limited + * to confined sessions and do not change behavior for servers that + * intentionally follow symlinks (the default, like OpenSSH's sftp-server). */ +static int SFTP_SessionConfined(const char* defaultPath) +{ + if (defaultPath == NULL) { + return 0; + } + return (SFTP_ConfinedPathLen(defaultPath) > 1) ? 1 : 0; +} +#endif /* !USE_WINDOWS_API */ + + /* Builds a status packet and queues it for send. * Returns WS_SUCCESS on success; ssh takes ownership of the allocated buffer. */ static int SFTP_SendStatus(WOLFSSH* ssh, byte type, int reqId, const char* msg) @@ -2271,6 +2299,7 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) int rc; int fdOpened = 0; int outOwnedBySsh = 0; + int confined; word32 outSz = sizeof(WFD) + UINT32_SZ + WOLFSSH_SFTP_HEADER; byte* out = NULL; @@ -2290,6 +2319,10 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) WLOG(WS_LOG_SFTP, "Receiving WOLFSSH_FTP_OPEN"); + /* only apply the symlink-follow defenses when the session is confined, so + * unconfined servers keep following symlinks as they did before */ + confined = SFTP_SessionConfined(ssh->sftpDefaultPath); + #if defined(MICROCHIP_MPLAB_HARMONY) || defined(FREESCALE_MQX) fd = WBADFILE; #else @@ -2369,7 +2402,8 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) WS_SFTP_FILEATRB fileAtr; WMEMSET(&fileAtr, 0, sizeof(fileAtr)); if (SFTP_GetAttributes(ssh->fs, - dir, &fileAtr, 0, ssh->ctx->heap) == WS_SUCCESS) { + dir, &fileAtr, (byte)confined, ssh->ctx->heap) + == WS_SUCCESS) { if ((fileAtr.per & FILEATRB_PER_MASK_TYPE) != FILEATRB_PER_FILE) { ssh->error = WS_SFTP_NOT_FILE_E; @@ -2391,6 +2425,13 @@ int wolfSSH_SFTP_RecvOpen(WOLFSSH* ssh, int reqId, byte* data, word32 maxSz) atr.per = 0644; } + /* when confined, refuse to follow a symlink leaf, closing the TOCTOU + * window between the type check above and the open below. No-op where + * the platform has no O_NOFOLLOW (WOLFSSH_O_NOFOLLOW is 0). */ + if (confined) { + m |= WOLFSSH_O_NOFOLLOW; + } + #ifdef MICROCHIP_MPLAB_HARMONY { WFILE* f = &fd; diff --git a/tests/api.c b/tests/api.c index 5131a7721..5d65e83dd 100644 --- a/tests/api.c +++ b/tests/api.c @@ -1702,6 +1702,16 @@ static void test_wolfSSH_SFTP_Confinement(void) * run. */ char escSymlink[] = "confine_symlink"; char escSymThru[WOLFSSH_MAX_FILENAME]; + /* an in-jail symlink whose leaf points at an in-jail regular file. The + * path resolves and stays inside the jail and the target is a real file. + * This block is guarded by WOLFSSH_HAVE_SYMLINK, so GetAndCleanPath's + * per-component link check is always present here and rejects the leaf + * symlink first; the assertion therefore verifies the end-to-end "open a + * symlinked file is refused" behavior. The new RecvOpen lstat/O_NOFOLLOW + * handling is defense-in-depth behind that check, closing the TOCTOU window + * between the type check and the open. */ + char leafTarget[] = "confine_leaf_target"; + char leafSymlink[] = "confine_leaf_symlink"; #endif WFILE* fp = NULL; int snLen; @@ -1792,6 +1802,14 @@ static void test_wolfSSH_SFTP_Confinement(void) /* stage an in-jail symlink pointing at the out-of-jail temp root */ WREMOVE(NULL, escSymlink); AssertIntEQ(symlink(escRoot, escSymlink), 0); + + /* stage an in-jail regular file and an in-jail symlink that targets it */ + WREMOVE(NULL, leafTarget); + AssertIntEQ(WFOPEN(NULL, &fp, leafTarget, "wb"), 0); + AssertNotNull(fp); + WFCLOSE(NULL, fp); + WREMOVE(NULL, leafSymlink); + AssertIntEQ(symlink(leafTarget, leafSymlink), 0); #endif #endif @@ -1954,6 +1972,42 @@ static void test_wolfSSH_SFTP_Confinement(void) AssertNotNull(ls); wolfSSH_SFTPNAME_list_free(ls); ls = NULL; + + /* Positive control: opening the in-jail regular file directly must still + * succeed, proving the symlink rejection below does not also block plain + * regular-file opens. */ + handleSz = WOLFSSH_MAX_HANDLE; + ret = wolfSSH_SFTP_Open(ssh, leafTarget, WOLFSSH_FXF_READ, NULL, + handle, &handleSz); + AssertIntEQ(ret, WS_SUCCESS); + AssertIntEQ(wolfSSH_SFTP_Close(ssh, handle, handleSz), WS_SUCCESS); + ls = wolfSSH_SFTP_LS(ssh, curDir); + AssertNotNull(ls); + wolfSSH_SFTPNAME_list_free(ls); + ls = NULL; + + /* opening a leaf that is itself a symlink to an in-jail regular file must + * be rejected, not silently followed to the target. + * + * Coverage gap (intentional): this block is compiled under + * WOLFSSH_HAVE_SYMLINK, so GetAndCleanPath's per-component check is always + * present and rejects the leaf symlink first. This assertion therefore + * verifies only the end-to-end "symlinked file open is refused" behavior; + * it still passes if the RecvOpen lstat/O_NOFOLLOW branch is removed, so it + * does not independently exercise that branch. Isolating it requires a + * build where the per-component check is compiled out but lstat/O_NOFOLLOW + * stay real (WOLFSSH_NO_SYMLINK_CHECK on POSIX), which is not part of the + * default test build. That branch was confirmed by hand by disabling the + * GetAndCleanPath walk and checking that the open is followed without the + * fix and refused with it. */ + handleSz = WOLFSSH_MAX_HANDLE; + ret = wolfSSH_SFTP_Open(ssh, leafSymlink, WOLFSSH_FXF_READ, NULL, + handle, &handleSz); + AssertIntNE(ret, WS_SUCCESS); + ls = wolfSSH_SFTP_LS(ssh, curDir); + AssertNotNull(ls); + wolfSSH_SFTPNAME_list_free(ls); + ls = NULL; #endif /* Positive case: a relative write op that resolves inside the jail must be @@ -2005,6 +2059,8 @@ static void test_wolfSSH_SFTP_Confinement(void) #if !defined(WOLFSSH_ZEPHYR) && !defined(USE_WINDOWS_API) #ifdef WOLFSSH_HAVE_SYMLINK WREMOVE(NULL, escSymlink); + WREMOVE(NULL, leafSymlink); + WREMOVE(NULL, leafTarget); #endif WRMDIR(NULL, escRoot); /* escSibling is non-empty only if this run created it (see staging above), diff --git a/wolfssh/port.h b/wolfssh/port.h index 7329a2121..feacacee3 100644 --- a/wolfssh/port.h +++ b/wolfssh/port.h @@ -1484,6 +1484,9 @@ extern "C" { #define WOLFSSH_O_CREAT O_CREAT #define WOLFSSH_O_TRUNC O_TRUNC #define WOLFSSH_O_EXCL O_EXCL + #ifdef O_NOFOLLOW + #define WOLFSSH_O_NOFOLLOW O_NOFOLLOW + #endif #define WOPEN(fs,f,m,p) open((f),(m),(p)) #define WCLOSE(fs,fd) close((fd)) @@ -1505,6 +1508,12 @@ extern "C" { #endif #endif /* WOLFSSH_SFTP or WOLFSSH_SCP */ +/* Platforms without an open(2)-style O_NOFOLLOW (or without a filesystem) get a + * harmless no-op so callers can OR it into the open flags unconditionally. */ +#ifndef WOLFSSH_O_NOFOLLOW + #define WOLFSSH_O_NOFOLLOW 0 +#endif + /* SFTP path confinement must reject symbolic links because wolfSSH_RealPath * only normalizes the path string and does not resolve them. Define * WOLFSSH_HAVE_SYMLINK on the filesystems that can actually store a link -