Re: [PATCH] BUG11278: It is not possible to create subnets of internal networks in firewallgroups

[Michael Tremer <michael.tremer@ipfire.org>]

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

Hey,

although I am not perfectly happy with the code quality, I merged this patch.

The entire code base here needs to become cleaner and we need to break it down
better into small functions that serve a single and easy purpose.

Right now it is possible to completely change the behaviour of a function with a
paramter. That is hard to understand and document. Hence I would like to change
this.

We already have some good starting points and therefore I would like to aim for
improving this in the month of June.

I created a ticket on BZ to keep track of this and assigned it to you.

Any additional help is of course appreciated.

Best,
-Michael

On Wed, 2017-06-07 at 15:13 +0200, Alexander Marx wrote:
> Fixes: #11278
> 
> When creating networks which are part of an internal network, there was an
> errormessage displayed and the creation was prohibited.
> Now it is possible to create such subnets. This is used at own risk! Users
> have to take care of the firewallrule sequence.
> It is possible to create situations that are not wanted.
> 
> Signed-off-by: Alexander Marx <alexander.marx(a)ipfire.org>
> ---
>  config/cfgroot/general-functions.pl | 24 ++++++++++++++++++++++--
>  html/cgi-bin/fwhosts.cgi            |  2 +-
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/config/cfgroot/general-functions.pl b/config/cfgroot/general-
> functions.pl
> index 5e5417d..f448c34 100644
> --- a/config/cfgroot/general-functions.pl
> +++ b/config/cfgroot/general-functions.pl
> @@ -465,6 +465,7 @@ sub checksubnets
>  	my $ccdname=$_[0];
>  	my $ccdnet=$_[1];
>  	my $ownnet=$_[2];
> +	my $checktype=$_[3];
>  	my $errormessage;
>  	my ($ip,$cidr)=split(/\//,$ccdnet);
>  	$cidr=&iporsubtocidr($cidr);
> @@ -542,10 +543,15 @@ sub checksubnets
>  	}
>  	
>  	#call check_net_internal
> -	&General::check_net_internal($ccdnet);
> +	if ($checktype eq "exact")
> +	{
> +		&General::check_net_internal_exact($ccdnet);
> +	}else{
> +		&General::check_net_internal_range($ccdnet);
> +	}
>  }
>  
> -sub check_net_internal{
> +sub check_net_internal_range{
>  	my $network=shift;
>  	my ($ip,$cidr)=split(/\//,$network);
>  	my %ownnet=();
> @@ -559,6 +565,20 @@ sub check_net_internal{
>  	if (($ownnet{'RED_NETADDRESS'} 		ne '' &&
> $ownnet{'RED_NETADDRESS'} 		ne '0.0.0.0') &&
> &IpInSubnet($ip,$ownnet{'RED_NETADDRESS'},&iporsubtodec($ownnet{'RED_NETMASK'}
> ))){ $errormessage=$Lang::tr{'ccd err red'};return $errormessage;}
>  }
>  
> +sub check_net_internal_exact{
> +	my $network=shift;
> +	my ($ip,$cidr)=split(/\//,$network);
> +	my %ownnet=();
> +	my $errormessage;
> +	$cidr=&iporsubtocidr($cidr);
> +	#check if we use one of ipfire's networks (green,orange,blue)
> +	&readhash("${General::swroot}/ethernet/settings", \%ownnet);
> +	if (($ownnet{'GREEN_NETADDRESS'}  	ne '' &&
> $ownnet{'GREEN_NETADDRESS'} 	ne '0.0.0.0') &&
> &Network::network_equal("$ownnet{'GREEN_NETADDRESS'}/$ownnet{'GREEN_NETMASK'}"
> , $network)){ $errormessage=$Lang::tr{'ccd err green'};return $errormessage;}
> +	if (($ownnet{'ORANGE_NETADDRESS'}	ne '' &&
> $ownnet{'ORANGE_NETADDRESS'} 	ne '0.0.0.0') &&
> &Network::network_equal("$ownnet{'ORANGE_NETADDRESS'}/$ownnet{'ORANGE_NETMASK'
> }", $network)){ $errormessage=$Lang::tr{'ccd err orange'};return
> $errormessage;}
> +	if (($ownnet{'BLUE_NETADDRESS'} 	ne '' &&
> $ownnet{'BLUE_NETADDRESS'} 	ne '0.0.0.0') &&
> &Network::network_equal("$ownnet{'BLUE_NETADDRESS'}/$ownnet{'BLUE_NETMASK'}",
> $network)){ $errormessage=$Lang::tr{'ccd err blue'};return $errormessage;}
> +	if (($ownnet{'RED_NETADDRESS'} 		ne '' &&
> $ownnet{'RED_NETADDRESS'} 		ne '0.0.0.0') &&
> &Network::network_equal("$ownnet{'RED_NETADDRESS'}/$ownnet{'RED_NETMASK'}",
> $network)){ $errormessage=$Lang::tr{'ccd err red'};return $errormessage;}
> +}
> +
>  sub validport
>  {
>  	$_ = $_[0];
> diff --git a/html/cgi-bin/fwhosts.cgi b/html/cgi-bin/fwhosts.cgi
> index 1b0fe07..25ab489 100644
> --- a/html/cgi-bin/fwhosts.cgi
> +++ b/html/cgi-bin/fwhosts.cgi
> @@ -301,7 +301,7 @@ if ($fwhostsettings{'ACTION'} eq 'savenet' )
>  		}
>  		if($fwhostsettings{'error'} ne 'on'){
>  				my
> $fullip="$fwhostsettings{'IP'}/".&General::iporsubtocidr($fwhostsettings{'SUBN
> ET'});
> -				$errormessage=$errormessage.&General::checksu
> bnets($fwhostsettings{'HOSTNAME'},$fullip,"");
> +				$errormessage=$errormessage.&General::checksu
> bnets($fwhostsettings{'HOSTNAME'},$fullip,"","exact");
>  		}
>  		#only check plausi when no error till now
>  		if (!$errormessage){

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]