From 1a1a2ac5f1720f791416f3f16e15b369a5f05fe9 Mon Sep 17 00:00:00 2001
From: bagajjal <bagajjal@microsoft.com>
Date: Tue, 9 Mar 2021 10:02:51 -0800
Subject: [PATCH] administrators authorized keys file can have read permissions
 for other users. (#481)

---
 auth.c                                        | 43 ++++++++++---------
 .../Authorized_keys_fileperm.Tests.ps1        | 19 +++++++-
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/auth.c b/auth.c
index 77497ffc5..0f651c488 100644
--- a/auth.c
+++ b/auth.c
@@ -521,13 +521,16 @@ auth_openfile(const char *file, struct passwd *pw, int strict_modes,
 	FILE *f;
 
 #ifdef WINDOWS
-        /* Windows POSIX adapter does not support fdopen() on open(file)*/
-        if ((f = fopen(file, "r")) == NULL) {
-                debug("Could not open %s '%s': %s", file_type, file,
-                        strerror(errno));
-                return NULL;
-        }
-	if (strict_modes && check_secure_file_permission(file, pw, 0) != 0) {
+	/* Windows POSIX adapter does not support fdopen() on open(file)*/
+	if ((f = fopen(file, "r")) == NULL) {
+		debug("Could not open %s '%s': %s", file_type, file,
+			strerror(errno));
+		return NULL;
+	}
+
+	// read permissions for non-admin/non-system accounts are allowed.
+	// Unix does safe_path_fd() which allows 022 file permissions i.e., allowing read for other users.
+	if (strict_modes && check_secure_file_permission(file, pw, 1) != 0) {
 		fclose(f);
 		logit("Authentication refused.");
 		auth_debug_add("Ignored %s", file_type);
@@ -947,19 +950,19 @@ subprocess(const char *tag, struct passwd *pw, const char *command,
 	}
 	restore_uid();
 
-#ifdef FORK_NOT_SUPPORTED
-	{
-		posix_spawn_file_actions_t actions;
-		pid = -1;
-
-		if (posix_spawn_file_actions_init(&actions) != 0 ||
-			posix_spawn_file_actions_adddup2(&actions, p[1], STDOUT_FILENO) != 0)
-			fatal("posix_spawn initialization failed");
-		else if (__posix_spawn_asuser((pid_t*)&pid, av[0], &actions, NULL, av, NULL, pw->pw_name) != 0)
-			fatal("posix_spawn: %s", strerror(errno));
-
-		posix_spawn_file_actions_destroy(&actions);
-	}
+#ifdef FORK_NOT_SUPPORTED
+	{
+		posix_spawn_file_actions_t actions;
+		pid = -1;
+
+		if (posix_spawn_file_actions_init(&actions) != 0 ||
+			posix_spawn_file_actions_adddup2(&actions, p[1], STDOUT_FILENO) != 0)
+			fatal("posix_spawn initialization failed");
+		else if (__posix_spawn_asuser((pid_t*)&pid, av[0], &actions, NULL, av, NULL, pw->pw_name) != 0)
+			fatal("posix_spawn: %s", strerror(errno));
+
+		posix_spawn_file_actions_destroy(&actions);
+	}
 #else 
 	switch ((pid = fork())) {
 	case -1: /* error */
diff --git a/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1 b/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1
index 3577f2f9f..76b5cb930 100644
--- a/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1
+++ b/regress/pesterTests/Authorized_keys_fileperm.Tests.ps1
@@ -141,6 +141,21 @@ Describe "Tests for authorized_keys file permission" -Tags "CI" {
             $o | Should Be "1234"          
         }
 
+        It "$tC.$tI-authorized_keys-positive(other account can read authorized_keys file)"  -skip:$skip {
+            #setup to have current user as owner and grant it full control
+            Repair-FilePermission -Filepath $authorizedkeyPath -Owner $objUserSid -FullAccessNeeded $adminsSid,$systemSid,$objUserSid -confirm:$false
+
+            #add $PwdUser to access the file authorized_keys
+            $objPwdUserSid = Get-UserSid -User $PwdUser
+            Set-FilePermission -FilePath $authorizedkeyPath -User $objPwdUserSid -Perm "Read"
+
+            #Run
+            Start-SSHDTestDaemon -workDir $opensshbinpath -Arguments "-d -f $sshdconfig -o `"AuthorizedKeysFile .testssh/authorized_keys`" -E $sshdlog" -Port $port
+            $o = ssh -p $port -E $sshlog $ssouser@$server echo 1234
+            Stop-SSHDTestDaemon -Port $port
+            $o | Should Be "1234"
+        }
+
         It "$tC.$tI-authorized_keys-negative(authorized_keys is owned by other admin user)"  -skip:$skip {
             #setup to have current user (admin user) as owner and grant it full control
             Repair-FilePermission -Filepath $authorizedkeyPath -Owner $currentUserSid -FullAccessNeeded $adminsSid,$systemSid -confirm:$false
@@ -154,13 +169,13 @@ Describe "Tests for authorized_keys file permission" -Tags "CI" {
             $sshdlog | Should Contain "Authentication refused."            
         }
 
-        It "$tC.$tI-authorized_keys-negative(other account can access private key file)"  -skip:$skip {
+        It "$tC.$tI-authorized_keys-negative(other account has modify permissions to authorized_keys file)"  -skip:$skip {
             #setup to have current user as owner and grant it full control            
             Repair-FilePermission -Filepath $authorizedkeyPath -Owner $objUserSid -FullAccessNeeded $adminsSid,$systemSid,$objUserSid -confirm:$false
 
             #add $PwdUser to access the file authorized_keys
             $objPwdUserSid = Get-UserSid -User $PwdUser
-            Set-FilePermission -FilePath $authorizedkeyPath -User $objPwdUserSid -Perm "Read"
+            Set-FilePermission -FilePath $authorizedkeyPath -User $objPwdUserSid -Perm "Modify"
 
             #Run
             Start-SSHDTestDaemon -workDir $opensshbinpath -Arguments "-d -f $sshdconfig -o `"AuthorizedKeysFile .testssh/authorized_keys`" -E $sshdlog" -Port $port