- 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]'`; $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; }
- This changes the wording to allowing characters and spaces.
Fixes: Bug10595 part 2 Tested-by: Adolf Belka adolf.belka@ipfire.org Signed-off-by: Adolf Belka adolf.belka@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',
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-fun...
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-fun...
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
-
Adolf Belka -
Michael Tremer