ovpnmain.cgi: Fix for bug#11048 - insecure download icon for connections with a password

  - The insecure package download icon is shown if entry 41 in /var/ipfire/ovpn/ovpnconfig
   is set to no-pass. The code block on ovpnmain.cgi that deals with this checks if the
   connection is a host and if the first password entry is a null. Then it adds no-pass
   to ovpnconfig.
- The same block of code is also used for when he connection is edited. However at this
   stage the password entry is back to null because the password value is only kept until
   the connection has been saved. Therefore doing an edit results in the password value
   being taken as null even for connections with a password.
- This fix checks the password value only if entry 41 in the ovpnconfig file is a null.
   If it is a null then it enters no-pass if the password is a null and it enters pass
   if the password contains characters. This way the entry 41 always contains either pass
   or no-pass, except when the connection is being first added and saved.
- When adding this fix into a Core Update the update.sh script will need to check if
   ovpnconfig exists and then add pass to all lines that have a null at entry 41. This will
   only fix those connections that have not already been edited. Any connections already
   edited will have no-pass at entry 41 of ovpnconfig and will therefore show the insecure
   package download icon.
-  The only way I can think of dealing with entries that already have no-pass added is to
   go through all .p12 files in the certs directory and if there is a connection entry
   with that name then to change the no-pass to a pass. However that will only work if the
   connection name has been set the same as the certificate name, which is not a
   requirement.
- So I think we can only fix those coneections that have never been edited. Any connections
   with passwords that have already been edited and containg no-pass in ovpnconfig and
   showing the insecure package download icon will have to be manually dealt with by users.
- I think that should still be okay because they currently have two icons when they
   shouldn't and that will continue to be the case if they don't carry out a manual edit
   of ovpnconfig.
- Maybe someone can think of an alternative way of identifying all connections using a
   password so that they can have entry 41 changed to pass. I haven't been able to do that
   so far
- Looking forward to feedback

Tested-by: Adolf Belka <adolf.belka@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 html/cgi-bin/ovpnmain.cgi | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
  

Hi All,

I have marked this patch as superseded as it became clear that also the 
N2N entries in ovpnconfig needed to be explicitly specified as no-pass.

Also with the no password insecure option two icons were still shown for 
the download. There should only be one icon for each situation. One 
downloads the package with no password option and the certificates built 
into the .ovpn file and the other downloads the package with the .p12 
file protected by a password.

A new patch will be put together covering all above and tested and then 
submitted together with a tested script for giving all connections, N2N, 
password protected hosts and hosts with no password in ovpnconfig an 
explicit status of pass or no-pass.

Regards,
Adolf.


On 10/02/2023 19:13, Adolf Belka wrote:
> - The insecure package download icon is shown if entry 41 in /var/ipfire/ovpn/ovpnconfig
>     is set to no-pass. The code block on ovpnmain.cgi that deals with this checks if the
>     connection is a host and if the first password entry is a null. Then it adds no-pass
>     to ovpnconfig.
> - The same block of code is also used for when he connection is edited. However at this
>     stage the password entry is back to null because the password value is only kept until
>     the connection has been saved. Therefore doing an edit results in the password value
>     being taken as null even for connections with a password.
> - This fix checks the password value only if entry 41 in the ovpnconfig file is a null.
>     If it is a null then it enters no-pass if the password is a null and it enters pass
>     if the password contains characters. This way the entry 41 always contains either pass
>     or no-pass, except when the connection is being first added and saved.
> - When adding this fix into a Core Update the update.sh script will need to check if
>     ovpnconfig exists and then add pass to all lines that have a null at entry 41. This will
>     only fix those connections that have not already been edited. Any connections already
>     edited will have no-pass at entry 41 of ovpnconfig and will therefore show the insecure
>     package download icon.
> -  The only way I can think of dealing with entries that already have no-pass added is to
>     go through all .p12 files in the certs directory and if there is a connection entry
>     with that name then to change the no-pass to a pass. However that will only work if the
>     connection name has been set the same as the certificate name, which is not a
>     requirement.
> - So I think we can only fix those coneections that have never been edited. Any connections
>     with passwords that have already been edited and containg no-pass in ovpnconfig and
>     showing the insecure package download icon will have to be manually dealt with by users.
> - I think that should still be okay because they currently have two icons when they
>     shouldn't and that will continue to be the case if they don't carry out a manual edit
>     of ovpnconfig.
> - Maybe someone can think of an alternative way of identifying all connections using a
>     password so that they can have entry 41 changed to pass. I haven't been able to do that
>     so far
> - Looking forward to feedback
> 
> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
> ---
>   html/cgi-bin/ovpnmain.cgi | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi
> index 42a7354fc..2586a1796 100644
> --- a/html/cgi-bin/ovpnmain.cgi
> +++ b/html/cgi-bin/ovpnmain.cgi
> @@ -4326,9 +4326,13 @@ if ($cgiparams{'TYPE'} eq 'net') {
>   	$confighash{$key}[39]		= $cgiparams{'DAUTH'};
>   	$confighash{$key}[40]		= $cgiparams{'DCIPHER'};
>   
> -	if (($cgiparams{'TYPE'} eq 'host') && ($cgiparams{'CERT_PASS1'} eq "")) {
> -		$confighash{$key}[41] = "no-pass";
> -	}
> +  if ($confighash{$key}[41] eq "") {
> +    if (($cgiparams{'TYPE'} eq 'host') && ($cgiparams{'CERT_PASS1'} eq "")) {
> +      $confighash{$key}[41] = "no-pass";
> +    } elsif (($cgiparams{'TYPE'} eq 'host') && ($cgiparams{'CERT_PASS1'} ne "")) {
> +      $confighash{$key}[41] = "pass";
> +    }
> +  }
>   
>      $confighash{$key}[42] = 'HOTP/T30/6';
>   	$confighash{$key}[43] = $cgiparams{'OTP_STATE'};