upstream: Separate parsing of string array options from applying them

to the active configuration. This fixes the config parser from erroneously
rejecting cases like:

AuthenticationMethods password
Match User ivy
 AuthenticationMethods any

bz3657 ok markus@

OpenBSD-Commit-ID: 7f196cba634c2a3dba115f3fac3c4635a2199491

1 files changed, 110 insertions, 56 deletions

diff --git a/servconf.c b/servconf.c
index fc873195d..4b434909a 100644
--- a/servconf.c
+++ b/servconf.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: servconf.c,v 1.404 2024/02/20 04:10:03 djm Exp $ */
+/* $OpenBSD: servconf.c,v 1.405 2024/03/04 02:16:11 djm Exp $ */
 /*
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
  *                    All rights reserved
@@ -1298,12 +1298,12 @@ process_server_config_line_depth(ServerOptions *options, char *line,
     struct include_list *includes)
 {
 	char *str, ***chararrayptr, **charptr, *arg, *arg2, *p, *keyword;
-	int cmdline = 0, *intptr, value, value2, n, port, oactive, r, found;
-	int ca_only = 0;
+	int cmdline = 0, *intptr, value, value2, n, port, oactive, r;
+	int ca_only = 0, found = 0;
 	SyslogFacility *log_facility_ptr;
 	LogLevel *log_level_ptr;
 	ServerOpCodes opcode;
-	u_int i, *uintptr, uvalue, flags = 0;
+	u_int i, *uintptr, flags = 0;
 	size_t len;
 	long long val64;
 	const struct multistate *multistate_ptr;
@@ -1313,6 +1313,8 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 	char **oav = NULL, **av;
 	int oac = 0, ac;
 	int ret = -1;
+	char **strs = NULL; /* string array arguments; freed implicitly */
+	u_int nstrs = 0;
 
 	/* Strip trailing whitespace. Allow \f (form feed) at EOL only */
 	if ((len = strlen(line)) == 0)
@@ -1775,7 +1777,6 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 
 	case sLogVerbose:
 		found = options->num_log_verbose == 0;
-		i = 0;
 		while ((arg = argv_next(&ac, &av)) != NULL) {
 			if (*arg == '\0') {
 				error("%s line %d: keyword %s empty argument",
@@ -1784,19 +1785,25 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 			}
 			/* Allow "none" only in first position */
 			if (strcasecmp(arg, "none") == 0) {
-				if (i > 0 || ac > 0) {
+				if (nstrs > 0 || ac > 0) {
 					error("%s line %d: keyword %s \"none\" "
 					    "argument must appear alone.",
 					    filename, linenum, keyword);
 					goto out;
 				}
 			}
-			i++;
-			if (!found || !*activep)
-				continue;
 			opt_array_append(filename, linenum, keyword,
-			    &options->log_verbose, &options->num_log_verbose,
-			    arg);
+			    &strs, &nstrs, arg);
+		}
+		if (nstrs == 0) {
+			fatal("%s line %d: no %s specified",
+			    filename, linenum, keyword);
+		}
+		if (found && *activep) {
+			options->log_verbose = strs;
+			options->num_log_verbose = nstrs;
+			strs = NULL; /* transferred */
+			nstrs = 0;
 		}
 		break;
 
@@ -1822,16 +1829,22 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 		chararrayptr = &options->allow_users;
 		uintptr = &options->num_allow_users;
  parse_allowdenyusers:
+		/* XXX appends to list; doesn't respect first-match-wins */
 		while ((arg = argv_next(&ac, &av)) != NULL) {
 			if (*arg == '\0' ||
 			    match_user(NULL, NULL, NULL, arg) == -1)
 				fatal("%s line %d: invalid %s pattern: \"%s\"",
 				    filename, linenum, keyword, arg);
+			found = 1;
 			if (!*activep)
 				continue;
 			opt_array_append(filename, linenum, keyword,
 			    chararrayptr, uintptr, arg);
 		}
+		if (!found) {
+			fatal("%s line %d: no %s specified",
+			    filename, linenum, keyword);
+		}
 		break;
 
 	case sDenyUsers:
@@ -1842,16 +1855,22 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 	case sAllowGroups:
 		chararrayptr = &options->allow_groups;
 		uintptr = &options->num_allow_groups;
+		/* XXX appends to list; doesn't respect first-match-wins */
  parse_allowdenygroups:
 		while ((arg = argv_next(&ac, &av)) != NULL) {
 			if (*arg == '\0')
 				fatal("%s line %d: empty %s pattern",
 				    filename, linenum, keyword);
+			found = 1;
 			if (!*activep)
 				continue;
 			opt_array_append(filename, linenum, keyword,
 			    chararrayptr, uintptr, arg);
 		}
+		if (!found) {
+			fatal("%s line %d: no %s specified",
+			    filename, linenum, keyword);
+		}
 		break;
 
 	case sDenyGroups:
@@ -2035,7 +2054,7 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 	 * AuthorizedKeysFile	/etc/ssh_keys/%u
 	 */
 	case sAuthorizedKeysFile:
-		uvalue = options->num_authkeys_files;
+		found = options->num_authkeys_files == 0;
 		while ((arg = argv_next(&ac, &av)) != NULL) {
 			if (*arg == '\0') {
 				error("%s line %d: keyword %s empty argument",
@@ -2043,13 +2062,20 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 				goto out;
 			}
 			arg2 = tilde_expand_filename(arg, getuid());
-			if (*activep && uvalue == 0) {
-				opt_array_append(filename, linenum, keyword,
-				    &options->authorized_keys_files,
-				    &options->num_authkeys_files, arg2);
-			}
+			opt_array_append(filename, linenum, keyword,
+			    &strs, &nstrs, arg2);
 			free(arg2);
 		}
+		if (nstrs == 0) {
+			fatal("%s line %d: no %s specified",
+			    filename, linenum, keyword);
+		}
+		if (found && *activep) {
+			options->authorized_keys_files = strs;
+			options->num_authkeys_files = nstrs;
+			strs = NULL; /* transferred */
+			nstrs = 0;
+		}
 		break;
 
 	case sAuthorizedPrincipalsFile:
@@ -2075,34 +2101,47 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 		goto parse_int;
 
 	case sAcceptEnv:
+		/* XXX appends to list; doesn't respect first-match-wins */
 		while ((arg = argv_next(&ac, &av)) != NULL) {
 			if (*arg == '\0' || strchr(arg, '=') != NULL)
 				fatal("%s line %d: Invalid environment name.",
 				    filename, linenum);
+			found = 1;
 			if (!*activep)
 				continue;
 			opt_array_append(filename, linenum, keyword,
 			    &options->accept_env, &options->num_accept_env,
 			    arg);
 		}
+		if (!found) {
+			fatal("%s line %d: no %s specified",
+			    filename, linenum, keyword);
+		}
 		break;
 
 	case sSetEnv:
-		uvalue = options->num_setenv;
+		found = options->num_setenv == 0;
 		while ((arg = argv_next(&ac, &av)) != NULL) {
 			if (*arg == '\0' || strchr(arg, '=') == NULL)
 				fatal("%s line %d: Invalid environment.",
 				    filename, linenum);
-			if (!*activep || uvalue != 0)
-				continue;
-			if (lookup_setenv_in_list(arg, options->setenv,
-			    options->num_setenv) != NULL) {
+			if (lookup_setenv_in_list(arg, strs, nstrs) != NULL) {
 				debug2("%s line %d: ignoring duplicate env "
 				    "name \"%.64s\"", filename, linenum, arg);
 				continue;
 			}
 			opt_array_append(filename, linenum, keyword,
-			    &options->setenv, &options->num_setenv, arg);
+			    &strs, &nstrs, arg);
+		}
+		if (nstrs == 0) {
+			fatal("%s line %d: no %s specified",
+			    filename, linenum, keyword);
+		}
+		if (found && *activep) {
+			options->setenv = strs;
+			options->num_setenv = nstrs;
+			strs = NULL; /* transferred */
+			nstrs = 0;
 		}
 		break;
 
@@ -2253,21 +2292,20 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 			uintptr = &options->num_permitted_opens;
 			chararrayptr = &options->permitted_opens;
 		}
-		arg = argv_next(&ac, &av);
-		if (!arg || *arg == '\0')
-			fatal("%s line %d: %s missing argument.",
-			    filename, linenum, keyword);
-		uvalue = *uintptr;	/* modified later */
-		if (strcmp(arg, "any") == 0 || strcmp(arg, "none") == 0) {
-			if (*activep && uvalue == 0) {
-				*uintptr = 1;
-				*chararrayptr = xcalloc(1,
-				    sizeof(**chararrayptr));
-				(*chararrayptr)[0] = xstrdup(arg);
+		found = *uintptr == 0;
+		while ((arg = argv_next(&ac, &av)) != NULL) {
+			if (strcmp(arg, "any") == 0 ||
+			    strcmp(arg, "none") == 0) {
+				if (nstrs != 0) {
+					fatal("%s line %d: %s must appear "
+					    "alone on a %s line.",
+					    filename, linenum, arg, keyword);
+				}
+				opt_array_append(filename, linenum, keyword,
+				    &strs, &nstrs, arg);
+				continue;
 			}
-			break;
-		}
-		for (; arg != NULL && *arg != '\0'; arg = argv_next(&ac, &av)) {
+
 			if (opcode == sPermitListen &&
 			    strchr(arg, ':') == NULL) {
 				/*
@@ -2289,12 +2327,20 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 				fatal("%s line %d: %s bad port number",
 				    filename, linenum, keyword);
 			}
-			if (*activep && uvalue == 0) {
-				opt_array_append(filename, linenum, keyword,
-				    chararrayptr, uintptr, arg2);
-			}
+			opt_array_append(filename, linenum, keyword,
+			    &strs, &nstrs, arg2);
 			free(arg2);
 		}
+		if (nstrs == 0) {
+			fatal("%s line %d: %s missing argument.",
+			    filename, linenum, keyword);
+		}
+		if (found && *activep) {
+			*chararrayptr = strs;
+			*uintptr = nstrs;
+			strs = NULL; /* transferred */
+			nstrs = 0;
+		}
 		break;
 
 	case sForceCommand:
@@ -2419,10 +2465,9 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 	case sAuthenticationMethods:
 		found = options->num_auth_methods == 0;
 		value = 0; /* seen "any" pseudo-method */
-		value2 = 0; /* successfully parsed any method */
 		while ((arg = argv_next(&ac, &av)) != NULL) {
 			if (strcmp(arg, "any") == 0) {
-				if (options->num_auth_methods > 0) {
+				if (nstrs > 0) {
 					fatal("%s line %d: \"any\" must "
 					    "appear alone in %s",
 					    filename, linenum, keyword);
@@ -2435,17 +2480,19 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 				fatal("%s line %d: invalid %s method list.",
 				    filename, linenum, keyword);
 			}
-			value2 = 1;
-			if (!found || !*activep)
-				continue;
 			opt_array_append(filename, linenum, keyword,
-			    &options->auth_methods,
-			    &options->num_auth_methods, arg);
+			    &strs, &nstrs, arg);
 		}
-		if (value2 == 0) {
+		if (nstrs == 0) {
 			fatal("%s line %d: no %s specified",
 			    filename, linenum, keyword);
 		}
+		if (found && *activep) {
+			options->auth_methods = strs;
+			options->num_auth_methods = nstrs;
+			strs = NULL; /* transferred */
+			nstrs = 0;
+		}
 		break;
 
 	case sStreamLocalBindMask:
@@ -2505,12 +2552,11 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 		goto parse_int;
 
 	case sChannelTimeout:
-		uvalue = options->num_channel_timeouts;
-		i = 0;
+		found = options->num_channel_timeouts == 0;
 		while ((arg = argv_next(&ac, &av)) != NULL) {
 			/* Allow "none" only in first position */
 			if (strcasecmp(arg, "none") == 0) {
-				if (i > 0 || ac > 0) {
+				if (nstrs > 0 || ac > 0) {
 					error("%s line %d: keyword %s \"none\" "
 					    "argument must appear alone.",
 					    filename, linenum, keyword);
@@ -2521,11 +2567,18 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 				fatal("%s line %d: invalid channel timeout %s",
 				    filename, linenum, arg);
 			}
-			if (!*activep || uvalue != 0)
-				continue;
 			opt_array_append(filename, linenum, keyword,
-			    &options->channel_timeouts,
-			    &options->num_channel_timeouts, arg);
+			    &strs, &nstrs, arg);
+		}
+		if (nstrs == 0) {
+			fatal("%s line %d: no %s specified",
+			    filename, linenum, keyword);
+		}
+		if (found && *activep) {
+			options->channel_timeouts = strs;
+			options->num_channel_timeouts = nstrs;
+			strs = NULL; /* transferred */
+			nstrs = 0;
 		}
 		break;
 
@@ -2565,6 +2618,7 @@ process_server_config_line_depth(ServerOptions *options, char *line,
 	/* success */
 	ret = 0;
  out:
+	opt_array_free2(strs, NULL, nstrs);
 	argv_free(oav, oac);
 	return ret;
 }