[PATCH 1/3] dhcp-server: Fix double free in l_dhcp_server_expire_by_mac

[PATCH 1/3] dhcp-server: Fix double free in l_dhcp_server_expire_by_mac

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1867 bytes --]

When using l_queue_foreach_remove() on lease_list we can't call
lease_release() because that will try to do l_queue_remove() on the same
queue, in addition to all the things we need it to do.
---
 ell/dhcp-server.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index f92807a..84dc41d 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -1418,25 +1418,42 @@ LIB_EXPORT bool l_dhcp_server_lease_remove(struct l_dhcp_server *server,
 struct dhcp_expire_by_mac_data {
 	struct l_dhcp_server *server;
 	const uint8_t *mac;
+	unsigned int expired_cnt;
 };
 
 static bool dhcp_expire_by_mac(void *data, void *user_data)
 {
 	struct l_dhcp_lease *lease = data;
 	struct dhcp_expire_by_mac_data *expire_data = user_data;
+	struct l_dhcp_server *server = expire_data->server;
 
 	if (!match_lease_mac(lease, expire_data->mac))
 		return false;
 
-	lease_release(expire_data->server, lease);
+	if (server->event_handler)
+		server->event_handler(server, L_DHCP_SERVER_EVENT_LEASE_EXPIRED,
+					server->user_data, lease);
+
+	if (!lease->offering) {
+		if (l_queue_length(server->expired_list) > server->max_expired)
+			_dhcp_lease_free(l_queue_pop_head(server->expired_list));
+
+		l_queue_push_tail(server->expired_list, lease);
+	} else
+		_dhcp_lease_free(lease);
+
+	expire_data->expired_cnt++;
 	return true;
 }
 
 LIB_EXPORT void l_dhcp_server_expire_by_mac(struct l_dhcp_server *server,
 						const uint8_t *mac)
 {
-	struct dhcp_expire_by_mac_data expire_data = { server, mac };
+	struct dhcp_expire_by_mac_data expire_data = { server, mac, 0 };
 
 	l_queue_foreach_remove(server->lease_list, dhcp_expire_by_mac,
 				&expire_data);
+
+	if (expire_data.expired_cnt)
+		set_next_expire_timer(server, NULL);
 }
-- 
2.30.2

[PATCH 2/3] dhcp-server: Validate chaddr against source MAC

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3538 bytes --]

In DHCP over ethernet -- the only thing we currently support -- the
chaddr in a DHCP message should always match the client's address, i.e.
the source address in messages from client to server.  On wifi, where
the client's frame source can't be easily spoofed, validating chaddr
may add a little security.  Some other DHCP servers support this too,
either on or off by default.
---
 ell/dhcp-private.h   |  3 ++-
 ell/dhcp-server.c    |  6 +++++-
 ell/dhcp-transport.c | 16 +++++++++++++---
 ell/dhcp.c           |  3 ++-
 4 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index 5a09f5e..bc10765 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -107,7 +107,8 @@ const char *_dhcp_option_to_string(uint8_t option);
 uint16_t _dhcp_checksum(const void *buf, size_t len);
 uint16_t _dhcp_checksumv(const struct iovec *iov, size_t iov_cnt);
 
-typedef void (*dhcp_transport_rx_cb_t)(const void *, size_t, void *);
+typedef void (*dhcp_transport_rx_cb_t)(const void *, size_t, void *,
+					const uint8_t *);
 
 struct dhcp_transport {
 	int (*open)(struct dhcp_transport *s, uint32_t xid);
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 84dc41d..05f9b73 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -705,7 +705,8 @@ static void send_ack(struct l_dhcp_server *server,
 					server->user_data, lease);
 }
 
-static void listener_event(const void *data, size_t len, void *user_data)
+static void listener_event(const void *data, size_t len, void *user_data,
+				const uint8_t *saddr)
 {
 	struct l_dhcp_server *server = user_data;
 	const struct dhcp_message *message = data;
@@ -722,6 +723,9 @@ static void listener_event(const void *data, size_t len, void *user_data)
 
 	SERVER_DEBUG("");
 
+	if (saddr && memcmp(saddr, message->chaddr, ETH_ALEN))
+		return;
+
 	if (!_dhcp_message_iter_init(&iter, message, len))
 		return;
 
diff --git a/ell/dhcp-transport.c b/ell/dhcp-transport.c
index 68108e2..7c73fee 100644
--- a/ell/dhcp-transport.c
+++ b/ell/dhcp-transport.c
@@ -111,8 +111,11 @@ static bool _dhcp_default_transport_read_handler(struct l_io *io,
 	ssize_t len;
 	struct dhcp_packet *p;
 	uint16_t c;
+	struct sockaddr_ll saddr;
+	socklen_t saddr_len = sizeof(saddr);
 
-	len = read(fd, buf, sizeof(buf));
+	len = recvfrom(fd, buf, sizeof(buf), 0, (struct sockaddr *) &saddr,
+			&saddr_len);
 	if (len < 0)
 		return false;
 
@@ -148,8 +151,15 @@ static bool _dhcp_default_transport_read_handler(struct l_io *io,
 
 	len -= sizeof(struct udphdr) - sizeof(struct iphdr);
 
-	if (transport->super.rx_cb)
-		transport->super.rx_cb(&p->dhcp, len, transport->super.rx_data);
+	if (transport->super.rx_cb) {
+		const uint8_t *src_mac = NULL;
+
+		if (saddr_len >= sizeof(saddr) && saddr.sll_halen == ETH_ALEN)
+			src_mac = saddr.sll_addr;
+
+		transport->super.rx_cb(&p->dhcp, len, transport->super.rx_data,
+					src_mac);
+	}
 
 	return true;
 }
diff --git a/ell/dhcp.c b/ell/dhcp.c
index 2efbf39..32e4a7c 100644
--- a/ell/dhcp.c
+++ b/ell/dhcp.c
@@ -775,7 +775,8 @@ static int dhcp_client_receive_offer(struct l_dhcp_client *client,
 	return 0;
 }
 
-static void dhcp_client_rx_message(const void *data, size_t len, void *userdata)
+static void dhcp_client_rx_message(const void *data, size_t len, void *userdata,
+					const uint8_t *saddr)
 {
 	struct l_dhcp_client *client = userdata;
 	const struct dhcp_message *message = data;
-- 
2.30.2

[PATCH 3/3] unit: In test-dhcp update rx callback parameters

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 3591 bytes --]

---
 unit/test-dhcp.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/unit/test-dhcp.c b/unit/test-dhcp.c
index 11ee10e..e502be7 100644
--- a/unit/test-dhcp.c
+++ b/unit/test-dhcp.c
@@ -721,7 +721,7 @@ static void test_discover(const void *data)
 	assert(dhcp_message_compare(discover_data_1, sizeof(discover_data_1),
 					client_packet, client_packet_len));
 
-	transport->rx_cb(offer_data_1, sizeof(offer_data_1), client);
+	transport->rx_cb(offer_data_1, sizeof(offer_data_1), client, NULL);
 
 	assert(client_send_called);
 	client_send_called = false;
@@ -729,7 +729,7 @@ static void test_discover(const void *data)
 					client_packet, client_packet_len));
 
 	event_handler_called = false;
-	transport->rx_cb(ack_data_1, sizeof(ack_data_1), client);
+	transport->rx_cb(ack_data_1, sizeof(ack_data_1), client, NULL);
 	assert(!client_send_called);
 	assert(event_handler_called);
 
@@ -829,30 +829,36 @@ static void client_connect(struct l_dhcp_client *client,
 					_dhcp_server_get_transport(server);
 	struct dhcp_transport *cli_transport =
 					_dhcp_client_get_transport(client);
+	uint8_t cli_addr[ETH_ALEN];
+	const struct dhcp_message *msg = (struct dhcp_message *) client_packet;
 
 	assert(l_dhcp_client_start(client));
 	assert(client_send_called);
 	client_send_called = false;
+	memcpy(cli_addr, msg->chaddr, ETH_ALEN);
 
 	/* RX DISCOVER */
-	srv_transport->rx_cb(client_packet, client_packet_len, server);
+	srv_transport->rx_cb(client_packet, client_packet_len, server,
+				cli_addr);
 	assert(l2_send_called);
 	l2_send_called = false;
 
 	if (!rapid_commit) {
 		/* RX OFFER */
-		cli_transport->rx_cb(server_packet, server_packet_len, client);
+		cli_transport->rx_cb(server_packet, server_packet_len, client,
+					NULL);
 		assert(client_send_called);
 		client_send_called = false;
 
 		/* RX REQUEST */
-		srv_transport->rx_cb(client_packet, client_packet_len, server);
+		srv_transport->rx_cb(client_packet, client_packet_len, server,
+					cli_addr);
 		assert(l2_send_called);
 		l2_send_called = false;
 	}
 
 	/* RX ACK */
-	cli_transport->rx_cb(server_packet, server_packet_len, client);
+	cli_transport->rx_cb(server_packet, server_packet_len, client, NULL);
 	assert(!client_send_called);
 
 	assert(event_handler_called);
@@ -966,7 +972,7 @@ static void test_complete_run(const void *data)
 	l_dhcp_client_stop(client1);
 	assert(client_send_called);
 	client_send_called = false;
-	srv_transport->rx_cb(client_packet, client_packet_len, server);
+	srv_transport->rx_cb(client_packet, client_packet_len, server, addr1);
 	assert(expired_client);
 	assert(!strcmp(expired_client, "192.168.1.2"));
 	l_free(expired_client);
@@ -975,7 +981,7 @@ static void test_complete_run(const void *data)
 	l_dhcp_client_stop(client2);
 	assert(client_send_called);
 	client_send_called = false;
-	srv_transport->rx_cb(client_packet, client_packet_len, server);
+	srv_transport->rx_cb(client_packet, client_packet_len, server, addr2);
 	assert(expired_client);
 	assert(!strcmp(expired_client, "192.168.1.3"));
 	l_free(expired_client);
@@ -1023,7 +1029,8 @@ static void test_expired_ip_reuse(const void *data)
 		l_dhcp_client_destroy(client);
 		assert(client_send_called);
 		client_send_called = false;
-		srv_transport->rx_cb(client_packet, client_packet_len, server);
+		srv_transport->rx_cb(client_packet, client_packet_len, server,
+					addr);
 		l_free(expired_client);
 		expired_client = NULL;
 	}
-- 
2.30.2

Re: [PATCH 1/3] dhcp-server: Fix double free in l_dhcp_server_expire_by_mac

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

Hi Andrew,

On 8/6/21 6:52 PM, Andrew Zaborowski wrote:
> When using l_queue_foreach_remove() on lease_list we can't call
> lease_release() because that will try to do l_queue_remove() on the same
> queue, in addition to all the things we need it to do.
> ---
>   ell/dhcp-server.c | 21 +++++++++++++++++++--
>   1 file changed, 19 insertions(+), 2 deletions(-)
> 

All applied, thanks.

Regards,
-Denis