Fix some issues in redir spec handling, discovered thanks to dlg testing

- purge irrelevant addresses from the lists before collapsing
- ensure the lists are freed after they're collapsed
- more careful ifname copying, avoiding double-free / use-after-free traps

2 files changed, 37 insertions, 27 deletions

diff --git a/sbin/pfctl/parse.y b/sbin/pfctl/parse.y
index b8203bfce55..aeecc8ad8e5 100644
--- a/sbin/pfctl/parse.y
+++ b/sbin/pfctl/parse.y
@@ -1,4 +1,4 @@
-/*	$OpenBSD: parse.y,v 1.582 2010/01/12 03:33:28 mcbride Exp $	*/
+/*	$OpenBSD: parse.y,v 1.583 2010/01/12 14:44:26 mcbride Exp $	*/
 
 /*
  * Copyright (c) 2001 Markus Friedl.  All rights reserved.
@@ -3763,8 +3763,16 @@ route_host	: STRING			{
 
 			$$ = $3;
 			/* XXX check masks, only full mask should be allowed */
-			for (n = $3; n != NULL; n = n->next)
-				$$->ifname = $2;
+			for (n = $3; n != NULL; n = n->next) {
+				if ($$->ifname) {
+					yyerror("cannot specify interface twice "
+					    "in route spec");
+					YYERROR;
+				}
+				if (($$->ifname = strdup($2)) == NULL)
+					errx(1, "host: strdup");
+			}
+			free($2);
 		}
 		;
 
@@ -4509,6 +4517,8 @@ collapse_redirspec(struct pf_pool *rpool, struct pf_rule *r, struct redirspec *r
 	}
 
 	h = rs->rdr->host;
+	if (r->af)
+		remove_invalid_hosts(&h, &r->af);
 	if (h == NULL)			/* no pool address */
 		return (0);
 	else if (h->next == NULL) {	/* only one address */
@@ -4549,6 +4559,8 @@ collapse_redirspec(struct pf_pool *rpool, struct pf_rule *r, struct redirspec *r
 			h = h->next;
                 }
 	}
+	freehostlist(h);
+	rs->rdr->host = NULL;
 	if (tbl) {
 		if ((pf->opts & PF_OPT_NOACTION) == 0 &&
 		     pf_opt_create_table(pf, tbl))
@@ -4581,9 +4593,6 @@ apply_redirspec(struct pf_pool *rpool, struct pf_rule *r, struct redirspec *rs,
 	if (!rs || !rs->rdr)
 		return (0);
 
-	if (check_netmask(rs->rdr->host, r->af))
-		return (1);
-
 	rpool->proxy_port[0] = ntohs(rs->rdr->rport.a);
 
 	if (isrdr) {
@@ -4602,10 +4611,8 @@ apply_redirspec(struct pf_pool *rpool, struct pf_rule *r, struct redirspec *rs,
 	}
 
 	rpool->opts = rs->pool_opts.type;
-	if ((rpool->opts & PF_POOL_TYPEMASK) == PF_POOL_NONE &&
-	    (rs->rdr->host->next != NULL ||
-	    rs->rdr->host->addr.type == PF_ADDR_TABLE ||
-	    DYNIF_MULTIADDR(rs->rdr->host->addr)))
+	if (rpool->addr.type == PF_ADDR_TABLE ||
+	    DYNIF_MULTIADDR(rpool->addr))
 		rpool->opts = PF_POOL_ROUNDROBIN;
 
 	if (rs->pool_opts.key != NULL)
@@ -4667,9 +4674,9 @@ expand_rule(struct pf_rule *r, int keeprule, struct node_if *interfaces,
 	flagset = r->flagset;
 	keep_state = r->keep_state;
 
-	collapse_redirspec(&r->rdr, r, rdr);
-	collapse_redirspec(&r->nat, r, nat);
-	collapse_redirspec(&r->route, r, rroute);
+	error += collapse_redirspec(&r->rdr, r, rdr);
+	error += collapse_redirspec(&r->nat, r, nat);
+	error += collapse_redirspec(&r->route, r, rroute);
 
 	r->src.addr.type = r->dst.addr.type = PF_ADDR_ADDRMASK;
 
@@ -4880,10 +4887,6 @@ expand_rule(struct pf_rule *r, int keeprule, struct node_if *interfaces,
 		FREE_LIST(struct node_uid, uids);
 		FREE_LIST(struct node_gid, gids);
 		FREE_LIST(struct node_icmp, icmp_types);
-		if (nat && nat->rdr)
-			FREE_LIST(struct node_host, nat->rdr->host);
-		if (rdr && rdr->rdr)
-			FREE_LIST(struct node_host, rdr->rdr->host);
 	}
 
 	if (!added)
@@ -4929,6 +4932,11 @@ expand_skip_interface(struct node_if *interfaces)
 void
 freehostlist(struct node_host *h)
 {
+	struct node_host *n;
+
+	for (n = h; n != NULL; n = n->next)
+		if (n->ifname)
+			free(n->ifname);
 	FREE_LIST(struct node_host, h);
 }
 
diff --git a/sbin/pfctl/pfctl_parser.c b/sbin/pfctl/pfctl_parser.c
index 09f6475d30f..4b56aa11ad8 100644
--- a/sbin/pfctl/pfctl_parser.c
+++ b/sbin/pfctl/pfctl_parser.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfctl_parser.c,v 1.256 2010/01/12 03:20:51 mcbride Exp $ */
+/*	$OpenBSD: pfctl_parser.c,v 1.257 2010/01/12 14:44:26 mcbride Exp $ */
 
 /*
  * Copyright (c) 2001 Daniel Hartmeier
@@ -1407,7 +1407,8 @@ host(const char *s)
 	if ((if_name = strrchr(ps, '@')) != NULL) {
 		if_name[0] = '\0';
 		if_name++;
-	} else if ((p = strrchr(ps, '/')) != NULL) {
+	} 
+	if ((p = strrchr(ps, '/')) != NULL) {
 		mask = strtol(p+1, &q, 0);
 		if (!q || *q || mask > 128 || q == (p+1)) {
 			fprintf(stderr, "invalid netmask '%s'\n", p);
@@ -1422,7 +1423,7 @@ host(const char *s)
 		cont = 0;
 
 	/* IPv4 address? */
-	if (cont && (h = host_v4(s, mask)) != NULL)
+	if (cont && (h = host_v4(ps, mask)) != NULL)
 		cont = 0;
 
 	/* IPv6 address? */
@@ -1432,18 +1433,19 @@ host(const char *s)
 	/* dns lookup */
 	if (cont && (h = host_dns(ps, v4mask, v6mask)) != NULL)
 		cont = 0;
-	free(ps);
 
+	if (if_name && if_name[0])
+		for (n = h; n != NULL; n = n->next)
+			if ((n->ifname = strdup(if_name)) == NULL)
+				err(1, "host: strdup");
+
+	free(ps);	/* after we copy the name out */
 	if (h == NULL || cont == 1) {
 		fprintf(stderr, "no IP address found for %s\n", s);
 		return (NULL);
 	}
-	if (if_name)
-		for (n = h; n != NULL; n = n->next)
-			if ((h->ifname = strdup(if_name)) == NULL)
-				err(1, "host: strdup");
-
-	h->addr.type = PF_ADDR_ADDRMASK;
+	for (n = h; n != NULL; n = n->next)
+		n->addr.type = PF_ADDR_ADDRMASK;
 	return (h);
 }