From e1f28ec908e9c2cda74875e851e3cecfd136023f Mon Sep 17 00:00:00 2001 From: jsing Date: Mon, 15 Aug 2016 13:48:24 +0000 Subject: [PATCH] Make httpd stricter with respect to TLS configuration - in particular, do not allow TLS and non-TLS to be configured on the same port, do not allow TLS options to be specified without a TLS listener and ensure that the TLS options are the same when a server is specified on the same address/port. Currently, these configurations are permitted but do not work as intended. Also factor out and reuse the server matching code, which was previously duplicated. ok reyk@ --- usr.sbin/httpd/httpd.h | 3 +- usr.sbin/httpd/parse.y | 109 +++++++++++++++++++++++++++------------- usr.sbin/httpd/server.c | 26 +++++++++- 3 files changed, 101 insertions(+), 37 deletions(-) diff --git a/usr.sbin/httpd/httpd.h b/usr.sbin/httpd/httpd.h index aa9df949383..cbe5dcb8bd2 100644 --- a/usr.sbin/httpd/httpd.h +++ b/usr.sbin/httpd/httpd.h @@ -1,4 +1,4 @@ -/* $OpenBSD: httpd.h,v 1.104 2016/07/13 16:35:47 jsing Exp $ */ +/* $OpenBSD: httpd.h,v 1.105 2016/08/15 13:48:24 jsing Exp $ */ /* * Copyright (c) 2006 - 2015 Reyk Floeter @@ -526,6 +526,7 @@ int cmdline_symset(char *); /* server.c */ pid_t server(struct privsep *, struct privsep_proc *); +int server_tls_cmp(struct server *, struct server *); int server_tls_load_keypair(struct server *); int server_privinit(struct server *); void server_purge(struct server *); diff --git a/usr.sbin/httpd/parse.y b/usr.sbin/httpd/parse.y index 2995f8908fd..f7433f3fa41 100644 --- a/usr.sbin/httpd/parse.y +++ b/usr.sbin/httpd/parse.y @@ -1,4 +1,4 @@ -/* $OpenBSD: parse.y,v 1.78 2016/06/21 21:35:24 benno Exp $ */ +/* $OpenBSD: parse.y,v 1.79 2016/08/15 13:48:24 jsing Exp $ */ /* * Copyright (c) 2007 - 2015 Reyk Floeter @@ -107,6 +107,7 @@ int host_if(const char *, struct addresslist *, int host(const char *, struct addresslist *, int, struct portrange *, const char *, int); void host_free(struct addresslist *); +struct server *server_match(struct server *, int); struct server *server_inherit(struct server *, struct server_config *, struct server_config *); int getservice(char *); @@ -283,24 +284,13 @@ server : SERVER optmatch STRING { TAILQ_INSERT_TAIL(&srv->srv_hosts, srv_conf, entry); } '{' optnl serveropts_l '}' { - struct server *s = NULL, *sn; + struct server *s, *sn; struct server_config *a, *b; srv_conf = &srv->srv_conf; - TAILQ_FOREACH(s, conf->sc_servers, srv_entry) { - if ((s->srv_conf.flags & - SRVFLAG_LOCATION) == 0 && - strcmp(s->srv_conf.name, - srv->srv_conf.name) == 0 && - s->srv_conf.port == srv->srv_conf.port && - sockaddr_cmp( - (struct sockaddr *)&s->srv_conf.ss, - (struct sockaddr *)&srv->srv_conf.ss, - s->srv_conf.prefixlen) == 0) - break; - } - if (s != NULL) { + /* Check if the new server already exists. */ + if (server_match(srv, 1) != NULL) { yyerror("server \"%s\" defined twice", srv->srv_conf.name); serverconfig_free(srv_conf); @@ -315,16 +305,39 @@ server : SERVER optmatch STRING { YYERROR; } + if ((s = server_match(srv, 0)) != NULL) { + if ((s->srv_conf.flags & SRVFLAG_TLS) != + (srv->srv_conf.flags & SRVFLAG_TLS)) { + yyerror("server \"%s\": tls and " + "non-tls on same address/port", + srv->srv_conf.name); + serverconfig_free(srv_conf); + free(srv); + YYERROR; + } + if (server_tls_cmp(s, srv) != 0) { + yyerror("server \"%s\": tls " + "configuration mismatch on same " + "address/port", + srv->srv_conf.name); + serverconfig_free(srv_conf); + free(srv); + YYERROR; + } + } + if ((srv->srv_conf.flags & SRVFLAG_TLS) && srv->srv_conf.tls_protocols == 0) { - yyerror("no TLS protocols"); + yyerror("server \"%s\": no tls protocols", + srv->srv_conf.name); + serverconfig_free(srv_conf); free(srv); YYERROR; } if (server_tls_load_keypair(srv) == -1) { - yyerror("failed to load public/private keys " - "for server %s", srv->srv_conf.name); + yyerror("server \"%s\": failed to load " + "public/private keys", srv->srv_conf.name); serverconfig_free(srv_conf); free(srv); YYERROR; @@ -398,7 +411,7 @@ serveroptsl : LISTEN ON STRING opttls port { sizeof(*alias))) == NULL) fatal("out of memory"); - /* Add as an alias */ + /* Add as an IP-based alias. */ s_conf = alias; } else s_conf = &srv->srv_conf; @@ -416,9 +429,8 @@ serveroptsl : LISTEN ON STRING opttls port { s_conf->prefixlen = h->prefixlen; host_free(&al); - if ($4) { + if ($4) s_conf->flags |= SRVFLAG_TLS; - } if (alias != NULL) { /* IP-based; use name match flags from parent */ @@ -468,10 +480,22 @@ serveroptsl : LISTEN ON STRING opttls port { } } | tls { + struct server_config *sc; + int tls_flag = 0; + if (parentsrv != NULL) { yyerror("tls configuration inside location"); YYERROR; } + + /* Ensure that at least one server has TLS enabled. */ + TAILQ_FOREACH(sc, &srv->srv_hosts, entry) { + tls_flag |= (sc->flags & SRVFLAG_TLS); + } + if (tls_flag == 0) { + yyerror("tls options without tls listener"); + YYERROR; + } } | root | directory @@ -716,7 +740,7 @@ tlsopts : CERTIFICATE STRING { | PROTOCOLS STRING { if (tls_config_parse_protocols( &srv_conf->tls_protocols, $2) != 0) { - yyerror("invalid TLS protocols"); + yyerror("invalid tls protocols"); free($2); YYERROR; } @@ -1967,6 +1991,33 @@ host_free(struct addresslist *al) } } +struct server * +server_match(struct server *s2, int match_name) +{ + struct server *s1; + + /* Attempt to find matching server. */ + TAILQ_FOREACH(s1, conf->sc_servers, srv_entry) { + if ((s1->srv_conf.flags & SRVFLAG_LOCATION) != 0) + continue; + if (match_name) { + if (strcmp(s1->srv_conf.name, s2->srv_conf.name) != 0) + continue; + } + if (s1->srv_conf.port != s2->srv_conf.port) + continue; + if (sockaddr_cmp( + (struct sockaddr *)&s1->srv_conf.ss, + (struct sockaddr *)&s2->srv_conf.ss, + s1->srv_conf.prefixlen) != 0) + continue; + + return (s1); + } + + return (NULL); +} + struct server * server_inherit(struct server *src, struct server_config *alias, struct server_config *addr) @@ -2028,19 +2079,7 @@ server_inherit(struct server *src, struct server_config *alias, } /* Check if the new server already exists */ - TAILQ_FOREACH(s, conf->sc_servers, srv_entry) { - if ((s->srv_conf.flags & - SRVFLAG_LOCATION) == 0 && - strcmp(s->srv_conf.name, - dst->srv_conf.name) == 0 && - s->srv_conf.port == dst->srv_conf.port && - sockaddr_cmp( - (struct sockaddr *)&s->srv_conf.ss, - (struct sockaddr *)&dst->srv_conf.ss, - s->srv_conf.prefixlen) == 0) - break; - } - if (s != NULL) { + if (server_match(dst, 1) != NULL) { yyerror("server \"%s\" defined twice", dst->srv_conf.name); serverconfig_free(&dst->srv_conf); diff --git a/usr.sbin/httpd/server.c b/usr.sbin/httpd/server.c index db3b16d849a..7404d4960bb 100644 --- a/usr.sbin/httpd/server.c +++ b/usr.sbin/httpd/server.c @@ -1,4 +1,4 @@ -/* $OpenBSD: server.c,v 1.85 2016/04/28 17:18:06 jsing Exp $ */ +/* $OpenBSD: server.c,v 1.86 2016/08/15 13:48:24 jsing Exp $ */ /* * Copyright (c) 2006 - 2015 Reyk Floeter @@ -132,6 +132,30 @@ server_privinit(struct server *srv) return (0); } +int +server_tls_cmp(struct server *s1, struct server *s2) +{ + struct server_config *sc1, *sc2; + + sc1 = &s1->srv_conf; + sc2 = &s2->srv_conf; + + if (sc1->tls_protocols != sc2->tls_protocols) + return (-1); + if (strcmp(sc1->tls_cert_file, sc2->tls_cert_file) != 0) + return (-1); + if (strcmp(sc1->tls_key_file, sc2->tls_key_file) != 0) + return (-1); + if (strcmp(sc1->tls_ciphers, sc2->tls_ciphers) != 0) + return (-1); + if (strcmp(sc1->tls_dhe_params, sc2->tls_dhe_params) != 0) + return (-1); + if (strcmp(sc1->tls_ecdhe_curve, sc2->tls_ecdhe_curve) != 0) + return (-1); + + return (0); +} + int server_tls_load_keypair(struct server *srv) { -- 2.20.1