From 23d48f0b7f6a4756c73074f109b9db2a05599251 Mon Sep 17 00:00:00 2001 From: Stefan Ferstl Date: Fri, 20 Jun 2014 15:20:07 +0200 Subject: [PATCH 1/7] OpenVPN: Use the correct CN when saving certificates This addresses Bugzilla issue #10552 OpenVPN client certificates were saved by regexing the 'CN=' string in the output of '/usr/bin/openssl x509 -text -in '. However, the output of this command may not print the correct CN. For example, if the certificate's subject contains an emailAddress attribute, it will be appended to the CN separated with a slash ('/') character. There might also be some other edge cases with unusual certificate names that cause the regex not to work correctly. This change makes OpenSSL output the subject with each attribute on one line which makes parsing the CN much easier and safer. The CN is now guaranteed to be on exactly one line. --- html/cgi-bin/ovpnmain.cgi | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi index b2ce05e97a..1c15135c29 100644 --- a/html/cgi-bin/ovpnmain.cgi +++ b/html/cgi-bin/ovpnmain.cgi @@ -3891,11 +3891,9 @@ if ($cgiparams{'TYPE'} eq 'net') { &deletebackupcert(); } - my $temp = `/usr/bin/openssl x509 -text -in ${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem`; - $temp =~ /Subject:.*CN=(.*)[\n]/; + my $temp = `/usr/bin/openssl x509 -subject -nameopt sep_multiline,sname,esc_ctrl,esc_msb -noout -in ${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem`; + $temp =~ /.*CN=(.+)[\n]/; $temp = $1; - $temp =~ s+/Email+, E+; - $temp =~ s/ ST=/ S=/; $cgiparams{'CERT_NAME'} = $temp; $cgiparams{'CERT_NAME'} =~ s/,//g; $cgiparams{'CERT_NAME'} =~ s/\'//g; @@ -3945,14 +3943,13 @@ if ($cgiparams{'TYPE'} eq 'net') { } } - my $temp = `/usr/bin/openssl x509 -text -in ${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem`; - $temp =~ /Subject:.*CN=(.*)[\n]/; + my $temp = `/usr/bin/openssl x509 -subject -nameopt sep_multiline,sname,esc_ctrl,esc_msb -noout -in ${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem`; + $temp =~ /.*CN=(.+)[\n]/; $temp = $1; - $temp =~ s+/Email+, E+; - $temp =~ s/ ST=/ S=/; $cgiparams{'CERT_NAME'} = $temp; $cgiparams{'CERT_NAME'} =~ s/,//g; $cgiparams{'CERT_NAME'} =~ s/\'//g; + if ($cgiparams{'CERT_NAME'} eq '') { unlink ("${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem"); $errormessage = $Lang::tr{'could not retrieve common name from certificate'}; From 4ed03dfc01c7148aa4a036e3e7be1c0a7ecc23b6 Mon Sep 17 00:00:00 2001 From: Stefan Ferstl Date: Fri, 20 Jun 2014 18:09:27 +0200 Subject: [PATCH 2/7] OpenVPN: Extract the CN correctly and remain compatible with incorrectly saved CNs This addresses Bugzilla issue #10552 The regex that was used to extract the CN from the certificate subject extracted everything after the 'CN=' part which included possible other attributes in the subject, e.g. an emailAddress. This commit does mainly fix the regex. But since the same issue occurred when client certificates were saved, some compatibility code had to be added to still support previously and incorrectly saved certificates. --- config/ovpn/verify | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/config/ovpn/verify b/config/ovpn/verify index 1a1fcb501d..2ac599a46b 100644 --- a/config/ovpn/verify +++ b/config/ovpn/verify @@ -23,15 +23,14 @@ require '/var/ipfire/general-functions.pl'; -my $DEPTH = $ARGV[0]; -my $CN = $ARGV[1]; +my $DEPTH = $ARGV[0]; +my $CN = $ARGV[1]; # Exit immediately for every certificate depth other than 0. exit 0 unless ($DEPTH eq "0"); # Strip the CN from the X509 identifier. -$CN =~ /(\/|,\ )CN=(.*)$/i; -$CN = $2; +$CN =~ s/.*CN=([^,]+).*/$1/g; my %confighash = (); if (-f "${General::swroot}/ovpn/ovpnconfig"){ @@ -48,9 +47,18 @@ if (-f "${General::swroot}/ovpn/ovpnconfig"){ # Search for a matching CN. exit 0 if ($cn eq $CN); - # Compatibility code for incorrectly saved CNs. + # Compatibility code for incorrectly saved CNs: + # + my $cn_matcher = "$CN(\/.+=.+)?"; + + # 1) try to match an incorrectly saved CN + # See https://bugzilla.ipfire.org/show_bug.cgi?id=10552 + exit 0 if($cn =~ /$cn_matcher/); + + # 2) Handle OpenVPN's substitutions of space characters + # See http://lists.ipfire.org/pipermail/development/2013-January/000225.html $cn =~ s/\ /_/g; - exit 0 if ($cn eq $CN); + exit 0 if ($cn =~ /$cn_matcher/); } } From 9ecf2065f774ae29c5e0cc6806c3df59e9485886 Mon Sep 17 00:00:00 2001 From: Stefan Ferstl Date: Sat, 21 Jun 2014 14:29:43 +0200 Subject: [PATCH 3/7] OpenVPN: Fix connection status for incorrectly saved CNs This addresses Bugzilla issue #10552 In case of incorrectly saved CNs (i.e. CNs containing additional attributs from the certificate subject), the connection status of the client's was always 'DISCONNECTED'. This commit fixes this issue by matching the client's CN with the entry in ovpnconfig. --- html/cgi-bin/ovpnmain.cgi | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi index 1c15135c29..812babb3e1 100644 --- a/html/cgi-bin/ovpnmain.cgi +++ b/html/cgi-bin/ovpnmain.cgi @@ -5117,7 +5117,9 @@ END $cn = $match[1]; } $cn =~ s/[_]/ /g; - if ($cn eq "$confighash{$key}[2]") { + # For compatibility reasons, the CNs cannot be compared exactly with 'eq'. + # See https://bugzilla.ipfire.org/show_bug.cgi?id=10552 . + if ($confighash{$key}[2] =~ /$cn(\/.+=.+)?/) { $col1="bgcolor='${Header::colourgreen}'"; $active = "$Lang::tr{'capsopen'}"; } From 49a6b8981a828b1725231448b75f088ce501ed2b Mon Sep 17 00:00:00 2001 From: Stefan Ferstl Date: Mon, 23 Jun 2014 20:43:13 +0200 Subject: [PATCH 4/7] OpenVPN: Use a stricter pattern to get the certificate's CN This addresses Bugzilla issue #10552 The OpenSSL documentation [1] is pretty clear about how the subject is formatted in a multiline output. Thus, the regex to grep the CN can be made much stricter. [1] https://www.openssl.org/docs/apps/x509.html#NAME_OPTIONS --- html/cgi-bin/ovpnmain.cgi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi index 812babb3e1..0644cb9c52 100644 --- a/html/cgi-bin/ovpnmain.cgi +++ b/html/cgi-bin/ovpnmain.cgi @@ -3892,7 +3892,7 @@ if ($cgiparams{'TYPE'} eq 'net') { } my $temp = `/usr/bin/openssl x509 -subject -nameopt sep_multiline,sname,esc_ctrl,esc_msb -noout -in ${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem`; - $temp =~ /.*CN=(.+)[\n]/; + $temp =~ /[\ ]+CN=(.+)$/; $temp = $1; $cgiparams{'CERT_NAME'} = $temp; $cgiparams{'CERT_NAME'} =~ s/,//g; @@ -3944,7 +3944,7 @@ if ($cgiparams{'TYPE'} eq 'net') { } my $temp = `/usr/bin/openssl x509 -subject -nameopt sep_multiline,sname,esc_ctrl,esc_msb -noout -in ${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem`; - $temp =~ /.*CN=(.+)[\n]/; + $temp =~ /[\ ]+CN=(.+)$/; $temp = $1; $cgiparams{'CERT_NAME'} = $temp; $cgiparams{'CERT_NAME'} =~ s/,//g; From eb26f72437245cf23e6323f28bd6ae216c206ff0 Mon Sep 17 00:00:00 2001 From: Stefan Ferstl Date: Tue, 24 Jun 2014 00:32:50 +0200 Subject: [PATCH 5/7] OpenVPN: Improve matching of CNs This addresses Bugzilla issue #10552 This commit contains some improvement in matching CNs while remaining compatible with possible incorrectly saved CNs in ovpnconfig. --- config/ovpn/verify | 11 +++++------ html/cgi-bin/ovpnmain.cgi | 12 ++++++++++-- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/config/ovpn/verify b/config/ovpn/verify index 2ac599a46b..45699b7d75 100644 --- a/config/ovpn/verify +++ b/config/ovpn/verify @@ -23,8 +23,8 @@ require '/var/ipfire/general-functions.pl'; -my $DEPTH = $ARGV[0]; -my $CN = $ARGV[1]; +my $DEPTH = $ARGV[0]; +my $CN = $ARGV[1]; # Exit immediately for every certificate depth other than 0. exit 0 unless ($DEPTH eq "0"); @@ -48,17 +48,16 @@ if (-f "${General::swroot}/ovpn/ovpnconfig"){ exit 0 if ($cn eq $CN); # Compatibility code for incorrectly saved CNs: - # - my $cn_matcher = "$CN(\/.+=.+)?"; # 1) try to match an incorrectly saved CN # See https://bugzilla.ipfire.org/show_bug.cgi?id=10552 - exit 0 if($cn =~ /$cn_matcher/); + $cn =~ s/($CN)(\/.+)?/$1/g; # 2) Handle OpenVPN's substitutions of space characters # See http://lists.ipfire.org/pipermail/development/2013-January/000225.html $cn =~ s/\ /_/g; - exit 0 if ($cn =~ /$cn_matcher/); + + exit 0 if ($cn eq $CN); } } diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi index 0644cb9c52..4824033199 100644 --- a/html/cgi-bin/ovpnmain.cgi +++ b/html/cgi-bin/ovpnmain.cgi @@ -5108,7 +5108,9 @@ END }else { my $cn; + my $config_cn; my @match = (); + foreach my $line (@status) { chomp($line); if ( $line =~ /^(.+),(\d+\.\d+\.\d+\.\d+\:\d+),(\d+),(\d+),(.+)/) { @@ -5116,10 +5118,16 @@ END if ($match[1] ne "Common Name") { $cn = $match[1]; } + #Handle OpenVPN's substitutions of space characters + # See http://lists.ipfire.org/pipermail/development/2013-January/000225.html $cn =~ s/[_]/ /g; - # For compatibility reasons, the CNs cannot be compared exactly with 'eq'. + + # Work around incorrectly saved CNs # See https://bugzilla.ipfire.org/show_bug.cgi?id=10552 . - if ($confighash{$key}[2] =~ /$cn(\/.+=.+)?/) { + $config_cn = $confighash{$key}[2]; + $config_cn =~ s/($cn)(\/.+)?/$1/g; + + if ($config_cn eq $cn) { $col1="bgcolor='${Header::colourgreen}'"; $active = "$Lang::tr{'capsopen'}"; } From 80b1dcf192e4832e8472037b64c23b9f570a5639 Mon Sep 17 00:00:00 2001 From: Stefan Ferstl Date: Tue, 24 Jun 2014 01:12:26 +0200 Subject: [PATCH 6/7] OpenVPN: Stricter CN extraction from cert file or CSR This addresses Bugzilla issue #10552 The OpenSSL specification says 4 spaces at the beginning. So this should be part of the regular expression. --- html/cgi-bin/ovpnmain.cgi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi index 4824033199..125f009c3c 100644 --- a/html/cgi-bin/ovpnmain.cgi +++ b/html/cgi-bin/ovpnmain.cgi @@ -3892,7 +3892,7 @@ if ($cgiparams{'TYPE'} eq 'net') { } my $temp = `/usr/bin/openssl x509 -subject -nameopt sep_multiline,sname,esc_ctrl,esc_msb -noout -in ${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem`; - $temp =~ /[\ ]+CN=(.+)$/; + $temp =~ /^[\ ]{4}CN=(.+)$/m; $temp = $1; $cgiparams{'CERT_NAME'} = $temp; $cgiparams{'CERT_NAME'} =~ s/,//g; @@ -3944,7 +3944,7 @@ if ($cgiparams{'TYPE'} eq 'net') { } my $temp = `/usr/bin/openssl x509 -subject -nameopt sep_multiline,sname,esc_ctrl,esc_msb -noout -in ${General::swroot}/ovpn/certs/$cgiparams{'NAME'}cert.pem`; - $temp =~ /[\ ]+CN=(.+)$/; + $temp =~ /^[\ ]{4}CN=(.+)$/m; $temp = $1; $cgiparams{'CERT_NAME'} = $temp; $cgiparams{'CERT_NAME'} =~ s/,//g; From 93de7df40a689bb9d0024fc79957cbede0e55cd4 Mon Sep 17 00:00:00 2001 From: Stefan Ferstl Date: Thu, 26 Jun 2014 22:54:43 +0200 Subject: [PATCH 7/7] OpenVPN: Avoid constructing regular expressions with external data This addresses Bugzilla issue #10552 The previous solution to verify a client's CN was to construct a regular expression to match it against the entries in ovpnconfig. This must be avoided in order the client's CN is not guaranteed to be spoiled. --- config/ovpn/verify | 5 +++-- html/cgi-bin/ovpnmain.cgi | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/config/ovpn/verify b/config/ovpn/verify index 45699b7d75..ed13a468d6 100644 --- a/config/ovpn/verify +++ b/config/ovpn/verify @@ -30,7 +30,8 @@ my $CN = $ARGV[1]; exit 0 unless ($DEPTH eq "0"); # Strip the CN from the X509 identifier. -$CN =~ s/.*CN=([^,]+).*/$1/g; +$CN =~ /(\/|,\ )?CN=([^,\/ ]+)?/i; +$CN = $2; my %confighash = (); if (-f "${General::swroot}/ovpn/ovpnconfig"){ @@ -51,7 +52,7 @@ if (-f "${General::swroot}/ovpn/ovpnconfig"){ # 1) try to match an incorrectly saved CN # See https://bugzilla.ipfire.org/show_bug.cgi?id=10552 - $cn =~ s/($CN)(\/.+)?/$1/g; + $cn =~ s/^([^\/]+)(\/.*)?$/$1/g; # 2) Handle OpenVPN's substitutions of space characters # See http://lists.ipfire.org/pipermail/development/2013-January/000225.html diff --git a/html/cgi-bin/ovpnmain.cgi b/html/cgi-bin/ovpnmain.cgi index 125f009c3c..70e232a8a5 100644 --- a/html/cgi-bin/ovpnmain.cgi +++ b/html/cgi-bin/ovpnmain.cgi @@ -5125,7 +5125,7 @@ END # Work around incorrectly saved CNs # See https://bugzilla.ipfire.org/show_bug.cgi?id=10552 . $config_cn = $confighash{$key}[2]; - $config_cn =~ s/($cn)(\/.+)?/$1/g; + $config_cn =~ s/^([^\/]+)(\/.*)?/$1/g; if ($config_cn eq $cn) { $col1="bgcolor='${Header::colourgreen}'";