From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject:
 Re: [PATCH v3 4/4] proxy.cgi: fix subnet comparison for proxy.pac generation
Date: Wed, 14 Feb 2018 22:14:05 +0000
Message-ID: <1518646445.6463.6.camel@ipfire.org>
In-Reply-To: <20180214193522.26880-5-berny156@gmx.de>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============3802849262963965252=="
List-Id: <development.lists.ipfire.org>

--===============3802849262963965252==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

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(-)
>=20
> 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 =3D split(/\//);
> -				if (
> -					($temp[0] ne $netsettings{'GREEN_NETADDRESS'}) && ($temp[1] ne $netse=
ttings{'GREEN_NETMASK'}) &&
> -					($temp[0] ne $netsettings{'BLUE_NETADDRESS'}) && ($temp[1] ne $netset=
tings{'BLUE_NETMASK'})
> +				unless (
> +					# GREEN or BLUE networks are already added to "DIRECT". Check if give=
n network is different from these.
> +					&Network::network_equal("$temp[0]/$temp[1]", "$netsettings{'GREEN_NET=
ADDRESS'}/$netsettings{'GREEN_NETMASK'}") ||
> +					&Network::network_equal("$temp[0]/$temp[1]", "$netsettings{'BLUE_NETA=
DDRESS'}/$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

--===============3802849262963965252==--