[v3,4/4] proxy.cgi: fix subnet comparison for proxy.pac generation

  The logic of subnet comparison is broken. E.g. if the blue netmask is
255.255.255.0, it's impossible to add a VPN subnet with the same netmask.
The fix simplifies the logic by using Network::network_equal.
---
 html/cgi-bin/proxy.cgi | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
  

Hi,

On Wed, 2018-02-14 at 20:35 +0100, Bernhard Held wrote:
> The logic of subnet comparison is broken. E.g. if the blue netmask is
> 255.255.255.0, it's impossible to add a VPN subnet with the same netmask.
> The fix simplifies the logic by using Network::network_equal.
> ---
>  html/cgi-bin/proxy.cgi | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
> index d565ffbdc..d641c3df9 100644
> --- a/html/cgi-bin/proxy.cgi
> +++ b/html/cgi-bin/proxy.cgi
> @@ -3066,9 +3066,10 @@ END
>  			foreach (@templist)
>  			{
>  				@temp = split(/\//);
> -				if (
> -					($temp[0] ne $netsettings{'GREEN_NETADDRESS'}) && ($temp[1] ne $netsettings{'GREEN_NETMASK'}) &&
> -					($temp[0] ne $netsettings{'BLUE_NETADDRESS'}) && ($temp[1] ne $netsettings{'BLUE_NETMASK'})
> +				unless (
> +					# GREEN or BLUE networks are already added to "DIRECT". Check if given network is different from these.
> +					&Network::network_equal("$temp[0]/$temp[1]", "$netsettings{'GREEN_NETADDRESS'}/$netsettings{'GREEN_NETMASK'}") ||
> +					&Network::network_equal("$temp[0]/$temp[1]", "$netsettings{'BLUE_NETADDRESS'}/$netsettings{'BLUE_NETMASK'}")
>  					)
>  				{
>  					print FILE " ||\n     (isInNet(myIpAddress(), \"$temp[0]\", \"$temp[1]\"))";

Strictly, this should be checking if the network in question is either
the GREEN or BLUE network, or if it is a subnet of thereof. This might
be a not so common use-case, but it would make the check more correct.

-Michael
  

> Michael Tremer <michael.tremer@ipfire.org> hat am 14. Februar 2018 um 23:14 geschrieben:
> On Wed, 2018-02-14 at 20:35 +0100, Bernhard Held wrote:
> > The logic of subnet comparison is broken. E.g. if the blue netmask is
> > 255.255.255.0, it's impossible to add a VPN subnet with the same netmask.
> > The fix simplifies the logic by using Network::network_equal.
> > ---
> >  html/cgi-bin/proxy.cgi | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
> > index d565ffbdc..d641c3df9 100644
> > --- a/html/cgi-bin/proxy.cgi
> > +++ b/html/cgi-bin/proxy.cgi
> > @@ -3066,9 +3066,10 @@ END
> >  			foreach (@templist)
> >  			{
> >  				@temp = split(/\//);
> > -				if (
> > -					($temp[0] ne $netsettings{'GREEN_NETADDRESS'}) && ($temp[1] ne $netsettings{'GREEN_NETMASK'}) &&
> > -					($temp[0] ne $netsettings{'BLUE_NETADDRESS'}) && ($temp[1] ne $netsettings{'BLUE_NETMASK'})
> > +				unless (
> > +					# GREEN or BLUE networks are already added to "DIRECT". Check if given network is different from these.
> > +					&Network::network_equal("$temp[0]/$temp[1]", "$netsettings{'GREEN_NETADDRESS'}/$netsettings{'GREEN_NETMASK'}") ||
> > +					&Network::network_equal("$temp[0]/$temp[1]", "$netsettings{'BLUE_NETADDRESS'}/$netsettings{'BLUE_NETMASK'}")
> >  					)
> >  				{
> >  					print FILE " ||\n     (isInNet(myIpAddress(), \"$temp[0]\", \"$temp[1]\"))";
> 
> Strictly, this should be checking if the network in question is either
> the GREEN or BLUE network, or if it is a subnet of thereof. This might
> be a not so common use-case, but it would make the check more correct.

I'm sorry, that's beyond my mission. I'm just striving to get my 2.5 year old bug report closed and that's it. I'm actually not even running a single IPFire instance, thus I feel unable to invest time in any feature enhancement.

Regards,
Bernhard