Fix the --client-cert-not-required feature

Commit 2e8337de248ef0b5b48cbb2964da0d5c3f28b15b introduced a new feature for using other SSL certificate fields for authentication than then CN field.

This commit introduced a bug, which made the verify_callback() function getting called even if --client-cert-not-required was enabled in the config.

The reason for this was that an 'else' statement was lacking a couple of curly braces. The offending commit in reality moved the setup the verify_callback() function out of the 'else' statement.

Report-URL: https://community.openvpn.net/openvpn/ticket/108
Report-URL: https://forums.openvpn.net/topic7751.html

Signed-off-by David Sommerseth <[email protected]...>

---
 ssl.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

ssl.c +2 -2

index ed10714..6d9a9fd 100644
@@ -1874,13 +1874,15 @@ init_ssl (const struct options *options)
     }
   else
 #endif
+    {
 #ifdef ENABLE_X509ALTUSERNAME
-  x509_username_field = (char *) options->x509_username_field;
+      x509_username_field = (char *) options->x509_username_field;
 #else
-  x509_username_field = X509_USERNAME_FIELD_DEFAULT;

Hi David,

Quoted 35 lines · David Sommerseth

Commit 2e8337de248ef0b5b48cbb2964da0d5c3f28b15b introduced a new feature for using other SSL certificate fields for authentication than then CN field.

This commit introduced a bug, which made the verify_callback() function getting called even if --client-cert-not-required was enabled in the config.

The reason for this was that an 'else' statement was lacking a couple of curly braces. The offending commit in reality moved the setup the verify_callback() function out of the 'else' statement.

Report-URL: https://community.openvpn.net/openvpn/ticket/108
Report-URL: https://forums.openvpn.net/topic7751.html

Signed-off-by David Sommerseth <[email protected]...>

---
 ssl.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

ssl.c +2 -2

index ed10714..6d9a9fd 100644
@@ -1874,13 +1874,15 @@ init_ssl (const struct options *options)
     }
   else
 #endif
+    {
 #ifdef ENABLE_X509ALTUSERNAME
-  x509_username_field = (char *) options->x509_username_field;
+      x509_username_field = (char *) options->x509_username_field;
 #else
-  x509_username_field = X509_USERNAME_FIELD_DEFAULT;

ssl.c init_ssl (const struct options *options) · line 1888

Quoted 10 lines

ssl.c +4 -2

+      x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
-  SSL_CTX_set_verify (ctx, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT,
-			verify_callback);
+      SSL_CTX_set_verify (ctx, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT,
+                          verify_callback);
+    }
 
   /* Connection information callback */
   SSL_CTX_set_info_callback (ctx, info_callback);

weird, but with your patch I get a completely different error:

Thu Mar 31 09:55:01 2011 us=368010 194.171.96.28:44859 Could not create temporary file 'openvpn_acf_ab60cdb4d9d6bdf5d3c7812ea9710705.tmp':
Permission denied
Thu Mar 31 09:55:01 2011 us=368145 194.171.96.28:44859 Exiting

if I run it with '--tmp-dir /tmp' it works again - looks like this patch triggers another bug?

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Quoted 52 lines · Jan Just Keijser

Hi David,

Quoted 36 lines · David Sommerseth

Commit 2e8337de248ef0b5b48cbb2964da0d5c3f28b15b introduced a new feature for using other SSL certificate fields for authentication than then CN field.

This commit introduced a bug, which made the verify_callback() function getting called even if --client-cert-not-required was enabled in the config.

The reason for this was that an 'else' statement was lacking a couple of curly braces. The offending commit in reality moved the setup the verify_callback() function out of the 'else' statement.

Report-URL: https://community.openvpn.net/openvpn/ticket/108
Report-URL: https://forums.openvpn.net/topic7751.html

Signed-off-by David Sommerseth <[email protected]...>

---
 ssl.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

ssl.c +2 -2

index ed10714..6d9a9fd 100644
@@ -1874,13 +1874,15 @@ init_ssl (const struct options *options)
     }
   else
 #endif
+    {
 #ifdef ENABLE_X509ALTUSERNAME
-  x509_username_field = (char *) options->x509_username_field;
+      x509_username_field = (char *) options->x509_username_field;
 #else
-  x509_username_field = X509_USERNAME_FIELD_DEFAULT;

ssl.c init_ssl (const struct options *options) · line 1888

Quoted 10 lines

ssl.c +4 -2

+      x509_username_field = X509_USERNAME_FIELD_DEFAULT;
 #endif
-  SSL_CTX_set_verify (ctx, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT,
-			verify_callback);
+      SSL_CTX_set_verify (ctx, SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT,
+                          verify_callback);
+    }
 
   /* Connection information callback */
   SSL_CTX_set_info_callback (ctx, info_callback);

weird, but with your patch I get a completely different error:

Quoted 9 lines

Thu Mar 31 09:55:01 2011 us=368010 194.171.96.28:44859 Could not create temporary file 'openvpn_acf_ab60cdb4d9d6bdf5d3c7812ea9710705.tmp':
Permission denied
Thu Mar 31 09:55:01 2011 us=368145 194.171.96.28:44859 Exiting

if I run it with '--tmp-dir /tmp' it works again - looks like this patch triggers another bug?

Actually, it's not another bug, it's an expected behavioural change. In OpenVPN 2.1 and earlier there is a function which is called quite often when plug-in's or the script hooks are used, which generates a unique filename and passes this filesname to the plug-in or script. This function was called create_temp_filename(). Plug-ins or script could then create this file if it wanted to pass some information back to OpenVPN.

This might be a security issue, where it is, theoretically at least, possible to intercept OpenVPN and put configuration statements which would then be processed by the OpenVPN server.

To solve this the create_temp_filename() function has been replaced by create_temp_file(), which creates a temp file which is read/writeable only by the OpenVPN user before the plug-in/script is called. The plug-in/script only needs to open this file and write to it to pass information back. The OpenVPN server evaluates the file and deletes it when the plug-in/script is done.

The reality is that when you use --plugin or some of the script hooks, you now need to have a writeable directory anyway. Earlier this was only needed if the plug-in/script did create the file. So it's not really a bug, but it reveals this behaviour more easily now.

For those concerned about performance, it is possible to put this --tmp-dir on a tmpfs or RAM disk mounted directory. It's also been discussed briefly if we should default to /tmp, %TMPDIR%, or similar standard paths for
- --tmp-dir. I think we need to do that.

The man page has been updated to cover this change. But it could probably be mentioned as well in the --client-connect and --plugin sections too.

For OpenVPN 2.3, a new plug-in API is comming. I'm considering to extend that so that the plug-in can create this file and provide the filename back to OpenVPN. This sounds easy, but I have not looked at possible security considerations here yet.

kind regards,

David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2UO1wACgkQDC186MBRfrqGOACgmtxxYU2riZxQPJS/GXx9Ah6O f5UAnjqD1uhwUwFqMOnDv1jGPM43ZMnx
=bEod
-----END PGP SIGNATURE-----

Commit 2e8337de248ef0b5b48cbb2964da0d5c3f28b15b introduced a new feature for using other SSL certificate fields for authentication than then CN field.

This commit introduced a bug, which made the verify_callback() function getting called even if --client-cert-not-required was enabled in the config.

The reason for this was that an 'else' statement was lacking a couple of curly braces. The offending commit in reality moved the setup the verify_callback() function out of the 'else' statement.