Update ssh folder permissions check in SSHD (#761)

* remove check on progdata/ssh/log folder permissions

* add pester test

* modify permissions check to log event without failing startup

* modify perm check

* update test

* uncomment code

* modify pester test

* address review feedback

* address review feedback

* fix multi-line logging

* cleanup allocations

* address review feedback

* address additional review feedback

* store value in tmp var

contrib/win32/win32compat/misc.c

@@ -1419,7 +1419,7 @@ is_absolute_path(const char *path)
 
 /* return -1 - in case of failure, 0 - success */
 int
-create_directory_withsddl(wchar_t *path_w, wchar_t *sddl_w)
+create_directory_withsddl(wchar_t *path_w, wchar_t *sddl_w, BOOL check_permissions)
 {
 	if (GetFileAttributesW(path_w) == INVALID_FILE_ATTRIBUTES) {
 		PSECURITY_DESCRIPTOR pSD = NULL;
@@ -1444,12 +1444,9 @@ create_directory_withsddl(wchar_t *path_w, wchar_t *sddl_w)
 			return -1;
 		}
 	}
-	else {
+	else if (check_permissions) {
 		// directory already exists; need to confirm permissions are correct
-		if (check_secure_folder_permission(path_w, 1) != 0) {
-			error("Directory already exists but folder permissions are invalid");
-			return -1;
-		}
+		check_secure_folder_permission(path_w, 1);
 	}
 
 	return 0;

contrib/win32/win32compat/misc_internal.h

@@ -67,7 +67,7 @@ void to_lower_case(char *s);
 void to_wlower_case(wchar_t *s);
 HANDLE get_user_token(const char* user, int impersonation);
 int load_user_profile(HANDLE user_token, char* user);
-int create_directory_withsddl(wchar_t *path, wchar_t *sddl);
+int create_directory_withsddl(wchar_t *path, wchar_t *sddl, BOOL check_permissions);
 int is_absolute_path(const char *);
 int file_in_chroot_jail(HANDLE);
 PSID lookup_sid(const wchar_t* name_utf16, PSID psid, DWORD * psid_len);

contrib/win32/win32compat/w32-sshfileperm.c

@@ -33,6 +33,8 @@
 #include <Aclapi.h>
 #include <lm.h>
 #include <stdio.h> 
+#include <stdlib.h>
+#include <string.h>
 
 #include "inc\pwd.h"
 #include "sshfileperm.h"
@@ -40,6 +42,12 @@
 #include "misc_internal.h"
 #include "config.h"
 
+#define NULL_TERMINATOR_LEN		1
+#define COMMA_SPACE_LEN			2
+#define BACKSLASH_LEN			1
+
+extern int log_on_stderr;
+
 /*
 * The function is to check if current user is secure to access to the file. 
 * Check the owner of the file is one of these types: Local Administrators groups, system account, current user account
@@ -178,18 +186,22 @@ cleanup:
 * Check the owner of the file is one of these types: Local Administrators groups or system account
 * Check the users have access permission to the file don't violate the following rules:
 	1. no user other than local administrators group and system account have write permission on the folder
-* Returns 0 on success and -1 on failure
+* Logs a message if the rules are violated, but does not prevent further execution
 */
-int
+void
 check_secure_folder_permission(const wchar_t* path_utf16, int read_ok)
 {
 	PSECURITY_DESCRIPTOR pSD = NULL;
 	PSID owner_sid = NULL, ti_sid = NULL;
 	PACL dacl = NULL;
 	DWORD error_code = ERROR_SUCCESS;
-	BOOL is_valid_sid = FALSE, is_valid_acl = FALSE;
+	BOOL is_valid_sid = FALSE, is_valid_acl = FALSE, need_log_msg = FALSE, is_first = TRUE;
 	wchar_t* bad_user = NULL;
-	int ret = 0;
+	size_t log_msg_len = (DNLEN + BACKSLASH_LEN + UNLEN) * 2 + COMMA_SPACE_LEN + NULL_TERMINATOR_LEN;
+	wchar_t* log_msg = (wchar_t*)malloc(log_msg_len * sizeof(wchar_t));
+	if (log_msg != NULL) {
+		log_msg[0] = '\0';
+	}
 
 	/*Get the owner sid of the file.*/
 	if ((error_code = GetNamedSecurityInfoW(path_utf16, SE_FILE_OBJECT,
@@ -197,18 +209,15 @@ check_secure_folder_permission(const wchar_t* path_utf16, int read_ok)
 		&owner_sid, NULL, &dacl, NULL, &pSD)) != ERROR_SUCCESS) {
 		printf("failed to retrieve the owner sid and dacl of file %S with error code: %d", path_utf16, error_code);
 		errno = EOTHER;
-		ret = -1;
 		goto cleanup;
 	}
 	if (((is_valid_sid = IsValidSid(owner_sid)) == FALSE) || ((is_valid_acl = IsValidAcl(dacl)) == FALSE)) {
 		printf("IsValidSid: %d; is_valid_acl: %d", is_valid_sid, is_valid_acl);
-		ret = -1;
 		goto cleanup;
 	}
 	if (!IsWellKnownSid(owner_sid, WinBuiltinAdministratorsSid) &&
 		!IsWellKnownSid(owner_sid, WinLocalSystemSid)) {
 		printf("Bad owner on %S", path_utf16);
-		ret = -1;
 		goto cleanup;
 	}
 	/*
@@ -224,7 +233,6 @@ check_secure_folder_permission(const wchar_t* path_utf16, int read_ok)
 		if (!GetAce(dacl, i, &current_ace)) {
 			printf("GetAce() failed");
 			errno = EOTHER;
-			ret = -1;
 			goto cleanup;
 		}
 
@@ -247,15 +255,112 @@ check_secure_folder_permission(const wchar_t* path_utf16, int read_ok)
 			continue;
 		}
 		else {
-			ret = -1;
+			/* collect all SIDs with write permissions */
+			wchar_t resolved_trustee[UNLEN + NULL_TERMINATOR_LEN] = L"UNKNOWN";
+			wchar_t resolved_trustee_domain[DNLEN + NULL_TERMINATOR_LEN] = L"UNKNOWN";
+			DWORD resolved_trustee_len = _countof(resolved_trustee), resolved_trustee_domain_len = _countof(resolved_trustee_domain);
+			SID_NAME_USE resolved_trustee_type;
+
+			need_log_msg = TRUE;
+
+			if (log_msg != NULL &&
+				LookupAccountSidW(NULL, current_trustee_sid, resolved_trustee, &resolved_trustee_len,
+				resolved_trustee_domain, &resolved_trustee_domain_len, &resolved_trustee_type) != 0) {
+				if (is_first) {
+					_snwprintf_s(log_msg, log_msg_len, _TRUNCATE, L"%ls\\%ls", resolved_trustee_domain, resolved_trustee);
+					is_first = FALSE;
+				}
+				else {
+					size_t currentLength = wcslen(log_msg);
+					size_t userLength = resolved_trustee_domain_len + BACKSLASH_LEN + resolved_trustee_len + COMMA_SPACE_LEN;
+					if (wcslen(log_msg) + userLength + NULL_TERMINATOR_LEN > log_msg_len) {
+						log_msg_len *= 2;
+						wchar_t* temp_log_msg = (wchar_t*)malloc(log_msg_len * sizeof(wchar_t));
+						if (temp_log_msg == NULL) {
+							break;
+						}
+						wcscpy_s(temp_log_msg, log_msg_len, log_msg);
+						if (log_msg)
+							free(log_msg);
+						log_msg = temp_log_msg;
+					}
+					_snwprintf_s(log_msg + currentLength, log_msg_len - currentLength, _TRUNCATE, 
+						L", %ls\\%ls", resolved_trustee_domain, resolved_trustee);
+				}
+			}
 		}
 	}
+
+	if (need_log_msg) {
+		log_folder_perms_msg_etw(path_utf16, log_msg);
+	}
 cleanup:
-	if (bad_user)
+	if (bad_user) {
 		LocalFree(bad_user);
-	if (pSD)
+	}
+	if (log_msg) {
+		free(log_msg);
+	}
+	if (pSD) {
 		LocalFree(pSD);
-	if (ti_sid)
+	}
+	if (ti_sid) {
 		free(ti_sid);
-	return ret;
+	}
+}
+
+/* 
+* This function takes in the full path to the ProgramData\ssh folder
+* and a string of comma-separated domain\usernames. The function converts 
+* the well-known built-in Administrators group sid and the Local System 
+* sid to their corresponding names. With these names, and the input string, 
+* it logs a message to the Event Viewer. If logging the detailed message fails, 
+* a generic log message is written to the Event Viewer instead.
+*/
+void log_folder_perms_msg_etw(const wchar_t* path_utf16, wchar_t* log_msg) {
+	PSID adminSid = NULL;
+	WCHAR adminName[UNLEN + NULL_TERMINATOR_LEN];
+	WCHAR adminDomain[DNLEN + NULL_TERMINATOR_LEN];
+	DWORD adminNameSize = UNLEN + NULL_TERMINATOR_LEN;
+	DWORD adminDomainSize = DNLEN + NULL_TERMINATOR_LEN;
+	DWORD adminSidSize = SECURITY_MAX_SID_SIZE;
+	PSID systemSid = NULL;
+	WCHAR systemName[UNLEN + NULL_TERMINATOR_LEN];
+	WCHAR systemDomain[DNLEN + NULL_TERMINATOR_LEN];
+	DWORD systemNameSize = UNLEN + NULL_TERMINATOR_LEN;
+	DWORD systemDomainSize = DNLEN + NULL_TERMINATOR_LEN;
+	DWORD systemSidSize = SECURITY_MAX_SID_SIZE;
+	SID_NAME_USE sidType;
+	BOOL needLog = TRUE;
+	int temp_log_on_stderr = log_on_stderr;
+	log_on_stderr = 0;
+
+	adminSid = (PSID)malloc(SECURITY_MAX_SID_SIZE);
+	if (log_msg != NULL && adminSid != NULL &&
+		CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, adminSid, &adminSidSize) != 0 &&
+		LookupAccountSidW(NULL, adminSid, adminName, &adminNameSize, adminDomain, &adminDomainSize, &sidType) != 0) {
+		systemSid = (PSID)malloc(SECURITY_MAX_SID_SIZE);
+		if (systemSid != NULL &&
+			CreateWellKnownSid(WinLocalSystemSid, NULL, systemSid, &systemSidSize) != 0 &&
+			LookupAccountSidW(NULL, systemSid, systemName, &systemNameSize, systemDomain, &systemDomainSize, &sidType) != 0) {
+			logit("For '%S' folder, write access is granted to the following users: %S. "
+				"Consider reviewing users to ensure that only %S\\%S, and the %S\\%S group, and its members, have write access.", 
+				path_utf16, log_msg, systemDomain, systemName, adminDomain, adminName);
+			needLog = FALSE;
+		}
+	}
+
+	if (needLog) {
+		/* log generic warning message in unlikely case that lookup for either well-known SID fails or user list is empty */
+		logit("for '%S' folder, consider downgrading permissions for any users with unnecessary write access.", path_utf16);
+	}
+
+	log_on_stderr = temp_log_on_stderr;
+
+	if (adminSid) {
+		free(adminSid);
+	}
+	if (systemSid) {
+		free(systemSid);
+	}
 }

contrib/win32/win32compat/wmain_sshd.c

@@ -135,7 +135,7 @@ create_prgdata_ssh_folder()
 	wchar_t ssh_cfg_dir[PATH_MAX] = { 0, };
 	wcscpy_s(ssh_cfg_dir, _countof(ssh_cfg_dir), __wprogdata);
 	wcscat_s(ssh_cfg_dir, _countof(ssh_cfg_dir), L"\\ssh");
-	if (create_directory_withsddl(ssh_cfg_dir, L"O:BAD:PAI(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;0x1200a9;;;AU)") < 0) {
+	if (create_directory_withsddl(ssh_cfg_dir, L"O:BAD:PAI(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;0x1200a9;;;AU)", TRUE) < 0) {
 		printf("failed to create %S", ssh_cfg_dir);
 		exit(255);
 	}
@@ -144,7 +144,7 @@ create_prgdata_ssh_folder()
 	wchar_t logs_dir[PATH_MAX] = { 0, };
 	wcscat_s(logs_dir, _countof(logs_dir), ssh_cfg_dir);
 	wcscat_s(logs_dir, _countof(logs_dir), L"\\logs");
-	if (create_directory_withsddl(logs_dir, L"O:BAD:PAI(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)") < 0) {
+	if (create_directory_withsddl(logs_dir, L"O:BAD:PAI(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)", FALSE) < 0) {
 		printf("failed to create %S", logs_dir);
 		exit(255);
 	}

log.c

@@ -54,7 +54,11 @@
 #include "match.h"
 
 static LogLevel log_level = SYSLOG_LEVEL_INFO;
+#ifdef WINDOWS
+int log_on_stderr = 1;
+#else
 static int log_on_stderr = 1;
+#endif /* WINDOWS */
 static int log_stderr_fd = STDERR_FILENO;
 static int log_facility = LOG_AUTH;
 static const char *argv0;

regress/pesterTests/Setup.Tests.ps1

@@ -521,4 +521,52 @@ Describe "Setup Tests" -Tags "Setup" {
             $fwportFilter.RemotePort | Should Be 'Any'
         }
     }
+
+    Context "$tC - Validate SSHD service startup" {
+        BeforeAll {
+            $tI=1
+            $sshFolderPath = Join-Path $env:ProgramData "ssh"
+            $sshACL = $null
+            if (Test-Path -Path $sshFolderPath) {
+                $sshACL = Get-Acl $sshFolderPath
+            }
+            $logFolderPath = Join-Path $env:ProgramData "ssh" "logs"
+            $logACL = $null
+            if (Test-Path -Path $logFolderPath) {
+                $logACL = Get-Acl $logFolderPath
+            }
+        }
+        AfterAll {
+            $tC++
+            if ($logACL -eq $null) {
+                Remove-Item -Path $logFolderPath -Recurse -Force
+            }
+            if ($sshACL -eq $null) {
+                Remove-Item -Path $sshFolderPath -Recurse -Force
+            }
+        }
+        AfterEach {
+            $tI++
+            net stop sshd
+            if ($sshACL -ne $null) {
+                Set-Acl -Path $sshFolderPath -AclObject $sshACL
+            }
+            if ($logACL -ne $null) {
+                Set-Acl -Path $logFolderPath -AclObject $logACL
+            }
+        }
+
+        It "$tC.$tI - SSHD starts up successfully when Authenticated Users have read control over log folder" {
+            if (-not (Test-Path -Path $logFolderPath)) {
+                New-Item -Path $logFolderPath -ItemType Directory -Force
+            }
+            # Set ACLs on the folder
+            $acl = Get-Acl $logFolderPath
+            $accessRule = New-Object System.Security.AccessControl.FileSystemAccessRule($authenticatedUserSid, "ReadAndExecute", "Allow")
+            $acl.SetAccessRule($accessRule)
+            Set-Acl -Path $logFolderPath -AclObject $acl
+            net start sshd
+            $LASTEXITCODE | Should Be 0
+        }
+    }
 }

sshfileperm.h

@@ -26,5 +26,6 @@
 #define _SSH_FILE_PERM_H
 
 int check_secure_file_permission(const char *, struct passwd *, int);
-int check_secure_folder_permission(const wchar_t*, int);
+void check_secure_folder_permission(const wchar_t*, int);
+void log_folder_perms_msg_etw(const wchar_t*, wchar_t*);
 #endif /* _SSH_FILE_PERM_H */