| Message ID | 20200908154157.13809-2-arne@rfc2549.org |
|---|---|
| State | Superseded |
| Delegated to: | Antonio Quartulli |
| Headers | show |
| Series | Allow setting up OpenVPN in TLS mode without CA | expand |
Hi, This patch conflicts a bit with current master, but can be applied using "git am -3". Intro: it's not easy to understand what "verify-hash" was really meant for, but I am happy to see it being restructured to actually become useful :-) On 08/09/2020 17:41, Arne Schwabe wrote: > For a new syntax introduced now it does not make much sense to support > deprecated and old hashes, so support only SHA-256. > > Also give a warning about SHA1 hash being deprecated to verify certificates > as it is now "industry standard". I would reword the commit as follows: --- This patch introduces support for verify-hash inlining. When inlined, this options now allows to specify multiple fingerprints, one per line. Since this is a new syntax, there is no backwards compatibility to take care of, therefore we can drop support for SHA1. Inlined fingerprints are assumed be to SHA-256 only. Also print a warning about SHA1 hash being deprecated to verify certificates as it is not "industry standard" anymore. --- More comments below: > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> > --- > doc/man-sections/inline-files.rst | 4 +- > doc/man-sections/tls-options.rst | 10 ++++ > src/openvpn/options.c | 91 +++++++++++++++++++++++++++---- > src/openvpn/options.h | 11 +++- > src/openvpn/ssl_common.h | 2 +- > src/openvpn/ssl_verify.c | 17 +++++- > 6 files changed, 119 insertions(+), 16 deletions(-) > > diff --git a/doc/man-sections/inline-files.rst b/doc/man-sections/inline-files.rst > index 819bd3c8..303bb3c8 100644 > --- a/doc/man-sections/inline-files.rst > +++ b/doc/man-sections/inline-files.rst > @@ -4,8 +4,8 @@ INLINE FILE SUPPORT > OpenVPN allows including files in the main configuration for the ``--ca``, > ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``, > ``--secret``, ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``, > -``--auth-gen-token-secret``, ``--tls-crypt`` and ``--tls-crypt-v2`` > -options. > +``--auth-gen-token-secret``, ``--tls-crypt``, ``--tls-crypt-v2`` and > +``--verify-hash`` options. > > Each inline file started by the line ``<option>`` and ended by the line > ``</option>`` > diff --git a/doc/man-sections/tls-options.rst b/doc/man-sections/tls-options.rst > index 8c2db7cd..52d4137e 100644 > --- a/doc/man-sections/tls-options.rst > +++ b/doc/man-sections/tls-options.rst > @@ -579,6 +579,16 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa > The ``algo`` flag can be either :code:`SHA1` or :code:`SHA256`. If not > provided, it defaults to :code:`SHA1`. > > + This option can also be inlined > + :: > + > + <verify-hash> > + 00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff > + 11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00 > + </verify-hash> > + > +If the option is inlined, ``algo`` is always :code:`SHA256`. > + > --verify-x509-name args > Accept connections only if a host's X.509 name is equal to **name.** The > remote host must also pass all other tests of verification. > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index 8bf82c57..068f3e75 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -1082,12 +1082,24 @@ string_substitute(const char *src, int from, int to, struct gc_arena *gc) > return ret; > } > > -static uint8_t * > +/** > + * Parses a hexstring and checks if the string has the correct length. Return > + * a verify_hash_list of the parsed string. "a verify_hash_list object containing the parsed string." > + * > + * @param str String to check/parse > + * @param nbytes Number of bytes expected in the hexstr (e.g. 20 for SHA1) > + * @param msglevel message level to use when printing warnings/errors > + * @param gc The returned string will be allocated in this gc. returned string -> returned object We never put the period at the end of doxygen strings. it should be removed > + */ > +static struct verify_hash_list * > parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct gc_arena *gc) > { > int i; > const char *cp = str; > - uint8_t *ret = (uint8_t *) gc_malloc(nbytes, true, gc); > + > + struct verify_hash_list *ret; > + ALLOC_OBJ_CLEAR_GC(ret, struct verify_hash_list, gc); > + > char term = 1; > int byte; > char bs[3]; > @@ -1106,7 +1118,7 @@ parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct gc_aren > { > msg(msglevel, "format error in hash fingerprint hex byte: %s", str); > } > - ret[i] = (uint8_t)byte; > + ret->hash[i] = (uint8_t)byte; > term = *cp++; > if (term != ':' && term != 0) > { > @@ -1120,10 +1132,54 @@ parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct gc_aren > if (term != 0 || i != nbytes-1) > { > msg(msglevel, "hash fingerprint is different length than expected (%d bytes): %s", nbytes, str); > + return NULL; > } > return ret; > } > > +/** > + * Parses a string consisting of multiple lines of hexstrings and checks if each > + * string has the correct length. Lines that are empty are ignored. Returns > + * a verify_hash_list. a linked list of (possibly) multiple verify_hash_list objects. > + * > + * @param str String to check/parse > + * @param nbytes Number of bytes expected in the hexstr (e.g. 20 for SHA1) in the hexstr -> in each hexstring > + * @param msglevel message level to use when printing warnings/errors > + * @param gc The returned string will be allocated in this gc. returned string -> returned list no dot at the end. > + */ > +static struct verify_hash_list * > +parse_hash_fingerprint_multiline(const char *str, int nbytes, int msglevel, > + struct gc_arena *gc) > +{ > + struct gc_arena gc_temp = gc_new(); > + char *lines = string_alloc(str, &gc_temp); > + > + struct verify_hash_list *ret = NULL; > + > + const char *line; > + while ((line = strsep(&lines, "\n"))) > + { > + /* skip empty lines */ > + if (strlen(line) == 0) > + { > + continue; > + } > + > + struct verify_hash_list *hash = parse_hash_fingerprint(line, nbytes, > + msglevel, gc); > + > + if (!hash) > + { > + return NULL; I believe we are leaking gc_temp here. It is allocated on the stack of this function, therefore it should be free'd before the return. > + } > + > + hash->next = ret; > + ret = hash; > + } > + gc_free(&gc_temp); > + > + return ret; > +} > #ifdef _WIN32 > > #ifndef ENABLE_SMALL > @@ -8138,22 +8194,37 @@ add_option(struct options *options, > } > else if (streq(p[0], "verify-hash") && p[1] && !p[3]) > { > - VERIFY_PERMISSION(OPT_P_GENERAL); > + VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INLINE); > + options->verify_hash_algo = MD_SHA256; > + > + int digest_len = SHA256_DIGEST_LENGTH; > > if (!p[2] || (p[2] && streq(p[2], "SHA1"))) This condition has to be modified. p[2] is NULL when this option is inlined, therefore this condition is evaluated to true in that case. Maybe you should change it to: if (!is_inline && (...)) ? I tested and I indeed get the following with an inlined verify-hash: DEPRECATED FEATURE: Usage of SHA1 fingerprints for verify-hash is deprecated. You should switch to the SHA256. > { > - options->verify_hash = parse_hash_fingerprint(p[1], SHA_DIGEST_LENGTH, msglevel, &options->gc); > options->verify_hash_algo = MD_SHA1; > + msg(M_WARN, "DEPRECATED FEATURE: Usage of SHA1 fingerprints for " > + "verify-hash is deprecated. You should switch to the " > + "SHA256."); > + options->verify_hash_algo = MD_SHA1; > + digest_len = SHA_DIGEST_LENGTH; > } > - else if (p[2] && streq(p[2], "SHA256")) > + 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; > + msg(msglevel, "invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)", p[2]); > + goto err; > + } > + > + if (is_inline) > + { > + /* We do not have a inline flag in options struct as it is either > + * a one line or multiline string */ I am not sure what this comment is telling us. Is it trying to explain some decision about how to call the following function? If so, which decision? To me the code makes sense also without the comment. > + options->verify_hash = parse_hash_fingerprint_multiline(p[1], digest_len, msglevel, > + &options->gc); > } > else > { > - msg(msglevel, "invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)", p[2]); > - goto err; > + options->verify_hash = parse_hash_fingerprint(p[1], digest_len, msglevel, > + &options->gc); > } > } > #ifdef ENABLE_CRYPTOAPI > diff --git a/src/openvpn/options.h b/src/openvpn/options.h > index 877e9396..c0dbbd8a 100644 > --- a/src/openvpn/options.h > +++ b/src/openvpn/options.h > @@ -191,6 +191,15 @@ enum genkey_type { > GENKEY_AUTH_TOKEN > }; > > +struct verify_hash_list > +{ > + /* Currently we only support SHA256 for longer lists, for a one item > + * list with SHA1 we ignore the 12 byte memory wasted */ This comment explains how we are solving something, without telling what that something is. My guess is that you are referring to SHA1 having 20bytes long hash, while for SHA256 it is 32bytes long. Is that right? Maybe it could reworded a little bit, because these numbers are not obvious to everybody. > + uint8_t hash[32]; > + struct verify_hash_list *next; > + no need for an empty line here. > +}; > + > /* Command line options */ > struct options > { > @@ -553,7 +562,7 @@ struct options > int ns_cert_type; /* set to 0, NS_CERT_CHECK_SERVER, or NS_CERT_CHECK_CLIENT */ > unsigned remote_cert_ku[MAX_PARMS]; > const char *remote_cert_eku; > - uint8_t *verify_hash; > + struct verify_hash_list *verify_hash; > hash_algo_type verify_hash_algo; > unsigned int ssl_flags; /* set to SSLF_x flags from ssl.h */ > > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index 96897e48..7ccfc0f8 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -282,7 +282,7 @@ struct tls_options > int ns_cert_type; > unsigned remote_cert_ku[MAX_PARMS]; > const char *remote_cert_eku; > - uint8_t *verify_hash; > + struct verify_hash_list *verify_hash; > hash_algo_type verify_hash_algo; > char *x509_username_field; > > diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c > index 97ccb93b..73d14e01 100644 > --- a/src/openvpn/ssl_verify.c > +++ b/src/openvpn/ssl_verify.c > @@ -720,9 +720,22 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep > goto cleanup; > } > > - if (memcmp(BPTR(&ca_hash), opt->verify_hash, BLEN(&ca_hash))) > + struct verify_hash_list *current_hash = opt->verify_hash; > + bool hash_matched = false; we don't really need this extra variable here. see below: > + > + while (current_hash) > + { > + if (memcmp_constant_time(BPTR(&ca_hash), current_hash->hash, > + BLEN(&ca_hash)) == 0) > + { > + hash_matched = true; .. you can just break here .. > + } > + current_hash = current_hash->next; > + } > + > + if (!hash_matched) .. and change this to if (!current_hash) << it implies we reached the end of the list without a match. > { > - msg(D_TLS_ERRORS, "TLS Error: level-1 certificate hash verification failed"); > + msg(D_TLS_ERRORS, "TLS Error: --tls-verify certificate hash verification failed"); > goto cleanup; > } > } > I believe the comments are all somewhat minor. The change is sound and well implemented. I'd ACK but I think it's better if Arne sends a new patch with some code changes, rather than having Gert change the patch on the fly. Cheers,
diff --git a/doc/man-sections/inline-files.rst b/doc/man-sections/inline-files.rst index 819bd3c8..303bb3c8 100644 --- a/doc/man-sections/inline-files.rst +++ b/doc/man-sections/inline-files.rst @@ -4,8 +4,8 @@ INLINE FILE SUPPORT OpenVPN allows including files in the main configuration for the ``--ca``, ``--cert``, ``--dh``, ``--extra-certs``, ``--key``, ``--pkcs12``, ``--secret``, ``--crl-verify``, ``--http-proxy-user-pass``, ``--tls-auth``, -``--auth-gen-token-secret``, ``--tls-crypt`` and ``--tls-crypt-v2`` -options. +``--auth-gen-token-secret``, ``--tls-crypt``, ``--tls-crypt-v2`` and +``--verify-hash`` options. Each inline file started by the line ``<option>`` and ended by the line ``</option>`` diff --git a/doc/man-sections/tls-options.rst b/doc/man-sections/tls-options.rst index 8c2db7cd..52d4137e 100644 --- a/doc/man-sections/tls-options.rst +++ b/doc/man-sections/tls-options.rst @@ -579,6 +579,16 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa The ``algo`` flag can be either :code:`SHA1` or :code:`SHA256`. If not provided, it defaults to :code:`SHA1`. + This option can also be inlined + :: + + <verify-hash> + 00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff + 11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:00 + </verify-hash> + +If the option is inlined, ``algo`` is always :code:`SHA256`. + --verify-x509-name args Accept connections only if a host's X.509 name is equal to **name.** The remote host must also pass all other tests of verification. diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 8bf82c57..068f3e75 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -1082,12 +1082,24 @@ string_substitute(const char *src, int from, int to, struct gc_arena *gc) return ret; } -static uint8_t * +/** + * Parses a hexstring and checks if the string has the correct length. Return + * a verify_hash_list of the parsed string. + * + * @param str String to check/parse + * @param nbytes Number of bytes expected in the hexstr (e.g. 20 for SHA1) + * @param msglevel message level to use when printing warnings/errors + * @param gc The returned string will be allocated in this gc. + */ +static struct verify_hash_list * parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct gc_arena *gc) { int i; const char *cp = str; - uint8_t *ret = (uint8_t *) gc_malloc(nbytes, true, gc); + + struct verify_hash_list *ret; + ALLOC_OBJ_CLEAR_GC(ret, struct verify_hash_list, gc); + char term = 1; int byte; char bs[3]; @@ -1106,7 +1118,7 @@ parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct gc_aren { msg(msglevel, "format error in hash fingerprint hex byte: %s", str); } - ret[i] = (uint8_t)byte; + ret->hash[i] = (uint8_t)byte; term = *cp++; if (term != ':' && term != 0) { @@ -1120,10 +1132,54 @@ parse_hash_fingerprint(const char *str, int nbytes, int msglevel, struct gc_aren if (term != 0 || i != nbytes-1) { msg(msglevel, "hash fingerprint is different length than expected (%d bytes): %s", nbytes, str); + return NULL; } return ret; } +/** + * Parses a string consisting of multiple lines of hexstrings and checks if each + * string has the correct length. Lines that are empty are ignored. Returns + * a verify_hash_list. + * + * @param str String to check/parse + * @param nbytes Number of bytes expected in the hexstr (e.g. 20 for SHA1) + * @param msglevel message level to use when printing warnings/errors + * @param gc The returned string will be allocated in this gc. + */ +static struct verify_hash_list * +parse_hash_fingerprint_multiline(const char *str, int nbytes, int msglevel, + struct gc_arena *gc) +{ + struct gc_arena gc_temp = gc_new(); + char *lines = string_alloc(str, &gc_temp); + + struct verify_hash_list *ret = NULL; + + const char *line; + while ((line = strsep(&lines, "\n"))) + { + /* skip empty lines */ + if (strlen(line) == 0) + { + continue; + } + + struct verify_hash_list *hash = parse_hash_fingerprint(line, nbytes, + msglevel, gc); + + if (!hash) + { + return NULL; + } + + hash->next = ret; + ret = hash; + } + gc_free(&gc_temp); + + return ret; +} #ifdef _WIN32 #ifndef ENABLE_SMALL @@ -8138,22 +8194,37 @@ add_option(struct options *options, } else if (streq(p[0], "verify-hash") && p[1] && !p[3]) { - VERIFY_PERMISSION(OPT_P_GENERAL); + VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INLINE); + options->verify_hash_algo = MD_SHA256; + + int digest_len = SHA256_DIGEST_LENGTH; 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; + msg(M_WARN, "DEPRECATED FEATURE: Usage of SHA1 fingerprints for " + "verify-hash is deprecated. You should switch to the " + "SHA256."); + options->verify_hash_algo = MD_SHA1; + digest_len = SHA_DIGEST_LENGTH; } - else if (p[2] && streq(p[2], "SHA256")) + 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; + msg(msglevel, "invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)", p[2]); + goto err; + } + + if (is_inline) + { + /* We do not have a inline flag in options struct as it is either + * a one line or multiline string */ + options->verify_hash = parse_hash_fingerprint_multiline(p[1], digest_len, msglevel, + &options->gc); } else { - msg(msglevel, "invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)", p[2]); - goto err; + options->verify_hash = parse_hash_fingerprint(p[1], digest_len, msglevel, + &options->gc); } } #ifdef ENABLE_CRYPTOAPI diff --git a/src/openvpn/options.h b/src/openvpn/options.h index 877e9396..c0dbbd8a 100644 --- a/src/openvpn/options.h +++ b/src/openvpn/options.h @@ -191,6 +191,15 @@ enum genkey_type { GENKEY_AUTH_TOKEN }; +struct verify_hash_list +{ + /* Currently we only support SHA256 for longer lists, for a one item + * list with SHA1 we ignore the 12 byte memory wasted */ + uint8_t hash[32]; + struct verify_hash_list *next; + +}; + /* Command line options */ struct options { @@ -553,7 +562,7 @@ struct options int ns_cert_type; /* set to 0, NS_CERT_CHECK_SERVER, or NS_CERT_CHECK_CLIENT */ unsigned remote_cert_ku[MAX_PARMS]; const char *remote_cert_eku; - uint8_t *verify_hash; + struct verify_hash_list *verify_hash; hash_algo_type verify_hash_algo; unsigned int ssl_flags; /* set to SSLF_x flags from ssl.h */ diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 96897e48..7ccfc0f8 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -282,7 +282,7 @@ struct tls_options int ns_cert_type; unsigned remote_cert_ku[MAX_PARMS]; const char *remote_cert_eku; - uint8_t *verify_hash; + struct verify_hash_list *verify_hash; hash_algo_type verify_hash_algo; char *x509_username_field; diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 97ccb93b..73d14e01 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -720,9 +720,22 @@ verify_cert(struct tls_session *session, openvpn_x509_cert_t *cert, int cert_dep goto cleanup; } - if (memcmp(BPTR(&ca_hash), opt->verify_hash, BLEN(&ca_hash))) + struct verify_hash_list *current_hash = opt->verify_hash; + bool hash_matched = false; + + while (current_hash) + { + if (memcmp_constant_time(BPTR(&ca_hash), current_hash->hash, + BLEN(&ca_hash)) == 0) + { + hash_matched = true; + } + current_hash = current_hash->next; + } + + if (!hash_matched) { - msg(D_TLS_ERRORS, "TLS Error: level-1 certificate hash verification failed"); + msg(D_TLS_ERRORS, "TLS Error: --tls-verify certificate hash verification failed"); goto cleanup; } }
For a new syntax introduced now it does not make much sense to support deprecated and old hashes, so support only SHA-256. Also give a warning about SHA1 hash being deprecated to verify certificates as it is now "industry standard". Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- doc/man-sections/inline-files.rst | 4 +- doc/man-sections/tls-options.rst | 10 ++++ src/openvpn/options.c | 91 +++++++++++++++++++++++++++---- src/openvpn/options.h | 11 +++- src/openvpn/ssl_common.h | 2 +- src/openvpn/ssl_verify.c | 17 +++++- 6 files changed, 119 insertions(+), 16 deletions(-)