From dceabc7ad7ebc7769c8214a1647af64c9a1d92e5 Mon Sep 17 00:00:00 2001 From: "djm@openbsd.org" Date: Thu, 5 Oct 2017 15:52:03 +0000 Subject: [PATCH] upstream commit replace statically-sized arrays in ServerOptions with dynamic ones managed by xrecallocarray, removing some arbitrary (though large) limits and saving a bit of memory; "much nicer" markus@ Upstream-ID: 1732720b2f478fe929d6687ac7b0a97ff2efe9d2 --- monitor.c | 4 +- monitor_wrap.c | 10 +-- servconf.c | 210 +++++++++++++++++++++++++++---------------------- sshd.c | 54 ++++++------- 4 files changed, 143 insertions(+), 135 deletions(-) diff --git a/monitor.c b/monitor.c index f517da482..a0ad9857d 100644 --- a/monitor.c +++ b/monitor.c @@ -1,4 +1,4 @@ -/* $OpenBSD: monitor.c,v 1.174 2017/10/02 19:33:20 djm Exp $ */ +/* $OpenBSD: monitor.c,v 1.175 2017/10/05 15:52:03 djm Exp $ */ /* * Copyright 2002 Niels Provos * Copyright 2002 Markus Friedl @@ -760,12 +760,10 @@ mm_answer_pwnamallow(int sock, Buffer *m) for (i = 0; i < options.nx; i++) \ buffer_put_cstring(m, options.x[i]); \ } while (0) -#define M_CP_STRARRAYOPT_ALLOC(x, nx) M_CP_STRARRAYOPT(x, nx) /* See comment in servconf.h */ COPY_MATCH_STRING_OPTS(); #undef M_CP_STROPT #undef M_CP_STRARRAYOPT -#undef M_CP_STRARRAYOPT_ALLOC /* Create valid auth method lists */ if (auth2_setup_methods_lists(authctxt) != 0) { diff --git a/monitor_wrap.c b/monitor_wrap.c index 69212aaf3..a46628fb1 100644 --- a/monitor_wrap.c +++ b/monitor_wrap.c @@ -1,4 +1,4 @@ -/* $OpenBSD: monitor_wrap.c,v 1.94 2017/10/02 19:33:20 djm Exp $ */ +/* $OpenBSD: monitor_wrap.c,v 1.95 2017/10/05 15:52:03 djm Exp $ */ /* * Copyright 2002 Niels Provos * Copyright 2002 Markus Friedl @@ -287,19 +287,15 @@ out: newopts->x = buffer_get_string(&m, NULL); \ } while (0) #define M_CP_STRARRAYOPT(x, nx) do { \ - for (i = 0; i < newopts->nx; i++) \ - newopts->x[i] = buffer_get_string(&m, NULL); \ - } while (0) -#define M_CP_STRARRAYOPT_ALLOC(x, nx) do { \ newopts->x = newopts->nx == 0 ? \ NULL : xcalloc(newopts->nx, sizeof(*newopts->x)); \ - M_CP_STRARRAYOPT(x, nx); \ + for (i = 0; i < newopts->nx; i++) \ + newopts->x[i] = buffer_get_string(&m, NULL); \ } while (0) /* See comment in servconf.h */ COPY_MATCH_STRING_OPTS(); #undef M_CP_STROPT #undef M_CP_STRARRAYOPT -#undef M_CP_STRARRAYOPT_ALLOC copy_set_server_options(&options, newopts, 1); log_change_level(options.log_level); diff --git a/servconf.c b/servconf.c index 956862959..a96df4f67 100644 --- a/servconf.c +++ b/servconf.c @@ -1,5 +1,5 @@ -/* $OpenBSD: servconf.c,v 1.313 2017/10/04 18:49:30 djm Exp $ */ +/* $OpenBSD: servconf.c,v 1.314 2017/10/05 15:52:03 djm Exp $ */ /* * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland * All rights reserved @@ -188,10 +188,45 @@ assemble_algorithms(ServerOptions *o) fatal("kex_assemble_names failed"); } +static void +array_append(const char *file, const int line, const char *directive, + char ***array, u_int *lp, const char *s) +{ + + if (*lp >= INT_MAX) + fatal("%s line %d: Too many %s entries", file, line, directive); + + *array = xrecallocarray(*array, *lp, *lp + 1, sizeof(**array)); + (*array)[*lp] = xstrdup(s); + (*lp)++; +} + +void +servconf_add_hostkey(const char *file, const int line, + ServerOptions *options, const char *path) +{ + char *apath = derelativise_path(path); + + array_append(file, line, "HostKey", + &options->host_key_files, &options->num_host_key_files, apath); + free(apath); +} + +void +servconf_add_hostcert(const char *file, const int line, + ServerOptions *options, const char *path) +{ + char *apath = derelativise_path(path); + + array_append(file, line, "HostCertificate", + &options->host_cert_files, &options->num_host_cert_files, apath); + free(apath); +} + void fill_default_server_options(ServerOptions *options) { - int i; + u_int i; /* Portable-specific options */ if (options->use_pam == -1) @@ -200,16 +235,16 @@ fill_default_server_options(ServerOptions *options) /* Standard Options */ if (options->num_host_key_files == 0) { /* fill default hostkeys for protocols */ - options->host_key_files[options->num_host_key_files++] = - _PATH_HOST_RSA_KEY_FILE; - options->host_key_files[options->num_host_key_files++] = - _PATH_HOST_DSA_KEY_FILE; + servconf_add_hostkey("[default]", 0, options, + _PATH_HOST_RSA_KEY_FILE); + servconf_add_hostkey("[default]", 0, options, + _PATH_HOST_DSA_KEY_FILE); #ifdef OPENSSL_HAS_ECC - options->host_key_files[options->num_host_key_files++] = - _PATH_HOST_ECDSA_KEY_FILE; + servconf_add_hostkey("[default]", 0, options, + _PATH_HOST_ECDSA_KEY_FILE); #endif - options->host_key_files[options->num_host_key_files++] = - _PATH_HOST_ED25519_KEY_FILE; + servconf_add_hostkey("[default]", 0, options, + _PATH_HOST_ED25519_KEY_FILE); } /* No certificates by default */ if (options->num_ports == 0) @@ -313,10 +348,14 @@ fill_default_server_options(ServerOptions *options) if (options->client_alive_count_max == -1) options->client_alive_count_max = 3; if (options->num_authkeys_files == 0) { - options->authorized_keys_files[options->num_authkeys_files++] = - xstrdup(_PATH_SSH_USER_PERMITTED_KEYS); - options->authorized_keys_files[options->num_authkeys_files++] = - xstrdup(_PATH_SSH_USER_PERMITTED_KEYS2); + array_append("[default]", 0, "AuthorizedKeysFiles", + &options->authorized_keys_files, + &options->num_authkeys_files, + _PATH_SSH_USER_PERMITTED_KEYS); + array_append("[default]", 0, "AuthorizedKeysFiles", + &options->authorized_keys_files, + &options->num_authkeys_files, + _PATH_SSH_USER_PERMITTED_KEYS2); } if (options->permit_tun == -1) options->permit_tun = SSH_TUNMODE_NO; @@ -1128,22 +1167,12 @@ process_server_config_line(ServerOptions *options, char *line, break; case sHostKeyFile: - intptr = &options->num_host_key_files; - if (*intptr >= MAX_HOSTKEYS) - fatal("%s line %d: too many host keys specified (max %d).", - filename, linenum, MAX_HOSTKEYS); - charptr = &options->host_key_files[*intptr]; - parse_filename: arg = strdelim(&cp); if (!arg || *arg == '\0') fatal("%s line %d: missing file name.", filename, linenum); - if (*activep && *charptr == NULL) { - *charptr = derelativise_path(arg); - /* increase optional counter */ - if (intptr != NULL) - *intptr = *intptr + 1; - } + if (*activep) + servconf_add_hostkey(filename, linenum, options, arg); break; case sHostKeyAgent: @@ -1158,17 +1187,28 @@ process_server_config_line(ServerOptions *options, char *line, break; case sHostCertificate: - intptr = &options->num_host_cert_files; - if (*intptr >= MAX_HOSTKEYS) - fatal("%s line %d: too many host certificates " - "specified (max %d).", filename, linenum, - MAX_HOSTCERTS); - charptr = &options->host_cert_files[*intptr]; - goto parse_filename; + arg = strdelim(&cp); + if (!arg || *arg == '\0') + fatal("%s line %d: missing file name.", + filename, linenum); + if (*activep) + servconf_add_hostcert(filename, linenum, options, arg); + break; case sPidFile: charptr = &options->pid_file; - goto parse_filename; + parse_filename: + arg = strdelim(&cp); + if (!arg || *arg == '\0') + fatal("%s line %d: missing file name.", + filename, linenum); + if (*activep && *charptr == NULL) { + *charptr = derelativise_path(arg); + /* increase optional counter */ + if (intptr != NULL) + *intptr = *intptr + 1; + } + break; case sPermitRootLogin: intptr = &options->permit_root_login; @@ -1412,55 +1452,47 @@ process_server_config_line(ServerOptions *options, char *line, case sAllowUsers: while ((arg = strdelim(&cp)) && *arg != '\0') { - if (options->num_allow_users >= MAX_ALLOW_USERS) - fatal("%s line %d: too many allow users.", - filename, linenum); if (match_user(NULL, NULL, NULL, arg) == -1) fatal("%s line %d: invalid AllowUsers pattern: " "\"%.100s\"", filename, linenum, arg); if (!*activep) continue; - options->allow_users[options->num_allow_users++] = - xstrdup(arg); + array_append(filename, linenum, "AllowUsers", + &options->allow_users, &options->num_allow_users, + arg); } break; case sDenyUsers: while ((arg = strdelim(&cp)) && *arg != '\0') { - if (options->num_deny_users >= MAX_DENY_USERS) - fatal("%s line %d: too many deny users.", - filename, linenum); if (match_user(NULL, NULL, NULL, arg) == -1) fatal("%s line %d: invalid DenyUsers pattern: " "\"%.100s\"", filename, linenum, arg); if (!*activep) continue; - options->deny_users[options->num_deny_users++] = - xstrdup(arg); + array_append(filename, linenum, "DenyUsers", + &options->deny_users, &options->num_deny_users, + arg); } break; case sAllowGroups: while ((arg = strdelim(&cp)) && *arg != '\0') { - if (options->num_allow_groups >= MAX_ALLOW_GROUPS) - fatal("%s line %d: too many allow groups.", - filename, linenum); if (!*activep) continue; - options->allow_groups[options->num_allow_groups++] = - xstrdup(arg); + array_append(filename, linenum, "AllowGroups", + &options->allow_groups, &options->num_allow_groups, + arg); } break; case sDenyGroups: while ((arg = strdelim(&cp)) && *arg != '\0') { - if (options->num_deny_groups >= MAX_DENY_GROUPS) - fatal("%s line %d: too many deny groups.", - filename, linenum); if (!*activep) continue; - options->deny_groups[options->num_deny_groups++] = - xstrdup(arg); + array_append(filename, linenum, "DenyGroups", + &options->deny_groups, &options->num_deny_groups, + arg); } break; @@ -1579,14 +1611,12 @@ process_server_config_line(ServerOptions *options, char *line, case sAuthorizedKeysFile: if (*activep && options->num_authkeys_files == 0) { while ((arg = strdelim(&cp)) && *arg != '\0') { - if (options->num_authkeys_files >= - MAX_AUTHKEYS_FILES) - fatal("%s line %d: " - "too many authorized keys files.", - filename, linenum); - options->authorized_keys_files[ - options->num_authkeys_files++] = - tilde_expand_filename(arg, getuid()); + arg = tilde_expand_filename(arg, getuid()); + array_append(filename, linenum, + "AuthorizedKeysFile", + &options->authorized_keys_files, + &options->num_authkeys_files, arg); + free(arg); } } return 0; @@ -1618,13 +1648,11 @@ process_server_config_line(ServerOptions *options, char *line, if (strchr(arg, '=') != NULL) fatal("%s line %d: Invalid environment name.", filename, linenum); - if (options->num_accept_env >= MAX_ACCEPT_ENV) - fatal("%s line %d: too many allow env.", - filename, linenum); if (!*activep) continue; - options->accept_env[options->num_accept_env++] = - xstrdup(arg); + array_append(filename, linenum, "AcceptEnv", + &options->accept_env, &options->num_accept_env, + arg); } break; @@ -1684,15 +1712,12 @@ process_server_config_line(ServerOptions *options, char *line, fatal("%s line %d: bad port number in " "PermitOpen", filename, linenum); if (*activep && value == 0) { - options->permitted_opens = xrecallocarray( - options->permitted_opens, - options->num_permitted_opens, - options->num_permitted_opens + 1, - sizeof(*options->permitted_opens)); - i = options->num_permitted_opens++; - options->permitted_opens[i] = arg2; - } else - free(arg2); + array_append(filename, linenum, + "PermitOpen", + &options->permitted_opens, + &options->num_permitted_opens, arg2); + } + free(arg2); } break; @@ -1815,11 +1840,6 @@ process_server_config_line(ServerOptions *options, char *line, value = 0; /* seen "any" pseudo-method */ value2 = 0; /* sucessfully parsed any method */ while ((arg = strdelim(&cp)) && *arg != '\0') { - if (options->num_auth_methods >= - MAX_AUTH_METHODS) - fatal("%s line %d: " - "too many authentication methods.", - filename, linenum); if (strcmp(arg, "any") == 0) { if (options->num_auth_methods > 0) { fatal("%s line %d: \"any\" " @@ -1840,8 +1860,10 @@ process_server_config_line(ServerOptions *options, char *line, value2 = 1; if (!*activep) continue; - options->auth_methods[ - options->num_auth_methods++] = xstrdup(arg); + array_append(filename, linenum, + "AuthenticationMethods", + &options->auth_methods, + &options->num_auth_methods, arg); } if (value2 == 0) { fatal("%s line %d: no AuthenticationMethods " @@ -2057,17 +2079,16 @@ copy_set_server_options(ServerOptions *dst, ServerOptions *src, int preauth) dst->n = src->n; \ } \ } while(0) -#define M_CP_STRARRAYOPT(n, num_n) do {\ - if (src->num_n != 0) { \ - for (dst->num_n = 0; dst->num_n < src->num_n; dst->num_n++) \ - dst->n[dst->num_n] = xstrdup(src->n[dst->num_n]); \ - } \ -} while(0) -#define M_CP_STRARRAYOPT_ALLOC(n, num_n) do { \ - if (src->num_n != 0) { \ - dst->n = xcalloc(src->num_n, sizeof(*dst->n)); \ - M_CP_STRARRAYOPT(n, num_n); \ - dst->num_n = src->num_n; \ +#define M_CP_STRARRAYOPT(s, num_s) do {\ + u_int i; \ + if (src->num_s != 0) { \ + for (i = 0; i < dst->num_s; i++) \ + free(dst->s[i]); \ + free(dst->s); \ + dst->s = xcalloc(src->num_s, sizeof(*dst->s)); \ + for (i = 0; i < src->num_s; i++) \ + dst->s[i] = xstrdup(src->s[i]); \ + dst->num_s = src->num_s; \ } \ } while(0) @@ -2100,7 +2121,6 @@ copy_set_server_options(ServerOptions *dst, ServerOptions *src, int preauth) #undef M_CP_INTOPT #undef M_CP_STROPT #undef M_CP_STRARRAYOPT -#undef M_CP_STRARRAYOPT_ALLOC void parse_server_config(ServerOptions *options, const char *filename, Buffer *conf, diff --git a/sshd.c b/sshd.c index 51a1aaf6e..2ff385552 100644 --- a/sshd.c +++ b/sshd.c @@ -1,4 +1,4 @@ -/* $OpenBSD: sshd.c,v 1.492 2017/09/12 06:32:07 djm Exp $ */ +/* $OpenBSD: sshd.c,v 1.493 2017/10/05 15:52:03 djm Exp $ */ /* * Author: Tatu Ylonen * Copyright (c) 1995 Tatu Ylonen , Espoo, Finland @@ -467,7 +467,7 @@ sshd_exchange_identification(struct ssh *ssh, int sock_in, int sock_out) void destroy_sensitive_data(void) { - int i; + u_int i; for (i = 0; i < options.num_host_key_files; i++) { if (sensitive_data.host_keys[i]) { @@ -486,7 +486,7 @@ void demote_sensitive_data(void) { struct sshkey *tmp; - int i; + u_int i; for (i = 0; i < options.num_host_key_files; i++) { if (sensitive_data.host_keys[i]) { @@ -685,7 +685,7 @@ list_hostkey_types(void) Buffer b; const char *p; char *ret; - int i; + u_int i; struct sshkey *key; buffer_init(&b); @@ -745,7 +745,7 @@ list_hostkey_types(void) static struct sshkey * get_hostkey_by_type(int type, int nid, int need_private, struct ssh *ssh) { - int i; + u_int i; struct sshkey *key; for (i = 0; i < options.num_host_key_files; i++) { @@ -785,7 +785,7 @@ get_hostkey_private_by_type(int type, int nid, struct ssh *ssh) struct sshkey * get_hostkey_by_index(int ind) { - if (ind < 0 || ind >= options.num_host_key_files) + if (ind < 0 || (u_int)ind >= options.num_host_key_files) return (NULL); return (sensitive_data.host_keys[ind]); } @@ -793,7 +793,7 @@ get_hostkey_by_index(int ind) struct sshkey * get_hostkey_public_by_index(int ind, struct ssh *ssh) { - if (ind < 0 || ind >= options.num_host_key_files) + if (ind < 0 || (u_int)ind >= options.num_host_key_files) return (NULL); return (sensitive_data.host_pubkeys[ind]); } @@ -801,7 +801,7 @@ get_hostkey_public_by_index(int ind, struct ssh *ssh) int get_hostkey_index(struct sshkey *key, int compare, struct ssh *ssh) { - int i; + u_int i; for (i = 0; i < options.num_host_key_files; i++) { if (key_is_cert(key)) { @@ -830,7 +830,8 @@ notify_hostkeys(struct ssh *ssh) { struct sshbuf *buf; struct sshkey *key; - int i, nkeys, r; + u_int i, nkeys; + int r; char *fp; /* Some clients cannot cope with the hostkeys message, skip those. */ @@ -861,7 +862,7 @@ notify_hostkeys(struct ssh *ssh) packet_put_string(sshbuf_ptr(buf), sshbuf_len(buf)); nkeys++; } - debug3("%s: sent %d hostkeys", __func__, nkeys); + debug3("%s: sent %u hostkeys", __func__, nkeys); if (nkeys == 0) fatal("%s: no hostkeys", __func__); packet_send(); @@ -1357,13 +1358,12 @@ main(int ac, char **av) struct ssh *ssh = NULL; extern char *optarg; extern int optind; - int r, opt, i, j, on = 1, already_daemon; + int r, opt, on = 1, already_daemon, remote_port; int sock_in = -1, sock_out = -1, newsock = -1; const char *remote_ip; - int remote_port; char *fp, *line, *laddr, *logfile = NULL; int config_s[2] = { -1 , -1 }; - u_int n; + u_int i, j; u_int64_t ibytes, obytes; mode_t new_umask; struct sshkey *key; @@ -1416,12 +1416,8 @@ main(int ac, char **av) config_file_name = optarg; break; case 'c': - if (options.num_host_cert_files >= MAX_HOSTCERTS) { - fprintf(stderr, "too many host certificates.\n"); - exit(1); - } - options.host_cert_files[options.num_host_cert_files++] = - derelativise_path(optarg); + servconf_add_hostcert("[command-line]", 0, + &options, optarg); break; case 'd': if (debug_flag == 0) { @@ -1480,12 +1476,8 @@ main(int ac, char **av) /* protocol 1, ignored */ break; case 'h': - if (options.num_host_key_files >= MAX_HOSTKEYS) { - fprintf(stderr, "too many host keys.\n"); - exit(1); - } - options.host_key_files[options.num_host_key_files++] = - derelativise_path(optarg); + servconf_add_hostkey("[command-line]", 0, + &options, optarg); break; case 't': test_flag = 1; @@ -1611,12 +1603,12 @@ main(int ac, char **av) * and warns for trivial misconfigurations that could break login. */ if (options.num_auth_methods != 0) { - for (n = 0; n < options.num_auth_methods; n++) { - if (auth2_methods_valid(options.auth_methods[n], + for (i = 0; i < options.num_auth_methods; i++) { + if (auth2_methods_valid(options.auth_methods[i], 1) == 0) break; } - if (n >= options.num_auth_methods) + if (i >= options.num_auth_methods) fatal("AuthenticationMethods cannot be satisfied by " "enabled authentication methods"); } @@ -1752,7 +1744,7 @@ main(int ac, char **av) continue; } sensitive_data.host_certificates[j] = key; - debug("host certificate: #%d type %d %s", j, key->type, + debug("host certificate: #%u type %d %s", j, key->type, key_type(key)); } @@ -1796,8 +1788,10 @@ main(int ac, char **av) debug("setgroups() failed: %.200s", strerror(errno)); if (rexec_flag) { + if (rexec_argc < 0) + fatal("rexec_argc %d < 0", rexec_argc); rexec_argv = xcalloc(rexec_argc + 2, sizeof(char *)); - for (i = 0; i < rexec_argc; i++) { + for (i = 0; i < (u_int)rexec_argc; i++) { debug("rexec_argv[%d]='%s'", i, saved_argv[i]); rexec_argv[i] = saved_argv[i]; }