diff --git a/auth.c b/auth.c index c2cffae3e..77497ffc5 100644 --- a/auth.c +++ b/auth.c @@ -527,7 +527,7 @@ auth_openfile(const char *file, struct passwd *pw, int strict_modes, strerror(errno)); return NULL; } - if (strict_modes && check_secure_file_permission(file, pw) != 0) { + if (strict_modes && check_secure_file_permission(file, pw, 0) != 0) { fclose(f); logit("Authentication refused."); auth_debug_add("Ignored %s", file_type); @@ -925,11 +925,20 @@ subprocess(const char *tag, struct passwd *pw, const char *command, restore_uid(); return 0; } +#ifdef WINDOWS + if (check_secure_file_permission(av[0], pw, 1) != 0) { + error("Permissions on %s:\"%s\" are too open", tag, av[0]); + restore_uid(); + return 0; + } +#else if (safe_path(av[0], &st, NULL, 0, errmsg, sizeof(errmsg)) != 0) { error("Unsafe %s \"%s\": %s", tag, av[0], errmsg); restore_uid(); return 0; } +#endif + /* Prepare to keep the child's stdout if requested */ if (pipe(p) == -1) { error("%s: pipe: %s", tag, strerror(errno)); @@ -938,6 +947,20 @@ subprocess(const char *tag, struct passwd *pw, const char *command, } restore_uid(); +#ifdef FORK_NOT_SUPPORTED + { + posix_spawn_file_actions_t actions; + pid = -1; + + if (posix_spawn_file_actions_init(&actions) != 0 || + posix_spawn_file_actions_adddup2(&actions, p[1], STDOUT_FILENO) != 0) + fatal("posix_spawn initialization failed"); + else if (__posix_spawn_asuser((pid_t*)&pid, av[0], &actions, NULL, av, NULL, pw->pw_name) != 0) + fatal("posix_spawn: %s", strerror(errno)); + + posix_spawn_file_actions_destroy(&actions); + } +#else switch ((pid = fork())) { case -1: /* error */ error("%s: fork: %s", tag, strerror(errno)); @@ -1004,6 +1027,7 @@ subprocess(const char *tag, struct passwd *pw, const char *command, default: /* parent */ break; } +#endif close(p[1]); if ((flags & SSH_SUBPROCESS_STDOUT_CAPTURE) == 0) diff --git a/auth2-pubkey.c b/auth2-pubkey.c index 97901b8f3..d342ca681 100644 --- a/auth2-pubkey.c +++ b/auth2-pubkey.c @@ -311,6 +311,12 @@ check_principals_line(struct ssh *ssh, char *cp, const struct sshkey_cert *cert, while (ep > cp && (*ep == '\n' || *ep == ' ' || *ep == '\t')) *ep-- = '\0'; +#ifdef SUPPORT_CRLF + /* account for \r at line end */ + if (*ep == '\r') + *ep-- = '\0'; +#endif + /* * If the line has internal whitespace then assume it has * key options. diff --git a/authfile.c b/authfile.c index 494fd24c6..0fd35b31e 100644 --- a/authfile.c +++ b/authfile.c @@ -154,7 +154,7 @@ sshkey_perm_ok(int fd, const char *filename) #endif #ifdef WINDOWS /*implement permission checks on Windows*/ - if(check_secure_file_permission(filename, NULL) != 0) { + if(check_secure_file_permission(filename, NULL, 0) != 0) { error("@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@"); error("@ WARNING: UNPROTECTED PRIVATE KEY FILE! @"); error("@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@"); diff --git a/contrib/win32/win32compat/fileio.c b/contrib/win32/win32compat/fileio.c index 45804e386..80b2a137a 100644 --- a/contrib/win32/win32compat/fileio.c +++ b/contrib/win32/win32compat/fileio.c @@ -906,31 +906,64 @@ fileio_lseek(struct w32_io* pio, unsigned __int64 offset, int origin) return 0; } -/* - * fdopen implementation - use with caution - * this implementation deviates from POSIX spec the following way - * - the underlying file descriptor is closed automatically - * hence no further POSIX io operations (read, write, close, etc) on the - * underlying file descriptor are supported - */ -FILE* -fileio_fdopen(struct w32_io* pio, const char *mode) +/* fdopen() to be used on pipe handles */ +static FILE* +fileio_fdopen_pipe(struct w32_io* pio, const char *mode) +{ + int fd_flags = 0; + FILE* ret; + debug4("fdopen - io:%p", pio); + + if (mode[1] == '\0') { + switch (*mode) { + case 'r': + fd_flags = _O_RDONLY; + break; + case 'w': + break; + case 'a': + fd_flags = _O_APPEND; + break; + default: + errno = ENOTSUP; + debug3("fdopen - ERROR unsupported mode %s", mode); + return NULL; + } + } + else { + errno = ENOTSUP; + debug3("fdopen - ERROR unsupported mode %s", mode); + return NULL; + } + + int fd = _open_osfhandle((intptr_t)pio->handle, fd_flags); + + if (fd == -1 || (ret = _fdopen(fd, mode)) == NULL) { + errno = EOTHER; + debug3("fdopen - ERROR:%d _open_osfhandle()", errno); + return NULL; + } + + // overwrite underlying win32 handle - its expected to be closed via fclose + // and close pio + pio->handle = NULL; + int w32_close(int); + w32_close(pio->table_index); + return ret; +} + +/* fdopen() to be used on file handles */ +static FILE* +fileio_fdopen_disk(struct w32_io* pio, const char *mode) { wchar_t *file_path, *wmode = NULL; FILE* ret = NULL; - DWORD type = 0; debug4("fdopen - io:%p", pio); if ((wmode = utf8_to_utf16(mode)) == NULL) goto cleanup; - /* for non-disk files, just return the descriptor */ - type = GetFileType(pio->handle); - if (type != FILE_TYPE_DISK) { - return _fdopen(pio->table_index, mode); - } - file_path = get_final_path_by_handle(pio->handle); if (!file_path) goto cleanup; @@ -950,6 +983,31 @@ cleanup: return ret; } +/* + * fdopen implementation - use with caution + * this implementation deviates from POSIX spec the following way + * - the underlying file descriptor is closed automatically + * hence no further POSIX io operations (read, write, close, etc) on the + * underlying file descriptor are supported + */ +FILE* +fileio_fdopen(struct w32_io* pio, const char *mode) +{ + DWORD type = 0; + + debug4("fdopen - io:%p", pio); + + type = GetFileType(pio->handle); + if (type == FILE_TYPE_DISK) { + return fileio_fdopen_disk(pio, mode); + } else if (type == FILE_TYPE_PIPE) { + return fileio_fdopen_pipe(pio, mode); + } else { + errno = ENOTSUP; + return NULL; + } +} + void fileio_on_select(struct w32_io* pio, BOOL rd) { diff --git a/contrib/win32/win32compat/w32-sshfileperm.c b/contrib/win32/win32compat/w32-sshfileperm.c index 257da2216..d8f6d4e4c 100644 --- a/contrib/win32/win32compat/w32-sshfileperm.c +++ b/contrib/win32/win32compat/w32-sshfileperm.c @@ -45,15 +45,14 @@ * Check the owner of the file is one of these types: Local Administrators groups, system account, current user account * Check the users have access permission to the file don't voilate the following rules: 1. no user other than local administrators group, system account, and pwd user have write permission on the file - 2. sshd account can only have read permission * Returns 0 on success and -1 on failure */ int -check_secure_file_permission(const char *input_path, struct passwd * pw) +check_secure_file_permission(const char *input_path, struct passwd * pw, int read_ok) { PSECURITY_DESCRIPTOR pSD = NULL; wchar_t * path_utf16 = NULL; - PSID owner_sid = NULL, user_sid = NULL; + PSID owner_sid = NULL, user_sid = NULL, ti_sid = NULL; PACL dacl = NULL; DWORD error_code = ERROR_SUCCESS; BOOL is_valid_sid = FALSE, is_valid_acl = FALSE; @@ -68,6 +67,8 @@ check_secure_file_permission(const char *input_path, struct passwd * pw) goto cleanup; } + ti_sid = get_sid("NT SERVICE\\TrustedInstaller"); + /*Get the owner sid of the file.*/ if ((error_code = GetNamedSecurityInfoW(path_utf16, SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, @@ -83,8 +84,9 @@ check_secure_file_permission(const char *input_path, struct passwd * pw) goto cleanup; } if (!IsWellKnownSid(owner_sid, WinBuiltinAdministratorsSid) && - !IsWellKnownSid(owner_sid, WinLocalSystemSid) && - !EqualSid(owner_sid, user_sid)) { + !IsWellKnownSid(owner_sid, WinLocalSystemSid) && + !EqualSid(owner_sid, user_sid) && + !(ti_sid && EqualSid(owner_sid, ti_sid))) { debug3("Bad owner on %S", path_utf16); ret = -1; goto cleanup; @@ -92,7 +94,6 @@ check_secure_file_permission(const char *input_path, struct passwd * pw) /* iterate all aces of the file to find out if there is voilation of the following rules: 1. no others than administrators group, system account, and current user account have write permission on the file - 2. sshd account can only have read permission */ for (DWORD i = 0; i < dacl->AceCount; i++) { PVOID current_ace = NULL; @@ -118,8 +119,12 @@ check_secure_file_permission(const char *input_path, struct passwd * pw) /*no need to check administrators group, pwd user account, and system account*/ if (IsWellKnownSid(current_trustee_sid, WinBuiltinAdministratorsSid) || - IsWellKnownSid(current_trustee_sid, WinLocalSystemSid) || - EqualSid(current_trustee_sid, user_sid)) { + IsWellKnownSid(current_trustee_sid, WinLocalSystemSid) || + EqualSid(current_trustee_sid, user_sid) || + (ti_sid && EqualSid(current_trustee_sid, ti_sid))) { + continue; + } else if (read_ok && (current_access_mask & (FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA)) == 0 ) { + /* if read is allowed, allow ACES that do not give write access*/ continue; } else { @@ -159,6 +164,8 @@ cleanup: LocalFree(pSD); if (user_sid) free(user_sid); + if (ti_sid) + free(ti_sid); if(path_utf16) free(path_utf16); return ret; diff --git a/readconf.c b/readconf.c index 1eb22bf0b..fcb1a490e 100644 --- a/readconf.c +++ b/readconf.c @@ -1810,7 +1810,7 @@ read_config_file_depth(const char *filename, struct passwd *pw, implementation on windows to make sure the config file is owned by a user, administrators group, or LOCALSYSTEM account and nobody else except Administrators group, LOCALSYSTEM, and file owner account has the write permission */ - if (check_secure_file_permission(filename, pw) != 0) + if (check_secure_file_permission(filename, pw, 1) != 0) fatal("Bad owner or permissions on %s", filename); #else struct stat sb; diff --git a/regress/pesterTests/AuthorizedKeysCommand.Tests.ps1 b/regress/pesterTests/AuthorizedKeysCommand.Tests.ps1 new file mode 100644 index 000000000..59626f146 --- /dev/null +++ b/regress/pesterTests/AuthorizedKeysCommand.Tests.ps1 @@ -0,0 +1,57 @@ +If ($PSVersiontable.PSVersion.Major -le 2) {$PSScriptRoot = Split-Path -Parent $MyInvocation.MyCommand.Path} +Import-Module $PSScriptRoot\CommonUtils.psm1 -Force +$tC = 1 +$tI = 0 +$suite = "authorizedKeysCommand" + +Describe "E2E scenarios for AuthorizedKeysCommand" -Tags "CI" { + BeforeAll { + if($OpenSSHTestInfo -eq $null) + { + Throw "`$OpenSSHTestInfo is null. Please run Set-OpenSSHTestEnvironment to set test environments." + } + + $server = $OpenSSHTestInfo["Target"] + $port = 47004 + $opensshbinpath = $OpenSSHTestInfo['OpenSSHBinPath'] + $ssouser = $OpenSSHTestInfo["SSOUser"] + $sshdconfig = Join-Path $Global:OpenSSHTestInfo["ServiceConfigDir"] sshd_config + + $testDir = Join-Path $OpenSSHTestInfo["TestDataPath"] $suite + if(-not (Test-Path $testDir)) + { + $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue + } + } + + BeforeEach { + $stderrFile=Join-Path $testDir "$tC.$tI.stderr.txt" + $stdoutFile=Join-Path $testDir "$tC.$tI.stdout.txt" + $logFile = Join-Path $testDir "$tC.$tI.log.txt" + } + + AfterEach {$tI++;} + + Context "$tC - basic test cases" { + + BeforeAll {$tI=1} + AfterAll{$tC++} + + It "$tC.$tI - keys command with %k argument" { + #override authorizedkeysfile location to an unknown location, so AuthorizedKeysCommand gets executed + $kcOutFile = Join-Path $testDir "$tC.$tI.kcout.txt" + Remove-Item -Force $kcOutFile -ErrorAction SilentlyContinue + $sshdArgs = "-d -f $sshdconfig -E $logFile -o `"AuthorizedKeysFile .fake/authorized_keys`"" + $sshdArgs += " -o `"AuthorizedKeysCommand=$env:windir\system32\cmd.exe /c echo ssh-ed25519 %k & whoami > $kcOutFile`"" + $sshdArgs += " -o `"AuthorizedKeysCommandUser=$ssouser`"" + $sshdArgs += " -o PasswordAuthentication=no" + Start-SSHDTestDaemon -WorkDir $opensshbinpath -Arguments $sshdArgs -Port $port + $o = ssh -p $port test_target echo 1234 + Stop-SSHDTestDaemon -Port $port + $o | Should Be "1234" + #check the command is run as AuthorizedKeysCommandUser + (gc $kcOutFile).Contains($ssouser) | Should Be $true + } + + } +} diff --git a/regress/pesterTests/CertAuth.Tests.ps1 b/regress/pesterTests/CertAuth.Tests.ps1 index f075b5ffc..f1bf9c570 100644 --- a/regress/pesterTests/CertAuth.Tests.ps1 +++ b/regress/pesterTests/CertAuth.Tests.ps1 @@ -15,6 +15,9 @@ Describe "E2E scenarios for certificate authentication" -Tags "CI" { $port = $OpenSSHTestInfo["Port"] $pkuser = $OpenSSHTestInfo["PubKeyUser"] $cakey = $OpenSSHTestInfo["CA_Private_Key"] + $opensshbinpath = $OpenSSHTestInfo['OpenSSHBinPath'] + $ssouser = $OpenSSHTestInfo["SSOUser"] + $sshdconfig = Join-Path $Global:OpenSSHTestInfo["ServiceConfigDir"] sshd_config $testDir = Join-Path $OpenSSHTestInfo["TestDataPath"] $suite if(-not (Test-Path $testDir)) @@ -61,6 +64,28 @@ Describe "E2E scenarios for certificate authentication" -Tags "CI" { $o | Should Be "1234" Remove-PasswordSetting } + + It "$tC.$tI - authenticate using certificate via AuthorizedPrincipalsCommand" { + $pcOutFile = Join-Path $testDir "$tC.$tI.pcout.txt" + $logFile = Join-Path $testDir "$tC.$tI.log.txt" + Remove-Item -Force $pcOutFile -ErrorAction SilentlyContinue + $sshdArgs = "-d -f $sshdconfig -E $logFile -o `"AuthorizedKeysFile .fake/authorized_keys`"" + $sshdArgs += " -o `"AuthorizedPrincipalsCommand=$env:windir\system32\cmd.exe /c echo otheruser& echo $pkuser& whoami > $pcOutFile`"" + $sshdArgs += " -o `"AuthorizedPrincipalsCommandUser=$ssouser`"" + $sshdArgs += " -o PasswordAuthentication=no" + + Start-SSHDTestDaemon -WorkDir $opensshbinpath -Arguments $sshdArgs -Port 47004 + + #set up SSH_ASKPASS for key passphrase + Add-PasswordSetting -Pass $keypassphrase + $o = ssh -i $user_key -p 47004 $pkuser@$server echo 2345 + Remove-PasswordSetting + + Stop-SSHDTestDaemon -Port 47004 + $o | Should Be "2345" + #check the command is run as AuthorizedPrincipalsCommandUser + (gc $pcOutFile).Contains($ssouser) | Should Be $true + } } } diff --git a/regress/pesterTests/Cfginclude.Tests.ps1 b/regress/pesterTests/Cfginclude.Tests.ps1 index 0639e04e8..0e43ba519 100644 --- a/regress/pesterTests/Cfginclude.Tests.ps1 +++ b/regress/pesterTests/Cfginclude.Tests.ps1 @@ -127,7 +127,7 @@ Describe "Tests for ssh config" -Tags "CI" { It "$tC.$tI-User SSHConfig-ReadConfig negative (others has permission)" { #setup - Repair-FilePermission -Filepath $userConfigFile -Owners $currentUserSid -FullAccessNeeded $adminsSid,$systemSid,$currentUserSid -ReadAccessNeeded $objUserSid -confirm:$false + Repair-FilePermission -Filepath $userConfigFile -Owners $currentUserSid -FullAccessNeeded $adminsSid,$systemSid,$currentUserSid,$objUserSid -confirm:$false #Run cmd /c "ssh test_target echo 1234 2> $logPath" diff --git a/regress/pesterTests/data/known_hosts b/regress/pesterTests/data/known_hosts index 5ff138c9a..f8b759a69 100644 --- a/regress/pesterTests/data/known_hosts +++ b/regress/pesterTests/data/known_hosts @@ -5,5 +5,6 @@ [localhost]:47002 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIMtJMxwn+iJU0X4+EC7PSj/cfcMbdP6ahhodtXx+6RHv sshtest_hostkey_ed25519 [localhost]:47002 ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDU+NcQ5NuRutQJoZVjDmP/vE6IYZOaE59FTUjaoZkuPl4prdOPgqAnCwSy9XtnfzPm/oe62SyYIHgj8wRzhqjMU8g8aGqfv9ryF+hpNXZrFYXIdkdxnubzfb4e70RRRoTH8P5vuY8sAn0FIRlV/3EDkSKBFy2W3InMTO6l8gbkzzkgbn1GLvH06QJVdb2PcHksSn7dJBVHWASYi3TJWWu4muI+ZNfothujxAHqjKTJuJ9apDZIc0tnkPmlifRmolSUS4OAH2KWZ+5Gwaj7gsB8bk4QuA+QCT60OCcuzCcy4FBuXvvXkM9MBe/P2KZjVLAn86SriRtoE4RI+9R9S7DV sshtest_hostkey_rsa [localhost]:47003 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIMtJMxwn+iJU0X4+EC7PSj/cfcMbdP6ahhodtXx+6RHv sshtest_hostkey_ed25519 +[localhost]:47004 ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIMtJMxwn+iJU0X4+EC7PSj/cfcMbdP6ahhodtXx+6RHv sshtest_hostkey_ed25519 ###OpenSSHE2ETests diff --git a/servconf.c b/servconf.c index 5846ed785..8f281337b 100644 --- a/servconf.c +++ b/servconf.c @@ -2044,7 +2044,7 @@ process_server_config_line(ServerOptions *options, char *line, linenum); len = strspn(cp, WHITESPACE); if (*activep && options->authorized_keys_command == NULL) { - if (cp[len] != '/' && strcasecmp(cp + len, "none") != 0) + if (!path_absolute(cp + len) && strcasecmp(cp + len, "none") != 0) fatal("%.200s line %d: AuthorizedKeysCommand " "must be an absolute path", filename, linenum); @@ -2070,7 +2070,7 @@ process_server_config_line(ServerOptions *options, char *line, len = strspn(cp, WHITESPACE); if (*activep && options->authorized_principals_command == NULL) { - if (cp[len] != '/' && strcasecmp(cp + len, "none") != 0) + if (!path_absolute(cp + len) && strcasecmp(cp + len, "none") != 0) fatal("%.200s line %d: " "AuthorizedPrincipalsCommand must be " "an absolute path", filename, linenum); diff --git a/sshfileperm.h b/sshfileperm.h index 09005ce85..1d13f40f1 100644 --- a/sshfileperm.h +++ b/sshfileperm.h @@ -25,5 +25,5 @@ #ifndef _SSH_FILE_PERM_H #define _SSH_FILE_PERM_H -int check_secure_file_permission(const char *, struct passwd *); +int check_secure_file_permission(const char *, struct passwd *, int); #endif /* _SSH_FILE_PERM_H */ \ No newline at end of file