From 65a44a8a4f7d902a64d4e60eda84384b2e2a24a2 Mon Sep 17 00:00:00 2001 From: "djm@openbsd.org" Date: Mon, 4 Mar 2024 02:16:11 +0000 Subject: [PATCH] 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 --- misc.c | 15 ++++- misc.h | 3 +- readconf.c | 147 +++++++++++++++++++++++++++-------------------- readconf.h | 6 +- servconf.c | 166 +++++++++++++++++++++++++++++++++++------------------ 5 files changed, 215 insertions(+), 122 deletions(-) diff --git a/misc.c b/misc.c index 3db2e4d0b..5dc9d54a2 100644 --- a/misc.c +++ b/misc.c @@ -1,4 +1,4 @@ -/* $OpenBSD: misc.c,v 1.189 2023/10/12 03:36:32 djm Exp $ */ +/* $OpenBSD: misc.c,v 1.190 2024/03/04 02:16:11 djm Exp $ */ /* * Copyright (c) 2000 Markus Friedl. All rights reserved. * Copyright (c) 2005-2020 Damien Miller. All rights reserved. @@ -2644,6 +2644,19 @@ opt_array_append(const char *file, const int line, const char *directive, opt_array_append2(file, line, directive, array, NULL, lp, s, 0); } +void +opt_array_free2(char **array, int **iarray, u_int l) +{ + u_int i; + + if (array == NULL || l == 0) + return; + for (i = 0; i < l; i++) + free(array[i]); + free(array); + free(iarray); +} + sshsig_t ssh_signal(int signum, sshsig_t handler) { diff --git a/misc.h b/misc.h index 74c6f832c..9bacce520 100644 --- a/misc.h +++ b/misc.h @@ -1,4 +1,4 @@ -/* $OpenBSD: misc.h,v 1.106 2023/10/11 22:42:26 djm Exp $ */ +/* $OpenBSD: misc.h,v 1.107 2024/03/04 02:16:11 djm Exp $ */ /* * Author: Tatu Ylonen @@ -210,6 +210,7 @@ void opt_array_append(const char *file, const int line, void opt_array_append2(const char *file, const int line, const char *directive, char ***array, int **iarray, u_int *lp, const char *s, int i); +void opt_array_free2(char **array, int **iarray, u_int l); struct timespec; void ptimeout_init(struct timespec *pt); diff --git a/readconf.c b/readconf.c index 7b3754283..804fcca2f 100644 --- a/readconf.c +++ b/readconf.c @@ -1,4 +1,4 @@ -/* $OpenBSD: readconf.c,v 1.384 2024/01/11 01:45:36 djm Exp $ */ +/* $OpenBSD: readconf.c,v 1.385 2024/03/04 02:16:11 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -1032,21 +1032,24 @@ process_config_line_depth(Options *options, struct passwd *pw, const char *host, { char *str, **charptr, *endofnumber, *keyword, *arg, *arg2, *p; char **cpptr, ***cppptr, fwdarg[256]; - u_int i, *uintptr, uvalue, max_entries = 0; + u_int i, *uintptr, max_entries = 0; int r, oactive, negated, opcode, *intptr, value, value2, cmdline = 0; - int remotefwd, dynamicfwd, ca_only = 0; + int remotefwd, dynamicfwd, ca_only = 0, found = 0; LogLevel *log_level_ptr; SyslogFacility *log_facility_ptr; long long val64; size_t len; struct Forward fwd; const struct multistate *multistate_ptr; - struct allowed_cname *cname; glob_t gl; const char *errstr; char **oav = NULL, **av; int oac = 0, ac; int ret = -1; + struct allowed_cname *cnames = NULL; + u_int ncnames = 0; + char **strs = NULL; /* string array arguments; freed implicitly */ + u_int nstrs = 0; if (activep == NULL) { /* We are processing a command line directive */ cmdline = 1; @@ -1662,14 +1665,13 @@ parse_pubkey_algos: case oPermitRemoteOpen: uintptr = &options->num_permitted_remote_opens; cppptr = &options->permitted_remote_opens; - uvalue = *uintptr; /* modified later */ - i = 0; + found = *uintptr == 0; while ((arg = argv_next(&ac, &av)) != NULL) { arg2 = xstrdup(arg); /* Allow any/none only in first position */ if (strcasecmp(arg, "none") == 0 || strcasecmp(arg, "any") == 0) { - if (i > 0 || ac > 0) { + if (nstrs > 0 || ac > 0) { error("%s line %d: keyword %s \"%s\" " "argument must appear alone.", filename, linenum, keyword, arg); @@ -1695,17 +1697,20 @@ parse_pubkey_algos: lookup_opcode_name(opcode)); } } - if (*activep && uvalue == 0) { - opt_array_append(filename, linenum, - lookup_opcode_name(opcode), - cppptr, uintptr, arg2); - } + opt_array_append(filename, linenum, + lookup_opcode_name(opcode), + &strs, &nstrs, arg2); free(arg2); - i++; } - if (i == 0) + if (nstrs == 0) fatal("%s line %d: missing %s specification", filename, linenum, lookup_opcode_name(opcode)); + if (found && *activep) { + *cppptr = strs; + *uintptr = nstrs; + strs = NULL; /* transferred */ + nstrs = 0; + } break; case oClearAllForwardings: @@ -1823,12 +1828,14 @@ parse_pubkey_algos: goto parse_int; case oSendEnv: + /* XXX appends to list; doesn't respect first-match-wins */ while ((arg = argv_next(&ac, &av)) != NULL) { if (*arg == '\0' || strchr(arg, '=') != NULL) { error("%s line %d: Invalid environment name.", filename, linenum); goto out; } + found = 1; if (!*activep) continue; if (*arg == '-') { @@ -1840,27 +1847,38 @@ parse_pubkey_algos: lookup_opcode_name(opcode), &options->send_env, &options->num_send_env, arg); } + if (!found) { + fatal("%s line %d: no %s specified", + filename, linenum, keyword); + } break; case oSetEnv: - value = options->num_setenv; + found = options->num_setenv == 0; while ((arg = argv_next(&ac, &av)) != NULL) { if (strchr(arg, '=') == NULL) { error("%s line %d: Invalid SetEnv.", filename, linenum); goto out; } - if (!*activep || value != 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, lookup_opcode_name(opcode), - &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; @@ -2069,52 +2087,46 @@ parse_pubkey_algos: goto parse_flag; case oCanonicalDomains: - value = options->num_canonical_domains != 0; - i = 0; + found = options->num_canonical_domains == 0; while ((arg = argv_next(&ac, &av)) != NULL) { - if (*arg == '\0') { - error("%s line %d: keyword %s empty argument", - filename, linenum, keyword); - goto out; - } /* 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 (!valid_domain(arg, 1, &errstr)) { error("%s line %d: %s", filename, linenum, errstr); goto out; } - if (!*activep || value) - continue; - if (options->num_canonical_domains >= - MAX_CANON_DOMAINS) { - error("%s line %d: too many hostname suffixes.", - filename, linenum); - goto out; - } - options->canonical_domains[ - options->num_canonical_domains++] = xstrdup(arg); + opt_array_append(filename, linenum, keyword, + &strs, &nstrs, arg); + } + if (nstrs == 0) { + fatal("%s line %d: no %s specified", + filename, linenum, keyword); + } + if (found && *activep) { + options->canonical_domains = strs; + options->num_canonical_domains = nstrs; + strs = NULL; /* transferred */ + nstrs = 0; } break; case oCanonicalizePermittedCNAMEs: - value = options->num_permitted_cnames != 0; - i = 0; + found = options->num_permitted_cnames == 0; while ((arg = argv_next(&ac, &av)) != NULL) { /* * Either 'none' (only in first position), '*' for * everything or 'list:list' */ if (strcasecmp(arg, "none") == 0) { - if (i > 0 || ac > 0) { + if (ncnames > 0 || ac > 0) { error("%s line %d: keyword %s \"none\" " "argument must appear alone.", filename, linenum, keyword); @@ -2135,19 +2147,25 @@ parse_pubkey_algos: *arg2 = '\0'; arg2++; } - i++; - if (!*activep || value) - continue; - if (options->num_permitted_cnames >= - MAX_CANON_DOMAINS) { - error("%s line %d: too many permitted CNAMEs.", - filename, linenum); - goto out; + cnames = xrecallocarray(cnames, ncnames, ncnames + 1, + sizeof(*cnames)); + cnames[ncnames].source_list = xstrdup(arg); + cnames[ncnames].target_list = xstrdup(arg2); + ncnames++; + } + if (ncnames == 0) { + fatal("%s line %d: no %s specified", + filename, linenum, keyword); + } + if (found && *activep) { + options->permitted_cnames = cnames; + options->num_permitted_cnames = ncnames; + } else { + for (i = 0; i < ncnames; i++) { + free(cnames[i].source_list); + free(cnames[i].target_list); } - cname = options->permitted_cnames + - options->num_permitted_cnames++; - cname->source_list = xstrdup(arg); - cname->target_list = xstrdup(arg2); + free(cnames); } break; @@ -2329,12 +2347,11 @@ parse_pubkey_algos: break; case oChannelTimeout: - 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); @@ -2345,11 +2362,18 @@ parse_pubkey_algos: 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; @@ -2381,6 +2405,7 @@ parse_pubkey_algos: /* success */ ret = 0; out: + opt_array_free2(strs, NULL, nstrs); argv_free(oav, oac); return ret; } diff --git a/readconf.h b/readconf.h index b18536ab9..9447d5d6e 100644 --- a/readconf.h +++ b/readconf.h @@ -1,4 +1,4 @@ -/* $OpenBSD: readconf.h,v 1.155 2024/01/11 01:45:36 djm Exp $ */ +/* $OpenBSD: readconf.h,v 1.156 2024/03/04 02:16:11 djm Exp $ */ /* * Author: Tatu Ylonen @@ -155,12 +155,12 @@ typedef struct { int proxy_use_fdpass; int num_canonical_domains; - char *canonical_domains[MAX_CANON_DOMAINS]; + char **canonical_domains; int canonicalize_hostname; int canonicalize_max_dots; int canonicalize_fallback_local; int num_permitted_cnames; - struct allowed_cname permitted_cnames[MAX_CANON_DOMAINS]; + struct allowed_cname *permitted_cnames; char *revoked_host_keys; 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 , 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; }