Re: [PATCH 1/2] vpnmain.cgi: Fixes bug12298 - IPSec password cannot use semicolon

[Michael Tremer <michael.tremer@ipfire.org>]

Hello Adolf,

Oh this is bad. Not the patch, what we have there before…

I would say I will accept this patch as it is because it slightly mitigates the problem. However, there is still a chance to run shell commands using backticks and $(…).

Would you be able to rewrite this command to use one of the &General::system* commands? That way, there will no command injection be possible any more and we can in theory allow quotes and semicolons again.

Best,
-Michael

> On 9 Mar 2025, at 14:12, Adolf Belka <adolf.belka@ipfire.org> wrote:
> 
> - The password for the pkcs12 certificate is passed to the open ssl command via $opt but
>   it is not quoted and so the ; is taken as the end of the command rather than as part
>   of the password. This also means that a pkcs12 file is not created and the .pem
>   intermediate file is what is left in the directory.
> - This patch makes the -passout option quoted in the same way as the -name and -caname
>   options.
> - Based on being the same as the name and caname parts in $opt, I believe that this should
>   not give rise to a vulnerability but I am open to being corrected.
> - By quoting the -passout then the password must not contain double quotation marks, ",
>   so a test for the password containing a " has been added.
> - The message about the use of the double quotation mark has been added to the english,
>   dutch and german language files. Feel free to correct if what I have used is not
>   correct. Those are in the other patch of this patch set.
> - Tested out on my testbed system. I was able to create a pkcs12 certificate with a
>   password containing a variety of characters, including the semicolon, and getting
>   a message that the password contains a double quotation mark when I used that.
> 
> Fixes: bug12298
> Tested-by: Adolf Belka <adolf.belka@ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
> ---
> html/cgi-bin/vpnmain.cgi | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
> mode change 100755 => 100644 html/cgi-bin/vpnmain.cgi
> 
> diff --git a/html/cgi-bin/vpnmain.cgi b/html/cgi-bin/vpnmain.cgi
> old mode 100755
> new mode 100644
> index c9bbbb494..8106ee24e
> --- a/html/cgi-bin/vpnmain.cgi
> +++ b/html/cgi-bin/vpnmain.cgi
> @@ -2149,6 +2149,10 @@ END
> $errormessage = $Lang::tr{'password too short'};
> goto VPNCONF_ERROR;
> }
> + if ($cgiparams{'CERT_PASS1'} =~ /["]/) {
> + $errormessage = $Lang::tr{'password has quotation mark'};
> + goto VPNCONF_ERROR;
> + }
> if ($cgiparams{'CERT_PASS1'} ne $cgiparams{'CERT_PASS2'}) {
> $errormessage = $Lang::tr{'passwords do not match'};
> goto VPNCONF_ERROR;
> @@ -2226,7 +2230,7 @@ END
> $opt .= " -inkey ${General::swroot}/certs/$cgiparams{'NAME'}key.pem";
> $opt .= " -in ${General::swroot}/certs/$cgiparams{'NAME'}cert.pem";
> $opt .= " -name \"$cgiparams{'NAME'}\"";
> - $opt .= " -passout pass:$cgiparams{'CERT_PASS1'}";
> + $opt .= " -passout pass:\"$cgiparams{'CERT_PASS1'}\"";
> $opt .= " -certfile ${General::swroot}/ca/cacert.pem";
> $opt .= " -caname \"$vpnsettings{'ROOTCERT_ORGANIZATION'} CA\"";
> $opt .= " -out ${General::swroot}/certs/$cgiparams{'NAME'}.p12";
> -- 
> 2.48.1
> 
>