crypto: Enable SHA256 fingerprint checking in --verify-hash
PATCH 1/1 crypto: Enable SHA256 fingerprint checking in --verify-hash
This enhances --verify-hash with an optional algorithm flag. If not provided, it defaults to SHA1 to preserve backwards compatbilitity with existing configurations. The only valid flags are SHA1 and SHA256.
In addition it moves the hash verification away from memcmp() to memcmp_constant_time(). And slightly it enhances the layout of the --verify-hash section in the man page.
---
Simple test script
==================
* Server:
# openvpn --dev tun --reneg-sec 60 --verb 4 \
--server 10.8.0.0 255.255.255.0 --topology subnet \
--ca sample/sample-keys/ca.crt \
--key sample/sample-keys/server.key \
--cert sample/sample-keys/server.crt \
--dh sample/sample-keys/dh2048.pem \
--cipher AES-256-CBC --auth SHA256 \
--verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:F9 SHA256* Client:
# openvpn --dev tun --verb 4 \
--remote 192.168.122.1 --explicit-exit-notify \
--client --ca sample/sample-keys/ca.crt \
--key sample/sample-keys/client.key \
--cert sample/sample-keys/client.crt \
--cipher AES-256-CBC --auth SHA256 \
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44Alternative --verify-hash variant which must succeed:
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44 SHA1
These --verify-hash variants must fail:
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44 SHA256
--verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:F9
--verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:00 SHA256
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:00 SHA1
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:00
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:XXTo retrieve the SHA fingerprints in certificates:
$ openssl x509 -noout -fingerprint -in sample/sample-keys/ca.crt
$ openssl x509 -noout -fingerprint -sha256 -in sample/sample-keys/ca.crt
---
Changes.rst | 3 +++
doc/openvpn.8 | 18 +++++++++++++++---
src/openvpn/crypto_backend.h | 6 ++++++
src/openvpn/init.c | 1 +
src/openvpn/options.c | 22 +++++++++++++++++++---
src/openvpn/options.h | 5 +++++
src/openvpn/ssl_common.h | 1 +
src/openvpn/ssl_verify.c | 15 +++++++++++++--
8 files changed, 63 insertions(+), 8 deletions(-)Changes.rst +3 -0
@@ -318,3 +318,6 @@ Version 2.4.1 ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage extension instead. Make sure your certificates carry these to be able to use ``--remote-cert-tls``. + - ``--verify-hash`` can now take an optional flag which changes the hashing + algorithm. It can be either SHA1 or SHA256. The default if not provided is + SHA1 to preserve backwards compatibility with existing configurations.
doc/openvpn.8 +15 -3
@@ -4694,15 +4694,27 @@ and Not available with PolarSSL. .\"********************************************************* .TP -.B \-\-verify\-hash hash -Specify SHA1 fingerprint for level-1 cert. The level-1 cert is the +.B \-\-verify\-hash hash [algo] +Specify SHA1 or SHA256 fingerprint for level-1 cert. The level-1 cert is the CA (or intermediate cert) that signs the leaf certificate, and is one removed from the leaf certificate in the direction of the root. When accepting a connection from a peer, the level-1 cert fingerprint must match .B hash or certificate verification will fail. Hash is specified -as XX:XX:... For example: AD:B0:95:D8:09:C8:36:45:12:A9:89:C8:90:09:CB:13:72:A6:AD:16 +as XX:XX:... For example: + +.nf +.ft 3 +.in +4 +AD:B0:95:D8:09:C8:36:45:12:A9:89:C8:90:09:CB:13:72:A6:AD:16 +.in -4 +.ft +.fi + +The +.B algo +flag can be either SHA1 or SHA256. If not provided, it defaults to SHA1. .\"********************************************************* .TP .B \-\-pkcs11\-cert\-private [0|1]...
src/openvpn/crypto_backend.h +6 -0
@@ -47,6 +47,12 @@/* Maximum HMAC digest size (bytes) */ #define OPENVPN_MAX_HMAC_SIZE 64 +/** Types referencing specific message digest hashing algorithms */ +typedef enum { +MD_SHA1 , +MD_SHA256 +}hash_algo_type ; +/** Struct used in cipher name translation table */ typedef struct { const char *openvpn_name; /**< Cipher name used by OpenVPN */
src/openvpn/init.c +1 -0
@@ -2619,6 +2619,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)memmove (to.remote_cert_ku, options->remote_cert_ku,sizeof (to.remote_cert_ku)); to.remote_cert_eku = options->remote_cert_eku; to.verify_hash = options->verify_hash; + to.verify_hash_algo = options->verify_hash_algo;#ifdef ENABLE_X509ALTUSERNAME to.x509_username_field = (char *) options->x509_username_field;#else
src/openvpn/options.c +19 -3
@@ -591,7 +591,8 @@ static const char usage_message[] ="--x509-username-field : Field in x509 certificate containing the username.\n" " Default is CN in the Subject field.\n"#endif -"--verify-hash : Specify SHA1 fingerprint for level-1 cert.\n" +"--verify-hash hash [algo] : Specify fingerprint for level-1 certificate.\n" +" Valid algo flags are SHA1 and SHA256. \n" #ifdef _WIN32"--cryptoapicert select-string : Load the certificate and private key from the\n" " Windows Certificate System Store.\n"
@@ -7687,10 +7688,25 @@ add_option(struct options *options, options->extra_certs_file_inline = p[2 ]; } } -else if (streq (p[0 ],"verify-hash" ) && p[1 ] && !p[2 ]) +else if (streq (p[0 ],"verify-hash" ) && p[1 ] && !p[3 ]) {VERIFY_PERMISSION (OPT_P_GENERAL ); - options->verify_hash =parse_hash_fingerprint (p[1 ],SHA_DIGEST_LENGTH , msglevel, &options->gc); + +if (!p[2 ] || (p[2 ] &&streq (p[2 ],"SHA1" ))) + { + options->verify_hash =parse_hash_fingerprint (p[1 ],SHA_DIGEST_LENGTH , msglevel, &options->gc); + options->verify_hash_algo =MD_SHA1 ; + } +else if (p[2 ] &&streq (p[2 ],"SHA256" )) + { + options->verify_hash =parse_hash_fingerprint (p[1 ],SHA256_DIGEST_LENGTH , msglevel, &options->gc); + options->verify_hash_algo =MD_SHA256 ; + } +else + { +msg (msglevel,"invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)" , p[2 ]); + goto err; + } }#ifdef ENABLE_CRYPTOAPI else if (streq(p[0], "cryptoapicert") && p[1] && !p[2])
src/openvpn/options.h +5 -0
@@ -42,6 +42,10 @@#include "comp.h" #include "pushlist.h" #include "clinat.h" +#ifdef ENABLE_CRYPTO +#include "crypto_backend.h" +#endif +/* * Maximum number of parameters associated with an option,
@@ -518,6 +522,7 @@ struct optionsunsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;uint8_t *verify_hash; +hash_algo_type verify_hash_algo;unsigned int ssl_flags;/* set to SSLF_x flags from ssl.h */ #ifdef ENABLE_PKCS11
src/openvpn/ssl_common.h +1 -0
@@ -271,6 +271,7 @@ struct tls_optionsunsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;uint8_t *verify_hash; +hash_algo_type verify_hash_algo;char *x509_username_field;/* allow openvpn config info to be
src/openvpn/ssl_verify.c +13 -2
@@ -718,8 +718,19 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep/* verify level 1 cert, i.e. the CA that signed our leaf cert */ if (cert_depth ==1 && opt->verify_hash) { -struct buffer sha1_hash =x509_get_sha1_fingerprint (cert, &gc); -if (memcmp (BPTR (&sha1_hash), opt->verify_hash,BLEN (&sha1_hash))) +struct buffer ca_hash; + +switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + } +if (memcmp_constant_time (BPTR (&ca_hash), opt->verify_hash,BLEN (&ca_hash))) {msg (D_TLS_ERRORS ,"TLS Error: level-1 certificate hash verification failed" ); goto cleanup;
Signature · 1 line
2.11.0
Hi,
Quoted 3 lines · David Sommerseth
This enhances --verify-hash with an optional algorithm flag. If not provided, it defaults to SHA1 to preserve backwards compatbilitity with existing configurations. The only valid flags are SHA1 and SHA256.
Feature-ACK.
Quoted 3 lines
In addition it moves the hash verification away from
memcmp()tomemcmp_constant_time(). And slightly it enhances the layout of the --verify-hash section in the man page.
Hashes are public, so there is no real need to use memcmp_constant_time() here. It doesn't hurt either though - so I won't complain further if you prefer to keep it ;-)
Quoted 185 lines
Signed-off-by David Sommerseth <[email protected]...>---
Simple test script
==================* Server:
# openvpn --dev tun --reneg-sec 60 --verb 4 \ --server 10.8.0.0 255.255.255.0 --topology subnet \ --ca sample/sample-keys/ca.crt \ --key sample/sample-keys/server.key \ --cert sample/sample-keys/server.crt \ --dh sample/sample-keys/dh2048.pem \ --cipher AES-256-CBC --auth SHA256 \ --verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:F9 SHA256* Client:
# openvpn --dev tun --verb 4 \ --remote 192.168.122.1 --explicit-exit-notify \ --client --ca sample/sample-keys/ca.crt \ --key sample/sample-keys/client.key \ --cert sample/sample-keys/client.crt \ --cipher AES-256-CBC --auth SHA256 \ --verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44Alternative --verify-hash variant which must succeed:
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44 SHA1These --verify-hash variants must fail:
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44 SHA256 --verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:F9 --verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:00 SHA256 --verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:00 SHA1 --verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:00 --verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:XXTo retrieve the SHA fingerprints in certificates:
$ openssl x509 -noout -fingerprint -in sample/sample-keys/ca.crt $ openssl x509 -noout -fingerprint -sha256 -in sample/sample-keys/ca.crt --- Changes.rst | 3 +++ doc/openvpn.8 | 18 +++++++++++++++--- src/openvpn/crypto_backend.h | 6 ++++++ src/openvpn/init.c | 1 + src/openvpn/options.c | 22 +++++++++++++++++++--- src/openvpn/options.h | 5 +++++ src/openvpn/ssl_common.h | 1 + src/openvpn/ssl_verify.c | 15 +++++++++++++-- 8 files changed, 63 insertions(+), 8 deletions(-)Changes.rst +3
-0@@ -318,3 +318,6 @@ Version 2.4.1 ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage extension instead. Make sure your certificates carry these to be able to use ``--remote-cert-tls``. + - ``--verify-hash`` can now take an optional flag which changes the hashing + algorithm. It can be either SHA1 or SHA256. The default if not provided is + SHA1 to preserve backwards compatibility with existing configurations.doc/openvpn.8 +15
-3@@ -4694,15 +4694,27 @@ and Not available with PolarSSL. .\"********************************************************* .TP -.B \-\-verify\-hash hash -Specify SHA1 fingerprint for level-1 cert. The level-1 cert is the +.B \-\-verify\-hash hash [algo] +Specify SHA1 or SHA256 fingerprint for level-1 cert. The level-1 cert is the CA (or intermediate cert) that signs the leaf certificate, and is one removed from the leaf certificate in the direction of the root. When accepting a connection from a peer, the level-1 cert fingerprint must match .B hash or certificate verification will fail. Hash is specified -as XX:XX:... For example: AD:B0:95:D8:09:C8:36:45:12:A9:89:C8:90:09:CB:13:72:A6:AD:16 +as XX:XX:... For example: + +.nf +.ft 3 +.in +4 +AD:B0:95:D8:09:C8:36:45:12:A9:89:C8:90:09:CB:13:72:A6:AD:16 +.in -4 +.ft +.fi + +The +.B algo +flag can be either SHA1 or SHA256. If not provided, it defaults to SHA1. .\"********************************************************* .TP .B \-\-pkcs11\-cert\-private [0|1]...src/openvpn/crypto_backend.h +6
-0@@ -47,6 +47,12 @@/* Maximum HMAC digest size (bytes) */ #define OPENVPN_MAX_HMAC_SIZE 64 +/** Types referencing specific message digest hashing algorithms */ +typedef enum { +MD_SHA1 , +MD_SHA256 +}hash_algo_type ; +/** Struct used in cipher name translation table */ typedef struct { const char *openvpn_name; /**< Cipher name used by OpenVPN */src/openvpn/init.c +1
-0@@ -2619,6 +2619,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)memmove (to.remote_cert_ku, options->remote_cert_ku,sizeof (to.remote_cert_ku)); to.remote_cert_eku = options->remote_cert_eku; to.verify_hash = options->verify_hash; + to.verify_hash_algo = options->verify_hash_algo;#ifdef ENABLE_X509ALTUSERNAME to.x509_username_field = (char *) options->x509_username_field;#else src/openvpn/options.c +19
-3@@ -591,7 +591,8 @@ static const char usage_message[] ="--x509-username-field : Field in x509 certificate containing the username.\n" " Default is CN in the Subject field.\n"#endif -"--verify-hash : Specify SHA1 fingerprint for level-1 cert.\n" +"--verify-hash hash [algo] : Specify fingerprint for level-1 certificate.\n" +" Valid algo flags are SHA1 and SHA256. \n" #ifdef _WIN32"--cryptoapicert select-string : Load the certificate and private key from the\n" " Windows Certificate System Store.\n" @@ -7687,10 +7688,25 @@ add_option(struct options *options, options->extra_certs_file_inline = p[2 ]; } } -else if (streq (p[0 ],"verify-hash" ) && p[1 ] && !p[2 ]) +else if (streq (p[0 ],"verify-hash" ) && p[1 ] && !p[3 ]) {VERIFY_PERMISSION (OPT_P_GENERAL ); - options->verify_hash =parse_hash_fingerprint (p[1 ],SHA_DIGEST_LENGTH , msglevel, &options->gc); + +if (!p[2 ] || (p[2 ] &&streq (p[2 ],"SHA1" ))) + { + options->verify_hash =parse_hash_fingerprint (p[1 ],SHA_DIGEST_LENGTH , msglevel, &options->gc); + options->verify_hash_algo =MD_SHA1 ; + } +else if (p[2 ] &&streq (p[2 ],"SHA256" )) + { + options->verify_hash =parse_hash_fingerprint (p[1 ],SHA256_DIGEST_LENGTH , msglevel, &options->gc); + options->verify_hash_algo =MD_SHA256 ; + } +else + { +msg (msglevel,"invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)" , p[2 ]); + goto err; + } }#ifdef ENABLE_CRYPTOAPI else if (streq(p[0], "cryptoapicert") && p[1] && !p[2])src/openvpn/options.h +4
-0@@ -42,6 +42,10 @@#include "comp.h" #include "pushlist.h" #include "clinat.h" +#ifdef ENABLE_CRYPTO +#include "crypto_backend.h" +#endif +/* * Maximum number of parameters associated with an option,
src/openvpn/options.h struct options · line 525
Quoted 4 lines
src/openvpn/options.h +1
-0@@ -518,6 +522,7 @@ struct optionsunsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;uint8_t *verify_hash; +hash_algo_type verify_hash_algo;
Wouldn't it be nicer to more tightly couple the hash and algo, by e.g. replacing verify_hash by a struct like:
struct openvpn_hash {
hash_algo_type type;
uint8_t[SHA256_DIGEST_LENGTH] hash;
}(But that's really nitpicking, so feel free to ignore.)
Quoted 27 lines
src/openvpn/options.h +0
-0unsigned int ssl_flags;/* set to SSLF_x flags from ssl.h */ #ifdef ENABLE_PKCS11 src/openvpn/ssl_common.h +1
-0@@ -271,6 +271,7 @@ struct tls_optionsunsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;uint8_t *verify_hash; +hash_algo_type verify_hash_algo;char *x509_username_field;/* allow openvpn config info to be src/openvpn/ssl_verify.c +2
-2@@ -718,8 +718,19 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep/* verify level 1 cert, i.e. the CA that signed our leaf cert */ if (cert_depth ==1 && opt->verify_hash) { -struct buffer sha1_hash =x509_get_sha1_fingerprint (cert, &gc); -if (memcmp (BPTR (&sha1_hash), opt->verify_hash,BLEN (&sha1_hash))) +struct buffer ca_hash; +
src/openvpn/ssl_verify.c verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep · line 732
Quoted 10 lines
src/openvpn/ssl_verify.c +10
-0+switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + }
This switch should have a default: case that fails, otherwise we might be reading from uninitialized memory. And you might want to consider initializing ca_hash to "{ 0 }". (But you still need to default: case, otherwise you'll be doing a 0-length memcmp()).
-Steffan
Quoted 8 lines · Steffan Karger
Quoted 3 lines · David Sommerseth
In addition it moves the hash verification away from
memcmp()tomemcmp_constant_time(). And slightly it enhances the layout of the --verify-hash section in the man page.Hashes are public, so there is no real need to use
memcmp_constant_time()here. It doesn't hurt either though - so I won't complain further if you prefer to keep it ;-)
Hrmpf, on sending I realize this is not the right argument.
Public doesn't matter (MACs are public too), what matters is that we are not comparing attacker input (a MAC), but the calculated hash of attacker input (the hash of the supplied cert). As long as the hash function is preimage resistant, we don't have to do constant time comparison. (And SHA1/SHA256 and even MD5 are pre-image resistant.)
Hashing the inputs and comparing the hashes actually is one of the options to do a constant time memcmp :-)
-Steffan
Quoted 10 lines · Steffan Karger
src/openvpn/ssl_verify.c verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep · line 732
Quoted 10 lines
src/openvpn/ssl_verify.c +10
-0+switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + }
Quoted 4 lines
This switch should have a default: case that fails, otherwise we might be reading from uninitialized memory. And you might want to consider initializing
ca_hashto "{ 0 }". (But you still need to default: case, otherwise you'll be doing a 0-lengthmemcmp()).
*grmbl* I was sure the enumerated type would make the compiler complain, but I must have mixed this with some C++ stuff I read not too long ago. But my stupid test allows assigning an enumerated type to any invalid value as well.
I'll double check again tomorrow (when head and mind is fresh) and submit a v2.
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc
Quoted 19 lines · David Sommerseth
Quoted 10 lines · Steffan Karger
src/openvpn/ssl_verify.c verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep · line 732
Quoted 10 lines
src/openvpn/ssl_verify.c +10
-0+switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + }Quoted 4 lines
This switch should have a default: case that fails, otherwise we might be reading from uninitialized memory. And you might want to consider initializing
ca_hashto "{ 0 }". (But you still need to default: case, otherwise you'll be doing a 0-lengthmemcmp()).
Quoted 5 lines
*grmbl* I was sure the enumerated type would make the compiler complain, but I must have mixed this with some C++ stuff I read not too long ago. But my stupid test allows assigning an enumerated type to any invalid value as well.
You are right.
The compiler would have complained if you had left out any value from the enum set. But the enum set is made of those two values only, hence a default case is not plausible unless you assign verify_hash_algo with a value out of the enum set (but then you should have a compiler warning on the assignment).
Cheers,
Quoted 28 lines · Antonio Quartulli
Quoted 20 lines · David Sommerseth
Quoted 10 lines · Steffan Karger
src/openvpn/ssl_verify.c verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep · line 732
Quoted 10 lines
src/openvpn/ssl_verify.c +10
-0+switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + }Quoted 4 lines
This switch should have a default: case that fails, otherwise we might be reading from uninitialized memory. And you might want to consider initializing
ca_hashto "{ 0 }". (But you still need to default: case, otherwise you'll be doing a 0-lengthmemcmp()).Quoted 5 lines
*grmbl* I was sure the enumerated type would make the compiler complain, but I must have mixed this with some C++ stuff I read not too long ago. But my stupid test allows assigning an enumerated type to any invalid value as well.
Quoted 6 lines
You are right.
The compiler would have complained if you had left out any value from the enum set. But the enum set is made of those two values only, hence a default case is not plausible unless you assignverify_hash_algowith a value out of the enum set (but then you should have a compiler warning on the assignment).
I forgot: the good point of *not* having a default is that when you will add a new enum value for that particular enum type, the compiler will throw warnings for every switch block you have, informing you that now they need to be updated. Having a default will leave you with manually check the various code spots.
Cheers,
--
Antonio Quartulli
Hi,
Quoted 34 lines · Antonio Quartulli
Quoted 27 lines · Antonio Quartulli
Quoted 19 lines · David Sommerseth
Quoted 10 lines · Steffan Karger
src/openvpn/ssl_verify.c verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep · line 732
Quoted 10 lines
src/openvpn/ssl_verify.c +10
-0+switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + }Quoted 4 lines
This switch should have a default: case that fails, otherwise we might be reading from uninitialized memory. And you might want to consider initializing
ca_hashto "{ 0 }". (But you still need to default: case, otherwise you'll be doing a 0-lengthmemcmp()).Quoted 5 lines
*grmbl* I was sure the enumerated type would make the compiler complain, but I must have mixed this with some C++ stuff I read not too long ago. But my stupid test allows assigning an enumerated type to any invalid value as well.
Quoted 6 lines
You are right.
The compiler would have complained if you had left out any value from the enum set. But the enum set is made of those two values only, hence a default case is not plausible unless you assignverify_hash_algowith a value out of the enum set (but then you should have a compiler warning on the assignment).
Quoted 4 lines
I forgot: the good point of *not* having a default is that when you will add a new enum value for that particular enum type, the compiler will throw warnings for every switch block you have, informing you that now they need to be updated. Having a default will leave you with manually check the various code spots.
This only holds when compiling with -Wall, which we do not do by default (because our current code would emit quite some warnings).
If we would fix our code and compile with -Wall -Werror, I'd agree. But as long as we don't, I prefer a defensive code style where I can exclude undefined behaviour just by looking at the code.
-Steffan
Quoted 45 lines · Steffan Karger
Hi,
Quoted 34 lines · Antonio Quartulli
Quoted 27 lines · Antonio Quartulli
Quoted 19 lines · David Sommerseth
Quoted 10 lines · Steffan Karger
src/openvpn/ssl_verify.c verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep · line 732
Quoted 10 lines
src/openvpn/ssl_verify.c +10
-0+switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + }Quoted 4 lines
This switch should have a default: case that fails, otherwise we might be reading from uninitialized memory. And you might want to consider initializing
ca_hashto "{ 0 }". (But you still need to default: case, otherwise you'll be doing a 0-lengthmemcmp()).Quoted 5 lines
*grmbl* I was sure the enumerated type would make the compiler complain, but I must have mixed this with some C++ stuff I read not too long ago. But my stupid test allows assigning an enumerated type to any invalid value as well.
Quoted 6 lines
You are right.
The compiler would have complained if you had left out any value from the enum set. But the enum set is made of those two values only, hence a default case is not plausible unless you assignverify_hash_algowith a value out of the enum set (but then you should have a compiler warning on the assignment).Quoted 4 lines
I forgot: the good point of *not* having a default is that when you will add a new enum value for that particular enum type, the compiler will throw warnings for every switch block you have, informing you that now they need to be updated. Having a default will leave you with manually check the various code spots.
Quoted 7 lines
This only holds when compiling with -Wall, which we do not do by default (because our current code would emit quite some warnings).
If we would fix our code and compile with -Wall -Werror, I'd agree. But as long as we don't, I prefer a defensive code style where I can exclude undefined behaviour just by looking at the code.
Agreed. This sounds like a call for a cleanup mission :]
Cheers,
--
Antonio Quartulli
Quoted 7 lines · Antonio Quartulli
Quoted 7 lines · Steffan Karger
Hi,
Quoted 1 line · Antonio Quartulli
[...snip...]
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
I forgot: the good point of *not* having a default is that when you will add a new enum value for that particular enum type, the compiler will throw warnings for every switch block you have, informing you that now they need to be updated. Having a default will leave you with manually check the various code spots.
This only holds when compiling with -Wall, which we do not do by default (because our current code would emit quite some warnings).
I actually do my local builds with -Wall, as well as the Fedora/Fedora EPEL builds do that too. It's not that terrifying as it once was. Well, currently most of the errors are in ntlm.c; the rest are just a few handful ones, where at least one of them already have a patch awaiting review on the ML (lz4 API update).
Quoted 6 lines
Quoted 4 lines
If we would fix our code and compile with -Wall -Werror, I'd agree. But as long as we don't, I prefer a defensive code style where I can exclude undefined behaviour just by looking at the code.
Agreed. This sounds like a call for a cleanup mission :]
I presume you meant this on a generic level, not this patch explicitly, right? If so, I don't need to worry about a v3, as the v2 of this patch is already in this mail thread.
Signature · 4 lines
kind regards, David Sommerseth OpenVPN Technologies, Inc
Quoted 10 lines · David Sommerseth
Quoted 2 lines · Antonio Quartulli
Agreed. This sounds like a call for a cleanup mission :]
I presume you meant this on a generic level, not this patch explicitly, right? If so, I don't need to worry about a v3, as the v2 of this patch is already in this mail thread.
Yeah, I meant in general. For this patch we don’t need to worry :)
Cheers,
--
Antonio Quartulli
PATCH v2 1/1 crypto: Enable SHA256 fingerprint checking in --verify-hash
In reply to David SommersethThis enhances --verify-hash with an optional algorithm flag. If not provided, it defaults to SHA1 to preserve backwards compatbilitity with existing configurations. The only valid flags are SHA1 and SHA256.
In addition enhance the layout of the --verify-hash section in the man page.
---
v2 - Ensure ca_hash is initialized to a known value
- Add a default target to the switch()
- Move back to memcmp(), as memcmp_constant_time() isn't really needed here; the result of MD hashes have defined lengths regardless of the certificates an attacker may decide to use.
Simple test script
==================
* Server:
# openvpn --dev tun --reneg-sec 60 --verb 4 \
--server 10.8.0.0 255.255.255.0 --topology subnet \
--ca sample/sample-keys/ca.crt \
--key sample/sample-keys/server.key \
--cert sample/sample-keys/server.crt \
--dh sample/sample-keys/dh2048.pem \
--cipher AES-256-CBC --auth SHA256 \
--verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:F8 SHA256* Client:
# openvpn --dev tun --verb 4 \
--remote 192.168.122.1 --explicit-exit-notify \
--client --ca sample/sample-keys/ca.crt \
--key sample/sample-keys/client.key \
--cert sample/sample-keys/client.crt \
--cipher AES-256-CBC --auth SHA256 \
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44Alternative --verify-hash variant which must succeed:
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44 SHA1
These --verify-hash variants must fail:
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44 SHA256
--verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:F9
--verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:00 SHA256
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:00 SHA1
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:00
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:XXTo retrieve the SHA fingerprints in certificates:
$ openssl x509 -noout -fingerprint -in sample/sample-keys/ca.crt
$ openssl x509 -noout -fingerprint -sha256 -in sample/sample-keys/ca.crt
---
Changes.rst | 3 +++
doc/openvpn.8 | 18 +++++++++++++++---
src/openvpn/crypto_backend.h | 6 ++++++
src/openvpn/init.c | 1 +
src/openvpn/options.c | 22 +++++++++++++++++++---
src/openvpn/options.h | 5 +++++
src/openvpn/ssl_common.h | 1 +
src/openvpn/ssl_verify.c | 27 +++++++++++++++++++++++++--
8 files changed, 75 insertions(+), 8 deletions(-)Changes.rst +3 -0
@@ -318,3 +318,6 @@ Version 2.4.1 ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage extension instead. Make sure your certificates carry these to be able to use ``--remote-cert-tls``. + - ``--verify-hash`` can now take an optional flag which changes the hashing + algorithm. It can be either SHA1 or SHA256. The default if not provided is + SHA1 to preserve backwards compatibility with existing configurations.
doc/openvpn.8 +15 -3
@@ -4694,15 +4694,27 @@ and Not available with PolarSSL. .\"********************************************************* .TP -.B \-\-verify\-hash hash -Specify SHA1 fingerprint for level-1 cert. The level-1 cert is the +.B \-\-verify\-hash hash [algo] +Specify SHA1 or SHA256 fingerprint for level-1 cert. The level-1 cert is the CA (or intermediate cert) that signs the leaf certificate, and is one removed from the leaf certificate in the direction of the root. When accepting a connection from a peer, the level-1 cert fingerprint must match .B hash or certificate verification will fail. Hash is specified -as XX:XX:... For example: AD:B0:95:D8:09:C8:36:45:12:A9:89:C8:90:09:CB:13:72:A6:AD:16 +as XX:XX:... For example: + +.nf +.ft 3 +.in +4 +AD:B0:95:D8:09:C8:36:45:12:A9:89:C8:90:09:CB:13:72:A6:AD:16 +.in -4 +.ft +.fi + +The +.B algo +flag can be either SHA1 or SHA256. If not provided, it defaults to SHA1. .\"********************************************************* .TP .B \-\-pkcs11\-cert\-private [0|1]...
src/openvpn/crypto_backend.h +6 -0
@@ -47,6 +47,12 @@/* Maximum HMAC digest size (bytes) */ #define OPENVPN_MAX_HMAC_SIZE 64 +/** Types referencing specific message digest hashing algorithms */ +typedef enum { +MD_SHA1 , +MD_SHA256 +}hash_algo_type ; +/** Struct used in cipher name translation table */ typedef struct { const char *openvpn_name; /**< Cipher name used by OpenVPN */
src/openvpn/init.c +1 -0
@@ -2619,6 +2619,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)memmove (to.remote_cert_ku, options->remote_cert_ku,sizeof (to.remote_cert_ku)); to.remote_cert_eku = options->remote_cert_eku; to.verify_hash = options->verify_hash; + to.verify_hash_algo = options->verify_hash_algo;#ifdef ENABLE_X509ALTUSERNAME to.x509_username_field = (char *) options->x509_username_field;#else
src/openvpn/options.c +19 -3
@@ -591,7 +591,8 @@ static const char usage_message[] ="--x509-username-field : Field in x509 certificate containing the username.\n" " Default is CN in the Subject field.\n"#endif -"--verify-hash : Specify SHA1 fingerprint for level-1 cert.\n" +"--verify-hash hash [algo] : Specify fingerprint for level-1 certificate.\n" +" Valid algo flags are SHA1 and SHA256. \n" #ifdef _WIN32"--cryptoapicert select-string : Load the certificate and private key from the\n" " Windows Certificate System Store.\n"
@@ -7687,10 +7688,25 @@ add_option(struct options *options, options->extra_certs_file_inline = p[2 ]; } } -else if (streq (p[0 ],"verify-hash" ) && p[1 ] && !p[2 ]) +else if (streq (p[0 ],"verify-hash" ) && p[1 ] && !p[3 ]) {VERIFY_PERMISSION (OPT_P_GENERAL ); - options->verify_hash =parse_hash_fingerprint (p[1 ],SHA_DIGEST_LENGTH , msglevel, &options->gc); + +if (!p[2 ] || (p[2 ] &&streq (p[2 ],"SHA1" ))) + { + options->verify_hash =parse_hash_fingerprint (p[1 ],SHA_DIGEST_LENGTH , msglevel, &options->gc); + options->verify_hash_algo =MD_SHA1 ; + } +else if (p[2 ] &&streq (p[2 ],"SHA256" )) + { + options->verify_hash =parse_hash_fingerprint (p[1 ],SHA256_DIGEST_LENGTH , msglevel, &options->gc); + options->verify_hash_algo =MD_SHA256 ; + } +else + { +msg (msglevel,"invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)" , p[2 ]); + goto err; + } }#ifdef ENABLE_CRYPTOAPI else if (streq(p[0], "cryptoapicert") && p[1] && !p[2])
src/openvpn/options.h +5 -0
@@ -42,6 +42,10 @@#include "comp.h" #include "pushlist.h" #include "clinat.h" +#ifdef ENABLE_CRYPTO +#include "crypto_backend.h" +#endif +/* * Maximum number of parameters associated with an option,
@@ -518,6 +522,7 @@ struct optionsunsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;uint8_t *verify_hash; +hash_algo_type verify_hash_algo;unsigned int ssl_flags;/* set to SSLF_x flags from ssl.h */ #ifdef ENABLE_PKCS11
src/openvpn/ssl_common.h +1 -0
@@ -271,6 +271,7 @@ struct tls_optionsunsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;uint8_t *verify_hash; +hash_algo_type verify_hash_algo;char *x509_username_field;/* allow openvpn config info to be
src/openvpn/ssl_verify.c +25 -2
@@ -718,8 +718,31 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep/* verify level 1 cert, i.e. the CA that signed our leaf cert */ if (cert_depth ==1 && opt->verify_hash) { -struct buffer sha1_hash =x509_get_sha1_fingerprint (cert, &gc); -if (memcmp (BPTR (&sha1_hash), opt->verify_hash,BLEN (&sha1_hash))) +struct buffer ca_hash = {0 }; + +switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + +default : +/* This should normally not happen at all; the algorithm used +* is parsed by add_option() [options.c] and set to a predefined +* value in an enumerated type. So if this unlikely scenario +* happens, consider this a failure +*/ +msg (M_WARN ,"Unexpected invalid algorithm used with " +"--verify-hash (%i)" , opt->verify_hash_algo); + ret =FAILURE ; + goto cleanup; + } + +if (memcmp (BPTR (&ca_hash), opt->verify_hash,BLEN (&ca_hash))) {msg (D_TLS_ERRORS ,"TLS Error: level-1 certificate hash verification failed" ); goto cleanup;
Signature · 1 line
2.11.0
Hi,
Quoted 245 lines · David Sommerseth
This enhances --verify-hash with an optional algorithm flag. If not provided, it defaults to SHA1 to preserve backwards compatbilitity with existing configurations. The only valid flags are SHA1 and SHA256.
In addition enhance the layout of the --verify-hash section in the man page.
Signed-off-by David Sommerseth <[email protected]...>---
v2 - Ensure
ca_hashis initialized to a known value
- Add a default target to theswitch()
- Move back tomemcmp(), asmemcmp_constant_time()isn't really needed here; the result of MD hashes have defined lengths regardless of the certificates an attacker may decide to use.Simple test script
==================* Server:
# openvpn --dev tun --reneg-sec 60 --verb 4 \ --server 10.8.0.0 255.255.255.0 --topology subnet \ --ca sample/sample-keys/ca.crt \ --key sample/sample-keys/server.key \ --cert sample/sample-keys/server.crt \ --dh sample/sample-keys/dh2048.pem \ --cipher AES-256-CBC --auth SHA256 \ --verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:F8 SHA256* Client:
# openvpn --dev tun --verb 4 \ --remote 192.168.122.1 --explicit-exit-notify \ --client --ca sample/sample-keys/ca.crt \ --key sample/sample-keys/client.key \ --cert sample/sample-keys/client.crt \ --cipher AES-256-CBC --auth SHA256 \ --verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44Alternative --verify-hash variant which must succeed:
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44 SHA1These --verify-hash variants must fail:
--verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:44 SHA256 --verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:F9 --verify-hash 57:4D:B3:49:7E:FB:9E:FD:DC:BC:A4:ED:BC:B3:C2:8D:C3:D7:2A:E3:0F:6F:76:57:F0:BB:F4:90:56:1C:DC:00 SHA256 --verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:00 SHA1 --verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:00 --verify-hash 06:30:DA:EA:77:41:5B:D6:8C:C5:CC:CA:F5:D7:91:87:FA:6D:89:XXTo retrieve the SHA fingerprints in certificates:
$ openssl x509 -noout -fingerprint -in sample/sample-keys/ca.crt $ openssl x509 -noout -fingerprint -sha256 -in sample/sample-keys/ca.crt --- Changes.rst | 3 +++ doc/openvpn.8 | 18 +++++++++++++++--- src/openvpn/crypto_backend.h | 6 ++++++ src/openvpn/init.c | 1 + src/openvpn/options.c | 22 +++++++++++++++++++--- src/openvpn/options.h | 5 +++++ src/openvpn/ssl_common.h | 1 + src/openvpn/ssl_verify.c | 27 +++++++++++++++++++++++++-- 8 files changed, 75 insertions(+), 8 deletions(-)Changes.rst +3
-0@@ -318,3 +318,6 @@ Version 2.4.1 ``--remote-cert-tls`` uses the far more common keyUsage and extendedKeyUsage extension instead. Make sure your certificates carry these to be able to use ``--remote-cert-tls``. + - ``--verify-hash`` can now take an optional flag which changes the hashing + algorithm. It can be either SHA1 or SHA256. The default if not provided is + SHA1 to preserve backwards compatibility with existing configurations.doc/openvpn.8 +15
-3@@ -4694,15 +4694,27 @@ and Not available with PolarSSL. .\"********************************************************* .TP -.B \-\-verify\-hash hash -Specify SHA1 fingerprint for level-1 cert. The level-1 cert is the +.B \-\-verify\-hash hash [algo] +Specify SHA1 or SHA256 fingerprint for level-1 cert. The level-1 cert is the CA (or intermediate cert) that signs the leaf certificate, and is one removed from the leaf certificate in the direction of the root. When accepting a connection from a peer, the level-1 cert fingerprint must match .B hash or certificate verification will fail. Hash is specified -as XX:XX:... For example: AD:B0:95:D8:09:C8:36:45:12:A9:89:C8:90:09:CB:13:72:A6:AD:16 +as XX:XX:... For example: + +.nf +.ft 3 +.in +4 +AD:B0:95:D8:09:C8:36:45:12:A9:89:C8:90:09:CB:13:72:A6:AD:16 +.in -4 +.ft +.fi + +The +.B algo +flag can be either SHA1 or SHA256. If not provided, it defaults to SHA1. .\"********************************************************* .TP .B \-\-pkcs11\-cert\-private [0|1]...src/openvpn/crypto_backend.h +6
-0@@ -47,6 +47,12 @@/* Maximum HMAC digest size (bytes) */ #define OPENVPN_MAX_HMAC_SIZE 64 +/** Types referencing specific message digest hashing algorithms */ +typedef enum { +MD_SHA1 , +MD_SHA256 +}hash_algo_type ; +/** Struct used in cipher name translation table */ typedef struct { const char *openvpn_name; /**< Cipher name used by OpenVPN */src/openvpn/init.c +1
-0@@ -2619,6 +2619,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)memmove (to.remote_cert_ku, options->remote_cert_ku,sizeof (to.remote_cert_ku)); to.remote_cert_eku = options->remote_cert_eku; to.verify_hash = options->verify_hash; + to.verify_hash_algo = options->verify_hash_algo;#ifdef ENABLE_X509ALTUSERNAME to.x509_username_field = (char *) options->x509_username_field;#else src/openvpn/options.c +19
-3@@ -591,7 +591,8 @@ static const char usage_message[] ="--x509-username-field : Field in x509 certificate containing the username.\n" " Default is CN in the Subject field.\n"#endif -"--verify-hash : Specify SHA1 fingerprint for level-1 cert.\n" +"--verify-hash hash [algo] : Specify fingerprint for level-1 certificate.\n" +" Valid algo flags are SHA1 and SHA256. \n" #ifdef _WIN32"--cryptoapicert select-string : Load the certificate and private key from the\n" " Windows Certificate System Store.\n" @@ -7687,10 +7688,25 @@ add_option(struct options *options, options->extra_certs_file_inline = p[2 ]; } } -else if (streq (p[0 ],"verify-hash" ) && p[1 ] && !p[2 ]) +else if (streq (p[0 ],"verify-hash" ) && p[1 ] && !p[3 ]) {VERIFY_PERMISSION (OPT_P_GENERAL ); - options->verify_hash =parse_hash_fingerprint (p[1 ],SHA_DIGEST_LENGTH , msglevel, &options->gc); + +if (!p[2 ] || (p[2 ] &&streq (p[2 ],"SHA1" ))) + { + options->verify_hash =parse_hash_fingerprint (p[1 ],SHA_DIGEST_LENGTH , msglevel, &options->gc); + options->verify_hash_algo =MD_SHA1 ; + } +else if (p[2 ] &&streq (p[2 ],"SHA256" )) + { + options->verify_hash =parse_hash_fingerprint (p[1 ],SHA256_DIGEST_LENGTH , msglevel, &options->gc); + options->verify_hash_algo =MD_SHA256 ; + } +else + { +msg (msglevel,"invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)" , p[2 ]); + goto err; + } }#ifdef ENABLE_CRYPTOAPI else if (streq(p[0], "cryptoapicert") && p[1] && !p[2])src/openvpn/options.h +5
-0@@ -42,6 +42,10 @@#include "comp.h" #include "pushlist.h" #include "clinat.h" +#ifdef ENABLE_CRYPTO +#include "crypto_backend.h" +#endif +/* * Maximum number of parameters associated with an option, @@ -518,6 +522,7 @@ struct optionsunsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;uint8_t *verify_hash; +hash_algo_type verify_hash_algo;unsigned int ssl_flags;/* set to SSLF_x flags from ssl.h */ #ifdef ENABLE_PKCS11 src/openvpn/ssl_common.h +1
-0@@ -271,6 +271,7 @@ struct tls_optionsunsigned remote_cert_ku[MAX_PARMS ];const char *remote_cert_eku;uint8_t *verify_hash; +hash_algo_type verify_hash_algo;char *x509_username_field;/* allow openvpn config info to be src/openvpn/ssl_verify.c +18
-2@@ -718,8 +718,31 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep/* verify level 1 cert, i.e. the CA that signed our leaf cert */ if (cert_depth ==1 && opt->verify_hash) { -struct buffer sha1_hash =x509_get_sha1_fingerprint (cert, &gc); -if (memcmp (BPTR (&sha1_hash), opt->verify_hash,BLEN (&sha1_hash))) +struct buffer ca_hash = {0 }; + +switch (opt->verify_hash_algo) + { +case MD_SHA1 : + ca_hash =x509_get_sha1_fingerprint (cert, &gc); +break ; + +case MD_SHA256 : + ca_hash =x509_get_sha256_fingerprint (cert, &gc); +break ; + +default : +/* This should normally not happen at all; the algorithm used +* is parsed by add_option() [options.c] and set to a predefined +* value in an enumerated type. So if this unlikely scenario +* happens, consider this a failure +*/
src/openvpn/ssl_verify.c verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep · line 748
Quoted 10 lines
src/openvpn/ssl_verify.c +7
-0+msg (M_WARN ,"Unexpected invalid algorithm used with " +"--verify-hash (%i)" , opt->verify_hash_algo); + ret =FAILURE ; + goto cleanup; + } + +if (memcmp (BPTR (&ca_hash), opt->verify_hash,BLEN (&ca_hash))) {msg (D_TLS_ERRORS ,"TLS Error: level-1 certificate hash verification failed" ); goto cleanup;
ACK
The Changes.rst bit will need some adjustment because of the 2.4.2 release but I you'll manage that on-the-fly.
-Steffan
Your patch has been applied to the master and release/2.4 branch.
"Changes.rst" changes adjusted - this is no longer "new in 2.4.1" :-)
commit 2193d7c08484d56ed07ba2e649abc2d08adcb245 (master) commit 5d5437710c282b1a60a892d1910160027a81db92 (release/2.4)
Author: David Sommerseth Date: Thu May 4 22:42:01 2017 +0200
crypto: Enable SHA256 fingerprint checking in --verify-hash
Signed-off-by: David Sommerseth <[email protected]...>
Acked-by: Steffan Karger <[email protected]...>
Message-Id: <[email protected]...>
URL: https://www.mail-archive.com/[email protected]/msg14538.html
Signed-off-by: Gert Doering <[email protected]...>--
kind regards,
Gert Doering