Re: [PATCH] BUG10940: remove leading zeros in ip address

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

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

On Mon, 2015-11-09 at 20:14 +0100, Alexander Marx wrote:
> 
> FULL ACK, Michael.
> 
> But unfortunately it seems not to be possible to catch that issue
> with modification of central functions like check_ip_address, becaus
> ethat functions only return true or false not the "normalized ip"
> without zero's.
> 
> I am open for better solutions....  anyone ideas?!

It is too late for that now. I think the best option would simply to
consider those inputs as invalid and maybe fix this at some places
where we can and where we find this makes sense.

-Michael

P.S. Please make sure to reply to the list. Especially if you are
asking a question to the group :)

> 
> cheers,
> 
> Alex
> 
> Am 09.11.2015 um 19:33 schrieb Michael Tremer:
> > Hi,
> > 
> > this does not look like the cleanest solution to be honest, but I
> > guess
> > this is acceptable. It solves the issue at hand, but generally I
> > would
> > like to have this solved one time so that one does not have to keep
> > this problem in mind when writing new code.
> > 
> > Merged.
> > 
> > Best,
> > -Michael
> > 
> > On Mon, 2015-11-09 at 12:42 +0100, Alexander Marx wrote:
> > > in firewallgroups (hosts) an error was created when using ip
> > > adresses
> > > like 192.168.000.008. Now all leading zeros are deleted in
> > > firewallgroups and in the firewall itself when using single ip
> > > addresses
> > > as source or target.
> > > 
> > > Signed-off-by: Alexander Marx <alexander.marx(a)ipfire.org>
> > > ---
> > >  config/cfgroot/network-functions.pl | 13 +++++++++++++
> > >  html/cgi-bin/firewall.cgi           |  8 ++++++++
> > >  html/cgi-bin/fwhosts.cgi            | 13 +++++++------
> > >  3 files changed, 28 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/config/cfgroot/network-functions.pl
> > > b/config/cfgroot/network-functions.pl
> > > index cb4ca3d..70fa5ed 100644
> > > --- a/config/cfgroot/network-functions.pl
> > > +++ b/config/cfgroot/network-functions.pl
> > > @@ -122,6 +122,19 @@ sub network2bin($) {
> > >  	return ($network_start, $netmask_bin);
> > >  }
> > >  
> > > +# Deletes leading zeros in ip address
> > > +sub ip_remove_zero{
> > > +	my $address = shift;
> > > +	my @ip = split (/\./, $address);
> > > +
> > > +	foreach my $octet (@ip) {
> > > +		$octet = int($octet);
> > > +	}
> > > +
> > > +	$address = join (".", @ip);
> > > +
> > > +	return $address;
> > > +}
> > >  # Returns True for all valid IP addresses
> > >  sub check_ip_address($) {
> > >  	my $address = shift;
> > > diff --git a/html/cgi-bin/firewall.cgi b/html/cgi
> > > -bin/firewall.cgi
> > > index 682c285..8007182 100644
> > > --- a/html/cgi-bin/firewall.cgi
> > > +++ b/html/cgi-bin/firewall.cgi
> > > @@ -31,6 +31,7 @@ no warnings 'uninitialized';
> > >  #use CGI::Carp 'fatalsToBrowser';
> > >  
> > >  require '/var/ipfire/general-functions.pl';
> > > +require '/var/ipfire/network-functions.pl';
> > >  require "${General::swroot}/lang.pl";
> > >  require "${General::swroot}/header.pl";
> > >  require "${General::swroot}/geoip-functions.pl";
> > > @@ -465,6 +466,9 @@ sub checksource
> > >  			}
> > >  		}
> > >  		if ($fwdfwsettings{'isip'} eq 'on'){
> > > +			#remove leading zero
> > > +			$ip = &Network::ip_remove_zero($ip);
> > > +
> > >  			##check if ip is valid
> > >  			if (! &General::validip($ip)){
> > >  				$errormessage.=$Lang::tr{'fwdfw
> > > err
> > > src_addr'}."<br>";
> > > @@ -569,11 +573,15 @@ sub checktarget
> > >  			($ip,$subnet)=split
> > > (/\//,$fwdfwsettings{'tgt_addr'});
> > >  			$subnet =
> > > &General::iporsubtocidr($subnet);
> > >  		}
> > > +
> > >  		#check if only ip
> > >  		if($fwdfwsettings{'tgt_addr'}=~/^(\d{1,3})\.(\d{
> > > 1,3}
> > > )\.(\d{1,3})\.(\d{1,3})$/){
> > >  			$ip=$fwdfwsettings{'tgt_addr'};
> > >  			$subnet='32';
> > >  		}
> > > +		#remove leading zero
> > > +		$ip = &Network::ip_remove_zero($ip);
> > > +
> > >  		#check if ip is valid
> > >  		if (! &General::validip($ip)){
> > >  			$errormessage.=$Lang::tr{'fwdfw err
> > > tgt_addr'}."<br>";
> > > diff --git a/html/cgi-bin/fwhosts.cgi b/html/cgi-bin/fwhosts.cgi
> > > index 994a50a..35afad3 100644
> > > --- a/html/cgi-bin/fwhosts.cgi
> > > +++ b/html/cgi-bin/fwhosts.cgi
> > > @@ -27,6 +27,7 @@ use Sort::Naturally;
> > >  use CGI::Carp 'fatalsToBrowser';
> > >  no warnings 'uninitialized';
> > >  require '/var/ipfire/general-functions.pl';
> > > +require '/var/ipfire/network-functions.pl';
> > >  require "/var/ipfire/geoip-functions.pl";
> > >  require "/usr/lib/firewall/firewall-lib.pl";
> > >  require "${General::swroot}/lang.pl";
> > > @@ -277,6 +278,9 @@ if ($fwhostsettings{'ACTION'} eq 'savenet' )
> > >  		&addnet;
> > >  		&viewtablenet;
> > >  	}else{
> > > +		#convert ip if leading '0' exists
> > > +		$fwhostsettings{'IP'} =
> > > &Network::ip_remove_zero($fwhostsettings{'IP'});
> > > +
> > >  		#check valid ip 
> > >  		if
> > > (!&General::validipandmask($fwhostsettings{'IP'}."/".$fwhostsetti
> > > ngs{
> > > 'SUBNET'}))
> > >  		{
> > > @@ -372,9 +376,6 @@ if ($fwhostsettings{'ACTION'} eq 'savenet' )
> > >  			foreach my $i (0 .. 3) {
> > > $customnetwork{$key}[$i] = "";}
> > >  			$fwhostsettings{'SUBNET'}	=
> > > &General::iporsubtocidr($fwhostsettings{'SUBNET'});
> > >  			$customnetwork{$key}[0] 	=
> > > $fwhostsettings{'HOSTNAME'};
> > > -			#convert ip when leading '0' in byte
> > > -			$fwhostsettings{'IP'}		=&G
> > > ener
> > > al::ip2dec($fwhostsettings{'IP'});
> > > -			$fwhostsettings{'IP'}		=&G
> > > ener
> > > al::dec2ip($fwhostsettings{'IP'});
> > >  			$customnetwork{$key}[1] 	=
> > > &General::getnetworkip($fwhostsettings{'IP'},$fwhostsettings{'SUB
> > > NET'
> > > }) ;
> > >  			$customnetwork{$key}[2] 	=
> > > &General::iporsubtodec($fwhostsettings{'SUBNET'}) ;
> > >  			$customnetwork{$key}[3] 	=
> > > $fwhostsettings{'NETREMARK'};
> > > @@ -423,6 +424,9 @@ if ($fwhostsettings{'ACTION'} eq 'savehost')
> > >  		}
> > >  		#CHECK IP-PART
> > >  		if ($fwhostsettings{'type'} eq 'ip'){
> > > +			#convert ip if leading '0' exists
> > > +			$fwhostsettings{'IP'} =
> > > &Network::ip_remove_zero($fwhostsettings{'IP'});
> > > +
> > >  			#check for subnet
> > >  			if (rindex($fwhostsettings{'IP'},'/') eq
> > > '
> > > -1' ){
> > >  				if($fwhostsettings{'type'} eq
> > > 'ip'
> > > && !&General::validipandmask($fwhostsettings{'IP'}."/32"))
> > > @@ -503,9 +507,6 @@ if ($fwhostsettings{'ACTION'} eq 'savehost')
> > >  			$customhost{$key}[0] =
> > > $fwhostsettings{'HOSTNAME'} ;
> > >  			$customhost{$key}[1] =
> > > $fwhostsettings{'type'} ;
> > >  			if ($fwhostsettings{'type'} eq 'ip'){
> > > -				#convert ip when leading '0' in
> > > byte
> > > -				$fwhostsettings{'IP'}=&General::
> > > ip2d
> > > ec($fwhostsettings{'IP'});
> > > -				$fwhostsettings{'IP'}=&General::
> > > dec2
> > > ip($fwhostsettings{'IP'});
> > >  				$customhost{$key}[2] =
> > > $fwhostsettings{'IP'}."/".&General::iporsubtodec($fwhostsettings{
> > > 'SUB
> > > NET'});
> > >  			}else{
> > >  				$customhost{$key}[2] =
> > > $fwhostsettings{'IP'};

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