[openssh-commits] [openssh] 01/02: upstream: Improve rules for %-expansion of username.

[git+noreply at mindrot.org]

This is an automated email from the git hooks/post-receive script.

djm pushed a commit to branch master
in repository openssh.

commit 35d5917652106aede47621bb3f64044604164043
Author: djm at openbsd.org <djm at openbsd.org>
AuthorDate: Thu Sep 4 00:29:09 2025 +0000

    upstream: Improve rules for %-expansion of username.
    
    Usernames passed on the commandline will no longer be subject to
    % expansion. Some tools invoke ssh with connection information
    (i.e. usernames and host names) supplied from untrusted sources.
    These may contain % expansion sequences which could yield
    unexpected results.
    
    Since openssh-9.6, all usernames have been subject to validity
    checking. This change tightens the validity checks by refusing
    usernames that include control characters (again, these can cause
    surprises when supplied adversarially).
    
    This change also relaxes the validity checks in one small way:
    usernames supplied via the configuration file as literals (i.e.
    include no % expansion characters) are not subject to these
    validity checks. This allows usernames that contain arbitrary
    characters to be used, but only via configuration files. This
    is done on the basis that ssh's configuration is trusted.
    
    Pointed out by David Leadbeater, ok deraadt@
    
    OpenBSD-Commit-ID: e2f0c871fbe664aba30607321575e7c7fc798362
---
 ssh.c | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/ssh.c b/ssh.c
index 1eaa5986e..6bbb82874 100644
--- a/ssh.c
+++ b/ssh.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: ssh.c,v 1.616 2025/08/29 03:50:38 djm Exp $ */
+/* $OpenBSD: ssh.c,v 1.617 2025/09/04 00:29:09 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo at cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo at cs.hut.fi>, Espoo, Finland
@@ -668,6 +668,8 @@ valid_ruser(const char *s)
 	if (*s == '-')
 		return 0;
 	for (i = 0; s[i] != 0; i++) {
+		if (iscntrl((u_char)s[i]))
+			return 0;
 		if (strchr("'`\";&<>|(){}", s[i]) != NULL)
 			return 0;
 		/* Disallow '-' after whitespace */
@@ -689,6 +691,7 @@ main(int ac, char **av)
 	struct ssh *ssh = NULL;
 	int i, r, opt, exit_status, use_syslog, direct, timeout_ms;
 	int was_addr, config_test = 0, opt_terminated = 0, want_final_pass = 0;
+	int user_on_commandline = 0, user_was_default = 0, user_expanded = 0;
 	char *p, *cp, *line, *argv0, *logfile, *args;
 	char cname[NI_MAXHOST], thishost[NI_MAXHOST];
 	struct stat st;
@@ -1037,8 +1040,10 @@ main(int ac, char **av)
 			}
 			break;
 		case 'l':
-			if (options.user == NULL)
+			if (options.user == NULL) {
 				options.user = xstrdup(optarg);
+				user_on_commandline = 1;
+			}
 			break;
 
 		case 'L':
@@ -1141,6 +1146,7 @@ main(int ac, char **av)
 			if (options.user == NULL) {
 				options.user = tuser;
 				tuser = NULL;
+				user_on_commandline = 1;
 			}
 			free(tuser);
 			if (options.port == -1 && tport != -1)
@@ -1155,6 +1161,7 @@ main(int ac, char **av)
 				if (options.user == NULL) {
 					options.user = p;
 					p = NULL;
+					user_on_commandline = 1;
 				}
 				*cp++ = '\0';
 				host = xstrdup(cp);
@@ -1314,8 +1321,10 @@ main(int ac, char **av)
 	if (fill_default_options(&options) != 0)
 		cleanup_exit(255);
 
-	if (options.user == NULL)
+	if (options.user == NULL) {
+		user_was_default = 1;
 		options.user = xstrdup(pw->pw_name);
+	}
 
 	/*
 	 * If ProxyJump option specified, then construct a ProxyCommand now.
@@ -1464,20 +1473,30 @@ main(int ac, char **av)
 	    "" : options.jump_host);
 
 	/*
-	 * Expand User. It cannot contain %r (itself) or %C since User is
+	 * If the user was specified via a configuration directive then attempt
+	 * to expand it. It cannot contain %r (itself) or %C since User is
 	 * a component of the hash.
 	 */
-	if (options.user != NULL) {
+	if (!user_on_commandline && !user_was_default) {
 		if ((p = percent_dollar_expand(options.user,
 		    DEFAULT_CLIENT_PERCENT_EXPAND_ARGS_NOUSER(cinfo),
 		    (char *)NULL)) == NULL)
 			fatal("invalid environment variable expansion");
+		user_expanded = strcmp(p, options.user) != 0;
 		free(options.user);
 		options.user = p;
-		if (!valid_ruser(options.user))
-			fatal("remote username contains invalid characters");
 	}
 
+	/*
+	 * Usernames specified on the commandline or expanded from the
+	 * configuration file must be validated.
+	 * Conversely, usernames from getpwnam(3) or specified as literals
+	 * via configuration (i.e. not expanded) are not subject to validation.
+	 */
+	if ((user_on_commandline || user_expanded) &&
+	    !valid_ruser(options.user))
+		fatal("remote username contains invalid characters");
+
 	/* Now User is expanded, store it and calculate hash. */
 	cinfo->remuser = xstrdup(options.user);
 	cinfo->conn_hash_hex = ssh_connection_hash(cinfo->thishost,

-- 
To stop receiving notification emails like this one, please contact
djm at mindrot.org.