6:23 p.m.
An authoritative DHCP server basically assumes that there are no other
DHCP servers on the network and replies to DHCPREQUEST messages
requesting IPs from outside of its subnet or with a wrong server ID
value. This speeds up connections from clients who try to use IP
leases they have cached from other networks and who start their IP
configuration by trying DHCPREQUESTs for those leases.
The term is not defined in the DHCP standard but is used by various
implementations. Default to authoritative being true. When set to
false, handle a DHCPREQUEST message directed at other servers by taking
it as meaning the client is declining our own lease offer as mandated
by RFC 2131 on page 16.
Refactor the checks in DHCPREQUEST handling quoting parts of the RFC.
The new checks are stricter and I'm not preserving some of the checks
that didn't seem justified, for example in the
"if (server_id_opt || !lease) ... send_nak()" block it didn't seem to
make sense to only reply to clients who have no lease (offered or
active) and ignore those who do have a lease with a different IP
address, the opposite would make more sense.
---
ell/dhcp-server.c | 109 ++++++++++++++++++++++++++++++++++++++++------
ell/dhcp.h | 2 +
ell/ell.sym | 1 +
3 files changed, 98 insertions(+), 14 deletions(-)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index bae0f10..6415872 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -87,6 +87,8 @@ struct l_dhcp_server {
struct dhcp_transport *transport;
struct l_acd *acd;
+
+ bool authoritative : 1;
};
#define MAC "%02x:%02x:%02x:%02x:%02x:%02x"
@@ -615,9 +617,6 @@ static void listener_event(const void *data, size_t len, void
*user_data)
if (l == 4)
server_id_opt = l_get_u32(v);
- if (server->address != server_id_opt)
- return;
-
break;
case L_DHCP_OPTION_REQUESTED_IP_ADDRESS:
if (l == 4)
@@ -640,6 +639,9 @@ static void listener_event(const void *data, size_t len, void
*user_data)
SERVER_DEBUG("Received DISCOVER, requested IP "NIPQUAD_FMT,
NIPQUAD(requested_ip_opt));
+ if (server_id_opt && server_id_opt != server->address)
+ break;
+
send_offer(server, message, lease, requested_ip_opt);
break;
@@ -647,27 +649,92 @@ static void listener_event(const void *data, size_t len, void
*user_data)
SERVER_DEBUG("Received REQUEST, requested IP "NIPQUAD_FMT,
NIPQUAD(requested_ip_opt));
- if (requested_ip_opt == 0) {
- requested_ip_opt = message->ciaddr;
- if (requested_ip_opt == 0)
+ /*
+ * RFC2131 Section 3.5: "Those servers not selected by the
+ * DHCPREQUEST message use the message as notification that
+ * the client has declined that server's offer."
+ */
+ if (server_id_opt && server_id_opt != server->address) {
+ if (server->authoritative) {
+ send_nak(server, message);
break;
- }
+ }
- if (lease && requested_ip_opt == lease->address) {
- send_ack(server, message, lease->address);
+ if (!lease || !lease->offering)
+ break;
+
+ remove_lease(server, lease);
break;
}
- if (server_id_opt || !lease) {
- send_nak(server, message);
+ /*
+ * RFC2131 Section 3.5: "If the selected server is unable to
+ * satisfy the DHCPREQUEST message (...), the server SHOULD
+ * respond with a DHCPNAK message."
+ *
+ * But:
+ * 4.3.2: "If the DHCP server has no record of this client,
+ * then it MUST remain silent (...)"
+ */
+ if (!lease) {
+ if (server_id_opt == server->address ||
+ server->authoritative)
+ send_nak(server, message);
+
break;
}
+
+ /*
+ * 4.3.2: "If the DHCPREQUEST message contains a 'server
+ * identifier' option, the message is in response to a
+ * DHCPOFFER message. Otherwise, the message is a request
+ * to verify or extend an existing lease."
+ */
+ if (server_id_opt == server->address) {
+ /*
+ * Allow either no 'requested IP address' option or
+ * a value identical with the one we offered because
+ * the spec is unclear on whether it is to be
+ * included:
+ *
+ * Section 4.3.2: "DHCPREQUEST generated during
+ * SELECTING state: (...) 'requested IP address' MUST
+ * be filled in with the yiaddr value from the chosen
+ * DHCPOFFER."
+ *
+ * but section 3.5 suggests only in the INIT-REBOOT
+ * state: "The 'requested IP address' option is to be
+ * filled in only in a DHCPREQUEST message when the
+ * client is verifying network parameters obtained
+ * previously."
+ */
+ if (!lease->offering ||
+ (requested_ip_opt &&
+ requested_ip_opt != lease->address)) {
+ send_nak(server, message);
+ break;
+ }
+ } else {
+ /*
+ * 3.5: "If a server receives a DHCPREQUEST message
+ * with an invalid 'requested IP address', the server
+ * SHOULD respond to the client with a DHCPNAK message"
+ */
+ if (lease->offering ||
+ (requested_ip_opt &&
+ requested_ip_opt != lease->address)) {
+ send_nak(server, message);
+ break;
+ }
+ }
+
+ send_ack(server, message, lease->address);
break;
case DHCP_MESSAGE_TYPE_DECLINE:
SERVER_DEBUG("Received DECLINE");
- if (!server_id_opt || !requested_ip_opt || !lease ||
- !lease->offering)
+ if (server_id_opt != server->address || !requested_ip_opt ||
+ !lease || !lease->offering)
break;
if (requested_ip_opt == lease->address)
@@ -677,7 +744,8 @@ static void listener_event(const void *data, size_t len, void
*user_data)
case DHCP_MESSAGE_TYPE_RELEASE:
SERVER_DEBUG("Received RELEASE");
- if (!server_id_opt || !lease || lease->offering)
+ if (server_id_opt != server->address || !lease ||
+ lease->offering)
break;
if (message->ciaddr == lease->address)
@@ -687,6 +755,9 @@ static void listener_event(const void *data, size_t len, void
*user_data)
case DHCP_MESSAGE_TYPE_INFORM:
SERVER_DEBUG("Received INFORM");
+ if (server_id_opt && server_id_opt != server->address)
+ break;
+
send_inform(server, message);
break;
}
@@ -732,6 +803,7 @@ LIB_EXPORT struct l_dhcp_server *l_dhcp_server_new(int ifindex)
server->expired_list = l_queue_new();
server->started = false;
+ server->authoritative = true;
server->lease_seconds = DEFAULT_DHCP_LEASE_SEC;
server->max_expired = MAX_EXPIRED_LEASES;
@@ -1053,6 +1125,15 @@ failed:
return false;
}
+LIB_EXPORT void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
+ bool authoritative)
+{
+ if (unlikely(!server))
+ return;
+
+ server->authoritative = authoritative;
+}
+
LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
struct l_dhcp_server *server,
uint32_t requested_ip_opt,
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 2804c78..830cbb1 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -141,6 +141,8 @@ bool l_dhcp_server_set_ip_address(struct l_dhcp_server *server,
bool l_dhcp_server_set_netmask(struct l_dhcp_server *server, const char *mask);
bool l_dhcp_server_set_gateway(struct l_dhcp_server *server, const char *ip);
bool l_dhcp_server_set_dns(struct l_dhcp_server *server, char **dns);
+void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
+ bool authoritative);
struct l_dhcp_lease *l_dhcp_server_discover(struct l_dhcp_server *server,
uint32_t requested_ip_opt,
diff --git a/ell/ell.sym b/ell/ell.sym
index 4d41448..3be10dc 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -259,6 +259,7 @@ global:
l_dhcp_server_set_netmask;
l_dhcp_server_set_gateway;
l_dhcp_server_set_dns;
+ l_dhcp_server_set_authoritative;
l_dhcp_server_discover;
l_dhcp_server_request;
l_dhcp_server_decline;
--
2.30.2
6:23 p.m.
New subject: [PATCH 2/8] dhcp-server: Handle DHCPDECLINE for active leases
RFC2131 Section 3.1 mentions a scenario where the client is supposed to
drop its lease using DHCPDECLINE after sending the DHCPREQUEST and
receiving the DHCPACK meaning that lease is no longer in the offering
state:
"5. The client receives the DHCPACK message with configuration
parameters. The client SHOULD perform a final check on the
parameters [...]. If the client detects that the address is already
in use (e.g., through the use of ARP), the client MUST send a
DHCPDECLINE message..."
---
ell/dhcp-server.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 6415872..9f4703c 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -734,7 +734,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
SERVER_DEBUG("Received DECLINE");
if (server_id_opt != server->address || !requested_ip_opt ||
- !lease || !lease->offering)
+ !lease)
break;
if (requested_ip_opt == lease->address)
--
2.30.2
6:23 p.m.
New subject: [PATCH 3/8] dhcp-server: Respect client's broadcast flag
Follow notes in RFC2131 4.1, 4.3.2 and 4.4.4 when deciding whether we
can use unicast for a given message. We want to prefer unicast when
possible but we should follow the spec on the broadcast flag and the
destination address.
---
ell/dhcp-private.h | 3 ++
ell/dhcp-server.c | 90 +++++++++++++++++++++++++++++++++++-----------
2 files changed, 72 insertions(+), 21 deletions(-)
diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index c422322..f761162 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -66,6 +66,9 @@ enum dhcp_option_overload {
#define DHCP_OPTION_MAXIMUM_MESSAGE_SIZE 57 /* Section 9.10 */
#define DHCP_OPTION_CLIENT_IDENTIFIER 61 /* Section 9.14 */
+/* RFC 2131, Figure 2 */
+#define DHCP_FLAG_BROADCAST (1 << 15)
+
/* RFC 2131, Figure 1 */
struct dhcp_message {
uint8_t op;
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 9f4703c..fb26e29 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -420,6 +420,71 @@ static void server_message_init(struct l_dhcp_server *server,
reply->ciaddr = client_msg->ciaddr;
}
+static bool server_message_send(struct l_dhcp_server *server,
+ struct dhcp_message *reply, size_t len,
+ uint8_t type)
+{
+ uint32_t daddr;
+ uint16_t dport;
+ const uint8_t *dest_mac;
+
+ /*
+ * RFC2131 Section 4.1: "If the 'giaddr' field in a DHCP message from
+ * a client is non-zero, the server sends any return messages to the
+ * 'DHCP server' port on the BOOTP relay agent whose address appears
+ * in 'giaddr'. If the 'giaddr' field is zero and the 'ciaddr'
field
+ * is nonzero, then the server unicasts DHCPOFFER and DHCPACK messages
+ * to the address in 'ciaddr'. If 'giaddr' is zero and 'ciaddr'
is
+ * zero, and the broadcast bit is set, then the server broadcasts
+ * DHCPOFFER and DHCPACK messages to 0xffffffff. If the broadcast bit
+ * is not set and 'giaddr' is zero and 'ciaddr' is zero, then the
+ * server unicasts DHCPOFFER and DHCPACK messages to the client's
+ * hardware address and 'yiaddr' address. In all cases, when 'giaddr'
+ * is zero, the server broadcasts any DHCPNAK messages to 0xffffffff."
+ *
+ * 4.3.2: "If 'giaddr' is set in the DHCPREQUEST message, the client
+ * is on a different subnet. The server MUST set the broadcast bit in
+ * the DHCPNAK, so that the relay agent will broadcast the DHCPNAK to
+ * the client, because the client may not have a correct network
+ * address or subnet mask, and the client may not be answering ARP
+ * requests."
+ */
+ if (reply->giaddr) {
+ dport = DHCP_PORT_SERVER;
+ daddr = reply->giaddr;
+ dest_mac = reply->chaddr;
+
+ if (type == DHCP_MESSAGE_TYPE_NAK)
+ reply->flags |= L_CPU_TO_BE16(DHCP_FLAG_BROADCAST);
+ } else {
+ dport = DHCP_PORT_CLIENT;
+
+ if (type == DHCP_MESSAGE_TYPE_NAK) {
+ daddr = 0xffffffff;
+ dest_mac = MAC_BCAST_ADDR;
+ } else if (reply->ciaddr) {
+ daddr = reply->ciaddr;
+ dest_mac = reply->chaddr;
+ } else if (L_BE16_TO_CPU(reply->flags) & DHCP_FLAG_BROADCAST) {
+ daddr = 0xffffffff;
+ dest_mac = MAC_BCAST_ADDR;
+ } else {
+ daddr = reply->yiaddr;
+ dest_mac = reply->chaddr;
+ }
+ }
+
+ if (server->transport->l2_send(server->transport, server->address,
+ DHCP_PORT_SERVER, daddr, dport,
+ dest_mac, reply, len) < 0) {
+ SERVER_DEBUG("Failed to send %s",
+ _dhcp_message_type_to_string(type));
+ return false;
+ }
+
+ return true;
+}
+
static void add_server_options(struct l_dhcp_server *server,
struct dhcp_message_builder *builder)
{
@@ -491,11 +556,7 @@ static void send_offer(struct l_dhcp_server *server,
SERVER_DEBUG("Sending OFFER of "NIPQUAD_FMT " to "MAC,
NIPQUAD(reply->yiaddr), MAC_STR(reply->chaddr));
- if (server->transport->l2_send(server->transport, server->address,
- DHCP_PORT_SERVER,
- reply->ciaddr, DHCP_PORT_CLIENT,
- reply->chaddr, reply, len) < 0)
- SERVER_DEBUG("Failed to send OFFER");
+ server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_OFFER);
}
static void send_inform(struct l_dhcp_server *server,
@@ -515,11 +576,7 @@ static void send_inform(struct l_dhcp_server *server,
_dhcp_message_builder_finalize(&builder, &len);
- if (server->transport->l2_send(server->transport, server->address,
- DHCP_PORT_SERVER, reply->ciaddr,
- DHCP_PORT_CLIENT, reply->chaddr,
- reply, len) < 0)
- SERVER_DEBUG("Failed to send INFORM");
+ server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_ACK);
}
static void send_nak(struct l_dhcp_server *server,
@@ -537,11 +594,7 @@ static void send_nak(struct l_dhcp_server *server,
_dhcp_message_builder_finalize(&builder, &len);
- if (server->transport->l2_send(server->transport, server->address,
- DHCP_PORT_SERVER, reply->ciaddr,
- DHCP_PORT_CLIENT, MAC_BCAST_ADDR,
- reply, len) < 0)
- SERVER_DEBUG("Failed to send NACK");
+ server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_NAK);
}
static void send_ack(struct l_dhcp_server *server,
@@ -574,13 +627,8 @@ static void send_ack(struct l_dhcp_server *server,
SERVER_DEBUG("Sending ACK to "NIPQUAD_FMT, NIPQUAD(reply->yiaddr));
- if (server->transport->l2_send(server->transport, server->address,
- DHCP_PORT_SERVER, reply->ciaddr,
- DHCP_PORT_CLIENT,
- reply->chaddr, reply, len) < 0) {
- SERVER_DEBUG("Failed to send ACK");
+ if (!server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_ACK))
return;
- }
lease = add_lease(server, false, reply->chaddr, reply->yiaddr);
--
2.30.2
3:47 p.m.
New subject: [PATCH 3/8] dhcp-server: Respect client's broadcast flag
Hi Andrew,
On 7/23/21 1:23 PM, Andrew Zaborowski wrote:
Follow notes in RFC2131 4.1, 4.3.2 and 4.4.4 when deciding whether
we
can use unicast for a given message. We want to prefer unicast when
possible but we should follow the spec on the broadcast flag and the
destination address.
---
ell/dhcp-private.h | 3 ++
ell/dhcp-server.c | 90 +++++++++++++++++++++++++++++++++++-----------
2 files changed, 72 insertions(+), 21 deletions(-)
diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index c422322..f761162 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -66,6 +66,9 @@ enum dhcp_option_overload {
#define DHCP_OPTION_MAXIMUM_MESSAGE_SIZE 57 /* Section 9.10 */
#define DHCP_OPTION_CLIENT_IDENTIFIER 61 /* Section 9.14 */
+/* RFC 2131, Figure 2 */
+#define DHCP_FLAG_BROADCAST (1 << 15)
+
/* RFC 2131, Figure 1 */
struct dhcp_message {
uint8_t op;
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 9f4703c..fb26e29 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -420,6 +420,71 @@ static void server_message_init(struct l_dhcp_server *server,
reply->ciaddr = client_msg->ciaddr;
}
+static bool server_message_send(struct l_dhcp_server *server,
+ struct dhcp_message *reply, size_t len,
+ uint8_t type)
+{
+ uint32_t daddr;
+ uint16_t dport;
+ const uint8_t *dest_mac;
+
+ /*
+ * RFC2131 Section 4.1: "If the 'giaddr' field in a DHCP message from
+ * a client is non-zero, the server sends any return messages to the
+ * 'DHCP server' port on the BOOTP relay agent whose address appears
+ * in 'giaddr'. If the 'giaddr' field is zero and the 'ciaddr'
field
+ * is nonzero, then the server unicasts DHCPOFFER and DHCPACK messages
+ * to the address in 'ciaddr'. If 'giaddr' is zero and
'ciaddr' is
+ * zero, and the broadcast bit is set, then the server broadcasts
+ * DHCPOFFER and DHCPACK messages to 0xffffffff. If the broadcast bit
+ * is not set and 'giaddr' is zero and 'ciaddr' is zero, then the
+ * server unicasts DHCPOFFER and DHCPACK messages to the client's
+ * hardware address and 'yiaddr' address. In all cases, when 'giaddr'
+ * is zero, the server broadcasts any DHCPNAK messages to 0xffffffff."
+ *
+ * 4.3.2: "If 'giaddr' is set in the DHCPREQUEST message, the client
+ * is on a different subnet. The server MUST set the broadcast bit in
+ * the DHCPNAK, so that the relay agent will broadcast the DHCPNAK to
+ * the client, because the client may not have a correct network
+ * address or subnet mask, and the client may not be answering ARP
+ * requests."
+ */
+ if (reply->giaddr) {
+ dport = DHCP_PORT_SERVER;
+ daddr = reply->giaddr;
+ dest_mac = reply->chaddr;
+
+ if (type == DHCP_MESSAGE_TYPE_NAK)
+ reply->flags |= L_CPU_TO_BE16(DHCP_FLAG_BROADCAST);
+ } else {
+ dport = DHCP_PORT_CLIENT;
+
+ if (type == DHCP_MESSAGE_TYPE_NAK) {
+ daddr = 0xffffffff;
+ dest_mac = MAC_BCAST_ADDR;
+ } else if (reply->ciaddr) {
+ daddr = reply->ciaddr;
+ dest_mac = reply->chaddr;
+ } else if (L_BE16_TO_CPU(reply->flags) & DHCP_FLAG_BROADCAST) {
+ daddr = 0xffffffff;
+ dest_mac = MAC_BCAST_ADDR;
+ } else {
+ daddr = reply->yiaddr;
+ dest_mac = reply->chaddr;
+ }
+ }
+
+ if (server->transport->l2_send(server->transport, server->address,
+ DHCP_PORT_SERVER, daddr, dport,
+ dest_mac, reply, len) < 0) {
+ SERVER_DEBUG("Failed to send %s",
+ _dhcp_message_type_to_string(type));
+ return false;
+ }
+
+ return true;
+}
+
static void add_server_options(struct l_dhcp_server *server,
struct dhcp_message_builder *builder)
{
@@ -491,11 +556,7 @@ static void send_offer(struct l_dhcp_server *server,
SERVER_DEBUG("Sending OFFER of "NIPQUAD_FMT " to "MAC,
NIPQUAD(reply->yiaddr), MAC_STR(reply->chaddr));
- if (server->transport->l2_send(server->transport, server->address,
- DHCP_PORT_SERVER,
- reply->ciaddr, DHCP_PORT_CLIENT,
- reply->chaddr, reply, len) < 0)
- SERVER_DEBUG("Failed to send OFFER");
+ server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_OFFER);
}
static void send_inform(struct l_dhcp_server *server,
@@ -515,11 +576,7 @@ static void send_inform(struct l_dhcp_server *server,
_dhcp_message_builder_finalize(&builder, &len);
- if (server->transport->l2_send(server->transport, server->address,
- DHCP_PORT_SERVER, reply->ciaddr,
- DHCP_PORT_CLIENT, reply->chaddr,
- reply, len) < 0)
- SERVER_DEBUG("Failed to send INFORM");
+ server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_ACK);
Inform?
}
static void send_nak(struct l_dhcp_server *server,
@@ -537,11 +594,7 @@ static void send_nak(struct l_dhcp_server *server,
_dhcp_message_builder_finalize(&builder, &len);
- if (server->transport->l2_send(server->transport, server->address,
- DHCP_PORT_SERVER, reply->ciaddr,
- DHCP_PORT_CLIENT, MAC_BCAST_ADDR,
- reply, len) < 0)
- SERVER_DEBUG("Failed to send NACK");
+ server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_NAK);
}
static void send_ack(struct l_dhcp_server *server,
@@ -574,13 +627,8 @@ static void send_ack(struct l_dhcp_server *server,
SERVER_DEBUG("Sending ACK to "NIPQUAD_FMT, NIPQUAD(reply->yiaddr));
- if (server->transport->l2_send(server->transport, server->address,
- DHCP_PORT_SERVER, reply->ciaddr,
- DHCP_PORT_CLIENT,
- reply->chaddr, reply, len) < 0) {
- SERVER_DEBUG("Failed to send ACK");
+ if (!server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_ACK))
return;
- }
lease = add_lease(server, false, reply->chaddr, reply->yiaddr);
Regards,
-Denis
5:57 p.m.
New subject: [PATCH 3/8] dhcp-server: Respect client's broadcast flag
On Wed, 28 Jul 2021 at 18:06, Denis Kenzior <denkenz(a)gmail.com> wrote:
On 7/23/21 1:23 PM, Andrew Zaborowski wrote:
> static void send_inform(struct l_dhcp_server *server,
> @@ -515,11 +576,7 @@ static void send_inform(struct l_dhcp_server *server,
>
> _dhcp_message_builder_finalize(&builder, &len);
>
> - if (server->transport->l2_send(server->transport,
server->address,
> - DHCP_PORT_SERVER, reply->ciaddr,
> - DHCP_PORT_CLIENT, reply->chaddr,
> - reply, len) < 0)
> - SERVER_DEBUG("Failed to send INFORM");
> + server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_ACK);
Inform?
Yep, right.
Best regards
6:23 p.m.
New subject: [PATCH 4/8] dhcp-lease: Check duplicate options in _dhcp_lease_parse_options
Guard against leaks if some of the options are duplicated in the message
being parsed in _dhcp_lease_parse_options.
---
ell/dhcp-lease.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/ell/dhcp-lease.c b/ell/dhcp-lease.c
index 94b67b4..ffed6d6 100644
--- a/ell/dhcp-lease.c
+++ b/ell/dhcp-lease.c
@@ -90,6 +90,9 @@ struct l_dhcp_lease *_dhcp_lease_parse_options(struct dhcp_message_iter
*iter)
lease->broadcast = l_get_u32(v);
break;
case L_DHCP_OPTION_DOMAIN_NAME_SERVER:
+ if (lease->dns)
+ goto error;
+
if (l >= 4 && !(l % 4)) {
unsigned i = 0;
@@ -105,7 +108,7 @@ struct l_dhcp_lease *_dhcp_lease_parse_options(struct
dhcp_message_iter *iter)
}
break;
case L_DHCP_OPTION_DOMAIN_NAME:
- if (l < 1 || l > 253)
+ if (l < 1 || l > 253 || lease->domain_name)
goto error;
/* Disallow embedded NUL bytes. */
--
2.30.2
3:50 p.m.
New subject: [PATCH 4/8] dhcp-lease: Check duplicate options in
_dhcp_lease_parse_options
Hi Andrew,
On 7/23/21 1:23 PM, Andrew Zaborowski wrote:
Guard against leaks if some of the options are duplicated in the
message
being parsed in _dhcp_lease_parse_options.
---
ell/dhcp-lease.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
Applied, thanks.
Regards,
-Denis
6:23 p.m.
New subject: [PATCH 5/8] dhcp-server: Look up leases by client identifier option
If the client identifier option is present in the client message, that
should be used for lease lookups, overriding the MAC value, according to
RFC2131. Parse that option and save the value for lookups.
---
ell/dhcp-lease.c | 9 ++++++
ell/dhcp-private.h | 1 +
ell/dhcp-server.c | 72 ++++++++++++++++++++++++++++++++--------------
ell/dhcp.h | 1 +
4 files changed, 62 insertions(+), 21 deletions(-)
diff --git a/ell/dhcp-lease.c b/ell/dhcp-lease.c
index ffed6d6..c596c6b 100644
--- a/ell/dhcp-lease.c
+++ b/ell/dhcp-lease.c
@@ -49,6 +49,7 @@ void _dhcp_lease_free(struct l_dhcp_lease *lease)
l_free(lease->dns);
l_free(lease->domain_name);
+ l_free(lease->client_id);
l_free(lease);
}
@@ -137,6 +138,14 @@ struct l_dhcp_lease *_dhcp_lease_parse_options(struct
dhcp_message_iter *iter)
if (l_net_hostname_is_localhost(lease->domain_name))
goto error;
+ break;
+ case DHCP_OPTION_CLIENT_IDENTIFIER:
+ if (l < 1 || l > 253 || lease->client_id)
+ goto error;
+
+ lease->client_id = l_malloc(l + 1);
+ lease->client_id[0] = l;
+ memcpy(lease->client_id + 1, v, l);
break;
default:
break;
diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index f761162..f9c0c6f 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -167,6 +167,7 @@ struct l_dhcp_lease {
char *domain_name;
/* for server */
uint8_t mac[6];
+ uint8_t *client_id;
/* set for an offered lease, but not ACK'ed */
bool offering : 1;
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index fb26e29..2d90f8b 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -114,6 +114,15 @@ static bool is_expired_lease(const struct l_dhcp_lease *lease)
return !l_time_after(get_lease_expiry_time(lease), l_time_now());
}
+static bool match_lease_client_id(const void *data, const void *user_data)
+{
+ const struct l_dhcp_lease *lease = data;
+ const uint8_t *client_id = user_data;
+
+ return lease->client_id &&
+ !memcmp(lease->client_id, client_id, client_id[0] + 1);
+}
+
static bool match_lease_mac(const void *data, const void *user_data)
{
const struct l_dhcp_lease *lease = data;
@@ -122,16 +131,21 @@ static bool match_lease_mac(const void *data, const void
*user_data)
return !memcmp(lease->mac, mac, 6);
}
-static struct l_dhcp_lease *find_lease_by_mac(struct l_dhcp_server *server,
+static struct l_dhcp_lease *find_lease_by_id(struct l_dhcp_server *server,
+ const uint8_t *client_id,
const uint8_t *mac)
{
+ if (client_id)
+ return l_queue_find(server->lease_list, match_lease_client_id,
+ client_id);
+
return l_queue_find(server->lease_list, match_lease_mac, mac);
}
/* Clear the old lease and create the new one */
static int get_lease(struct l_dhcp_server *server, uint32_t yiaddr,
- const uint8_t *mac,
- struct l_dhcp_lease **lease_out)
+ const uint8_t *client_id, const uint8_t *mac,
+ struct l_dhcp_lease **lease_out)
{
struct l_dhcp_lease *lease;
@@ -150,7 +164,7 @@ static int get_lease(struct l_dhcp_server *server, uint32_t yiaddr,
if (l_memeqzero(mac, ETH_ALEN))
return -ENXIO;
- lease = find_lease_by_mac(server, mac);
+ lease = find_lease_by_id(server, client_id, mac);
if (lease) {
l_queue_remove(server->lease_list, lease);
@@ -245,13 +259,13 @@ static void lease_expired_cb(struct l_timeout *timeout, void
*user_data)
}
static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
- bool offering, const uint8_t *chaddr,
- uint32_t yiaddr)
+ bool offering, const uint8_t *client_id,
+ const uint8_t *chaddr, uint32_t yiaddr)
{
struct l_dhcp_lease *lease = NULL;
int ret;
- ret = get_lease(server, yiaddr, chaddr, &lease);
+ ret = get_lease(server, yiaddr, client_id, chaddr, &lease);
if (ret != 0)
return NULL;
@@ -262,6 +276,9 @@ static struct l_dhcp_lease *add_lease(struct l_dhcp_server *server,
lease->subnet_mask = server->netmask;
lease->router = server->gateway;
+ if (client_id)
+ lease->client_id = l_memdup(client_id, client_id[0] + 1);
+
lease->offering = offering;
lease->bound_time = l_time_now();
@@ -509,7 +526,8 @@ static void add_server_options(struct l_dhcp_server *server,
static void send_offer(struct l_dhcp_server *server,
const struct dhcp_message *client_msg,
- struct l_dhcp_lease *lease, uint32_t requested_ip)
+ struct l_dhcp_lease *lease, uint32_t requested_ip,
+ const uint8_t *client_id)
{
struct dhcp_message_builder builder;
size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
@@ -531,7 +549,8 @@ static void send_offer(struct l_dhcp_server *server,
return;
}
- lease = add_lease(server, true, client_msg->chaddr, reply->yiaddr);
+ lease = add_lease(server, true, client_id, client_msg->chaddr,
+ reply->yiaddr);
if (!lease) {
SERVER_DEBUG("add_lease() failed");
return;
@@ -598,13 +617,13 @@ static void send_nak(struct l_dhcp_server *server,
}
static void send_ack(struct l_dhcp_server *server,
- const struct dhcp_message *client_msg, uint32_t dest)
+ const struct dhcp_message *client_msg,
+ const struct l_dhcp_lease *lease)
{
struct dhcp_message_builder builder;
size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
L_AUTO_FREE_VAR(struct dhcp_message *, reply);
uint32_t lease_time = L_CPU_TO_BE32(server->lease_seconds);
- struct l_dhcp_lease *lease;
reply = (struct dhcp_message *) l_new(uint8_t, len);
@@ -612,7 +631,7 @@ static void send_ack(struct l_dhcp_server *server,
_dhcp_message_builder_init(&builder, reply, len, DHCP_MESSAGE_TYPE_ACK);
- reply->yiaddr = dest;
+ reply->yiaddr = lease->address;
_dhcp_message_builder_append(&builder,
L_DHCP_OPTION_IP_ADDRESS_LEASE_TIME,
@@ -630,7 +649,8 @@ static void send_ack(struct l_dhcp_server *server,
if (!server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_ACK))
return;
- lease = add_lease(server, false, reply->chaddr, reply->yiaddr);
+ lease = add_lease(server, false, lease->client_id, reply->chaddr,
+ reply->yiaddr);
if (server->event_handler)
server->event_handler(server, L_DHCP_SERVER_EVENT_NEW_LEASE,
@@ -648,6 +668,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
uint8_t type = 0;
uint32_t server_id_opt = 0;
uint32_t requested_ip_opt = 0;
+ L_AUTO_FREE_VAR(uint8_t *, client_id_opt) = NULL;
SERVER_DEBUG("");
@@ -671,13 +692,21 @@ static void listener_event(const void *data, size_t len, void
*user_data)
requested_ip_opt = l_get_u32(v);
break;
+ case DHCP_OPTION_CLIENT_IDENTIFIER:
+ if (l < 1 || l > 253 || client_id_opt)
+ break;
+
+ client_id_opt = l_malloc(l + 1);
+ client_id_opt[0] = l;
+ memcpy(client_id_opt + 1, v, l);
+ break;
}
}
if (type == 0)
return;
- lease = find_lease_by_mac(server, message->chaddr);
+ lease = find_lease_by_id(server, client_id_opt, message->chaddr);
if (!lease)
SERVER_DEBUG("No lease found for "MAC,
MAC_STR(message->chaddr));
@@ -690,8 +719,8 @@ static void listener_event(const void *data, size_t len, void
*user_data)
if (server_id_opt && server_id_opt != server->address)
break;
- send_offer(server, message, lease, requested_ip_opt);
-
+ send_offer(server, message, lease, requested_ip_opt,
+ client_id_opt);
break;
case DHCP_MESSAGE_TYPE_REQUEST:
SERVER_DEBUG("Received REQUEST, requested IP "NIPQUAD_FMT,
@@ -776,7 +805,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
}
}
- send_ack(server, message, lease->address);
+ send_ack(server, message, lease);
break;
case DHCP_MESSAGE_TYPE_DECLINE:
SERVER_DEBUG("Received DECLINE");
@@ -1185,6 +1214,7 @@ LIB_EXPORT void l_dhcp_server_set_authoritative(struct l_dhcp_server
*server,
LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
struct l_dhcp_server *server,
uint32_t requested_ip_opt,
+ const uint8_t *client_id,
const uint8_t *mac)
{
struct l_dhcp_lease *lease;
@@ -1192,7 +1222,7 @@ LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
SERVER_DEBUG("Requested IP " NIPQUAD_FMT " for " MAC,
NIPQUAD(requested_ip_opt), MAC_STR(mac));
- if ((lease = find_lease_by_mac(server, mac)))
+ if ((lease = find_lease_by_id(server, client_id, mac)))
requested_ip_opt = lease->address;
else if (!check_requested_ip(server, requested_ip_opt)) {
requested_ip_opt = find_free_or_expired_ip(server, mac);
@@ -1203,7 +1233,7 @@ LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
}
}
- lease = add_lease(server, true, mac, requested_ip_opt);
+ lease = add_lease(server, true, client_id, mac, requested_ip_opt);
if (unlikely(!lease)) {
SERVER_DEBUG("add_lease() failed");
return NULL;
@@ -1223,7 +1253,7 @@ LIB_EXPORT bool l_dhcp_server_request(struct l_dhcp_server *server,
SERVER_DEBUG("Requested IP " NIPQUAD_FMT " for " MAC,
NIPQUAD(lease->address), MAC_STR(lease->mac));
- lease = add_lease(server, false, lease->mac, lease->address);
+ lease = add_lease(server, false, NULL, lease->mac, lease->address);
if (server->event_handler)
server->event_handler(server, L_DHCP_SERVER_EVENT_NEW_LEASE,
@@ -1276,7 +1306,7 @@ LIB_EXPORT bool l_dhcp_server_lease_remove(struct l_dhcp_server
*server,
LIB_EXPORT void l_dhcp_server_expire_by_mac(struct l_dhcp_server *server,
const uint8_t *mac)
{
- struct l_dhcp_lease *lease = find_lease_by_mac(server, mac);
+ struct l_dhcp_lease *lease = find_lease_by_id(server, NULL, mac);
if (likely(lease))
lease_release(server, lease);
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 830cbb1..58deb7e 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -146,6 +146,7 @@ void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
struct l_dhcp_lease *l_dhcp_server_discover(struct l_dhcp_server *server,
uint32_t requested_ip_opt,
+ const uint8_t *client_id,
const uint8_t *mac);
bool l_dhcp_server_request(struct l_dhcp_server *server,
struct l_dhcp_lease *lease);
--
2.30.2
6:23 p.m.
New subject: [PATCH 6/8] dhcp-server: Copy client identifier from the client message
As mandated by RFC 6842 copy the client identifier option from the
client message we're replying to into the reply message.
---
ell/dhcp-server.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 2d90f8b..21adcab 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -524,6 +524,16 @@ static void add_server_options(struct l_dhcp_server *server,
}
}
+/* Copy the client identifier option from the client message per RFC6842 */
+static void copy_client_id(struct dhcp_message_builder *builder,
+ const uint8_t *client_id)
+{
+ if (client_id)
+ _dhcp_message_builder_append(builder,
+ DHCP_OPTION_CLIENT_IDENTIFIER,
+ client_id[0], client_id + 1);
+}
+
static void send_offer(struct l_dhcp_server *server,
const struct dhcp_message *client_msg,
struct l_dhcp_lease *lease, uint32_t requested_ip,
@@ -569,6 +579,7 @@ static void send_offer(struct l_dhcp_server *server,
4, &server->address);
add_server_options(server, &builder);
+ copy_client_id(&builder, client_id);
_dhcp_message_builder_finalize(&builder, &len);
@@ -579,7 +590,8 @@ static void send_offer(struct l_dhcp_server *server,
}
static void send_inform(struct l_dhcp_server *server,
- const struct dhcp_message *client_msg)
+ const struct dhcp_message *client_msg,
+ const uint8_t *client_id)
{
struct dhcp_message_builder builder;
size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
@@ -592,6 +604,7 @@ static void send_inform(struct l_dhcp_server *server,
_dhcp_message_builder_init(&builder, reply, len, DHCP_MESSAGE_TYPE_ACK);
add_server_options(server, &builder);
+ copy_client_id(&builder, client_id);
_dhcp_message_builder_finalize(&builder, &len);
@@ -599,7 +612,8 @@ static void send_inform(struct l_dhcp_server *server,
}
static void send_nak(struct l_dhcp_server *server,
- const struct dhcp_message *client_msg)
+ const struct dhcp_message *client_msg,
+ const uint8_t *client_id)
{
struct dhcp_message_builder builder;
size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
@@ -610,7 +624,7 @@ static void send_nak(struct l_dhcp_server *server,
server_message_init(server, client_msg, reply);
_dhcp_message_builder_init(&builder, reply, len, DHCP_MESSAGE_TYPE_NAK);
-
+ copy_client_id(&builder, client_id);
_dhcp_message_builder_finalize(&builder, &len);
server_message_send(server, reply, len, DHCP_MESSAGE_TYPE_NAK);
@@ -638,6 +652,7 @@ static void send_ack(struct l_dhcp_server *server,
4, &lease_time);
add_server_options(server, &builder);
+ copy_client_id(&builder, lease->client_id);
_dhcp_message_builder_append(&builder, L_DHCP_OPTION_SERVER_IDENTIFIER,
4, &server->address);
@@ -733,7 +748,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
*/
if (server_id_opt && server_id_opt != server->address) {
if (server->authoritative) {
- send_nak(server, message);
+ send_nak(server, message, client_id_opt);
break;
}
@@ -756,7 +771,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
if (!lease) {
if (server_id_opt == server->address ||
server->authoritative)
- send_nak(server, message);
+ send_nak(server, message, client_id_opt);
break;
}
@@ -788,7 +803,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
if (!lease->offering ||
(requested_ip_opt &&
requested_ip_opt != lease->address)) {
- send_nak(server, message);
+ send_nak(server, message, client_id_opt);
break;
}
} else {
@@ -800,7 +815,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
if (lease->offering ||
(requested_ip_opt &&
requested_ip_opt != lease->address)) {
- send_nak(server, message);
+ send_nak(server, message, client_id_opt);
break;
}
}
@@ -835,7 +850,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
if (server_id_opt && server_id_opt != server->address)
break;
- send_inform(server, message);
+ send_inform(server, message, client_id_opt);
break;
}
}
--
2.30.2
6:23 p.m.
New subject: [PATCH 7/8] dhcp-server: Support RFC4039 Rapid Commit
Handle the RFC4039 Rapid Commit option in a DHCPDISCOVERY by directly
creating an active lease and replying with a DHCPACK, i.e. skipping the
DHCPOFFER and DHCPREQUEST roundtrip.
Currently enabled unconditionally, the support can be made optional if
needed in the future.
---
ell/dhcp-private.h | 1 +
ell/dhcp-server.c | 26 ++++++++++++++++++++++++--
ell/dhcp-util.c | 4 +++-
3 files changed, 28 insertions(+), 3 deletions(-)
diff --git a/ell/dhcp-private.h b/ell/dhcp-private.h
index f9c0c6f..5a09f5e 100644
--- a/ell/dhcp-private.h
+++ b/ell/dhcp-private.h
@@ -65,6 +65,7 @@ enum dhcp_option_overload {
#define DHCP_OPTION_PARAMETER_REQUEST_LIST 55 /* Section 9.8 */
#define DHCP_OPTION_MAXIMUM_MESSAGE_SIZE 57 /* Section 9.10 */
#define DHCP_OPTION_CLIENT_IDENTIFIER 61 /* Section 9.14 */
+#define DHCP_OPTION_RAPID_COMMIT 80 /* RFC 4039 Section 4 */
/* RFC 2131, Figure 2 */
#define DHCP_FLAG_BROADCAST (1 << 15)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 21adcab..265d52e 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -632,7 +632,8 @@ static void send_nak(struct l_dhcp_server *server,
static void send_ack(struct l_dhcp_server *server,
const struct dhcp_message *client_msg,
- const struct l_dhcp_lease *lease)
+ const struct l_dhcp_lease *lease,
+ bool rapid_commit)
{
struct dhcp_message_builder builder;
size_t len = sizeof(struct dhcp_message) + DHCP_MIN_OPTIONS_SIZE;
@@ -657,6 +658,10 @@ static void send_ack(struct l_dhcp_server *server,
_dhcp_message_builder_append(&builder, L_DHCP_OPTION_SERVER_IDENTIFIER,
4, &server->address);
+ if (rapid_commit)
+ _dhcp_message_builder_append(&builder, DHCP_OPTION_RAPID_COMMIT,
+ 0, "");
+
_dhcp_message_builder_finalize(&builder, &len);
SERVER_DEBUG("Sending ACK to "NIPQUAD_FMT, NIPQUAD(reply->yiaddr));
@@ -684,6 +689,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
uint32_t server_id_opt = 0;
uint32_t requested_ip_opt = 0;
L_AUTO_FREE_VAR(uint8_t *, client_id_opt) = NULL;
+ bool rapid_commit_opt = false;
SERVER_DEBUG("");
@@ -715,6 +721,9 @@ static void listener_event(const void *data, size_t len, void
*user_data)
client_id_opt[0] = l;
memcpy(client_id_opt + 1, v, l);
break;
+ case DHCP_OPTION_RAPID_COMMIT:
+ rapid_commit_opt = true;
+ break;
}
}
@@ -734,6 +743,19 @@ static void listener_event(const void *data, size_t len, void
*user_data)
if (server_id_opt && server_id_opt != server->address)
break;
+ if (rapid_commit_opt) {
+ lease = l_dhcp_server_discover(server, requested_ip_opt,
+ client_id_opt,
+ message->chaddr);
+ if (!lease) {
+ send_nak(server, message, client_id_opt);
+ break;
+ }
+
+ send_ack(server, message, lease, rapid_commit_opt);
+ break;
+ }
+
send_offer(server, message, lease, requested_ip_opt,
client_id_opt);
break;
@@ -820,7 +842,7 @@ static void listener_event(const void *data, size_t len, void
*user_data)
}
}
- send_ack(server, message, lease);
+ send_ack(server, message, lease, false);
break;
case DHCP_MESSAGE_TYPE_DECLINE:
SERVER_DEBUG("Received DECLINE");
diff --git a/ell/dhcp-util.c b/ell/dhcp-util.c
index d4d715a..e5c88b2 100644
--- a/ell/dhcp-util.c
+++ b/ell/dhcp-util.c
@@ -73,7 +73,9 @@ bool _dhcp_message_builder_append(struct dhcp_message_builder *builder,
builder->pos[0] = code;
builder->pos[1] = optlen;
- memcpy(builder->pos + 2, optval, optlen);
+
+ if (optlen)
+ memcpy(builder->pos + 2, optval, optlen);
builder->pos += optlen + 2;
break;
--
2.30.2
6:23 p.m.
New subject: [PATCH 8/8] dhcp: Support RFC4039 Rapid Commit
Use the RFC4039 Rapid Commit option in the DHCPDISCOVERY to attempt to
get the server to directly reply with a DHCPACK, i.e. skipping the
DHCPOFFER and DHCPREQUEST roundtrip.
Currently enabled unconditionally, the support can be made optional if
needed in the future.
---
ell/dhcp.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/ell/dhcp.c b/ell/dhcp.c
index 115325e..2efbf39 100644
--- a/ell/dhcp.c
+++ b/ell/dhcp.c
@@ -361,6 +361,8 @@ static int dhcp_client_send_discover(struct l_dhcp_client *client)
client->hostname))
return -EINVAL;
+ _dhcp_message_builder_append(&builder, DHCP_OPTION_RAPID_COMMIT,
+ 0, "");
_dhcp_message_builder_finalize(&builder, &len);
return client->transport->l2_send(client->transport,
@@ -815,6 +817,17 @@ static void dhcp_client_rx_message(const void *data, size_t len, void
*userdata)
case DHCP_STATE_INIT:
return;
case DHCP_STATE_SELECTING:
+ if (msg_type == DHCP_MESSAGE_TYPE_ACK) {
+ _dhcp_message_iter_init(&iter, message, len);
+
+ while (_dhcp_message_iter_next(&iter, &t, &l, &v))
+ if (t == DHCP_OPTION_RAPID_COMMIT) {
+ CLIENT_ENTER_STATE(
+ DHCP_STATE_REQUESTING);
+ goto receive_rapid_commit;
+ }
+ }
+
if (msg_type != DHCP_MESSAGE_TYPE_OFFER)
return;
@@ -835,6 +848,7 @@ static void dhcp_client_rx_message(const void *data, size_t len, void
*userdata)
case DHCP_STATE_REQUESTING:
case DHCP_STATE_RENEWING:
case DHCP_STATE_REBINDING:
+ receive_rapid_commit:
if (msg_type == DHCP_MESSAGE_TYPE_NAK) {
CLIENT_DEBUG("Received NAK, Stopping...");
l_dhcp_client_stop(client);
--
2.30.2
3:42 p.m.
Hi Andrew,
On 7/23/21 1:23 PM, Andrew Zaborowski wrote:
An authoritative DHCP server basically assumes that there are no
other
DHCP servers on the network and replies to DHCPREQUEST messages
requesting IPs from outside of its subnet or with a wrong server ID
value. This speeds up connections from clients who try to use IP
leases they have cached from other networks and who start their IP
configuration by trying DHCPREQUESTs for those leases.
The term is not defined in the DHCP standard but is used by various
implementations. Default to authoritative being true. When set to
false, handle a DHCPREQUEST message directed at other servers by taking
it as meaning the client is declining our own lease offer as mandated
by RFC 2131 on page 16.
Refactor the checks in DHCPREQUEST handling quoting parts of the RFC.
The new checks are stricter and I'm not preserving some of the checks
that didn't seem justified, for example in the
"if (server_id_opt || !lease) ... send_nak()" block it didn't seem to
make sense to only reply to clients who have no lease (offered or
active) and ignore those who do have a lease with a different IP
address, the opposite would make more sense.
It seems like this is supposed to be handling the init-reboot, bound, renewing,
rebinding states where server_id isn't present...?
---
ell/dhcp-server.c | 109 ++++++++++++++++++++++++++++++++++++++++------
ell/dhcp.h | 2 +
ell/ell.sym | 1 +
3 files changed, 98 insertions(+), 14 deletions(-)
diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index bae0f10..6415872 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -87,6 +87,8 @@ struct l_dhcp_server {
struct dhcp_transport *transport;
struct l_acd *acd;
+
+ bool authoritative : 1;
};
#define MAC "%02x:%02x:%02x:%02x:%02x:%02x"
@@ -615,9 +617,6 @@ static void listener_event(const void *data, size_t len, void
*user_data)
if (l == 4)
server_id_opt = l_get_u32(v);
- if (server->address != server_id_opt)
- return;
-
break;
case L_DHCP_OPTION_REQUESTED_IP_ADDRESS:
if (l == 4)
@@ -640,6 +639,9 @@ static void listener_event(const void *data, size_t len, void
*user_data)
SERVER_DEBUG("Received DISCOVER, requested IP "NIPQUAD_FMT,
NIPQUAD(requested_ip_opt));
+ if (server_id_opt && server_id_opt != server->address)
+ break;
+
So strictly speaking, according to DHCPDISCOVER from the client shouldn't even
have this option...
send_offer(server, message, lease, requested_ip_opt);
break;
@@ -647,27 +649,92 @@ static void listener_event(const void *data, size_t len, void
*user_data)
SERVER_DEBUG("Received REQUEST, requested IP "NIPQUAD_FMT,
NIPQUAD(requested_ip_opt));
- if (requested_ip_opt == 0) {
- requested_ip_opt = message->ciaddr;
- if (requested_ip_opt == 0)
+ /*
+ * RFC2131 Section 3.5: "Those servers not selected by the
+ * DHCPREQUEST message use the message as notification that
+ * the client has declined that server's offer."
+ */
+ if (server_id_opt && server_id_opt != server->address) {
+ if (server->authoritative) {
+ send_nak(server, message);
break;
So you send a NAK here if server_id_opt is provided and doesn't match...
- }
+ }
- if (lease && requested_ip_opt == lease->address) {
- send_ack(server, message, lease->address);
+ if (!lease || !lease->offering)
+ break;
+
+ remove_lease(server, lease);
break;
}
- if (server_id_opt || !lease) {
- send_nak(server, message);
+ /*
+ * RFC2131 Section 3.5: "If the selected server is unable to
+ * satisfy the DHCPREQUEST message (...), the server SHOULD
+ * respond with a DHCPNAK message."
+ *
+ * But:
+ * 4.3.2: "If the DHCP server has no record of this client,
+ * then it MUST remain silent (...)"
+ */
+ if (!lease) {
+ if (server_id_opt == server->address ||
+ server->authoritative)
So why check server->authoritative here? It seems like you can only get to this
point if the server_id_opt is provided and matches, or not provided at all. The
'server_id_opt ==' part also seems unsafe for those cases where server_id
isn't
provided at all...?
+ send_nak(server, message);
+
break;
}
+
+ /*
+ * 4.3.2: "If the DHCPREQUEST message contains a 'server
+ * identifier' option, the message is in response to a
+ * DHCPOFFER message. Otherwise, the message is a request
+ * to verify or extend an existing lease."
+ */
+ if (server_id_opt == server->address) {
Same here, checking server_id_opt seems unsafe?
+ /*
+ * Allow either no 'requested IP address' option or
+ * a value identical with the one we offered because
+ * the spec is unclear on whether it is to be
+ * included:
+ *
+ * Section 4.3.2: "DHCPREQUEST generated during
+ * SELECTING state: (...) 'requested IP address' MUST
+ * be filled in with the yiaddr value from the chosen
+ * DHCPOFFER."
+ *
+ * but section 3.5 suggests only in the INIT-REBOOT
+ * state: "The 'requested IP address' option is to be
+ * filled in only in a DHCPREQUEST message when the
+ * client is verifying network parameters obtained
+ * previously."
+ */
+ if (!lease->offering ||
+ (requested_ip_opt &&
+ requested_ip_opt != lease->address)) {
+ send_nak(server, message);
+ break;
+ }
+ } else {
+ /*
+ * 3.5: "If a server receives a DHCPREQUEST message
+ * with an invalid 'requested IP address', the server
+ * SHOULD respond to the client with a DHCPNAK message"
+ */
+ if (lease->offering ||
+ (requested_ip_opt &&
+ requested_ip_opt != lease->address)) {
+ send_nak(server, message);
+ break;
+ }
+ }
+
+ send_ack(server, message, lease->address);
break;
case DHCP_MESSAGE_TYPE_DECLINE:
SERVER_DEBUG("Received DECLINE");
- if (!server_id_opt || !requested_ip_opt || !lease ||
- !lease->offering)
+ if (server_id_opt != server->address || !requested_ip_opt ||
+ !lease || !lease->offering)
This also seems unsafe? server_id must be present, but you're not checking this
here?
break;
if (requested_ip_opt == lease->address)
@@ -677,7 +744,8 @@ static void listener_event(const void *data, size_t len, void
*user_data)
case DHCP_MESSAGE_TYPE_RELEASE:
SERVER_DEBUG("Received RELEASE");
- if (!server_id_opt || !lease || lease->offering)
+ if (server_id_opt != server->address || !lease ||
+ lease->offering)
Or here?
break;
if (message->ciaddr == lease->address)
@@ -687,6 +755,9 @@ static void listener_event(const void *data, size_t len, void
*user_data)
case DHCP_MESSAGE_TYPE_INFORM:
SERVER_DEBUG("Received INFORM");
+ if (server_id_opt && server_id_opt != server->address)
+ break;
+
send_inform(server, message);
break;
}
@@ -732,6 +803,7 @@ LIB_EXPORT struct l_dhcp_server *l_dhcp_server_new(int ifindex)
server->expired_list = l_queue_new();
server->started = false;
+ server->authoritative = true;
server->lease_seconds = DEFAULT_DHCP_LEASE_SEC;
server->max_expired = MAX_EXPIRED_LEASES;
@@ -1053,6 +1125,15 @@ failed:
return false;
}
+LIB_EXPORT void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
+ bool authoritative)
+{
+ if (unlikely(!server))
+ return;
+
+ server->authoritative = authoritative;
+}
+
LIB_EXPORT struct l_dhcp_lease *l_dhcp_server_discover(
struct l_dhcp_server *server,
uint32_t requested_ip_opt,
diff --git a/ell/dhcp.h b/ell/dhcp.h
index 2804c78..830cbb1 100644
--- a/ell/dhcp.h
+++ b/ell/dhcp.h
@@ -141,6 +141,8 @@ bool l_dhcp_server_set_ip_address(struct l_dhcp_server *server,
bool l_dhcp_server_set_netmask(struct l_dhcp_server *server, const char *mask);
bool l_dhcp_server_set_gateway(struct l_dhcp_server *server, const char *ip);
bool l_dhcp_server_set_dns(struct l_dhcp_server *server, char **dns);
+void l_dhcp_server_set_authoritative(struct l_dhcp_server *server,
+ bool authoritative);
struct l_dhcp_lease *l_dhcp_server_discover(struct l_dhcp_server *server,
uint32_t requested_ip_opt,
diff --git a/ell/ell.sym b/ell/ell.sym
index 4d41448..3be10dc 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -259,6 +259,7 @@ global:
l_dhcp_server_set_netmask;
l_dhcp_server_set_gateway;
l_dhcp_server_set_dns;
+ l_dhcp_server_set_authoritative;
l_dhcp_server_discover;
l_dhcp_server_request;
l_dhcp_server_decline;
Regards,
-Denis
5:51 p.m.
Hi Denis,
On Wed, 28 Jul 2021 at 18:01, Denis Kenzior <denkenz(a)gmail.com> wrote:
On 7/23/21 1:23 PM, Andrew Zaborowski wrote:
> An authoritative DHCP server basically assumes that there are no other
> DHCP servers on the network and replies to DHCPREQUEST messages
> requesting IPs from outside of its subnet or with a wrong server ID
> value. This speeds up connections from clients who try to use IP
> leases they have cached from other networks and who start their IP
> configuration by trying DHCPREQUESTs for those leases.
>
> The term is not defined in the DHCP standard but is used by various
> implementations. Default to authoritative being true. When set to
> false, handle a DHCPREQUEST message directed at other servers by taking
> it as meaning the client is declining our own lease offer as mandated
> by RFC 2131 on page 16.
>
> Refactor the checks in DHCPREQUEST handling quoting parts of the RFC.
> The new checks are stricter and I'm not preserving some of the checks
> that didn't seem justified, for example in the
> "if (server_id_opt || !lease) ... send_nak()" block it didn't seem to
> make sense to only reply to clients who have no lease (offered or
> active) and ignore those who do have a lease with a different IP
> address, the opposite would make more sense.
It seems like this is supposed to be handling the init-reboot, bound, renewing,
rebinding states where server_id isn't present...?
We were checking the opposite, i.e. we were sending a NAK if it *was* present.
However I was referring to the "!lease" part which seemed to be the
opposite of what was desired because we were supposed to remain silent
if we had no previous knowledge of the client.
> ---
> ell/dhcp-server.c | 109 ++++++++++++++++++++++++++++++++++++++++------
> ell/dhcp.h | 2 +
> ell/ell.sym | 1 +
> 3 files changed, 98 insertions(+), 14 deletions(-)
>
> diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
> index bae0f10..6415872 100644
> --- a/ell/dhcp-server.c
> +++ b/ell/dhcp-server.c
> @@ -87,6 +87,8 @@ struct l_dhcp_server {
> struct dhcp_transport *transport;
>
> struct l_acd *acd;
> +
> + bool authoritative : 1;
> };
>
> #define MAC "%02x:%02x:%02x:%02x:%02x:%02x"
> @@ -615,9 +617,6 @@ static void listener_event(const void *data, size_t len, void
*user_data)
> if (l == 4)
> server_id_opt = l_get_u32(v);
>
> - if (server->address != server_id_opt)
> - return;
> -
> break;
> case L_DHCP_OPTION_REQUESTED_IP_ADDRESS:
> if (l == 4)
> @@ -640,6 +639,9 @@ static void listener_event(const void *data, size_t len, void
*user_data)
> SERVER_DEBUG("Received DISCOVER, requested IP "NIPQUAD_FMT,
> NIPQUAD(requested_ip_opt));
>
> + if (server_id_opt && server_id_opt != server->address)
> + break;
> +
So strictly speaking, according to DHCPDISCOVER from the client shouldn't even
have this option...
Ok, I can make this check stricter here. I preserved the original
logic where we would ignore the whole message if (erver_id_opt !=
server->address) but otherwise we weren't checking whether the
server_id was correct or absent.
> send_offer(server, message, lease, requested_ip_opt);
>
> break;
> @@ -647,27 +649,92 @@ static void listener_event(const void *data, size_t len, void
*user_data)
> SERVER_DEBUG("Received REQUEST, requested IP "NIPQUAD_FMT,
> NIPQUAD(requested_ip_opt));
>
> - if (requested_ip_opt == 0) {
> - requested_ip_opt = message->ciaddr;
> - if (requested_ip_opt == 0)
> + /*
> + * RFC2131 Section 3.5: "Those servers not selected by the
> + * DHCPREQUEST message use the message as notification that
> + * the client has declined that server's offer."
> + */
> + if (server_id_opt && server_id_opt != server->address) {
> + if (server->authoritative) {
> + send_nak(server, message);
> break;
So you send a NAK here if server_id_opt is provided and doesn't match...
> - }
> + }
>
> - if (lease && requested_ip_opt == lease->address) {
> - send_ack(server, message, lease->address);
> + if (!lease || !lease->offering)
> + break;
> +
> + remove_lease(server, lease);
> break;
> }
>
> - if (server_id_opt || !lease) {
> - send_nak(server, message);
> + /*
> + * RFC2131 Section 3.5: "If the selected server is unable to
> + * satisfy the DHCPREQUEST message (...), the server SHOULD
> + * respond with a DHCPNAK message."
> + *
> + * But:
> + * 4.3.2: "If the DHCP server has no record of this client,
> + * then it MUST remain silent (...)"
> + */
> + if (!lease) {
> + if (server_id_opt == server->address ||
> + server->authoritative)
So why check server->authoritative here? It seems like you can only get to this
point if the server_id_opt is provided and matches, or not provided at all.
Right, I guess I could have simply written (server_id_opt ||
server->authoritative) beause as you noted, we've already checked that
if server_id was present, it was correct.
The server->authoritative part is so that we send a NAK if no
server_id was provided and we know we're the only server on the
network.
The
'server_id_opt ==' part also seems unsafe for those cases where server_id
isn't
provided at all...?
server_id_opt would be 0 in that case, so if (server_id_opt ==
server->address) we know that it was present in the message and
correct, i.e. addressed at us.
> + send_nak(server, message);
> +
> break;
> }
> +
> + /*
> + * 4.3.2: "If the DHCPREQUEST message contains a 'server
> + * identifier' option, the message is in response to a
> + * DHCPOFFER message. Otherwise, the message is a request
> + * to verify or extend an existing lease."
> + */
> + if (server_id_opt == server->address) {
Same here, checking server_id_opt seems unsafe?
Same comment as above.
> + /*
> + * Allow either no 'requested IP address' option or
> + * a value identical with the one we offered because
> + * the spec is unclear on whether it is to be
> + * included:
> + *
> + * Section 4.3.2: "DHCPREQUEST generated during
> + * SELECTING state: (...) 'requested IP address' MUST
> + * be filled in with the yiaddr value from the chosen
> + * DHCPOFFER."
> + *
> + * but section 3.5 suggests only in the INIT-REBOOT
> + * state: "The 'requested IP address' option is
to be
> + * filled in only in a DHCPREQUEST message when the
> + * client is verifying network parameters obtained
> + * previously."
> + */
> + if (!lease->offering ||
> + (requested_ip_opt &&
> + requested_ip_opt != lease->address)) {
> + send_nak(server, message);
> + break;
> + }
> + } else {
> + /*
> + * 3.5: "If a server receives a DHCPREQUEST message
> + * with an invalid 'requested IP address', the server
> + * SHOULD respond to the client with a DHCPNAK message"
> + */
> + if (lease->offering ||
> + (requested_ip_opt &&
> + requested_ip_opt != lease->address)) {
> + send_nak(server, message);
> + break;
> + }
> + }
> +
> + send_ack(server, message, lease->address);
> break;
> case DHCP_MESSAGE_TYPE_DECLINE:
> SERVER_DEBUG("Received DECLINE");
>
> - if (!server_id_opt || !requested_ip_opt || !lease ||
> - !lease->offering)
> + if (server_id_opt != server->address || !requested_ip_opt ||
> + !lease || !lease->offering)
This also seems unsafe? server_id must be present, but you're not checking this
here?
Here we bail out if the server_id was absent (server_id_opt == 0) or
incorrect (server_id_opt != 0 && server_id_opt != server->address),
i.e. preserving the old logic.
> break;
>
> if (requested_ip_opt == lease->address)
> @@ -677,7 +744,8 @@ static void listener_event(const void *data, size_t len, void
*user_data)
> case DHCP_MESSAGE_TYPE_RELEASE:
> SERVER_DEBUG("Received RELEASE");
>
> - if (!server_id_opt || !lease || lease->offering)
> + if (server_id_opt != server->address || !lease ||
> + lease->offering)
Or here?
Same as above.
Best regards
6:39 p.m.
Hi Andrew,
We were checking the opposite, i.e. we were sending a NAK if it *was*
present.
However I was referring to the "!lease" part which seemed to be the
opposite of what was desired because we were supposed to remain silent
if we had no previous knowledge of the client.
Okay, yep, it looks like the original logic got this wrong.
>> @@ -640,6 +639,9 @@ static void listener_event(const void
*data, size_t len, void *user_data)
>> SERVER_DEBUG("Received DISCOVER, requested IP
"NIPQUAD_FMT,
>> NIPQUAD(requested_ip_opt));
>>
>> + if (server_id_opt && server_id_opt != server->address)
>> + break;
>> +
>
> So strictly speaking, according to DHCPDISCOVER from the client shouldn't even
> have this option...
Ok, I can make this check stricter here. I preserved the original
logic where we would ignore the whole message if (erver_id_opt !=
server->address) but otherwise we weren't checking whether the
server_id was correct or absent.
Well, the thing is that you've introduced a small ambiguity. Maybe it doesn't
matter... but...
The prior logic looked at the server_id option if present and compared to our
own server id. So the only way you get past the pre-check is if the server_id
matched or was omitted entirely.
But now, you can have server_id option from the client that is equal to '0', and
we treat both the same. So ideally this should be tightened up a bit.
>
>> send_offer(server, message, lease, requested_ip_opt);
>>
>> break;
>> @@ -647,27 +649,92 @@ static void listener_event(const void *data, size_t len,
void *user_data)
>> SERVER_DEBUG("Received REQUEST, requested IP
"NIPQUAD_FMT,
>> NIPQUAD(requested_ip_opt));
>>
>> - if (requested_ip_opt == 0) {
>> - requested_ip_opt = message->ciaddr;
>> - if (requested_ip_opt == 0)
>> + /*
>> + * RFC2131 Section 3.5: "Those servers not selected by the
>> + * DHCPREQUEST message use the message as notification that
>> + * the client has declined that server's offer."
>> + */
>> + if (server_id_opt && server_id_opt != server->address) {
This is inconsistent with how you do the server_id_opt checks later on. I
should have checked the original and not just the diff, but from the diff it
seemed like server_id_opt was meant to be a pointer. Not sure why my brain
didn't register that there was no dereference. So you can ignore some of my
comments based on the above.
>> + if (server->authoritative) {
>> + send_nak(server, message);
>> break;
>
> So you send a NAK here if server_id_opt is provided and doesn't match...
>
>> - }
>> + }
>>
>> - if (lease && requested_ip_opt == lease->address) {
>> - send_ack(server, message, lease->address);
>> + if (!lease || !lease->offering)
>> + break;
>> +
>> + remove_lease(server, lease);
>> break;
>> }
>>
>> - if (server_id_opt || !lease) {
>> - send_nak(server, message);
>> + /*
>> + * RFC2131 Section 3.5: "If the selected server is unable to
>> + * satisfy the DHCPREQUEST message (...), the server SHOULD
>> + * respond with a DHCPNAK message."
>> + *
>> + * But:
>> + * 4.3.2: "If the DHCP server has no record of this client,
>> + * then it MUST remain silent (...)"
>> + */
>> + if (!lease) {
>> + if (server_id_opt == server->address ||
>> + server->authoritative)
>
> So why check server->authoritative here? It seems like you can only get to this
> point if the server_id_opt is provided and matches, or not provided at all.
Right, I guess I could have simply written (server_id_opt ||
server->authoritative) beause as you noted, we've already checked that
if server_id was present, it was correct.
The server->authoritative part is so that we send a NAK if no
server_id was provided and we know we're the only server on the
network.
Okay, makes sense.
Regards,
-Denis
7:29 p.m.
On Wed, 28 Jul 2021 at 20:45, Denis Kenzior <denkenz(a)gmail.com> wrote:
>>> @@ -640,6 +639,9 @@ static void listener_event(const
void *data, size_t len, void *user_data)
>>> SERVER_DEBUG("Received DISCOVER, requested IP
"NIPQUAD_FMT,
>>> NIPQUAD(requested_ip_opt));
>>>
>>> + if (server_id_opt && server_id_opt !=
server->address)
>>> + break;
>>> +
>>
>> So strictly speaking, according to DHCPDISCOVER from the client shouldn't
even
>> have this option...
>
> Ok, I can make this check stricter here. I preserved the original
> logic where we would ignore the whole message if (erver_id_opt !=
> server->address) but otherwise we weren't checking whether the
> server_id was correct or absent.
>
Well, the thing is that you've introduced a small ambiguity. Maybe it doesn't
matter... but...
The prior logic looked at the server_id option if present and compared to our
own server id. So the only way you get past the pre-check is if the server_id
matched or was omitted entirely.
But now, you can have server_id option from the client that is equal to '0', and
we treat both the same. So ideally this should be tightened up a bit.
True, if the client includes a 0 server_id we'd treat it as if the
option was omitted. Let me replace uint32_t server_id_opt with a bool
server_id_opt and a bool server_id_match.
Best regards
386
days inactive
391
days old
14 comments
2 participants
participants (2)
-
Andrew Zaborowski -
Denis Kenzior