diff --git a/authfile.c b/authfile.c index caa450d68..807d2816e 100644 --- a/authfile.c +++ b/authfile.c @@ -61,14 +61,6 @@ sshkey_save_private_blob(struct sshbuf *keybuf, const char *filename) if ((fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0600)) < 0) return SSH_ERR_SYSTEM_ERROR; -#ifdef WINDOWS /* WINDOWS */ - /* - Set the owner of the private key file to current user and only grant - current user the full control access - */ - if (set_secure_file_permission(filename, NULL) != 0) - return SSH_ERR_SYSTEM_ERROR; -#endif /* WINDOWS */ if (atomicio(vwrite, fd, (u_char *)sshbuf_ptr(keybuf), sshbuf_len(keybuf)) != sshbuf_len(keybuf)) { oerrno = errno; diff --git a/contrib/win32/openssh/OpenSSHBuildHelper.psm1 b/contrib/win32/openssh/OpenSSHBuildHelper.psm1 index 18b946380..c48b59e6c 100644 --- a/contrib/win32/openssh/OpenSSHBuildHelper.psm1 +++ b/contrib/win32/openssh/OpenSSHBuildHelper.psm1 @@ -300,7 +300,7 @@ function Package-OpenSSH } $buildDir = Join-Path $repositoryRoot ("bin\" + $folderName + "\" + $Configuration) $payload = "sshd.exe", "ssh.exe", "ssh-agent.exe", "ssh-add.exe", "sftp.exe" - $payload += "sftp-server.exe", "scp.exe", "ssh-shellhost.exe", "ssh-keygen.exe" + $payload += "sftp-server.exe", "scp.exe", "ssh-shellhost.exe", "ssh-keygen.exe", "ssh-keyscan.exe" $payload += "sshd_config", "install-sshd.ps1", "uninstall-sshd.ps1" $packageName = "OpenSSH-Win64" @@ -503,7 +503,7 @@ function Install-OpenSSH & "$OpenSSHDir\ssh-keygen.exe" -A $keyFiles = Get-ChildItem "$OpenSSHDir\ssh_host_*_key*" | % { - Add-PermissionToFileACL -FilePath $_.FullName -User "NT Service\sshd" -Perm "Read" + Adjust-HostKeyFileACL -FilePath $_.FullName } diff --git a/contrib/win32/openssh/OpenSSHCommonUtils.psm1 b/contrib/win32/openssh/OpenSSHCommonUtils.psm1 index aa63a7497..5aa1ba7a0 100644 --- a/contrib/win32/openssh/OpenSSHCommonUtils.psm1 +++ b/contrib/win32/openssh/OpenSSHCommonUtils.psm1 @@ -30,48 +30,147 @@ function Get-RepositoryRoot <# .Synopsis - Sets the Secure File ACL. - 1. Removed all user acl except Administrators group, system, and current user - 2. whether or not take the owner + Set owner of the file to by LOCALSYSTEM account + Set private host key be fully controlled by LOCALSYSTEM and Administrators + Set public host key be fully controlled by LOCALSYSTEM and Administrators, read access by everyone .Outputs N/A .Inputs FilePath - The path to the file - takeowner - if want to take the ownership #> -function Cleanup-SecureFileACL +function Adjust-HostKeyFileACL { - [CmdletBinding()] - param([string]$FilePath, [System.Security.Principal.NTAccount] $Owner) + param ( + [parameter(Mandatory=$true)] + [string]$FilePath + ) - $myACL = Get-ACL $filePath - $myACL.SetAccessRuleProtection($True, $True) - Set-Acl -Path $filePath -AclObject $myACL + $myACL = Get-ACL $FilePath + $myACL.SetAccessRuleProtection($True, $FALSE) + Set-Acl -Path $FilePath -AclObject $myACL + + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $everyoneAccount = New-Object System.Security.Principal.NTAccount("EveryOne") + $myACL = Get-ACL $FilePath + + $myACL.SetOwner($systemAccount) - $myACL = Get-ACL $filePath - if($owner -ne $null) - { - $myACL.SetOwner($owner) - } - if($myACL.Access) { $myACL.Access | % { - if (($_ -ne $null) -and ($_.IdentityReference.Value -ine "BUILTIN\Administrators") -and - ($_.IdentityReference.Value -ine "NT AUTHORITY\SYSTEM") -and - ($_.IdentityReference.Value -ine "$(whoami)")) + if(-not ($myACL.RemoveAccessRule($_))) { - if(-not ($myACL.RemoveAccessRule($_))) - { - throw "failed to remove access of $($_.IdentityReference.Value) rule in setup " - } + throw "failed to remove access of $($_.IdentityReference.Value) rule in setup " } } + } + + $adminACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($adminAccount, "FullControl", "None", "None", "Allow") + $myACL.AddAccessRule($adminACE) + + $systemACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($systemAccount, "FullControl", "None", "None", "Allow") + $myACL.AddAccessRule($systemACE) + + if($FilePath.EndsWith(".pub")) + { + $everyoneAce = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ("Everyone", "Read", "None", "None", "Allow") + $myACL.AddAccessRule($everyoneAce) + } + else + { + #this only is needed when the private host keys are not registered with agent + $sshdAce = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ("NT service\sshd", "Read", "None", "None", "Allow") + $myACL.AddAccessRule($sshdAce) + } + Set-Acl -Path $FilePath -AclObject $myACL +} + +<# +.Synopsis + Set owner of the user key file + Set ACL to have private user key be fully controlled by LOCALSYSTEM and Administrators, Read, write access by owner + Set public user key be fully controlled by LOCALSYSTEM and Administrators, Read, write access by owner, read access by everyone + +.Outputs + N/A + +.Inputs + FilePath - The path to the file + Owner - owner of the file + OwnerPerms - the permissions grant to the owner +#> +function Adjust-UserKeyFileACL +{ + param ( + [parameter(Mandatory=$true)] + [string]$FilePath, + [System.Security.Principal.NTAccount] $Owner = $null, + [System.Security.AccessControl.FileSystemRights[]] $OwnerPerms = $null + ) + + $myACL = Get-ACL $FilePath + $myACL.SetAccessRuleProtection($True, $FALSE) + Set-Acl -Path $FilePath -AclObject $myACL + + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $everyoneAccount = New-Object System.Security.Principal.NTAccount("EveryOne") + $myACL = Get-ACL $FilePath + + $actualOwner = $null + if($Owner -eq $null) + { + $actualOwner = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) + } + else + { + $actualOwner = $Owner + } + + $myACL.SetOwner($actualOwner) + + if($myACL.Access) + { + $myACL.Access | % { + if(-not ($myACL.RemoveAccessRule($_))) + { + throw "failed to remove access of $($_.IdentityReference.Value) rule in setup " + } + } + } + + $adminACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($adminAccount, "FullControl", "None", "None", "Allow") + $myACL.AddAccessRule($adminACE) + + $systemACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($systemAccount, "FullControl", "None", "None", "Allow") + $myACL.AddAccessRule($systemACE) + + if($OwnerPerms) + { + $OwnerPerms | % { + $ownerACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($actualOwner, $_, "None", "None", "Allow") + $myACL.AddAccessRule($ownerACE) + } } - Set-Acl -Path $filePath -AclObject $myACL + if($FilePath.EndsWith(".pub")) + { + $everyoneAce = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ("Everyone", "Read", "None", "None", "Allow") + $myACL.AddAccessRule($everyoneAce) + } + + Set-Acl -Path $FilePath -AclObject $myACL } <# @@ -88,20 +187,27 @@ function Cleanup-SecureFileACL #> function Add-PermissionToFileACL { - [CmdletBinding()] - param( + param ( + [parameter(Mandatory=$true)] [string]$FilePath, + [parameter(Mandatory=$true)] [System.Security.Principal.NTAccount] $User, - [System.Security.AccessControl.FileSystemRights]$Perm + [parameter(Mandatory=$true)] + [System.Security.AccessControl.FileSystemRights[]]$Perms ) - $myACL = Get-ACL $filePath + $myACL = Get-ACL $FilePath - $objACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` - ($User, $perm, "None", "None", "Allow") - $myACL.AddAccessRule($objACE) + if($Perms) + { + $Perms | % { + $userACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($User, $_, "None", "None", "Allow") + $myACL.AddAccessRule($userACE) + } + } - Set-Acl -Path $filePath -AclObject $myACL + Set-Acl -Path $FilePath -AclObject $myACL } -Export-ModuleMember -Function Get-RepositoryRoot, Add-PermissionToFileACL, Cleanup-SecureFileACL \ No newline at end of file +Export-ModuleMember -Function Get-RepositoryRoot, Add-PermissionToFileACL, Adjust-HostKeyFileACL, Adjust-UserKeyFileACL \ No newline at end of file diff --git a/contrib/win32/openssh/OpenSSHTestHelper.psm1 b/contrib/win32/openssh/OpenSSHTestHelper.psm1 index 37f60bed0..27004c821 100644 --- a/contrib/win32/openssh/OpenSSHTestHelper.psm1 +++ b/contrib/win32/openssh/OpenSSHTestHelper.psm1 @@ -1,5 +1,5 @@ $ErrorActionPreference = 'Stop' -Import-Module $PSScriptRoot\OpenSSHCommonUtils.psm1 -DisableNameChecking +Import-Module $PSScriptRoot\OpenSSHCommonUtils.psm1 -DisableNameChecking -Force [System.IO.DirectoryInfo] $repositoryRoot = Get-RepositoryRoot # test environment parameters initialized with defaults @@ -158,17 +158,12 @@ WARNING: Following changes will be made to OpenSSH configuration # copy new sshd_config Copy-Item (Join-Path $Script:E2ETestDirectory sshd_config) (Join-Path $script:OpenSSHBinPath sshd_config) -Force - #workaround for the cariggage new line added by git before copy them - Get-ChildItem "$($Script:E2ETestDirectory)\sshtest_*key*" | % { - (Get-Content $_.FullName -Raw).Replace("`r`n","`n") | Set-Content $_.FullName -Force - } - #copy sshtest keys - Copy-Item "$($Script:E2ETestDirectory)\sshtest*hostkey*" $script:OpenSSHBinPath -Force - $owner = New-Object System.Security.Principal.NTAccount($env:USERDOMAIN, $env:USERNAME) - Get-ChildItem "$($script:OpenSSHBinPath)\sshtest*hostkey*" -Exclude *.pub | % { - Cleanup-SecureFileACL -FilePath $_.FullName -Owner $owner - Add-PermissionToFileACL -FilePath $_.FullName -User "NT Service\sshd" -Perm "Read" + Copy-Item "$($Script:E2ETestDirectory)\sshtest*hostkey*" $script:OpenSSHBinPath -Force + Get-ChildItem "$($script:OpenSSHBinPath)\sshtest*hostkey*"| % { + #workaround for the cariggage new line added by git before copy them + (Get-Content $_.FullName -Raw).Replace("`r`n","`n") | Set-Content $_.FullName -Force + Adjust-HostKeyFileACL -FilePath $_.FullName } Restart-Service sshd -Force @@ -190,6 +185,7 @@ WARNING: Following changes will be made to OpenSSH configuration Copy-Item $sshConfigFilePath (Join-Path $dotSshDirectoryPath config.ori) -Force } Copy-Item (Join-Path $Script:E2ETestDirectory ssh_config) $sshConfigFilePath -Force + Adjust-UserKeyFileACL -FilePath $sshConfigFilePath -OwnerPerms "Read,Write" # create test accounts #TODO - this is Windows specific. Need to be in PAL @@ -216,9 +212,12 @@ WARNING: Following changes will be made to OpenSSH configuration $authorizedKeyPath = Join-Path $ssouserProfile .ssh\authorized_keys $testPubKeyPath = Join-Path $Script:E2ETestDirectory sshtest_userssokey_ed25519.pub Copy-Item $testPubKeyPath $authorizedKeyPath -Force -ErrorAction SilentlyContinue + $owner = New-Object System.Security.Principal.NTAccount($SSOUser) + Adjust-UserKeyFileACL -FilePath $authorizedKeyPath -Owner $owner -OwnerPerms "Read","Write" Add-PermissionToFileACL -FilePath $authorizedKeyPath -User "NT Service\sshd" -Perm "Read" $testPriKeypath = Join-Path $Script:E2ETestDirectory sshtest_userssokey_ed25519 - Cleanup-SecureFileACL -FilePath $testPriKeypath -owner $owner + (Get-Content $testPriKeypath -Raw).Replace("`r`n","`n") | Set-Content $testPriKeypath -Force + Adjust-UserKeyFileACL -FilePath $testPriKeypath -OwnerPerms "Read, Write" cmd /c "ssh-add $testPriKeypath 2>&1 >> $Script:TestSetupLogFile" Backup-OpenSSHTestInfo } diff --git a/contrib/win32/win32compat/fileio.c b/contrib/win32/win32compat/fileio.c index 3fc924190..136ee22a2 100644 --- a/contrib/win32/win32compat/fileio.c +++ b/contrib/win32/win32compat/fileio.c @@ -39,13 +39,32 @@ #include "w32fd.h" #include "inc\utf.h" #include "inc\fcntl.h" +#include "inc\pwd.h" #include "misc_internal.h" #include "debug.h" +#include /* internal read buffer size */ #define READ_BUFFER_SIZE 100*1024 /* internal write buffer size */ #define WRITE_BUFFER_SIZE 100*1024 + +/* +* A ACE is a binary data structure of changeable length +* https://msdn.microsoft.com/en-us/library/windows/desktop/aa374928(v=vs.85).aspx +* The value is calculated based on current need: max sid string (184) plus the enough spaces for other fields in ACEs +*/ +#define MAX_ACE_LENGTH 225 +/* +* A security descriptor is a binary data structure of changeable length +* https://msdn.microsoft.com/en-us/library/windows/desktop/aa379570(v=vs.85).aspx +* The value is calculated based on current need: 4 ACEs plus the enough spaces for owner sid and dcal flag +*/ +#define SDDL_LENGTH 5* MAX_ACE_LENGTH + +/*MAX length attribute string looks like 0xffffffff*/ +#define MAX_ATTRIBUTE_LENGTH 10 + #define errno_from_Win32LastError() errno_from_Win32Error(GetLastError()) struct createFile_flags { @@ -242,13 +261,48 @@ error: return -1; } +static int +st_mode_to_file_att(int mode, wchar_t * attributes) +{ + DWORD att = 0; + switch (mode) { + case S_IRWXO: + swprintf_s(attributes, MAX_ATTRIBUTE_LENGTH, L"FA"); + break; + case S_IXOTH: + swprintf_s(attributes, MAX_ATTRIBUTE_LENGTH, L"FX"); + break; + case S_IWOTH: + swprintf_s(attributes, MAX_ATTRIBUTE_LENGTH, L"FW"); + break; + case S_IROTH: + swprintf_s(attributes, MAX_ATTRIBUTE_LENGTH, L"FR"); + break; + default: + if((mode & S_IROTH) != 0) + att |= FILE_GENERIC_READ; + if ((mode & S_IWOTH) != 0) + att |= FILE_GENERIC_WRITE; + if ((mode & S_IXOTH) != 0) + att |= FILE_GENERIC_EXECUTE; + swprintf_s(attributes, MAX_ATTRIBUTE_LENGTH, L"%#lx", att); + break; + } + return 0; +} + /* maps open() file modes and flags to ones needed by CreateFile */ static int -createFile_flags_setup(int flags, int mode, struct createFile_flags* cf_flags) +createFile_flags_setup(int flags, u_short mode, struct createFile_flags* cf_flags) { /* check flags */ - int rwflags = flags & 0x3; - int c_s_flags = flags & 0xfffffff0; + int rwflags = flags & 0x3, c_s_flags = flags & 0xfffffff0, ret = -1; + PSECURITY_DESCRIPTOR pSD = NULL; + wchar_t sddl[SDDL_LENGTH + 1] = { 0 }, owner_ace[MAX_ACE_LENGTH + 1] = {0}, everyone_ace[MAX_ACE_LENGTH + 1] = {0}; + wchar_t owner_access[MAX_ATTRIBUTE_LENGTH + 1] = {0}, everyone_access[MAX_ATTRIBUTE_LENGTH + 1] = {0}, *sid_utf16; + PACL dacl = NULL; + struct passwd * pwd; + PSID owner_sid = NULL; /* * should be one of one of the following access modes: @@ -268,7 +322,7 @@ createFile_flags_setup(int flags, int mode, struct createFile_flags* cf_flags) } /*validate mode*/ - if (mode &~(S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH)) { + if (mode & ~(S_IRWXU | S_IRWXG | S_IRWXO)) { debug3("open - ERROR: unsupported mode: %d", mode); errno = ENOTSUP; return -1; @@ -287,12 +341,7 @@ createFile_flags_setup(int flags, int mode, struct createFile_flags* cf_flags) case O_RDWR: cf_flags->dwDesiredAccess = GENERIC_READ | GENERIC_WRITE; break; - } - - cf_flags->securityAttributes.lpSecurityDescriptor = NULL; - cf_flags->securityAttributes.bInheritHandle = TRUE; - cf_flags->securityAttributes.nLength = 0; - + } cf_flags->dwCreationDisposition = OPEN_EXISTING; if (c_s_flags & O_TRUNC) cf_flags->dwCreationDisposition = TRUNCATE_EXISTING; @@ -308,16 +357,58 @@ createFile_flags_setup(int flags, int mode, struct createFile_flags* cf_flags) cf_flags->dwFlagsAndAttributes = FILE_FLAG_OVERLAPPED | SECURITY_IMPERSONATION | FILE_FLAG_BACKUP_SEMANTICS; - /*TODO - map mode */ + /*map mode*/ + if ((pwd = getpwuid(0)) == NULL) + fatal("getpwuid failed."); - return 0; + if ((sid_utf16 = utf8_to_utf16(pwd->pw_sid)) == NULL) { + debug3("Failed to get utf16 of the sid string"); + errno = ENOMEM; + goto cleanup; + } + if ((mode & S_IRWXU) != 0) { + if (st_mode_to_file_att((mode & S_IRWXU) >> 6, owner_access) != 0) { + debug3("st_mode_to_file_att()"); + goto cleanup; + } + swprintf_s(owner_ace, MAX_ACE_LENGTH, L"(A;;%s;;;%s)", owner_access, sid_utf16); + } + + if (mode & S_IRWXO) { + if (st_mode_to_file_att(mode & S_IRWXO, everyone_access) != 0) { + debug3("st_mode_to_file_att()"); + goto cleanup; + } + swprintf_s(everyone_ace, MAX_ACE_LENGTH, L"(A;;%s;;;WD)", everyone_access); + } + + swprintf_s(sddl, SDDL_LENGTH, L"O:%sD:PAI(A;;FA;;;BA)(A;;FA;;;SY)%s%s", sid_utf16, owner_ace, everyone_ace); + if (ConvertStringSecurityDescriptorToSecurityDescriptorW(sddl, SDDL_REVISION, &pSD, NULL) == FALSE) { + debug3("ConvertStringSecurityDescriptorToSecurityDescriptorW failed with error code %d", GetLastError()); + goto cleanup; + } + + if (IsValidSecurityDescriptor(pSD) == FALSE) { + debug3("IsValidSecurityDescriptor return FALSE"); + goto cleanup; + } + + cf_flags->securityAttributes.lpSecurityDescriptor = pSD; + cf_flags->securityAttributes.bInheritHandle = TRUE; + cf_flags->securityAttributes.nLength = sizeof(cf_flags->securityAttributes); + + ret = 0; +cleanup: + if (sid_utf16) + free(sid_utf16); + return ret; } #define NULL_DEVICE "/dev/null" /* open() implementation. Uses CreateFile to open file, console, device, etc */ struct w32_io* -fileio_open(const char *path_utf8, int flags, int mode) +fileio_open(const char *path_utf8, int flags, u_short mode) { struct w32_io* pio = NULL; struct createFile_flags cf_flags; @@ -342,27 +433,28 @@ fileio_open(const char *path_utf8, int flags, int mode) return NULL; } - if (createFile_flags_setup(flags, mode, &cf_flags) == -1) - return NULL; + if (createFile_flags_setup(flags, mode, &cf_flags) == -1) { + debug3("createFile_flags_setup() failed."); + goto cleanup; + } handle = CreateFileW(path_utf16, cf_flags.dwDesiredAccess, cf_flags.dwShareMode, &cf_flags.securityAttributes, cf_flags.dwCreationDisposition, - cf_flags.dwFlagsAndAttributes, NULL); + cf_flags.dwFlagsAndAttributes, NULL); if (handle == INVALID_HANDLE_VALUE) { errno = errno_from_Win32LastError(); debug3("failed to open file:%s error:%d", path_utf8, GetLastError()); - free(path_utf16); - return NULL; + goto cleanup; } - free(path_utf16); + pio = (struct w32_io*)malloc(sizeof(struct w32_io)); if (pio == NULL) { CloseHandle(handle); errno = ENOMEM; debug3("fileio_open(), failed to allocate memory error:%d", errno); - return NULL; + goto cleanup; } memset(pio, 0, sizeof(struct w32_io)); @@ -371,6 +463,11 @@ fileio_open(const char *path_utf8, int flags, int mode) pio->fd_status_flags = O_NONBLOCK; pio->handle = handle; +cleanup: + if ((&cf_flags.securityAttributes != NULL) && (&cf_flags.securityAttributes.lpSecurityDescriptor != NULL)) + LocalFree(cf_flags.securityAttributes.lpSecurityDescriptor); + if(path_utf16) + free(path_utf16); return pio; } diff --git a/contrib/win32/win32compat/inc/fcntl.h b/contrib/win32/win32compat/inc/fcntl.h index ec7e854af..d22f9c782 100644 --- a/contrib/win32/win32compat/inc/fcntl.h +++ b/contrib/win32/win32compat/inc/fcntl.h @@ -16,8 +16,8 @@ int w32_fcntl(int fd, int cmd, ... /* arg */); #define fcntl(a,b,...) w32_fcntl((a), (b), __VA_ARGS__) -#define open w32_open -int w32_open(const char *pathname, int flags, ...); +#define open(a,b,...) w32_open((a), (b), __VA_ARGS__) +int w32_open(const char *pathname, int flags, ... /* arg */); void* w32_fd_to_handle(int fd); int w32_allocate_fd_for_handle(HANDLE, BOOL); diff --git a/contrib/win32/win32compat/w32-sshfileperm.c b/contrib/win32/win32compat/w32-sshfileperm.c index 32c393721..5a0647a82 100644 --- a/contrib/win32/win32compat/w32-sshfileperm.c +++ b/contrib/win32/win32compat/w32-sshfileperm.c @@ -31,33 +31,28 @@ #include #include #include -#include #include #include #include "inc\pwd.h" #include "sshfileperm.h" -#include "misc_internal.h" #include "debug.h" #define SSHD_ACCOUNT L"NT Service\\sshd" /* * The function is to check if user prepresented by pw is secure to access to the file. -* Check the owner of the file is one of these types: Local Administrators groups, system account, -* direct user accounts in local administrators, or user represented by pw +* Check the owner of the file is one of these types: Local Administrators groups, system 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, user represented by pw, - and owner accounts have write permission on the file + 1. no user other than local administrators group, system account, and owner accounts have write permission on the file 2. sshd account can only have read permission - 3. user represented by pw and file owner should at least have read permission. + 3. file owner should at least have read permission. * Returns 0 on success and -1 on failure */ int check_secure_file_permission(const char *name, struct passwd * pw) -{ - return 0; - /*PSECURITY_DESCRIPTOR pSD = NULL; +{ + PSECURITY_DESCRIPTOR pSD = NULL; wchar_t * name_utf16 = NULL; PSID owner_sid = NULL, user_sid = NULL; PACL dacl = NULL; @@ -80,10 +75,10 @@ check_secure_file_permission(const char *name, struct passwd * pw) if ((name_utf16 = utf8_to_utf16(name)) == NULL) { errno = ENOMEM; goto cleanup; - }*/ + } /*Get the owner sid of the file.*/ - /*if ((error_code = GetNamedSecurityInfoW(name_utf16, SE_FILE_OBJECT, + if ((error_code = GetNamedSecurityInfoW(name_utf16, SE_FILE_OBJECT, OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, &owner_sid, NULL, &dacl, NULL, &pSD)) != ERROR_SUCCESS) { debug3("failed to retrieve the owner sid and dacl of file %s with error code: %d", name, error_code); @@ -98,19 +93,18 @@ check_secure_file_permission(const char *name, struct passwd * pw) } if (!IsWellKnownSid(owner_sid, WinBuiltinAdministratorsSid) && !IsWellKnownSid(owner_sid, WinLocalSystemSid) && - !EqualSid(owner_sid, user_sid) && - !is_admin_account(owner_sid)) { + !EqualSid(owner_sid, user_sid)) { debug3("Bad owner on %s", name); ret = -1; goto cleanup; - }*/ + } /* 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, owner accounts have write permission on the file + 1. no others than administrators group, system account, and owner account have write permission on the file 2. sshd account can only have read permission - 3. this user and file owner should at least have read permission + 3. file owner should at least have read permission */ - /*for (DWORD i = 0; i < dacl->AceCount; i++) { + for (DWORD i = 0; i < dacl->AceCount; i++) { PVOID current_ace = NULL; PACE_HEADER current_aceHeader = NULL; PSID current_trustee_sid = NULL; @@ -141,11 +135,6 @@ check_secure_file_permission(const char *name, struct passwd * pw) ret = -1; goto cleanup; } - else if (EqualSid(current_trustee_sid, user_sid)) { - debug3("Bad permission on %s. The user should at least have read permission.", name); - ret = -1; - goto cleanup; - } } continue; } @@ -153,14 +142,12 @@ check_secure_file_permission(const char *name, struct passwd * pw) // Not interested ACE continue; } - }*/ + } - /*no need to check administrators group, owner account, user account and system account*/ - /*if (IsWellKnownSid(current_trustee_sid, WinBuiltinAdministratorsSid) || + /*no need to check administrators group, owner account, and system account*/ + if (IsWellKnownSid(current_trustee_sid, WinBuiltinAdministratorsSid) || IsWellKnownSid(current_trustee_sid, WinLocalSystemSid) || - EqualSid(current_trustee_sid, owner_sid) || - EqualSid(current_trustee_sid, user_sid) || - is_admin_account(current_trustee_sid)) { + EqualSid(current_trustee_sid, owner_sid)) { continue; } else if(is_sshd_account(current_trustee_sid)){ @@ -189,7 +176,7 @@ cleanup: FreeSid(user_sid); if(name_utf16) free(name_utf16); - return ret;*/ + return ret; } static BOOL @@ -210,145 +197,3 @@ is_sshd_account(PSID user_sid) { wmemcpy(full_name + domain_name_length + 1, user_name, wcslen(user_name)+1); return (wcsicmp(full_name, SSHD_ACCOUNT) == 0); } - -/* - * Check if the user is in local administrators group - * currently only check if the user is directly in the group - * Returns TRUE if the user is in administrators group; otherwise false; -*/ -static BOOL -is_admin_account(PSID user_sid) -{ - DWORD entries_read = 0, total_entries = 0, i = 0, name_length = UNCLEN, domain_name_length = DNLEN, sid_size; - LPLOCALGROUP_MEMBERS_INFO_1 local_groups_member_info = NULL; - char admins_sid[SECURITY_MAX_SID_SIZE]; - wchar_t admins_group_name[UNCLEN], domain_name[DNLEN]; - SID_NAME_USE sid_type = SidTypeInvalid; - NET_API_STATUS status; - BOOL ret = FALSE; - - if (CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, admins_sid, &sid_size) == FALSE) { - debug3("CreateWellKnownSid failed with error code: %d.", GetLastError()); - goto done; - } - - if (LookupAccountSidLocalW(admins_sid, admins_group_name, &name_length, - domain_name, &domain_name_length, &sid_type) == FALSE) { - debug3("LookupAccountSidLocalW() failed with error: %d. ", GetLastError()); - errno = ENOENT; - goto done; - } - - status = NetLocalGroupGetMembers(NULL, admins_group_name, 1, (LPBYTE*)&local_groups_member_info, - MAX_PREFERRED_LENGTH, &entries_read, &total_entries, NULL); - if (status != NERR_Success) { - debug3("NetLocalGroupGetMembers failed with error code: %d.", status); - goto done; - } - - for (i = 0; i < entries_read; i++) { - if (local_groups_member_info[i].lgrmi1_sidusage == SidTypeDeletedAccount) - continue; - else if (EqualSid(local_groups_member_info[i].lgrmi1_sid, user_sid)) { - ret = TRUE; - break; - } - } - -done: - if (local_groups_member_info) - NetApiBufferFree(local_groups_member_info); - return ret; -} - -/* -* Set the owner of the secure file to the user represented by pw and only grant -* it the full control access -*/ -int -set_secure_file_permission(const char *name, struct passwd * pw) -{ - return 0; - /*PSECURITY_DESCRIPTOR pSD = NULL; - PSID owner_sid = NULL; - PACL dacl = NULL; - wchar_t *name_utf16 = NULL, *sid_utf16 = NULL, sddl[256]; - DWORD error_code = ERROR_SUCCESS; - struct passwd * pwd = pw; - BOOL present, defaulted; - int ret = 0; - - if (pwd == NULL) - if ((pwd = getpwuid(0)) == NULL) - fatal("getpwuid failed."); - - if (ConvertStringSidToSid(pwd->pw_sid, &owner_sid) == FALSE) { - debug3("failed to retrieve the sid of the pwd with error code: %d", GetLastError()); - ret = -1; - goto cleanup; - } - - if((IsValidSid(owner_sid) == FALSE)) { - debug3("IsValidSid(owner_sid): FALSE"); - ret = -1; - goto cleanup; - } - - if ((sid_utf16 = utf8_to_utf16(pwd->pw_sid)) == NULL) { - debug3("Failed to get utf16 of the sid string"); - errno = ENOMEM; - ret = -1; - goto cleanup; - } - swprintf(sddl, sizeof(sddl) - 1, L"D:P(A;;FA;;;%s)", sid_utf16); - if(ConvertStringSecurityDescriptorToSecurityDescriptorW(sddl, SDDL_REVISION, &pSD, NULL) == FALSE) { - debug3("ConvertStringSecurityDescriptorToSecurityDescriptorW failed with error code %d", GetLastError()); - ret = -1; - goto cleanup; - } - - if (IsValidSecurityDescriptor(pSD) == FALSE) { - debug3("IsValidSecurityDescriptor return FALSE"); - ret = -1; - goto cleanup; - } - - if (GetSecurityDescriptorDacl(pSD, &present, &dacl, &defaulted) == FALSE) { - debug3("GetSecurityDescriptorDacl failed with error code %d", GetLastError()); - ret = -1; - goto cleanup; - } - if (!present || dacl == NULL) { - debug3("failed to find the acl from security descriptior."); - ret = -1; - goto cleanup; - } - - if ((name_utf16 = utf8_to_utf16(name)) == NULL) { - debug3("Failed to get utf16 of the name"); - errno = ENOMEM; - ret = -1; - goto cleanup; - }*/ - - /*Set the owner sid and acl of the file.*/ - /*if ((error_code = SetNamedSecurityInfoW(name_utf16, SE_FILE_OBJECT, - OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, - owner_sid, NULL, dacl, NULL)) != ERROR_SUCCESS) { - debug3("failed to set the owner sid and dacl of file %s with error code: %d", name, error_code); - errno = EOTHER; - ret = -1; - goto cleanup; - } -cleanup: - if (pSD) - LocalFree(pSD); - if (name_utf16) - free(name_utf16); - if(sid_utf16) - free(sid_utf16); - if (owner_sid) - FreeSid(owner_sid); - - return ret;*/ -} diff --git a/contrib/win32/win32compat/w32fd.c b/contrib/win32/win32compat/w32fd.c index 984ecabc2..7d3c0ed75 100644 --- a/contrib/win32/win32compat/w32fd.c +++ b/contrib/win32/win32compat/w32fd.c @@ -387,16 +387,24 @@ w32_pipe(int *pfds) } int -w32_open(const char *pathname, int flags, ...) +w32_open(const char *pathname, int flags, ... /* arg */) { int min_index = fd_table_get_min_index(); struct w32_io* pio; + va_list valist; + u_short mode = 0; errno = 0; if (min_index == -1) return -1; + if (flags & O_CREAT) { + va_start(valist, flags); + mode = va_arg(valist, u_short); + va_end(valist); + } - pio = fileio_open(sanitized_path(pathname), flags, 0); + pio = fileio_open(sanitized_path(pathname), flags, mode); + if (pio == NULL) return -1; diff --git a/contrib/win32/win32compat/w32fd.h b/contrib/win32/win32compat/w32fd.h index 356f89c81..a21d92fa9 100644 --- a/contrib/win32/win32compat/w32fd.h +++ b/contrib/win32/win32compat/w32fd.h @@ -144,7 +144,7 @@ int fileio_close(struct w32_io* pio); int fileio_pipe(struct w32_io* pio[2]); struct w32_io* fileio_afunix_socket(); int fileio_connect(struct w32_io*, char*); -struct w32_io* fileio_open(const char *pathname, int flags, int mode); +struct w32_io* fileio_open(const char *pathname, int flags, u_short mode); int fileio_read(struct w32_io* pio, void *dst, unsigned int max); int fileio_write(struct w32_io* pio, const void *buf, unsigned int max); int fileio_fstat(struct w32_io* pio, struct _stat64 *buf); diff --git a/readconf.c b/readconf.c index 88edede69..4baee2ee7 100644 --- a/readconf.c +++ b/readconf.c @@ -1710,9 +1710,9 @@ read_config_file_depth(const char *filename, struct passwd *pw, if (flags & SSHCONF_CHECKPERM) { #if WINDOWS /* - file permissions are designed differently on windows - implementation on windows to make sure the config file is owned by the user and - nobody else except Administrators group and current user of calling process, and SYSTEM account has the write permission + file permissions are designed differently on windows. + 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) fatal("Bad owner or permissions on %s", filename); diff --git a/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 b/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 index 3e92990c1..e9888e5be 100644 --- a/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 +++ b/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 @@ -1,52 +1,39 @@ -Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -Describe "Tests for authorized_keys file permission" -Tags "Scenario" { +Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking +$tC = 1 +$tI = 0 +$suite = "authorized_keys_fileperm" +Describe "Tests for authorized_keys file permission" -Tags "CI" { BeforeAll { if($OpenSSHTestInfo -eq $null) { Throw "`$OpenSSHTestInfo is null. Please run Setup-OpenSSHTestEnvironment to setup test environment." } - $testDir = "$($OpenSSHTestInfo["TestDataPath"])\authorized_keys_fileperm" + $testDir = "$($OpenSSHTestInfo["TestDataPath"])\$suite" if( -not (Test-path $testDir -PathType Container)) { $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue } $fileName = "test.txt" - $filePath = Join-Path $testDir $fileName - $logName = "log.txt" - $logPath = Join-Path $testDir $logName + $logName = "sshdlog.txt" $server = $OpenSSHTestInfo["Target"] $port = 47003 $ssouser = $OpenSSHTestInfo["SSOUser"] $PwdUser = $OpenSSHTestInfo["PasswdUser"] $ssouserProfile = $OpenSSHTestInfo["SSOUserProfile"] - $script:logNum = 0 - - # for the first time, delete the existing log files. - if ($OpenSSHTestInfo['DebugMode']) - { - Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" -Force -ErrorAction ignore - } - - Remove-Item -Path $filePath -Force -ErrorAction ignore + Remove-Item -Path (Join-Path $testDir "*$fileName") -Force -ErrorAction ignore } - AfterEach { - if( $OpenSSHTestInfo["DebugMode"]) - { - Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\failedagent$script:logNum.log" -Force -ErrorAction ignore - Copy-Item $logPath "$($script:logNum)$($logPath)" -Force -ErrorAction ignore - Clear-Content $logPath -Force -ErrorAction ignore - $script:logNum++ - - # clear the ssh-agent so that next testcase will get fresh logs. - Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" -Force -ErrorAction ignore - } - } + AfterEach { $tI++ } Context "Authorized key file permission" { - BeforeAll { + BeforeAll { + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $objUser = New-Object System.Security.Principal.NTAccount($ssouser) + $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) + $ssouserSSHProfilePath = Join-Path $ssouserProfile .testssh if(-not (Test-Path $ssouserSSHProfilePath -PathType Container)) { New-Item $ssouserSSHProfilePath -ItemType directory -Force -ErrorAction Stop | Out-Null @@ -54,66 +41,98 @@ Describe "Tests for authorized_keys file permission" -Tags "Scenario" { $authorizedkeyPath = Join-Path $ssouserProfile .testssh\authorized_keys $Source = Join-Path $ssouserProfile .ssh\authorized_keys $testknownhosts = Join-path $PSScriptRoot testdata\test_known_hosts - if(Test-Path $authorizedkeyPath) { - Set-SecureFileACL -filepath $authorizedkeyPath - } - Copy-Item $Source $ssouserSSHProfilePath -Force -ErrorAction Stop + Copy-Item $Source $ssouserSSHProfilePath -Force -ErrorAction Stop + + Adjust-UserKeyFileACL -Filepath $authorizedkeyPath -Owner $objUser -OwnerPerms "Read, Write" - Remove-Item $filePath -Force -ErrorAction Ignore Get-Process -Name sshd | Where-Object {$_.SI -ne 0} | Stop-process #add wrong password so ssh does not prompt password if failed with authorized keys Add-PasswordSetting -Pass "WrongPass" + $tI=1 } AfterAll { if(Test-Path $authorizedkeyPath) { - Set-SecureFileACL -filepath $authorizedkeyPath + Adjust-UserKeyFileACL -Filepath $authorizedkeyPath -Owner $objUser -OwnerPerms "Read, Write" Remove-Item $authorizedkeyPath -Force -ErrorAction Ignore } if(Test-Path $ssouserSSHProfilePath) { - Remove-Item $ssouserSSHProfilePath -Force -ErrorAction Ignore + Remove-Item $ssouserSSHProfilePath -Force -ErrorAction Ignore -Recurse } Remove-PasswordSetting + $tC++ } - AfterEach { - Remove-Item -Path $filePath -Force -ErrorAction ignore + BeforeEach { + $filePath = Join-Path $testDir "$tC.$tI.$fileName" + $logPath = Join-Path $testDir "$tC.$tI.$logName" + } + + It "$tC.$tI-authorized_keys-positive(Secured file and running process can access to the file)" { + #setup to have ssouser as owner and grant ssouser read and write, admins group, and local system full control + Adjust-UserKeyFileACL -Filepath $authorizedkeyPath -Owner $objUser -OwnerPerms "Read, Write" + + #Run + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow + $o = ssh -p $port $ssouser@$server -o "UserKnownHostsFile $testknownhosts" echo 1234 + $o | Should Be "1234" + + #Cleanup + Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } } - It 'Authorized key file -- positive (authorized_keys is owned by current user and running process can access to the file)' { + It "$tC.$tI-authorized_keys-positive(authorized_keys is owned by local system and running process can access to the file)" { + #setup to have system as owner and grant it full control + Set-FileOwnerAndACL -Filepath $authorizedkeyPath -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $adminAccount -Perms "FullControl" + + #Run + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow + $o = ssh -p $port $ssouser@$server -o "UserKnownHostsFile $testknownhosts" echo 1234 + $o | Should Be "1234" + + #Cleanup + Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } + } + + It "$tC.$tI-authorized_keys-positive(authorized_keys is owned by admins group and running process can access to the file)" { + #setup to have admin group as owner and grant it full control + + Set-FileOwnerAndACL -Filepath $authorizedkeyPath -Owner $adminAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $systemAccount -Perms "FullControl" + + #Run + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow + $o = ssh -p $port $ssouser@$server -o "UserKnownHostsFile $testknownhosts" echo 1234 + $o | Should Be "1234" + + #Cleanup + Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } + } + + It "$tC.$tI-authorized_keys-negative(authorized_keys is owned by other admin user)" { #setup to have current user (admin user) as owner and grant it full control - Set-SecureFileACL -filepath $authorizedkeyPath + Set-FileOwnerAndACL -Filepath $authorizedkeyPath -Owner $currentUser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $adminAccount -Perms "FullControl" #Run Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow - $o = ssh -p $port $ssouser@$server -o "UserKnownHostsFile $testknownhosts" echo 1234 - $o | Should Be "1234" + ssh -p $port -E $filePath -o "UserKnownHostsFile $testknownhosts" $ssouser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + $matches = Get-Content $filePath | Select-String -pattern "^Permission denied" + $matches.Count | Should BeGreaterThan 2 #Cleanup Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } } - It 'Authorized key file -- positive (Secured file and sshd can access to the file)' { - #setup to have ssouser as owner and grant it full control - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - Set-SecureFileACL -filepath $authorizedkeyPath -Owner $objUser - - #add running process account Read access the file authorized_keys - $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) - Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $currentUser -Perm "Read" - - #Run - Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow - $o = ssh -p $port $ssouser@$server -o "UserKnownHostsFile $testknownhosts" echo 1234 - $o | Should Be "1234" - - #Cleanup - Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } - } - - It 'Authorized key file -- negative (other account can access private key file)' { + It "$tC.$tI-authorized_keys-negative(other account can access private key file)" { #setup to have current user as owner and grant it full control - Set-SecureFileACL -filepath $authorizedkeyPath + Set-FileOwnerAndACL -Filepath $authorizedkeyPath -Owner $objUser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $adminAccount -Perms "FullControl" + #add $PwdUser to access the file authorized_keys $objPwdUser = New-Object System.Security.Principal.NTAccount($PwdUser) Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $objPwdUser -Perm "Read" @@ -122,44 +141,60 @@ Describe "Tests for authorized_keys file permission" -Tags "Scenario" { Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow ssh -p $port -E $filePath -o "UserKnownHostsFile $testknownhosts" $ssouser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 - $matches = Get-Content $filePath | Select-String -pattern "Permission denied" - $matches.Count | Should Not Be 0 + $matches = Get-Content $filePath | Select-String -pattern "^Permission denied" + $matches.Count | Should BeGreaterThan 2 #Cleanup Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } } - It 'Authorized key file -- negative (the authorized_keys has wrong owner)' { - #setup to have ssouser as owner and grant it full control + It "$tC.$tI-authorized_keys-negative(authorized_keys is owned by other non-admin user)" { + #setup to have PwdUser as owner and grant it full control $objPwdUser = New-Object System.Security.Principal.NTAccount($PwdUser) - Set-SecureFileACL -filepath $authorizedkeyPath -owner $objPwdUser - - #add current user full access the file authorized_keys - $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) - Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $currentUser -Perm "FullControl" + Set-FileOwnerAndACL -Filepath $authorizedkeyPath -owner $objPwdUser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $adminAccount -Perms "FullControl" #Run Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow - ssh -p $port -E $filePath -o "UserKnownHostsFile $testknownhosts" $ssouser@$server echo 1234 + ssh -p $port -E $FilePath -o "UserKnownHostsFile $testknownhosts" $ssouser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 - $matches = Get-Content $filePath | Select-String -pattern "Permission denied" - $matches.Count | Should Not Be 0 + $matches = Get-Content $filePath | Select-String -pattern "^Permission denied" + $matches.Count | Should BeGreaterThan 2 #Cleanup Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } } - It 'Authorized key file -- negative (the running process does not have read access to the authorized_keys)' { - #setup to have ssouser as owner and grant it full control - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - Set-SecureFileACL -filepath $authorizedkeyPath -Owner $objUser + It "$tC.$tI-authorized_keys-negative(the running process does not have read access to the authorized_keys)" { + #setup to have ssouser as owner and grant it full control + Set-FileOwnerAndACL -Filepath $authorizedkeyPath -Owner $objUser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $systemAccount -Perms "FullControl" #Run Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow ssh -p $port -E $filePath -o "UserKnownHostsFile $testknownhosts" $ssouser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 - $matches = Get-Content $filePath | Select-String -pattern "Permission denied" - $matches.Count | Should Not Be 0 + $matches = Get-Content $filePath | Select-String -pattern "^Permission denied" + $matches.Count | Should BeGreaterThan 2 + + #Cleanup + Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } + } + + It "$tC.$tI-authorized_keys-negative(the owner of authorized_keys file is denied to access to it)" { + Set-FileOwnerAndACL -Filepath $authorizedkeyPath -Owner $objUser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $adminAccount -Perms "FullControl" + #add rule to denied the owner + Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $objUser -Perm "Read" -AccessType Deny + + #Run + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-o `"AuthorizedKeysFile .testssh/authorized_keys`"", "-E $logPath") -NoNewWindow + ssh -p $port -E $filePath -o "UserKnownHostsFile $testknownhosts" $ssouser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + $matches = Get-Content $filePath | Select-String -pattern "^Permission denied" + $matches.Count | Should BeGreaterThan 2 #Cleanup Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } diff --git a/regress/pesterTests/Cfginclude.Tests.ps1 b/regress/pesterTests/Cfginclude.Tests.ps1 index 9e472c9e1..a2bbe4361 100644 --- a/regress/pesterTests/Cfginclude.Tests.ps1 +++ b/regress/pesterTests/Cfginclude.Tests.ps1 @@ -1,4 +1,8 @@ -Describe "Tests for ssh config" -Tags "Scenario" { +Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking +$tC = 1 +$tI = 0 +$suite = "authorized_keys_fileperm" +Describe "Tests for ssh config" -Tags "CI" { BeforeAll { if($OpenSSHTestInfo -eq $null) { @@ -9,144 +13,133 @@ { $null = New-Item $OpenSSHTestInfo["TestDataPath"] -ItemType directory -Force -ErrorAction SilentlyContinue } - $testDir = "$($OpenSSHTestInfo["TestDataPath"])\cfginclude" - $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue - $fileName = "test.txt" - $filePath = Join-Path $testDir $fileName - $logName = "log.txt" - $logPath = Join-Path $testDir $logName + $testDir = "$($OpenSSHTestInfo["TestDataPath"])\$suite" + if( -not (Test-path $testDir -PathType Container)) + { + $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue + } + $logName = "testlog.txt" $server = $OpenSSHTestInfo["Target"] $port = $OpenSSHTestInfo["Port"] $ssouser = $OpenSSHTestInfo["SSOUser"] - $script:logNum = 0 # for the first time, delete the existing log files. if ($OpenSSHTestInfo['DebugMode']) { Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" -Force -ErrorAction ignore Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd.log" -Force -ErrorAction ignore + Remove-Item -Path (Join-Path $testDir "*log*.log") -Force -ErrorAction ignore } - Remove-Item -Path $filePath -Force -ErrorAction ignore - - function Set-SecureFileACL - { - [CmdletBinding()] - param( - [string]$FilePath, - [System.Security.Principal.NTAccount]$Owner = $null, - [System.Security.AccessControl.FileSystemAccessRule]$ACE = $null - ) - - $myACL = Get-ACL -Path $FilePath - $myACL.SetAccessRuleProtection($True, $True) - Set-Acl -Path $FilePath -AclObject $myACL - - $myACL = Get-ACL $FilePath - if($owner -ne $null) - { - $myACL.SetOwner($Owner) - } - - if($myACL.Access) - { - $myACL.Access | % { - if (($_ -ne $null) -and ($_.IdentityReference.Value -ine "BUILTIN\Administrators") -and - ($_.IdentityReference.Value -ine "NT AUTHORITY\SYSTEM") -and - ($_.IdentityReference.Value -ine "$(whoami)")) - { - if(-not ($myACL.RemoveAccessRule($_))) - { - throw "failed to remove access of $($_.IdentityReference.Value) rule in setup " - } - } - } - } - if($ACE -ne $null) - { - $myACL.AddAccessRule($ACE) - } - - Set-Acl -Path $FilePath -AclObject $myACL - } - } - - AfterAll { - Remove-Item -Path $filePath -Force -ErrorAction ignore + + Remove-Item -Path (Join-Path $testDir "*logName") -Force -ErrorAction ignore } AfterEach { if( $OpenSSHTestInfo["DebugMode"]) { - Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\failedagent$script:logNum.log" -Force -ErrorAction ignore - Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd.log" "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\failedsshd$script:logNum.log" -Force -ErrorAction ignore - Copy-Item $logPath "$($script:logNum)$($logPath)" -Force -ErrorAction ignore - Clear-Content $logPath -Force -ErrorAction ignore - $script:logNum++ + Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" "$testDir\agentlog$tC.$tI.log" -Force -ErrorAction ignore + Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd.log" "$testDir\sshdlog$tC.$tI.log" -Force -ErrorAction ignore - # clear the ssh-agent, sshd logs so that next testcase will get fresh logs. + #Clear the ssh-agent, sshd logs so that next testcase will get fresh logs. Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" -Force -ErrorAction ignore Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd.log" -Force -ErrorAction ignore } - Remove-Item -Path $filePath -Force -ErrorAction ignore + $tI++ } - Context "User SSHConfig -- ReadConfig" { + Context "$tC-User SSHConfig--ReadConfig" { BeforeAll { + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $objUser = New-Object System.Security.Principal.NTAccount($ssouser) + $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) + $userConfigFile = Join-Path $home ".ssh\config" - Copy-item "$PSScriptRoot\testdata\ssh_config" $userConfigFile -force + if( -not (Test-path $userConfigFile) ) { + Copy-item "$PSScriptRoot\testdata\ssh_config" $userConfigFile -force + } $oldACL = Get-ACL $userConfigFile + $tI=1 } + + BeforeEach { + $logPath = Join-Path $testDir "$tC.$tI.$logName" + } + AfterEach { Set-Acl -Path $userConfigFile -AclObject $oldACL } - AfterAll { - Remove-Item -Path $userConfigFile -Force -ErrorAction ignore + AfterAll { + $tC++ } - It 'User SSHConfig -- ReadConfig (admin user is the owner)' { + It "$tC.$tI-User SSHConfig-ReadConfig positive (current logon user is the owner)" { #setup - Set-SecureFileACL -filepath $userConfigFile + Set-FileOwnerAndACL -Filepath $userConfigFile -Owner $currentUser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $userConfigFile -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $userConfigFile -User $adminAccount -Perms "FullControl" #Run - $str = "ssh -p $($port) -E $logPath $($Options) $($ssouser)@$($server) hostname > $filePath" - cmd /c $str - $LASTEXITCODE | Should Be 0 - - #validate file content. - Get-Content $filePath | Should be $env:COMPUTERNAME - - #clean up - Set-Acl -Path $userConfigFile -AclObject $oldACL + $o = ssh test_target echo 1234 + $o | Should Be "1234" } - It 'User SSHConfig -- ReadConfig (wrong owner)' { + + It "$tC.$tI-User SSHConfig-ReadConfig positive (local system is the owner)" { #setup - Set-SecureFileACL -filepath $userConfigFile -owner $ssouser + Set-FileOwnerAndACL -Filepath $userConfigFile -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $userConfigFile -User $adminAccount -Perms "FullControl" #Run - $str = "ssh -p $($port) -E $logPath $($Options) $($ssouser)@$($server) hostname > $filePath" - cmd /c $str - - #clean up - $LASTEXITCODE | Should Not Be 0 + $o = ssh test_target echo 1234 + $o | Should Be "1234" } - It 'User SSHConfig -- ReadConfig (wrong permission)' { - #setup - $owner = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) - - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - $objACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` - ($objUser, "Read, Write", "None", "None", "Allow") - Set-SecureFileACL -filepath $userConfigFile -owner $owner -Ace $objACE + It "$tC.$tI-User SSHConfig-ReadConfig positive (admin is the owner)" { + #setup + Set-FileOwnerAndACL -Filepath $userConfigFile -Owner $adminAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $userConfigFile -User $systemAccount -Perms "FullControl" #Run - $str = "ssh -p $($port) -E $logPath $($Options) $($ssouser)@$($server) hostname > $filePath" - cmd /c $str + $o = ssh test_target echo 1234 + $o | Should Be "1234" + } - #clean up - $LASTEXITCODE | Should Not Be 0 + It "$tC.$tI-User SSHConfig-ReadConfig negative (wrong owner)" { + #setup + Set-FileOwnerAndACL -Filepath $userConfigFile -Owner $ssouser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $userConfigFile -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $userConfigFile -User $adminAccount -Perms "FullControl" + + #Run + cmd /c "ssh test_target echo 1234 2> $logPath" + $LASTEXITCODE | Should Not Be 0 + Get-Content $logPath | Should Match "^Bad owner or permissions on [a-fA-F]:[/\\]{1,}Users[/\\]{1,}\w+[/\\]{1,}.ssh[/\\]{1,}config$" + } + + It "$tC.$tI-User SSHConfig-ReadConfig negative (others has permission)" { + #setup + Set-FileOwnerAndACL -Filepath $userConfigFile -Owner $currentUser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $userConfigFile -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $userConfigFile -User $adminAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $userConfigFile -User $objUser -Perms "Read" + + #Run + cmd /c "ssh test_target echo 1234 2> $logPath" + $LASTEXITCODE | Should Not Be 0 + Get-Content $logPath | Should Match "^Bad owner or permissions on [a-fA-F]:[/\\]{1,}Users[/\\]{1,}\w+[/\\]{1,}.ssh[/\\]{1,}config$" + } + It "$tC.$tI-User SSHConfig-ReadConfig negative (owner is denied Read permission)" { + #setup + Set-FileOwnerAndACL -Filepath $userConfigFile -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $userConfigFile -User $adminAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $userConfigFile -User $systemAccount -Perms "Read" -AccessType Deny + + #Run + cmd /c "ssh test_target echo 1234 2> $logPath" + $LASTEXITCODE | Should Not Be 0 + Get-Content $logPath | Should Match "^Bad owner or permissions on [a-fA-F]:[/\\]{1,}Users[/\\]{1,}\w+[/\\]{1,}.ssh[/\\]{1,}config$" } } } diff --git a/regress/pesterTests/CommonUtils.psm1 b/regress/pesterTests/CommonUtils.psm1 index e8374a670..49f345a5a 100644 --- a/regress/pesterTests/CommonUtils.psm1 +++ b/regress/pesterTests/CommonUtils.psm1 @@ -31,15 +31,98 @@ function Get-Platform { } } -function Set-SecureFileACL +<# +.Synopsis + user key should be owned by current user account + private key can be accessed only by the file owner, localsystem and Administrators + public user key can be accessed by only file owner, localsystem and Administrators and read by everyone + +.Outputs + N/A + +.Inputs + FilePath - The path to the file + Owner - The file owner + OwnerPerms - The permissions grant to owner +#> +function Adjust-UserKeyFileACL +{ + param ( + [parameter(Mandatory=$true)] + [string]$FilePath, + [System.Security.Principal.NTAccount] $Owner = $null, + [System.Security.AccessControl.FileSystemRights[]] $OwnerPerms = $null + ) + + $myACL = Get-ACL $FilePath + $myACL.SetAccessRuleProtection($True, $FALSE) + Set-Acl -Path $FilePath -AclObject $myACL + + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $everyoneAccount = New-Object System.Security.Principal.NTAccount("EveryOne") + $myACL = Get-ACL $FilePath + + $actualOwner = $null + if($Owner -eq $null) + { + $actualOwner = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) + } + else + { + $actualOwner = $Owner + } + + $myACL.SetOwner($actualOwner) + + if($myACL.Access) + { + $myACL.Access | % { + if(-not ($myACL.RemoveAccessRule($_))) + { + throw "failed to remove access of $($_.IdentityReference.Value) rule in setup " + } + } + } + + $adminACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($adminAccount, "FullControl", "None", "None", "Allow") + $myACL.AddAccessRule($adminACE) + + $systemACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($systemAccount, "FullControl", "None", "None", "Allow") + $myACL.AddAccessRule($systemACE) + + if(-not ($actualOwner.Equals($adminAccount)) -and (-not $actualOwner.Equals($systemAccount)) -and $OwnerPerms) + { + $OwnerPerms | % { + $ownerACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($actualOwner, $_, "None", "None", "Allow") + $myACL.AddAccessRule($ownerACE) + } + } + + if($FilePath.EndsWith(".pub")) + { + $everyoneAce = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ("Everyone", "Read", "None", "None", "Allow") + $myACL.AddAccessRule($everyoneAce) + } + + Set-Acl -Path $FilePath -AclObject $myACL +} + +function Set-FileOwnerAndACL { param( + [parameter(Mandatory=$true)] [string]$FilePath, - [System.Security.Principal.NTAccount]$Owner = $null + [System.Security.Principal.NTAccount]$Owner = $null, + [System.Security.AccessControl.FileSystemRights[]] $OwnerPerms = @("Read", "Write") ) $myACL = Get-ACL -Path $FilePath - $myACL.SetAccessRuleProtection($True, $True) + $myACL.SetAccessRuleProtection($True, $FALSE) Set-Acl -Path $FilePath -AclObject $myACL $myACL = Get-ACL $FilePath @@ -65,9 +148,14 @@ function Set-SecureFileACL } } - $objACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` - ($actualOwner, "FullControl", "None", "None", "Allow") - $myACL.AddAccessRule($objACE) + if($OwnerPerms) + { + $OwnerPerms | % { + $ownerACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($actualOwner, $_, "None", "None", "Allow") + $myACL.AddAccessRule($ownerACE) + } + } Set-Acl -Path $FilePath -AclObject $myACL } @@ -75,18 +163,25 @@ function Set-SecureFileACL function Add-PermissionToFileACL { param( + [parameter(Mandatory=$true)] [string]$FilePath, [System.Security.Principal.NTAccount] $User, - [System.Security.AccessControl.FileSystemRights]$Perm + [System.Security.AccessControl.FileSystemRights[]]$Perms, + [System.Security.AccessControl.AccessControlType] $AccessType = "Allow" ) - $myACL = Get-ACL $filePath - - $objACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` - ($User, $perm, "None", "None", "Allow") - $myACL.AddAccessRule($objACE) + $myACL = Get-ACL $FilePath - Set-Acl -Path $filePath -AclObject $myACL + if($Perms) + { + $Perms | % { + $userACE = New-Object System.Security.AccessControl.FileSystemAccessRule ` + ($User, $_, "None", "None", $AccessType) + $myACL.AddAccessRule($userACE) + } + } + + Set-Acl -Path $FilePath -AclObject $myACL } function Add-PasswordSetting diff --git a/regress/pesterTests/Hostkey_fileperm.Tests.ps1 b/regress/pesterTests/Hostkey_fileperm.Tests.ps1 index 8bead1a9f..b6b6fb975 100644 --- a/regress/pesterTests/Hostkey_fileperm.Tests.ps1 +++ b/regress/pesterTests/Hostkey_fileperm.Tests.ps1 @@ -1,147 +1,164 @@ -Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -Describe "Tests for host keys file permission" -Tags "Scenario" { +Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking +$tC = 1 +$tI = 0 +$suite = "hostkey_fileperm" +Describe "Tests for host keys file permission" -Tags "CI" { BeforeAll { if($OpenSSHTestInfo -eq $null) { Throw "`$OpenSSHTestInfo is null. Please run Setup-OpenSSHTestEnvironment to setup test environment." } - $testDir = "$($OpenSSHTestInfo["TestDataPath"])\hostkey_fileperm" + $testDir = "$($OpenSSHTestInfo["TestDataPath"])\$suite" if( -not (Test-path $testDir -PathType Container)) { $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue } - $fileName = "test.txt" - $filePath = Join-Path $testDir $fileName - $logName = "log.txt" - $logPath = Join-Path $testDir $logName + $logName = "sshdlog.txt" $port = 47003 $ssouser = $OpenSSHTestInfo["SSOUser"] $script:logNum = 0 - - # for the first time, delete the existing log files. - if ($OpenSSHTestInfo['DebugMode']) - { - Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" -Force -ErrorAction ignore - } - - Remove-Item -Path $filePath -Force -ErrorAction ignore + Remove-Item -Path (Join-Path $testDir "*$logName") -Force -ErrorAction ignore } - AfterEach { - if( $OpenSSHTestInfo["DebugMode"]) - { - Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\failedagent$script:logNum.log" -Force -ErrorAction ignore - Copy-Item $logPath "$($script:logNum)$($logPath)" -Force -ErrorAction ignore - Clear-Content $logPath -Force -ErrorAction ignore - $script:logNum++ - - # clear the ssh-agent so that next testcase will get fresh logs. - Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" -Force -ErrorAction ignore - } - Remove-Item -Path $filePath -Force -ErrorAction ignore - } + AfterEach { $tI++ } - Context "Host key files permission" { + Context "$tC - Host key files permission" { BeforeAll { - $hostKeyFilePath = "$($OpenSSHTestInfo['OpenSSHBinPath'])\hostkeyFilePermTest_ed25519_key" - if(Test-path $hostKeyFilePath -PathType Leaf){ - Set-SecureFileACL -filepath $hostKeyFilePath + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $objUser = New-Object System.Security.Principal.NTAccount($ssouser) + $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) + $everyone = New-Object System.Security.Principal.NTAccount("EveryOne") + + $hostKeyFilePath = join-path $testDir hostkeyFilePermTest_ed25519_key + if(Test-path $hostKeyFilePath -PathType Leaf) { + Set-FileOwnerAndACL -filepath $hostKeyFilePath } if(Test-path "$hostKeyFilePath.pub" -PathType Leaf){ - Set-SecureFileACL -filepath "$hostKeyFilePath.pub" + Set-FileOwnerAndACL -filepath "$hostKeyFilePath.pub" } Remove-Item -path "$hostKeyFilePath*" -Force -ErrorAction Ignore - ssh-keygen.exe -t ed25519 -f $hostKeyFilePath -P `"`" - - Remove-Item $filePath -Force -ErrorAction Ignore + ssh-keygen.exe -t ed25519 -f $hostKeyFilePath -P `"`" Get-Process -Name sshd | Where-Object {$_.SI -ne 0} | Stop-process + $tI=1 } - AfterEach { - Remove-Item -Path $filePath -Force -ErrorAction ignore + BeforeEach { + $logPath = Join-Path $testDir "$tC.$tI.$logName" } AfterAll { if(Test-path $hostKeyFilePath -PathType Leaf){ - Set-SecureFileACL -filepath $hostKeyFilePath + Adjust-UserKeyFileACL -Filepath $hostKeyFilePath -Owner $systemAccount } if(Test-path "$hostKeyFilePath.pub" -PathType Leaf){ - Set-SecureFileACL -filepath "$hostKeyFilePath.pub" + Adjust-UserKeyFileACL -Filepath "$hostKeyFilePath.pub" -Owner $systemAccount } + $tC++ } - It 'Host keys -- positive (Secured private key and sshd can access to public key file)' { - #setup to have current user as owner and grant it full control - Set-SecureFileACL -filepath $hostKeyFilePath - #grant sshd Read permission to public key - Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User "NT Service\sshd" -Perm "Read" + It "$tC.$tI-Host keys-positive (both public and private keys are owned by admin groups and running process can access to public key file)" { + Set-FileOwnerAndACL -Filepath $hostKeyFilePath -Owner $adminAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $systemAccount -Perms "FullControl" + + Set-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $adminAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $everyOne -Perms "Read" #Run - Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $filePath") -NoNewWindow + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } - #validate file content does not contain unprotected. - $matches = Get-Content $filePath | Select-String -pattern "UNPROTECTED PRIVATE KEY FILE!" - $matches.Count | Should Be 0 + #validate file content does not contain unprotected info. + $logPath | Should Not Contain "UNPROTECTED PRIVATE KEY FILE!" } - It 'Host keys -- negative (other account can access private key file)' { - #setup to have current user as owner and grant it full control - Set-SecureFileACL -filepath $hostKeyFilePath - #add ssouser to access the private key - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $objUser -Perm "Read" - - #grant sshd Read permission to the public key - Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User "NT Service\sshd" -Perm "Read" + It "$tC.$tI-Host keys-positive (both public and private keys are owned by system and running process can access to public key file)" { + Set-FileOwnerAndACL -Filepath $hostKeyFilePath -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $adminAccount -Perms "Read" + + Set-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $adminAccount -Perms "Read" #Run - Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $filePath") -NoNewWindow + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } + - #validate file content does not contain unprotected. - $matches = Get-Content $filePath | Select-String -pattern "key_load_private: bad permissions" - $matches.Count | Should Be 1 + #validate file content does not contain unprotected info. + $logPath | Should Not Contain "UNPROTECTED PRIVATE KEY FILE!" } - It 'Host keys -- negative (the private key has wrong owner)' { + It "$tC.$tI-Host keys-negative (other account can access private key file)" { + Set-FileOwnerAndACL -Filepath $hostKeyFilePath -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $adminAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $objUser -Perms "Read" + + Set-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $adminAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $everyOne -Perms "Read" + + #Run + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow + Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } + + #validate file content contains unprotected info. + $logPath | Should Contain "key_load_private: bad permissions" + } + + It "$tC.$tI-Host keys-negative (the private key has wrong owner)" { #setup to have ssouser as owner and grant it full control - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - Set-SecureFileACL -filepath $hostKeyFilePath -owner $objUser - $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) - Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $currentUser -Perm "FullControl" + Set-FileOwnerAndACL -FilePath $hostKeyFilePath -Owner $objUser -OwnerPerms "Read","Write" + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $adminAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $systemAccount -Perms "FullControl" - #grant sshd Read permission to the public key - Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User "NT Service\sshd" -Perm "Read" + Set-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $adminAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $systemAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $everyOne -Perms "Read" #Run - Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $filePath") -NoNewWindow + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } - #validate file content does not contain unprotected. - $matches = Get-Content $filePath | Select-String -pattern "key_load_private: bad permissions" - $matches.Count | Should Be 1 + #validate file content contains unprotected info. + $logPath | Should Contain "key_load_private: bad permissions" } - It 'Host keys -- negative (the running process does not have read access to pub key)' { + It "$tC.$tI-Host keys-negative (the running process does not have read access to public key)" { #setup to have ssouser as owner and grant it full control - Set-SecureFileACL -filepath $hostKeyFilePath + Set-FileOwnerAndACL -FilePath $hostKeyFilePath -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $adminAccount -Perms "Read" - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - Set-SecureFileACL -filepath "$hostKeyFilePath.pub" -owner $objUser - - #grant current user Read permission to public key - Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $objUser -Perm "Read" + Set-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $systemAccount -OwnerPerms "FullControl" #Run - Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $filePath") -NoNewWindow + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow + Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } + + #validate file content contains unprotected info. + $logPath | Should Contain "key_load_public: Permission denied" + } + + It "$tC.$tI-Host keys-negative (the owner of private host key is denied Read access to private key)" { + #setup to have ssouser as owner and grant it full control + Set-FileOwnerAndACL -FilePath $hostKeyFilePath -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $adminAccount -Perms "FullControl" + + Set-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $adminAccount -Perms "FullControl" + Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $everyOne -Perms "Read" + + #add rule to denied the owner + Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $systemAccount -Perms "Read" -AccessType Deny + + #Run + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-h $hostKeyFilePath", "-E $logPath") -NoNewWindow Get-Process -Name sshd | % { if($_.SI -ne 0) { Start-sleep 1; Stop-Process $_; Start-sleep 1 } } #validate file content does not contain unprotected. - $matches = Get-Content $filePath | Select-String -pattern "key_load_public: Permission denied" - $matches.Count | Should Be 1 + $logPath | Should Contain "key_load_private: bad permissions" } } } diff --git a/regress/pesterTests/KeyUtils.Tests.ps1 b/regress/pesterTests/KeyUtils.Tests.ps1 index 9744c9b11..009200e09 100644 --- a/regress/pesterTests/KeyUtils.Tests.ps1 +++ b/regress/pesterTests/KeyUtils.Tests.ps1 @@ -1,9 +1,10 @@ -$tC = 1 +Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking +$tC = 1 $tI = 0 $suite = "keyutils" -Describe "E2E scenarios for ssh key management" -Tags "Scenario" { - BeforeAll { +Describe "E2E scenarios for ssh key management" -Tags "CI" { + BeforeAll { if($OpenSSHTestInfo -eq $null) { Throw "`$OpenSSHTestInfo is null. Please run Setup-OpenSSHTestEnvironment to setup test environment." @@ -16,23 +17,60 @@ Describe "E2E scenarios for ssh key management" -Tags "Scenario" { } $keypassphrase = "testpassword" - $keytypes = @("rsa","dsa","ecdsa","ed25519") - #only validate owner and ACE of the file - function ValidKeyFile { - param($Path) + $keytypes = @("rsa","dsa","ecdsa","ed25519") + + $ssouser = $OpenSSHTestInfo["SSOUser"] - $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) - $myACL = Get-ACL $Path + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminsAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) + $everyone = New-Object System.Security.Principal.NTAccount("EveryOne") + $objUser = New-Object System.Security.Principal.NTAccount($ssouser) + + #only validate owner and ACEs of the file + function ValidateKeyFile { + param([string]$FilePath) + + $myACL = Get-ACL $FilePath $myACL.Owner.Equals($currentUser.Value) | Should Be $true $myACL.Access | Should Not Be $null - $myACL.Access.Count | Should Be 1 + if($FilePath.EndsWith(".pub")) { + $myACL.Access.Count | Should Be 4 + $identities = @($systemAccount.Value, $adminsAccount.Value, $currentUser.Value, $everyone.Value) + } + else { + $myACL.Access.Count | Should Be 3 + $identities = @($systemAccount.Value, $adminsAccount.Value, $currentUser.Value) + } + + foreach ($a in $myACL.Access) { + $a.IdentityReference.Value -in $identities | Should Be $true + + switch ($a.IdentityReference.Value) + { + {$_ -in @($systemAccount.Value, $adminsAccount.Value)} + { + $a.FileSystemRights | Should Be "FullControl" + break; + } + + $currentUser.Value + { + $a.FileSystemRights | Should Be "Write, Read, Synchronize" + break; + } + $everyone.Value + { + $a.FileSystemRights | Should Be "Read, Synchronize" + break; + } + } - $myACL.Access[0].IdentityReference.Equals($currentUser) | Should Be $true - $myACL.Access[0].AccessControlType | Should Be ([System.Security.AccessControl.AccessControlType]::Allow) - $myACL.Access[0].FileSystemRights | Should Be ([System.Security.AccessControl.FileSystemRights]::FullControl) - $myACL.Access[0].IsInherited | Should Be $false - $myACL.Access[0].InheritanceFlags | Should Be ([System.Security.AccessControl.InheritanceFlags]::None) - $myACL.Access[0].PropagationFlags | Should Be ([System.Security.AccessControl.PropagationFlags]::None) + $a.AccessControlType | Should Be ([System.Security.AccessControl.AccessControlType]::Allow) + $a.IsInherited | Should Be $false + $a.InheritanceFlags | Should Be ([System.Security.AccessControl.InheritanceFlags]::None) + $a.PropagationFlags | Should Be ([System.Security.AccessControl.PropagationFlags]::None) + } } } @@ -44,41 +82,40 @@ Describe "E2E scenarios for ssh key management" -Tags "Scenario" { AfterEach {$tI++;} - Context "$tC - ssh-keygen all key types" { + Context "$tC -ssh-keygen all key types" { BeforeAll {$tI=1} AfterAll{$tC++} It "$tC.$tI - Keygen -A" { - $cd = (pwd).Path - cd $testDir + Push-Location $testDir remove-item ssh_host_*_key* -ErrorAction SilentlyContinue ssh-keygen -A + Pop-Location - Get-ChildItem ssh_host_*_key | % { - ValidKeyFile -Path $_.FullName + Get-ChildItem (join-path $testDir ssh_host_*_key) | % { + ValidateKeyFile -FilePath $_.FullName } - Get-ChildItem ssh_host_*_key.pub | % { - ValidKeyFile -Path $_.FullName - } - cd $cd + Get-ChildItem (join-path $testDir ssh_host_*_key.pub) | % { + ValidateKeyFile -FilePath $_.FullName + } } It "$tC.$tI - Keygen -t -f" { foreach($type in $keytypes) { $keyPath = Join-Path $testDir "id_$type" - remove-item $keyPath -ErrorAction SilentlyContinue + remove-item $keyPath -ErrorAction ignore ssh-keygen -t $type -P $keypassphrase -f $keyPath - ValidKeyFile -Path $keyPath - ValidKeyFile -Path "$keyPath.pub" + ValidateKeyFile -FilePath $keyPath + ValidateKeyFile -FilePath "$keyPath.pub" } } } # This uses keys generated in above context - Context "$tC - ssh-add test cases" { + Context "$tC -ssh-add test cases" { BeforeAll {$tI=1} AfterAll{$tC++} @@ -110,8 +147,7 @@ Describe "E2E scenarios for ssh key management" -Tags "Scenario" { It "$tC.$tI - ssh-add - add and remove all key types" { #set up SSH_ASKPASS - if (-not($env:DISPLAY)) { $env:DISPLAY = 1 } - $env:SSH_ASKPASS="$($env:ComSpec) /c echo $($keypassphrase)" + Add-PasswordSetting -Pass $keypassphrase $nullFile = join-path $testDir ("$tC.$tI.nullfile") $null > $nullFile @@ -124,8 +160,7 @@ Describe "E2E scenarios for ssh key management" -Tags "Scenario" { } #remove SSH_ASKPASS - if ($env:DISPLAY -eq 1) { Remove-Item env:\DISPLAY } - remove-item "env:SSH_ASKPASS" -ErrorAction SilentlyContinue + Remove-PasswordSetting #ensure added keys are listed $allkeys = ssh-add -L @@ -135,7 +170,7 @@ Describe "E2E scenarios for ssh key management" -Tags "Scenario" { { $keyPath = Join-Path $testDir "id_$type" $pubkeyraw = ((Get-Content "$keyPath.pub").Split(' '))[1] - ($allkeys | foreach {$_.Contains($pubkeyraw)}).Contains($true) | Should Be $true + ($allkeys | where { $_.contains($pubkeyraw) }).count | Should Be 1 } #delete added keys @@ -153,43 +188,157 @@ Describe "E2E scenarios for ssh key management" -Tags "Scenario" { { $keyPath = Join-Path $testDir "id_$type" $pubkeyraw = ((Get-Content "$keyPath.pub").Split(' '))[1] - if ($allkeys.Count -eq 1) { - $allkeys.Contains($pubkeyraw) | Should Be $false - } - else { - ($allkeys | foreach {$_.Contains($pubkeyraw)}).Contains($true) | Should Be $false - } + ($allkeys | where { $_.contains($pubkeyraw) }).count | Should Be 0 + } + } + } - } + Context "$tC-ssh-add key files with different file perms" { + BeforeAll { + $keyFileName = "sshadd_userPermTestkey_ed25519" + $keyFilePath = Join-Path $testDir $keyFileName + Remove-Item -path "$keyFilePath*" -Force -ErrorAction Ignore + ssh-keygen.exe -t ed25519 -f $keyFilePath -P $keypassphrase + #set up SSH_ASKPASS + Add-PasswordSetting -Pass $keypassphrase + $tI=1 + } + BeforeEach { + $nullFile = join-path $testDir ("$tC.$tI.nullfile") + $null > $nullFile + } + AfterEach { + if(Test-Path $keyFilePath) { + Adjust-UserKeyFileACL -FilePath $keyFilePath -Owner $currentUser -OwnerPerms "Read, Write" + } + } + + AfterAll { + #remove SSH_ASKPASS + Remove-PasswordSetting + $tC++ + } + + It "$tC.$tI- ssh-add - positive (Secured private key owned by current user)" { + #setup to have current user as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $currentUser -OwnerPerms "FullControl" + + # for ssh-add to consume SSh_ASKPASS, stdin should not be TTY + cmd /c "ssh-add $keyFilePath < $nullFile 2> nul" + $LASTEXITCODE | Should Be 0 + $allkeys = ssh-add -L + $pubkeyraw = ((Get-Content "$keyFilePath.pub").Split(' '))[1] + ($allkeys | where { $_.contains($pubkeyraw) }).count | Should Be 1 + #clean up + cmd /c "ssh-add -d $keyFilePath 2> nul " + } + + It "$tC.$tI - ssh-add - positive (Secured private key owned by Administrators group)" { + #setup to have local admin group as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $adminsAccount -OwnerPerms "FullControl" + + # for ssh-add to consume SSh_ASKPASS, stdin should not be TTY + cmd /c "ssh-add $keyFilePath < $nullFile 2> nul " + $LASTEXITCODE | Should Be 0 + $allkeys = ssh-add -L + $pubkeyraw = ((Get-Content "$keyFilePath.pub").Split(' '))[1] + ($allkeys | where { $_.contains($pubkeyraw) }).count | Should Be 1 + + #clean up + cmd /c "ssh-add -d $keyFilePath 2> nul " + } + + It "$tC.$tI - ssh-add - positive (Secured private key owned by local system group)" { + #setup to have local admin group as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $keyFilePath -User $adminsAccount -Perm "FullControl" + + # for ssh-add to consume SSh_ASKPASS, stdin should not be TTY + cmd /c "ssh-add $keyFilePath < $nullFile 2> nul " + $LASTEXITCODE | Should Be 0 + $allkeys = ssh-add -L + $pubkeyraw = ((Get-Content "$keyFilePath.pub").Split(' '))[1] + ($allkeys | where { $_.contains($pubkeyraw) }).count | Should Be 1 + + #clean up + cmd /c "ssh-add -d $keyFilePath 2> nul " } + It "$tC.$tI- ssh-add - negative (other account can access private key file)" { + #setup to have current user as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $currentUser -OwnerPerms "FullControl" + + #add ssouser to access the private key + Add-PermissionToFileACL -FilePath $keyFilePath -User $objUser -Perm "Read" + + cmd /c "ssh-add $keyFilePath < $nullFile 2> nul " + $LASTEXITCODE | Should Not Be 0 + + $allkeys = ssh-add -L + $pubkeyraw = ((Get-Content "$keyFilePath.pub").Split(' '))[1] + ($allkeys | where { $_.contains($pubkeyraw) }).count | Should Be 0 + } + + It "$tC.$tI - ssh-add - negative (the private key has wrong owner)" { + #setup to have ssouser as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -owner $objUser -OwnerPerms "Read, Write" + Add-PermissionToFileACL -FilePath $keyFilePath -User $adminsAccount -Perm "FullControl" + + cmd /c "ssh-add $keyFilePath < $nullFile 2> nul " + $LASTEXITCODE | Should Not Be 0 + + $allkeys = ssh-add -L + $pubkeyraw = ((Get-Content "$keyFilePath.pub").Split(' '))[1] + ($allkeys | where { $_.contains($pubkeyraw) }).count | Should Be 0 + } + + It "$tC.$tI - ssh-add- negative (the owner is denied Read perm on private key)" { + #setup to have local ssytem account as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $keyFilePath -User $adminsAccount -Perm "FullControl" + #deny owner + Add-PermissionToFileACL -FilePath $keyFilePath -User $systemAccount -Perm "Read, Write" -AccessType Deny + + cmd /c "ssh-add $keyFilePath < $nullFile 2> nul " + $LASTEXITCODE | Should Not Be 0 + + $allkeys = ssh-add -L + $pubkeyraw = ((Get-Content "$keyFilePath.pub").Split(' '))[1] + ($allkeys | where { $_.contains($pubkeyraw) }).count | Should Be 0 + } } Context "$tC - ssh-keyscan test cases" { - BeforeAll {$tI=1} + BeforeAll { + $tI=1 + Remove-item (join-path $testDir "$tC.$tI.out.txt") -force -ErrorAction Ignore + } + BeforeEach { + $outputFile = join-path $testDir "$tC.$tI.out.txt" + } AfterAll{$tC++} It "$tC.$tI - ssh-keyscan with default arguments" { - iex "ssh-keyscan -p 22 github.com 2>&1 > out.txt" - 'out.txt' | Should Contain 'github.com ssh-rsa.*' + cmd /c "ssh-keyscan -p 22 github.com 2>&1 > $outputFile" + $outputFile | Should Contain 'github.com ssh-rsa.*' } It "$tC.$tI - ssh-keyscan with -p" { - iex "ssh-keyscan -p 22 github.com 2>&1 > out.txt" - 'out.txt' | Should Contain 'github.com ssh-rsa.*' + cmd /c "ssh-keyscan -p 22 github.com 2>&1 > $outputFile" + $outputFile | Should Contain 'github.com ssh-rsa.*' } It "$tC.$tI - ssh-keyscan with -f" { Set-Content -Path tmp.txt -Value "github.com" - iex "ssh-keyscan -f tmp.txt 2>&1 > out.txt" - 'out.txt' | Should Contain 'github.com ssh-rsa.*' + cmd /c "ssh-keyscan -f tmp.txt 2>&1 > $outputFile" + $outputFile | Should Contain 'github.com ssh-rsa.*' } It "$tC.$tI - ssh-keyscan with -f -t" { Set-Content -Path tmp.txt -Value "github.com" - iex "ssh-keyscan -f tmp.txt -t rsa,dsa 2>&1 > out.txt" - 'out.txt' | Should Contain 'github.com ssh-rsa.*' + cmd /c "ssh-keyscan -f tmp.txt -t rsa,dsa 2>&1 > $outputFile" + $outputFile | Should Contain 'github.com ssh-rsa.*' } } } diff --git a/regress/pesterTests/Log_fileperm.Tests.ps1 b/regress/pesterTests/Log_fileperm.Tests.ps1 new file mode 100644 index 000000000..0dbe490ad --- /dev/null +++ b/regress/pesterTests/Log_fileperm.Tests.ps1 @@ -0,0 +1,86 @@ +$tC = 1 +$tI = 0 +$suite = "log_fileperm" + +Describe "Tests for log file permission" -Tags "CI" { + BeforeAll { + if($OpenSSHTestInfo -eq $null) + { + Throw "`$OpenSSHTestInfo is null. Please run Setup-OpenSSHTestEnvironment to setup test environment." + } + + $testDir = "$($OpenSSHTestInfo["TestDataPath"])\$suite" + if( -not (Test-path $testDir -PathType Container)) + { + $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue + } + $port = 47003 + $logName = "log.txt" + + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminsAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) + + Remove-Item (Join-Path $testDir "*$logName") -Force -ErrorAction Ignore + + #only validate owner and ACEs of the file + function ValiLogFilePerm { + param([string]$FilePath) + + $myACL = Get-ACL $FilePath + $myACL.Owner.Equals($currentUser.Value) | Should Be $true + $myACL.Access | Should Not Be $null + $myACL.Access.Count | Should Be 3 + $identities = @($systemAccount.Value, $adminsAccount.Value, $currentUser.Value) + + foreach ($a in $myACL.Access) { + $a.IdentityReference.Value -in $identities | Should Be $true + + switch ($a.IdentityReference.Value) + { + {$_ -in @($systemAccount.Value, $adminsAccount.Value)} + { + $a.FileSystemRights | Should Be "FullControl" + break; + } + + $currentUser.Value + { + $a.FileSystemRights | Should Be "Write, Read, Synchronize" + break; + } + } + + $a.AccessControlType | Should Be ([System.Security.AccessControl.AccessControlType]::Allow) + $a.IsInherited | Should Be $false + $a.InheritanceFlags | Should Be ([System.Security.AccessControl.InheritanceFlags]::None) + $a.PropagationFlags | Should Be ([System.Security.AccessControl.PropagationFlags]::None) + } + } + } + + BeforeEach { + $logPath = Join-Path $testDir "$tC.$tI.$logName" + } + + AfterEach {$tI++;} + + Context "$tC-SSHD -E Log file permission" { + BeforeAll { + Get-Process -Name sshd | Where-Object {$_.SI -ne 0} | Stop-process + $tI=1 + } + + AfterAll { + $tC++ + } + + It "$tC.$tI-SSHD -E Log file permission" { + #Run + Start-Process -FilePath sshd.exe -WorkingDirectory $($OpenSSHTestInfo['OpenSSHBinPath']) -ArgumentList @("-d", "-p $port", "-E $logPath") -NoNewWindow + Start-sleep 1; + ValiLogFilePerm -FilePath $logPath + Get-Process -Name sshd | % { if($_.SI -ne 0) { Stop-Process $_; Start-sleep 1 } } + } + } +} \ No newline at end of file diff --git a/regress/pesterTests/SFTP.Tests.ps1 b/regress/pesterTests/SFTP.Tests.ps1 index a12c53239..3d17df3e6 100644 --- a/regress/pesterTests/SFTP.Tests.ps1 +++ b/regress/pesterTests/SFTP.Tests.ps1 @@ -1,4 +1,5 @@ -Describe "SFTP Test Cases" -Tags "CI" { +Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking +Describe "SFTP Test Cases" -Tags "CI" { BeforeAll { if($OpenSSHTestInfo -eq $null) { @@ -9,9 +10,6 @@ $outputFileName = "output.txt" $batchFileName = "sftp-batchcmds.txt" - $outputFilePath = Join-Path $rootDirectory $outputFileName - $batchFilePath = Join-Path $rootDirectory $batchFileName - $tempFileName = "tempFile.txt" $tempFilePath = Join-Path $rootDirectory $tempFileName @@ -23,8 +21,6 @@ $null = New-Item $clientDirectory -ItemType directory -Force $null = New-Item $serverDirectory -ItemType directory -Force - $null = New-Item $batchFilePath -ItemType file -Force - $null = New-Item $outputFilePath -ItemType file -Force $null = New-Item $tempFilePath -ItemType file -Force -value "temp file data" $null = New-Item $tempUnicodeFilePath -ItemType file -Force -value "temp file data" @@ -33,6 +29,9 @@ $ssouser = $OpenSSHTestInfo["SSOUser"] $script:testId = 1 + Remove-item (Join-Path $rootDirectory "*.$outputFileName") -Force -ErrorAction Ignore + Remove-item (Join-Path $rootDirectory "*.$batchFileName") -Force -ErrorAction Ignore + $testData1 = @( @{ title = "put, ls for non-unicode file names" @@ -175,8 +174,6 @@ Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd.log" "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd_$script:testId.log" -Force Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sftp-server.log" "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sftp-server_$script:testId.log" -Force - $script:testId++ - # clear the ssh-agent, sshd logs so that next testcase will get fresh logs. Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" -Force -ErrorAction ignore Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd.log" -Force -ErrorAction ignore @@ -186,22 +183,21 @@ } AfterAll { - if(!$OpenSSHTestInfo["DebugMode"]) - { - Get-Item $rootDirectory | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue - } + Get-ChildItem $serverDirectory | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue + Get-ChildItem $clientDirectory | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue } BeforeEach { Get-ChildItem $serverDirectory | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue - Get-ChildItem $clientDirectory | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue - Remove-Item $batchFilePath - Remove-Item $outputFilePath + Get-ChildItem $clientDirectory | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue + $outputFilePath = Join-Path $rootDirectory "$($script:testId).$outputFileName" + $batchFilePath = Join-Path $rootDirectory "$($script:testId).$batchFileName" } AfterEach { CopyDebugLogs - } + $script:testId++ + } It '' -TestCases:$testData1 { param([string]$Title, $Options, $Commands, $ExpectedOutput) @@ -265,4 +261,32 @@ iex $str Test-Path $tmpDirectoryPath2 | Should be $false } + + It "$script:testId-ls lists items the user has no read permission" { + $permTestHasAccessFile = "permTestHasAccessFile.txt" + $permTestHasAccessFilePath = Join-Path $serverDirectory $permTestHasAccessFile + Remove-Item $permTestHasAccessFilePath -Force -ErrorAction Ignore + New-Item $permTestHasAccessFilePath -ItemType file -Force -value "perm test has access file data" | Out-Null + + $permTestNoAccessFile = "permTestNoAccessFile.txt" + $permTestNoAccessFilePath = Join-Path $serverDirectory $permTestNoAccessFile + Remove-Item $permTestNoAccessFilePath -Force -ErrorAction Ignore + New-Item $permTestNoAccessFilePath -ItemType file -Force -value "perm test no access file data" | Out-Null + Set-FileOwnerAndACL -Filepath $permTestNoAccessFilePath -OwnerPerms "Read","Write" + + $Commands = "ls $serverDirectory" + Set-Content $batchFilePath -Encoding UTF8 -value $Commands + $str = $ExecutionContext.InvokeCommand.ExpandString("sftp -b $batchFilePath test_target > $outputFilePath") + iex $str + $content = Get-Content $outputFilePath + + #cleanup + $HasAccessPattern = $permTestHasAccessFilePath.Replace("\", "[/\\]") + $matches = $content | select-string -Pattern "^/$HasAccessPattern\s{0,}$" + $matches.count | Should be 1 + + $NoAccessPattern = $permTestNoAccessFilePath.Replace("\", "[/\\]") + $matches = $content | select-string -Pattern "^/$NoAccessPattern\s{0,}$" + $matches.count | Should be 1 + } } diff --git a/regress/pesterTests/Userkey_fileperm.Tests.ps1 b/regress/pesterTests/Userkey_fileperm.Tests.ps1 index ee45e2043..6c2de181c 100644 --- a/regress/pesterTests/Userkey_fileperm.Tests.ps1 +++ b/regress/pesterTests/Userkey_fileperm.Tests.ps1 @@ -1,185 +1,139 @@ -Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -Describe "Tests for user Key file permission" -Tags "Scenario" { +Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking + +$tC = 1 +$tI = 0 +$suite = "userkey_fileperm" + +Describe "Tests for user Key file permission" -Tags "CI" { BeforeAll { if($OpenSSHTestInfo -eq $null) { Throw "`$OpenSSHTestInfo is null. Please run Setup-OpenSSHTestEnvironment to setup test environment." } - $testDir = "$($OpenSSHTestInfo["TestDataPath"])\usertkey_fileperm" + $testDir = "$($OpenSSHTestInfo["TestDataPath"])\$suite" if( -not (Test-path $testDir -PathType Container)) { $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue - } - $fileName = "test.txt" - $filePath = Join-Path $testDir $fileName + } + + $logName = "log.txt" $port = $OpenSSHTestInfo["Port"] $ssouser = $OpenSSHTestInfo["SSOUser"] $pubKeyUser = $OpenSSHTestInfo["PubKeyUser"] $pubKeyUserProfile = $OpenSSHTestInfo["PubKeyUserProfile"] $server = $OpenSSHTestInfo["Target"] - $keyFileName = "sshtest_userPermTestkey_ed25519" - $keyFilePath = Join-Path $testDir $keyFileName - Remove-Item -path "$keyFilePath*" -Force -ErrorAction Ignore - ssh-keygen.exe -t ed25519 -f $keyFilePath -P `"`" $userName = "$env:USERNAME@$env:USERDOMAIN" - if(Test-Path $keyFilePath) { - Set-SecureFileACL -filepath $keyFilePath - } - #add wrong password so ssh does not prompt password if failed with authorized keys - Add-PasswordSetting -Pass "WrongPass" + $keypassphrase = "testpassword" + + $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM") + $adminsAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators") + $objUser = New-Object System.Security.Principal.NTAccount($ssouser) + $pubKeyUserAccount = New-Object System.Security.Principal.NTAccount($pubKeyUser) + $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) + $everyone = New-Object System.Security.Principal.NTAccount("EveryOne") + + Add-PasswordSetting -Pass $keypassphrase } AfterAll { Remove-PasswordSetting } + BeforeEach { + $logPath = Join-Path $testDir "$tC.$tI.$logName" + } - <# comment the test out since ssh-add have impact on - existing default test environment. - Context "ssh-add key files" { - BeforeEach { - ssh-add -D 2>&1 > $fileName - } + AfterEach {$tI++;} - AfterEach { - ssh-add -D 2>&1 > $fileName - if(Test-Path $keyFilePath) { - Set-SecureFileACL -filepath $keyFilePath - } - } + Context "$tC-ssh with private key file" { + BeforeAll { + $keyFileName = "sshtest_userPermTestkey_ed25519" + $keyFilePath = Join-Path $testDir $keyFileName + Remove-Item -path "$keyFilePath*" -Force -ErrorAction Ignore + ssh-keygen.exe -t ed25519 -f $keyFilePath -P $keypassphrase - It 'ssh-add positive (Secured private key owned by current user)' { - #setup to have current user as owner and grant it full control - Set-SecureFileACL -filepath $keyFilePath - ssh-add $keyFilePath 2>&1 > $fileName - $LASTEXITCODE | Should Be 0 - $o = ssh-add -l - $matches = $o | Select-String -pattern "no identities" - $matches.Count | Should Be 0 - - $matches = $o | Select-String -pattern $userName - $matches.Count | Should Be 1 - } - - It 'ssh-add positive (Secured private key owned by Administrators group)' { - #setup to have local admin group as owner and grant it full control - $objAdmin = New-Object System.Security.Principal.NTAccount("BUILTIN", "Administrators") - Set-SecureFileACL -filepath $keyFilePath -Owner $objAdmin - ssh-add $keyFilePath 2>&1 > $fileName - $LASTEXITCODE | Should Be 0 - $o = ssh-add -l - $matches = $o | Select-String -pattern "no identities" - $matches.Count | Should Be 0 - - $matches = $o | Select-String -pattern $userName - $matches.Count | Should Be 1 - } - - It 'ssh-add -- negative (other account can access private key file)' { - #setup to have current user as owner and grant it full control - Set-SecureFileACL -filepath $keyFilePath - - #add ssouser to access the private key - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - Add-PermissionToFileACL -FilePath $keyFilePath -User $objUser -Perm "Read" - - ssh-add $keyFilePath 2>&1 > $fileName - $LASTEXITCODE | Should Not Be 0 - $o = ssh-add -l - $matches = $o | Select-String -pattern "no identities" - $matches.Count | Should Be 1 - } - - It 'ssh-add -- negative (the private key has wrong owner)' { - #setup to have ssouser as owner and grant it full control - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - Set-SecureFileACL -filepath $keyFilePath -owner $objUser - $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) - Add-PermissionToFileACL -FilePath $keyFilePath -User $currentUser -Perm "FullControl" - - ssh-add $keyFilePath 2>&1 > $fileName - $LASTEXITCODE | Should Not Be 0 - $o = ssh-add -l - $matches = $o | Select-String -pattern "no identities" - $matches.Count | Should Be 1 - } - }#> - - Context "ssh with private key file" { - BeforeAll { $pubKeyUserProfilePath = Join-Path $pubKeyUserProfile .ssh if(-not (Test-Path $pubKeyUserProfilePath -PathType Container)) { - New-Item $pubKeyUserProfilePath -ItemType directory -Force -ErrorAction Stop | Out-Null + New-Item $pubKeyUserProfilePath -ItemType Directory -Force -ErrorAction Stop | Out-Null } - Set-SecureFileACL -filepath $keyFilePath + $testAuthorizedKeyPath = Join-Path $pubKeyUserProfilePath authorized_keys Copy-Item "$keyFilePath.pub" $testAuthorizedKeyPath -Force -ErrorAction SilentlyContinue + Adjust-UserKeyFileACL -FilePath $testAuthorizedKeyPath -Owner $pubKeyUserAccount -OwnerPerms "Read, Write" Add-PermissionToFileACL -FilePath $testAuthorizedKeyPath -User "NT Service\sshd" -Perm "Read" - Remove-Item -Path $filePath -Force -ErrorAction ignore + $tI=1 } AfterAll { - if(Test-Path $testAuthorizedKeyPath) { - Set-SecureFileACL -filepath $testAuthorizedKeyPath + if(Test-Path $testAuthorizedKeyPath) { Remove-Item $testAuthorizedKeyPath -Force -ErrorAction Ignore } if(Test-Path $pubKeyUserProfilePath) { - Remove-Item $pubKeyUserProfilePath -Force -ErrorAction Ignore + Remove-Item $pubKeyUserProfilePath -Recurse -Force -ErrorAction Ignore } - } + $tC++ + } - AfterEach { - if(Test-Path $keyFilePath) { - Set-SecureFileACL -filepath $keyFilePath - } - - Remove-Item -Path $filePath -Force -ErrorAction ignore - } - - It 'ssh with private key file -- positive (Secured private key owned by current user)' { - Set-SecureFileACL -filepath $keyFilePath + It "$tC.$tI-ssh with private key file -- positive (Secured private key owned by current user)" { + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $currentUser -OwnerPerms "Read, Write" #Run $o = ssh -p $port -i $keyFilePath $pubKeyUser@$server echo 1234 $o | Should Be "1234" } - It 'ssh with private key file -- positive (Secured private key owned by Administrators group)' { + It "$tC.$tI-ssh with private key file -- positive(Secured private key owned by Administrators group)" { #setup to have local admin group as owner and grant it full control - $objAdmin = New-Object System.Security.Principal.NTAccount("BUILTIN", "Administrators") - Set-SecureFileACL -filepath $keyFilePath -Owner $objAdmin + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $adminsAccount -OwnerPerms "FullControl" + + #Run + $o = ssh -p $port -i $keyFilePath $pubKeyUser@$server echo 1234 + $o | Should Be "1234" + } + + It "$tC.$tI-ssh with private key file -- positive (Secured private key owned by local system)" { + #setup to have local system as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $systemAccount -OwnerPerms "FullControl" + Add-PermissionToFileACL -FilePath $keyFilePath -User $adminsAccount -Perm "Read" #Run $o = ssh -p $port -i $keyFilePath $pubKeyUser@$server echo 1234 $o | Should Be "1234" } - It 'ssh with private key file -- negative (other account can access private key file)' { + It "$tC.$tI-ssh with private key file -- negative(other account can access private key file)" { #setup to have current user as owner and grant it full control - Set-SecureFileACL -filepath $keyFilePath + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $currentUser -OwnerPerms "Read, Write" - #add ssouser to access the private key - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) + #add ssouser to access the private key Add-PermissionToFileACL -FilePath $keyFilePath -User $objUser -Perm "Read" #Run - $o = ssh -p $port -i $keyFilePath -E $filePath $pubKeyUser@$server echo 1234 + $o = ssh -p $port -i $keyFilePath -E $logPath $pubKeyUser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 - $matches = Get-Content $filePath | Select-String -pattern "UNPROTECTED PRIVATE KEY FILE!" - $matches.Count | Should Be 1 + $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!" } - It 'ssh with private key file -- (the private key has wrong owner)' { - #setup to have ssouser as owner and grant it full control - $objUser = New-Object System.Security.Principal.NTAccount($ssouser) - Set-SecureFileACL -filepath $keyFilePath -owner $objUser + It "$tC.$tI-ssh with private key file -- negative(the private key has wrong owner)" { + #setup to have ssouser as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $objUser -OwnerPerms "Read, Write" + Add-PermissionToFileACL -FilePath $keyFilePath -User $adminsAccount -Perm "FullControl" - $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME)) - Add-PermissionToFileACL -FilePath $keyFilePath -User $currentUser -Perm "FullControl" - - $o = ssh -p $port -i $keyFilePath -E $filePath $pubKeyUser@$server echo 1234 + $o = ssh -p $port -i $keyFilePath -E $logPath $pubKeyUser@$server echo 1234 $LASTEXITCODE | Should Not Be 0 - $matches = Get-Content $filePath | Select-String -pattern "UNPROTECTED PRIVATE KEY FILE!" - $matches.Count | Should Be 1 + $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!" + } + + It "$tC.$tI-ssh with private key file -- negative(the owner is denied read perm)" { + #setup to have local system as owner and grant it full control + Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $systemAccount -OwnerPerms "Read, Write" + Add-PermissionToFileACL -FilePath $keyFilePath -User $adminsAccount -Perm "FullControl" + #deny local system read access + Add-PermissionToFileACL -FilePath $keyFilePath -User $systemAccount -Perm "Read,write" -AccessType Deny + + $o = ssh -p $port -i $keyFilePath -E $logPath $pubKeyUser@$server echo 1234 + $LASTEXITCODE | Should Not Be 0 + + $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!" } } } diff --git a/regress/unittests/win32compat/dir_tests.c b/regress/unittests/win32compat/dir_tests.c index bdfbe8e07..1768c6887 100644 --- a/regress/unittests/win32compat/dir_tests.c +++ b/regress/unittests/win32compat/dir_tests.c @@ -122,7 +122,7 @@ dir_tests_1() retValue = chdir(tes_dirname_2); ASSERT_INT_EQ(retValue, 0); - f = open(tmpfile, O_RDWR | O_CREAT | O_TRUNC); + f = open(tmpfile, O_RDWR | O_CREAT | O_TRUNC, 0600); ASSERT_INT_NE(f, -1); close(f); diff --git a/regress/unittests/win32compat/file_tests.c b/regress/unittests/win32compat/file_tests.c index 4d66f090b..ae04883aa 100644 --- a/regress/unittests/win32compat/file_tests.c +++ b/regress/unittests/win32compat/file_tests.c @@ -81,15 +81,18 @@ file_blocking_io_tests() void file_simple_fileio() { - TEST_START("file io"); + TEST_START("file io and fstat"); char* small_write_buf = "sample payload"; char small_read_buf[SMALL_RECV_BUF_SIZE]; int f; struct stat st; - { f = open(tmp_filename, O_WRONLY | O_CREAT | O_TRUNC); + ASSERT_INT_EQ(f, -1); + } + { + f = open(tmp_filename, O_WRONLY | O_CREAT | O_TRUNC, 0600); ASSERT_INT_NE(f, -1); close(f); } @@ -100,13 +103,14 @@ void file_simple_fileio() retValue = fstat(f, &st); ASSERT_INT_EQ(retValue, 0); ASSERT_INT_EQ(st.st_size, 0); + ASSERT_INT_EQ(st.st_mode & 0777, 0666); retValue = read(f, small_read_buf, SMALL_RECV_BUF_SIZE); ASSERT_INT_EQ(retValue, 0); close(f); } { - f = open(tmp_filename, O_WRONLY | O_CREAT | O_TRUNC); + f = open(tmp_filename, O_WRONLY | O_TRUNC); ASSERT_INT_NE(f, -1); retValue = write(f, small_write_buf, strlen(small_write_buf)); ASSERT_INT_EQ(retValue, strlen(small_write_buf)); @@ -119,7 +123,7 @@ void file_simple_fileio() retValue = stat(tmp_filename, &st); ASSERT_INT_EQ(retValue, 0); ASSERT_INT_EQ(st.st_size, strlen(small_write_buf)); - + ASSERT_INT_EQ(st.st_mode & 0777, 0666); char mode[12]; strmode(st.st_mode, mode); ASSERT_CHAR_EQ(mode[0], '-'); @@ -162,7 +166,7 @@ void file_simple_fileio() { /* test writev, ftruncate, isatty, lseek, fdopen */ - f = open(tmp_filename, O_RDWR | O_CREAT | O_TRUNC); + f = open(tmp_filename, O_RDWR | O_TRUNC); ASSERT_INT_NE(f, -1); struct iovec iov; iov.iov_base = small_write_buf; @@ -205,7 +209,7 @@ void file_simple_fileio() void file_simple_fileio_mode() { - TEST_START("file mode"); + TEST_START("file io and mode"); char * small_write_buf = "sample payload", *c, small_read_buf[SMALL_RECV_BUF_SIZE]; int ret; @@ -420,7 +424,7 @@ file_miscellaneous_tests() ASSERT_INT_NE(f, -1); close(f); - f = open(tmp_filename, O_RDWR | O_CREAT | O_TRUNC); + f = open(tmp_filename, O_RDWR | O_CREAT | O_TRUNC, 0600); ASSERT_INT_NE(f, -1); int f1 = dup(f); ASSERT_INT_EQ(f1, -1); diff --git a/ssh-keygen.c b/ssh-keygen.c index 75be7b964..74fa63702 100644 --- a/ssh-keygen.c +++ b/ssh-keygen.c @@ -1043,21 +1043,6 @@ do_gen_all_hostkeys(struct passwd *pw) } sshkey_free(private); strlcat(identity_file, ".pub", sizeof(identity_file)); -#ifdef WINDOWS - /* Windows POSIX adpater does not support fdopen() on open(file)*/ - if ((f = fopen(identity_file, "w")) == NULL) { - error("fopen %s failed: %s", identity_file, strerror(errno)); - sshkey_free(public); - first = 0; - continue; - } - /* - Set the owner of the private key file to the user represented by pw - and only grant it the full control access - */ - if (set_secure_file_permission(identity_file, pw) !=0) { - error("set_secure_file_permission on %s failed!", identity_file); -#else /* !WINDOWS */ fd = open(identity_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); if (fd == -1) { error("Could not save your public key in %s", @@ -1066,6 +1051,12 @@ do_gen_all_hostkeys(struct passwd *pw) first = 0; continue; } +#ifdef WINDOWS + /* Windows POSIX adpater does not support fdopen() on open(file)*/ + close(fd); + if ((f = fopen(identity_file, "w")) == NULL) { + error("fopen %s failed: %s", identity_file, strerror(errno)); +#else /* !WINDOWS */ f = fdopen(fd, "w"); if (f == NULL) { error("fdopen %s failed", identity_file); @@ -1506,14 +1497,6 @@ do_change_comment(struct passwd *pw) strlcat(identity_file, ".pub", sizeof(identity_file)); fd = open(identity_file, O_WRONLY | O_CREAT | O_TRUNC, 0644); -#ifdef WINDOWS - /* - Set the owner of the private key file to user represented by pw and only grant - it the full control access - */ - if (set_secure_file_permission(identity_file, pw) != 0) - fatal("set_secure_file_permission on %s failed!", identity_file); -#endif /* WINDOWS*/ if (fd == -1) fatal("Could not save your public key in %s", identity_file); f = fdopen(fd, "w"); @@ -1709,16 +1692,7 @@ do_ca_sign(struct passwd *pw, int argc, char **argv) if ((fd = open(out, O_WRONLY|O_CREAT|O_TRUNC, 0644)) == -1) fatal("Could not open \"%s\" for writing: %s", out, - strerror(errno)); -#ifdef WINDOWS - /* - Set the owner of the private key file to user represented by pw and only grant - it the full control access - */ - if (set_secure_file_permission(out, pw) != 0) - fatal("set_secure_file_permission on %s failed!", identity_file); -#endif /* WINDOWS */ - + strerror(errno)); if ((f = fdopen(fd, "w")) == NULL) fatal("%s: fdopen: %s", __func__, strerror(errno)); if ((r = sshkey_write(public, f)) != 0) @@ -2241,14 +2215,6 @@ do_gen_krl(struct passwd *pw, int updating, int argc, char **argv) fatal("Couldn't generate KRL"); if ((fd = open(identity_file, O_WRONLY|O_CREAT|O_TRUNC, 0644)) == -1) fatal("open %s: %s", identity_file, strerror(errno)); -#ifdef WINDOWS - /* - Set the owner of the private key file to user represented by pw and only grant - it the full control access - */ - if (set_secure_file_permission(identity_file, pw) != 0) - fatal("set_secure_file_permission on %s failed!", identity_file); -#endif /* WINDOWS */ if (atomicio(vwrite, fd, (void *)sshbuf_ptr(kbuf), sshbuf_len(kbuf)) != sshbuf_len(kbuf)) fatal("write %s: %s", identity_file, strerror(errno)); @@ -2808,20 +2774,15 @@ passphrase_again: printf("Your identification has been saved in %s.\n", identity_file); strlcat(identity_file, ".pub", sizeof(identity_file)); -#ifdef WINDOWS - /* Windows POSIX adpater does not support fdopen() on open(file)*/ - if ((f = fopen(identity_file, "w")) == NULL) - fatal("fopen %s failed: %s", identity_file, strerror(errno)); - /* - Set the owner of the private key file to the user represented by pw and only grant - it the full control access - */ - if (set_secure_file_permission(identity_file, pw) != 0) - error("set_secure_file_permission on %s failed!", identity_file); -#else /* !WINDOWS */ if ((fd = open(identity_file, O_WRONLY|O_CREAT|O_TRUNC, 0644)) == -1) fatal("Unable to save public key to %s: %s", identity_file, strerror(errno)); +#ifdef WINDOWS + /* Windows POSIX adpater does not support fdopen() on open(file)*/ + close(fd); + if ((f = fopen(identity_file, "w")) == NULL) + fatal("fopen %s failed: %s", identity_file, strerror(errno)); +#else /* !WINDOWS */ if ((f = fdopen(fd, "w")) == NULL) fatal("fdopen %s failed: %s", identity_file, strerror(errno)); #endif /* !WINDOWS */ diff --git a/sshfileperm.h b/sshfileperm.h index 5c5c3ab27..09005ce85 100644 --- a/sshfileperm.h +++ b/sshfileperm.h @@ -26,5 +26,4 @@ #define _SSH_FILE_PERM_H int check_secure_file_permission(const char *, struct passwd *); -int set_secure_file_permission(const char *, struct passwd *); #endif /* _SSH_FILE_PERM_H */ \ No newline at end of file