Add mode support in open() and onboard file perm related changes and tests (#145)

PowerShell/Win32-OpenSSH#710
PowerShell/Win32-OpenSSH#725
PowerShell/Win32-OpenSSH#729
PowerShell/Win32-OpenSSH#731
PowerShell/Win32-OpenSSH#732

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;

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
     }
 
 

contrib/win32/openssh/OpenSSHCommonUtils.psm1

@@ -30,48 +30,147 @@ function Get-RepositoryRoot
 
 <#
 .Synopsis
-    Sets the Secure File ACL. 
+    Set owner of the file to by LOCALSYSTEM account
-    1. Removed all user acl except Administrators group, system, and current user
+    Set private host key be fully controlled by LOCALSYSTEM and Administrators
-    2. whether or not take the owner
+    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 (
-    param([string]$FilePath, [System.Security.Principal.NTAccount] $Owner)
+        [parameter(Mandatory=$true)]
+        [string]$FilePath
+    )
 
-    $myACL = Get-ACL $filePath
+    $myACL = Get-ACL $FilePath
-    $myACL.SetAccessRuleProtection($True, $True)
+    $myACL.SetAccessRuleProtection($True, $FALSE)
-    Set-Acl -Path $filePath -AclObject $myACL
+    Set-Acl -Path $FilePath -AclObject $myACL
 
-    $myACL = Get-ACL $filePath
+    $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM")
-    if($owner -ne $null)
+    $adminAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators")
-    {        
+    $everyoneAccount = New-Object System.Security.Principal.NTAccount("EveryOne")
-        $myACL.SetOwner($owner)
+    $myACL = Get-ACL $FilePath
-    }
+
+    $myACL.SetOwner($systemAccount)
 
     if($myACL.Access) 
     {        
         $myACL.Access | % {
-            if (($_ -ne $null) -and ($_.IdentityReference.Value -ine "BUILTIN\Administrators") -and 
+            if(-not ($myACL.RemoveAccessRule($_)))
-            ($_.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 "
-                {
-                    throw "failed to remove access of $($_.IdentityReference.Value) rule in setup "
-                }
             }
         }
     }    
 
-    Set-Acl -Path $filePath -AclObject $myACL
+    $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)
+        }
+    }
+
+    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 `
+    if($Perms)
-        ($User, $perm, "None", "None", "Allow") 
+    {
-    $myACL.AddAccessRule($objACE)    
+        $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 
+Export-ModuleMember -Function Get-RepositoryRoot, Add-PermissionToFileACL, Adjust-HostKeyFileACL, Adjust-UserKeyFileACL

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*"| % {
-    Get-ChildItem "$($script:OpenSSHBinPath)\sshtest*hostkey*" -Exclude *.pub | % {
+        #workaround for the cariggage new line added by git before copy them
-        Cleanup-SecureFileACL -FilePath $_.FullName -Owner $owner
+        (Get-Content $_.FullName -Raw).Replace("`r`n","`n") | Set-Content $_.FullName -Force
-        Add-PermissionToFileACL -FilePath $_.FullName -User "NT Service\sshd" -Perm "Read"
+        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
 }

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 <Sddl.h>
 
 /* 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 rwflags = flags & 0x3, c_s_flags = flags & 0xfffffff0, ret = -1;
-	int c_s_flags = flags & 0xfffffff0;
+	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;
@@ -288,11 +342,6 @@ createFile_flags_setup(int flags, int mode, struct createFile_flags* cf_flags)
 		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,8 +433,10 @@ fileio_open(const char *path_utf8, int flags, int mode)
 		return NULL;
 	}
 
-	if (createFile_flags_setup(flags, mode, &cf_flags) == -1)
+	if (createFile_flags_setup(flags, mode, &cf_flags) == -1) {
-		return NULL;
+		debug3("createFile_flags_setup() failed.");
+		goto cleanup;
+	}	
 
 	handle = CreateFileW(path_utf16, cf_flags.dwDesiredAccess, cf_flags.dwShareMode,
 		&cf_flags.securityAttributes, cf_flags.dwCreationDisposition,
@@ -352,17 +445,16 @@ fileio_open(const char *path_utf8, int flags, int mode)
 	if (handle == INVALID_HANDLE_VALUE) {
 		errno = errno_from_Win32LastError();
 		debug3("failed to open file:%s error:%d", path_utf8, GetLastError());
-		free(path_utf16);
+		goto cleanup;
-		return NULL;
 	}
 
-	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;
 }
 

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
+#define open(a,b,...) w32_open((a), (b),  __VA_ARGS__)
-int w32_open(const char *pathname, int flags, ...);
+int w32_open(const char *pathname, int flags, ... /* arg */);
 
 void* w32_fd_to_handle(int fd);
 int w32_allocate_fd_for_handle(HANDLE, BOOL);

contrib/win32/win32compat/w32-sshfileperm.c

@@ -31,33 +31,28 @@
 #include <Windows.h>
 #include <Sddl.h>
 #include <Aclapi.h>
-#include <Ntsecapi.h>
 #include <lm.h>
 #include <stdio.h> 
 
 #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,
+* 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 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,
+	1. no user other than local administrators group, system account, and owner accounts have write permission on the file
-	   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) &&
+		!EqualSid(owner_sid, user_sid)) {
-		!is_admin_account(owner_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*/
+		/*no need to check administrators group, owner account, and system account*/
-		/*if (IsWellKnownSid(current_trustee_sid, WinBuiltinAdministratorsSid) ||
+		if (IsWellKnownSid(current_trustee_sid, WinBuiltinAdministratorsSid) ||
 			IsWellKnownSid(current_trustee_sid, WinLocalSystemSid) ||
-			EqualSid(current_trustee_sid, owner_sid) ||
+			EqualSid(current_trustee_sid, owner_sid)) {
-			EqualSid(current_trustee_sid, user_sid) ||
-			is_admin_account(current_trustee_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;*/
-}

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, mode);
 	
-	pio = fileio_open(sanitized_path(pathname), flags, 0);
 	if (pio == NULL)
 		return -1;
 

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);

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
+		file permissions are designed differently on windows.
-		implementation on windows to make sure the config file is owned by the user and
+		implementation on windows to make sure the config file is owned by a user, administrators group, or LOCALSYSTEM account
-		nobody else except Administrators group and  current user of calling process, and SYSTEM account has the write permission
+		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);

regress/pesterTests/Authorized_keys_fileperm.Tests.ps1

@@ -1,52 +1,39 @@
-Import-Module $PSScriptRoot\CommonUtils.psm1 -Force
+Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking
-Describe "Tests for authorized_keys file permission" -Tags "Scenario" {
+$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 = "sshdlog.txt"
-        $logName = "log.txt"
-        $logPath = Join-Path $testDir $logName
         $server = $OpenSSHTestInfo["Target"]
         $port = 47003
         $ssouser = $OpenSSHTestInfo["SSOUser"]
         $PwdUser = $OpenSSHTestInfo["PasswdUser"]
         $ssouserProfile = $OpenSSHTestInfo["SSOUserProfile"]
-        $script:logNum = 0
+        Remove-Item -Path (Join-Path $testDir "*$fileName") -Force -ErrorAction ignore
-
-        # 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
     }
 
-    AfterEach {        
+    AfterEach { $tI++ }
-        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            
-        }
-    }
 
     Context "Authorized key file permission" {
         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
 
-            Remove-Item $filePath -Force -ErrorAction Ignore
+            Adjust-UserKeyFileACL -Filepath $authorizedkeyPath -Owner $objUser -OwnerPerms "Read, Write"
+
             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 {
+        BeforeEach {
-            Remove-Item -Path $filePath -Force -ErrorAction ignore            
+            $filePath = Join-Path $testDir "$tC.$tI.$fileName"            
+            $logPath = Join-Path $testDir "$tC.$tI.$logName"
         }       
 
-        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(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 "$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
+            ssh -p $port -E $filePath -o "UserKnownHostsFile $testknownhosts" $ssouser@$server echo 1234
-            $o | Should Be "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)' {
+        It "$tC.$tI-authorized_keys-negative(other account can access private key 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)' {
             #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 = Get-Content $filePath | Select-String -pattern "^Permission denied"
-            $matches.Count | Should Not Be 0
+            $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)' {
+        It "$tC.$tI-authorized_keys-negative(authorized_keys is owned by other non-admin user)" {
-            #setup to have ssouser as owner and grant it full control
+            #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
+            Set-FileOwnerAndACL -Filepath $authorizedkeyPath -owner $objPwdUser -OwnerPerms "Read","Write"
-
+            Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $systemAccount -Perms "FullControl"
-            #add current user full access the file authorized_keys
+            Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $adminAccount -Perms "FullControl"
-            $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME))
-            Add-PermissionToFileACL -FilePath $authorizedkeyPath -User $currentUser -Perm "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 = Get-Content $filePath | Select-String -pattern "^Permission denied"
-            $matches.Count | Should Not Be 0
+            $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)' {
+        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            
-            $objUser = New-Object System.Security.Principal.NTAccount($ssouser)
+            Set-FileOwnerAndACL -Filepath $authorizedkeyPath -Owner $objUser -OwnerPerms "Read","Write"
-            Set-SecureFileACL -filepath $authorizedkeyPath -Owner $objUser
+            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 = Get-Content $filePath | Select-String -pattern "^Permission denied"
-            $matches.Count | Should Not Be 0
+            $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 } } 

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"
+        $testDir = "$($OpenSSHTestInfo["TestDataPath"])\$suite"
-        $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue
+        if( -not (Test-path $testDir -PathType Container))
-        $fileName = "test.txt"
+        {
-        $filePath = Join-Path $testDir $fileName
+            $null = New-Item $testDir -ItemType directory -Force -ErrorAction SilentlyContinue
-        $logName = "log.txt"
+        }
-        $logPath = Join-Path $testDir $logName
+        $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 
+        Remove-Item -Path (Join-Path $testDir "*logName") -Force -ErrorAction ignore
-        {
-            [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
     }
 
     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\ssh-agent.log" "$testDir\agentlog$tC.$tI.log" -Force -ErrorAction ignore
-            Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd.log" "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\failedsshd$script:logNum.log" -Force -ErrorAction ignore
+            Copy-Item "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\sshd.log" "$testDir\sshdlog$tC.$tI.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, 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
+            $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"
+            $o = ssh test_target echo 1234
-            cmd /c $str
+            $o | Should Be "1234"
-            $LASTEXITCODE | Should Be 0
-
-            #validate file content.
-            Get-Content $filePath | Should be $env:COMPUTERNAME 
-            
-            #clean up
-            Set-Acl -Path $userConfigFile -AclObject $oldACL
         }
-        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"
+            $o = ssh test_target echo 1234
-            cmd /c $str
+            $o | Should Be "1234"
+        }
 
-            #clean up
+        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
+            $o = ssh test_target echo 1234
+            $o | Should Be "1234"
+        }
+
+        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 'User SSHConfig -- ReadConfig (wrong permission)' {
+        It "$tC.$tI-User SSHConfig-ReadConfig negative (others has permission)" {
             #setup
-            $owner = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME))
+            Set-FileOwnerAndACL -Filepath $userConfigFile -Owner $currentUser -OwnerPerms "Read","Write"
-
+            Add-PermissionToFileACL -FilePath $userConfigFile -User $systemAccount -Perms "FullControl"
-            $objUser = New-Object System.Security.Principal.NTAccount($ssouser)
+            Add-PermissionToFileACL -FilePath $userConfigFile -User $adminAccount -Perms "FullControl"
-            $objACE = New-Object System.Security.AccessControl.FileSystemAccessRule `
+            Add-PermissionToFileACL -FilePath $userConfigFile -User $objUser -Perms "Read"
-                ($objUser, "Read, Write", "None", "None", "Allow")
-             Set-SecureFileACL -filepath $userConfigFile -owner $owner -Ace $objACE
 
             #Run
-            $str = "ssh -p $($port) -E $logPath $($Options) $($ssouser)@$($server) hostname > $filePath"
+            cmd /c "ssh test_target echo 1234 2> $logPath"
-            cmd /c $str
-
-            #clean up
             $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$"
         }
     }
 }

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 `
+    if($OwnerPerms)
-        ($actualOwner, "FullControl", "None", "None", "Allow")
+    {
-    $myACL.AddAccessRule($objACE)
+        $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
+    $myACL = Get-ACL $FilePath
 
-    $objACE = New-Object System.Security.AccessControl.FileSystemAccessRule `
+    if($Perms)
-        ($User, $perm, "None", "None", "Allow") 
+    {
-    $myACL.AddAccessRule($objACE)    
+        $Perms | % { 
+            $userACE = New-Object System.Security.AccessControl.FileSystemAccessRule `
+                ($User, $_, "None", "None", $AccessType)
+            $myACL.AddAccessRule($userACE)
+        }
+    }   
 
-    Set-Acl -Path $filePath -AclObject $myACL
+    Set-Acl -Path $FilePath -AclObject $myACL
 }
 
 function Add-PasswordSetting 

regress/pesterTests/Hostkey_fileperm.Tests.ps1

@@ -1,147 +1,164 @@
-Import-Module $PSScriptRoot\CommonUtils.psm1 -Force
+Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking
-Describe "Tests for host keys file permission" -Tags "Scenario" {
+$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"
+        $logName = "sshdlog.txt"
-        $filePath = Join-Path $testDir $fileName
-        $logName = "log.txt"
-        $logPath = Join-Path $testDir $logName
         $port = 47003
         $ssouser = $OpenSSHTestInfo["SSOUser"]
         $script:logNum = 0
-
+        Remove-Item -Path (Join-Path $testDir "*$logName") -Force -ErrorAction ignore
-        # 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
     }
 
-    AfterEach {        
+    AfterEach { $tI++ }
-        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.
+    Context "$tC - Host key files permission" {
-            Clear-Content "$($OpenSSHTestInfo['OpenSSHBinPath'])\logs\ssh-agent.log" -Force -ErrorAction ignore            
-        }
-        Remove-Item -Path $filePath -Force -ErrorAction ignore
-    }
-
-    Context "Host key files permission" {
         BeforeAll {
-            $hostKeyFilePath = "$($OpenSSHTestInfo['OpenSSHBinPath'])\hostkeyFilePermTest_ed25519_key"
+            $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM")
-            if(Test-path $hostKeyFilePath -PathType Leaf){
+            $adminAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators")
-                Set-SecureFileACL -filepath $hostKeyFilePath            
+            $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
             Get-Process -Name sshd | Where-Object {$_.SI -ne 0} | Stop-process
+            $tI=1
         }
 
-        AfterEach {
+        BeforeEach {
-            Remove-Item -Path $filePath -Force -ErrorAction ignore            
+            $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)' {
+        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)" {
-            #setup to have current user as owner and grant it full control        
+            Set-FileOwnerAndACL -Filepath $hostKeyFilePath -Owner $adminAccount -OwnerPerms "FullControl"
-            Set-SecureFileACL -filepath $hostKeyFilePath
+            Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $systemAccount -Perms "FullControl"
-            #grant sshd Read permission to 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.
+            #validate file content does not contain unprotected info.
-            $matches = Get-Content $filePath | Select-String -pattern "UNPROTECTED PRIVATE KEY FILE!"
+            $logPath | Should Not Contain "UNPROTECTED PRIVATE KEY FILE!"
-            $matches.Count | Should Be 0
         }
 
-        It 'Host keys -- negative (other account can access private key file)' {
+        It "$tC.$tI-Host keys-positive (both public and private keys are owned by system and running process can access to public key file)" {               
-            #setup to have current user as owner and grant it full control        
+            Set-FileOwnerAndACL -Filepath $hostKeyFilePath -Owner $systemAccount -OwnerPerms "FullControl"
-            Set-SecureFileACL -filepath $hostKeyFilePath
+            Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $adminAccount -Perms "Read"
-            #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
+            Set-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $systemAccount -OwnerPerms "FullControl"
-            Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User "NT Service\sshd" -Perm "Read"
+            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"
+            #validate file content does not contain unprotected info.
-            $matches.Count | Should Be 1
+            $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-FileOwnerAndACL -FilePath $hostKeyFilePath -Owner $objUser -OwnerPerms "Read","Write"
-            Set-SecureFileACL -filepath $hostKeyFilePath -owner $objUser
+            Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $adminAccount -Perms "FullControl"
-            $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME))
+            Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $systemAccount -Perms "FullControl"
-            Add-PermissionToFileACL -FilePath $hostKeyFilePath -User $currentUser -Perm "FullControl"
             
-            #grant sshd Read permission to the public key
+            Set-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $adminAccount -OwnerPerms "FullControl"
-            Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User "NT Service\sshd" -Perm "Read"
+            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.
+            #validate file content contains unprotected info.
-            $matches = Get-Content $filePath | Select-String -pattern "key_load_private: bad permissions"
+            $logPath | Should Contain "key_load_private: bad permissions"
-            $matches.Count | Should Be 1
         }
-        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-FileOwnerAndACL -Filepath "$hostKeyFilePath.pub" -Owner $systemAccount -OwnerPerms "FullControl"
-            Set-SecureFileACL -filepath "$hostKeyFilePath.pub" -owner $objUser
-
-            #grant current user Read permission to public key
-            Add-PermissionToFileACL -FilePath "$hostKeyFilePath.pub" -User $objUser -Perm "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 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"
+            $logPath | Should Contain "key_load_private: bad permissions"            
-            $matches.Count | Should Be 1
         }
     }
 }

regress/pesterTests/KeyUtils.Tests.ps1

@@ -1,8 +1,9 @@
-$tC = 1
+Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking
+$tC = 1
 $tI = 0
 $suite = "keyutils"
 
-Describe "E2E scenarios for ssh key management" -Tags "Scenario" {
+Describe "E2E scenarios for ssh key management" -Tags "CI" {
     BeforeAll {
         if($OpenSSHTestInfo -eq $null)
         {
@@ -17,22 +18,59 @@ 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)
         
-            $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME))
+        $ssouser = $OpenSSHTestInfo["SSOUser"]
-            $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)
+            }
 
-            $myACL.Access[0].IdentityReference.Equals($currentUser) | Should Be $true
+            foreach ($a in $myACL.Access) {
-            $myACL.Access[0].AccessControlType | Should Be ([System.Security.AccessControl.AccessControlType]::Allow)
+                $a.IdentityReference.Value -in $identities | Should Be $true           
-            $myACL.Access[0].FileSystemRights | Should Be ([System.Security.AccessControl.FileSystemRights]::FullControl)
+
-            $myACL.Access[0].IsInherited | Should Be $false
+                switch ($a.IdentityReference.Value)
-            $myACL.Access[0].InheritanceFlags | Should Be ([System.Security.AccessControl.InheritanceFlags]::None)
+                {
-            $myACL.Access[0].PropagationFlags | Should Be ([System.Security.AccessControl.PropagationFlags]::None)            
+                    {$_ -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;
+                    }
+                }
+            
+                $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
+            Push-Location $testDir
-            cd $testDir
             remove-item ssh_host_*_key* -ErrorAction SilentlyContinue
             ssh-keygen -A
+            Pop-Location
             
-            Get-ChildItem ssh_host_*_key | % {
+            Get-ChildItem (join-path $testDir ssh_host_*_key) | % {
-                ValidKeyFile -Path $_.FullName
+                ValidateKeyFile -FilePath $_.FullName
             }
 
-            Get-ChildItem ssh_host_*_key.pub | % {
+            Get-ChildItem (join-path $testDir ssh_host_*_key.pub) | % {
-                ValidKeyFile -Path $_.FullName
+                ValidateKeyFile -FilePath $_.FullName
             }            
-            cd $cd
         }
 
         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
+                ValidateKeyFile -FilePath $keyPath
-                ValidKeyFile -Path "$keyPath.pub"
+                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 }
+            Add-PasswordSetting -Pass $keypassphrase
-            $env:SSH_ASKPASS="$($env:ComSpec) /c echo $($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-PasswordSetting
-            remove-item "env:SSH_ASKPASS" -ErrorAction SilentlyContinue
 
             #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 | where { $_.contains($pubkeyraw) }).count | Should Be 0
-                    $allkeys.Contains($pubkeyraw) | Should Be $false
-                }
-                else { 
-                    ($allkeys | foreach {$_.Contains($pubkeyraw)}).Contains($true) | Should Be $false  
-                }
-
             }            
+        }        
+    }
 
+    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"
+			cmd /c "ssh-keyscan -p 22 github.com 2>&1 > $outputFile"
-			'out.txt' | Should Contain 'github.com ssh-rsa.*' 			
+			$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"
+			cmd /c "ssh-keyscan -p 22 github.com 2>&1 > $outputFile"
-			'out.txt' | Should Contain 'github.com ssh-rsa.*' 			
+			$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"
+			cmd /c "ssh-keyscan -f tmp.txt 2>&1 > $outputFile"
-			'out.txt' | Should Contain 'github.com ssh-rsa.*' 			
+			$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"
+			cmd /c "ssh-keyscan -f tmp.txt -t rsa,dsa 2>&1 > $outputFile"
-			'out.txt' | Should Contain 'github.com ssh-rsa.*' 			
+			$outputFile | Should Contain 'github.com ssh-rsa.*' 			
 		}
 	}
 }

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 } }
+        }
+    }
+}

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,21 +183,20 @@
     }
 
     AfterAll {
-        if(!$OpenSSHTestInfo["DebugMode"])
+       Get-ChildItem $serverDirectory | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue
-        {
+       Get-ChildItem $clientDirectory | Remove-Item -Recurse -Force -ErrorAction SilentlyContinue
-            Get-Item $rootDirectory | 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
+       $outputFilePath = Join-Path $rootDirectory "$($script:testId).$outputFileName"
-       Remove-Item $outputFilePath
+       $batchFilePath = Join-Path $rootDirectory "$($script:testId).$batchFileName"
     }
 
     AfterEach {
         CopyDebugLogs
+        $script:testId++
     }    
 
     It '<Title>' -TestCases:$testData1 {
@@ -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
+    }
 }

regress/pesterTests/Userkey_fileperm.Tests.ps1

@@ -1,185 +1,139 @@
-Import-Module $PSScriptRoot\CommonUtils.psm1 -Force
+Import-Module $PSScriptRoot\CommonUtils.psm1 -Force -DisableNameChecking
-Describe "Tests for user Key file permission" -Tags "Scenario" {
+
+$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) {
+        $keypassphrase = "testpassword"
-            Set-SecureFileACL -filepath $keyFilePath
+
-        }
+        $systemAccount = New-Object System.Security.Principal.NTAccount("NT AUTHORITY", "SYSTEM")
-        #add wrong password so ssh does not prompt password if failed with authorized keys
+        $adminsAccount = New-Object System.Security.Principal.NTAccount("BUILTIN","Administrators")
-        Add-PasswordSetting -Pass "WrongPass"
+        $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 
+    AfterEach {$tI++;}    
-    existing default test environment.
-    Context "ssh-add key files" {
-        BeforeEach {
-            ssh-add -D 2>&1 > $fileName
-        }
 
-        AfterEach {
+    Context "$tC-ssh with private key file" {
-            ssh-add -D 2>&1 > $fileName
-            if(Test-Path $keyFilePath) {
-                Set-SecureFileACL -filepath $keyFilePath
-            }            
-        }
-
-        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 {            
+            $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 
+
             $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
                 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 {
+        It "$tC.$tI-ssh with private key file -- positive (Secured private key owned by current user)" {
-            if(Test-Path $keyFilePath) {
+            Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $currentUser -OwnerPerms "Read, Write"
-                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
             #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-FileOwnerAndACL -FilePath $keyFilePath -Owner $adminsAccount -OwnerPerms "FullControl"
-            Set-SecureFileACL -filepath $keyFilePath -Owner $objAdmin
 
             #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 -- 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 "$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-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!"
+            $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!"
-            $matches.Count | Should Be 1
         }
 
-        It 'ssh with private key file -- (the private key has wrong owner)' {
+        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            
-            $objUser = New-Object System.Security.Principal.NTAccount($ssouser)
+            Set-FileOwnerAndACL -FilePath $keyFilePath -Owner $objUser -OwnerPerms "Read, Write"            
-            Set-SecureFileACL -filepath $keyFilePath -owner $objUser
+            Add-PermissionToFileACL -FilePath $keyFilePath -User $adminsAccount -Perm "FullControl"
 
-            $currentUser = New-Object System.Security.Principal.NTAccount($($env:USERDOMAIN), $($env:USERNAME))
+            $o = ssh -p $port -i $keyFilePath -E $logPath $pubKeyUser@$server echo 1234
-            Add-PermissionToFileACL -FilePath $keyFilePath -User $currentUser -Perm "FullControl"
-
-            $o = ssh -p $port -i $keyFilePath -E $filePath $pubKeyUser@$server echo 1234
             $LASTEXITCODE | Should Not Be 0
 
-            $matches = Get-Content $filePath | Select-String -pattern "UNPROTECTED PRIVATE KEY FILE!"
+            $logPath | Should Contain "UNPROTECTED PRIVATE KEY FILE!"
-            $matches.Count | Should Be 1
+        }
+
+        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!"
         }
     }
 }

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);
 

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);

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");
@@ -1710,15 +1693,6 @@ 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 */
-		
 		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 */

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 */