Re: [Openvpn-devel] [PATCH 5/5] Prefer TLS libraries TLS PRF function, fix OpenVPN in FIPS mode
Am 29.01.21 um 15:09 schrieb Antonio Quartulli:
> Hi,
>
> witht his review I want to open a broader discussion about the use of
> ASSERT in the OpenVPN code.
>
> My comments below will get to the point.
>
> On 07/09/2020 18:22, Arne Schwabe wrote:
>> This moves from using our own copy of the TLS1 PRF function to using
>> TLS library provided function where possible. This includes currently
>> OpenSSL 1.1.0+ and mbed TLS 2.18+.
>>
>> For the libraries where it is not possible to use the library's own
>> function, we still use our own implementation. mbed TLS will continue
>> to use our own old PRF function while for OpenSSL we will use a
>> adapted version from OpenSSL 1.0.2t code. The version allows to be
>> used in a FIPS enabled environment.
>>
>
> Does this mean that a system that is FIPS enabled running mbedTLS <2.18
> will still not work? Or the old mbedTLS implementeation had no issue?
There is no special FIPS mode in mbed TLS. It just means that for
OpenSSL 1.1.0+ and mbed TLS 2.18+, we rely on a library provided
function instead of implementing the TLS1 PRF ourselves. When we drop
OpenSSL 1.0.2 and mbed TLS <2.18 support our own version can be removed.
For OpenSSL the library function has the additional advantage that it is
considered to be allowed in FIPS mode without special flags etc.
>
>> The old OpenSSL and mbed TLS implementation could have shared some
>> more code but as we will eventually drop support for older TLS
>> libraries, the separation makes it easier it remove that code
>> invdidually.
>>
>> No FIPS conformitiy testing etc has been done, this is only about
>> allowing OpenVPN on a system where FIPS mode has been enabled system
>> wide (e.g. on RHEL derivates).
>
> Could you add some description of why we need to change the PRF
> implementation for FISP-enabled systems?
V2 will include this paragraph:
In FIPS mode MD5 is normally forbidden, the TLS1 PRF1 function we
use, makes uses of MD5, which in the past has caused OpenVPN to segfault.
The new implementation for OpenSSL version of our custom implementation
has added the special flags that tell OpenSSL that this specific use
of MD5 is allowed in FIPS mode.
>> +{
>> +struct gc_arena gc = gc_new();
>> +uint8_t A1[MAX_HMAC_KEY_LENGTH];
>> +
>> +#ifdef ENABLE_DEBUG
>> +const int olen_orig = olen;
>> +const uint8_t *out_orig = out;
>> +#endif
>
> I think this block above is not needed anymore.
The last line in the function uses it, I just inherited this code:
dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash out: %s", format_hex(out_orig,
olen_orig, 0, &gc));
For the ASSERTs, I will send a follow up patch, which changes this code
path into something that can catch the failure.
Arne
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH 5/5] Prefer TLS libraries TLS PRF function, fix OpenVPN in FIPS mode
Hi,
witht his review I want to open a broader discussion about the use of
ASSERT in the OpenVPN code.
My comments below will get to the point.
On 07/09/2020 18:22, Arne Schwabe wrote:
> This moves from using our own copy of the TLS1 PRF function to using
> TLS library provided function where possible. This includes currently
> OpenSSL 1.1.0+ and mbed TLS 2.18+.
>
> For the libraries where it is not possible to use the library's own
> function, we still use our own implementation. mbed TLS will continue
> to use our own old PRF function while for OpenSSL we will use a
> adapted version from OpenSSL 1.0.2t code. The version allows to be
> used in a FIPS enabled environment.
>
Does this mean that a system that is FIPS enabled running mbedTLS <2.18
will still not work? Or the old mbedTLS implementeation had no issue?
> The old OpenSSL and mbed TLS implementation could have shared some
> more code but as we will eventually drop support for older TLS
> libraries, the separation makes it easier it remove that code
> invdidually.
>
> No FIPS conformitiy testing etc has been done, this is only about
> allowing OpenVPN on a system where FIPS mode has been enabled system
> wide (e.g. on RHEL derivates).
Could you add some description of why we need to change the PRF
implementation for FISP-enabled systems?
>
> Signed-off-by: Arne Schwabe
> ---
> src/openvpn/crypto_backend.h | 19 +++
> src/openvpn/crypto_mbedtls.c | 143
> src/openvpn/crypto_openssl.c | 172 -
> src/openvpn/ssl.c | 130 +--
> tests/unit_tests/openvpn/test_crypto.c | 32 +
> 5 files changed, 366 insertions(+), 130 deletions(-)
>
> diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
> index 85cb084a..b72684ce 100644
> --- a/src/openvpn/crypto_backend.h
> +++ b/src/openvpn/crypto_backend.h
> @@ -699,4 +699,23 @@ const char *translate_cipher_name_from_openvpn(const
> char *cipher_name);
> */
> const char *translate_cipher_name_to_openvpn(const char *cipher_name);
>
> +
> +/**
> + * Calculates the TLS 1.0-1.1 PRF function. For the exact specification of
> the
> + * fun ction definition see the TLS RFCs like RFC 4346.
typ0: "fun ction" -> "function"
> + *
> + * @param seed seed to use
> + * @param seed_len length of the seed
> + * @param secretsecret to use
> + * @param secret_lenlength of the secret
> + * @param outputoutput destination
> + * @param output_lenlength of output/number of bytes to generate
> + */
> +void
> +ssl_tls1_PRF(const uint8_t *seed,
> + int seed_len,
> + const uint8_t *secret,
> + int secret_len,
> + uint8_t *output,
> + int output_len);
We don't use this kind of indentation for prototypes. Please reindent it
as the other functions in this file.
(I know you have moved this signature, but while at it, make it right :))
> #endif /* CRYPTO_BACKEND_H_ */
> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
> index fbb1f120..dcdd964a 100644
> --- a/src/openvpn/crypto_mbedtls.c
> +++ b/src/openvpn/crypto_mbedtls.c
> @@ -54,6 +54,7 @@
> #include
>
> #include
> +#include
>
>
> /*
> @@ -984,4 +985,146 @@ memcmp_constant_time(const void *a, const void *b,
> size_t size)
>
> return diff;
> }
> +/* mbedtls-2.18.0 or newer */
> +#ifdef HAVE_MBEDTLS_SSL_TLS_PRF
> +void
> +ssl_tls1_PRF(const uint8_t *seed,
> +int seed_len,
> +const uint8_t *secret,
> +int secret_len,
> +uint8_t *output,
> +int output_len)
same indentation issue.
> +{
> +mbedtls_ssl_tls_prf(MBEDTLS_SSL_TLS_PRF_TLS1, secret, secret_len, "",
> seed,
> +seed_len, output, output_len);
> +}
> +#else
> +/*
> + * Generate the hash required by for the \c tls1_PRF function.
> + *
> + * @param md_kt Message digest to use
> + * @param sec Secret to base the hash on
> + * @param sec_len Length of the secret
> + * @param seed Seed to hash
> + * @param seed_len Length of the seed
> + * @param out Output buffer
> + * @param olen Length of the output buffer
> + */
> +static void
> +tls1_P_hash(const md_kt_t *md_kt,
> +const uint8_t *sec,
> +int sec_len,
> +const uint8_t *seed,
> +int seed_len,
> +uint8_t *out,
> +int olen)
same indentation issue.
> +{
> +struct gc_arena gc = gc_new();
> +uint8_t A1[MAX_HMAC_KEY_LENGTH];
> +
> +#ifdef ENABLE_DEBUG
> +const int olen_orig = olen;
> +const uint8_t *out_orig = out;
> +#endif
I think this block above is not needed anymore.
> +
> +hmac_ctx_t *ctx = hmac_ctx_new();
> +hmac_ctx_t *ctx_tmp = hmac_ctx_new();
> +
> +dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash sec: %s", format_hex(sec, sec_len,
> 0, &gc));
> +dmsg(D_
[Openvpn-devel] [PATCH 5/5] Prefer TLS libraries TLS PRF function, fix OpenVPN in FIPS mode
This moves from using our own copy of the TLS1 PRF function to using
TLS library provided function where possible. This includes currently
OpenSSL 1.1.0+ and mbed TLS 2.18+.
For the libraries where it is not possible to use the library's own
function, we still use our own implementation. mbed TLS will continue
to use our own old PRF function while for OpenSSL we will use a
adapted version from OpenSSL 1.0.2t code. The version allows to be
used in a FIPS enabled environment.
The old OpenSSL and mbed TLS implementation could have shared some
more code but as we will eventually drop support for older TLS
libraries, the separation makes it easier it remove that code
invdidually.
No FIPS conformitiy testing etc has been done, this is only about
allowing OpenVPN on a system where FIPS mode has been enabled system
wide (e.g. on RHEL derivates).
Signed-off-by: Arne Schwabe
---
src/openvpn/crypto_backend.h | 19 +++
src/openvpn/crypto_mbedtls.c | 143
src/openvpn/crypto_openssl.c | 172 -
src/openvpn/ssl.c | 130 +--
tests/unit_tests/openvpn/test_crypto.c | 32 +
5 files changed, 366 insertions(+), 130 deletions(-)
diff --git a/src/openvpn/crypto_backend.h b/src/openvpn/crypto_backend.h
index 85cb084a..b72684ce 100644
--- a/src/openvpn/crypto_backend.h
+++ b/src/openvpn/crypto_backend.h
@@ -699,4 +699,23 @@ const char *translate_cipher_name_from_openvpn(const char
*cipher_name);
*/
const char *translate_cipher_name_to_openvpn(const char *cipher_name);
+
+/**
+ * Calculates the TLS 1.0-1.1 PRF function. For the exact specification of the
+ * fun ction definition see the TLS RFCs like RFC 4346.
+ *
+ * @param seed seed to use
+ * @param seed_len length of the seed
+ * @param secretsecret to use
+ * @param secret_lenlength of the secret
+ * @param outputoutput destination
+ * @param output_lenlength of output/number of bytes to generate
+ */
+void
+ssl_tls1_PRF(const uint8_t *seed,
+ int seed_len,
+ const uint8_t *secret,
+ int secret_len,
+ uint8_t *output,
+ int output_len);
#endif /* CRYPTO_BACKEND_H_ */
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index fbb1f120..dcdd964a 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -54,6 +54,7 @@
#include
#include
+#include
/*
@@ -984,4 +985,146 @@ memcmp_constant_time(const void *a, const void *b, size_t
size)
return diff;
}
+/* mbedtls-2.18.0 or newer */
+#ifdef HAVE_MBEDTLS_SSL_TLS_PRF
+void
+ssl_tls1_PRF(const uint8_t *seed,
+int seed_len,
+const uint8_t *secret,
+int secret_len,
+uint8_t *output,
+int output_len)
+{
+mbedtls_ssl_tls_prf(MBEDTLS_SSL_TLS_PRF_TLS1, secret, secret_len, "", seed,
+seed_len, output, output_len);
+}
+#else
+/*
+ * Generate the hash required by for the \c tls1_PRF function.
+ *
+ * @param md_kt Message digest to use
+ * @param sec Secret to base the hash on
+ * @param sec_len Length of the secret
+ * @param seed Seed to hash
+ * @param seed_len Length of the seed
+ * @param out Output buffer
+ * @param olen Length of the output buffer
+ */
+static void
+tls1_P_hash(const md_kt_t *md_kt,
+const uint8_t *sec,
+int sec_len,
+const uint8_t *seed,
+int seed_len,
+uint8_t *out,
+int olen)
+{
+struct gc_arena gc = gc_new();
+uint8_t A1[MAX_HMAC_KEY_LENGTH];
+
+#ifdef ENABLE_DEBUG
+const int olen_orig = olen;
+const uint8_t *out_orig = out;
+#endif
+
+hmac_ctx_t *ctx = hmac_ctx_new();
+hmac_ctx_t *ctx_tmp = hmac_ctx_new();
+
+dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash sec: %s", format_hex(sec, sec_len, 0,
&gc));
+dmsg(D_SHOW_KEY_SOURCE, "tls1_P_hash seed: %s", format_hex(seed, seed_len,
0, &gc));
+
+int chunk = md_kt_size(md_kt);
+unsigned int A1_len = md_kt_size(md_kt);
+
+hmac_ctx_init(ctx, sec, sec_len, md_kt);
+hmac_ctx_init(ctx_tmp, sec, sec_len, md_kt);
+
+hmac_ctx_update(ctx,seed,seed_len);
+hmac_ctx_final(ctx, A1);
+
+for (;; )
+{
+hmac_ctx_reset(ctx);
+hmac_ctx_reset(ctx_tmp);
+hmac_ctx_update(ctx,A1,A1_len);
+hmac_ctx_update(ctx_tmp,A1,A1_len);
+hmac_ctx_update(ctx,seed,seed_len);
+
+if (olen > chunk)
+{
+hmac_ctx_final(ctx, out);
+out += chunk;
+olen -= chunk;
+hmac_ctx_final(ctx_tmp, A1); /* calc the next A1 value */
+}
+else/* last one */
+{
+hmac_ctx_final(ctx, A1);
+memcpy(out,A1,olen);
+break;
+}
+}
+hmac_ctx_cleanup(ctx);
+hmac_ctx_free(ctx);
+hmac_ctx_cleanup(ctx_tmp);
+
