TLS versioning

This is the TLS versioning patch as discussed in last Thursday's IRC meeting.

It combines these two patches:

https://github.com/jamesyonan/openvpn/commit/03a5599202bdc3ba07983dc4efdae387fb8fb436

https://github.com/jamesyonan/openvpn/commit/d23005413b0e0f28a3c48a6342f494763d5c9b40

James

index 174fd6d..d3fb468 100644
@@ -4235,6 +4235,15 @@ when you built your peer's certificate (see
 above).
 .\"*********************************************************
 .TP
+.B \-\-tls-version-min version ['or-highest']
+Sets the minimum
+TLS version we will accept from the peer (default is "1.0").
+Examples for version
+include "1.0", "1.1", or "1.2".  If 'or-highest' is specified
+and version is not recognized, we will only accept the highest TLS
+version supported by the local SSL implementation.
+.\"*********************************************************
+.TP
 .B \-\-pkcs12 file
 Specify a PKCS #12 file containing local private key,
 local certificate, and root CA certificate.

index 883136f..ba5bb1f 100644
@@ -582,6 +582,9 @@ static const char usage_message[] =
   "                  by a Certificate Authority in --ca file.\n"
   "--extra-certs file : one or more PEM certs that complete the cert chain.\n"
   "--key file      : Local private key in .pem format.\n"
+  "--tls-version-min <version> ['or-highest'] : sets the minimum TLS version we\n"
+  "    will accept from the peer.  If version is unrecognized and 'or-highest'\n"
+  "    is specified, require max TLS version supported by SSL implementation.\n"
 #ifndef ENABLE_CRYPTO_POLARSSL
   "--pkcs12 file   : PKCS#12 file containing local private key, local certificate\n"
   "                  and optionally the root CA certificate.\n"

@@ -6506,6 +6509,19 @@ add_option (struct options *options,
 	  options->priv_key_file_inline = p[2];
 	}
     }
+  else if (streq (p[0], "tls-version-min") && p[1])
+    {
+      int ver;
+      VERIFY_PERMISSION (OPT_P_GENERAL);
+      ver = tls_version_min_parse(p[1], p[2]);
+      if (ver == TLS_VER_BAD)
+	{
+	  msg (msglevel, "unknown tls-version-min parameter: %s", p[1]);
+          goto err;
+	}
+      options->ssl_flags &= ~(SSLF_TLS_VERSION_MASK << SSLF_TLS_VERSION_SHIFT);
+      options->ssl_flags |= (ver << SSLF_TLS_VERSION_SHIFT);
+    }
 #ifndef ENABLE_CRYPTO_POLARSSL
   else if (streq (p[0], "pkcs12") && p[1])
     {

index 736dfc9..a8b5d3d 100644
@@ -447,6 +447,27 @@ ssl_put_auth_challenge (const char *cr_str)
 #endif
 
 /*
+ * Parse a TLS version string, returning a TLS_VER_x constant.
+ * If version string is not recognized and extra == "or-highest",
+ * return tls_version_max().
+ */
+int
+tls_version_min_parse(const char *vstr, const char *extra)
+{
+  const int max_version = tls_version_max();
+  if (!strcmp(vstr, "1.0") && TLS_VER_1_0 <= max_version)
+    return TLS_VER_1_0;
+  else if (!strcmp(vstr, "1.1") && TLS_VER_1_1 <= max_version)
+    return TLS_VER_1_1;
+  else if (!strcmp(vstr, "1.2") && TLS_VER_1_2 <= max_version)
+    return TLS_VER_1_2;
+  else if (extra && !strcmp(extra, "or-highest"))
+    return max_version;
+  else
+    return TLS_VER_BAD;
+}
+
+/*
  * Initialize SSL context.
  * All files are in PEM format.
  */

index 72235ae..4516897 100644
@@ -94,6 +94,29 @@ void tls_free_lib();
 void tls_clear_error();
 
 /**
+ * Parse a TLS version specifier
+ *
+ * @param vstr		The TLS version string
+ * @param extra	        An optional extra parameter, may be NULL
+ *
+ * @return 		One of the TLS_VER_x constants or TLS_VER_BAD
+ *                      if a parse error should be flagged.
+ */
+#define TLS_VER_BAD   -1
+#define TLS_VER_1_0    0 /* default */
+#define TLS_VER_1_1    1
+#define TLS_VER_1_2    2
+int tls_version_min_parse(const char *vstr, const char *extra);
+
+/**
+ * Return the maximum TLS version (as a TLS_VER_x constant)
+ * supported by current SSL implementation
+ *
+ * @return 		One of the TLS_VER_x constants (but not TLS_VER_BAD).
+ */
+int tls_version_max(void);
+
+/**
  * Initialise a library-specific TLS context for a server.
  *
  * @param ctx		TLS context to initialise

index 7e52f9a..04ba789 100644
@@ -290,12 +290,14 @@ struct tls_options
   struct compress_options comp_options;
 #endif
 
-  /* configuration file boolean options */
+  /* configuration file SSL-related boolean and low-permutation options */
 # define SSLF_CLIENT_CERT_NOT_REQUIRED (1<<0)
 # define SSLF_USERNAME_AS_COMMON_NAME  (1<<1)
 # define SSLF_AUTH_USER_PASS_OPTIONAL  (1<<2)
 # define SSLF_OPT_VERIFY               (1<<4)
 # define SSLF_CRL_VERIFY_DIR           (1<<5)
+# define SSLF_TLS_VERSION_SHIFT        6
+# define SSLF_TLS_VERSION_MASK         0xF /* (uses bit positions 6 to 9) */
   unsigned int ssl_flags;
 
 #ifdef MANAGEMENT_DEF_AUTH

index 5db717d..12c725d 100644
@@ -114,7 +114,7 @@ tls_ctx_server_new(struct tls_root_ctx *ctx)
 {
   ASSERT(NULL != ctx);
 
-  ctx->ctx = SSL_CTX_new (TLSv1_server_method ());
+  ctx->ctx = SSL_CTX_new (SSLv23_server_method ());
 
   if (ctx->ctx == NULL)
     msg (M_SSLERR, "SSL_CTX_new TLSv1_server_method");

@@ -127,7 +127,7 @@ tls_ctx_client_new(struct tls_root_ctx *ctx)
 {
   ASSERT(NULL != ctx);
 
-  ctx->ctx = SSL_CTX_new (TLSv1_client_method ());
+  ctx->ctx = SSL_CTX_new (SSLv23_client_method ());
 
   if (ctx->ctx == NULL)
     msg (M_SSLERR, "SSL_CTX_new TLSv1_client_method");

@@ -174,13 +174,46 @@ info_callback (INFO_CALLBACK_SSL_CONST SSL * s, int where, int ret)
     }
 }
 
+/*
+ * Return maximum TLS version supported by local OpenSSL library.
+ * Assume that presence of SSL_OP_NO_TLSvX macro indicates that
+ * TLSvX is supported.
+ */
+int
+tls_version_max(void)
+{
+#if defined(SSL_OP_NO_TLSv1_2)
+  return TLS_VER_1_2;
+#elif defined(SSL_OP_NO_TLSv1_1)
+  return TLS_VER_1_1;
+#else
+  return TLS_VER_1_0;
+#endif
+}
+
 void
 tls_ctx_set_options (struct tls_root_ctx *ctx, unsigned int ssl_flags)
 {
   ASSERT(NULL != ctx);
 
+  /* process SSL options including minimum TLS version we will accept from peer */
+  {
+    long sslopt = SSL_OP_SINGLE_DH_USE | SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3;
+    const int tls_version_min = (ssl_flags >> SSLF_TLS_VERSION_SHIFT) & SSLF_TLS_VERSION_MASK;
+    if (tls_version_min > TLS_VER_1_0)
+      sslopt |= SSL_OP_NO_TLSv1;
+#ifdef SSL_OP_NO_TLSv1_1
+    if (tls_version_min > TLS_VER_1_1)
+      sslopt |= SSL_OP_NO_TLSv1_1;
+#endif
+#ifdef SSL_OP_NO_TLSv1_2
+    if (tls_version_min > TLS_VER_1_2)
+      sslopt |= SSL_OP_NO_TLSv1_2;
+#endif
+    SSL_CTX_set_options (ctx->ctx, sslopt);
+  }
+
   SSL_CTX_set_session_cache_mode (ctx->ctx, SSL_SESS_CACHE_OFF);
-  SSL_CTX_set_options (ctx->ctx, SSL_OP_SINGLE_DH_USE);
   SSL_CTX_set_default_passwd_cb (ctx->ctx, pem_password_callback);
 
   /* Require peer certificate verification */

index 8a917b3..fb73225 100644
@@ -501,6 +501,18 @@ void tls_ctx_personalise_random(struct tls_root_ctx *ctx)
     }
 }
 
+int
+tls_version_max(void)
+{
+#if defined(SSL_MAJOR_VERSION_3) && defined(SSL_MINOR_VERSION_3)
+  return TLS_VER_1_2;
+#elif defined(SSL_MAJOR_VERSION_3) && defined(SSL_MINOR_VERSION_2)
+  return TLS_VER_1_1;
+#else
+  return TLS_VER_1_0;
+#endif
+}
+
 void key_state_ssl_init(struct key_state_ssl *ks_ssl,
     const struct tls_root_ctx *ssl_ctx, bool is_server, struct tls_session *session)
 {

@@ -550,6 +562,34 @@ void key_state_ssl_init(struct key_state_ssl *ks_ssl,
       /* TODO: PolarSSL does not currently support sending the CA chain to the client */
       ssl_set_ca_chain (ks_ssl->ctx, ssl_ctx->ca_chain, NULL, NULL );
 
+      /* Initialize minimum TLS version */
+      {
+	const int tls_version_min = (session->opt->ssl_flags >> SSLF_TLS_VERSION_SHIFT) & SSLF_TLS_VERSION_MASK;
+	int polar_major;
+	int polar_minor;
+	switch (tls_version_min)
+	  {
+	  case TLS_VER_1_0:
+	  default:
+	    polar_major = SSL_MAJOR_VERSION_3;
+	    polar_minor = SSL_MINOR_VERSION_1;
+	    break;
+#if defined(SSL_MAJOR_VERSION_3) && defined(SSL_MINOR_VERSION_2)
+	  case TLS_VER_1_1:
+	    polar_major = SSL_MAJOR_VERSION_3;
+	    polar_minor = SSL_MINOR_VERSION_2;
+	    break;
+#endif
+#if defined(SSL_MAJOR_VERSION_3) && defined(SSL_MINOR_VERSION_3)
+	  case TLS_VER_1_2:
+	    polar_major = SSL_MAJOR_VERSION_3;
+	    polar_minor = SSL_MINOR_VERSION_3;
+	    break;
+#endif
+	  }
+	ssl_set_min_version(ks_ssl->ctx, polar_major, polar_minor);
+      }
+
       /* Initialise BIOs */
       ALLOC_OBJ_CLEAR (ks_ssl->ct_in, endless_buffer);
       ALLOC_OBJ_CLEAR (ks_ssl->ct_out, endless_buffer);

From James Yonan <[email protected]...>:

For completeness: looks good to me, I have no comments that have not already been seen on this list.

Joachim

From: David Sommerseth <[email protected]...>

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

Your patch has been applied to the master branch.

commit 35e5d8c5f05a1821a5ca4384462427b00b871f82

Author: James Yonan
Date:   Mon Jun 10 22:59:30 2013 -0600

     TLS version negotiation

     Signed-off-by: James Yonan <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Acked-by: Arne Schwabe <[email protected]...>
     URL: http://thread.gmane.org/gmane.network.openvpn.devel/7743
     URL: http://thread.gmane.org/gmane.network.openvpn.devel/7744
     Message-Id: [email protected]...
     Signed-off-by: David Sommerseth <[email protected]...>

- --
kind regards,

David Sommerseth

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)

iEYEARECAAYFAlHe/08ACgkQDC186MBRfrqzmgCfZE9K8QB+eXQreHYh6j/Ptb2Q U2kAn1UHEX4RZv9aii13vNQV5+H0WIPV
=HIhP
-----END PGP SIGNATURE-----

Am 24.06.13 01:04, schrieb James Yonan:

OpenVPN for Android already ships this change and there seem some incompatibility. I have a report from a user which reports that against his OpenVPN server (Tomato Router Firmware - OpenVPN 2.2.2):

I am sure if this due to a broken configuration on the users side or the TLS.

P:TLS: Initial packet from [AF_INET]x.x.x.x:443, sid=3D3be6e216 89488=7fd P:TLS_ERROR: BIO read tls_read_plaintext error: error:14092105:SSL routines:SSL3_GET_SERVER_HELLO:wrong cipher returned
P:TLS Error: TLS object -> incoming plaintext read error P:TLS Error: TLS handshake failed
P:Fatal TLS error (check_tls_errors_co), restarting

Hi,

[..]

So if I understand the issue right, it's caused by OpenSSL 0.96 on the server side (which is ancient, but obviously still shipping).

I think we have three options now, and need to decide

- tell users "this is how it is, your server side is a gaping security hole due to ancient OpenSSL (do we have facts to back that?), get your server upgraded!"
- not overly nice, especially if the user has no real control about what router firmware bundlers ship

- back out the change
- I'd rather not do that, as 2.3.x will be around for a long time, and we *want* to be able to use higher TLS versions if possible

- introduce a new option --talking-to-old-server-disable-tls-nego-yes-stupid-I-know that will run-time disable TLS negotiation, falling back to the old code path that only does TLS 1.0
- while making our code even more complex and adding even more option, this would give us "higher TLS version" security, and a knob to get back compatibility for those setups where OpenSSL on either side is too broken to handle TLS 1.1 or 1.2
- we could name this option "--tls-max-version", to complement the existing --tls-min-version option to leave negotiation enabled, but set bounds - from some other project, I have learned that there are combinations of OpenSSL versions where TLS 1.0 and 1.1 negotiate fine, but 1.2 fails (just hangs) - so that might be a more clean approach. Add big warning to the log if used .-)

I can neither comment on the problem nor write the code, so I can only try to get the discussion going to fix this.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

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

Am 21.08.13 09:27, schrieb Gert Doering:

Well I am not really sure what is going on on the Tomato firmware. I build a OpenSSL 0.9.7e (0.9.7e-3sarge3 to be exact, might already have some fixes in it, Tomato has 0.9.8d) on amd64 + OpenVPN 2.2.2 and that worked against 2.4-master. Before backing out the change or adding a backward fix I would like to understand what the real problem here is.

Arne

Arne
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Darwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlIUejQACgkQe8+cMNS4zRdYwACguUMi+4kMLDuKfgLVH3BScQLa Z4YAn0EItxf2cOsxKyf6deQYEbo8VAfH
=0r25
-----END PGP SIGNATURE-----

Hi,

Yeah, fine with me. I thought it was somewhat easily reproduceable and affected a larger user base - "doing nothing except document that this is a very rare ocurrance and can be fixed by <x>" is much better than adding code :-)

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

On 21/08/13 10:28, Arne Schwabe wrote:
[...snip...]

<grumpy_maintainer tongue_in_cheek="1">

IIRC, we decided a long time ago that the oldest distro we would support is RHEL5. That code base is really getting ancient these days, but it ships OpenSSL 0.9.8e. So I think we should have that in mind.

However, I understand that some firmware "vendors" doesn't have the same "quick" turn-over as RHEL does. So I think I would rather let these old firmwares run OpenVPN 2.0 or 2.1 and let them support their own ancient code base on their own. We need to move forward and not cling to the old days forever.

After all, the community supports OpenVPN 2.2 (to some degree) and 2.3 (latest stable) currently. And I wouldn't be surprised if we drop 2.2 when 2.4 sees the light of day. In addition, I think we raised the minimum requirement in 2.3 to use OpenSSL 0.9.7 or newer (see commit 9b33b5a4). I would even say we should probably even raise that one to 0.9.8 in OpenVPN 2.4.

</grumpy_maintainer>

--
kind regards,

David Sommerseth

kind regards,

David Sommerseth

Hi,

This is not about "our source does not compile with 0.96 anymore" - it's about "2.3 with the TLS changes does not *talk* to OpenVPN 2.2-compiled-with-0.96 anymore" (so asking the router vendors to use OpenVPN 2.1 or 2.0 instead won't exactly improve things as the TLS handshake code would be the same).

If this hits two users out of the whole Android user base, I tend to "well, bad luck, get your router firmware updated" - if it hits more "mainstream" users, we'll need to have a better answer.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

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

I've been using a Windows build based on the official 2.3.2 release sources with the TLS negotiation (v1.0-v1.2) support patched in from the development codebase. This has been running fine for me, although I don't connect to a wide variety of servers with it.

I'd be happy to publish a copy of this build for early-adopters interested in trying out the TLS change in their own environments. Consider this kind of a "pre-rc release" to try out just this upcoming feature so we can see if anyone experiences problems with a bit wider usage. I need to make some changes to my build system to include the signed tap-windows drivers, but I can probably have a gpg-signed installer available in a day or two for the interested.

I suspect the impact of places this will break is limited to _very_ old server setups where they should really either upgrade their server, or simply continue using an old client that continues to speak ancient OpenSSL if that's "not possible."

- --
Josh Cepek
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.20 (GNU/Linux)

iQGcBAEBAgAGBQJSFxImAAoJENcx2Xpgb9RjLe8L/1r1KpFscCskKbv+qKGQhLX/ 1wO+OMoDmj1uZkMwfBmxRmhD7lWlLf/rIjq1wQeFwm6xF+kiO3Dyv034d2Z+htP5 PahLbWeC2Z7OLSzAn92uKCi8wfv1m16hNic6d6Z39+Yaw4kej+UQb5l8Z3DrxOyg aCUdmLjpl5qOrgaSCptflhaRgqVGPD/7/ivsh42gUTpvcUHBnm41Ikj3fkYu+pK5 TC90XOWVqZ9mcUBcZDtW5+1PslFkxog+2VloNGMGY6gPHyotSz5E6Xmps6IDuXlp vFNAQQm06wkHj+Qi1DN6JRAx8aKD+t3/gkR6LxxZbT/G4EAgpjHqWyLhoSq4Dn5P f9s9lT/KpYxGM5RBetOF+Wm5+Duw7EkVQqvGuNNha8rC6zyvnfVqLn6onQCJ51x3 aPX/8rnZh3wRA7LbkvZbkr1sTiWpyWFNlpePXPKrDphKhFaSmKraRV0oreRlNlDk AoGoVNNr6rx7HhkAuR58tmbME8hYyw9qP7UhFoQkFw==
=6Swa
-----END PGP SIGNATURE-----

Fully understood!

Well, that was actually _exactly_ what I meant :)

I generally have no bad feelings if we state publicly that OpenVPN doesn't support any older OpenSSL libraries than 0.9.8 (or 0.9.7, if insisting hard), even if OpenVPN compiles with them. Running anything that old is a security risk (and has been for quite a while [1]). So I see no reason why we should make it easier for anyone in these cases. We've ditched Win2k and RHEL4 support for similar convenience reasons as well.

[1] Last update to 0.9.6 was in March 2004

    Last update to 0.9.7 was in Feb 2007.
    0.9.8 and newer are getting updates.
    <https://www.openssl.org/source/>

kind regards,

David Sommerseth

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

Am 21.08.13 09:27, schrieb Gert Doering:

Okay, I have some new light on the issue. Tomato Firmware seems not be the same everywhere. I had contact with another user who identified the Tomato Fork which exhibits this bug. As it turn out the OpenSSL version is rather modern (1.0.1c).

OpenSSL seems to stripped down to a minimum.

[email protected]...:/tmp/home/root# openssl version -a
openssl:Error: 'version' is an invalid command.

Standard commands
enc req rsa x509

Cipher commands (see the `enc' command for more details)

aes-128-cbc    aes-128-ecb    aes-192-cbc    aes-192-ecb    aes-256-cbc
aes-256-ecb    bf-cbc         bf-ecb

The Makefile lists a lot of no-xxx options
https://github.com/Victek/TomatoRAF/blob/master/release/src/router/openssl/

The problem must be somehting different than a terrible old OpenSSL version. I am leaning towards a "terrible configured" OpenSSL version but my understanding of the whole cipher selection and TLS process is too limited to spot anything.

Arne

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Darwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlIood4ACgkQe8+cMNS4zRfP7wCfcA9OHECNvQN8nj59INIXtEt2 ikcAoJa8NUP2z78G+dtf6iG+lJXMOYyA
=xqiF
-----END PGP SIGNATURE-----

Screw that. OpenVPN version of github and running on the router don't match. The source on github (even with history) does match the router which has the problem.

Arne