Merge branch 'dhcp-fixes'

Fixes some issues in the dhcp plugin like avoiding ICMP port unreachables
when setting a specific server address, or increasing the maximum size for
options e.g. for DNs in the client identifier option. The latter is also
only sent now if identity_lease is enabled (for most DHCP servers it
serves the same function as a unique MAC address does).

conf/plugins/dhcp.opt

@@ -9,7 +9,8 @@ charon.plugins.dhcp.force_server_address = no
 	192.168.0.255) as server address might work.
 
 charon.plugins.dhcp.identity_lease = no
-	Derive user-defined MAC address from hash of IKE identity.
+	Derive user-defined MAC address from hash of IKE identity and send client
+	identity DHCP option.
 
 charon.plugins.dhcp.server = 255.255.255.255
 	DHCP server unicast or broadcast IP address.

src/libcharon/plugins/dhcp/dhcp_socket.c

@@ -1,4 +1,7 @@
 /*
+ * Copyright (C) 2012-2018 Tobias Brunner
+ * HSR Hochschule fuer Technik Rapperswil
+ *
  * Copyright (C) 2010 Martin Willi
  * Copyright (C) 2010 revosec AG
  *
@@ -157,7 +160,7 @@ typedef struct __attribute__((packed)) {
 } dhcp_option_t;
 
 /**
- * DHCP message format, with a maximum size options buffer
+ * DHCP message format, with a minimum size options buffer
  */
 typedef struct __attribute__((packed)) {
 	uint8_t opcode;
@@ -176,9 +179,19 @@ typedef struct __attribute__((packed)) {
 	char server_hostname[64];
 	char boot_filename[128];
 	uint32_t magic_cookie;
-	u_char options[252];
+	u_char options[308];
 } dhcp_t;
 
+/**
+ * Check if the given address equals the broadcast address
+ */
+static inline bool is_broadcast(host_t *host)
+{
+	chunk_t broadcast = chunk_from_chars(0xFF,0xFF,0xFF,0xFF);
+
+	return chunk_equals(broadcast, host->get_address(host));
+}
+
 /**
  * Prepare a DHCP message for a given transaction
  */
@@ -186,10 +199,10 @@ static int prepare_dhcp(private_dhcp_socket_t *this,
 						dhcp_transaction_t *transaction,
 						dhcp_message_type_t type, dhcp_t *dhcp)
 {
-	chunk_t chunk, broadcast = chunk_from_chars(0xFF,0xFF,0xFF,0xFF);
+	chunk_t chunk;
 	identification_t *identity;
 	dhcp_option_t *option;
-	int optlen = 0;
+	int optlen = 0, remaining;
 	host_t *src;
 	uint32_t id;
 
@@ -198,7 +211,7 @@ static int prepare_dhcp(private_dhcp_socket_t *this,
 	dhcp->hw_type = ARPHRD_ETHER;
 	dhcp->hw_addr_len = 6;
 	dhcp->transaction_id = transaction->get_id(transaction);
-	if (chunk_equals(broadcast, this->dst->get_address(this->dst)))
+	if (is_broadcast(this->dst))
 	{
 		/* Set broadcast flag to get broadcasted replies, as we actually
 		 * do not own the MAC we request an address for. */
@@ -241,21 +254,29 @@ static int prepare_dhcp(private_dhcp_socket_t *this,
 	option->data[0] = type;
 	optlen += sizeof(dhcp_option_t) + option->len;
 
+	/* the REQUEST message has the most static overhead in the 'options' field
+	 * with 17 bytes */
+	remaining = sizeof(dhcp->options) - optlen - 17;
+
 	if (identity->get_type(identity) == ID_FQDN)
 	{
 		option = (dhcp_option_t*)&dhcp->options[optlen];
 		option->type = DHCP_HOST_NAME;
-		option->len = min(chunk.len, 64);
+		option->len = min(min(chunk.len, remaining-sizeof(dhcp_option_t)), 255);
+		memcpy(option->data, chunk.ptr, option->len);
+		optlen += sizeof(dhcp_option_t) + option->len;
+		remaining -= sizeof(dhcp_option_t) + option->len;
+	}
+
+	if (this->identity_lease &&
+		remaining >= sizeof(dhcp_option_t) + 2)
+	{
+		option = (dhcp_option_t*)&dhcp->options[optlen];
+		option->type = DHCP_CLIENT_ID;
+		option->len = min(min(chunk.len, remaining-sizeof(dhcp_option_t)), 255);
 		memcpy(option->data, chunk.ptr, option->len);
 		optlen += sizeof(dhcp_option_t) + option->len;
 	}
-
-	option = (dhcp_option_t*)&dhcp->options[optlen];
-	option->type = DHCP_CLIENT_ID;
-	option->len = min(chunk.len, 64);
-	memcpy(option->data, chunk.ptr, option->len);
-	optlen += sizeof(dhcp_option_t) + option->len;
-
 	return optlen;
 }
 
@@ -273,7 +294,7 @@ static bool send_dhcp(private_dhcp_socket_t *this,
 	{
 		dst = this->dst;
 	}
-	len = offsetof(dhcp_t, magic_cookie) + ((optlen + 4) / 64 * 64 + 64);
+	len = offsetof(dhcp_t, magic_cookie) + optlen + 4;
 	return sendto(this->send, dhcp, len, 0, dst->get_sockaddr(dst),
 				  *dst->get_sockaddr_len(dst)) == len;
 }
@@ -675,7 +696,7 @@ dhcp_socket_t *dhcp_socket_create()
 		},
 	};
 	char *iface;
-	int on = 1;
+	int on = 1, rcvbuf = 0;
 	struct sock_filter dhcp_filter_code[] = {
 		BPF_STMT(BPF_LD+BPF_B+BPF_ABS,
 				 offsetof(struct iphdr, protocol)),
@@ -685,9 +706,9 @@ dhcp_socket_t *dhcp_socket_create()
 		BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, DHCP_SERVER_PORT, 0, 14),
 		BPF_STMT(BPF_LD+BPF_H+BPF_ABS, sizeof(struct iphdr) +
 				 offsetof(struct udphdr, dest)),
-		BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, DHCP_CLIENT_PORT, 0, 2),
+		BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, DHCP_CLIENT_PORT, 2, 0),
-		BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, DHCP_SERVER_PORT, 0, 1),
+		BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, DHCP_SERVER_PORT, 1, 0),
-		BPF_JUMP(BPF_JMP+BPF_JA, 0, 0, 10),
+		BPF_JUMP(BPF_JMP+BPF_JA, 10, 0, 0),
 		BPF_STMT(BPF_LD+BPF_B+BPF_ABS, sizeof(struct iphdr) +
 				 sizeof(struct udphdr) + offsetof(dhcp_t, opcode)),
 		BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, BOOTREPLY, 0, 8),
@@ -766,6 +787,30 @@ dhcp_socket_t *dhcp_socket_create()
 		destroy(this);
 		return NULL;
 	}
+	/* we won't read any data from this socket, so reduce the buffer to save
+	 * some memory (there is some minimum, still try 0, though).
+	 * note that we might steal some packets from other processes if e.g. a DHCP
+	 * client (or server) is running on the same host, but by reducing the
+	 * buffer size the impact should be minimized */
+	if (setsockopt(this->send, SOL_SOCKET, SO_RCVBUF, &rcvbuf,
+				   sizeof(rcvbuf)) == -1)
+	{
+		DBG1(DBG_CFG, "unable to reduce receive buffer on DHCP send socket: %s",
+			 strerror(errno));
+		destroy(this);
+		return NULL;
+	}
+	if (!is_broadcast(this->dst))
+	{
+		/* when setting giaddr (which we do when we don't broadcast), the server
+		 * should respond to the server port on that IP, according to RFC 2131,
+		 * section 4.1.  while we do receive such messages via raw socket, the
+		 * kernel will respond with an ICMP port unreachable if there is no
+		 * socket bound to that port, which might be problematic with certain
+		 * DHCP servers.  instead of opening an additional socket, that we don't
+		 * actually use, we can also just send our requests from port 67 */
+		src.sin_port = htons(DHCP_SERVER_PORT);
+	}
 	if (bind(this->send, (struct sockaddr*)&src, sizeof(src)) == -1)
 	{
 		DBG1(DBG_CFG, "unable to bind DHCP send socket: %s", strerror(errno));

src/libcharon/plugins/dhcp/dhcp_transaction.h

@@ -33,7 +33,7 @@ typedef struct dhcp_transaction_t dhcp_transaction_t;
 struct dhcp_transaction_t {
 
 	/**
-	 * Get the DCHP transaction ID.
+	 * Get the DHCP transaction ID.
 	 *
 	 * @return			DHCP transaction identifier
 	 */
@@ -61,7 +61,7 @@ struct dhcp_transaction_t {
 	host_t* (*get_address)(dhcp_transaction_t *this);
 
 	/**
-	 * Set the DCHP server address discovered.
+	 * Set the DHCP server address discovered.
 	 *
 	 * @param server	DHCP server address
 	 */
@@ -75,7 +75,7 @@ struct dhcp_transaction_t {
 	host_t* (*get_server)(dhcp_transaction_t *this);
 
 	/**
-	 * An an additional attribute to serve to peer.
+	 * Add an additional attribute to serve to peer.
 	 *
 	 * @param type		type of attribute
 	 * @param data		attribute data

testing/tests/ikev2/dhcp-dynamic/hosts/moon/etc/iptables.rules

@@ -5,8 +5,8 @@
 -P OUTPUT DROP
 -P FORWARD DROP
 
-# allow bootpc and bootps
+# allow bootps (in relay mode also in OUTPUT)
--A OUTPUT -p udp --sport bootpc --dport bootps -j ACCEPT
+-A OUTPUT -p udp --sport bootps --dport bootps -j ACCEPT
 -A INPUT  -p udp --sport bootps --dport bootps -j ACCEPT
 
 # allow broadcasts from eth1

testing/tests/ikev2/dhcp-dynamic/posttest.dat

@@ -1,8 +1,9 @@
 moon::ipsec stop
 carol::ipsec stop
 dave::ipsec stop
-venus::cat /var/state/dhcp/dhcpd.leases
+venus::cat /var/lib/dhcp/dhcpd.leases
 venus::service isc-dhcp-server stop 2> /dev/null
+venus::rm /var/lib/dhcp/dhcpd.leases*; touch /var/lib/dhcp/dhcpd.leases
 moon::iptables-restore < /etc/iptables.flush
 carol::iptables-restore < /etc/iptables.flush
 dave::iptables-restore < /etc/iptables.flush

testing/tests/ikev2/dhcp-static-client-id/hosts/moon/etc/iptables.rules

@@ -5,8 +5,8 @@
 -P OUTPUT DROP
 -P FORWARD DROP
 
-# allow bootpc and bootps
+# allow bootps (in relay mode also in OUTPUT)
--A OUTPUT -p udp --sport bootpc --dport bootps -j ACCEPT
+-A OUTPUT -p udp --sport bootps --dport bootps -j ACCEPT
 -A INPUT  -p udp --sport bootps --dport bootps -j ACCEPT
 
 # allow broadcasts from eth1

testing/tests/ikev2/dhcp-static-client-id/hosts/moon/etc/strongswan.conf

@@ -6,6 +6,7 @@ charon {
   plugins {
     dhcp {
       server = 10.1.255.255
+      identity_lease = yes
     }
   }
 }

testing/tests/ikev2/dhcp-static-mac/hosts/moon/etc/iptables.rules

@@ -5,8 +5,8 @@
 -P OUTPUT DROP
 -P FORWARD DROP
 
-# allow bootpc and bootps
+# allow bootps (in relay mode also in OUTPUT)
--A OUTPUT -p udp --sport bootpc --dport bootps -j ACCEPT
+-A OUTPUT -p udp --sport bootps --dport bootps -j ACCEPT
 -A INPUT  -p udp --sport bootps --dport bootps -j ACCEPT
 
 # allow broadcasts from eth1

testing/tests/swanctl/dhcp-dynamic/hosts/moon/etc/iptables.rules

@@ -5,8 +5,8 @@
 -P OUTPUT DROP
 -P FORWARD DROP
 
-# allow bootpc and bootps
+# allow bootps (in relay mode also in OUTPUT)
--A OUTPUT -p udp --sport bootpc --dport bootps -j ACCEPT
+-A OUTPUT -p udp --sport bootps --dport bootps -j ACCEPT
 -A INPUT  -p udp --sport bootps --dport bootps -j ACCEPT
 
 # allow broadcasts from eth1

testing/tests/swanctl/dhcp-dynamic/posttest.dat

@@ -3,8 +3,9 @@ dave::swanctl --terminate --ike home
 carol::systemctl stop strongswan-swanctl
 dave::systemctl stop strongswan-swanctl
 moon::systemctl stop strongswan-swanctl
-venus::cat /var/state/dhcp/dhcpd.leases
+venus::cat /var/lib/dhcp/dhcpd.leases
-venus::server isc-dhcp-server stop 2> /dev/null
+venus::service isc-dhcp-server stop 2> /dev/null
+venus::rm /var/lib/dhcp/dhcpd.leases*; touch /var/lib/dhcp/dhcpd.leases
 moon::iptables-restore < /etc/iptables.flush
 carol::iptables-restore < /etc/iptables.flush
 dave::iptables-restore < /etc/iptables.flush