Re: [Openvpn-devel] [PATCH] build: Remove --disable-server from ./configure
Am 27.02.20 um 21:54 schrieb David Sommerseth:
> After some discussion among the core community developers [1,2], it was
> decided to remove the possibility to build openvpn as a pure client.
> This was alterted on the mailing list [3] that it was scheduled for
> removal unless anyone had strong arguments why it was needed.
>
> The general consensus was that we had not received any strong arguments
> to keep this possibility after approximately 5 months, so it was fine to
> remove this ./configure option.
>
> By removing this, we remove quite some entangled sections of #ifdef
> scattered all over the code base, making it more readable.
>
> One note:
> Inside the options_postprocess_mutate_invariant() function,
> the #ifdef P2MP_SERVER and #ifdef _WIN32 blocks where slightly
> reworked to make the _WIN32 block more continous and avoiding having an
> empty if(options->mode == MODE_SERVER) block.
There are three trivial conflicts when applying this patch and push.c
needs a small fix:
> ret = process_incoming_push_request(c);
> }
> else
> -#endif
>
> if (honor_received_options && buf_string_compare_advance(&buf,
> "PUSH_REPLY"))
> {
This if needs to be moved to its else to become else if since the #endif
is now gone.
I also compared the fix with "unifdef -DP2MP_SERVER=1 -m *.c *.h" and
the results are almost identical (minus a few things unifdef did not get
right/autoconf stuff).
Acked-By: Arne Schwabe
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v9] convert *_inline attributes to bool
Am 22.04.20 um 11:26 schrieb Antonio Quartulli:
> Carrying around the INLINE_TAG is not really efficient,
> because it requires a strcmp() to be performed every
> time we want to understand if the data is stored inline
> or not.
>
> Convert all the *_inline attributes to bool to make the
> logic easier and checks more efficient.
>
Look through it. It is only a minor improvement but Antonio has been
persistent enough and spend enough time to make 9 versions of this so I
took the time to look through it:
Acked-By: Arne Schwabe
The patch is not uncrustify clean at the moment. So uncrustify should be
run on commit or we need a v10 of this patch.
The patch does not apply cleanly anymore: The test_tls_crypt.c part
should be now.
--- a/tests/unit_tests/openvpn/test_tls_crypt.c
+++ b/tests/unit_tests/openvpn/test_tls_crypt.c
@@ -549,8 +549,7 @@ test_tls_crypt_v2_write_client_key_file(void **state) {
expect_string(__wrap_buffer_read_from_file, filename, filename);
will_return(__wrap_buffer_read_from_file, test_client_key);
-tls_crypt_v2_write_client_key_file(filename, NULL, INLINE_FILE_TAG,
- test_server_key);
+ tls_crypt_v2_write_client_key_file(filename, NULL, test_server_key,
true);
}
static void
@@ -567,8 +566,7 @@
test_tls_crypt_v2_write_client_key_file_metadata(void **state) {
expect_string(__wrap_buffer_read_from_file, filename, filename);
will_return(__wrap_buffer_read_from_file, test_client_key_metadata);
-tls_crypt_v2_write_client_key_file(filename, b64metadata,
INLINE_FILE_TAG,
- test_server_key);
+tls_crypt_v2_write_client_key_file(filename, NULL, test_server_key,
true);
}
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH v3] Do not write extra 0 byte for --gen-key with auth-token/tls-crypt-v2
Change crypto_pem_encode to not put a nul-terminated terminated
string into the buffer. This was useful for printf but should
not be written into the file.
Instead do not assume that the buffer is null terminated and
print only the number of bytes in the buffer. Also fix a
similar case in printing static key where the 0 byte was
never added to the buffer
Patch V2: make pem_encode behave more like other similar functions in OpenVPN
and do not null terminate.
Patch V3: also make the mbed TLS variant of pem_decode behave like other
similar functions in OpeNVPN and accept a not null-terminated
buffer.
Signed-off-by: Arne Schwabe
---
src/openvpn/crypto.c | 4 ++--
src/openvpn/crypto_mbedtls.c | 28 +--
src/openvpn/crypto_openssl.c | 3 +--
src/openvpn/tls_crypt.c | 2 +-
tests/unit_tests/openvpn/test_tls_crypt.c | 6 +++--
5 files changed, 29 insertions(+), 14 deletions(-)
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 1678cba8..b05262e1 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -1478,7 +1478,7 @@ write_key_file(const int nkeys, const char *filename)
/* write key file to stdout if no filename given */
if (!filename || strcmp(filename, "")==0)
{
-printf("%s\n", BPTR(&out));
+printf("%.*s\n", BLEN(&out), BPTR(&out));
}
/* write key file, now formatted in out, to file */
else if (!buffer_write_file(filename, &out))
@@ -1887,7 +1887,7 @@ write_pem_key_file(const char *filename, const char
*pem_name)
if (!filename || strcmp(filename, "")==0)
{
-printf("%s\n", BPTR(&server_key_pem));
+printf("%.*s", BLEN(&server_key_pem), BPTR(&server_key_pem));
}
else if (!buffer_write_file(filename, &server_key_pem))
{
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 3e77fa9e..9529b319 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -239,10 +239,12 @@ crypto_pem_encode(const char *name, struct buffer *dst,
return false;
}
+/* We set the size buf to out_len-1 to NOT include the 0 byte that
+ * mbedtls_pem_write_buffer in its length calculation */
*dst = alloc_buf_gc(out_len, gc);
if (!mbed_ok(mbedtls_pem_write_buffer(header, footer, BPTR(src), BLEN(src),
BPTR(dst), BCAP(dst), &out_len))
-|| !buf_inc_len(dst, out_len))
+|| !buf_inc_len(dst, out_len-1))
{
CLEAR(*dst);
return false;
@@ -259,11 +261,6 @@ crypto_pem_decode(const char *name, struct buffer *dst,
char header[1000+1] = { 0 };
char footer[1000+1] = { 0 };
-if (*(BLAST(src)) != '\0')
-{
-msg(M_WARN, "PEM decode error: source buffer not null-terminated");
-return false;
-}
if (!openvpn_snprintf(header, sizeof(header), "-BEGIN %s-", name))
{
return false;
@@ -273,9 +270,25 @@ crypto_pem_decode(const char *name, struct buffer *dst,
return false;
}
+/* mbed TLS requires the src to be null-terminated */
+struct gc_arena gc = gc_new();
+struct buffer input;
+
+if (*(BLAST(src)) == '\0')
+{
+input = *src;
+}
+else
+{
+/* allocate a new buffer to avoid modifying the src buffer */
+input = alloc_buf_gc(BLEN(src) + 1, &gc);
+buf_copy(&input, src);
+buf_null_terminate(&input);
+}
+
size_t use_len = 0;
mbedtls_pem_context ctx = { 0 };
-bool ret = mbed_ok(mbedtls_pem_read_buffer(&ctx, header, footer, BPTR(src),
+bool ret = mbed_ok(mbedtls_pem_read_buffer(&ctx, header, footer,
BPTR(&input),
NULL, 0, &use_len));
if (ret && !buf_write(dst, ctx.buf, ctx.buflen))
{
@@ -284,6 +297,7 @@ crypto_pem_decode(const char *name, struct buffer *dst,
}
mbedtls_pem_free(&ctx);
+gc_free(&gc);
return ret;
}
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index a81dcfd8..4fa65ba8 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -400,9 +400,8 @@ crypto_pem_encode(const char *name, struct buffer *dst,
BUF_MEM *bptr;
BIO_get_mem_ptr(bio, &bptr);
-*dst = alloc_buf_gc(bptr->length + 1, gc);
+*dst = alloc_buf_gc(bptr->length, gc);
ASSERT(buf_write(dst, bptr->data, bptr->length));
-buf_null_terminate(dst);
ret = true;
cleanup:
diff --git a/src/openvpn/tls_crypt.c b/src/openvpn/tls_crypt.c
index e9f9cc2a..3018c18e 100644
--- a/src/openvpn/tls_crypt.c
+++ b/src/openvpn/tls_crypt.c
@@ -702,7 +702,7 @@ tls_crypt_v2_write_client_key_file(const char *filename,
if (!filename || streq(filename, ""))
{
-printf("%s\n", BPTR(&client_key_pem));
+printf("%.*s\n", BLEN(&client_key_pem), BPTR(&client_key_pem));
client_filen
Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
Am 26.04.20 um 11:34 schrieb Gert Doering: > Hi, > > On Sun, Apr 26, 2020 at 11:25:49AM +0200, Steffan Karger wrote: well, sometimes to adhere to the codestyle, you have to re-arrange code :) >>> >>> "rearrange" and "rewrite in a not easy to understand way" (which looks >>> a bit overthought to me, TBH - unlike "secure memzero" I cannot see an >>> obvious reason why all that volatile would be relevant). >> >> This secure memcmp is relevant to avoid timing side channels in e.g. >> authentication tag compare. Think about the HMAC in our tls-auth/crypt >> and the HMAC of (non-AEAD) data channel packets. > > I do understand why it has to be constant *time*, in regards to "do the > compared buffers differ or not". > > I do not see how all this "volatile" and "copy from pointer to variables > to other stuff" handwaving is going to make any difference wrt constant > time comparison. > > And it hurts my eyes. Yeah the goal is basically do what the crypto library is doing. And if you play with godbolt. So we don't need to go down this path of having to come up with our own version. And if you care about speed then we should definitively go for OpenSSL's memcmp function. Since it is implemented in assembler it does not try to breaks the compiler compilation optimisation with volatile that might trigger more memory loads than necessary but can be a small fast constant time function. If you play a bit with godbolt.org (https://godbolt.org/z/gXgcC9) you will see that the code that is generated from our current version is not yet optimised to non constant versions but especially the clang compiler is getting close. It completely optimises away the function for compile strings (main2 just becomes return 95), builds a version optimised for one fixed string and vectorises the generic version. So I am not sure if these variants still have their constant with modern processes and speculative execution. Arne signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Uncrustify the tests/unit_tests/ part of our tree.
Hi,
On 26-04-2020 11:54, Gert Doering wrote:
> Apply uncrustify 0.70.1 (FreeBSD port) with our rules to that part
> of the tree, which followed a more compact coding style so far.
> ---
>
> @@ -155,20 +157,21 @@ test_packet_id_write_long_wrap(void **state)
> }
>
> int
> -main(void) {
> +main(void)
> +{
> const struct CMUnitTest tests[] = {
> -cmocka_unit_test_setup_teardown(test_packet_id_write_short,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -cmocka_unit_test_setup_teardown(test_packet_id_write_long,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -
> cmocka_unit_test_setup_teardown(test_packet_id_write_short_prepend,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -
> cmocka_unit_test_setup_teardown(test_packet_id_write_long_prepend,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -cmocka_unit_test_setup_teardown(test_packet_id_write_short_wrap,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> -cmocka_unit_test_setup_teardown(test_packet_id_write_long_wrap,
> -test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_short,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_long,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_short_prepend,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_long_prepend,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_short_wrap,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> +cmocka_unit_test_setup_teardown(test_packet_id_write_long_wrap,
> +test_packet_id_write_setup,
> test_packet_id_write_teardown),
> };
This now exceeds 80 chars by a fair amount. Perhaps rewrap?
Otherwise this looks good to me. I think having consistent style over
the code base, including tests, would be good.
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Uncrustify the tests/unit_tests/ part of our tree.
Hi,
On Sun, Apr 26, 2020 at 11:54:02AM +0200, Gert Doering wrote:
> Apply uncrustify 0.70.1 (FreeBSD port) with our rules to that part
> of the tree, which followed a more compact coding style so far.
Just as a remark: the coding style in tests/unit_tests is fairly
consistent, just *different*.
We can apply the same rules, or we can define a "more compact" ruleset,
which is sometimes more important for tests ("I want to see what the
test is doing, not stare at pages of { } lines").
So this is more out for discussion than for "please apply this patch".
gert
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
[Openvpn-devel] [PATCH] Uncrustify the tests/unit_tests/ part of our tree.
Apply uncrustify 0.70.1 (FreeBSD port) with our rules to that part
of the tree, which followed a more compact coding style so far.
---
tests/unit_tests/example_test/test.c | 18 ++-
tests/unit_tests/example_test/test2.c | 6 +-
tests/unit_tests/openvpn/test_argv.c | 5 +-
tests/unit_tests/openvpn/test_buffer.c | 6 +-
tests/unit_tests/openvpn/test_crypto.c | 6 +-
tests/unit_tests/openvpn/test_ncp.c| 6 +-
tests/unit_tests/openvpn/test_networking.c | 40 +
tests/unit_tests/openvpn/test_packet_id.c | 35 +++--
tests/unit_tests/openvpn/test_tls_crypt.c | 161 -
9 files changed, 180 insertions(+), 103 deletions(-)
diff --git a/tests/unit_tests/example_test/test.c
b/tests/unit_tests/example_test/test.c
index d48e5f55..bc3fdc17 100644
--- a/tests/unit_tests/example_test/test.c
+++ b/tests/unit_tests/example_test/test.c
@@ -7,7 +7,8 @@
#include
static int
-setup(void **state) {
+setup(void **state)
+{
int *answer = malloc(sizeof(int));
*answer = 42;
@@ -17,31 +18,36 @@ setup(void **state) {
}
static int
-teardown(void **state) {
+teardown(void **state)
+{
free(*state);
return 0;
}
static void
-null_test_success(void **state) {
+null_test_success(void **state)
+{
(void) state;
}
static void
-int_test_success(void **state) {
+int_test_success(void **state)
+{
int *answer = *state;
assert_int_equal(*answer, 42);
}
static void
-failing_test(void **state) {
+failing_test(void **state)
+{
/* This tests fails to test that make check fails */
assert_int_equal(0, 42);
}
int
-main(void) {
+main(void)
+{
const struct CMUnitTest tests[] = {
cmocka_unit_test(null_test_success),
cmocka_unit_test_setup_teardown(int_test_success, setup, teardown),
diff --git a/tests/unit_tests/example_test/test2.c
b/tests/unit_tests/example_test/test2.c
index b5d4fa60..5a186d5d 100644
--- a/tests/unit_tests/example_test/test2.c
+++ b/tests/unit_tests/example_test/test2.c
@@ -8,13 +8,15 @@
static void
-test_true(void **state) {
+test_true(void **state)
+{
(void) state;
}
int
-main(void) {
+main(void)
+{
const struct CMUnitTest tests[] = {
cmocka_unit_test(test_true),
};
diff --git a/tests/unit_tests/openvpn/test_argv.c
b/tests/unit_tests/openvpn/test_argv.c
index 25e80c1c..3dc470a5 100644
--- a/tests/unit_tests/openvpn/test_argv.c
+++ b/tests/unit_tests/openvpn/test_argv.c
@@ -204,7 +204,7 @@ argv_str__multiple_argv__correct_output(void **state)
argv_printf_cat(&a, "%lu", 1L );
output = argv_str(&a, &gc, PA_BRACKET);
assert_string_equal(output, "[" PATH1 PATH2 "] [" PARAM1 "] [" PARAM2 "]"
- " [-1] [4294967295] [1]");
+" [-1] [4294967295] [1]");
argv_free(&a);
gc_free(&gc);
@@ -234,7 +234,8 @@ argv_insert_head__non_empty_argv__head_added(void **state)
argv_printf(&a, "%s", PATH2);
b = argv_insert_head(&a, PATH1);
assert_int_equal(b.argc, a.argc + 1);
-for (i = 0; i < b.argc; i++) {
+for (i = 0; i < b.argc; i++)
+{
if (i == 0)
{
assert_string_equal(b.argv[i], PATH1);
diff --git a/tests/unit_tests/openvpn/test_buffer.c
b/tests/unit_tests/openvpn/test_buffer.c
index 7c9a9e2f..d2188b02 100644
--- a/tests/unit_tests/openvpn/test_buffer.c
+++ b/tests/unit_tests/openvpn/test_buffer.c
@@ -63,7 +63,8 @@ struct test_buffer_list_aggregate_ctx {
struct buffer_list *empty_buffers;
};
-static int test_buffer_list_setup(void **state)
+static int
+test_buffer_list_setup(void **state)
{
struct test_buffer_list_aggregate_ctx *ctx = calloc(1, sizeof(*ctx));
ctx->empty = buffer_list_new(0);
@@ -86,7 +87,8 @@ static int test_buffer_list_setup(void **state)
return 0;
}
-static int test_buffer_list_teardown(void **state)
+static int
+test_buffer_list_teardown(void **state)
{
struct test_buffer_list_aggregate_ctx *ctx = *state;
diff --git a/tests/unit_tests/openvpn/test_crypto.c
b/tests/unit_tests/openvpn/test_crypto.c
index 7027d3da..fdf814de 100644
--- a/tests/unit_tests/openvpn/test_crypto.c
+++ b/tests/unit_tests/openvpn/test_crypto.c
@@ -44,7 +44,8 @@
static const char testtext[] = "Dummy text to test PEM encoding";
static void
-crypto_pem_encode_decode_loopback(void **state) {
+crypto_pem_encode_decode_loopback(void **state)
+{
struct gc_arena gc = gc_new();
struct buffer src_buf;
buf_set_read(&src_buf, (void *)testtext, sizeof(testtext));
@@ -69,7 +70,8 @@ crypto_pem_encode_decode_loopback(void **state) {
}
int
-main(void) {
+main(void)
+{
const struct CMUnitTest tests[] = {
cmocka_unit_test(crypto_pem_encode_decode_loopback),
};
diff --git a/tests/unit_tests/openvpn/test_ncp.c
b/tests/unit_tests/openvpn/test_ncp.c
index f8d03b35..19432410 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -66,12 +
[Openvpn-devel] [PATCH applied] Re: Add tls-crypt-v2 test writing metadata
Your patch has been applied to the master branch. Basic "make check" testing with cmocka on linux with openssl 1.1.1 and mbedtls passed. I have changed whitespaceing of the *new* lines, as instructed, but I notice that this whole file is not according to coding conventions - so it seems the last round of uncrustify patches overlooked the "tests/" subdirectory. Patch coming. commit a17e73531404aeb9d26ef874d55e46754ec523ab (master) Author: Arne Schwabe Date: Mon Apr 20 12:44:35 2020 +0200 Add tls-crypt-v2 test writing metadata Acked-by: Steffan Karger Message-Id: <20200420104435.7082-1-a...@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19798.html Signed-off-by: Gert Doering -- kind regards, Gert Doering ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
Hi, On 26-04-2020 11:34, Gert Doering wrote: > On Sun, Apr 26, 2020 at 11:25:49AM +0200, Steffan Karger wrote: well, sometimes to adhere to the codestyle, you have to re-arrange code :) >>> >>> "rearrange" and "rewrite in a not easy to understand way" (which looks >>> a bit overthought to me, TBH - unlike "secure memzero" I cannot see an >>> obvious reason why all that volatile would be relevant). >> >> This secure memcmp is relevant to avoid timing side channels in e.g. >> authentication tag compare. Think about the HMAC in our tls-auth/crypt >> and the HMAC of (non-AEAD) data channel packets. > > I do understand why it has to be constant *time*, in regards to "do the > compared buffers differ or not". > > I do not see how all this "volatile" and "copy from pointer to variables > to other stuff" handwaving is going to make any difference wrt constant > time comparison. > > And it hurts my eyes. Pretty it is not. But "let's not try to outsmart the crypto library folks" makes sense to me. The copying stuff is probably just about making some annoying compiler happy. We could probably leave that out, but that makes it harder to see that we follow the mbed internal implementation. When inlining this without volatile keywords, a compiler might at some point become smart enough to realize "hey, this caller only cares about zero/nonzero, not the actual value. And this code will only return zero if *all* bytes are equal, so I can stop comparing as soon as soon as I've found a difference". Up to now, it seems compilers have not gotten this smart. But it's not like we're keeping a close eye on that. So I would love to get rid of the responsibility :-) -Steffan signature.asc Description: OpenPGP digital signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
Hi, On Sun, Apr 26, 2020 at 11:25:49AM +0200, Steffan Karger wrote: > >> well, sometimes to adhere to the codestyle, you have to re-arrange code :) > > > > "rearrange" and "rewrite in a not easy to understand way" (which looks > > a bit overthought to me, TBH - unlike "secure memzero" I cannot see an > > obvious reason why all that volatile would be relevant). > > This secure memcmp is relevant to avoid timing side channels in e.g. > authentication tag compare. Think about the HMAC in our tls-auth/crypt > and the HMAC of (non-AEAD) data channel packets. I do understand why it has to be constant *time*, in regards to "do the compared buffers differ or not". I do not see how all this "volatile" and "copy from pointer to variables to other stuff" handwaving is going to make any difference wrt constant time comparison. And it hurts my eyes. [..] > This kind of code is a tricky balance between "prevent the compiler from > optimizing it to a not-constant-time implementation" How could the compiler optimize *this* code? It has very explicit instructions to build the xor of every single byte in the buffer and or them all together, and return the result as an integer. Unlike secure_memzero() whatever compiler optimization is chosen, it still needs to do the actual math for *all bytes*. It can not optimize out "if a string does not match early on, end comparisions more early". (It could, if the result is already 0xff, but that optimization would slow down the loop, so I would find very surprising) > and "as much > performance as we can get". Moving this responsibility to the crypto > library seems like a good idea to me. > > And because our recommended data channel ciphers are AEAD ciphers for > which the auth tag compare is handled internally by the crypto library, > I don't care so much for the performance aspect. Want best security? Use > AEAD! Want best performance? Use AEAD! > > You get the point. Use AEAD ;-) Now that's definitely a strong argument against my "inline! performance!" argument. gert -- "If was one thing all people took for granted, was conviction that if you feed honest figures into a computer, honest figures come out. Never doubted it myself till I met a computer with a sense of humor." Robert A. Heinlein, The Moon is a Harsh Mistress Gert Doering - Munich, Germany g...@greenie.muc.de signature.asc Description: PGP signature ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [Openvpn-users] new openssl = new OpenVPN release ?
On 22-04-2020 10:27, Jan Just Keijser wrote: > On 22/04/20 10:13, Arne Schwabe wrote: SSL_check_chain() function". Which we don't, I just grepped through our source tree. So, unless I misunderstand something about OpenSSL intricacies, I think we're safe - no new installers needed, and OpenVPN is not in risk. >>> the advisory applies only to application that use the SSL_check_chain() >>> function as part of a TLS 1.3 handshake. AFAIK, iIn OpenVPN 2.4 we don't >>> do anything with TLS 1.3 just yet, so this security advisory does not >>> apply to OpenVPN. Also note that this bug appears only in OpenSSL 1.1.1 >>> [d-f] , so anything older is fine as well. >> Hu? OpenVPN 2.4 supports TLS 1.3 just fine. We have support for it in >> tls-version-min and also tls-ciphersuites which is TLS 1.3 specific. >> >> > what I meant was that OpenVPN 2.4 does not do any *specific* with any of > the new features of TLS 1.3, like the new psk callbacks etc. If the > control session is negotiated using TLS 1.3 then sure, OpenVPN will use > that, but other that OpenVPN does not make use of any of the new > features or crypto algorithms that come with OpenSSL 1.1.1 or TLS 1.3 > (chacha20 anyone ;) ? ) Arne has been working on some of the TLS 1.3-specific features, such as using CHACHA20-POLY1305 for the control channel: $ openvpn --show-tls Available TLS Ciphers, listed in order of preference: For TLS 1.3 and newer (--tls-ciphersuites): TLS_AES_256_GCM_SHA384 TLS_CHACHA20_POLY1305_SHA256 TLS_AES_128_GCM_SHA256 For TLS 1.2 and older (--tls-cipher): TLS-ECDHE-ECDSA-WITH-AES-256-GCM-SHA384 (This is with OpenVPN 2.4.7 from the Ubuntu 20.04 package.) -Steffan ___ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
Hi,
On 17-04-2020 17:36, Gert Doering wrote:
> On Fri, Apr 17, 2020 at 03:42:49PM +0200, Antonio Quartulli wrote:
-static inline int
-memcmp_constant_time(const void *a, const void *b, size_t size)
-{
>>>
>>> Not sure I understand the motivation for this change. "Just so uncrustify
>>> stops trying to change this" is not really strong.
>>
>> well, sometimes to adhere to the codestyle, you have to re-arrange code :)
>
> "rearrange" and "rewrite in a not easy to understand way" (which looks
> a bit overthought to me, TBH - unlike "secure memzero" I cannot see an
> obvious reason why all that volatile would be relevant).
This secure memcmp is relevant to avoid timing side channels in e.g.
authentication tag compare. Think about the HMAC in our tls-auth/crypt
and the HMAC of (non-AEAD) data channel packets.
>> On top of that, this is basically allowing us to re-use an existing
>> openssl API when possible.
>
> True, but if that turns out to be a code complication and reduces
> efficiency, I'm not convinced it's the right way to spend our cycles.
>
>>> Also, keeping the "inline" part would be good... this is in the per-packet
>>> path.
>>
>> I am not sure it would work in this case, because the function is
>> defined in a .c file now - it's not inlineable anymore outside of the
>> mbedtls code.
>
> This is why I'm not exactly happy with the change.
>
> We could do it "openvpn style" all in header files, or we could just
> leave the function alone.
This kind of code is a tricky balance between "prevent the compiler from
optimizing it to a not-constant-time implementation" and "as much
performance as we can get". Moving this responsibility to the crypto
library seems like a good idea to me.
And because our recommended data channel ciphers are AEAD ciphers for
which the auth tag compare is handled internally by the crypto library,
I don't care so much for the performance aspect. Want best security? Use
AEAD! Want best performance? Use AEAD!
You get the point. Use AEAD ;-)
-Steffan
signature.asc
Description: OpenPGP digital signature
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Re: [Openvpn-devel] [PATCH] Add tls-crypt-v2 test writing metadata
Hi,
On 20-04-2020 12:44, Arne Schwabe wrote:
> ---
> tests/unit_tests/openvpn/test_tls_crypt.c | 44 +--
> 1 file changed, 41 insertions(+), 3 deletions(-)
>
> diff --git a/tests/unit_tests/openvpn/test_tls_crypt.c
> b/tests/unit_tests/openvpn/test_tls_crypt.c
> index b9e3a7a6..91a4d209 100644
> --- a/tests/unit_tests/openvpn/test_tls_crypt.c
> +++ b/tests/unit_tests/openvpn/test_tls_crypt.c
> @@ -72,6 +72,24 @@ static const char *test_client_key = \
> "/Z5wtPCAZ0tOzj4ItTI77fBOYRTfEayzHgEr\n"
> "-END OpenVPN tls-crypt-v2 client key-\n";
>
> +
> +/* Has custom metadata of AABBCCDD (base64) */
> +static const char *test_client_key_metadata= \
> +"-BEGIN OpenVPN tls-crypt-v2 client key-\n"
> +"AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8gISIjJCUmJygpKissLS4v\n"
> +"MDEyMzQ1Njc4OTo7PD0+P0BBQkNERUZHSElKS0xNTk9QUVJTVFVWV1hZWltcXV5f\n"
> +"YGFiY2RlZmdoaWprbG1ub3BxcnN0dXZ3eHl6e3x9fn+AgYKDhIWGh4iJiouMjY6P\n"
> +"kJGSk5SVlpeYmZqbnJ2en6ChoqOkpaanqKmqq6ytrq+wsbKztLW2t7i5uru8vb6/\n"
> +"wMHCw8TFxsfIycrLzM3Oz9DR0tPU1dbX2Nna29zd3t/g4eLj5OXm5+jp6uvs7e7v\n"
> +"8PHy8/T19vf4+fr7/P3+/2ntp1WCqhcLjJQY/igkjNt3Yb6i0neqFkfrOp2UCDcz\n"
> +"6RSJtPLZbvOOKUHk2qwxPYUsFCnz/IWV6/ZiLRrabzUpS8oSN1HS6P7qqAdrHKgf\n"
> +"hVTHasdSf2UdMTPC7HBgnP9Ll0FhKN0h7vSzbbt7QM7wH9mr1ecc/Mt0SYW2lpwA\n"
> +"aJObYGTyk6hTgWm0g/MLrworLrezTqUHBZzVsu+LDyqLWK1lzJNd66MuNOsGA4YF\n"
> +"fbCsDh8n3H+Cw1k5YNBZDYYJOtVUgBWXheO6vgoOmqDdI0dAQ3hVo9DE+SkCFjgf\n"
> +"l4FY2yLEh9ZVZZrl1eD1Owh/X178CkHrBJYl9LNQSyQEKlDGWwBLQ/pY3qtjctr3\n"
> +"pV62MPQdBo+1lcsjDCJVQA6XUyltas4BKQ==\n"
> +"-END OpenVPN tls-crypt-v2 client key-\n";
> +
> int
> __wrap_parse_line(const char *line, char **p, const int n, const char *file,
>const int line_num, int msglevel, struct gc_arena *gc)
> @@ -520,21 +538,40 @@ test_tls_crypt_v2_write_server_key_file(void **state) {
>
> static void
> test_tls_crypt_v2_write_client_key_file(void **state) {
> + const char *filename = "testfilename.key";
> +
> + /* Test writing the client key */
> + expect_string(__wrap_buffer_write_file, filename, filename);
> + expect_string(__wrap_buffer_write_file, pem, test_client_key);
> + will_return(__wrap_buffer_write_file, true);
> +
> + /* Key generation re-reads the created file as a sanity check */
> + expect_string(__wrap_buffer_read_from_file, filename, filename);
> + will_return(__wrap_buffer_read_from_file, test_client_key);
> +
> + tls_crypt_v2_write_client_key_file(filename, NULL, INLINE_FILE_TAG,
> + test_server_key);
> +}
> +
The indenting of this block is wrong: 2 instead of 4 spaces. This also
makes the changes in this patch harder to see.
> +static void
> +test_tls_crypt_v2_write_client_key_file_metadata(void **state) {
> const char *filename = "testfilename.key";
> +const char *b64metadata = "AABBCCDD";
>
> /* Test writing the client key */
> expect_string(__wrap_buffer_write_file, filename, filename);
> -expect_string(__wrap_buffer_write_file, pem, test_client_key);
> +expect_string(__wrap_buffer_write_file, pem, test_client_key_metadata);
> will_return(__wrap_buffer_write_file, true);
>
> /* Key generation re-reads the created file as a sanity check */
> expect_string(__wrap_buffer_read_from_file, filename, filename);
> -will_return(__wrap_buffer_read_from_file, test_client_key);
> +will_return(__wrap_buffer_read_from_file, test_client_key_metadata);
>
> -tls_crypt_v2_write_client_key_file(filename, NULL, INLINE_FILE_TAG,
> +tls_crypt_v2_write_client_key_file(filename, b64metadata,
> INLINE_FILE_TAG,
> test_server_key);
> }
>
> +
> int
> main(void) {
> const struct CMUnitTest tests[] = {
> @@ -576,6 +613,7 @@ main(void) {
> test_tls_crypt_v2_teardown),
> cmocka_unit_test(test_tls_crypt_v2_write_server_key_file),
> cmocka_unit_test(test_tls_crypt_v2_write_client_key_file),
> +cmocka_unit_test(test_tls_crypt_v2_write_client_key_file_metadata),
> };
>
> #if defined(ENABLE_CRYPTO_OPENSSL)
>
Otherwise this looks good. So ACK-if-whitespace-is-fixed :)
Acked-by: Steffan Karger
-Steffan
___
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
