From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Adolf Belka (ipfire-dev)" <adolf.belka@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] bug#10629: Prevent dynamic and fixed leases overlapping
Date: Thu, 18 Feb 2021 13:17:30 +0100
Message-ID: <4a440c35-f9df-4738-6fc5-eed99b30fab4@ipfire.org>
In-Reply-To: <4642CAB0-66FC-4B92-A0E5-096052FD79A6@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============5101750128695386604=="
List-Id: <development.lists.ipfire.org>

--===============5101750128695386604==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hi Michael,

On 18/02/2021 12:37, Michael Tremer wrote:
> Hello,
>=20
> This has come up a couple of times before, and I am not sure if we can make=
 this change without breaking any existing setups.
>=20
> As I understand it, we do. Editing a static lease and hitting save will no =
longer be possible if that IP address is part of the dynamic range.
>=20
> Can you confirm that?

If the static lease is edited to have an IP address in the dynamic range then=
 it will not be possible any more. There will be an error message saying that=
 you have selected an IP from the dynamic range. It will not be entered into =
the dhcp.conf file.

If we don't implement this fix then anyone who defines a fixed IP address fro=
m the dynamic range is doing something that ISC DHCP say should not be done.

Most people will probably get away with it most of the time. You could end up=
 with a fixed lease computer off line for some reason and its lease expires. =
That IP Address can then be given to another computer and when the original c=
lient comes back it cannot get the fixed lease. It will probably be offered a=
 dynamic lease now. Either way the fixed IP address will now be allocated to =
a different computer.

pfSense and OPNsense have this restriction implemented in their WUI.

Regards,

Adolf.

>=20
>> On 17 Feb 2021, at 13:58, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>
>> - This is a fix for bug #10629
>> - I have tested this out on my vm testbed system. Everything worked fine
>>   with this. It would be good to get other test feedback in case I have
>>   missed something.
>> - This fix flags up if a fixed lease is created within the existing dynamic
>>   range
>> - This fix also works if a dynamic lease is converted to a fixed lease. A
>>   new IP outside the dynamic range has to be selected.
>> - A check has also been added if the dynamic range is modified to overlap
>>   any existing fixed leases. The error message will also inform how many
>>   fixed leases are now overlapped by the modified dynamic range.
>> - If an interface is disabled and fixed leases within the dynamic range
>>   created or the dynamic range expanded to overlap with existing fixed
>>   leases, then when the interface is enabled again the check is carried
>>   out and catches these and prevents them being set.
>> - New error messages added to en.pl file
>>
>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>> ---
>> config/cfgroot/general-functions.pl | 18 ++++++++++++
>> doc/language_issues.de              |  2 ++
>> doc/language_issues.en              |  2 ++
>> doc/language_issues.es              |  2 ++
>> doc/language_issues.fr              |  2 ++
>> doc/language_issues.it              |  2 ++
>> doc/language_issues.nl              |  2 ++
>> doc/language_issues.pl              |  2 ++
>> doc/language_issues.ru              |  2 ++
>> doc/language_issues.tr              |  2 ++
>> doc/language_missings               | 24 ++++++++++++++++
>> html/cgi-bin/dhcp.cgi               | 43 +++++++++++++++++++++++++++++
>> langs/en/cgi-bin/en.pl              |  3 ++
>> 13 files changed, 106 insertions(+)
>>
>> diff --git a/config/cfgroot/general-functions.pl b/config/cfgroot/general-=
functions.pl
>> index a6656ccf5..a8c8d171c 100644
>> --- a/config/cfgroot/general-functions.pl
>> +++ b/config/cfgroot/general-functions.pl
>> @@ -591,6 +591,24 @@ sub check_net_internal_exact{
>> 	if (($ownnet{'RED_NETADDRESS'} 		ne '' && $ownnet{'RED_NETADDRESS'} 		ne =
'0.0.0.0') && &Network::network_equal("$ownnet{'RED_NETADDRESS'}/$ownnet{'RED=
_NETMASK'}", $network)){ $errormessage=3D$Lang::tr{'ccd err red'};return $err=
ormessage;}
>> }
>>
>> +sub ip_address_in_ip_range($$) {
>> +# Returns True if $ipaddress is within $ipstart and $ipend range.
>> +	my $ipaddress =3D shift;
>> +	my $ipstart =3D shift;
>> +	my $ipend =3D shift;
>> +
>> +	my $ipaddress_bin =3D &Network::ip2bin($ipaddress);
>> +	return undef unless (defined $ipaddress_bin);
>> +
>> +	my $ipstart_bin =3D &Network::ip2bin($ipstart);
>> +	return undef unless (defined $ipstart_bin);
>> +
>> +	my $ipend_bin =3D &Network::ip2bin($ipend);
>> +	return undef unless (defined $ipend_bin);
>> +
>> +	return (($ipaddress_bin >=3D $ipstart_bin) && ($ipaddress_bin <=3D $ipen=
d_bin));
>> +}
>=20
> This function should live in network-functions.pl since it clearly is a net=
work function :)
>=20
> Ideally a test could be added for it at the end of it.
>=20
>> +
>> sub validport
>> {
>> 	$_ =3D $_[0];
>> diff --git a/doc/language_issues.de b/doc/language_issues.de
>> index 5d079036a..cb3e89b2e 100644
>> --- a/doc/language_issues.de
>> +++ b/doc/language_issues.de
>> @@ -840,6 +840,8 @@ WARNING: translation string unused: zoneconf val vlan =
amount assignment error
>> WARNING: translation string unused: zoneconf val vlan tag assignment error
>> WARNING: translation string unused: zoneconf val zoneslave amount error
>> WARNING: untranslated string: desired =3D Desired
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: disable =3D Disable
>> WARNING: untranslated string: enable =3D Enable
>> WARNING: untranslated string: error the to date has to be later than the f=
rom date =3D The to date has to be later than the from date!
>> diff --git a/doc/language_issues.en b/doc/language_issues.en
>> index 6e30eb995..832ff8d92 100644
>> --- a/doc/language_issues.en
>> +++ b/doc/language_issues.en
>> @@ -582,6 +582,8 @@ WARNING: untranslated string: dhcp dns key name =3D Ke=
y Name
>> WARNING: untranslated string: dhcp dns update =3D DNS Update
>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm
>> WARNING: untranslated string: dhcp dns update secret =3D Secret
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: dhcp server =3D DHCP Server
>> WARNING: untranslated string: dhcp server disabled =3D DHCP server disable=
d.  Stopped.
>> WARNING: untranslated string: dhcp server enabled =3D DHCP server enabled.=
  Restarting.
>> diff --git a/doc/language_issues.es b/doc/language_issues.es
>> index 82d65d99c..b65ecd164 100644
>> --- a/doc/language_issues.es
>> +++ b/doc/language_issues.es
>> @@ -893,6 +893,8 @@ WARNING: untranslated string: dhcp dns key name =3D Ke=
y Name
>> WARNING: untranslated string: dhcp dns update =3D DNS Update
>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm
>> WARNING: untranslated string: dhcp dns update secret =3D Secret
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: dhcp valid range required when deny known cl=
ients checked =3D Valid range required when "Deny known clients:" is checked
>> WARNING: untranslated string: disable =3D Disable
>> WARNING: untranslated string: disconnected =3D Disconnected
>> diff --git a/doc/language_issues.fr b/doc/language_issues.fr
>> index 942be73ec..71de90bd7 100644
>> --- a/doc/language_issues.fr
>> +++ b/doc/language_issues.fr
>> @@ -880,6 +880,8 @@ WARNING: translation string unused: zoneconf val vlan =
amount assignment error
>> WARNING: translation string unused: zoneconf val vlan tag assignment error
>> WARNING: translation string unused: zoneconf val zoneslave amount error
>> WARNING: untranslated string: dhcp deny known clients: =3D Deny known clie=
nts:
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: dhcp valid range required when deny known cl=
ients checked =3D Valid range required when "Deny known clients:" is checked
>> WARNING: untranslated string: fwhost cust locationgrp =3D unknown string
>> WARNING: untranslated string: fwhost err hostip =3D unknown string
>> diff --git a/doc/language_issues.it b/doc/language_issues.it
>> index 98074e59f..a4cd8c5db 100644
>> --- a/doc/language_issues.it
>> +++ b/doc/language_issues.it
>> @@ -917,6 +917,8 @@ WARNING: untranslated string: dhcp dns key name =3D Ke=
y Name
>> WARNING: untranslated string: dhcp dns update =3D DNS Update
>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm
>> WARNING: untranslated string: dhcp dns update secret =3D Secret
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: dhcp valid range required when deny known cl=
ients checked =3D Valid range required when "Deny known clients:" is checked
>> WARNING: untranslated string: disable =3D Disable
>> WARNING: untranslated string: disconnected =3D Disconnected
>> diff --git a/doc/language_issues.nl b/doc/language_issues.nl
>> index 8eebbd57f..9cef4790e 100644
>> --- a/doc/language_issues.nl
>> +++ b/doc/language_issues.nl
>> @@ -918,6 +918,8 @@ WARNING: untranslated string: dhcp dns key name =3D Ke=
y Name
>> WARNING: untranslated string: dhcp dns update =3D DNS Update
>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm
>> WARNING: untranslated string: dhcp dns update secret =3D Secret
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: disable =3D Disable
>> WARNING: untranslated string: disconnected =3D Disconnected
>> WARNING: untranslated string: dl client arch insecure =3D Download insecur=
e Client Package (zip)
>> diff --git a/doc/language_issues.pl b/doc/language_issues.pl
>> index 82d65d99c..b65ecd164 100644
>> --- a/doc/language_issues.pl
>> +++ b/doc/language_issues.pl
>> @@ -893,6 +893,8 @@ WARNING: untranslated string: dhcp dns key name =3D Ke=
y Name
>> WARNING: untranslated string: dhcp dns update =3D DNS Update
>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm
>> WARNING: untranslated string: dhcp dns update secret =3D Secret
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: dhcp valid range required when deny known cl=
ients checked =3D Valid range required when "Deny known clients:" is checked
>> WARNING: untranslated string: disable =3D Disable
>> WARNING: untranslated string: disconnected =3D Disconnected
>> diff --git a/doc/language_issues.ru b/doc/language_issues.ru
>> index 43c1f8c08..76fd6b350 100644
>> --- a/doc/language_issues.ru
>> +++ b/doc/language_issues.ru
>> @@ -895,6 +895,8 @@ WARNING: untranslated string: dhcp dns key name =3D Ke=
y Name
>> WARNING: untranslated string: dhcp dns update =3D DNS Update
>> WARNING: untranslated string: dhcp dns update algo =3D Algorithm
>> WARNING: untranslated string: dhcp dns update secret =3D Secret
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: dhcp valid range required when deny known cl=
ients checked =3D Valid range required when "Deny known clients:" is checked
>> WARNING: untranslated string: disable =3D Disable
>> WARNING: untranslated string: disconnected =3D Disconnected
>> diff --git a/doc/language_issues.tr b/doc/language_issues.tr
>> index 439a58890..bd78a5a4e 100644
>> --- a/doc/language_issues.tr
>> +++ b/doc/language_issues.tr
>> @@ -896,6 +896,8 @@ WARNING: untranslated string: dangerous =3D Dangerous
>> WARNING: untranslated string: default IP address =3D Default IP Address
>> WARNING: untranslated string: desired =3D Desired
>> WARNING: untranslated string: dhcp deny known clients: =3D Deny known clie=
nts:
>> +WARNING: untranslated string: dhcp dynamic range overlap =3D Dynamic rang=
e overlapped with
>> +WARNING: untranslated string: dhcp fixed ip address =3D  Fixed IP Address=
(es)
>> WARNING: untranslated string: dhcp valid range required when deny known cl=
ients checked =3D Valid range required when "Deny known clients:" is checked
>> WARNING: untranslated string: disable =3D Disable
>> WARNING: untranslated string: disconnected =3D Disconnected
>> diff --git a/doc/language_missings b/doc/language_missings
>> index 0d89426ca..3d6c5103d 100644
>> --- a/doc/language_missings
>> +++ b/doc/language_missings
>> @@ -28,6 +28,9 @@
>> < could not connect to www ipfire org
>> < cryptographic settings
>> < desired
>> +< dhcp dynamic range overlap
>> +< dhcp fixed ip address
>> +< dhcp fixed ip address in dynamic range
>> < dhcp server disabled on blue interface
>> < dhcp server enabled on blue interface
>> < dh name is invalid
>> @@ -230,6 +233,9 @@
>> < dhcp dns update
>> < dhcp dns update algo
>> < dhcp dns update secret
>> +< dhcp dynamic range overlap
>> +< dhcp fixed ip address
>> +< dhcp fixed ip address in dynamic range
>> < dhcp valid range required when deny known clients checked
>> < dh key move failed
>> < dh key warn
>> @@ -969,6 +975,9 @@
>> < bewan adsl pci st
>> < bewan adsl usb
>> < dhcp deny known clients:
>> +< dhcp dynamic range overlap
>> +< dhcp fixed ip address
>> +< dhcp fixed ip address in dynamic range
>> < dhcp valid range required when deny known clients checked
>> < g.dtm
>> < g.lite
>> @@ -1071,6 +1080,9 @@
>> < dhcp dns update
>> < dhcp dns update algo
>> < dhcp dns update secret
>> +< dhcp dynamic range overlap
>> +< dhcp fixed ip address
>> +< dhcp fixed ip address in dynamic range
>> < dhcp valid range required when deny known clients checked
>> < disable
>> < Disabled
>> @@ -1460,6 +1472,9 @@
>> < dhcp dns update
>> < dhcp dns update algo
>> < dhcp dns update secret
>> +< dhcp dynamic range overlap
>> +< dhcp fixed ip address
>> +< dhcp fixed ip address in dynamic range
>> < dh key move failed
>> < dh key warn
>> < dh key warn1
>> @@ -1965,6 +1980,9 @@
>> < dhcp dns update
>> < dhcp dns update algo
>> < dhcp dns update secret
>> +< dhcp dynamic range overlap
>> +< dhcp fixed ip address
>> +< dhcp fixed ip address in dynamic range
>> < dhcp valid range required when deny known clients checked
>> < dh key move failed
>> < dh key warn
>> @@ -2848,6 +2866,9 @@
>> < dhcp dns update
>> < dhcp dns update algo
>> < dhcp dns update secret
>> +< dhcp dynamic range overlap
>> +< dhcp fixed ip address
>> +< dhcp fixed ip address in dynamic range
>> < dhcp valid range required when deny known clients checked
>> < dh key move failed
>> < dh key warn
>> @@ -3595,6 +3616,9 @@
>> < default IP address
>> < desired
>> < dhcp deny known clients:
>> +< dhcp dynamic range overlap
>> +< dhcp fixed ip address
>> +< dhcp fixed ip address in dynamic range
>> < dhcp valid range required when deny known clients checked
>> < disable
>> < Disabled
>> diff --git a/html/cgi-bin/dhcp.cgi b/html/cgi-bin/dhcp.cgi
>> index 867614f2a..82ea754c7 100644
>> --- a/html/cgi-bin/dhcp.cgi
>> +++ b/html/cgi-bin/dhcp.cgi
>> @@ -130,6 +130,7 @@ open(FILE, "$filename2") or die 'Unable to open fixed =
leases file.';
>> our @current2 =3D <FILE>;
>> close(FILE);
>>
>> +
>> # Check Settings1 first because they are needed by &buildconf
>> if ($dhcpsettings{'ACTION'} eq $Lang::tr{'save'}) {
>>      foreach my $itf (@ITFs) {
>> @@ -183,6 +184,24 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'save'}) {
>> 		}
>> 	    }
>>
>> +	    # Check if dynamic range and Fixed IP Addresses overlap
>> +	    if ((!$dhcpsettings{"START_ADDR_${itf}"}) eq '' && (!$dhcpsettings{"=
END_ADDR_${itf}"}) eq '') {
>=20
> For better readability, writing =E2=80=9Cne=E2=80=9D instead of !eq might b=
e a good idea.
>=20
>> +		my $count=3D0;
>> +		foreach my $line (@current2) {
>> +			chomp($line);
>> +			my @temp =3D split(/\,/,$line);
>> +			if (&General::ip_address_in_ip_range($temp[1],
>> +							     $dhcpsettings{"START_ADDR_${itf}"},
>> +							     $dhcpsettings{"END_ADDR_${itf}"})) {
>> +				$count++;
>> +			}
>> +		}
>> +		if ($count > 0) {
>> +			$errormessage =3D "DHCP on ${itf}: " . $Lang::tr{'dhcp dynamic range o=
verlap'} . $count . $Lang::tr{'dhcp fixed ip address'};
>> +			goto ERROR;
>> +		}
>> +	    }
>> +
>> 	    if (!($dhcpsettings{"DEFAULT_LEASE_TIME_${itf}"} =3D~ /^\d+$/)) {
>> 		$errormessage =3D "DHCP on ${itf}: " . $Lang::tr{'invalid default lease =
time'} . $dhcpsettings{'DEFAULT_LEASE_TIME_${itf}'};
>> 		goto ERROR;
>> @@ -415,10 +434,34 @@ if ($dhcpsettings{'ACTION'} eq $Lang::tr{'toggle ena=
ble disable'}.'2') {
>> if ($dhcpsettings{'ACTION'} eq $Lang::tr{'add'}.'2') {
>>      $dhcpsettings{'FIX_MAC'} =3D~ tr/-/:/;
>>      unless(&General::validip($dhcpsettings{'FIX_ADDR'})) { $errormessage =
=3D $Lang::tr{'invalid fixed ip address'}; }
>> +# Check if fixed address is in the dynamic range, if defined
>> +    foreach my $itf (@ITFs) {
>> +	if ($dhcpsettings{"ENABLE_${itf}"} eq 'on' ) {
>> +		if ($dhcpsettings{"START_ADDR_${itf}"}) {
>> +    			if (&General::ip_address_in_ip_range($dhcpsettings{'FIX_ADDR'},
>> +							     $dhcpsettings{"START_ADDR_${itf}"},
>> +							     $dhcpsettings{"END_ADDR_${itf}"})) {
>> +				$errormessage =3D $Lang::tr{"dhcp fixed ip address in dynamic range"};
>> +			}
>> +		}
>> +	}
>> +    }
>>      unless(&General::validmac($dhcpsettings{'FIX_MAC'})) { $errormessage =
=3D $Lang::tr{'invalid fixed mac address'}; }
>>      if ($dhcpsettings{'FIX_NEXTADDR'}) {
>>          unless(&General::validip($dhcpsettings{'FIX_NEXTADDR'})) { $error=
message =3D $Lang::tr{'invalid fixed ip address'}; }
>>      }
>> +# Check if fixed next address is in the dynamic range, if defined
>> +    foreach my $itf (@ITFs) {
>> +	if ($dhcpsettings{"ENABLE_${itf}"} eq 'on' ) {
>> +		if ($dhcpsettings{"START_ADDR_${itf}"}) {
>> +    			if (&General::ip_address_in_ip_range($dhcpsettings{'FIX_NEXTADDR'},
>> +							     $dhcpsettings{"START_ADDR_${itf}"},
>> +							     $dhcpsettings{"END_ADDR_${itf}"})) {
>> +				$errormessage =3D $Lang::tr{"dhcp fixed ip address in dynamic range"};
>> +			}
>> +		}
>> +	}
>> +    }
>> =09
>>      my $key =3D 0;
>>      CHECK:foreach my $line (@current2) {
>> diff --git a/langs/en/cgi-bin/en.pl b/langs/en/cgi-bin/en.pl
>> index 95a1cfda4..0dbdf7bd5 100644
>> --- a/langs/en/cgi-bin/en.pl
>> +++ b/langs/en/cgi-bin/en.pl
>> @@ -806,6 +806,9 @@
>> 'dhcp dns update' =3D> 'DNS Update',
>> 'dhcp dns update algo' =3D> 'Algorithm',
>> 'dhcp dns update secret' =3D> 'Secret',
>> +'dhcp dynamic range overlap' =3D> 'Dynamic range overlapped with ',
>> +'dhcp fixed ip address' =3D> ' Fixed IP Address(es)',
>> +'dhcp fixed ip address in dynamic range' =3D> 'Fixed IP Address in dynami=
c range is not allowed',
>> 'dhcp fixed lease err1' =3D> 'For a fix lease you have to enter the MAC ad=
dress or the hostname, or you enter both.',
>> 'dhcp fixed lease help1' =3D> 'IP Addresses might be entered as FQDN',
>> 'dhcp mode' =3D> 'DHCP',
>> --=20
>> 2.30.1
>>
>=20

--===============5101750128695386604==--