Bug 10552 - tls-verify with email address in X509
Summary: tls-verify with email address in X509
Status: CLOSED FIXED
Alias: None
Product: IPFire
Classification: Unclassified
Component: openvpn (show other bugs)
Version: 2
Hardware: all Unspecified
: - Unknown - Minor Usability
Assignee: Michael Tremer
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-15 15:45 UTC by Stefan Ferstl
Modified: 2015-11-01 01:19 UTC (History)
1 user (show)

See Also:


Attachments
OpenVPN verify (361 bytes, text/plain)
2014-06-15 17:03 UTC, Michael Tremer
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Ferstl 2014-06-15 15:45:59 UTC
Note: There is a GitHub Pull Request [1] that solves this issue.


OpenVPN’s TLS verification will fail when the client’s certificate contains an “emailAddress” attribute.
The reason is that when a client certificate is uploaded to IPFire the resulting entry in /var/ipfire/ovpn/ovpnconfig will contain the CN and the emailAddress separated by a slash character as certificate name.
The script used for tls-verify, however, treats everything after the CN= string in the X509 identifier as certificate name. So if the client’s certificate happens to contain an email address the certificate name that the TLS verification script expects will never match the entries in ovpnconfig.

Example
The certificate with the identification C=XX, L=Xxxxxx, O=xxx, OU=XX, CN=ovpnClient, emailAddress=ovpnClient@example.com will generate this name in ovpnconfig when uploaded:

> ovpnClient/emailAddress=ovpnClient@example.com.

On the other hand, the TLS verification script expects the certificate name

> ovpnClient, emailAddress=ovpnClient@example.com

(there’s a comma and a space and not a slash) to be in ovpnconfig when the client tries to connect. The expected name will never match the entries in ovpnconfig which results in an error during the TLS handshake:

> WARNING: Failed running command (--tls-verify script): external program exited with error status: 1
> VERIFY SCRIPT ERROR: depth=0, C=XX, L=Xxxxxx, O=xxx, OU=XX, CN=ovpnClient, emailAddress=ovpnClient@example.com
> TLS_ERROR: BIO read tls_read_plaintext error: error:140890B2:SSL routines:SSL3_GET_CLIENT_CERTIFICATE:no certificate returned

To fix this issue, all commas in the expected certificate name need to be replaced by a slash character in order to get the certificate name in line with the entries in ovpnconfig.


[1] https://github.com/ipfire/ipfire-2.x/pull/25
Comment 1 Michael Tremer 2014-06-15 17:03:01 UTC
Created attachment 206 [details]
OpenVPN verify

I would like to suggest the attached patch, which should match exactly what is needed instead of correcting a wrong result afterwards. The one we used did not work if the certificate has an email address - I can confirm that.

If you could please check if this one works for you as well and get back to me, that would be terrific.
Comment 2 Stefan Ferstl 2014-06-15 18:55:47 UTC
Unfortunately, it does still not work. The verify script extracts the CN correctly when I'm using your patch. However, when I upload the client certificate the CN in "/var/ipfire/ovpn/ovpnconfig" will still contain the email address.
For example, if I have a certificate with "CN=myName, emailAddress=me@example.com", the verify script extracts "myName" and compares it with "myName/emailAddress=me@example.com" in the "ovpnconfig" file.
Comment 3 Stefan Ferstl 2014-06-21 16:15:10 UTC
Ok, I took a deeper look into the problem and found a solution that fixes it and stays compatible with previously uploaded client certificates that got a wrong CN in /var/ipfire/ovpn/ovpnconfig.

I opened a new Pull Request [1] on GitHub with my fixes and I tested the changes with Core Update 79. I did also close the original Pull Request because it is more a workaround as a solution to the problem (as Michael mentioned in the previous comment).

The basic problem is that the CNs of OpenVPN clients are handled incorrectly at different places when they contain further attributes after the CN. This leads to three issues in IPFire:

1) Upload of client certificates

When a client certificate with further attributes after the CN is uploaded, the resulting entry in /var/ipfire/ovpn/ovpnconfig contains the CN and all other attributes separated by a slash (/) character. The reason is that /srv/web/ipfire/cgi-bin/ovpnmain.cgi executes /usr/bin/openssl x509 -text ... and tries to grep the CN from the output of the command. However, the -text option causes attributes after the CN to be appended to the CN with a slash character as separator.

Example
The certificate with the subject C=XX, L=Xxxxxx, O=xxx, OU=XX, CN=ovpnClient, emailAddress=ovpnClient@example.com will generate this name in ovpnconfig when uploaded: ovpnClient/emailAddress=ovpnClient@example.com


2) tls-verify is not working

The script /usr/lib/openvpn/verify which is executed by OpenVPN during the TLS handshake parses the CN incorrecly as well. It will contain all attributes of the subject after the CN separated by a comma. Using the example from the last chapter, the parsed CN will be ovpnClient, emailAddress=ovpnClient@example.com. This CN is then compared with the entries in /var/ipfire/ovpn/ovpnconfig which will never match. As a consequence, the TLS handshake will fail with this error:

> WARNING: Failed running command (--tls-verify script): external program exited with error status: 1
> VERIFY SCRIPT ERROR: depth=0, C=XX, L=Xxxxxx, O=xxx, OU=XX, CN=ovpnClient, emailAddress=ovpnClient@example.com
> TLS_ERROR: BIO read tls_read_plaintext error: error:140890B2:SSL routines:SSL3_GET_CLIENT_CERTIFICATE:no certificate returned


3) To display the connection status of the OpenVPN clients the script /srv/web/ipfire/cgi-bin/ovpnmain.cgi reads the correct CNs of the connected clients from /var/log/ovpnserver.log and tries to compare them with the (possibly wrong) CNs in /var/ipfire/ovpn/ovpnconfig. As a consequence, the clients will always be displayed as DISCONNECTED even if they are connected.


The commits in the Pull Request fix these three problems as follows:

1) The /usr/lib/openvpn/verify script will parse the CN correctly from the subject. If it cannot be matched exactly with an entry in /var/ipfire/ovpn/ovpnconfig, it uses a regex to match it with an eventually incorrectly saved CN.

2) /srv/web/ipfire/cgi-bin/ovpnmain.cgi has been fixed to use the correct CN when uploading or creating a client certificate.

3) /srv/web/ipfire/cgi-bin/ovpnmain.cgi will use a regex to match the CNs of the connected clients to the possibly wrong CNs in /var/ipfire/ovpn/ovpnconfig.


[1] https://github.com/ipfire/ipfire-2.x/pull/33
Comment 4 Michael Tremer 2014-06-23 18:25:02 UTC
Hey Stefan,

thank you for taking the time looking into this, but there many new problems I found...

(In reply to comment #3)
> Ok, I took a deeper look into the problem and found a solution that fixes it
> and stays compatible with previously uploaded client certificates that got a
> wrong CN in /var/ipfire/ovpn/ovpnconfig.

We are fighting various inconsistencies with the output of openssl and openvpn. This really is a struggle and we must be very careful so that the verify script does not permit access to people that are not allowed to get access.

> I opened a new Pull Request [1] on GitHub with my fixes and I tested the
> changes with Core Update 79. I did also close the original Pull Request
> because it is more a workaround as a solution to the problem (as Michael
> mentioned in the previous comment).

Well, this hasn't really changed that much, yet.

> The basic problem is that the CNs of OpenVPN clients are handled incorrectly
> at different places when they contain further attributes after the CN. This
> leads to three issues in IPFire:
> 
> 1) Upload of client certificates
> 
> When a client certificate with further attributes after the CN is uploaded,
> the resulting entry in /var/ipfire/ovpn/ovpnconfig contains the CN and all
> other attributes separated by a slash (/) character. The reason is that
> /srv/web/ipfire/cgi-bin/ovpnmain.cgi executes /usr/bin/openssl x509 -text
> ... and tries to grep the CN from the output of the command. However, the
> -text option causes attributes after the CN to be appended to the CN with a
> slash character as separator.
> 
> Example
> The certificate with the subject C=XX, L=Xxxxxx, O=xxx, OU=XX,
> CN=ovpnClient, emailAddress=ovpnClient@example.com will generate this name
> in ovpnconfig when uploaded: ovpnClient/emailAddress=ovpnClient@example.com

The output depends on various things. Indeed the string that is saved in the configuration file is WRONG and we must fix this for future uploads and in the verify script for connections that have already been imported with this problem.

The latter part will be done in the verify script, the other part must be done in the CGI script.

Your proposed change alters the output of the openssl command which makes parsing the CN a bit easier, but the regular expression does severe overmatching because of the .* characters in the end and beginning and should be more string (i.e. require an end of line $ and only accept whitespace prior to CN=). We don't need to care for the slash or comma notation here at all.

> 2) tls-verify is not working
> 
> The script /usr/lib/openvpn/verify which is executed by OpenVPN during the
> TLS handshake parses the CN incorrecly as well. It will contain all
> attributes of the subject after the CN separated by a comma. Using the
> example from the last chapter, the parsed CN will be ovpnClient,
> emailAddress=ovpnClient@example.com. This CN is then compared with the
> entries in /var/ipfire/ovpn/ovpnconfig which will never match. As a
> consequence, the TLS handshake will fail with this error:
> 
> > WARNING: Failed running command (--tls-verify script): external program exited with error status: 1
> > VERIFY SCRIPT ERROR: depth=0, C=XX, L=Xxxxxx, O=xxx, OU=XX, CN=ovpnClient, emailAddress=ovpnClient@example.com
> > TLS_ERROR: BIO read tls_read_plaintext error: error:140890B2:SSL routines:SSL3_GET_CLIENT_CERTIFICATE:no certificate returned


We already discussed this before and I proposed a regular expression in comment #1. The one you proposed with the last patch does not work for me as it does not care for the slash notation at all.

For example C=XX/L=Xxxxxx/O=xxx/OU=XX/CN=ovpnClient/emailAddress=ovpnClient@example.com results in ovpnClient/emailAddress=ovpnClient@example.com which is still wrong and why we are working on this in the first place.

What is the reason for dropping the case-insensitivity?

I would also suggest to use "([^,\/ ]+)" for fixing the trailing data that is saved unintentionally.

> 3) To display the connection status of the OpenVPN clients the script
> /srv/web/ipfire/cgi-bin/ovpnmain.cgi reads the correct CNs of the connected
> clients from /var/log/ovpnserver.log and tries to compare them with the
> (possibly wrong) CNs in /var/ipfire/ovpn/ovpnconfig. As a consequence, the
> clients will always be displayed as DISCONNECTED even if they are connected.

Same as above.

> [1] https://github.com/ipfire/ipfire-2.x/pull/33
Comment 5 Stefan Ferstl 2014-06-24 00:43:00 UTC
(In reply to comment #4)
> Hey Stefan,
> 
> thank you for taking the time looking into this, but there many new problems
> I found...
> 
> (In reply to comment #3)
> > Ok, I took a deeper look into the problem and found a solution that fixes it
> > and stays compatible with previously uploaded client certificates that got a
> > wrong CN in /var/ipfire/ovpn/ovpnconfig.
> 
> We are fighting various inconsistencies with the output of openssl and
> openvpn. This really is a struggle and we must be very careful so that the
> verify script does not permit access to people that are not allowed to get
> access.
> 
> > I opened a new Pull Request [1] on GitHub with my fixes and I tested the
> > changes with Core Update 79. I did also close the original Pull Request
> > because it is more a workaround as a solution to the problem (as Michael
> > mentioned in the previous comment).
> 
> Well, this hasn't really changed that much, yet.
> 
> > The basic problem is that the CNs of OpenVPN clients are handled incorrectly
> > at different places when they contain further attributes after the CN. This
> > leads to three issues in IPFire:
> > 
> > 1) Upload of client certificates
> > 
> > When a client certificate with further attributes after the CN is uploaded,
> > the resulting entry in /var/ipfire/ovpn/ovpnconfig contains the CN and all
> > other attributes separated by a slash (/) character. The reason is that
> > /srv/web/ipfire/cgi-bin/ovpnmain.cgi executes /usr/bin/openssl x509 -text
> > ... and tries to grep the CN from the output of the command. However, the
> > -text option causes attributes after the CN to be appended to the CN with a
> > slash character as separator.
> > 
> > Example
> > The certificate with the subject C=XX, L=Xxxxxx, O=xxx, OU=XX,
> > CN=ovpnClient, emailAddress=ovpnClient@example.com will generate this name
> > in ovpnconfig when uploaded: ovpnClient/emailAddress=ovpnClient@example.com
> 
> The output depends on various things. Indeed the string that is saved in the
> configuration file is WRONG and we must fix this for future uploads and in
> the verify script for connections that have already been imported with this
> problem.
> 
> The latter part will be done in the verify script, the other part must be
> done in the CGI script.
> 
> Your proposed change alters the output of the openssl command which makes
> parsing the CN a bit easier, but the regular expression does severe
> overmatching because of the .* characters in the end and beginning and
> should be more string (i.e. require an end of line $ and only accept
> whitespace prior to CN=). We don't need to care for the slash or comma
> notation here at all.
> 

True. The OpenSSL documentation (https://www.openssl.org/docs/apps/x509.html#NAME_OPTIONS) is quite clear about the multiline output. I changed the regex accordingly.


> > 2) tls-verify is not working
> > 
> > The script /usr/lib/openvpn/verify which is executed by OpenVPN during the
> > TLS handshake parses the CN incorrecly as well. It will contain all
> > attributes of the subject after the CN separated by a comma. Using the
> > example from the last chapter, the parsed CN will be ovpnClient,
> > emailAddress=ovpnClient@example.com. This CN is then compared with the
> > entries in /var/ipfire/ovpn/ovpnconfig which will never match. As a
> > consequence, the TLS handshake will fail with this error:
> > 
> > > WARNING: Failed running command (--tls-verify script): external program exited with error status: 1
> > > VERIFY SCRIPT ERROR: depth=0, C=XX, L=Xxxxxx, O=xxx, OU=XX, CN=ovpnClient, emailAddress=ovpnClient@example.com
> > > TLS_ERROR: BIO read tls_read_plaintext error: error:140890B2:SSL routines:SSL3_GET_CLIENT_CERTIFICATE:no certificate returned
> 
> 
> We already discussed this before and I proposed a regular expression in
> comment #1. The one you proposed with the last patch does not work for me as
> it does not care for the slash notation at all.
> 
> For example
> C=XX/L=Xxxxxx/O=xxx/OU=XX/CN=ovpnClient/emailAddress=ovpnClient@example.com
> results in ovpnClient/emailAddress=ovpnClient@example.com which is still
> wrong and why we are working on this in the first place.
> 

Sorry, I forgot about your comment when I was making the changes. However, I don't understand your example. Where does a subject formatted like "C=XX/L=Xxxxxx/O=xxx/OU=XX/CN=ovpnClient/emailAddress=ovpnClient@example.com" come from? As far as I observed, OpenVPN separates the subject attributes with commas.
Furthermore, I think the regex that you proposed does not work when the certificate's subject does only contain the CN (i.e. none of the C, L, O and OU attributes are present).

> What is the reason for dropping the case-insensitivity?
> 

Why does the subject have to be matched case-insensitively?

> I would also suggest to use "([^,\/ ]+)" for fixing the trailing data that
> is saved unintentionally.
> 

Right. I do now use an expression that ignores everything that comes after the real CN (the one from OpenVPN's subject string) and starts with a slash. I don't think that an expression like "[^,\/ ]+" is necessary because everything after the first slash does not matter.

> > 3) To display the connection status of the OpenVPN clients the script
> > /srv/web/ipfire/cgi-bin/ovpnmain.cgi reads the correct CNs of the connected
> > clients from /var/log/ovpnserver.log and tries to compare them with the
> > (possibly wrong) CNs in /var/ipfire/ovpn/ovpnconfig. As a consequence, the
> > clients will always be displayed as DISCONNECTED even if they are connected.
> 
> Same as above.
> 

I changed it similarly as explained above.

You'll find all changes for this issue here: https://github.com/ipfire/ipfire-2.x/pull/33/files


Cheers,
Stefan
Comment 6 Michael Tremer 2014-06-25 11:22:59 UTC
(In reply to comment #5)
> True. The OpenSSL documentation
> (https://www.openssl.org/docs/apps/x509.html#NAME_OPTIONS) is quite clear
> about the multiline output. I changed the regex accordingly.

Yes this is much better. I would recommend matching the beginning of the line with the ^ character, because this would currently also match lines that look like: "   XXX=abc CN=abc"
I am not sure if you could enter a substring like " CN=abc" as an email address or something, but if it is possible, we should not use that here.

> > > 2) tls-verify is not working
> 
> Sorry, I forgot about your comment when I was making the changes. However, I
> don't understand your example. Where does a subject formatted like
> "C=XX/L=Xxxxxx/O=xxx/OU=XX/CN=ovpnClient/emailAddress=ovpnClient@example.
> com" come from? As far as I observed, OpenVPN separates the subject
> attributes with commas.

All connections created prior to IPFire Core Update 75 (or openvpn 2.3) use the slash syntax. The openvpn guys changed behaviour somewhat silently and we are apparently still fighting the problems caused by that.

> Furthermore, I think the regex that you proposed does not work when the
> certificate's subject does only contain the CN (i.e. none of the C, L, O and
> OU attributes are present).

You are right. We could simply change that with making the first group optional.

> > What is the reason for dropping the case-insensitivity?
> > 
> 
> Why does the subject have to be matched case-insensitively?

I thought you knew.

> > I would also suggest to use "([^,\/ ]+)" for fixing the trailing data that
> > is saved unintentionally.
> > 
> 
> Right. I do now use an expression that ignores everything that comes after
> the real CN (the one from OpenVPN's subject string) and starts with a slash.
> I don't think that an expression like "[^,\/ ]+" is necessary because
> everything after the first slash does not matter.

I don't think that the expression does what you want it to do. At least not with fake input. The argument that is supposed to find the end of the CN string is optional and therefore if not found this would probably match more than you want it to. You are also considering the slash notation here which you found obsolete earlier.
Comment 7 Stefan Ferstl 2014-06-26 23:16:47 UTC
(In reply to comment #6)
> (In reply to comment #5)
> > True. The OpenSSL documentation
> > (https://www.openssl.org/docs/apps/x509.html#NAME_OPTIONS) is quite clear
> > about the multiline output. I changed the regex accordingly.
> 
> Yes this is much better. I would recommend matching the beginning of the
> line with the ^ character, because this would currently also match lines
> that look like: "   XXX=abc CN=abc"

Fixed. Acutally, it has been fixed since my last comment on this issue: https://github.com/ferstl/ipfire-2.x/commit/80b1dcf192e4832e8472037b64c23b9f570a5639

> I am not sure if you could enter a substring like " CN=abc" as an email
> address or something, but if it is possible, we should not use that here.
> 

You can. And the output would be something like "emailAddress=CN=myName@example.com". But this would be on a different line and does not match the expression.

> > > > 2) tls-verify is not working
> > 
> > Sorry, I forgot about your comment when I was making the changes. However, I
> > don't understand your example. Where does a subject formatted like
> > "C=XX/L=Xxxxxx/O=xxx/OU=XX/CN=ovpnClient/emailAddress=ovpnClient@example.
> > com" come from? As far as I observed, OpenVPN separates the subject
> > attributes with commas.
> 
> All connections created prior to IPFire Core Update 75 (or openvpn 2.3) use
> the slash syntax. The openvpn guys changed behaviour somewhat silently and
> we are apparently still fighting the problems caused by that.
> 

Ok, I didn't know that. I changed the verify script to use your proposed expression but made the first group optional (as you suggested below).

> > Furthermore, I think the regex that you proposed does not work when the
> > certificate's subject does only contain the CN (i.e. none of the C, L, O and
> > OU attributes are present).
> 
> You are right. We could simply change that with making the first group
> optional.
> 

See above...

> > > What is the reason for dropping the case-insensitivity?
> > > 
> > 
> > Why does the subject have to be matched case-insensitively?
> 
> I thought you knew.
> 

No, I didn't. And as long as nobody tells me, I won't know.


> > > I would also suggest to use "([^,\/ ]+)" for fixing the trailing data that
> > > is saved unintentionally.
> > > 
> > 
> > Right. I do now use an expression that ignores everything that comes after
> > the real CN (the one from OpenVPN's subject string) and starts with a slash.
> > I don't think that an expression like "[^,\/ ]+" is necessary because
> > everything after the first slash does not matter.
> 
> I don't think that the expression does what you want it to do. At least not
> with fake input. The argument that is supposed to find the end of the CN
> string is optional and therefore if not found this would probably match more
> than you want it to. You are also considering the slash notation here which
> you found obsolete earlier.

Maybe. But I found another flaw in this solution. Someone could construct a certificate containing a regular expression as CN and probably get past the verify script. Such a certificate does still have to be signed by the CA that the OpenVPN server trusts. But this might be a problem if the server is trusting a commercial certificate authority. In this case a malicious person could easily buy a certificate with a crafted CN from the same CA and try to trick the verify script. 

Provided, that the verify script should be able to handle incorrectly saved entries in the ovpnconfig file, we have to face the following problem: 
An entry in the ovpnconfig file could either contain the correct CN or the CN followed by slash-separated further attributes, e.g. "ovpnClient/emailAddress=ovpnClient@example.com". As I explained in comment #3, such incorrect CNs are created by executing "/usr/bin/openssl x509 -text -in <certfile>" during the certificate upload.
I assume that the verify script extracts the client's CN correctly into the $CN variable. This CN does now need to be matched with the entries of the ovpnconfig file. The only thing that I know about these is that everything before the first slash character has to be the CN and it has to match the client's CN exactly. The slash character (which could be there or not) and everything else after it does not matter. So I think this expression should do the trick: "^([^\/]+)(\/.*)?$".
I use the same expression to detect the connected clients.

Please have another look at the adjustments: https://github.com/ipfire/ipfire-2.x/pull/33/files


Cheers,
Stefan
Comment 8 Michael Tremer 2014-07-05 22:19:44 UTC
(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > True. The OpenSSL documentation
> > > (https://www.openssl.org/docs/apps/x509.html#NAME_OPTIONS) is quite clear
> > > about the multiline output. I changed the regex accordingly.
> > 
> > Yes this is much better. I would recommend matching the beginning of the
> > line with the ^ character, because this would currently also match lines
> > that look like: "   XXX=abc CN=abc"
> 
> Fixed. Acutally, it has been fixed since my last comment on this issue:
> https://github.com/ferstl/ipfire-2.x/commit/
> 80b1dcf192e4832e8472037b64c23b9f570a5639

It is a bit hard to keep track because the changes are splitted over multiple commits and constantly change. Please bare with me.

> > I am not sure if you could enter a substring like " CN=abc" as an email
> > address or something, but if it is possible, we should not use that here.
> > 
> 
> You can. And the output would be something like
> "emailAddress=CN=myName@example.com". But this would be on a different line
> and does not match the expression.

It is indeed in the multiline output, but we don't have it at all times...

> > > > What is the reason for dropping the case-insensitivity?
> > > > 
> > > 
> > > Why does the subject have to be matched case-insensitively?
> > 
> > I thought you knew.
> > 
> 
> No, I didn't. And as long as nobody tells me, I won't know.

I didn't know it by heart either. Therefore I looked it up and as far as I understand RFC5280, this must be a case-insensitive comparison. Hence the i must stay.

> 
> 
> > > > I would also suggest to use "([^,\/ ]+)" for fixing the trailing data that
> > > > is saved unintentionally.
> > > > 
> > > 
> > > Right. I do now use an expression that ignores everything that comes after
> > > the real CN (the one from OpenVPN's subject string) and starts with a slash.
> > > I don't think that an expression like "[^,\/ ]+" is necessary because
> > > everything after the first slash does not matter.
> > 
> > I don't think that the expression does what you want it to do. At least not
> > with fake input. The argument that is supposed to find the end of the CN
> > string is optional and therefore if not found this would probably match more
> > than you want it to. You are also considering the slash notation here which
> > you found obsolete earlier.
> 
> Maybe. But I found another flaw in this solution. Someone could construct a
> certificate containing a regular expression as CN and probably get past the
> verify script. Such a certificate does still have to be signed by the CA
> that the OpenVPN server trusts. But this might be a problem if the server is
> trusting a commercial certificate authority. In this case a malicious person
> could easily buy a certificate with a crafted CN from the same CA and try to
> trick the verify script.

The certificate is certainly verified against the CA. Either if that is a self-signed CA or some public one and certificates for the individual connections are imported (I don't think that anybody would do this simply because it is too expensive), but it *always* is a problem that some certificates may be issues multiple times. So if CA-1 issues a certificate for google.com and CA-2 does the same thing there are two certificates around that are trusted by likely all web browsers and the fraud may remain unnoticed for a long time. In case this happens for the OpenVPN CA there is nothing we can do about it. You *have* to trust the CA - for obvious reasons, nobody would do that with a public CA.

> Provided, that the verify script should be able to handle incorrectly saved
> entries in the ovpnconfig file, we have to face the following problem: 
> An entry in the ovpnconfig file could either contain the correct CN or the
> CN followed by slash-separated further attributes, e.g.
> "ovpnClient/emailAddress=ovpnClient@example.com". As I explained in comment
> #3, such incorrect CNs are created by executing "/usr/bin/openssl x509 -text
> -in <certfile>" during the certificate upload.
> I assume that the verify script extracts the client's CN correctly into the
> $CN variable. This CN does now need to be matched with the entries of the
> ovpnconfig file. The only thing that I know about these is that everything
> before the first slash character has to be the CN and it has to match the
> client's CN exactly. The slash character (which could be there or not) and
> everything else after it does not matter. So I think this expression should
> do the trick: "^([^\/]+)(\/.*)?$".
> I use the same expression to detect the connected clients.

This assumes that there will never be a forward slash in the common name. Is it correct to do that?

> Please have another look at the adjustments:
> https://github.com/ipfire/ipfire-2.x/pull/33/files

This looks good. I guess we are getting to a good and safe solution here that fixes the problem and is backwards-compatible to all other bugs that have there been so far.

P.S. Please take care not to introduce any whitespace errors.
Comment 9 Stefan Ferstl 2014-07-15 22:23:29 UTC
(In reply to comment #8)
> ...
> > 
> > Fixed. Acutally, it has been fixed since my last comment on this issue:
> > https://github.com/ferstl/ipfire-2.x/commit/
> > 80b1dcf192e4832e8472037b64c23b9f570a5639
> 
> It is a bit hard to keep track because the changes are splitted over
> multiple commits and constantly change. Please bare with me.
> 

If you prefer, I'll squash all my intermediate commits into one single commit. I think, the intermediate commits are not of any value in this case.


> ...
> I didn't know it by heart either. Therefore I looked it up and as far as I
> understand RFC5280, this must be a case-insensitive comparison. Hence the i
> must stay.
> 

I agree.

> ...
> 
> > Provided, that the verify script should be able to handle incorrectly saved
> > entries in the ovpnconfig file, we have to face the following problem: 
> > An entry in the ovpnconfig file could either contain the correct CN or the
> > CN followed by slash-separated further attributes, e.g.
> > "ovpnClient/emailAddress=ovpnClient@example.com". As I explained in comment
> > #3, such incorrect CNs are created by executing "/usr/bin/openssl x509 -text
> > -in <certfile>" during the certificate upload.
> > I assume that the verify script extracts the client's CN correctly into the
> > $CN variable. This CN does now need to be matched with the entries of the
> > ovpnconfig file. The only thing that I know about these is that everything
> > before the first slash character has to be the CN and it has to match the
> > client's CN exactly. The slash character (which could be there or not) and
> > everything else after it does not matter. So I think this expression should
> > do the trick: "^([^\/]+)(\/.*)?$".
> > I use the same expression to detect the connected clients.
> 
> This assumes that there will never be a forward slash in the common name. Is
> it correct to do that?
> 

You are right. Slashes in the CN are possible. I really don't know how I could miss that...

Now we still have two problems in the verify script: Neither the expression that greps the CN out of the client's subject string (the one in your attachment) nor the expression to compare the CN with the one in ovpnconfig do work correctly when the CN contains one or more slashes.

I suggest to change the expression to grep the client's CN to this one:
   $CN =~ /(,\ )?CN=([^,]+)?/i;
   $CN = $2;
I don't think that we need to be backwards compatible here and support the slash notation because the subject string is always provided by the currently installed OpenVPN version (2.3.4 as of core update 79). So we know exactly how this implementation formats the subject string, right?

The comparison of the client's CN with the (possibly incorrect) entry in ovpnconfig is a bit trickier. We could first check whether the entry in ovpnconfig does start with the CN from the client's subject string. If this is the case, we do a second check whether the entry in ovpnconfig contains a slash followed by a further attribute after the CN. Only if the second check is also true, the verification is successful.

What do you think about that? I'll make the changes if the above suggestion is OK for you.
Comment 10 Michael Tremer 2015-11-01 01:19:05 UTC
I believe that this (as it hasn't been updated in a while) is fixed now. If not, please reopen.