From 10bbcef24d0c91cf551a35768906898936134b0e Mon Sep 17 00:00:00 2001 From: KirtiRamchandani Date: Sat, 8 Aug 2026 10:21:56 +0530 Subject: [PATCH 1/2] Resolve configured Windows shell paths safely --- auth.c | 38 ++++++- contrib/win32/win32compat/misc.c | 98 ++++++++++++++++++- contrib/win32/win32compat/misc_internal.h | 1 + .../win32compat/miscellaneous_tests.c | 36 +++++++ session.c | 41 +++++++- 5 files changed, 210 insertions(+), 4 deletions(-) diff --git a/auth.c b/auth.c index d6608e740f32..ab28de7d3880 100644 --- a/auth.c +++ b/auth.c @@ -132,9 +132,25 @@ allowed_user(struct ssh *ssh, struct passwd * pw) */ if (options.chroot_directory == NULL || strcasecmp(options.chroot_directory, "none") == 0) { +#ifdef WINDOWS + char *resolved_shell; +#endif char *shell = xstrdup((pw->pw_shell[0] == '\0') ? _PATH_BSHELL : pw->pw_shell); /* empty = /bin/sh */ +#ifdef WINDOWS + if ((resolved_shell = resolve_configured_user_path(shell, + pw->pw_name, 1)) == NULL) { + logit("User %.100s not allowed because shell %.100s " + "could not be resolved to an absolute path", + pw->pw_name, shell); + free(shell); + return 0; + } + free(shell); + shell = resolved_shell; +#endif + if (stat(shell, &st) == -1) { logit("User %.100s not allowed because shell %.100s " "does not exist", pw->pw_name, shell); @@ -570,8 +586,26 @@ getpwnamallow(struct ssh *ssh, const char *user) auth_close(as); #endif #endif - if (pw != NULL) - return (pwcopy(pw)); + if (pw != NULL) { +#ifdef WINDOWS + char *resolved_shell; +#endif + pw = pwcopy(pw); +#ifdef WINDOWS + if ((resolved_shell = resolve_configured_user_path(pw->pw_shell, + pw->pw_name, 1)) == NULL) { + free(pw->pw_name); + free(pw->pw_passwd); + free(pw->pw_dir); + free(pw->pw_shell); + free(pw); + return (NULL); + } + free(pw->pw_shell); + pw->pw_shell = resolved_shell; +#endif + return (pw); + } return (NULL); } diff --git a/contrib/win32/win32compat/misc.c b/contrib/win32/win32compat/misc.c index 861ee2d585e4..5e2dd5660114 100644 --- a/contrib/win32/win32compat/misc.c +++ b/contrib/win32/win32compat/misc.c @@ -37,6 +37,7 @@ #include #include #include +#include #include #include #include @@ -1417,6 +1418,102 @@ is_absolute_path(const char *path) return retVal; } +char * +resolve_configured_user_path(const char *path, const char *user, int require_absolute) +{ + wchar_t *path_utf16 = NULL, *expanded_path = NULL; + char *ret = NULL, *resolved = NULL; + HANDLE user_token = NULL; + DWORD expanded_len = 0, last_error = ERROR_SUCCESS; + + if (path == NULL || *path == '\0') { + errno = EINVAL; + return NULL; + } + + if ((path_utf16 = utf8_to_utf16(path)) == NULL) { + errno = ENOMEM; + goto cleanup; + } + + convertToBackslashW(path_utf16); + if (wcschr(path_utf16, L'%') != NULL) { + if (user != NULL && *user != '\0') { + if ((user_token = get_user_token(user, 1)) == NULL) { + errno = EOTHER; + goto cleanup; + } + + if (load_user_profile(user_token, (char *)user) != 0) + goto cleanup; + + expanded_len = ExpandEnvironmentStringsForUserW( + user_token, path_utf16, NULL, 0); + } else + expanded_len = ExpandEnvironmentStringsW(path_utf16, + NULL, 0); + + if (expanded_len == 0 || expanded_len > PATH_MAX) { + last_error = GetLastError(); + errno = last_error == ERROR_SUCCESS ? EINVAL : + errno_from_Win32Error(last_error); + goto cleanup; + } + + if ((expanded_path = calloc(expanded_len, sizeof(wchar_t))) == + NULL) { + errno = ENOMEM; + goto cleanup; + } + + if (user_token != NULL) { + if (!ExpandEnvironmentStringsForUserW(user_token, + path_utf16, expanded_path, expanded_len)) { + errno = errno_from_Win32LastError(); + goto cleanup; + } + } else if (ExpandEnvironmentStringsW(path_utf16, + expanded_path, expanded_len) == 0) { + errno = errno_from_Win32LastError(); + goto cleanup; + } + + if (wcschr(expanded_path, L'%') != NULL) { + errno = EINVAL; + goto cleanup; + } + } else { + expanded_path = path_utf16; + path_utf16 = NULL; + } + + convertToBackslashW(expanded_path); + if ((resolved = utf16_to_utf8(expanded_path)) == NULL) { + errno = ENOMEM; + goto cleanup; + } + + if (require_absolute && !is_absolute_path(resolved)) { + errno = EINVAL; + goto cleanup; + } + + ret = resolved; + resolved = NULL; + +cleanup: + if (user_token) + CloseHandle(user_token); + if (path_utf16) + free(path_utf16); + if (expanded_path) + free(expanded_path); + if (resolved) + free(resolved); + + return ret; +} + /* return -1 - in case of failure, 0 - success */ int create_directory_withsddl(wchar_t *path_w, wchar_t *sddl_w, BOOL check_permissions) @@ -2148,4 +2245,3 @@ strrstr(const char *inStr, const char *pattern) return last; } - diff --git a/contrib/win32/win32compat/misc_internal.h b/contrib/win32/win32compat/misc_internal.h index 5a43a992e820..cc0e7ed26f09 100644 --- a/contrib/win32/win32compat/misc_internal.h +++ b/contrib/win32/win32compat/misc_internal.h @@ -69,6 +69,7 @@ HANDLE get_user_token(const char* user, int impersonation); int load_user_profile(HANDLE user_token, char* user); int create_directory_withsddl(wchar_t *path, wchar_t *sddl, BOOL check_permissions); int is_absolute_path(const char *); +char * resolve_configured_user_path(const char *, const char *, int); int file_in_chroot_jail(HANDLE); int file_in_chroot_jail_helper(wchar_t*); PSID lookup_sid(const wchar_t* name_utf16, PSID psid, DWORD * psid_len); diff --git a/regress/unittests/win32compat/miscellaneous_tests.c b/regress/unittests/win32compat/miscellaneous_tests.c index 536e2a75e071..809d258b51cf 100644 --- a/regress/unittests/win32compat/miscellaneous_tests.c +++ b/regress/unittests/win32compat/miscellaneous_tests.c @@ -4,6 +4,7 @@ #include #include #include +#include #include "../test_helper/test_helper.h" #include "tests.h" @@ -420,6 +421,40 @@ test_build_commandline_string() TEST_DONE(); } +void +test_resolve_configured_user_path() +{ + char *out; + + TEST_START("configured path expansion"); + SetEnvironmentVariableA("OPENSSH_TEST_ROOT", "C:\\OpenSSHTest"); + out = resolve_configured_user_path("%OPENSSH_TEST_ROOT%\\pwsh.exe", + NULL, 1); + ASSERT_STRING_EQ(out, "C:\\OpenSSHTest\\pwsh.exe"); + free(out); + SetEnvironmentVariableA("OPENSSH_TEST_ROOT", NULL); + TEST_DONE(); + + TEST_START("configured path requires absolute result"); + out = resolve_configured_user_path("pwsh.exe", NULL, 1); + ASSERT_PTR_EQ(out, NULL); + ASSERT_INT_EQ(errno, EINVAL); + TEST_DONE(); + + TEST_START("configured path preserves explicit relative subsystem"); + out = resolve_configured_user_path("sftp-server.exe", NULL, 0); + ASSERT_STRING_EQ(out, "sftp-server.exe"); + free(out); + TEST_DONE(); + + TEST_START("configured path rejects unresolved environment"); + out = resolve_configured_user_path("%OPENSSH_MISSING_VAR%\\pwsh.exe", + NULL, 1); + ASSERT_PTR_EQ(out, NULL); + ASSERT_INT_EQ(errno, EINVAL); + TEST_DONE(); +} + void miscellaneous_tests() { @@ -432,4 +467,5 @@ miscellaneous_tests() test_chroot(); test_build_exec_command(); test_build_commandline_string(); + test_resolve_configured_user_path(); } diff --git a/session.c b/session.c index 0308f5bd7d8c..7ad3d9666b2d 100644 --- a/session.c +++ b/session.c @@ -96,6 +96,11 @@ #include "atomicio.h" #include "pal_doexec.h" +#ifdef WINDOWS +#include +#include "misc_internal.h" +#endif + #if defined(KRB5) && defined(USE_AFS) #include #endif @@ -1960,6 +1965,10 @@ session_subsystem_req(struct ssh *ssh, Session *s) struct stat st; int r, success = 0; char *prog, *cmd, *type; +#ifdef WINDOWS + char *resolved_prog = NULL, *resolved_cmd = NULL, *suffix; + size_t prog_len; +#endif u_int i; if ((r = sshpkt_get_cstring(ssh, &s->subsys, NULL)) != 0 || @@ -1976,6 +1985,32 @@ session_subsystem_req(struct ssh *ssh, Session *s) s->is_subsystem = SUBSYSTEM_INT_SFTP; debug("subsystem: %s", prog); } else { +#ifdef WINDOWS + if (strchr(prog, '%') != NULL) { + if ((resolved_prog = + resolve_configured_user_path(prog, + s->pw->pw_name, 1)) == NULL) { + debug("subsystem: cannot resolve %s", + prog); + break; + } + prog_len = strlen(prog); + if (strncmp(cmd, prog, prog_len) == 0) + suffix = cmd + prog_len; + else if (cmd[0] == '"' && + strncmp(cmd + 1, prog, prog_len) == + 0 && cmd[prog_len + 1] == '"') + suffix = cmd + prog_len + 2; + else + suffix = ""; + xasprintf(&resolved_cmd, + strchr(resolved_prog, ' ') == NULL ? + "%s%s" : "\"%s\"%s", + resolved_prog, suffix); + prog = resolved_prog; + cmd = resolved_cmd; + } +#endif if (stat(prog, &st) == -1) debug("subsystem: cannot stat %s: %s", prog, strerror(errno)); @@ -1990,6 +2025,10 @@ session_subsystem_req(struct ssh *ssh, Session *s) break; } } +#ifdef WINDOWS + free(resolved_prog); + free(resolved_cmd); +#endif if (!success) logit("subsystem request for %.100s by user %s failed, " @@ -2749,4 +2788,4 @@ do_setup_env_proxy(struct ssh *ssh, Session *s, const char *shell) { return do_setup_env(ssh, s, shell); } -#endif \ No newline at end of file +#endif From 9f72eeb5fdfb293367c12748bb0b997fadbd3955 Mon Sep 17 00:00:00 2001 From: KirtiRamchandani Date: Sat, 8 Aug 2026 12:36:36 +0530 Subject: [PATCH 2/2] Address Windows path resolution feedback --- auth.c | 6 +- contrib/win32/win32compat/misc.c | 232 ++++++++++++------ .../win32compat/miscellaneous_tests.c | 32 ++- session.c | 7 +- 4 files changed, 200 insertions(+), 77 deletions(-) diff --git a/auth.c b/auth.c index ab28de7d3880..8989b2f72a95 100644 --- a/auth.c +++ b/auth.c @@ -140,9 +140,9 @@ allowed_user(struct ssh *ssh, struct passwd * pw) #ifdef WINDOWS if ((resolved_shell = resolve_configured_user_path(shell, - pw->pw_name, 1)) == NULL) { + pw->pw_dir, 1)) == NULL) { logit("User %.100s not allowed because shell %.100s " - "could not be resolved to an absolute path", + "could not be resolved to a fully qualified path", pw->pw_name, shell); free(shell); return 0; @@ -593,7 +593,7 @@ getpwnamallow(struct ssh *ssh, const char *user) pw = pwcopy(pw); #ifdef WINDOWS if ((resolved_shell = resolve_configured_user_path(pw->pw_shell, - pw->pw_name, 1)) == NULL) { + pw->pw_dir, 1)) == NULL) { free(pw->pw_name); free(pw->pw_passwd); free(pw->pw_dir); diff --git a/contrib/win32/win32compat/misc.c b/contrib/win32/win32compat/misc.c index 5e2dd5660114..9c20423f9e58 100644 --- a/contrib/win32/win32compat/misc.c +++ b/contrib/win32/win32compat/misc.c @@ -37,7 +37,6 @@ #include #include #include -#include #include #include #include @@ -1418,98 +1417,193 @@ is_absolute_path(const char *path) return retVal; } -char * -resolve_configured_user_path(const char *path, const char *user, int require_absolute) +static int +is_fully_qualified_path(const char *path) { - wchar_t *path_utf16 = NULL, *expanded_path = NULL; - char *ret = NULL, *resolved = NULL; - HANDLE user_token = NULL; - DWORD expanded_len = 0, last_error = ERROR_SUCCESS; + char *slash; - if (path == NULL || *path == '\0') { + if (path == NULL || *path == '\0') + return 0; + if (*path == '\"' || *path == '\'') + path++; + + if (__isascii(path[0]) && isalpha(path[0]) && path[1] == ':' && + (path[2] == '\\' || path[2] == '/')) + return 1; + + if (path[0] != '\\' || path[1] != '\\') + return 0; + + if (_strnicmp(path, "\\\\?\\UNC\\", 8) == 0) + path += 8; + else if (_strnicmp(path, "\\\\?\\", 4) == 0) + return __isascii(path[4]) && isalpha(path[4]) && + path[5] == ':' && (path[6] == '\\' || path[6] == '/'); + else if (_strnicmp(path, "\\\\.\\", 4) == 0) + return 0; + else + path += 2; + + if (*path == '\0' || *path == '\\' || *path == '/') + return 0; + slash = strpbrk(path, "\\/"); + if (slash == NULL || slash == path || slash[1] == '\0' || + slash[1] == '\\' || slash[1] == '/') + return 0; + + return 1; +} + +static int +append_path_segment(char **buffer, size_t *capacity, size_t *length, + const char *segment, size_t segment_len) +{ + char *tmp; + size_t new_capacity; + + if (*length + segment_len + 1 > *capacity) { + new_capacity = *capacity == 0 ? 128 : *capacity; + while (*length + segment_len + 1 > new_capacity) + new_capacity *= 2; + if ((tmp = realloc(*buffer, new_capacity)) == NULL) { + errno = ENOMEM; + return -1; + } + *buffer = tmp; + *capacity = new_capacity; + } + + memcpy(*buffer + *length, segment, segment_len); + *length += segment_len; + (*buffer)[*length] = '\0'; + return 0; +} + +static int +append_profile_path(char **buffer, size_t *capacity, size_t *length, + const char *profile_path, const char *suffix) +{ + if (profile_path == NULL || *profile_path == '\0') { errno = EINVAL; - return NULL; + return -1; } + if (append_path_segment(buffer, capacity, length, profile_path, + strlen(profile_path)) != 0) + return -1; + if (suffix != NULL && append_path_segment(buffer, capacity, length, + suffix, strlen(suffix)) != 0) + return -1; + return 0; +} + +static int +append_configured_path_variable(char **buffer, size_t *capacity, + size_t *length, const char *name, size_t name_len, + const char *profile_path) +{ + char *name_buffer = NULL, *value = NULL; + size_t value_len; + int ret = -1; - if ((path_utf16 = utf8_to_utf16(path)) == NULL) { + if (name_len == 11 && _strnicmp(name, "USERPROFILE", name_len) == 0) + return append_profile_path(buffer, capacity, length, + profile_path, NULL); + if (name_len == 12 && _strnicmp(name, "LOCALAPPDATA", name_len) == 0) + return append_profile_path(buffer, capacity, length, + profile_path, "\\AppData\\Local"); + if (name_len == 7 && _strnicmp(name, "APPDATA", name_len) == 0) + return append_profile_path(buffer, capacity, length, + profile_path, "\\AppData\\Roaming"); + + if ((name_buffer = calloc(name_len + 1, 1)) == NULL) { errno = ENOMEM; - goto cleanup; + return -1; } + memcpy(name_buffer, name, name_len); - convertToBackslashW(path_utf16); - if (wcschr(path_utf16, L'%') != NULL) { - if (user != NULL && *user != '\0') { - if ((user_token = get_user_token(user, 1)) == NULL) { - errno = EOTHER; - goto cleanup; - } + if (_dupenv_s(&value, &value_len, name_buffer) != 0 || + value == NULL) { + errno = EINVAL; + goto cleanup; + } + ret = append_path_segment(buffer, capacity, length, value, + strlen(value)); - if (load_user_profile(user_token, (char *)user) != 0) - goto cleanup; +cleanup: + free(name_buffer); + free(value); + return ret; +} - expanded_len = ExpandEnvironmentStringsForUserW( - user_token, path_utf16, NULL, 0); - } else - expanded_len = ExpandEnvironmentStringsW(path_utf16, - NULL, 0); +static char * +expand_configured_path_variables(const char *path, const char *profile_path) +{ + char *buffer = NULL, *end; + size_t capacity = 0, length = 0; - if (expanded_len == 0 || expanded_len > PATH_MAX) { - last_error = GetLastError(); - errno = last_error == ERROR_SUCCESS ? EINVAL : - errno_from_Win32Error(last_error); - goto cleanup; + for (; *path != '\0'; path++) { + if (*path != '%') { + if (append_path_segment(&buffer, &capacity, &length, + path, 1) != 0) + goto cleanup; + continue; } - - if ((expanded_path = calloc(expanded_len, sizeof(wchar_t))) == - NULL) { - errno = ENOMEM; - goto cleanup; + if (path[1] == '%') { + if (append_path_segment(&buffer, &capacity, &length, + "%", 1) != 0) + goto cleanup; + path++; + continue; } - - if (user_token != NULL) { - if (!ExpandEnvironmentStringsForUserW(user_token, - path_utf16, expanded_path, expanded_len)) { - errno = errno_from_Win32LastError(); + if ((end = strchr(path + 1, '%')) == NULL) { + if (append_path_segment(&buffer, &capacity, &length, + path, 1) != 0) goto cleanup; - } - } else if (ExpandEnvironmentStringsW(path_utf16, - expanded_path, expanded_len) == 0) { - errno = errno_from_Win32LastError(); - goto cleanup; + continue; } - - if (wcschr(expanded_path, L'%') != NULL) { - errno = EINVAL; - goto cleanup; + if (end == path + 1) { + if (append_path_segment(&buffer, &capacity, &length, + "%", 1) != 0) + goto cleanup; + path = end; + continue; } - } else { - expanded_path = path_utf16; - path_utf16 = NULL; + if (append_configured_path_variable(&buffer, &capacity, + &length, path + 1, end - path - 1, profile_path) != 0) + goto cleanup; + path = end; } - convertToBackslashW(expanded_path); - if ((resolved = utf16_to_utf8(expanded_path)) == NULL) { - errno = ENOMEM; + if (buffer == NULL && append_path_segment(&buffer, &capacity, + &length, "", 0) != 0) goto cleanup; - } + return buffer; + +cleanup: + free(buffer); + return NULL; +} - if (require_absolute && !is_absolute_path(resolved)) { +char * +resolve_configured_user_path(const char *path, const char *profile_path, + int require_absolute) +{ + char *ret = NULL; + + if (path == NULL || *path == '\0') { errno = EINVAL; - goto cleanup; + return NULL; } - ret = resolved; - resolved = NULL; + if ((ret = expand_configured_path_variables(path, profile_path)) == NULL) + return NULL; + convertToBackslash(ret); -cleanup: - if (user_token) - CloseHandle(user_token); - if (path_utf16) - free(path_utf16); - if (expanded_path) - free(expanded_path); - if (resolved) - free(resolved); + if (require_absolute && !is_fully_qualified_path(ret)) { + errno = EINVAL; + free(ret); + return NULL; + } return ret; } diff --git a/regress/unittests/win32compat/miscellaneous_tests.c b/regress/unittests/win32compat/miscellaneous_tests.c index 809d258b51cf..df371b5af2d5 100644 --- a/regress/unittests/win32compat/miscellaneous_tests.c +++ b/regress/unittests/win32compat/miscellaneous_tests.c @@ -424,29 +424,55 @@ test_build_commandline_string() void test_resolve_configured_user_path() { - char *out; + char *out, *old_test_root = NULL; + size_t old_test_root_len = 0; TEST_START("configured path expansion"); + _dupenv_s(&old_test_root, &old_test_root_len, "OPENSSH_TEST_ROOT"); SetEnvironmentVariableA("OPENSSH_TEST_ROOT", "C:\\OpenSSHTest"); out = resolve_configured_user_path("%OPENSSH_TEST_ROOT%\\pwsh.exe", NULL, 1); ASSERT_STRING_EQ(out, "C:\\OpenSSHTest\\pwsh.exe"); free(out); - SetEnvironmentVariableA("OPENSSH_TEST_ROOT", NULL); + SetEnvironmentVariableA("OPENSSH_TEST_ROOT", old_test_root); + free(old_test_root); + TEST_DONE(); + + TEST_START("configured user profile path expansion"); + out = resolve_configured_user_path( + "%LOCALAPPDATA%\\Microsoft\\WindowsApps\\pwsh.exe", + "C:\\Users\\TestUser", 1); + ASSERT_STRING_EQ(out, + "C:\\Users\\TestUser\\AppData\\Local\\Microsoft\\WindowsApps\\pwsh.exe"); + free(out); TEST_DONE(); - TEST_START("configured path requires absolute result"); + TEST_START("configured path requires fully qualified result"); out = resolve_configured_user_path("pwsh.exe", NULL, 1); ASSERT_PTR_EQ(out, NULL); ASSERT_INT_EQ(errno, EINVAL); TEST_DONE(); + TEST_START("configured path rejects rooted drive-relative result"); + out = resolve_configured_user_path("\\Windows\\System32\\cmd.exe", + NULL, 1); + ASSERT_PTR_EQ(out, NULL); + ASSERT_INT_EQ(errno, EINVAL); + TEST_DONE(); + TEST_START("configured path preserves explicit relative subsystem"); out = resolve_configured_user_path("sftp-server.exe", NULL, 0); ASSERT_STRING_EQ(out, "sftp-server.exe"); free(out); TEST_DONE(); + TEST_START("configured path permits literal percent"); + out = resolve_configured_user_path("C:\\OpenSSH%%Test\\pwsh.exe", + NULL, 1); + ASSERT_STRING_EQ(out, "C:\\OpenSSH%Test\\pwsh.exe"); + free(out); + TEST_DONE(); + TEST_START("configured path rejects unresolved environment"); out = resolve_configured_user_path("%OPENSSH_MISSING_VAR%\\pwsh.exe", NULL, 1); diff --git a/session.c b/session.c index 7ad3d9666b2d..8430a48aac4c 100644 --- a/session.c +++ b/session.c @@ -1965,6 +1965,7 @@ session_subsystem_req(struct ssh *ssh, Session *s) struct stat st; int r, success = 0; char *prog, *cmd, *type; + const char *failure_reason = "subsystem not found"; #ifdef WINDOWS char *resolved_prog = NULL, *resolved_cmd = NULL, *suffix; size_t prog_len; @@ -1989,9 +1990,11 @@ session_subsystem_req(struct ssh *ssh, Session *s) if (strchr(prog, '%') != NULL) { if ((resolved_prog = resolve_configured_user_path(prog, - s->pw->pw_name, 1)) == NULL) { + s->pw->pw_dir, 1)) == NULL) { debug("subsystem: cannot resolve %s", prog); + failure_reason = + "configured subsystem path could not be resolved"; break; } prog_len = strlen(prog); @@ -2032,7 +2035,7 @@ session_subsystem_req(struct ssh *ssh, Session *s) if (!success) logit("subsystem request for %.100s by user %s failed, " - "subsystem not found", s->subsys, s->pw->pw_name); + "%s", s->subsys, s->pw->pw_name, failure_reason); return success; }