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

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

[Adolf Belka]

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

- 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]'`;
 	$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'};
 		goto UPLOADCA_ERROR;
 	}
 
-- 
2.47.1

[PATCH 2/2] en.pl: Update the wording for the check on the CA Name for upload

[Adolf Belka]

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

- This changes the wording to allowing characters and spaces.

Fixes: Bug10595 part 2
Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
---
 doc/language_issues.de | 1 +
 doc/language_issues.en | 1 +
 doc/language_issues.es | 1 +
 doc/language_issues.fr | 1 +
 doc/language_issues.it | 1 +
 doc/language_issues.nl | 1 +
 doc/language_issues.pl | 1 +
 doc/language_issues.ru | 1 +
 doc/language_issues.tr | 1 +
 doc/language_missings  | 8 ++++++++
 langs/en/cgi-bin/en.pl | 1 +
 11 files changed, 18 insertions(+)

diff --git a/doc/language_issues.de b/doc/language_issues.de
index 7883bef76..f83e1e775 100644
--- a/doc/language_issues.de
+++ b/doc/language_issues.de
@@ -930,6 +930,7 @@ WARNING: untranslated string: access point name = Access Point Name
 WARNING: untranslated string: access point name is invalid = Access Point Name is invalid
 WARNING: untranslated string: access point name is required = Access Point Name is required
 WARNING: untranslated string: aliases default interface = - Default Interface -
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: cake profile bridged-llcsnap 32 = Bridged LLC SNAP (32 bytes)
 WARNING: untranslated string: cake profile bridged-ptm 19 = Bridged PTM (19 bytes)
 WARNING: untranslated string: cake profile bridged-vcmux 24 = Bridged VC-MUX (24 bytes)
diff --git a/doc/language_issues.en b/doc/language_issues.en
index 3f1626b68..2a14bd370 100644
--- a/doc/language_issues.en
+++ b/doc/language_issues.en
@@ -360,6 +360,7 @@ WARNING: untranslated string: bytes received = Bytes Received
 WARNING: untranslated string: bytes sent = Bytes Sent
 WARNING: untranslated string: ca certificate = CA Certificate
 WARNING: untranslated string: ca name = CA name
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: cached = cached
 WARNING: untranslated string: cached memory = Cached Memory  
 WARNING: untranslated string: cached swap = Cached Swap
diff --git a/doc/language_issues.es b/doc/language_issues.es
index 0a89279d5..bfbd4a012 100644
--- a/doc/language_issues.es
+++ b/doc/language_issues.es
@@ -1003,6 +1003,7 @@ WARNING: untranslated string: access point name = Access Point Name
 WARNING: untranslated string: access point name is invalid = Access Point Name is invalid
 WARNING: untranslated string: access point name is required = Access Point Name is required
 WARNING: untranslated string: bypassed = Bypassed
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: cpu frequency = CPU frequency
 WARNING: untranslated string: data transfer = Data Transfer
 WARNING: untranslated string: dhcp fixed ip address in dynamic range = Fixed IP Address in dynamic range
diff --git a/doc/language_issues.fr b/doc/language_issues.fr
index 7f9349bc0..e1721e70e 100644
--- a/doc/language_issues.fr
+++ b/doc/language_issues.fr
@@ -968,6 +968,7 @@ WARNING: translation string unused: zoneconf val vlan tag assignment error
 WARNING: translation string unused: zoneconf val vlan tag range error
 WARNING: translation string unused: zoneconf val zoneslave amount error
 WARNING: untranslated string: bypassed = Bypassed
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: core notice 3 = available.
 WARNING: untranslated string: data transfer = Data Transfer
 WARNING: untranslated string: enable disable client = unknown string
diff --git a/doc/language_issues.it b/doc/language_issues.it
index 5870e2bc7..d21751c68 100644
--- a/doc/language_issues.it
+++ b/doc/language_issues.it
@@ -970,6 +970,7 @@ WARNING: untranslated string: available = available
 WARNING: untranslated string: block = Block
 WARNING: untranslated string: broken = Broken
 WARNING: untranslated string: bypassed = Bypassed
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: cake profile bridged-llcsnap 32 = Bridged LLC SNAP (32 bytes)
 WARNING: untranslated string: cake profile bridged-ptm 19 = Bridged PTM (19 bytes)
 WARNING: untranslated string: cake profile bridged-vcmux 24 = Bridged VC-MUX (24 bytes)
diff --git a/doc/language_issues.nl b/doc/language_issues.nl
index 88493d1d9..b9718913f 100644
--- a/doc/language_issues.nl
+++ b/doc/language_issues.nl
@@ -972,6 +972,7 @@ WARNING: untranslated string: available = available
 WARNING: untranslated string: block = Block
 WARNING: untranslated string: broken = Broken
 WARNING: untranslated string: bypassed = Bypassed
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: cake profile bridged-llcsnap 32 = Bridged LLC SNAP (32 bytes)
 WARNING: untranslated string: cake profile bridged-ptm 19 = Bridged PTM (19 bytes)
 WARNING: untranslated string: cake profile bridged-vcmux 24 = Bridged VC-MUX (24 bytes)
diff --git a/doc/language_issues.pl b/doc/language_issues.pl
index 5f3806102..b15e1bf63 100644
--- a/doc/language_issues.pl
+++ b/doc/language_issues.pl
@@ -897,6 +897,7 @@ WARNING: untranslated string: bit = bit
 WARNING: untranslated string: block = Block
 WARNING: untranslated string: broken = Broken
 WARNING: untranslated string: bypassed = Bypassed
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: cake profile bridged-llcsnap 32 = Bridged LLC SNAP (32 bytes)
 WARNING: untranslated string: cake profile bridged-ptm 19 = Bridged PTM (19 bytes)
 WARNING: untranslated string: cake profile bridged-vcmux 24 = Bridged VC-MUX (24 bytes)
diff --git a/doc/language_issues.ru b/doc/language_issues.ru
index 8891ce20e..c4c33bf32 100644
--- a/doc/language_issues.ru
+++ b/doc/language_issues.ru
@@ -892,6 +892,7 @@ WARNING: untranslated string: bit = bit
 WARNING: untranslated string: block = Block
 WARNING: untranslated string: broken = Broken
 WARNING: untranslated string: bypassed = Bypassed
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: cake profile bridged-llcsnap 32 = Bridged LLC SNAP (32 bytes)
 WARNING: untranslated string: cake profile bridged-ptm 19 = Bridged PTM (19 bytes)
 WARNING: untranslated string: cake profile bridged-vcmux 24 = Bridged VC-MUX (24 bytes)
diff --git a/doc/language_issues.tr b/doc/language_issues.tr
index c0cb2703a..56897ca62 100644
--- a/doc/language_issues.tr
+++ b/doc/language_issues.tr
@@ -957,6 +957,7 @@ WARNING: untranslated string: autonomous system = Autonomous System
 WARNING: untranslated string: available = available
 WARNING: untranslated string: broken = Broken
 WARNING: untranslated string: bypassed = Bypassed
+WARNING: untranslated string: ca name must only contain characters or spaces = CA Name must only contain characters or spaces.
 WARNING: untranslated string: cake profile bridged-llcsnap 32 = Bridged LLC SNAP (32 bytes)
 WARNING: untranslated string: cake profile bridged-ptm 19 = Bridged PTM (19 bytes)
 WARNING: untranslated string: cake profile bridged-vcmux 24 = Bridged VC-MUX (24 bytes)
diff --git a/doc/language_missings b/doc/language_missings
index 2a2333d94..f94e7f174 100644
--- a/doc/language_missings
+++ b/doc/language_missings
@@ -39,6 +39,7 @@
 < cake profile pppoe-ptm 27
 < cake profile pppoe-vcmux 32
 < cake profile raw 0
+< ca name must only contain characters or spaces
 < Captive heading terms
 < Captive heading voucher
 < Captive invalid coupon
@@ -122,6 +123,7 @@
 < access point name is required
 < addon
 < bypassed
+< ca name must only contain characters or spaces
 < cpu frequency
 < data transfer
 < dhcp fixed ip address in dynamic range
@@ -179,6 +181,7 @@
 < bewan adsl pci st
 < bewan adsl usb
 < bypassed
+< ca name must only contain characters or spaces
 < data transfer
 < extrahd because it it outside the allowed mount path
 < fwdfw syn flood protection
@@ -261,6 +264,7 @@
 < cake profile pppoe-ptm 27
 < cake profile pppoe-vcmux 32
 < cake profile raw 0
+< ca name must only contain characters or spaces
 < Captive
 < Captive 1day
 < Captive 1month
@@ -804,6 +808,7 @@
 < cake profile pppoe-ptm 27
 < cake profile pppoe-vcmux 32
 < cake profile raw 0
+< ca name must only contain characters or spaces
 < capabilities
 < Captive
 < Captive 1day
@@ -1387,6 +1392,7 @@
 < cake profile pppoe-ptm 27
 < cake profile pppoe-vcmux 32
 < cake profile raw 0
+< ca name must only contain characters or spaces
 < capabilities
 < Captive
 < Captive 1day
@@ -2403,6 +2409,7 @@
 < cake profile pppoe-ptm 27
 < cake profile pppoe-vcmux 32
 < cake profile raw 0
+< ca name must only contain characters or spaces
 < capabilities
 < Captive
 < Captive 1day
@@ -3400,6 +3407,7 @@
 < cake profile pppoe-ptm 27
 < cake profile pppoe-vcmux 32
 < cake profile raw 0
+< ca name must only contain characters or spaces
 < Captive delete logo
 < core update
 < cpu frequency
diff --git a/langs/en/cgi-bin/en.pl b/langs/en/cgi-bin/en.pl
index 5c8da52be..7576fbd0b 100644
--- a/langs/en/cgi-bin/en.pl
+++ b/langs/en/cgi-bin/en.pl
@@ -530,6 +530,7 @@
 'bytes sent' => 'Bytes Sent',
 'ca certificate' => 'CA Certificate',
 'ca name' => 'CA name',
+'ca name must only contain characters or spaces' => 'CA Name must only contain characters or spaces.',
 'cache management' => 'Cache management',
 'cache size' => 'Cache size (MB):',
 'cached' => 'cached',
-- 
2.47.1

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

[Michael Tremer]

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

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

[Adolf Belka]

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

Hi Michael,

On 11/12/2024 18:00, Michael Tremer wrote:
> 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.

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