[PATCH 0/2] proxy.cgi fixes for bugzilla #10852

[PATCH 0/2] proxy.cgi fixes for bugzilla #10852

[Bernhard Held]

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

Hi all,

these patches fix the possibility to add more subnets for the proxy, also reported in bugzilla #10852.

Have fun,
Bernhard

[PATCH 1/2] proxy.cgi: strip newline from subnet parameter

[Bernhard Held]

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

$temp[1] might end with a newline; this is unavoidable when specifying
serveral subnets. Thus, 'chomp $temp[1];' has to be moved before the
comparisons with the green and blue subnets. Otherwise the comparison
might always be true due to the newline.
---
 html/cgi-bin/proxy.cgi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
index 6aa14e15a..ea3b41126 100644
--- a/html/cgi-bin/proxy.cgi
+++ b/html/cgi-bin/proxy.cgi
@@ -3064,12 +3064,12 @@ END
 			foreach (@templist)
 			{
 				@temp = split(/\//);
+				chomp $temp[1];
 				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'})
 					)
 				{
-					chomp $temp[1];
 					print FILE " ||\n     (isInNet(myIpAddress(), \"$temp[0]\", \"$temp[1]\"))";
 				}
 			}
-- 
2.16.1

Re: [PATCH 1/2] proxy.cgi: strip newline from subnet parameter

[Michael Tremer]

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

This patch is fine.

On Sun, 2018-02-11 at 19:51 +0100, Bernhard Held wrote:
> $temp[1] might end with a newline; this is unavoidable when specifying
> serveral subnets. Thus, 'chomp $temp[1];' has to be moved before the
> comparisons with the green and blue subnets. Otherwise the comparison
> might always be true due to the newline.
> ---
>  html/cgi-bin/proxy.cgi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
> index 6aa14e15a..ea3b41126 100644
> --- a/html/cgi-bin/proxy.cgi
> +++ b/html/cgi-bin/proxy.cgi
> @@ -3064,12 +3064,12 @@ END
>  			foreach (@templist)
>  			{
>  				@temp = split(/\//);
> +				chomp $temp[1];
>  				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'})
>  					)
>  				{
> -					chomp $temp[1];
>  					print FILE " ||\n     (isInNet(myIpAddress(), \"$temp[0]\", \"$temp[1]\"))";
>  				}
>  			}

[PATCH 2/2] proxy.cgi: fix subnet comparison

[Bernhard Held]

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

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 proposed patch compares the subnets individually.
---
 html/cgi-bin/proxy.cgi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
index ea3b41126..4993dde86 100644
--- a/html/cgi-bin/proxy.cgi
+++ b/html/cgi-bin/proxy.cgi
@@ -3066,8 +3066,8 @@ END
 				@temp = split(/\//);
 				chomp $temp[1];
 				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'})
+					(($temp[0] ne $netsettings{'GREEN_NETADDRESS'}) || ($temp[1] ne $netsettings{'GREEN_NETMASK'})) &&
+					(($temp[0] ne $netsettings{'BLUE_NETADDRESS'}) || ($temp[1] ne $netsettings{'BLUE_NETMASK'}))
 					)
 				{
 					print FILE " ||\n     (isInNet(myIpAddress(), \"$temp[0]\", \"$temp[1]\"))";
-- 
2.16.1

Re: [PATCH 2/2] proxy.cgi: fix subnet comparison

[Michael Tremer]

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

I think you should better use &Network::equals() from
/var/ipfire/network-functions.pl. This will take care of converting
subnet masks to prefix notation and vice-versa.

Best,
-Michael

On Sun, 2018-02-11 at 19:51 +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 proposed patch compares the subnets individually.
> ---
>  html/cgi-bin/proxy.cgi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
> index ea3b41126..4993dde86 100644
> --- a/html/cgi-bin/proxy.cgi
> +++ b/html/cgi-bin/proxy.cgi
> @@ -3066,8 +3066,8 @@ END
>  				@temp = split(/\//);
>  				chomp $temp[1];
>  				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'})
> +					(($temp[0] ne $netsettings{'GREEN_NETADDRESS'}) || ($temp[1] ne $netsettings{'GREEN_NETMASK'})) &&
> +					(($temp[0] ne $netsettings{'BLUE_NETADDRESS'}) || ($temp[1] ne $netsettings{'BLUE_NETMASK'}))
>  					)
>  				{
>  					print FILE " ||\n     (isInNet(myIpAddress(), \"$temp[0]\", \"$temp[1]\"))";

Re: [PATCH 2/2] proxy.cgi: fix subnet comparison

[Tom Rymes]

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

Bernhard,

Does any of this also apply to the subnet comparisons made when adding subnets in the "Firewall Groups" portion of the WUI?

Tom

> On Feb 11, 2018, at 1:52 PM, Bernhard Held <berny156(a)gmx.de> 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 proposed patch compares the subnets individually.
> ---
> html/cgi-bin/proxy.cgi | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/html/cgi-bin/proxy.cgi b/html/cgi-bin/proxy.cgi
> index ea3b41126..4993dde86 100644
> --- a/html/cgi-bin/proxy.cgi
> +++ b/html/cgi-bin/proxy.cgi
> @@ -3066,8 +3066,8 @@ END
>                @temp = split(/\//);
>                chomp $temp[1];
>                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'})
> +                    (($temp[0] ne $netsettings{'GREEN_NETADDRESS'}) || ($temp[1] ne $netsettings{'GREEN_NETMASK'})) &&
> +                    (($temp[0] ne $netsettings{'BLUE_NETADDRESS'}) || ($temp[1] ne $netsettings{'BLUE_NETMASK'}))
>                    )
>                {
>                    print FILE " ||\n     (isInNet(myIpAddress(), \"$temp[0]\", \"$temp[1]\"))";
> -- 
> 2.16.1
> 

Re: [PATCH 0/2] proxy.cgi fixes for bugzilla #10852

[Michael Tremer]

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

Hello Bernhard,

thank you very much for contributing to IPFire and welcome to the team!

Best,
-Michael

On Sun, 2018-02-11 at 19:51 +0100, Bernhard Held wrote:
> Hi all,
> 
> these patches fix the possibility to add more subnets for the proxy, also reported in bugzilla #10852.
> 
> Have fun,
> Bernhard