From: Remi Tricot-Le Breton Date: Tue, 23 Mar 2021 15:41:53 +0000 (+0100) Subject: BUG/MINOR: ssl: Prevent disk access when using "add ssl crt-list" X-Git-Tag: v2.4-dev14~44 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fb00f31af4ba67c69a12807729514a2bdcd47efa;p=thirdparty%2Fhaproxy.git BUG/MINOR: ssl: Prevent disk access when using "add ssl crt-list" If an unknown CA file was first mentioned in an "add ssl crt-list" CLI command, it would result in a call to X509_STORE_load_locations which performs a disk access which is forbidden during runtime. The same would happen if a "ca-verify-file" or "crl-file" was specified. This was due to the fact that the crt-list file parsing and the crt-list related CLI commands parsing use the same functions. The patch simply adds a new parameter to all the ssl_bind parsing functions so that they know if the call is made during init or by the CLI, and the ssl_store_load_locations function can then reject any new cafile_entry creation coming from a CLI call. It can be backported as far as 2.2. --- diff --git a/include/haproxy/listener-t.h b/include/haproxy/listener-t.h index 7d3998eceb..b9c1290851 100644 --- a/include/haproxy/listener-t.h +++ b/include/haproxy/listener-t.h @@ -252,7 +252,7 @@ struct bind_kw { }; struct ssl_bind_kw { const char *kw; - int (*parse)(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err); + int (*parse)(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err); int skip; /* nb of args to skip */ }; diff --git a/include/haproxy/ssl_crtlist.h b/include/haproxy/ssl_crtlist.h index 13ac702367..961cfc385e 100644 --- a/include/haproxy/ssl_crtlist.h +++ b/include/haproxy/ssl_crtlist.h @@ -38,7 +38,7 @@ void crtlist_free(struct crtlist *crtlist); struct crtlist *crtlist_new(const char *filename, int unique); /* file loading */ -int crtlist_parse_line(char *line, char **crt_path, struct crtlist_entry *entry, const char *file, int linenum, char **err); +int crtlist_parse_line(char *line, char **crt_path, struct crtlist_entry *entry, const char *file, int linenum, int from_cli, char **err); int crtlist_parse_file(char *file, struct bind_conf *bind_conf, struct proxy *curproxy, struct crtlist **crtlist, char **err); int crtlist_load_cert_dir(char *path, struct bind_conf *bind_conf, struct crtlist **crtlist, char **err); diff --git a/include/haproxy/ssl_sock.h b/include/haproxy/ssl_sock.h index db6d8387e3..c68425a29d 100644 --- a/include/haproxy/ssl_sock.h +++ b/include/haproxy/ssl_sock.h @@ -120,7 +120,7 @@ int ssl_sock_load_srv_cert(char *path, struct server *server, char **err); void ssl_free_global_issuers(void); int ssl_sock_load_cert_list_file(char *file, int dir, struct bind_conf *bind_conf, struct proxy *curproxy, char **err); int ssl_init_single_engine(const char *engine_id, const char *def_algorithms); -int ssl_store_load_locations_file(char *path); +int ssl_store_load_locations_file(char *path, int create_if_none); /* ssl shctx macro */ diff --git a/reg-tests/ssl/add_ssl_crt-list.vtc b/reg-tests/ssl/add_ssl_crt-list.vtc index 6d3308bb97..f42e3af538 100644 --- a/reg-tests/ssl/add_ssl_crt-list.vtc +++ b/reg-tests/ssl/add_ssl_crt-list.vtc @@ -93,3 +93,22 @@ client c1 -connect ${h1_clearlst_sock} { rxresp expect resp.status == 200 } -run + + +# Try to add a new line that mentions an "unknown" CA file (not loaded yet). +# It should fail since no disk access are allowed during runtime. +shell { + printf "add ssl crt-list ${testdir}/localhost.crt-list/ <<\n${testdir}/ecdsa.pem [ca-file ${testdir}/ca-auth.crt] localhost\n\n" | socat "${tmpdir}/h1/stats" - | grep "unable to load ${testdir}/ca-auth.crt" +} +shell { + printf "add ssl crt-list ${testdir}/localhost.crt-list/ <<\n${testdir}/ecdsa.pem [ca-verify-file ${testdir}/ca-auth.crt] localhost\n\n" | socat "${tmpdir}/h1/stats" - | grep "unable to load ${testdir}/ca-auth.crt" +} +shell { + printf "add ssl crt-list ${testdir}/localhost.crt-list/ <<\n${testdir}/ecdsa.pem [crl-file ${testdir}/ca-auth.crt] localhost\n\n" | socat "${tmpdir}/h1/stats" - | grep "unable to load ${testdir}/ca-auth.crt" +} + +# Check that the new line was not added to the crt-list. +haproxy h1 -cli { + send "show ssl crt-list ${testdir}/localhost.crt-list//" + expect !~ ".*ca-file ${testdir}/ca-auth.crt" +} diff --git a/src/cfgparse-ssl.c b/src/cfgparse-ssl.c index 4f05ce1c58..16cac6ec47 100644 --- a/src/cfgparse-ssl.c +++ b/src/cfgparse-ssl.c @@ -529,7 +529,7 @@ static int ssl_parse_global_extra_noext(char **args, int section_type, struct pr /***************************** Bind keyword Parsing ********************************************/ /* for ca-file and ca-verify-file */ -static int ssl_bind_parse_ca_file_common(char **args, int cur_arg, char **ca_file_p, char **err) +static int ssl_bind_parse_ca_file_common(char **args, int cur_arg, char **ca_file_p, int from_cli, char **err) { if (!*args[cur_arg + 1]) { memprintf(err, "'%s' : missing CAfile path", args[cur_arg]); @@ -541,7 +541,7 @@ static int ssl_bind_parse_ca_file_common(char **args, int cur_arg, char **ca_fil else memprintf(ca_file_p, "%s", args[cur_arg + 1]); - if (!ssl_store_load_locations_file(*ca_file_p)) { + if (!ssl_store_load_locations_file(*ca_file_p, !from_cli)) { memprintf(err, "'%s' : unable to load %s", args[cur_arg], *ca_file_p); return ERR_ALERT | ERR_FATAL; } @@ -549,23 +549,23 @@ static int ssl_bind_parse_ca_file_common(char **args, int cur_arg, char **ca_fil } /* parse the "ca-file" bind keyword */ -static int ssl_bind_parse_ca_file(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_ca_file(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { - return ssl_bind_parse_ca_file_common(args, cur_arg, &conf->ca_file, err); + return ssl_bind_parse_ca_file_common(args, cur_arg, &conf->ca_file, from_cli, err); } static int bind_parse_ca_file(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_ca_file(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_ca_file(args, cur_arg, px, &conf->ssl_conf, 0, err); } /* parse the "ca-verify-file" bind keyword */ -static int ssl_bind_parse_ca_verify_file(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_ca_verify_file(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { - return ssl_bind_parse_ca_file_common(args, cur_arg, &conf->ca_verify_file, err); + return ssl_bind_parse_ca_file_common(args, cur_arg, &conf->ca_verify_file, from_cli, err); } static int bind_parse_ca_verify_file(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_ca_verify_file(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_ca_verify_file(args, cur_arg, px, &conf->ssl_conf, 0, err); } /* parse the "ca-sign-file" bind keyword */ @@ -596,7 +596,7 @@ static int bind_parse_ca_sign_pass(char **args, int cur_arg, struct proxy *px, s } /* parse the "ciphers" bind keyword */ -static int ssl_bind_parse_ciphers(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_ciphers(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { if (!*args[cur_arg + 1]) { memprintf(err, "'%s' : missing cipher suite", args[cur_arg]); @@ -609,12 +609,12 @@ static int ssl_bind_parse_ciphers(char **args, int cur_arg, struct proxy *px, st } static int bind_parse_ciphers(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_ciphers(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_ciphers(args, cur_arg, px, &conf->ssl_conf, 0, err); } #ifdef HAVE_SSL_CTX_SET_CIPHERSUITES /* parse the "ciphersuites" bind keyword */ -static int ssl_bind_parse_ciphersuites(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_ciphersuites(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { if (!*args[cur_arg + 1]) { memprintf(err, "'%s' : missing cipher suite", args[cur_arg]); @@ -627,7 +627,7 @@ static int ssl_bind_parse_ciphersuites(char **args, int cur_arg, struct proxy *p } static int bind_parse_ciphersuites(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_ciphersuites(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_ciphersuites(args, cur_arg, px, &conf->ssl_conf, 0, err); } #endif @@ -671,7 +671,7 @@ static int bind_parse_crt_list(char **args, int cur_arg, struct proxy *px, struc } /* parse the "crl-file" bind keyword */ -static int ssl_bind_parse_crl_file(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_crl_file(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { #ifndef X509_V_FLAG_CRL_CHECK memprintf(err, "'%s' : library does not support CRL verify", args[cur_arg]); @@ -687,7 +687,7 @@ static int ssl_bind_parse_crl_file(char **args, int cur_arg, struct proxy *px, s else memprintf(&conf->crl_file, "%s", args[cur_arg + 1]); - if (!ssl_store_load_locations_file(conf->crl_file)) { + if (!ssl_store_load_locations_file(conf->crl_file, !from_cli)) { memprintf(err, "'%s' : unable to load %s", args[cur_arg], conf->crl_file); return ERR_ALERT | ERR_FATAL; } @@ -696,11 +696,11 @@ static int ssl_bind_parse_crl_file(char **args, int cur_arg, struct proxy *px, s } static int bind_parse_crl_file(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_crl_file(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_crl_file(args, cur_arg, px, &conf->ssl_conf, 0, err); } /* parse the "curves" bind keyword keyword */ -static int ssl_bind_parse_curves(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_curves(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { #if defined(SSL_CTX_set1_curves_list) if (!*args[cur_arg + 1]) { @@ -716,11 +716,11 @@ static int ssl_bind_parse_curves(char **args, int cur_arg, struct proxy *px, str } static int bind_parse_curves(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_curves(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_curves(args, cur_arg, px, &conf->ssl_conf, 0, err); } /* parse the "ecdhe" bind keyword keyword */ -static int ssl_bind_parse_ecdhe(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_ecdhe(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { #if HA_OPENSSL_VERSION_NUMBER < 0x0090800fL memprintf(err, "'%s' : library does not support elliptic curve Diffie-Hellman (too old)", args[cur_arg]); @@ -741,7 +741,7 @@ static int ssl_bind_parse_ecdhe(char **args, int cur_arg, struct proxy *px, stru } static int bind_parse_ecdhe(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_ecdhe(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_ecdhe(args, cur_arg, px, &conf->ssl_conf, 0, err); } /* parse the "crt-ignore-err" and "ca-ignore-err" bind keywords */ @@ -850,7 +850,7 @@ static int parse_tls_method_minmax(char **args, int cur_arg, struct tls_version_ return 0; } -static int ssl_bind_parse_tls_method_minmax(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_tls_method_minmax(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { int ret; @@ -884,7 +884,7 @@ static int bind_parse_no_tls_tickets(char **args, int cur_arg, struct proxy *px, } /* parse the "allow-0rtt" bind keyword */ -static int ssl_bind_parse_allow_0rtt(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_allow_0rtt(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { conf->early_data = 1; return 0; @@ -897,7 +897,7 @@ static int bind_parse_allow_0rtt(char **args, int cur_arg, struct proxy *px, str } /* parse the "npn" bind keyword */ -static int ssl_bind_parse_npn(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_npn(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { #if defined(OPENSSL_NPN_NEGOTIATED) && !defined(OPENSSL_NO_NEXTPROTONEG) char *p1, *p2; @@ -948,7 +948,7 @@ static int ssl_bind_parse_npn(char **args, int cur_arg, struct proxy *px, struct static int bind_parse_npn(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_npn(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_npn(args, cur_arg, px, &conf->ssl_conf, 0, err); } @@ -1014,7 +1014,7 @@ int ssl_sock_parse_alpn(char *arg, char **alpn_str, int *alpn_len, char **err) } /* parse the "alpn" bind keyword */ -static int ssl_bind_parse_alpn(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_alpn(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { #ifdef TLSEXT_TYPE_application_layer_protocol_negotiation int ret; @@ -1033,7 +1033,7 @@ static int ssl_bind_parse_alpn(char **args, int cur_arg, struct proxy *px, struc static int bind_parse_alpn(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_alpn(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_alpn(args, cur_arg, px, &conf->ssl_conf, 0, err); } /* parse the "ssl" bind keyword */ @@ -1202,7 +1202,7 @@ static int bind_parse_tls_ticket_keys(char **args, int cur_arg, struct proxy *px } /* parse the "verify" bind keyword */ -static int ssl_bind_parse_verify(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_verify(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { if (!*args[cur_arg + 1]) { memprintf(err, "'%s' : missing verify method", args[cur_arg]); @@ -1225,18 +1225,18 @@ static int ssl_bind_parse_verify(char **args, int cur_arg, struct proxy *px, str } static int bind_parse_verify(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_verify(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_verify(args, cur_arg, px, &conf->ssl_conf, 0, err); } /* parse the "no-ca-names" bind keyword */ -static int ssl_bind_parse_no_ca_names(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, char **err) +static int ssl_bind_parse_no_ca_names(char **args, int cur_arg, struct proxy *px, struct ssl_bind_conf *conf, int from_cli, char **err) { conf->no_ca_names = 1; return 0; } static int bind_parse_no_ca_names(char **args, int cur_arg, struct proxy *px, struct bind_conf *conf, char **err) { - return ssl_bind_parse_no_ca_names(args, cur_arg, px, &conf->ssl_conf, err); + return ssl_bind_parse_no_ca_names(args, cur_arg, px, &conf->ssl_conf, 0, err); } /***************************** "server" keywords Parsing ********************************************/ @@ -1334,7 +1334,7 @@ static int srv_parse_ca_file(char **args, int *cur_arg, struct proxy *px, struct else memprintf(&newsrv->ssl_ctx.ca_file, "%s", args[*cur_arg + 1]); - if (!ssl_store_load_locations_file(newsrv->ssl_ctx.ca_file)) { + if (!ssl_store_load_locations_file(newsrv->ssl_ctx.ca_file, 1)) { memprintf(err, "'%s' : unable to load %s", args[*cur_arg], newsrv->ssl_ctx.ca_file); return ERR_ALERT | ERR_FATAL; } @@ -1430,7 +1430,7 @@ static int srv_parse_crl_file(char **args, int *cur_arg, struct proxy *px, struc else memprintf(&newsrv->ssl_ctx.crl_file, "%s", args[*cur_arg + 1]); - if (!ssl_store_load_locations_file(newsrv->ssl_ctx.crl_file)) { + if (!ssl_store_load_locations_file(newsrv->ssl_ctx.crl_file, 1)) { memprintf(err, "'%s' : unable to load %s", args[*cur_arg], newsrv->ssl_ctx.crl_file); return ERR_ALERT | ERR_FATAL; } diff --git a/src/ssl_crtlist.c b/src/ssl_crtlist.c index 9ce7115e88..d0013dc4fc 100644 --- a/src/ssl_crtlist.c +++ b/src/ssl_crtlist.c @@ -302,7 +302,7 @@ struct crtlist *crtlist_new(const char *filename, int unique) * is a ptr in * Return an error code */ -int crtlist_parse_line(char *line, char **crt_path, struct crtlist_entry *entry, const char *file, int linenum, char **err) +int crtlist_parse_line(char *line, char **crt_path, struct crtlist_entry *entry, const char *file, int linenum, int from_cli, char **err) { int cfgerr = 0; int arg, newarg, cur_arg, i, ssl_b = 0, ssl_e = 0; @@ -386,13 +386,14 @@ int crtlist_parse_line(char *line, char **crt_path, struct crtlist_entry *entry, goto error; } } + cur_arg = ssl_b ? ssl_b : 1; while (cur_arg < ssl_e) { newarg = 0; for (i = 0; ssl_bind_kws[i].kw != NULL; i++) { if (strcmp(ssl_bind_kws[i].kw, args[cur_arg]) == 0) { newarg = 1; - cfgerr |= ssl_bind_kws[i].parse(args, cur_arg, NULL, ssl_conf, err); + cfgerr |= ssl_bind_kws[i].parse(args, cur_arg, NULL, ssl_conf, from_cli, err); if (cur_arg + 1 + ssl_bind_kws[i].skip > ssl_e) { memprintf(err, "parsing [%s:%d]: ssl args out of '[]' for %s", file, linenum, args[cur_arg]); @@ -502,7 +503,7 @@ int crtlist_parse_file(char *file, struct bind_conf *bind_conf, struct proxy *cu goto error; } - cfgerr |= crtlist_parse_line(thisline, &crt_path, entry, file, linenum, err); + cfgerr |= crtlist_parse_line(thisline, &crt_path, entry, file, linenum, 0, err); if (cfgerr & ERR_CODE) goto error; @@ -1206,7 +1207,7 @@ static int cli_parse_add_crtlist(char **args, char *payload, struct appctx *appc goto error; } /* cert_path is filled here */ - cfgerr |= crtlist_parse_line(payload, &cert_path, entry, "CLI", 1, &err); + cfgerr |= crtlist_parse_line(payload, &cert_path, entry, "CLI", 1, 1, &err); if (cfgerr & ERR_CODE) goto error; } else { diff --git a/src/ssl_sock.c b/src/ssl_sock.c index 527ec8001a..a86ed26c7c 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -340,11 +340,16 @@ static X509_STORE* ssl_store_get0_locations_file(char *path) return NULL; } -int ssl_store_load_locations_file(char *path) +int ssl_store_load_locations_file(char *path, int create_if_none) { - if (ssl_store_get0_locations_file(path) == NULL) { + X509_STORE *store = ssl_store_get0_locations_file(path); + + /* If this function is called by the CLI, we should not call the + * X509_STORE_load_locations function because it performs forbidden disk + * accesses. */ + if (!store && create_if_none) { struct cafile_entry *ca_e; - X509_STORE *store = X509_STORE_new(); + store = X509_STORE_new(); if (X509_STORE_load_locations(store, path, NULL)) { int pathlen; pathlen = strlen(path); @@ -353,13 +358,13 @@ int ssl_store_load_locations_file(char *path) memcpy(ca_e->path, path, pathlen + 1); ca_e->ca_store = store; ebst_insert(&cafile_tree, &ca_e->node); - return 1; } + } else { + X509_STORE_free(store); + store = NULL; } - X509_STORE_free(store); - return 0; } - return 1; + return (store != NULL); } /* mimic what X509_STORE_load_locations do with store_ctx */