Commit Diff

Commit:
1ad6438137396bda84b2f0af346e86190b8b87f1
From:
reyk <reyk@openbsd.org>
Date:
Message:
carefully check some return values and make lint happier. never pass any truncated strings (table names/anchors/tags/...) to pf and the kernel. ok pyr@
Actions:
Patch | Tree
commit - f802d393d864dd33993acdeb9630fdb77859e192
commit + 1ad6438137396bda84b2f0af346e86190b8b87f1
blob - 158800305ebb991c7914716496654c43d6b3f9e6
blob + 905b0becbfdca60442dde7a3dadffd3d99a2db53
--- check_tcp.c
+++ check_tcp.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: check_tcp.c,v 1.21 2007/02/07 15:13:00 reyk Exp $	*/
+/*	$OpenBSD: check_tcp.c,v 1.22 2007/02/08 13:32:24 reyk Exp $	*/
 
 /*
  * Copyright (c) 2006 Pierre-Yves Ritschard <pyr@spootnik.org>
@@ -247,7 +247,8 @@ tcp_read_buf(int s, short event, void *arg)
 			    "tcp_read_buf: check failed");
 		return;
 	default:
-		buf_add(cte->buf, rbuf, br);
+		if (buf_add(cte->buf, rbuf, br) == -1)
+			fatal("tcp_read_buf: buf_add error");
 		if (cte->validate_read != NULL) {
 			if (cte->validate_read(cte) != 0)
 				goto retry;
@@ -326,7 +327,7 @@ check_http_code(struct ctl_tcp_event *cte)
 		host->up = HOST_DOWN;
 		return (1);
 	}
-	strlcpy(scode, head, sizeof(scode));
+	(void)strlcpy(scode, head, sizeof(scode));
 	code = strtonum(scode, 100, 999, &estr);
 	if (estr != NULL) {
 		log_debug("check_http_code: %s failed "
blob - 20e1cc04ff595fd980f420961389e4f8c1bcd1bd
blob + bfd318f45ea75a214aaca7859358c3ca9c847603
--- control.c
+++ control.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: control.c,v 1.12 2007/02/07 13:39:58 reyk Exp $	*/
+/*	$OpenBSD: control.c,v 1.13 2007/02/08 13:32:24 reyk Exp $	*/
 
 /*
  * Copyright (c) 2003, 2004 Henning Brauer <henning@openbsd.org>
@@ -56,7 +56,12 @@ control_init(void)
 
 	bzero(&sun, sizeof(sun));
 	sun.sun_family = AF_UNIX;
-	strlcpy(sun.sun_path, HOSTSTATED_SOCKET, sizeof(sun.sun_path));
+	if (strlcpy(sun.sun_path, HOSTSTATED_SOCKET,
+	    sizeof(sun.sun_path)) >= sizeof(sun.sun_path)) {
+		log_warn("control_init: %s name too long", HOSTSTATED_SOCKET);
+		close(fd);
+		return (-1);
+	}
 
 	if (unlink(HOSTSTATED_SOCKET) == -1)
 		if (errno != ENOENT) {
@@ -69,10 +74,10 @@ control_init(void)
 	if (bind(fd, (struct sockaddr *)&sun, sizeof(sun)) == -1) {
 		log_warn("control_init: bind: %s", HOSTSTATED_SOCKET);
 		close(fd);
-		umask(old_umask);
+		(void)umask(old_umask);
 		return (-1);
 	}
-	umask(old_umask);
+	(void)umask(old_umask);
 
 	if (chmod(HOSTSTATED_SOCKET, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP) == -1) {
 		log_warn("control_init: chmod");
@@ -106,8 +111,7 @@ control_listen(void)
 void
 control_cleanup(void)
 {
-
-	unlink(HOSTSTATED_SOCKET);
+	(void)unlink(HOSTSTATED_SOCKET);
 }
 
 /* ARGSUSED */
blob - fe81c4569aab98eb971b03e699d2ed99c092076d
blob + c5d53b444099f257dde98ca516434328787c1b77
--- parse.y
+++ parse.y
@@ -1,4 +1,4 @@
-/*	$OpenBSD: parse.y,v 1.23 2007/02/07 15:17:46 reyk Exp $	*/
+/*	$OpenBSD: parse.y,v 1.24 2007/02/08 13:32:24 reyk Exp $	*/
 
 /*
  * Copyright (c) 2006 Pierre-Yves Ritschard <pyr@spootnik.org>
@@ -412,8 +412,9 @@ tableoptsl	: host			{
 			}
 			table->check = CHECK_HTTP_CODE;
 			table->retcode = $5;
-			asprintf(&table->sendbuf, "HEAD %s HTTP/1.0\r\n\r\n",
-			    $3);
+			if (asprintf(&table->sendbuf,
+			    "HEAD %s HTTP/1.0\r\n\r\n", $3) == -1)
+				fatal("asprintf");
 			free($3);
 			if (table->sendbuf == NULL)
 				fatal("out of memory");
@@ -424,8 +425,9 @@ tableoptsl	: host			{
 				table->flags |= F_SSL;
 			}
 			table->check = CHECK_HTTP_DIGEST;
-			asprintf(&table->sendbuf, "GET %s HTTP/1.0\r\n\r\n",
-			    $3);
+			if (asprintf(&table->sendbuf,
+			    "GET %s HTTP/1.0\r\n\r\n", $3) == -1)
+				fatal("asprintf");
 			free($3);
 			if (table->sendbuf == NULL)
 				fatal("out of memory");
@@ -913,7 +915,8 @@ cmdline_symset(char *s)
 	if ((sym = malloc(len)) == NULL)
 		errx(1, "cmdline_symset: malloc");
 
-	strlcpy(sym, s, len);
+	if (strlcpy(sym, s, len) >= len)
+		errx(1, "cmdline_symset: macro too long");
 
 	ret = symset(sym, val + 1, 1);
 	free(sym);
blob - d02fbce1251d320ec3542fff822731281db03523
blob + 0af740996118c165ef5527bf9866f31833dd004a
--- pfe.c
+++ pfe.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfe.c,v 1.13 2007/02/06 11:21:35 pyr Exp $	*/
+/*	$OpenBSD: pfe.c,v 1.14 2007/02/08 13:32:24 reyk Exp $	*/
 
 /*
  * Copyright (c) 2006 Pierre-Yves Ritschard <pyr@spootnik.org>
@@ -132,8 +132,10 @@ pfe(struct hoststated *x_env, int pipe_parent2pfe[2], 
 	event_add(&ibuf_main->ev, NULL);
 
 	TAILQ_INIT(&ctl_conns);
-	control_listen();
 
+	if (control_listen() == -1)
+		fatalx("pfe: control socket listen failed");
+
 	event_dispatch();
 	pfe_shutdown();
 
blob - 060024f121900b54a624418396b54bee46c5cd82
blob + 6a38260d606f081c4bd5804e7c5b04a1dd42fae0
--- pfe_filter.c
+++ pfe_filter.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: pfe_filter.c,v 1.11 2007/02/07 14:45:12 reyk Exp $	*/
+/*	$OpenBSD: pfe_filter.c,v 1.12 2007/02/08 13:32:24 reyk Exp $	*/
 
 /*
  * Copyright (c) 2006 Pierre-Yves Ritschard <pyr@spootnik.org>
@@ -79,12 +79,16 @@ init_tables(struct hoststated *env)
 	i = 0;
 
 	TAILQ_FOREACH(service, &env->services, entry) {
-		(void)strlcpy(tables[i].pfrt_anchor, HOSTSTATED_ANCHOR "/",
-		    sizeof(tables[i].pfrt_anchor));
-		(void)strlcat(tables[i].pfrt_anchor, service->name,
-		    sizeof(tables[i].pfrt_anchor));
-		(void)strlcpy(tables[i].pfrt_name, service->name,
-		    sizeof(tables[i].pfrt_name));
+		if (strlcpy(tables[i].pfrt_anchor, HOSTSTATED_ANCHOR "/",
+		    sizeof(tables[i].pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
+			goto toolong;
+		if (strlcat(tables[i].pfrt_anchor, service->name,
+		    sizeof(tables[i].pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
+			goto toolong;
+		if (strlcpy(tables[i].pfrt_name, service->name,
+		    sizeof(tables[i].pfrt_name)) >=
+		    sizeof(tables[i].pfrt_name))
+			goto toolong;
 		tables[i].pfrt_flags |= PFR_TFLAG_PERSIST;
 		i++;
 	}
@@ -110,6 +114,11 @@ init_tables(struct hoststated *env)
 	 */
 	TAILQ_FOREACH(service, &env->services, entry)
 		flush_table(env, service);
+
+	return;
+
+ toolong:
+	fatal("init_tables: name too long");
 }
 
 void
@@ -119,14 +128,20 @@ kill_tables(struct hoststated *env) {
 
 	memset(&io, 0, sizeof(io));
 	TAILQ_FOREACH(service, &env->services, entry) {
-		(void)strlcpy(io.pfrio_table.pfrt_anchor, HOSTSTATED_ANCHOR "/",
-		    sizeof(io.pfrio_table.pfrt_anchor));
-		(void)strlcat(io.pfrio_table.pfrt_anchor, service->name,
-		    sizeof(io.pfrio_table.pfrt_anchor));
+		if (strlcpy(io.pfrio_table.pfrt_anchor, HOSTSTATED_ANCHOR "/",
+		    sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
+			goto toolong;
+		if (strlcat(io.pfrio_table.pfrt_anchor, service->name,
+		    sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
+			goto toolong;
 		if (ioctl(env->pf->dev, DIOCRCLRTABLES, &io) == -1)
 			fatal("kill_tables: ioctl faile: ioctl failed");
 	}
 	log_debug("kill_tables: deleted %d tables", io.pfrio_ndel);
+	return;
+
+ toolong:
+	fatal("kill_tables: name too long");
 }
 
 void
@@ -155,12 +170,16 @@ sync_table(struct hoststated *env, struct service *ser
 	io.pfrio_size = table->up;
 	io.pfrio_size2 = 0;
 	io.pfrio_buffer = addlist;
-	(void)strlcpy(io.pfrio_table.pfrt_anchor, HOSTSTATED_ANCHOR "/",
-	    sizeof(io.pfrio_table.pfrt_anchor));
-	(void)strlcat(io.pfrio_table.pfrt_anchor, service->name,
-	    sizeof(io.pfrio_table.pfrt_anchor));
-	(void)strlcpy(io.pfrio_table.pfrt_name, service->name,
-	    sizeof(io.pfrio_table.pfrt_name));
+	if (strlcpy(io.pfrio_table.pfrt_anchor, HOSTSTATED_ANCHOR "/",
+	    sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
+		goto toolong;
+	if (strlcat(io.pfrio_table.pfrt_anchor, service->name,
+	    sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
+		goto toolong;
+	if (strlcpy(io.pfrio_table.pfrt_name, service->name,
+	    sizeof(io.pfrio_table.pfrt_name)) >=
+	    sizeof(io.pfrio_table.pfrt_name))
+		goto toolong;
 
 	i = 0;
 	TAILQ_FOREACH(host, &table->hosts, entry) {
@@ -199,6 +218,10 @@ sync_table(struct hoststated *env, struct service *ser
 	log_debug("sync_table: table %s: %d added, %d deleted, %d changed",
 	    io.pfrio_table.pfrt_name,
 	    io.pfrio_nadd, io.pfrio_ndel, io.pfrio_nchange);
+	return;
+
+ toolong:
+	fatal("sync_table: name too long");
 }
 
 void
@@ -207,16 +230,23 @@ flush_table(struct hoststated *env, struct service *se
 	struct pfioc_table	io;
 
 	memset(&io, 0, sizeof(io));
-	(void)strlcpy(io.pfrio_table.pfrt_anchor, HOSTSTATED_ANCHOR "/",
-	    sizeof(io.pfrio_table.pfrt_anchor));
-	(void)strlcat(io.pfrio_table.pfrt_anchor, service->name,
-	    sizeof(io.pfrio_table.pfrt_anchor));
-	(void)strlcpy(io.pfrio_table.pfrt_name, service->name,
-	    sizeof(io.pfrio_table.pfrt_name));
+	if (strlcpy(io.pfrio_table.pfrt_anchor, HOSTSTATED_ANCHOR "/",
+	    sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
+		goto toolong;
+	if (strlcat(io.pfrio_table.pfrt_anchor, service->name,
+	    sizeof(io.pfrio_table.pfrt_anchor)) >= PF_ANCHOR_NAME_SIZE)
+		goto toolong;
+	if (strlcpy(io.pfrio_table.pfrt_name, service->name,
+	    sizeof(io.pfrio_table.pfrt_name)) >=
+    	    sizeof(io.pfrio_table.pfrt_name))
+		goto toolong;
 	if (ioctl(env->pf->dev, DIOCRCLRADDRS, &io) == -1)
 		fatal("flush_table: cannot flush table");
 	log_debug("flush_table: flushed table %s", service->name);
 	return;
+
+ toolong:
+	fatal("flush_table: name too long");
 }
 
 int
@@ -227,7 +257,7 @@ transaction_init(struct hoststated *env, const char *a
 	env->pf->pft.array = &env->pf->pfte;
 
 	memset(&env->pf->pfte, 0, sizeof env->pf->pfte);
-	strlcpy(env->pf->pfte.anchor, anchor, PF_ANCHOR_NAME_SIZE);
+	(void)strlcpy(env->pf->pfte.anchor, anchor, PF_ANCHOR_NAME_SIZE);
 	env->pf->pfte.rs_num = PF_RULESET_RDR;
 
 	if (ioctl(env->pf->dev, DIOCXBEGIN, &env->pf->pft) == -1)
@@ -254,13 +284,23 @@ sync_ruleset(struct hoststated *env, struct service *s
 	char			 anchor[PF_ANCHOR_NAME_SIZE];
 
 	bzero(anchor, sizeof(anchor));
-	(void)strlcpy(anchor, HOSTSTATED_ANCHOR "/", sizeof(anchor));
-	(void)strlcat(anchor, service->name, sizeof(anchor));
-	transaction_init(env, anchor);
+	if (strlcpy(anchor, HOSTSTATED_ANCHOR "/", sizeof(anchor)) >=
+	    PF_ANCHOR_NAME_SIZE)
+		goto toolong;
+	if (strlcat(anchor, service->name, sizeof(anchor)) >=
+	    PF_ANCHOR_NAME_SIZE)
+		goto toolong;
+	if (transaction_init(env, anchor) == -1) {
+		log_warn("sync_ruleset: transaction init failed");
+		return;
+	}
 
 	if (!enable) {
-		transaction_commit(env);
-		log_debug("sync_ruleset: rules removed");
+		if (transaction_commit(env) == -1)
+			log_warn("sync_ruleset: "
+			    "remove rules transaction failed");
+		else
+			log_debug("sync_ruleset: rules removed");
 		return;
 	}
 
@@ -306,8 +346,9 @@ sync_ruleset(struct hoststated *env, struct service *s
 		}
 
 		pio.addr.addr.type = PF_ADDR_TABLE;
-		(void)strlcpy(pio.addr.addr.v.tblname, service->name,
-		    sizeof(pio.addr.addr.v.tblname));
+		if (strlcpy(pio.addr.addr.v.tblname, service->name,
+		    sizeof(pio.addr.addr.v.tblname)) >= sizeof(pio.addr.addr.v.tblname))
+			fatal("sync_ruleset: table name too long");
 		if (ioctl(env->pf->dev, DIOCADDADDR, &pio) == -1)
 			fatal("sync_ruleset: cannot add address to pool");
 
@@ -321,7 +362,12 @@ sync_ruleset(struct hoststated *env, struct service *s
 			fatal("cannot add rule");
 		log_debug("sync_ruleset: rule added");
 	}
-	transaction_commit(env);
+	if (transaction_commit(env) == -1)
+		log_warn("sync_ruleset: add rules transaction failed");
+	return;
+
+ toolong:
+	fatal("sync_ruleset: name too long");
 }
 
 void
@@ -332,13 +378,27 @@ flush_rulesets(struct hoststated *env)
 
 	kill_tables(env);
 	TAILQ_FOREACH(service, &env->services, entry) {
-		strlcpy(anchor, HOSTSTATED_ANCHOR "/", sizeof(anchor));
-		strlcat(anchor, service->name, sizeof(anchor));
-		transaction_init(env, anchor);
-		transaction_commit(env);
+		if (strlcpy(anchor, HOSTSTATED_ANCHOR "/", sizeof(anchor)) >=
+		    PF_ANCHOR_NAME_SIZE)
+			goto toolong;
+		if (strlcat(anchor, service->name, sizeof(anchor)) >=
+		    PF_ANCHOR_NAME_SIZE)
+			goto toolong;
+		if (transaction_init(env, anchor) == -1 ||
+		    transaction_commit(env) == -1)
+			log_warn("flush_rulesets: transaction for %s/ failed",
+			    HOSTSTATED_ANCHOR);
 	}
-	strlcpy(anchor, HOSTSTATED_ANCHOR, sizeof(anchor));
-	transaction_init(env, anchor);
-	transaction_commit(env);
+	if (strlcpy(anchor, HOSTSTATED_ANCHOR, sizeof(anchor)) >=
+	    PF_ANCHOR_NAME_SIZE)
+		goto toolong;
+	if (transaction_init(env, anchor) == -1 ||
+	    transaction_commit(env) == -1)
+		log_warn("flush_rulesets: transaction for %s failed",
+		    HOSTSTATED_ANCHOR);
 	log_debug("flush_rulesets: flushed rules");
+ 	return;
+
+ toolong:
+	fatal("flush_rulesets: name too long");
 }
blob - e0aa22192feb59c69e376027c8b4e49baceffc92
blob + 976029c0e43234143f5e969327b0a0385fbd7457
--- relayd.c
+++ relayd.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: relayd.c,v 1.15 2007/02/07 13:30:17 reyk Exp $	*/
+/*	$OpenBSD: relayd.c,v 1.16 2007/02/08 13:32:24 reyk Exp $	*/
 
 /*
  * Copyright (c) 2006 Pierre-Yves Ritschard <pyr@spootnik.org>
@@ -152,8 +152,10 @@ main(int argc, char *argv[])
 	if (getpwnam(HOSTSTATED_USER) == NULL)
 		errx(1, "unknown user %s", HOSTSTATED_USER);
 
-	if (!debug)
-		daemon(1, 0);
+	if (!debug) {
+		if (daemon(1, 0) == -1)
+			err(1, "failed to daemonize");
+	}
 
 	log_info("startup");
 
blob - 87c887d5e218824d4e0e185980e25bfefa4723b2
blob + ddac5c5974c51976a73cf99a4cbb144dd4c0425a
--- ssl.c
+++ ssl.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ssl.c,v 1.5 2007/02/07 14:39:45 reyk Exp $	*/
+/*	$OpenBSD: ssl.c,v 1.6 2007/02/08 13:32:24 reyk Exp $	*/
 
 /*
  * Copyright (c) 2006 Pierre-Yves Ritschard <pyr@spootnik.org>
@@ -94,8 +94,8 @@ ssl_read(int s, short event, void *arg)
 		}
 		return;
 	} 
-	buf_add(cte->buf, rbuf, ret);
-
+	if (buf_add(cte->buf, rbuf, ret) == -1)
+		fatal("ssl_read: buf_add error");
 	if (cte->validate_read != NULL) {
 		if (cte->validate_read(cte) != 0)
 			goto retry;