We need to blacklist IP addresses at the packet level

so we can ignore NAKs from rogue servers who don't
supply a ServerID, or supply a fake one.

7 files changed, 55 insertions, 84 deletions

diff --git a/arp.c b/arp.c
index 3eac18c0..93bd3ea4 100644
--- a/arp.c
+++ b/arp.c
@@ -40,7 +40,7 @@
 #include "ipv4ll.h"
 #include "net.h"
 
-#define ARP_LEN								\
+#define ARP_LEN								      \
 	(sizeof(struct arphdr) + (2 * sizeof(uint32_t)) + (2 * HWADDR_LEN))
 
 static int
@@ -140,7 +140,7 @@ handle_arp_packet(void *arg)
 		memcpy(&reply_t, hw_t + ar.ar_hln, ar.ar_pln);
 
 		/* Check for conflict */
-		if (state->offer && 
+		if (state->offer &&
 		    (reply_s == state->offer->yiaddr ||
 			(reply_s == 0 && reply_t == state->offer->yiaddr)))
 			state->fail.s_addr = state->offer->yiaddr;
diff --git a/dhcpcd.8.in b/dhcpcd.8.in
index c1b8f637..4efa0e0d 100644
--- a/dhcpcd.8.in
+++ b/dhcpcd.8.in
@@ -22,7 +22,7 @@
 .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 .\" SUCH DAMAGE.
 .\"
-.Dd February 27, 2009
+.Dd March 10, 2009
 .Dt DHCPCD 8 SMM
 .Sh NAME
 .Nm dhcpcd
@@ -452,15 +452,8 @@ files.
 Display a list of option codes and the associated variable for use in
 .Xr dhcpcd-run-hooks 8 .
 .It Fl X, -blacklist Ar address Ns Op Ar /cidr
-Ignores all DHCP messages which have this
-.Ar address
-as the server ID or offered address.
-If
-.Ar cidr
-is given then we match against that network as well.
-This may be expanded in future releases to ignore all packets
-matching either the IP or hardware
-.Ar address .
+Ignore all packets from
+.Ar address Ns Op Ar /cidr .
 .It Fl Z , -denyinterfaces Ar pattern
 When discovering interfaces, the interface name must not match
 .Ar pattern
diff --git a/dhcpcd.c b/dhcpcd.c
index 664d0d11..752ca555 100644
--- a/dhcpcd.c
+++ b/dhcpcd.c
@@ -411,6 +411,17 @@ log_dhcp(int lvl, const char *msg,
 	free(a);
 }
 
+static int
+blacklisted_ip(const struct if_options *ifo, in_addr_t addr)
+{
+	size_t i;
+	
+	for (i = 0; i < ifo->blacklist_len; i += 2)
+		if (ifo->blacklist[i] == (addr & ifo->blacklist[i + 1]))
+			return 1;
+	return 0;
+}
+
 static void
 handle_dhcp(struct interface *iface, struct dhcp_message **dhcpp)
 {
@@ -419,9 +430,8 @@ handle_dhcp(struct interface *iface, struct dhcp_message **dhcpp)
 	struct dhcp_message *dhcp = *dhcpp;
 	struct dhcp_lease *lease = &state->lease;
 	uint8_t type, tmp;
-	struct in_addr addr, addr2;
+	struct in_addr addr;
 	size_t i;
-	char *a;
 
 	/* reset the message counter */
 	state->interval = 0;
@@ -430,52 +440,6 @@ handle_dhcp(struct interface *iface, struct dhcp_message **dhcpp)
 	if (get_option_uint8(&type, dhcp, DHO_MESSAGETYPE) == -1) 
 		type = 0;
 
-	/* Ensure that it's not from a blacklisted server.
-	 * We should expand this to check IP and/or hardware address
-	 * at the packet level. */
-	if (ifo->blacklist_len != 0) {
-		if (get_option_addr(&addr.s_addr, dhcp, DHO_SERVERID) != 0)
-			addr.s_addr = 0;
-		for (i = 0; i < ifo->blacklist_len; i += 2) {
-			if (ifo->blacklist[i] ==
-			    (addr.s_addr & ifo->blacklist[i + 1]))
-			{
-				if (dhcp->servername[0])
-					syslog(LOG_WARNING,
-					    "%s: blacklisted server %s `%s'",
-					    iface->name,
-					    inet_ntoa(addr), dhcp->servername);
-				else
-					syslog(LOG_WARNING,
-					    "%s: blacklisted server %s",
-					    iface->name, inet_ntoa(addr));
-				return;
-			}
-			if (ifo->blacklist[i] ==
-			    (dhcp->yiaddr & ifo->blacklist[i + 1]))
-			{
-				addr2.s_addr = dhcp->yiaddr;
-				a = xstrdup(inet_ntoa(addr2));
-				if (dhcp->servername[0])
-					syslog(LOG_WARNING,
-					    "%s: blacklisted offer"
-					    " %s from %s `%s'",
-					    iface->name, a,
-					    inet_ntoa(addr), dhcp->servername);
-				else if (addr.s_addr)
-					syslog(LOG_WARNING,
-					    "%s: blacklisted offer %s from %s",
-					    iface->name, a, inet_ntoa(addr));
-				else
-					syslog(LOG_WARNING,
-					    "%s: blacklisted offer %s",
-					    iface->name, a);
-				free(a);
-				return;
-			}
-		}
-	}
-
 	/* We should restart on a NAK */
 	if (type == DHCP_NAK) {
 		log_dhcp(LOG_WARNING, "NAK:", iface, dhcp);
@@ -514,7 +478,8 @@ handle_dhcp(struct interface *iface, struct dhcp_message **dhcpp)
 		lease->addr.s_addr = dhcp->yiaddr;
 		lease->server.s_addr = 0;
 		if (type)
-			get_option_addr(&lease->server.s_addr, dhcp, DHO_SERVERID);
+			get_option_addr(&lease->server.s_addr, dhcp,
+			    DHO_SERVERID);
 		log_dhcp(LOG_INFO, "offered", iface, dhcp);
 		free(state->offer);
 		state->offer = dhcp;
@@ -595,6 +560,7 @@ handle_dhcp_packet(void *arg)
 	struct dhcp_message *dhcp = NULL;
 	const uint8_t *pp;
 	ssize_t bytes;
+	struct in_addr from;
 
 	/* We loop through until our buffer is empty.
 	 * The benefit is that if we get >1 DHCP packet in our buffer and
@@ -605,35 +571,45 @@ handle_dhcp_packet(void *arg)
 		    packet, udp_dhcp_len);
 		if (bytes == 0 || bytes == -1)
 			break;
-		if (valid_udp_packet(packet, bytes) == -1)
+		if (valid_udp_packet(packet, bytes, &from) == -1) {
+			syslog(LOG_ERR, "%s: invalid UDP packet from %s",
+			    iface->name, inet_ntoa(from));
 			continue;
+		}
+		if (blacklisted_ip(iface->state->options, from.s_addr)) {
+			syslog(LOG_WARNING,
+			    "%s: blacklisted DHCP packet from %s",
+			    iface->name, inet_ntoa(from));
+			continue;
+		}
 		bytes = get_udp_data(&pp, packet);
 		if ((size_t)bytes > sizeof(*dhcp)) {
-			syslog(LOG_ERR, "%s: packet greater than DHCP size",
-			    iface->name);
+			syslog(LOG_ERR,
+			    "%s: packet greater than DHCP size from %s",
+			    iface->name, inet_ntoa(from));
 			continue;
 		}
 		if (!dhcp)
 			dhcp = xzalloc(sizeof(*dhcp));
 		memcpy(dhcp, pp, bytes);
 		if (dhcp->cookie != htonl(MAGIC_COOKIE)) {
-			syslog(LOG_DEBUG, "%s: bogus cookie, ignoring",
-			    iface->name);
+			syslog(LOG_DEBUG, "%s: bogus cookie from %s",
+			    iface->name, inet_ntoa(from));
 			continue;
 		}
 		/* Ensure it's the right transaction */
 		if (iface->state->xid != dhcp->xid) {
 			syslog(LOG_DEBUG,
-			    "%s: ignoring packet with xid 0x%x as"
-			    " it's not ours (0x%x)",
-			    iface->name, dhcp->xid, iface->state->xid);
+			    "%s: wrong xid 0x%x (expecting 0x%x) from %s",
+			    iface->name, dhcp->xid, iface->state->xid,
+			    inet_ntoa(from));
 			continue;
 		}
 		/* Ensure packet is for us */
 		if (iface->hwlen <= sizeof(dhcp->chaddr) &&
 		    memcmp(dhcp->chaddr, iface->hwaddr, iface->hwlen))
 		{
-			syslog(LOG_DEBUG, "%s: xid 0x%x is not for our hwaddr %s",
+			syslog(LOG_DEBUG, "%s: xid 0x%x is not for hwaddr %s",
 			    iface->name, dhcp->xid,
 			    hwaddr_ntoa(dhcp->chaddr, sizeof(dhcp->chaddr)));
 			continue;
diff --git a/dhcpcd.conf.5.in b/dhcpcd.conf.5.in
index 3c6ca1a0..4f71de15 100644
--- a/dhcpcd.conf.5.in
+++ b/dhcpcd.conf.5.in
@@ -22,7 +22,7 @@
 .\" OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
 .\" SUCH DAMAGE.
 .\"
-.Dd February 27, 2009
+.Dd March 10, 2009
 .Dt DHCPCD.CONF 5 SMM
 .Sh NAME
 .Nm dhcpcd.conf
@@ -61,15 +61,8 @@ Background immediately.
 This is useful for startup scripts which don't disable link messages for
 carrier status.
 .It Ic blacklist Ar address Ns Op Ar /cidr
-Ignores all DHCP messages which have this
-.Ar address
-as the server ID or offered address.
-If
-.Ar cidr
-is given then we match against that network as well.
-This may be expanded in future releases to ignore all packets
-matching either the IP or hardware
-.Ar address .
+Ignores all packets from
+.Ar address Ns Op Ar /cidr .
 .It Ic clientid Ar string
 Send the
 .Ar clientid .
diff --git a/if-options.c b/if-options.c
index 7e79633a..c49031df 100644
--- a/if-options.c
+++ b/if-options.c
@@ -634,9 +634,10 @@ parse_option(struct if_options *ifo, int opt, const char *arg)
 		}
 		break;
 	case 'X':
-		addr2.s_addr = ~0U;
 		if (parse_addr(&addr, &addr2, arg) != 0)
 			return -1;
+		if (strchr(arg, '/') == NULL)
+			addr2.s_addr = INADDR_BROADCAST;
 		ifo->blacklist = xrealloc(ifo->blacklist,
 		    sizeof(in_addr_t) * (ifo->blacklist_len + 2));
 		ifo->blacklist[ifo->blacklist_len++] = addr.s_addr;
diff --git a/net.c b/net.c
index b6d1f6bf..b9cb280c 100644
--- a/net.c
+++ b/net.c
@@ -639,16 +639,24 @@ get_udp_data(const uint8_t **data, const uint8_t *udp)
 }
 
 int
-valid_udp_packet(const uint8_t *data, size_t data_len)
+valid_udp_packet(const uint8_t *data, size_t data_len, struct in_addr *from)
 {
 	struct udp_dhcp_packet packet;
 	uint16_t bytes, udpsum;
 
+	if (data_len < sizeof(packet.ip)) {
+		if (from)
+			from->s_addr = INADDR_ANY;
+		errno = EINVAL;
+		return -1;
+	}
+	memcpy(&packet, data, MIN(data_len, sizeof(packet)));
+	if (from)
+		from->s_addr = packet.ip.ip_src.s_addr;
 	if (data_len > sizeof(packet)) {
 		errno = EINVAL;
 		return -1;
 	}
-	memcpy(&packet, data, data_len);
 	if (checksum(&packet.ip, sizeof(packet.ip)) != 0) {
 		errno = EINVAL;
 		return -1;
diff --git a/net.h b/net.h
index 3ebc454a..19abb91d 100644
--- a/net.h
+++ b/net.h
@@ -137,7 +137,7 @@ const size_t udp_dhcp_len;
 ssize_t make_udp_packet(uint8_t **, const uint8_t *, size_t,
     struct in_addr, struct in_addr);
 ssize_t get_udp_data(const uint8_t **, const uint8_t *);
-int valid_udp_packet(const uint8_t *, size_t);
+int valid_udp_packet(const uint8_t *, size_t, struct in_addr *);
 
 int open_socket(struct interface *, int);
 ssize_t send_packet(const struct interface *, struct in_addr,