From 646a492538aac4895ac728d3b867db0beeb61d22 Mon Sep 17 00:00:00 2001 From: Matt Selsky Date: Thu, 12 Apr 2018 00:17:49 -0400 Subject: [PATCH 1/7] dhcp: Fix typos in comments --- src/libcharon/plugins/dhcp/dhcp_transaction.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libcharon/plugins/dhcp/dhcp_transaction.h b/src/libcharon/plugins/dhcp/dhcp_transaction.h index 0c614f7b1..61fee2a8e 100644 --- a/src/libcharon/plugins/dhcp/dhcp_transaction.h +++ b/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 From e8b5c7b94ecca6d16748b210fd62b64826a5d2f7 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 16 Mar 2018 09:59:25 +0100 Subject: [PATCH 2/7] dhcp: Fix destination port check in packet filter The previous code compared the port in the packet to the client port and, if successful, checked it also against the server port, which, therefore, never matched, but due to incorrect offsets did skip the BPF_JA. If the client port didn't match the code also skipped to the instruction after the BPF_JA. However, the latter was incorrect also and processing would have continued at the next instruction anyway. Basically, DHCP packets to any port were accepted. What's not fixed with this is that the kernel returns an ICMP Port unreachable for packets sent to the server port (67) because we don't have a socket bound to it. Fixes: f0212e8837b5 ("Accept DHCP replies on bootps port, as we act as a relay agent if server address configured") --- src/libcharon/plugins/dhcp/dhcp_socket.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libcharon/plugins/dhcp/dhcp_socket.c b/src/libcharon/plugins/dhcp/dhcp_socket.c index 7541c3b49..02aa29853 100644 --- a/src/libcharon/plugins/dhcp/dhcp_socket.c +++ b/src/libcharon/plugins/dhcp/dhcp_socket.c @@ -685,9 +685,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_SERVER_PORT, 0, 1), - BPF_JUMP(BPF_JMP+BPF_JA, 0, 0, 10), + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, DHCP_CLIENT_PORT, 2, 0), + BPF_JUMP(BPF_JMP+BPF_JEQ+BPF_K, DHCP_SERVER_PORT, 1, 0), + 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), From becf027cd9b0af162247015a9fff6c00e59fd6ce Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 10 Apr 2018 17:04:10 +0200 Subject: [PATCH 3/7] dhcp: Bind server port when a specific server address is specified DHCP servers will respond to port 67 if giaddr is non-zero, which we set if we are not broadcasting. While such messages are received fine via RAW socket the kernel will respond with an ICMP port unreachable if no socket is bound to that port. Instead of opening a dummy socket on port 67 just to avoid the ICMPs we can also just operate with a single socket, bind it to port 67 and send our requests from that port. Since SO_REUSEADDR behaves on Linux like SO_REUSEPORT does on other systems we can bind that port even if a DHCP server is running on the same host as the daemon (this might have to be adapted to make this work on other systems, but due to the raw socket the plugin is not that portable anyway). --- src/libcharon/plugins/dhcp/dhcp_socket.c | 28 +++++++++++++++++-- .../hosts/moon/etc/iptables.rules | 4 +-- .../hosts/moon/etc/iptables.rules | 4 +-- .../hosts/moon/etc/iptables.rules | 4 +-- .../hosts/moon/etc/iptables.rules | 4 +-- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/src/libcharon/plugins/dhcp/dhcp_socket.c b/src/libcharon/plugins/dhcp/dhcp_socket.c index 02aa29853..765171ff4 100644 --- a/src/libcharon/plugins/dhcp/dhcp_socket.c +++ b/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 * @@ -179,6 +182,16 @@ typedef struct __attribute__((packed)) { u_char options[252]; } 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,7 +199,7 @@ 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; @@ -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. */ @@ -766,6 +779,17 @@ dhcp_socket_t *dhcp_socket_create() 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)); diff --git a/testing/tests/ikev2/dhcp-dynamic/hosts/moon/etc/iptables.rules b/testing/tests/ikev2/dhcp-dynamic/hosts/moon/etc/iptables.rules index 2d9a466b0..792fc56bc 100644 --- a/testing/tests/ikev2/dhcp-dynamic/hosts/moon/etc/iptables.rules +++ b/testing/tests/ikev2/dhcp-dynamic/hosts/moon/etc/iptables.rules @@ -5,8 +5,8 @@ -P OUTPUT DROP -P FORWARD DROP -# allow bootpc and bootps --A OUTPUT -p udp --sport bootpc --dport bootps -j ACCEPT +# allow bootps (in relay mode also in OUTPUT) +-A OUTPUT -p udp --sport bootps --dport bootps -j ACCEPT -A INPUT -p udp --sport bootps --dport bootps -j ACCEPT # allow broadcasts from eth1 diff --git a/testing/tests/ikev2/dhcp-static-client-id/hosts/moon/etc/iptables.rules b/testing/tests/ikev2/dhcp-static-client-id/hosts/moon/etc/iptables.rules index 2d9a466b0..792fc56bc 100644 --- a/testing/tests/ikev2/dhcp-static-client-id/hosts/moon/etc/iptables.rules +++ b/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 --A OUTPUT -p udp --sport bootpc --dport bootps -j ACCEPT +# allow bootps (in relay mode also in OUTPUT) +-A OUTPUT -p udp --sport bootps --dport bootps -j ACCEPT -A INPUT -p udp --sport bootps --dport bootps -j ACCEPT # allow broadcasts from eth1 diff --git a/testing/tests/ikev2/dhcp-static-mac/hosts/moon/etc/iptables.rules b/testing/tests/ikev2/dhcp-static-mac/hosts/moon/etc/iptables.rules index 2d9a466b0..792fc56bc 100644 --- a/testing/tests/ikev2/dhcp-static-mac/hosts/moon/etc/iptables.rules +++ b/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 --A OUTPUT -p udp --sport bootpc --dport bootps -j ACCEPT +# allow bootps (in relay mode also in OUTPUT) +-A OUTPUT -p udp --sport bootps --dport bootps -j ACCEPT -A INPUT -p udp --sport bootps --dport bootps -j ACCEPT # allow broadcasts from eth1 diff --git a/testing/tests/swanctl/dhcp-dynamic/hosts/moon/etc/iptables.rules b/testing/tests/swanctl/dhcp-dynamic/hosts/moon/etc/iptables.rules index 2d9a466b0..792fc56bc 100644 --- a/testing/tests/swanctl/dhcp-dynamic/hosts/moon/etc/iptables.rules +++ b/testing/tests/swanctl/dhcp-dynamic/hosts/moon/etc/iptables.rules @@ -5,8 +5,8 @@ -P OUTPUT DROP -P FORWARD DROP -# allow bootpc and bootps --A OUTPUT -p udp --sport bootpc --dport bootps -j ACCEPT +# allow bootps (in relay mode also in OUTPUT) +-A OUTPUT -p udp --sport bootps --dport bootps -j ACCEPT -A INPUT -p udp --sport bootps --dport bootps -j ACCEPT # allow broadcasts from eth1 From 13edecdc20487ad074cc0a7b1b14c8aec9bd9ff9 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 10 Apr 2018 18:14:32 +0200 Subject: [PATCH 4/7] dhcp: Reduce receive buffer size on send socket Since we won't read from the socket reducing the receive buffer saves some memory and it should also minimize the impact on other processes that bind the same port (Linux distributes packets to the sockets round-robin). --- src/libcharon/plugins/dhcp/dhcp_socket.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/src/libcharon/plugins/dhcp/dhcp_socket.c b/src/libcharon/plugins/dhcp/dhcp_socket.c index 765171ff4..3167b0503 100644 --- a/src/libcharon/plugins/dhcp/dhcp_socket.c +++ b/src/libcharon/plugins/dhcp/dhcp_socket.c @@ -688,7 +688,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)), @@ -779,6 +779,19 @@ 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 From 30e886fe3bb6ecb38a58370de447ec735ff9cc51 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 10 Apr 2018 18:19:35 +0200 Subject: [PATCH 5/7] dhcp: Increase buffer size for options in DHCP messages According to RFC 2131, the minimum size of the 'options' field is 312 bytes, including the 4 byte magic cookie. There also does not seem to be any restriction regarding the message length, previously the length was rounded to a multiple of 64 bytes. The latter might have been because in BOOTP the options field (or rather vendor-specific area as it was called back then) had a fixed length of 64 bytes (so max(optlen+4, 64) might actually have been what was intended), but for DHCP the field is explicitly variable length, so I don't think it's necessary to pad it. --- src/libcharon/plugins/dhcp/dhcp_socket.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libcharon/plugins/dhcp/dhcp_socket.c b/src/libcharon/plugins/dhcp/dhcp_socket.c index 3167b0503..7b64380b7 100644 --- a/src/libcharon/plugins/dhcp/dhcp_socket.c +++ b/src/libcharon/plugins/dhcp/dhcp_socket.c @@ -160,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; @@ -179,7 +179,7 @@ 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; /** @@ -286,7 +286,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; } From 0e5b94d03845bfb60842014ebb14f153a337954f Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 10 Apr 2018 18:45:16 +0200 Subject: [PATCH 6/7] dhcp: Increase maximum size of client identification option This increases the chances that subject DNs that might have been cut off with the arbitrary previous limit of 64 bytes might now be sent successfully. The REQUEST message has the most static overhead in terms of other options (17 bytes) as compared to DISCOVER (5) and RELEASE (7). Added to that are 3 bytes for the DHCP message type, which means we have 288 bytes left for the two options based on the client identity (host name and client identification). Since both contain the same value, a FQDN identity, which causes a host name option to get added, may be 142 bytes long, other identities like subject DNs may be 255 bytes long (the maximum for a DHCP option). --- src/libcharon/plugins/dhcp/dhcp_socket.c | 25 +++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/src/libcharon/plugins/dhcp/dhcp_socket.c b/src/libcharon/plugins/dhcp/dhcp_socket.c index 7b64380b7..320b17b1e 100644 --- a/src/libcharon/plugins/dhcp/dhcp_socket.c +++ b/src/libcharon/plugins/dhcp/dhcp_socket.c @@ -202,7 +202,7 @@ static int prepare_dhcp(private_dhcp_socket_t *this, chunk_t chunk; identification_t *identity; dhcp_option_t *option; - int optlen = 0; + int optlen = 0, remaining; host_t *src; uint32_t id; @@ -254,21 +254,28 @@ 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 (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; } From 7b660944b63569d2f9c8fdd945b034564d532d9b Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Wed, 11 Apr 2018 10:51:01 +0200 Subject: [PATCH 7/7] dhcp: Only send client identifier if identity_lease is enabled The client identifier serves as unique identifier just like a unique MAC address would, so even with identity_leases disabled some DHCP servers might assign unique leases per identity. --- conf/plugins/dhcp.opt | 3 ++- src/libcharon/plugins/dhcp/dhcp_socket.c | 3 ++- testing/tests/ikev2/dhcp-dynamic/posttest.dat | 3 ++- .../dhcp-static-client-id/hosts/moon/etc/strongswan.conf | 1 + testing/tests/swanctl/dhcp-dynamic/posttest.dat | 5 +++-- 5 files changed, 10 insertions(+), 5 deletions(-) diff --git a/conf/plugins/dhcp.opt b/conf/plugins/dhcp.opt index 9c7b86091..6b337bc34 100644 --- a/conf/plugins/dhcp.opt +++ b/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. diff --git a/src/libcharon/plugins/dhcp/dhcp_socket.c b/src/libcharon/plugins/dhcp/dhcp_socket.c index 320b17b1e..c26fcc920 100644 --- a/src/libcharon/plugins/dhcp/dhcp_socket.c +++ b/src/libcharon/plugins/dhcp/dhcp_socket.c @@ -268,7 +268,8 @@ static int prepare_dhcp(private_dhcp_socket_t *this, remaining -= sizeof(dhcp_option_t) + option->len; } - if (remaining >= sizeof(dhcp_option_t) + 2) + if (this->identity_lease && + remaining >= sizeof(dhcp_option_t) + 2) { option = (dhcp_option_t*)&dhcp->options[optlen]; option->type = DHCP_CLIENT_ID; diff --git a/testing/tests/ikev2/dhcp-dynamic/posttest.dat b/testing/tests/ikev2/dhcp-dynamic/posttest.dat index d4a05b28b..60be3f95c 100644 --- a/testing/tests/ikev2/dhcp-dynamic/posttest.dat +++ b/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 diff --git a/testing/tests/ikev2/dhcp-static-client-id/hosts/moon/etc/strongswan.conf b/testing/tests/ikev2/dhcp-static-client-id/hosts/moon/etc/strongswan.conf index c4a0ff8bb..0883bf058 100644 --- a/testing/tests/ikev2/dhcp-static-client-id/hosts/moon/etc/strongswan.conf +++ b/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 } } } diff --git a/testing/tests/swanctl/dhcp-dynamic/posttest.dat b/testing/tests/swanctl/dhcp-dynamic/posttest.dat index 37e8b02d8..466fc931c 100644 --- a/testing/tests/swanctl/dhcp-dynamic/posttest.dat +++ b/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::server isc-dhcp-server stop 2> /dev/null +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