Use SHA256 for the internal digest, instead of MD5
PATCH 1/1 Use SHA256 for the internal digest, instead of MD5
Our internal options digest uses MD5 hashes to store the state, instead of storing the full options string. There's nothing wrong with that, but it would still be better to use SHA256 because:
* That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5)
* We don't have to explain anymore that MD5 is fine too
The slightly less bytes for the digest (16 instead of 32) and operations per connection setup are not worth sticking to MD5.
---
This patch is meant for the master branch only.
src/openvpn/crypto.h | 6 +++--- src/openvpn/crypto_mbedtls.h | 1 + src/openvpn/crypto_openssl.h | 1 + src/openvpn/init.c | 10 +++++----- src/openvpn/openvpn.h | 6 +++--- src/openvpn/push.c | 23 ++++++++--------------- 6 files changed, 21 insertions(+), 26 deletions(-)
src/openvpn/crypto.h +3 -3
@@ -132,9 +132,9 @@#include "packet_id.h" #include "mtu.h" -/** Wrapper struct to pass around MD5 digests */ -struct md5_digest { -uint8_t digest[MD5_DIGEST_LENGTH ]; +/** Wrapper struct to pass around SHA256 digests */ +struct sha256_digest { +uint8_t digest[SHA256_DIGEST_LENGTH ]; };/*
src/openvpn/crypto_mbedtls.h +1 -0
@@ -73,6 +73,7 @@ typedef mbedtls_md_context_t hmac_ctx_t;#define MD4_DIGEST_LENGTH 16#define MD5_DIGEST_LENGTH 16#define SHA_DIGEST_LENGTH 20 +#define SHA256_DIGEST_LENGTH 32#define DES_KEY_LENGTH 8/**
src/openvpn/crypto_openssl.h +1 -0
@@ -33,6 +33,7 @@#include <openssl/evp.h> #include <openssl/hmac.h> #include <openssl/md5.h> +#include <openssl/sha.h> /** Generic cipher key type %context. */ typedef EVP_CIPHER cipher_kt_t ;
src/openvpn/init.c +5 -5
@@ -1903,12 +1903,12 @@ tun_abort()* equal, or either one is all-zeroes. */ static bool -options_hash_changed_or_zero (const struct md5_digest *a, -const struct md5_digest *b) +options_hash_changed_or_zero (const struct sha256_digest *a, +const struct sha256_digest *b) { -const struct md5_digest zero = {{0 }}; -return memcmp (a, b,sizeof (struct md5_digest )) - || !memcmp (a, &zero,sizeof (struct md5_digest )); +const struct sha256_digest zero = {{0 }}; +return memcmp (a, b,sizeof (struct sha256_digest )) + || !memcmp (a, &zero,sizeof (struct sha256_digest )); }#endif /* P2MP */
src/openvpn/openvpn.h +3 -3
@@ -202,7 +202,7 @@ struct context_1#endif /* if client mode, hash of option strings we pulled from server */ -struct md5_digest pulled_options_digest_save; +struct sha256_digest pulled_options_digest_save;/**< Hash of option strings received from the * remote OpenVPN server. Only used in * client-mode. */
@@ -471,9 +471,9 @@ struct context_2bool did_pre_pull_restore;/* hash of pulled options, so we can compare when options change */ -bool pulled_options_md5_init_done; +bool pulled_options_digest_init_done;md_ctx_t pulled_options_state; -struct md5_digest pulled_options_digest; +struct sha256_digest pulled_options_digest;struct event_timeout scheduled_exit;int scheduled_exit_signal;
src/openvpn/push.c +8 -15
@@ -677,23 +677,17 @@ process_incoming_push_request(struct context *c)#endif /* if P2MP_SERVER */ static void -push_update_digest (md_ctx_t *ctx,struct buffer *buf,const struct options *opt) +push_update_digest (md_ctx_t *ctx,struct buffer *buf) {char line[OPTION_PARM_SIZE ];while (buf_parse (buf,',' , line,sizeof (line))) {/* peer-id might change on restart and this should not trigger reopening tun */ -if (strprefix (line,"peer-id " )) +if (strstr (line,"peer-id " ) != line) { -continue ; - } -/* tun reopen only needed if cipher change can change tun MTU */ -if (strprefix (line,"cipher " ) && !opt->ce.tun_mtu_defined) - { -continue ; +md_ctx_update (ctx, (const uint8_t *) line,strlen (line)); } } -md_ctx_update (ctx, (const uint8_t *) line,strlen (line)+1 ); }int
@@ -720,10 +714,10 @@ process_incoming_push_msg(struct context *c,if (ch ==',' ) {struct buffer buf_orig = buf; -if (!c->c2.pulled_options_md5_init_done) +if (!c->c2.pulled_options_digest_init_done) { -md_ctx_init (&c->c2.pulled_options_state,md_kt_get ("MD5" )); - c->c2.pulled_options_md5_init_done = true; +md_ctx_init (&c->c2.pulled_options_state,md_kt_get ("SHA256" )); + c->c2.pulled_options_digest_init_done = true; }if (!c->c2.did_pre_pull_restore) {
@@ -736,15 +730,14 @@ process_incoming_push_msg(struct context *c, option_types_found, c->c2.es)) { -push_update_digest (&c->c2.pulled_options_state, &buf_orig, - &c->options); +push_update_digest (&c->c2.pulled_options_state, &buf_orig);switch (c->options.push_continuation) {case 0 :case 1 :md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);md_ctx_cleanup (&c->c2.pulled_options_state); - c->c2.pulled_options_md5_init_done = false; + c->c2.pulled_options_digest_init_done = false; ret =PUSH_MSG_REPLY ;break ;
Signature · 1 line
2.7.4
Hi,
Quoted 8 lines · Steffan Karger
Our internal options digest uses MD5 hashes to store the state, instead of storing the full options string. There's nothing wrong with that, but it would still be better to use SHA256 because:
* That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5)
* We don't have to explain anymore that MD5 is fine tooThe slightly less bytes for the digest (16 instead of 32) and operations per connection setup are not worth sticking to MD5.
I can't find very clear information on "which versions of OpenSSL do support sha256", but since we have a trac ticket about our windows builds having issues with sha256 certificates we might take this opportunity to revisit minimum OpenSSL versions supported in master from now on...
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]...Hi,
Quoted 7 lines · Gert Doering
Quoted 6 lines · Steffan Karger
Our internal options digest uses MD5 hashes to store the state, instead of storing the full options string. There's nothing wrong with that, but it would still be better to use SHA256 because:
* That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5)
* We don't have to explain anymore that MD5 is fine too
Quoted 8 lines
Quoted 2 lines
The slightly less bytes for the digest (16 instead of 32) and operations per connection setup are not worth sticking to MD5.
I can't find very clear information on "which versions of OpenSSL do support sha256", but since we have a trac ticket about our windows builds having issues with sha256 certificates we might take this opportunity to revisit minimum OpenSSL versions supported in master from now on...
The oldest OpenSSL we support in release/2.4 and master is 0.9.8, and has SHA256 support (was introduced in 2004). Also, the --tls-crypt feature already unconditionally requires SHA256 to be available.
-Steffan
Quoted 19 lines · Steffan Karger
Hi,
Quoted 7 lines · Gert Doering
Quoted 6 lines · Steffan Karger
Our internal options digest uses MD5 hashes to store the state, instead of storing the full options string. There's nothing wrong with that, but it would still be better to use SHA256 because:
* That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5)
* We don't have to explain anymore that MD5 is fine tooQuoted 8 lines
Quoted 2 lines
The slightly less bytes for the digest (16 instead of 32) and operations per connection setup are not worth sticking to MD5.
I can't find very clear information on "which versions of OpenSSL do support sha256", but since we have a trac ticket about our windows builds having issues with sha256 certificates we might take this opportunity to revisit minimum OpenSSL versions supported in master from now on...
Quoted 3 lines
The oldest OpenSSL we support in release/2.4 and master is 0.9.8, and has SHA256 support (was introduced in 2004). Also, the --tls-crypt feature already unconditionally requires SHA256 to be available.
The OpenSSL included in macOS (was OS X) 10.11 and 10.12 (the two most recent versions) is 0.9.8zh (an Apple-patched version) and as far as I can tell, it does not seem to include SHA256 (i.e., "openssl sha1 foo" works but "openssl sha256 foo" doesn't).
That's not a problem for Tunnelblick because Tunnelblick static-links in a more recent version of OpenSSL. But it could be a problem for anyone who uses a non-Tunnelblick OpenVPN binary from the command line.
- Jon
Quoted 4 lines · Jonathan K. Bullard
The OpenSSL included in macOS (was OS X) 10.11 and 10.12 (the two most recent versions) is 0.9.8zh (an Apple-patched version) and as far as I can tell, it does not seem to include SHA256 (i.e., "openssl sha1 foo" works but "openssl sha256 foo" doesn't).
Try "openssl dgst -sha256".
-Steffan
Am 26.12.16 um 09:14 schrieb Steffan Karger:
Quoted 1 line
openssl dgst -sha256
works for me
[9:37]{SIGINT}[email protected]...:~% openssl version OpenSSL 0.9.8zh 14 Jan 2016
[9:37][email protected]...:~% openssl dgst -sha256
abcd^D fc4b5fd6816f75a7c81fc8eaa9499d6a299bd803397166e8c4cf9280b801d62c [9:37][email protected]...:~%
But anyway. I do not think supporting this ancient OpenSSL version is wise idea. Even OpenSSH under OS X links against its own LibreSSL version.
Arne
Hi,
Quoted 3 lines · Steffan Karger
The oldest OpenSSL we support in release/2.4 and master is 0.9.8, and has SHA256 support (was introduced in 2004). Also, the --tls-crypt feature already unconditionally requires SHA256 to be available.
Good enough for me.
Have to look more closely at the actual patch, though :)
As for "build on MacOS", I seem to remember from plaisthos' buildslave that we cannot currently build on an "unmodified" MacOS anyway, but have to install a more recent (or more complete?) OpenSSL beforehand - so that particular ship has sailed.
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]...Am 26.12.16 um 08:05 schrieb Gert Doering:
Quoted 7 lines
Hi,
Quoted 3 lines · Steffan Karger
The oldest OpenSSL we support in release/2.4 and master is 0.9.8, and has SHA256 support (was introduced in 2004). Also, the --tls-crypt feature already unconditionally requires SHA256 to be available.
Quoted 8 lines
Good enough for me.
Have to look more closely at the actual patch, though :)
As for "build on MacOS", I seem to remember from plaisthos' buildslave that we cannot currently build on an "unmodified" MacOS anyway, but have to install a more recent (or more complete?) OpenSSL beforehand - so that particular ship has sailed.
There are no OpenSSL header files available for OpenSSL. So building against that system version is harder than installing a new version.
Arne
Quoted 19 lines · Steffan Karger
Hi,
Quoted 7 lines · Gert Doering
Quoted 6 lines · Steffan Karger
Our internal options digest uses MD5 hashes to store the state, instead of storing the full options string. There's nothing wrong with that, but it would still be better to use SHA256 because:
* That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5)
* We don't have to explain anymore that MD5 is fine tooQuoted 8 lines
Quoted 2 lines
The slightly less bytes for the digest (16 instead of 32) and operations per connection setup are not worth sticking to MD5.
I can't find very clear information on "which versions of OpenSSL do support sha256", but since we have a trac ticket about our windows builds having issues with sha256 certificates we might take this opportunity to revisit minimum OpenSSL versions supported in master from now on...
Quoted 3 lines
The oldest OpenSSL we support in release/2.4 and master is 0.9.8, and has SHA256 support (was introduced in 2004). Also, the --tls-crypt feature already unconditionally requires SHA256 to be available.
Just to confirm this is fine.
$ cat /etc/redhat-release CentOS release 5.11 (Final) $ rpm -q openssl openssl-0.9.8e-40.el5_11 $ echo "test" | openssl dgst -sha256 f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 $ echo "test" | sha256sum f2ca1bb6c7e907d06dafe4687e579fce76b37e4e93b7605022da52e6ccc26fd2 -
And from the earliest openssl RPM changelog entry where 0.9.8 is mentioned, I see this:
* Wed Nov 09 2005 Tomas Mraz <tmraz(a)redhat.com> 0.9.8a-1
- new upstream version
AFAIR, we don't support any other OS or distributions with an older OpenSSL library.
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc
Quoted 6 lines · Steffan Karger
Our internal options digest uses MD5 hashes to store the state, instead of storing the full options string. There's nothing wrong with that, but it would still be better to use SHA256 because:
* That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5)
* We don't have to explain anymore that MD5 is fine too
Quoted 7 lines
The slightly less bytes for the digest (16 instead of 32) and operations per connection setup are not worth sticking to MD5.
Signed-off-by Steffan Karger <[email protected]...>---
This patch is meant for the master branch only.
I started looking at this patch .... and I think it needs some rework, as it does a couple of unexpected things ...
[...snip...]
Quoted 19 lines
src/openvpn/push.c +2
-3@@ -677,23 +677,17 @@ process_incoming_push_request(struct context *c)#endif /* if P2MP_SERVER */ static void -push_update_digest (md_ctx_t *ctx,struct buffer *buf,const struct options *opt) +push_update_digest (md_ctx_t *ctx,struct buffer *buf) {char line[OPTION_PARM_SIZE ];while (buf_parse (buf,',' , line,sizeof (line))) {/* peer-id might change on restart and this should not trigger reopening tun */ -if (strprefix (line,"peer-id " )) +if (strstr (line,"peer-id " ) != line) { -continue ;
src/openvpn/push.c process_incoming_push_request(struct context *c) · line 691
Quoted 10 lines
src/openvpn/push.c +1
-6- } -/* tun reopen only needed if cipher change can change tun MTU */ -if (strprefix (line,"cipher " ) && !opt->ce.tun_mtu_defined) - { -continue ; +md_ctx_update (ctx, (const uint8_t *) line,strlen (line)); } } -md_ctx_update (ctx, (const uint8_t *) line,strlen (line)+1 ); }
This actually tries to revert commit ec4dff3bbdcc9fedf7844 ... which is quite surprising.
[...snip...]
src/openvpn/push.c process_incoming_push_msg(struct context *c, · line 733
Quoted 7 lines
src/openvpn/push.c +1
-2@@ -736,15 +730,14 @@ process_incoming_push_msg(struct context *c, option_types_found, c->c2.es)) { -push_update_digest (&c->c2.pulled_options_state, &buf_orig, - &c->options); +push_update_digest (&c->c2.pulled_options_state, &buf_orig);
And this too is also a revert of the same commit as above.
Had it been just a simple rebase, I'd be willing to tackle that on-the-fly. But as it tries to do some really unexpected reverts, there might be a few other issues I'm not able to spot instantly.
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc
Hi,
Quoted 10 lines · David Sommerseth
This actually tries to revert commit
ec4dff3bbdcc9fedf7844... which is quite surprising.[...snip...]
And this too is also a revert of the same commit as above.
Had it been just a simple rebase, I'd be willing to tackle that on-the-fly. But as it tries to do some really unexpected reverts, there might be a few other issues I'm not able to spot instantly.
Ouch, that's one ugly rebase error. I'll double-check the patch, and resend a properly rebased one.
-Steffan
PATCH v2 1/1 Use SHA256 for the internal digest, instead of MD5
Our internal options digest uses MD5 hashes to store the state, instead of storing the full options string. There's nothing wrong with that, but it would still be better to use SHA256 because:
* That makes it easier to make OpenVPN "FIPS-compliant" (forbids MD5)
* We don't have to explain anymore that MD5 is fine too
The slightly less bytes for the digest (16 instead of 32) and operations per connection setup are not worth sticking to MD5.
Note that might SHA256 not be available in de crypto lib, OpenVPN will refuse to start and shout "Message hash algorithm 'SHA256' not found".
---
v2: fix rebase error that erroneously reverted commit ec4dff3b
src/openvpn/crypto.h | 6 +++--- src/openvpn/crypto_mbedtls.h | 1 + src/openvpn/crypto_openssl.h | 1 + src/openvpn/init.c | 10 +++++----- src/openvpn/openvpn.h | 6 +++--- src/openvpn/push.c | 8 ++++---- 6 files changed, 17 insertions(+), 15 deletions(-)
src/openvpn/crypto.h +3 -3
@@ -131,9 +131,9 @@#include "packet_id.h" #include "mtu.h" -/** Wrapper struct to pass around MD5 digests */ -struct md5_digest { -uint8_t digest[MD5_DIGEST_LENGTH ]; +/** Wrapper struct to pass around SHA256 digests */ +struct sha256_digest { +uint8_t digest[SHA256_DIGEST_LENGTH ]; };/*
src/openvpn/crypto_mbedtls.h +1 -0
@@ -73,6 +73,7 @@ typedef mbedtls_md_context_t hmac_ctx_t;#define MD4_DIGEST_LENGTH 16#define MD5_DIGEST_LENGTH 16#define SHA_DIGEST_LENGTH 20 +#define SHA256_DIGEST_LENGTH 32#define DES_KEY_LENGTH 8/**
src/openvpn/crypto_openssl.h +1 -0
@@ -33,6 +33,7 @@#include <openssl/evp.h> #include <openssl/hmac.h> #include <openssl/md5.h> +#include <openssl/sha.h> /** Generic cipher key type %context. */ typedef EVP_CIPHER cipher_kt_t ;
src/openvpn/init.c +5 -5
@@ -1919,12 +1919,12 @@ tun_abort()* equal, or either one is all-zeroes. */ static bool -options_hash_changed_or_zero (const struct md5_digest *a, -const struct md5_digest *b) +options_hash_changed_or_zero (const struct sha256_digest *a, +const struct sha256_digest *b) { -const struct md5_digest zero = {{0 }}; -return memcmp (a, b,sizeof (struct md5_digest )) - || !memcmp (a, &zero,sizeof (struct md5_digest )); +const struct sha256_digest zero = {{0 }}; +return memcmp (a, b,sizeof (struct sha256_digest )) + || !memcmp (a, &zero,sizeof (struct sha256_digest )); }#endif /* P2MP */
src/openvpn/openvpn.h +3 -3
@@ -202,7 +202,7 @@ struct context_1#endif /* if client mode, hash of option strings we pulled from server */ -struct md5_digest pulled_options_digest_save; +struct sha256_digest pulled_options_digest_save;/**< Hash of option strings received from the * remote OpenVPN server. Only used in * client-mode. */
@@ -471,9 +471,9 @@ struct context_2bool did_pre_pull_restore;/* hash of pulled options, so we can compare when options change */ -bool pulled_options_md5_init_done; +bool pulled_options_digest_init_done;md_ctx_t pulled_options_state; -struct md5_digest pulled_options_digest; +struct sha256_digest pulled_options_digest;struct event_timeout scheduled_exit;int scheduled_exit_signal;
src/openvpn/push.c +4 -4
@@ -720,10 +720,10 @@ process_incoming_push_msg(struct context *c,if (ch ==',' ) {struct buffer buf_orig = buf; -if (!c->c2.pulled_options_md5_init_done) +if (!c->c2.pulled_options_digest_init_done) { -md_ctx_init (&c->c2.pulled_options_state,md_kt_get ("MD5" )); - c->c2.pulled_options_md5_init_done = true; +md_ctx_init (&c->c2.pulled_options_state,md_kt_get ("SHA256" )); + c->c2.pulled_options_digest_init_done = true; }if (!c->c2.did_pre_pull_restore) {
@@ -744,7 +744,7 @@ process_incoming_push_msg(struct context *c,case 1 :md_ctx_final (&c->c2.pulled_options_state, c->c2.pulled_options_digest.digest);md_ctx_cleanup (&c->c2.pulled_options_state); - c->c2.pulled_options_md5_init_done = false; + c->c2.pulled_options_digest_init_done = false; ret =PUSH_MSG_REPLY ;break ;
Signature · 1 line
2.7.4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
ACK.
Your patch has been applied to the following branches
commit 5b48e8c9f85442936f744c3c550d9d41fe8c7b60 (master) commit 5cf585ce605d2328d7a2bb5cf44e82b8b196b551 (release/2.4)
Author: Steffan Karger Date: Sun Jan 22 17:04:41 2017 +0100
Use SHA256 for the internal digest, instead of MD5
Signed-off-by: Steffan Karger <[email protected]...>
Acked-by: David Sommerseth <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg13926.html
Signed-off-by: David Sommerseth <[email protected]...>- --
kind regards,
David Sommerseth
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJYhU0wAAoJEIbPlEyWcf3yOuwQAKb6DRBLxLf4ad4QJ2L4LBNt u0gCREHT9HwliHvK485JQWCIZ29LpgNFVteQvWHEKzWl8NqjcHV7Af4xCT/MCGwz 8aMQrCTyhNu5lacZeH5n67PQqTfENXsZ/YrqVk7ve2Ku0Scn9irOQkeocGLEOvGS /X2gICZhpHBPJdjzc2WSkd2XtjpvhYXpNDl0zeUdqOyhjA2Vfki6Zh7tXFeu0zav GAjiR2uQjZGxQHudg9hC/9lReaZ1rkw90VVPldih+Fo5ip5Mja9r0epi/B9p0xoL OU6jCJ1SNtTjM78+b/+Zf3hhHRSW78GUAR315nv9XKEbecGQoBAi+kN7wFNB8EH9 oHxXgyrfg9eBBFkZRqaOYaM/VuAjBPGbEoh3dNOSAicMJzyIFB8HSBgEdUYTbQEH Uu95p9oMcQz+2+04K+Ac3O8sNWOiYaqI5GEwIZRoCDpg0WIpW61yuOg9yH3dTpGD l7CeRKvBQMrRhw6wpcdTl36HgL/F2qnWhKba0TKZE48jtZ5aRge7S4STYqq6KQxT lDHz2GqMWfPIq3JBLIT80jyBXd8JYPT9he508iRfdHX1alGen0lrIQkLWB7soyqE aAc/zIUvQ6l+b3TwJ+BbaKXn0JSU4yq80/o91ChRB45RFsG7J4/sEZkvgX8Ci/+F cMFQv2UkL9M/aW+4+M4F
=sLL8
-----END PGP SIGNATURE-----