mbox

BUG10941: fix single ip-addresses when no subnet given

Message ID 1444982254-5340-1-git-send-email-alexander.marx@ipfire.org
State Superseded
Headers

Message

Alexander Marx Oct. 16, 2015, 6:57 p.m. UTC 
  Signed-off-by: Alexander Marx <alexander.marx@ipfire.org>
---
 html/cgi-bin/routing.cgi | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Michael Tremer Oct. 17, 2015, 2:54 a.m. UTC | #1 
On Fri, 2015-10-16 at 09:57 +0200, Alexander Marx wrote:
> Signed-off-by: Alexander Marx <alexander.marx@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
Alexander Marx Oct. 17, 2015, 4:48 a.m. UTC | #2 
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@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.

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.


>>   	# 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 Tremer Oct. 17, 2015, 7:56 a.m. UTC | #3 
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@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