[PATCH 1/2] dhcp-server: Stricter start_ip/end_ip checks

[PATCH 1/2] dhcp-server: Stricter start_ip/end_ip checks

[Andrew Zaborowski]

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

Add basic checks that start_ip/end_ip are in the subnet defined by
server->address and server->netmask.  Exclude server->address from
allowed client addresses since we don't prohibit a start_ip/end_ip
combination that contains server->address (and that's Ok).  Slightly
modify how the default end_ip is derived from server->address to work
with any prefix_length -- that seems to have been the only place that
made assumptions about the prefix length.
---
 ell/dhcp-server.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
index 3bc1eac..68b6f13 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -323,7 +323,10 @@ static bool check_requested_ip(struct l_dhcp_server *server,
 	if (ntohl(requested_nip) < server->start_ip)
 		return false;
 
-	if (htonl(requested_nip) > server->end_ip)
+	if (ntohl(requested_nip) > server->end_ip)
+		return false;
+
+	if (requested_nip == server->address)
 		return false;
 
 	lease = find_lease_by_ip(server->lease_list, requested_nip);
@@ -361,6 +364,9 @@ static uint32_t find_free_or_expired_ip(struct l_dhcp_server *server,
 		if ((ip_addr & 0xff) == 0xff)
 			continue;
 
+		if (ip_nl == server->address)
+			continue;
+
 		/*
 		 * Search both active and expired leases. If this exausts all
 		 * IP's in the range pop the expired list (oldest expired lease)
@@ -778,13 +784,33 @@ LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server)
 
 	/*
 	 * Assign a default ip range if not already. This will default to
-	 * server->address + 1 ... 254
+	 * server->address + 1 ... subnet end address - 1
 	 */
 	if (!server->start_ip) {
-		server->start_ip = L_BE32_TO_CPU(server->address) + 1;
-		server->end_ip = (server->start_ip & 0xffffff00) | 0xfe;
+		server->start_ip = ntohl(server->address) + 1;
+		server->end_ip = ntohl(server->address) |
+			(~ntohl(server->netmask));
+	} else {
+		if ((server->start_ip ^ ntohl(server->address)) &
+				ntohl(server->netmask))
+			return false;
+
+		if ((server->end_ip ^ ntohl(server->address)) &
+				ntohl(server->netmask))
+			return false;
 	}
 
+	/*
+	 * We skip over IPs ending in 0 or 255 when selecting a free address
+	 * later on but make sure end_ip is not 0xffffffff so we can use
+	 * "<= server->end_ip" safely in the loop condition.
+	 */
+	if ((server->end_ip & 0xff) == 255)
+		server->end_ip--;
+
+	if (server->start_ip > server->end_ip)
+		return false;
+
 	if (!server->ifname) {
 		server->ifname = l_net_get_name(server->ifindex);
 
-- 
2.27.0

[PATCH 2/2] dhcp-server: Drop a redundant cast

[Andrew Zaborowski]

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

---
 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 68b6f13..67037cf 100644
--- a/ell/dhcp-server.c
+++ b/ell/dhcp-server.c
@@ -892,7 +892,7 @@ LIB_EXPORT bool l_dhcp_server_set_ip_range(struct l_dhcp_server *server,
 	if (unlikely(!server || !start_ip || !end_ip))
 		return false;
 
-	if (inet_aton((const char *)start_ip, &_host_addr) == 0)
+	if (inet_aton(start_ip, &_host_addr) == 0)
 		return false;
 
 	start = ntohl(_host_addr.s_addr);
-- 
2.27.0

Re: [PATCH 1/2] dhcp-server: Stricter start_ip/end_ip checks

[Denis Kenzior]

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

Hi Andrew,

On 3/3/21 3:35 PM, Andrew Zaborowski wrote:
> Add basic checks that start_ip/end_ip are in the subnet defined by
> server->address and server->netmask.  Exclude server->address from
> allowed client addresses since we don't prohibit a start_ip/end_ip
> combination that contains server->address (and that's Ok).  Slightly
> modify how the default end_ip is derived from server->address to work
> with any prefix_length -- that seems to have been the only place that
> made assumptions about the prefix length.
> ---
>   ell/dhcp-server.c | 34 ++++++++++++++++++++++++++++++----
>   1 file changed, 30 insertions(+), 4 deletions(-)
> 

Both applied, thanks.

Regards,
-Denis

Re: [PATCH 1/2] dhcp-server: Stricter start_ip/end_ip checks

[=?unknown-8bit?q?=C3=89rico?= Nogueira]

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

Em 03/03/2021 18:35, Andrew Zaborowski escreveu:
> Add basic checks that start_ip/end_ip are in the subnet defined by
> server->address and server->netmask.  Exclude server->address from
> allowed client addresses since we don't prohibit a start_ip/end_ip
> combination that contains server->address (and that's Ok).  Slightly
> modify how the default end_ip is derived from server->address to work
> with any prefix_length -- that seems to have been the only place that
> made assumptions about the prefix length.
> ---
>   ell/dhcp-server.c | 34 ++++++++++++++++++++++++++++++----
>   1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/ell/dhcp-server.c b/ell/dhcp-server.c
> index 3bc1eac..68b6f13 100644
> --- a/ell/dhcp-server.c
> +++ b/ell/dhcp-server.c
> @@ -323,7 +323,10 @@ static bool check_requested_ip(struct l_dhcp_server *server,
>   	if (ntohl(requested_nip) < server->start_ip)
>   		return false;
>   
> -	if (htonl(requested_nip) > server->end_ip)
> +	if (ntohl(requested_nip) > server->end_ip)

The commit message didn't mention anything about fixing wrong htonl 
usage, so is this change correct?

> +		return false;
> +
> +	if (requested_nip == server->address)
>   		return false;
>   
>   	lease = find_lease_by_ip(server->lease_list, requested_nip);
> @@ -361,6 +364,9 @@ static uint32_t find_free_or_expired_ip(struct l_dhcp_server *server,
>   		if ((ip_addr & 0xff) == 0xff)
>   			continue;
>   
> +		if (ip_nl == server->address)
> +			continue;
> +
>   		/*
>   		 * Search both active and expired leases. If this exausts all
>   		 * IP's in the range pop the expired list (oldest expired lease)
> @@ -778,13 +784,33 @@ LIB_EXPORT bool l_dhcp_server_start(struct l_dhcp_server *server)
>   
>   	/*
>   	 * Assign a default ip range if not already. This will default to
> -	 * server->address + 1 ... 254
> +	 * server->address + 1 ... subnet end address - 1
>   	 */
>   	if (!server->start_ip) {
> -		server->start_ip = L_BE32_TO_CPU(server->address) + 1;
> -		server->end_ip = (server->start_ip & 0xffffff00) | 0xfe;
> +		server->start_ip = ntohl(server->address) + 1;
> +		server->end_ip = ntohl(server->address) |
> +			(~ntohl(server->netmask));
> +	} else {
> +		if ((server->start_ip ^ ntohl(server->address)) &
> +				ntohl(server->netmask))
> +			return false;
> +
> +		if ((server->end_ip ^ ntohl(server->address)) &
> +				ntohl(server->netmask))
> +			return false;
>   	}
>   
> +	/*
> +	 * We skip over IPs ending in 0 or 255 when selecting a free address
> +	 * later on but make sure end_ip is not 0xffffffff so we can use
> +	 * "<= server->end_ip" safely in the loop condition.
> +	 */
> +	if ((server->end_ip & 0xff) == 255)
> +		server->end_ip--;
> +
> +	if (server->start_ip > server->end_ip)
> +		return false;
> +
>   	if (!server->ifname) {
>   		server->ifname = l_net_get_name(server->ifindex);
>   
> 

Re: [PATCH 1/2] dhcp-server: Stricter start_ip/end_ip checks

[Denis Kenzior]

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

Hi Érico,

>> -    if (htonl(requested_nip) > server->end_ip)
>> +    if (ntohl(requested_nip) > server->end_ip)
> 
> The commit message didn't mention anything about fixing wrong htonl usage, so is 
> this change correct?
> 

Eagle eyes ;)

But yes, the change is correct.  In the end both ntohl and htonl resolve to a 
no_op or bswap32.  So the end result is the same.

Not sure why it was originally written as htonl, but ntohl is more logically 
correct since requested_nip is is network order and end_ip is in host order.

Ideally there should have been a separate fixup commit for this...

Regards,
-Denis