login-utils/su-common: Check the user didn't change during PAM transaction

[3v1n0]

PAM modules can change the user during their execution, in such case su would still use the user that has been provided giving potentially access to another user with the credentials of another one.

So prevent this to happen, by ensuring that the final PAM user is matching the one required.

---

Another option is instead to perform the access for the new user as requested by PAM (and it looks somewhat better to me, but it has more implications):

diff --git a/login-utils/su-common.c b/login-utils/su-common.c
index 12c933633..409d7d4de 100644
--- a/login-utils/su-common.c
+++ b/login-utils/su-common.c
@@ -379,9 +379,10 @@ static void supam_export_environment(struct su_context *su)
 	}
 }
 
-static void supam_authenticate(struct su_context *su)
+static const char *supam_authenticate(struct su_context *su)
 {
 	const char *srvname = NULL;
+	const char *pam_user = NULL;
 	int rc;
 
 	srvname = su->runuser ?
@@ -411,7 +412,7 @@ static void supam_authenticate(struct su_context *su)
 		 */
 		if (su->restricted)
 			errx(EXIT_FAILURE, _("may not be used by non-root users"));
-		return;
+		return NULL;
 	}
 
 	rc = pam_authenticate(su->pamh, 0);
@@ -422,6 +423,16 @@ static void supam_authenticate(struct su_context *su)
 	rc = pam_acct_mgmt(su->pamh, 0);
 	if (rc == PAM_NEW_AUTHTOK_REQD)
 		rc = pam_chauthtok(su->pamh, PAM_CHANGE_EXPIRED_AUTHTOK);
+
+	rc = pam_get_item(su->pamh, PAM_USER, (const void **) &pam_user);
+	if (is_pam_failure(rc))
+		goto done;
+
+	if (pam_user == NULL) {
+		rc = PAM_USER_UNKNOWN;
+		goto done;
+	}
+
  done:
 	log_syslog(su, !is_pam_failure(rc));
 
@@ -436,6 +447,8 @@ static void supam_authenticate(struct su_context *su)
 		sleep(getlogindefs_num("FAIL_DELAY", 1));
 		errx(EXIT_FAILURE, "%s", msg ? msg : _("authentication failed"));
 	}
+
+	return pam_user;
 }
 
 static void supam_open_session(struct su_context *su)
@@ -997,6 +1010,7 @@ int su_main(int argc, char **argv, int mode)
 	char *command = NULL;
 	int request_same_session = 0;
 	char *shell = NULL;
+	const char *pam_user = NULL;
 
 	gid_t *groups = NULL;
 	size_t ngroups = 0;
@@ -1155,6 +1169,7 @@ int su_main(int argc, char **argv, int mode)
 	logindefs_set_loader(load_config, (void *) su);
 	init_tty(su);
 
+getpwnam:
 	su->pwd = xgetpwnam(su->new_user, &su->pwdbuf);
 	if (!su->pwd
 	    || !su->pwd->pw_passwd
@@ -1175,7 +1190,13 @@ int su_main(int argc, char **argv, int mode)
 	else if (use_gid)
 		su->pwd->pw_gid = gid;
 
-	supam_authenticate(su);
+	if (pam_user == NULL)
+		pam_user = supam_authenticate(su);
+
+	if (pam_user && strcmp (su->new_user, pam_user) != 0) {
+		su->new_user = (char *) pam_user;
+		goto getpwnam;
+	}
 
 	if (request_same_session || !command || !su->pwd->pw_uid)
 		su->same_session = 1;

[karelzak]

I don't like the idea that su(1) will accept username change by PAM (the goto getpwnam loop). It would be better to end with an error message if the desired user (su->new_user) does not match with pam_get_item(PAM_USER). It would be enough to make this check in supam_authenticate() after successful authentication.

[3v1n0]

I don't like the idea that su(1) will accept username change by PAM (the goto getpwnam loop). It would be better to end with an error message if the desired user (su->new_user) does not match with pam_get_item(PAM_USER). It would be enough to make this check in supam_authenticate() after successful authentication.

Sure @karelzak, that's in fact the proposed patch in my branch (we may even be more clear on error message), since I feel it's safer to use, the other diff was a (quick) alternative solution that can be also used, but that could indeed lead to more discussion.

[zeha]

@3v1n0 have you proposed forbidding this to PAM upstream?

[3v1n0]

@zeha I was indeed thinking on this, but I'm unsure if there are modules that we may break as they rely on the ability of changing the user, despite being set (if they're the one having set it already).

So I feel that this wouldn't be correct either, but maybe we can make sure that a module don't override an user if it's the one setting it. @ldv-alt may have further opinions on this.