On Sat, Aug 13, 2016 at 02:57:14AM +1000, Joel Sing wrote:
> The following diff makes httpd stricter with respect to TLS configuration:
>
> - 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.
> - Ensure that 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.
>
> This also factors out (and reuses) the server matching code that was already
> duplicated.
>
> ok?
>
- I think server_match() and server_tls_cmp() can both live in
server.c (server_match() somewhere close to server_foreach() - this
match function can be used for at least one other case outside of
parse.y).
- As discussed before, for consistency with the config, please use
"tls" instead of "TLS" in the log messages.
FYI, The SNI diff doesn't like the tls_cert_file and tls_key_file
checks in server_tls_cmp(), as they now become valid, but they can be
removed/changed later.
Otherwise OK reyk@
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/httpd/parse.y,v
> retrieving revision 1.78
> diff -u -p -r1.78 parse.y
> --- parse.y 21 Jun 2016 21:35:24 -0000 1.78
> +++ parse.y 12 Aug 2016 16:48:28 -0000
> @@ -107,8 +107,10 @@ int host_if(const char *, struct addre
> 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 server_tls_cmp(struct server *, struct server *);
> int getservice(char *);
> int is_if_in_group(const char *, const char *);
>
> @@ -283,24 +285,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 +306,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 +412,7 @@ serveroptsl : LISTEN ON STRING opttls po
> 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 +430,8 @@ serveroptsl : LISTEN ON STRING opttls po
> 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 +481,22 @@ serveroptsl : LISTEN ON STRING opttls po
> }
> }
> | 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
> @@ -1968,6 +1993,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 +2080,7 @@ server_inherit(struct server *src, struc
> }
>
> /* 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);
> @@ -2078,6 +2118,30 @@ server_inherit(struct server *src, struc
> }
>
> return (dst);
> +}
> +
> +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
>
--
