[Openvpn-devel,v3] Extend verify-hash to allow multiple hashes
Commit Message
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.
Patch v2: fix/clarify various comments, fix a few minor problems, allow
the option to be specified multiple times and have that
added to the list.
Patch v3: Remove leftover variable, always call
parse_hash_fingerprint_multiline, add comments clarifying list
appending
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 | 102 ++++++++++++++++++++++++++----
src/openvpn/options.h | 10 ++-
src/openvpn/ssl_common.h | 2 +-
src/openvpn/ssl_verify.c | 16 ++++-
6 files changed, 127 insertions(+), 17 deletions(-)
Comments
Hi,
This patch looks good to me.
There is just one minor note below:
On 21/03/2021 15:25, Arne Schwabe wrote:
> 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.
>
> Patch v2: fix/clarify various comments, fix a few minor problems, allow
> the option to be specified multiple times and have that
> added to the list.
>
> Patch v3: Remove leftover variable, always call
> parse_hash_fingerprint_multiline, add comments clarifying list
> appending
>
> 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 | 102 ++++++++++++++++++++++++++----
> src/openvpn/options.h | 10 ++-
> src/openvpn/ssl_common.h | 2 +-
> src/openvpn/ssl_verify.c | 16 ++++-
> 6 files changed, 127 insertions(+), 17 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 e13fb3c8..d8f9800e 100644
> --- a/doc/man-sections/tls-options.rst
> +++ b/doc/man-sections/tls-options.rst
> @@ -582,6 +582,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 86e78b05..3b1c69ba 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -1078,12 +1078,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 containing the parsed hash 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 object 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];
> @@ -1102,7 +1114,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)
> {
> @@ -1116,10 +1128,55 @@ 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;
The rest of this function assumes that msglevel is set to a FATAL level.
As you can also see in the hunk above, if blocks handling other errors
have no return statement, because they assume the program will exit there.
Personally I find this style very counter intuitive.
Also we should consider that the msglevel is a variable here, so this
very function CANNOT assume that the msglevel contains the FATAL bit.
Imagine using this function in a another context in the future, while
forgetting about this hidden assumption: disaster.
Now, this problem is mostly unrelated to this patch.
We just have to make a decision here:
1) remove the "return NULL" (it's just above my text), in order to
follow the current function's style - because the return will never be
hit; or
2) accept the patch as is and move on.
In both cases, this patch gets my ACK.
Acked-by: Antonio Quartulli <antonio@openvpn.net>
The error handling in the option parser is a topic we could discuss in a
meeting.
Cheers,
Your patch has been applied to the master branch.
I have not really tested it, but stared a bit at code and documentation,
and done a basic t_client test. Haven't set up anything that could
thoroughly test this (yet!).
As discussed on IRC, I have removed the "return NULL" as suggested,
and there will be a cleanup patch to fix this function for good.
commit d1fe6d52ca066ec2d49712081d5056825c8973b2
Author: Arne Schwabe
Date: Sun Mar 21 15:25:38 2021 +0100
Extend verify-hash to allow multiple hashes
Signed-off-by: Arne Schwabe <arne@rfc2549.org>
Acked-by: Antonio Quartulli <antonio@openvpn.net>
Message-Id: <20210321142538.1656-1-arne@rfc2549.org>
URL: https://www.mail-archive.com/search?l=mid&q=20210321142538.1656-1-arne@rfc2549.org
Signed-off-by: Gert Doering <gert@greenie.muc.de>
--
kind regards,
Gert Doering
@@ -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>``
@@ -582,6 +582,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.
@@ -1078,12 +1078,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 containing the parsed hash 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 object 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];
@@ -1102,7 +1114,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)
{
@@ -1116,10 +1128,55 @@ 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. Empty lines are ignored. Returns
+ * a linked list of (possibly) multiple verify_hash_list objects.
+ *
+ * @param str String to check/parse
+ * @param nbytes Number of bytes expected in the hexstring (e.g. 20 for SHA1)
+ * @param msglevel message level to use when printing warnings/errors
+ * @param gc The returned list items 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)
+ {
+ gc_free(&gc_temp);
+ return NULL;
+ }
+
+ hash->next = ret;
+ ret = hash;
+ }
+ gc_free(&gc_temp);
+
+ return ret;
+}
#ifdef _WIN32
#ifndef ENABLE_SMALL
@@ -8063,22 +8120,45 @@ 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")))
+ if ((!p[2] && !is_inline) || (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 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;
+ }
+
+ struct verify_hash_list *newlist;
+ newlist = parse_hash_fingerprint_multiline(p[1], digest_len,
+ msglevel, &options->gc);
+
+ /* Append the new list to the end of our current list */
+ if (!options->verify_hash)
+ {
+ options->verify_hash = newlist;
}
else
{
- msg(msglevel, "invalid or unsupported hashing algorithm: %s (only SHA1 and SHA256 are valid)", p[2]);
- goto err;
+ /* since both the old and new list can have multiple entries
+ * we need to go to the end of one of them to concatenate them */
+ struct verify_hash_list *listend = options->verify_hash;
+ while (listend->next)
+ {
+ listend = listend->next;
+ }
+ listend->next = newlist;
}
}
#ifdef ENABLE_CRYPTOAPI
@@ -191,6 +191,14 @@ enum genkey_type {
GENKEY_AUTH_TOKEN
};
+struct verify_hash_list
+{
+ /* We support SHA256 and SHA1 fingerpint. In the case of using the
+ * deprecated SHA1, only the first 20 bytes of each list item are used */
+ uint8_t hash[SHA256_DIGEST_LENGTH];
+ struct verify_hash_list *next;
+};
+
/* Command line options */
struct options
{
@@ -550,7 +558,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 */
@@ -283,7 +283,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;
#ifdef ENABLE_X509ALTUSERNAME
char *x509_username_field[MAX_PARMS];
@@ -748,9 +748,21 @@ 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;
+
+ while (current_hash)
+ {
+ if (memcmp_constant_time(BPTR(&ca_hash), current_hash->hash,
+ BLEN(&ca_hash)) == 0)
+ {
+ break;
+ }
+ current_hash = current_hash->next;
+ }
+
+ if (!current_hash)
{
- 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;
}
}