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

  - 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@ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka@ipfire.org>
---
 html/cgi-bin/vpnmain.cgi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
 mode change 100755 => 100644 html/cgi-bin/vpnmain.cgi
  

Hello Adolf,

> On 11 Dec 2024, at 11:51, Adolf Belka <adolf.belka@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@ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka@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
>
  

Hi Michael,

On 11/12/2024 18:00, Michael Tremer wrote:
> Hello Adolf,
> 
>> On 11 Dec 2024, at 11:51, Adolf Belka <adolf.belka@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@ipfire.org>
>> Signed-off-by: Adolf Belka <adolf.belka@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.

Just goes to show where my limits are. Now I know better.

> 
> 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.

Yes, sure I will have a go at trying to use that approach and submit a v2 version of the patch set.

> 
>> $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?

Yes, technically that is correct but then it should have previously said that the name must only contain alphanumerics. The same statement is also used for validation of the Name which is also only allowed to have alphanumerics but the same message is used there of name must only contain characters.

Should I change those uses of the word characters to alphanumerics.

The wording is correct for the PSK where it says invalid characters found in pre-shared key but that was modified to say Invalid single quotation mark found in pre-shared key. However I realise that I changed the English wording as follows

-'invalid characters found in pre-shared key' => 'Invalid characters found in pre-shared key.',
+'invalid characters found in pre-shared key' => 'Invalid single quotation mark found in pre-shared key.',

and I should also have changed the reference wording on the left hand side, otherwise people doing other language translations will not choose the correct wording.

Regards,
Adolf.

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