Re: [PATCH 1/2] vpnmain.cgi: Fix for 2nd part of bug10595

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

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

Hello Adolf,

> On 11 Dec 2024, at 11:51, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
> 
> - Bug10595 had two parts in it and was closed after the first part was fixed. The second
>   part was still unfixed at that time. I cam across it when checking out an open bug on
>   a similar issue with OpenVPN.
> - I found the section that checks on the CA Name and modified it to also allow spaces.
> - Having modified that then the subroutines getsubjectfromcert and getCNfromcert required
>   to have quotation marks put around the parameter that had the CA Name with spaces in it
>   otherwise the openssl statement only got a filename with the first portion of the ca
>   name until the first space was encountered.
> - Tested this change out on my vm and it worked fine. I was able to upload a ca
>   certificate into IPSec and use spaces in the CA Name.
> 
> Fixes: Bug10595 part 2
> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
> ---
> html/cgi-bin/vpnmain.cgi | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 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 3541aaa29..694eeed76
> --- a/html/cgi-bin/vpnmain.cgi
> +++ b/html/cgi-bin/vpnmain.cgi
> @@ -245,7 +245,7 @@ sub callssl ($) {
> ###
> sub getCNfromcert ($) {
> #&General::log("ipsec", "Extracting name from $_[0]...");
> - my $temp = `/usr/bin/openssl x509 -text -in $_[0]`;
> + my $temp = `/usr/bin/openssl x509 -text -in '$_[0]'`;

Oh no, this is really bad code and potentially exploitable. The ‘’ make it at least safe for spaces as you intended, but someone could type in a name like “Bobby’ Tables” and terminate the quoted string early.

We have a function called &Generall::system_output() which takes the command as an array and returns the output:

  https://git.ipfire.org/?p=ipfire-2.x.git;a=blob;f=config/cfgroot/general-functions.pl;h=8ba6e3f79f0a9660ba8f8630ad0c7f1a3f6c988d;hb=HEAD#l54

It has safeguard so that nothing can be injected into the command line.

So the code will look a little bit like:

  my @output = &General::system_output(“openssl”, “x509”, “-text”, “-in”, “$_[0]”);

  foreach my $line (@output) {
    my $subject =~ /Subject:…/; # basically the entire regular expression
  }

Do you want to have a try to implement it this way? There should be some other places in vpnmain.cgi where this is being used.

> $temp =~ /Subject:.*CN\s*=\s*(.*)[\n]/;
> $temp = $1;
> $temp =~ s+/Email+, E+;
> @@ -259,7 +259,7 @@ sub getCNfromcert ($) {
> ###
> sub getsubjectfromcert ($) {
> #&General::log("ipsec", "Extracting subject from $_[0]...");
> - my $temp = `/usr/bin/openssl x509 -text -in $_[0]`;
> + my $temp = `/usr/bin/openssl x509 -text -in '$_[0]'`;
> $temp =~ /Subject: (.*)[\n]/;
> $temp = $1;
> $temp =~ s+/Email+, E+;
> @@ -644,8 +644,8 @@ END
> } elsif ($cgiparams{'ACTION'} eq $Lang::tr{'upload ca certificate'}) {
> &General::readhasharray("${General::swroot}/vpn/caconfig", \%cahash);
> 
> - if ($cgiparams{'CA_NAME'} !~ /^[a-zA-Z0-9]+$/) {
> - $errormessage = $Lang::tr{'name must only contain characters'};
> + if ($cgiparams{'CA_NAME'} !~ /^[a-zA-Z0-9 ]*$/) {
> + $errormessage = $Lang::tr{'ca name must only contain characters or spaces'};

Isn’t everything a character?

> goto UPLOADCA_ERROR;
> }
> 
> -- 
> 2.47.1
>