PolarSSL 1.2 support for OpenVPN 2.3
Hi,
From PolarSSL 1.1 to 1.2 the cipher names have changed and the API changed a bit. This causes the 2.3 release of OpenVPN to be in- compatible with PolarSSL 1.2.
We've prepared three patches to add support for PolarSSL 1.2 to OpenVPN 2.3. The first two patches just improve error reporting, the third patch adds the actual PolarSSL 1.2 support.
Version 1.2.0 to 1.2.2 of PolarSSL contained several issues that caused it to be unfit for use with OpenVPN. I added checks in the configure file to report an error when someone is trying to build OpenVPN against one of these PolarSSL versions.
This should also close ticket #250.
Kind regards,
Steffan Karger
Fox-IT
PATCH 1/3 Improve PolarSSL key_state_read_{cipher, plain}text messages
From: Steffan Karger <[email protected]...>
Instead of logging the fact that an error occured, log the error string or (if the PolarSSL build does not include this feature) the error code.
--- src/openvpn/ssl_polarssl.c | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-)
src/openvpn/ssl_polarssl.c +18 -2
@@ -666,6 +666,9 @@ key_state_read_ciphertext (struct key_state_ssl *ks, struct buffer *buf, {int retval =0 ;int len =0 ; +#ifdef POLARSSL_ERROR_C +char error_message[1024 ]; +#endif perf_push (PERF_BIO_READ_CIPHERTEXT );
@@ -691,7 +694,12 @@ key_state_read_ciphertext (struct key_state_ssl *ks, struct buffer *buf,perf_pop ();if (POLARSSL_ERR_NET_WANT_WRITE == retval ||POLARSSL_ERR_NET_WANT_READ == retval)return 0 ; -msg (D_TLS_ERRORS ,"TLS_ERROR: read tls_read_plaintext error" ); +#ifdef POLARSSL_ERROR_C +error_strerror (retval, error_message,sizeof (error_message)); +msg (D_TLS_ERRORS ,"TLS_ERROR: read tls_read_ciphertext error: %s" , error_message); +#else +msg (D_TLS_ERRORS ,"TLS_ERROR: read tls_read_ciphertext error: %d" , retval); +#endif buf->len =0 ;return -1 ; }
@@ -763,6 +771,9 @@ key_state_read_plaintext (struct key_state_ssl *ks, struct buffer *buf, {int retval =0 ;int len =0 ; +#ifdef POLARSSL_ERROR_C +char error_message[1024 ]; +#endif perf_push (PERF_BIO_READ_PLAINTEXT );
@@ -787,7 +798,12 @@ key_state_read_plaintext (struct key_state_ssl *ks, struct buffer *buf, {if (POLARSSL_ERR_NET_WANT_WRITE == retval ||POLARSSL_ERR_NET_WANT_READ == retval)return 0 ; -msg (D_TLS_ERRORS ,"TLS_ERROR: read tls_read_plaintext error" ); +#ifdef POLARSSL_ERROR_C +error_strerror (retval, error_message,sizeof (error_message)); +msg (D_TLS_ERRORS ,"TLS_ERROR: read tls_read_plaintext error: %s" , error_message); +#else +msg (D_TLS_ERRORS ,"TLS_ERROR: read tls_read_plaintext error: %d" , retval); +#endif buf->len =0 ;perf_pop ();return -1 ;
Signature · 1 line
1.7.9.5
Re: [Openvpn-devel] [PATCH 1/3] Improve PolarSSL key_state_read_{cipher, plain}text messages
Hi,
Quoted 6 lines · On Thu, Jan 17, 2013 at 09:23:27AM +0100, [email protected]... wrote:
From: Steffan Karger <[email protected]...>
Instead of logging the fact that an error occured, log the error string or (if the PolarSSL build does not include this feature) the error code.
Signed-off-by Joachim Schipper <[email protected]...>
This looks good to me - ACK.
Description of error_strerror() is here:
https://github.com/polarssl/polarssl/blob/master/include/polarssl/error.h#L91
(Even though the design decision to call a library-specific error reporting function "error_strerror()" is not something I find particularily well chosen... what's wrong with "polar_strerror()"???)
gert
Signature · 4 lines
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]...Re: [Openvpn-devel] [PATCH 1/3] Improve PolarSSL key_state_read_{cipher, plain}text messages
Hi Gert,
Quoted 1 line
(Even though the design decision to call a library-specific error
reporting
Quoted 1 line
function "
error_strerror()" is not something I find particularily well
chosen...
Quoted 1 line
what's wrong with "
polar_strerror()"???)
Nothing is wrong with that. It's PolarSSL coding standard compliant
(https://polarssl.org/kb/development/polarssl-coding-standards)
The PolarSSL SSL library function naming standard is 'MODULE_FUNCTION_NAME'. That is the cause of the current naming convention.
If this proves to be an issue with other implementation we will reconsider of course ;)
Regards,
Paul
PATCH 2/3 Improve verify_callback messages
In reply to [email protected]...From: Steffan Karger <[email protected]...>
Print the *flags argument, which - for PolarSSL-1.2 - contains the reasons that the certificate failed (pre-)verification.
--- src/openvpn/ssl_verify_polarssl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
src/openvpn/ssl_verify_polarssl.c +3 -3
@@ -64,10 +64,10 @@ verify_callback (void *session_obj, x509_cert *cert, int cert_depth,char *subject =x509_get_subject (cert, &gc);if (subject) -msg (D_TLS_ERRORS ,"VERIFY ERROR: depth=%d, %s" , cert_depth, subject); +msg (D_TLS_ERRORS ,"VERIFY ERROR: depth=%d, flags=%0x, %s" , cert_depth, *flags, subject);else -msg (D_TLS_ERRORS ,"VERIFY ERROR: depth=%d, could not extract X509 " -"subject string from certificate" , cert_depth); +msg (D_TLS_ERRORS ,"VERIFY ERROR: depth=%d, flags=%0x, could not extract X509 " +"subject string from certificate" , *flags, cert_depth); goto cleanup; }
Signature · 1 line
1.7.9.5
PATCH 3/3 PolarSSL-1.2 support
In reply to [email protected]...From: Steffan Karger <[email protected]...>
Add support for PolarSSL-1.2, which has changed the API in several places. This is a minimal port: PolarSSL-1.2 works as well as PolarSSL-1.1, but the new features have not been enabled.
Blacklist PolarSSL-1.[0-2] for bugs when verifying certificate chains
--- configure.ac | 2 ++ src/openvpn/crypto_polarssl.h | 4 +++ src/openvpn/options.c | 4 +++ src/openvpn/ssl_polarssl.c | 29 ++++++++++++++++++++-- src/openvpn/ssl_polarssl.h | 4 +++ src/openvpn/ssl_verify_polarssl.c | 49 ++++++++++++++++++++++++++++--------- src/openvpn/ssl_verify_polarssl.h | 45 ++++++++++++++++++++++++++++------ 7 files changed, 115 insertions(+), 22 deletions(-)
configure.ac +2 -0
@@ -807,6 +807,8 @@ if test "${with_crypto_library}" = "polarssl" ; then [[ #if POLARSSL_VERSION_NUMBER < 0x01010000 #error invalid version +#elif POLARSSL_VERSION_NUMBER >= 0x01020000 && POLARSSL_VERSION_NUMBER < 0x01020300 +#error PolarSSL-1.2.x before 1.2.3 is incompatible with OpenVPN #endif ]] )],
src/openvpn/crypto_polarssl.h +4 -0
@@ -60,7 +60,11 @@ typedef md_context_t hmac_ctx_t;#define OPENVPN_MODE_OFB POLARSSL_MODE_OFB/** Cipher is in CFB mode */ +#if POLARSSL_VERSION_NUMBER <0x01020000 #define OPENVPN_MODE_CFB POLARSSL_MODE_CFB128 +#else +#define OPENVPN_MODE_CFB POLARSSL_MODE_CFB +#endif /** Cipher should encrypt */ #define OPENVPN_OP_ENCRYPT POLARSSL_ENCRYPT
src/openvpn/options.c +4 -0
@@ -827,7 +827,11 @@ init_options (struct options *o, const bool init_gc) o->server_poll_timeout =0 ;#endif #ifdef ENABLE_CRYPTO +#ifdef ENABLE_CRYPTO_POLARSSL + o->ciphername ="BLOWFISH-CBC" ; +#else o->ciphername ="BF-CBC" ; +#endif o->ciphername_defined = true; o->authname ="SHA1" ; o->authname_defined = true;
src/openvpn/ssl_polarssl.c +27 -2
@@ -44,6 +44,7 @@#include "manage.h" #include "ssl_common.h" +#include <polarssl/version.h> #include <polarssl/sha2.h> #include <polarssl/havege.h>
@@ -65,6 +66,8 @@ tls_clear_error() { } +#if POLARSSL_VERSION_NUMBER <0x01020000 +/* The default list of ciphers is already initialized in PolarSSL-1.2.0+ */ static int default_ciphersuites[] = { SSL_EDH_RSA_AES_256_SHA,
@@ -81,6 +84,7 @@ static int default_ciphersuites[] =SSL_RSA_RC4_128_MD5 ,0 }; +#endif void tls_ctx_server_new (struct tls_root_ctx *ctx)
@@ -514,27 +518,43 @@ void key_state_ssl_init(struct key_state_ssl *ks_ssl,ssl_set_rng (ks_ssl->ctx, ctr_drbg_random,rand_ctx_get ()); +#if POLARSSL_VERSION_NUMBER <0x01020000 +/* Handled implicitly in PolarSSL-1.2.0+ */ ALLOC_OBJ_CLEAR (ks_ssl->ssn, ssl_session);ssl_set_session (ks_ssl->ctx,0 ,0 , ks_ssl->ssn ); +#endif if (ssl_ctx->allowed_ciphers)ssl_set_ciphersuites (ks_ssl->ctx, ssl_ctx->allowed_ciphers); +#if POLARSSL_VERSION_NUMBER <0x01020000 else +/* Handled implicitly in PolarSSL-1.2.0+ */ ssl_set_ciphersuites (ks_ssl->ctx, default_ciphersuites); +#endif /* Initialise authentication information */ if (is_server)ssl_set_dh_param_ctx (ks_ssl->ctx, ssl_ctx->dhm_ctx );#if defined(ENABLE_PKCS11 ) if (ssl_ctx->priv_key_pkcs11 != NULL) +#if POLARSSL_VERSION_NUMBER <0x01020000 ssl_set_own_cert_pkcs11 ( ks_ssl->ctx, ssl_ctx->crt_chain, ssl_ctx->priv_key_pkcs11 ); +#else +ssl_set_own_cert_alt ( ks_ssl->ctx, ssl_ctx->crt_chain, + ssl_ctx->priv_key_pkcs11, ssl_pkcs11_decrypt, ssl_pkcs11_sign, + ssl_pkcs11_key_len ); +#endif else#endif ssl_set_own_cert ( ks_ssl->ctx, ssl_ctx->crt_chain, ssl_ctx->priv_key );/* Initialise SSL verification */ ssl_set_authmode (ks_ssl->ctx,SSL_VERIFY_REQUIRED ); +#if POLARSSL_VERSION_NUMBER <0x01020000 +ssl_set_verify (ks_ssl->ctx, verify_callback_1_1, session); +#else ssl_set_verify (ks_ssl->ctx, verify_callback, session); +#endif /* 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 );
@@ -543,7 +563,6 @@ void key_state_ssl_init(struct key_state_ssl *ks_ssl,ALLOC_OBJ_CLEAR (ks_ssl->ct_out, endless_buffer);ssl_set_bio (ks_ssl->ctx, endless_buf_read, ks_ssl->ct_in, endless_buf_write, ks_ssl->ct_out); - } }
@@ -556,8 +575,10 @@ key_state_ssl_free(struct key_state_ssl *ks_ssl)ssl_free (ks_ssl->ctx);free (ks_ssl->ctx); } +#if POLARSSL_VERSION_NUMBER <0x01020000 if (ks_ssl->ssn)free (ks_ssl->ssn); +#endif if (ks_ssl->ct_in) {buf_free_entries (ks_ssl->ct_in);free (ks_ssl->ct_in);
@@ -834,7 +855,7 @@ key_state_read_plaintext (struct key_state_ssl *ks, struct buffer *buf,void print_details (struct key_state_ssl * ks_ssl,const char *prefix) { -x509_cert *cert; +const x509_cert *cert;char s1[256 ];char s2[256 ];
@@ -844,7 +865,11 @@ print_details (struct key_state_ssl * ks_ssl, const char *prefix)ssl_get_version (ks_ssl->ctx), ssl_get_ciphersuite(ks_ssl->ctx)); +#if POLARSSL_VERSION_NUMBER <0x01020000 cert = ks_ssl->ctx->peer_cert; +#else + cert =ssl_get_peer_cert (ks_ssl->ctx); +#endif if (cert !=NULL ) {openvpn_snprintf (s2,sizeof (s2),", " counter_format" bit RSA" , (counter_type ) cert->rsa.len *8 );
src/openvpn/ssl_polarssl.h +4 -0
@@ -30,6 +30,7 @@#ifndef SSL_POLARSSL_H_ #define SSL_POLARSSL_H_ +#include <polarssl/version.h> #include <polarssl/ssl.h> #if defined(ENABLE_PKCS11 )
@@ -73,7 +74,10 @@ struct tls_root_ctx {struct key_state_ssl {ssl_context *ctx; +#if POLARSSL_VERSION_NUMBER <0x01020000 +/* Implicit in PolarSSL-1.2.0+ */ ssl_session *ssn; +#endif endless_buffer *ct_in;endless_buffer *ct_out; };
src/openvpn/ssl_verify_polarssl.c +37 -12
83 lines not on this page · raw message
src/openvpn/ssl_verify_polarssl.h +37 -8
75 lines not on this page · raw message
Signature · 1 line
1.7.9.5
Re: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 support
Hi,
src/openvpn/crypto_polarssl.h typedef md_context_t hmac_ctx_t; · line 67
Quoted 6 lines · On Thu, Jan 17, 2013 at 09:23:29AM +0100, [email protected]... wrote:
/** Cipher is in CFB mode */
src/openvpn/crypto_polarssl.h +4
-0+#if POLARSSL_VERSION_NUMBER <0x01020000 #define OPENVPN_MODE_CFB POLARSSL_MODE_CFB128 +#else +#define OPENVPN_MODE_CFB POLARSSL_MODE_CFB +#endif
I don't like this approach - we have way too many #ifdef's already.
Is there any benefit in keeping support for PolarSSL 1.1, given that it's not interoperable with OpenVPN's default crypto settings (blowfish)?
So unless there are strong reasons to support both 1.1 and 1.2, I'd just move over to 1.2, document the requirement "anything before 1.2.3 is not supported, period" and declare 1.1 historic...
gert
Signature · 4 lines
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]...Re: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 support
In reply to [email protected]...Is there any important system where requiring PolarSSL >= 1.2.3 is not an option, besides "admin is too lazy or can't convince his manager that he needs to upgrade"?
This #ifdef stuff makes the whole story a bit inconcise. It might be suitable for 2.3.X, but not to base 2.4 or newer releases on.
Barring that, I'd suggest to add stuff to fail the build with older PolarSSL versions and kill the PolarSSL < 1.2.3 code. It would seem from the changelogs that PolarSSL 1.2.N (with N highest available) does away with certain design issues in earlier versions, so there is a compelling reason to upgrade.
(I was irritated anyways that the newest released OpenVPN version would not work with the newest stable PolarSSL version, and am foregoing the PolarSSL option on the FreeBSD port - we do have an up-to-date PolarSSL, so it wouldn't build.)
Re: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 support
Quoted 12 lines
-----Original Message-----
From: Matthias Andree [mailto:[email protected]...]
Sent: zondag 20 januari 2013 14:09
To: [email protected]
Subject: Re: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 supportIs there any important system where requiring PolarSSL >= 1.2.3 is not an option, besides "admin is too lazy or can't convince his manager that he needs to upgrade"?
Quoted 9 lines
This #ifdef stuff makes the whole story a bit inconcise. It might be suitable for 2.3.X, but not to base 2.4 or newer releases on.
Barring that, I'd suggest to add stuff to fail the build with older PolarSSL versions and kill the PolarSSL < 1.2.3 code. It would seem from the changelogs that PolarSSL 1.2.N (with N highest available) does away with certain design issues in earlier versions, so there is a compelling reason to upgrade.
I agree that it's annoying, but I wouldn't remove support for an existing, still secure version of PolarSSL in a dot-release. We could mark 1.1 as deprecated, and remove it in 2.4? For the moment, recent versions Debian (sid/wheezy) and Ubuntu (precise+) still contain 1.1, meaning that it's a nice service for them to keep support for a while.
Quoted 5 lines
(I was irritated anyways that the newest released OpenVPN version would not work with the newest stable PolarSSL version, and am foregoing the PolarSSL option on the FreeBSD port - we do have an up-to-date PolarSSL, so it wouldn't build.)
Unfortunately getting PolarSSL to work wasn't trivial due to some (necessary) PolarSSL changes. We're still working on getting support patches for a number of the new features ready. Adding these patches to 2.3 during the RC phase would have caused to large a disruption for an RC.
PolarSSL targets a clean API, sometimes sacrificing the stability of the API in the process. While that is annoying in the short term, in the long term it helps to keep me sane as a developer.
Regards,
Adriaan
Re: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 support
Hi,
Quoted 6 lines · Adriaan de Jong
Quoted 5 lines
Barring that, I'd suggest to add stuff to fail the build with older PolarSSL versions and kill the PolarSSL < 1.2.3 code. It would seem from the changelogs that PolarSSL 1.2.N (with N highest available) does away with certain design issues in earlier versions, so there is a compelling reason to upgrade.
Quoted 6 lines
I agree that it's annoying, but I wouldn't remove support for an existing, still secure version of PolarSSL in a dot-release. We could mark 1.1 as deprecated, and remove it in 2.4? For the moment, recent versions Debian (sid/wheezy) and Ubuntu (precise+) still contain 1.1, meaning that it's a nice service for them to keep support for a while.
I don't particularily care about distributions that ship old stuff - they can always compile with OpenSSL if they do not want to upgrade Polar to 1.2 (not that Debian contains OpenVPN 2.3.x anyway).
I'm more concernced about maintainability of the OpenVPN code base, and having more #ifdef in there is not the way forward.
Quoted 5 lines
Unfortunately getting PolarSSL to work wasn't trivial due to some (necessary) PolarSSL changes. We're still working on getting support patches for a number of the new features ready. Adding these patches to 2.3 during the RC phase would have caused to large a disruption for an RC.
Understood, and appreciated. OTOH, the changes are not *that* large, if you get rid of the #ifdefs...
gert
Signature · 4 lines
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]...Re: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 support
Hi,
just for the sake of those not following the discussion on IRC...
Quoted 2 lines · Gert Doering
I'm more concernced about maintainability of the OpenVPN code base, and having more #ifdef in there is not the way forward.
... we have now come to some sort of conclusion on this:
- 2.3.x will support PolarSSL 1.1 and 1.2, to help people building OpenVPN with PolarSSL on distributions that ship 1.1
- 2.4.x will drop support for PolarSSL 1.1, to simplify the code
gert
Signature · 4 lines
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]...Re: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 support
Am 21.01.2013 14:23, schrieb Gert Doering:
Quoted 8 lines
Hi,
just for the sake of those not following the discussion on IRC...
Quoted 2 lines · Gert Doering
I'm more concernced about maintainability of the OpenVPN code base, and having more #ifdef in there is not the way forward.
Quoted 6 lines
... we have now come to some sort of conclusion on this:
- 2.3.x will support PolarSSL 1.1 and 1.2, to help people building OpenVPN with PolarSSL on distributions that ship 1.1
- 2.4.x will drop support for PolarSSL 1.1, to simplify the code
Thank you. This is a very reasonable approach - and exactly what I was proposing, perhaps not spelt out clearly enough.
BTW I wouldn't bother too much about distributions shipping outdated stuff either; if people run Debian with outdated PolarSSL, they are bound to also use whatever OpenVPN version Debian has packaged.
Re: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 support
In reply to [email protected]...Quoted 8 lines
-----Original Message-----
From: [email protected]... [mailto:[email protected]...]
Sent: donderdag 17 januari 2013 9:23
To: [email protected]
Subject: [Openvpn-devel] [PATCH 3/3] PolarSSL-1.2 supportFrom: Steffan Karger <[email protected]...>
Quoted 7 lines
Add support for PolarSSL-1.2, which has changed the API in several places. This is a minimal port: PolarSSL-1.2 works as well as PolarSSL- 1.1, but the new features have not been enabled.
Blacklist PolarSSL-1.[0-2] for bugs when verifying certificate chains
Signed-off-by Joachim Schipper <[email protected]...>
Ack on this and the other two patches in this series. I'd prefer someone from outside of Fox-IT to have a look at them though.
Adriaan