Re: [PATCH] BUG10941: fix single ip-addresses when no subnet given

Re: [PATCH] BUG10941: fix single ip-addresses when no subnet given

[Michael Tremer]

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

On Fri, 2015-10-16 at 19:48 +0200, Alexander Marx wrote:
> 
> 
> Am 16.10.2015 um 17:54 schrieb Michael Tremer:
> > On Fri, 2015-10-16 at 09:57 +0200, Alexander Marx wrote:
> > > Signed-off-by: Alexander Marx <alexander.marx(a)ipfire.org>
> > > ---
> > >  html/cgi-bin/routing.cgi | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/html/cgi-bin/routing.cgi b/html/cgi-bin/routing.cgi
> > > index c460a74..3b5a2f7 100644
> > > --- a/html/cgi-bin/routing.cgi
> > > +++ b/html/cgi-bin/routing.cgi
> > > @@ -118,6 +118,11 @@ if ($settings{'ACTION'} eq $Lang::tr{'toggle
> > > enable disable'}) {
> > >  }
> > >  
> > >  if ($settings{'ACTION'} eq $Lang::tr{'add'}) {
> > > +
> > > +	if (!&Network::check_prefix($settings{'IP'})){
> > > +		$settings{'IP'} .= '/32';
> > > +	}
> > > +
> > This won't work. The function &Network::check_prefix() takes the
> > prefix
> > (i.e. the number after the slash, e.g. 24). You are passing the IP
> > address to the function which will never be a valid prefix and /32
> > will
> > always be appended, even to valid inputs like 192.168.0.0/24
> > (result:
> > 192.168.0.0/24/32).
> I think it's not true that it's not true ;-)
> The function takes a whole ip with subnetmask, true. But if you pass
> only an ip to it or an ip with a wrong subnetmask , the function
> returns false in which case i add /32 to it.

http://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/network-functions.pl;h=cb4ca3dd88306f61290fab895308c0725c1ecdac;hb=HEAD#l136

This function clearly does what I said it would do.

I don't know why you don't get an error, but this is not a solution to
this bug. Is it possible that &General::iporsubtocidr() just ignores
everything after the prefix?

> 
> In addition i tested several combinations of ip-addresses in cidr or
> decimal notation and without subnetmask.
> All tests passed with expected behaviour.
> 
> I just checked it twice. I could not find any side effects. But you
> may drop this patch and i will try to find a new solution after my
> holidays.

When will you be back?

> 
> 
> > >  	# Convert subnet masks to CIDR notation.
> > >  	$settings{'IP'} =
> > > &General::iporsubtocidr($settings{'IP'});
> > > 
> > The remaining code should be amended that only prefixes are allowed
> > when /32 is appended to hosts.
> > 
> > -Michael 

-Michael

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

[PATCH] BUG10941: fix single ip-addresses when no subnet given

[Alexander Marx]

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

Some functions when adding a new route where senseless.
Now the ip address is checked and in case of a missing / wrong
subnetmask an errormessage is raised. The ip address is preserved.
ELSE
we convert the subnetmask to cidr notation and calculate the network ip
correctly.

Signed-off-by: Alexander Marx <alexander.marx(a)ipfire.org>
---
 html/cgi-bin/routing.cgi | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/html/cgi-bin/routing.cgi b/html/cgi-bin/routing.cgi
index c460a74..2c60f67 100644
--- a/html/cgi-bin/routing.cgi
+++ b/html/cgi-bin/routing.cgi
@@ -118,12 +118,16 @@ if ($settings{'ACTION'} eq $Lang::tr{'toggle enable disable'}) {
 }
 
 if ($settings{'ACTION'} eq $Lang::tr{'add'}) {
-	# Convert subnet masks to CIDR notation.
-	$settings{'IP'} = &General::iporsubtocidr($settings{'IP'});
 
-# Validate inputs
-	if (( !&General::validip($settings{'IP'})) and ( !&General::validipandmask($settings{'IP'}))){
+	# Validate inputs
+	if (!&General::validipandmask($settings{'IP'}))){
 	$errormessage = $Lang::tr{'invalid ip'}." / ".$Lang::tr{'invalid netmask'};
+	}else{
+		#set networkip if not already correctly defined
+		my($ip,$cidr) = split(/\//,$settings{'IP'});
+		$cidr = &General::iporsubtocidr($cidr);
+		my $netip=&General::getnetworkip($ip,$cidr);
+		$settings{'IP'} = "$netip/$cidr";
 	}
 
 	if ($settings{'IP'} =~ /^0\.0\.0\.0/){
-- 
1.9.1

[PATCH] BUG10941: fix single ip-addresses when no subnet given

[Alexander Marx]

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

Signed-off-by: Alexander Marx <alexander.marx(a)ipfire.org>
---
 html/cgi-bin/routing.cgi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/html/cgi-bin/routing.cgi b/html/cgi-bin/routing.cgi
index c460a74..3b5a2f7 100644
--- a/html/cgi-bin/routing.cgi
+++ b/html/cgi-bin/routing.cgi
@@ -118,6 +118,11 @@ if ($settings{'ACTION'} eq $Lang::tr{'toggle enable disable'}) {
 }
 
 if ($settings{'ACTION'} eq $Lang::tr{'add'}) {
+
+	if (!&Network::check_prefix($settings{'IP'})){
+		$settings{'IP'} .= '/32';
+	}
+
 	# Convert subnet masks to CIDR notation.
 	$settings{'IP'} = &General::iporsubtocidr($settings{'IP'});
 
-- 
1.9.1

Re: [PATCH] BUG10941: fix single ip-addresses when no subnet given

[Michael Tremer]

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

On Fri, 2015-10-16 at 09:57 +0200, Alexander Marx wrote:
> Signed-off-by: Alexander Marx <alexander.marx(a)ipfire.org>
> ---
>  html/cgi-bin/routing.cgi | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/html/cgi-bin/routing.cgi b/html/cgi-bin/routing.cgi
> index c460a74..3b5a2f7 100644
> --- a/html/cgi-bin/routing.cgi
> +++ b/html/cgi-bin/routing.cgi
> @@ -118,6 +118,11 @@ if ($settings{'ACTION'} eq $Lang::tr{'toggle
> enable disable'}) {
>  }
>  
>  if ($settings{'ACTION'} eq $Lang::tr{'add'}) {
> +
> +	if (!&Network::check_prefix($settings{'IP'})){
> +		$settings{'IP'} .= '/32';
> +	}
> +

This won't work. The function &Network::check_prefix() takes the prefix
(i.e. the number after the slash, e.g. 24). You are passing the IP
address to the function which will never be a valid prefix and /32 will
always be appended, even to valid inputs like 192.168.0.0/24 (result:
192.168.0.0/24/32).

>  	# Convert subnet masks to CIDR notation.
>  	$settings{'IP'} = &General::iporsubtocidr($settings{'IP'});
> 

The remaining code should be amended that only prefixes are allowed
when /32 is appended to hosts.

-Michael 

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