diff --git a/contrib/win32/win32compat/w32-sshfileperm.c b/contrib/win32/win32compat/w32-sshfileperm.c index 156afcd97162..9ddada23f650 100644 --- a/contrib/win32/win32compat/w32-sshfileperm.c +++ b/contrib/win32/win32compat/w32-sshfileperm.c @@ -46,6 +46,12 @@ #define COMMA_SPACE_LEN 2 #define BACKSLASH_LEN 1 +/* Access rights that are considered write-class for SSH file/folder security checks. + * No non-trusted principal may hold any of these rights on a secured path. */ +#define SSH_SECURE_WRITE_MASK \ + (FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA | \ + WRITE_DAC | WRITE_OWNER | DELETE) + extern int log_on_stderr; /* @@ -133,7 +139,7 @@ check_secure_file_permission(const char *input_path, struct passwd * pw, int rea 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 ) { + } else if (read_ok && (current_access_mask & SSH_SECURE_WRITE_MASK) == 0) { /* if read is allowed, allow ACES that do not give write access*/ continue; } else { @@ -250,7 +256,7 @@ check_secure_folder_permission(const wchar_t* path_utf16, int read_ok) IsWellKnownSid(current_trustee_sid, WinLocalSystemSid)) { continue; } - else if (read_ok && (current_access_mask & (FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA)) == 0) { + else if (read_ok && (current_access_mask & SSH_SECURE_WRITE_MASK) == 0) { /* if read is allowed, allow ACES that do not give write access*/ continue; } diff --git a/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 b/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 index 098957bf0c22..b9872e58425b 100644 --- a/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 +++ b/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 @@ -166,7 +166,7 @@ Describe "Tests for authorized_keys file permission" -Tags "CI" { #Run Start-SSHDTestDaemon -WorkDir $opensshbinpath -Arguments "-d -f $sshdconfig -o `"AuthorizedKeysFile .testssh/authorized_keys`" -E $sshdlog" -Port $port - ssh -p $port -E $sshlog $ssouser@$server echo 1234 + ssh -p $port -E $sshlog -o "PasswordAuthentication=no" $ssouser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 Stop-SSHDTestDaemon -Port $port sleep $sshdDelay @@ -184,7 +184,7 @@ Describe "Tests for authorized_keys file permission" -Tags "CI" { #Run Start-SSHDTestDaemon -workDir $opensshbinpath -Arguments "-d -f $sshdconfig -o `"AuthorizedKeysFile .testssh/authorized_keys`" -E $sshdlog" -Port $port - ssh -p $port -E $sshlog $ssouser@$server echo 1234 + ssh -p $port -E $sshlog -o "PasswordAuthentication=no" $ssouser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 Stop-SSHDTestDaemon -Port $port sleep $sshdDelay @@ -192,14 +192,55 @@ Describe "Tests for authorized_keys file permission" -Tags "CI" { $sshdlog | Should Contain "Authentication refused." } + It "$tC.$tI-authorized_keys-negative(other account has ChangePermissions on authorized_keys file)" -skip:$skip { + Repair-AuthorizedKeyPermission -Filepath $authorizedkeyPath -confirm:$false + $objPwdUserSid = Get-UserSid -User $PwdUser + Set-FilePermission -FilePath $authorizedkeyPath -User $objPwdUserSid -Perm "ChangePermissions" + + Start-SSHDTestDaemon -workDir $opensshbinpath -Arguments "-d -f $sshdconfig -o `"AuthorizedKeysFile .testssh/authorized_keys`" -E $sshdlog" -Port $port + ssh -p $port -E $sshlog -o "PasswordAuthentication=no" $ssouser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + Stop-SSHDTestDaemon -Port $port + sleep $sshdDelay + $sshlog | Should Contain "Permission denied" + $sshdlog | Should Contain "Authentication refused." + } + + It "$tC.$tI-authorized_keys-negative(other account has TakeOwnership on authorized_keys file)" -skip:$skip { + Repair-AuthorizedKeyPermission -Filepath $authorizedkeyPath -confirm:$false + $objPwdUserSid = Get-UserSid -User $PwdUser + Set-FilePermission -FilePath $authorizedkeyPath -User $objPwdUserSid -Perm "TakeOwnership" + + Start-SSHDTestDaemon -workDir $opensshbinpath -Arguments "-d -f $sshdconfig -o `"AuthorizedKeysFile .testssh/authorized_keys`" -E $sshdlog" -Port $port + ssh -p $port -E $sshlog -o "PasswordAuthentication=no" $ssouser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + Stop-SSHDTestDaemon -Port $port + sleep $sshdDelay + $sshlog | Should Contain "Permission denied" + $sshdlog | Should Contain "Authentication refused." + } + + It "$tC.$tI-authorized_keys-negative(other account has Delete on authorized_keys file)" -skip:$skip { + Repair-AuthorizedKeyPermission -Filepath $authorizedkeyPath -confirm:$false + $objPwdUserSid = Get-UserSid -User $PwdUser + Set-FilePermission -FilePath $authorizedkeyPath -User $objPwdUserSid -Perm "Delete" + + Start-SSHDTestDaemon -workDir $opensshbinpath -Arguments "-d -f $sshdconfig -o `"AuthorizedKeysFile .testssh/authorized_keys`" -E $sshdlog" -Port $port + ssh -p $port -E $sshlog -o "PasswordAuthentication=no" $ssouser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + Stop-SSHDTestDaemon -Port $port + sleep $sshdDelay + $sshlog | Should Contain "Permission denied" + $sshdlog | Should Contain "Authentication refused." + } + It "$tC.$tI-authorized_keys-negative(authorized_keys is owned by other non-admin user)" -skip:$skip { #setup to have PwdUser as owner and grant it full control $objPwdUserSid = Get-UserSid -User $PwdUser Repair-FilePermission -Filepath $authorizedkeyPath -Owner $objPwdUserSid -FullAccessNeeded $adminsSid,$systemSid,$objPwdUser -confirm:$false - #Run Start-SSHDTestDaemon -WorkDir $opensshbinpath -Arguments "-d -f $sshdconfig -o `"AuthorizedKeysFile .testssh/authorized_keys`" -E $sshdlog" -Port $port - ssh -p $port -E $sshlog $ssouser@$server echo 1234 + ssh -p $port -E $sshlog -o "PasswordAuthentication=no" $ssouser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 Stop-SSHDTestDaemon -Port $port sleep $sshdDelay diff --git a/regress/pesterTests/Hostkey_fileperm.Tests.ps1 b/regress/pesterTests/Hostkey_fileperm.Tests.ps1 index a6408829ef76..ebb61228750e 100644 --- a/regress/pesterTests/Hostkey_fileperm.Tests.ps1 +++ b/regress/pesterTests/Hostkey_fileperm.Tests.ps1 @@ -148,5 +148,32 @@ Describe "Tests for host keys file permission" -Tags "CI" { $logPath | Should Contain "bad permissions" } + It "$tC.$tI-Host keys-negative (other account has ChangePermissions on private key file)" { + Repair-FilePermission -Filepath $hostKeyFilePath -Owners $adminsSid -FullAccessNeeded $systemSid,$adminsSid -confirm:$false + Repair-FilePermission -Filepath "$hostKeyFilePath.pub" -Owners $adminsSid -FullAccessNeeded $systemSid,$adminsSid -ReadAccessNeeded $everyOneSid -confirm:$false + Set-FilePermission -FilePath $hostKeyFilePath -UserSid $objUserSid -Perm "ChangePermissions" + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow + WaitForValidation -LogPath $logPath -Length 1100 + $logPath | Should Contain "bad permissions" + } + + It "$tC.$tI-Host keys-negative (other account has TakeOwnership on private key file)" { + Repair-FilePermission -Filepath $hostKeyFilePath -Owners $adminsSid -FullAccessNeeded $systemSid,$adminsSid -confirm:$false + Repair-FilePermission -Filepath "$hostKeyFilePath.pub" -Owners $adminsSid -FullAccessNeeded $systemSid,$adminsSid -ReadAccessNeeded $everyOneSid -confirm:$false + Set-FilePermission -FilePath $hostKeyFilePath -UserSid $objUserSid -Perm "TakeOwnership" + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow + WaitForValidation -LogPath $logPath -Length 1100 + $logPath | Should Contain "bad permissions" + } + + It "$tC.$tI-Host keys-negative (other account has Delete on private key file)" { + Repair-FilePermission -Filepath $hostKeyFilePath -Owners $adminsSid -FullAccessNeeded $systemSid,$adminsSid -confirm:$false + Repair-FilePermission -Filepath "$hostKeyFilePath.pub" -Owners $adminsSid -FullAccessNeeded $systemSid,$adminsSid -ReadAccessNeeded $everyOneSid -confirm:$false + Set-FilePermission -FilePath $hostKeyFilePath -UserSid $objUserSid -Perm "Delete" + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow + WaitForValidation -LogPath $logPath -Length 1100 + $logPath | Should Contain "bad permissions" + } + } } diff --git a/regress/pesterTests/Setup.Tests.ps1 b/regress/pesterTests/Setup.Tests.ps1 index 145fcbbb652e..1b9617993199 100644 --- a/regress/pesterTests/Setup.Tests.ps1 +++ b/regress/pesterTests/Setup.Tests.ps1 @@ -572,6 +572,17 @@ Describe "Setup Tests" -Tags "Setup" { if ((Get-Service sshd).Status -eq 'Running') { net stop sshd } + # Register ETW manifest to capture OpenSSH events in the event log + $etwman = Join-Path $binPath "openssh-events.man" + if (Test-Path $etwman -PathType Leaf) { + wevtutil im "$etwman" 2>&1 | Out-Null + } + } + BeforeEach { + # Clear and enable the Operational event log channel before each test + wevtutil sl "OpenSSH/Operational" /e:false /q:true | Out-Null + wevtutil cl "OpenSSH/Operational" | Out-Null + wevtutil sl "OpenSSH/Operational" /e:true /q:true | Out-Null } AfterAll { $tC++ @@ -581,6 +592,10 @@ Describe "Setup Tests" -Tags "Setup" { if ($sshACL -eq $null) { Remove-Item -Path $sshFolderPath -Recurse -Force } + $etwman = Join-Path $binPath "openssh-events.man" + if (Test-Path $etwman -PathType Leaf) { + wevtutil um "$etwman" 2>&1 | Out-Null + } } AfterEach { $tI++ @@ -605,5 +620,57 @@ Describe "Setup Tests" -Tags "Setup" { net start sshd $LASTEXITCODE | Should Be 0 } + + It "$tC.$tI - SSHD starts successfully and logs warning when other account has ChangePermissions on ssh folder" { + if (-not (Test-Path -Path $sshFolderPath)) { + New-Item -Path $sshFolderPath -ItemType Directory -Force + } + # Grant ChangePermissions (WRITE_DAC) to Authenticated Users - advisory warning, does not block startup + $acl = Get-Acl $sshFolderPath + $accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule($authenticatedUserSid, "ChangePermissions", "Allow") + $acl.AddAccessRule($accessRule) + Set-Acl -Path $sshFolderPath -AclObject $acl + + net start sshd + $LASTEXITCODE | Should Be 0 + Start-Sleep -Seconds 1 # ensure event is logged before querying + $events = wevtutil qe "OpenSSH/Operational" /f:text + ($events | Out-String) | Should Match "write access is granted" + } + + It "$tC.$tI - SSHD starts successfully and logs warning when other account has TakeOwnership on ssh folder" { + if (-not (Test-Path -Path $sshFolderPath)) { + New-Item -Path $sshFolderPath -ItemType Directory -Force + } + # Grant TakeOwnership (WRITE_OWNER) to Authenticated Users - advisory warning, does not block startup + $acl = Get-Acl $sshFolderPath + $accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule($authenticatedUserSid, "TakeOwnership", "Allow") + $acl.AddAccessRule($accessRule) + Set-Acl -Path $sshFolderPath -AclObject $acl + + net start sshd + $LASTEXITCODE | Should Be 0 + Start-Sleep -Seconds 1 # ensure event is logged before querying + $events = wevtutil qe "OpenSSH/Operational" /f:text + ($events | Out-String) | Should Match "write access is granted" + } + + It "$tC.$tI - SSHD starts successfully and logs warning when other account has Delete on ssh folder" { + if (-not (Test-Path -Path $sshFolderPath)) { + New-Item -Path $sshFolderPath -ItemType Directory -Force + } + # Grant Delete to Authenticated Users using explicit inheritance flags to match the + # existing ReadAndExecute ACE, avoiding OS merging ambiguity. Advisory warning, does not block startup. + $acl = Get-Acl $sshFolderPath + $accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule($authenticatedUserSid, "Delete", "Allow") + $acl.AddAccessRule($accessRule) + Set-Acl -Path $sshFolderPath -AclObject $acl + + net start sshd + $LASTEXITCODE | Should Be 0 + Start-Sleep -Seconds 1 # ensure event is logged before querying + $events = wevtutil qe "OpenSSH/Operational" /f:text + ($events | Out-String) | Should Match "write access is granted" + } } } diff --git a/regress/pesterTests/Userkey_fileperm.Tests.ps1 b/regress/pesterTests/Userkey_fileperm.Tests.ps1 index 1b2379689244..37a1ea295c91 100644 --- a/regress/pesterTests/Userkey_fileperm.Tests.ps1 +++ b/regress/pesterTests/Userkey_fileperm.Tests.ps1 @@ -119,7 +119,7 @@ Describe "Tests for user Key file permission" -Tags "CI" { Repair-FilePermission -FilePath $keyFilePath -Owners $currentUserSid -FullAccessNeeded $currentUser,$adminsSid,$systemSid -ReadAccessNeeded $objUserSid -confirm:$false #Run - $o = ssh -p $port -i $keyFilePath -E $logPath $pubKeyUser@$server echo 1234 + $o = ssh -p $port -i $keyFilePath -E $logPath -o "PasswordAuthentication=no" $pubKeyUser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!" @@ -129,10 +129,34 @@ Describe "Tests for user Key file permission" -Tags "CI" { #setup to have ssouser as owner and grant it full control Repair-FilePermission -FilePath $keyFilePath -Owners $objUserSid -FullAccessNeeded $objUserSid,$adminsSid,$systemSid -ReadAccessNeeded $objUserSid -confirm:$false - $o = ssh -p $port -i $keyFilePath -E $logPath $pubKeyUser@$server echo 1234 + $o = ssh -p $port -i $keyFilePath -E $logPath -o "PasswordAuthentication=no" $pubKeyUser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!" } + + It "$tC.$tI-ssh with private key file -- negative(other account has ChangePermissions on private key file)" { + Repair-FilePermission -FilePath $keyFilePath -Owners $currentUserSid -FullAccessNeeded $adminsSid,$systemSid,$currentUserSid -confirm:$false + Set-FilePermission -FilePath $keyFilePath -UserSid $objUserSid -Perm "ChangePermissions" + $o = ssh -p $port -i $keyFilePath -E $logPath -o "PasswordAuthentication=no" $pubKeyUser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!" + } + + It "$tC.$tI-ssh with private key file -- negative(other account has TakeOwnership on private key file)" { + Repair-FilePermission -FilePath $keyFilePath -Owners $currentUserSid -FullAccessNeeded $adminsSid,$systemSid,$currentUserSid -confirm:$false + Set-FilePermission -FilePath $keyFilePath -UserSid $objUserSid -Perm "TakeOwnership" + $o = ssh -p $port -i $keyFilePath -E $logPath -o "PasswordAuthentication=no" $pubKeyUser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!" + } + + It "$tC.$tI-ssh with private key file -- negative(other account has Delete on private key file)" { + Repair-FilePermission -FilePath $keyFilePath -Owners $currentUserSid -FullAccessNeeded $adminsSid,$systemSid,$currentUserSid -confirm:$false + Set-FilePermission -FilePath $keyFilePath -UserSid $objUserSid -Perm "Delete" + $o = ssh -p $port -i $keyFilePath -E $logPath -o "PasswordAuthentication=no" $pubKeyUser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!" + } } } diff --git a/regress/pesterTests/data/SSHD_Config b/regress/pesterTests/data/SSHD_Config index 101202fab6ff..1f7d0940de23 100644 --- a/regress/pesterTests/data/SSHD_Config +++ b/regress/pesterTests/data/SSHD_Config @@ -10,6 +10,7 @@ # possible, but leave them commented. Uncommented options override the # default value. +PerSourcePenalties no Port 47002 #AddressFamily any #ListenAddress 0.0.0.0