* [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
@ 2020-04-16 11:39 Arne Schwabe
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 2/3] Minor style change to improve code style Arne Schwabe
` (5 more replies)
0 siblings, 6 replies; 20+ messages in thread
From: Arne Schwabe @ 2020-04-16 11:39 UTC (permalink / raw)
To: openvpn-devel
Signed-off-by: Arne Schwabe <arne@...1227...>
---
src/openvpn/crypto.h | 16 +---------------
src/openvpn/crypto_mbedtls.c | 19 +++++++++++++++++++
src/openvpn/crypto_openssl.c | 5 +++++
3 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 18a86ceb..dadf0a90 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -528,21 +528,7 @@ void crypto_read_openvpn_key(const struct key_type *key_type,
* As memcmp(), but constant-time.
* Returns 0 when data is equal, non-zero otherwise.
*/
-static inline int
-memcmp_constant_time(const void *a, const void *b, size_t size)
-{
- const uint8_t *a1 = a;
- const uint8_t *b1 = b;
- int ret = 0;
- size_t i;
-
- for (i = 0; i < size; i++)
- {
- ret |= *a1++ ^ *b1++;
- }
-
- return ret;
-}
+int memcmp_constant_time(const void *a, const void *b, size_t size);
static inline bool
key_ctx_bi_defined(const struct key_ctx_bi *key)
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 3e77fa9e..35072b73 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -972,4 +972,23 @@ hmac_ctx_final(mbedtls_md_context_t *ctx, uint8_t *dst)
ASSERT(0 == mbedtls_md_hmac_finish(ctx, dst));
}
+int
+memcmp_constant_time(const void *a, const void *b, size_t size)
+{
+ /* mbed TLS has a no const time memcmp function that it exposes
+ * via its APIs like OpenSSL does with CRYPTO_memcmp
+ * Adapt the function that mbedtls itself uses in
+ * mbedtls_safer_memcmp as it considers that to be safe */
+ volatile const unsigned char *A = (volatile const unsigned char *) a;
+ volatile const unsigned char *B = (volatile const unsigned char *) b;
+ volatile unsigned char diff = 0;
+
+ for (size_t i = 0; i < size; i++)
+ {
+ unsigned char x = A[i], y = B[i];
+ diff |= x ^ y;
+ }
+
+ return diff;
+}
#endif /* ENABLE_CRYPTO_MBEDTLS */
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index a81dcfd8..9e7ea0ff 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1066,4 +1066,9 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
HMAC_Final(ctx, dst, &in_hmac_len);
}
+int
+memcmp_constant_time(const void *a, const void *b, size_t size)
+{
+ return CRYPTO_memcmp(a, b, size);
+}
#endif /* ENABLE_CRYPTO_OPENSSL */
--
2.26.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Openvpn-devel] [PATCH v2 2/3] Minor style change to improve code style
2020-04-16 11:39 [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Arne Schwabe
@ 2020-04-16 11:39 ` Arne Schwabe
2020-04-17 14:56 ` Antonio Quartulli
2020-04-19 10:32 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again Arne Schwabe
` (4 subsequent siblings)
5 siblings, 2 replies; 20+ messages in thread
From: Arne Schwabe @ 2020-04-16 11:39 UTC (permalink / raw)
To: openvpn-devel
These are small manual changes that are done to improve the code
style and also make the result of uncrustify better without mixing
manual changes/automatic changes into a single commit.
- Make prototype and function identical for gc_addspecial. Also fixes
uncrustify misparsing the embedded function pointer decleration
- Disallow uncrustify to reformat link_socket_init_phase1, which it
messes up
- Format the the parameters of a call of mbedtls_ssl_tls_prf to
be more inline with the rest of our function calls with multiple
arguments
Signed-off-by: Arne Schwabe <arne@...1227...>
---
src/openvpn/buffer.c | 2 +-
src/openvpn/socket.h | 4 +++-
src/openvpn/ssl_mbedtls.c | 8 ++++----
3 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/openvpn/buffer.c b/src/openvpn/buffer.c
index 8575e295..b32bc8b2 100644
--- a/src/openvpn/buffer.c
+++ b/src/openvpn/buffer.c
@@ -474,7 +474,7 @@ x_gc_freespecial(struct gc_arena *a)
}
void
-gc_addspecial(void *addr, void (free_function)(void *), struct gc_arena *a)
+gc_addspecial(void *addr, void (*free_function)(void *), struct gc_arena *a)
{
ASSERT(a);
struct gc_entry_special *e;
diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h
index e95547d1..38e5138d 100644
--- a/src/openvpn/socket.h
+++ b/src/openvpn/socket.h
@@ -296,7 +296,8 @@ int openvpn_connect(socket_descriptor_t sd,
/*
* Initialize link_socket object.
*/
-
+/* *INDENT-OFF* uncrustify misparses this function declarion because of
+ * embedded #if/#endif tell it to skip this section */
void
link_socket_init_phase1(struct link_socket *sock,
const char *local_host,
@@ -327,6 +328,7 @@ link_socket_init_phase1(struct link_socket *sock,
int mark,
struct event_timeout *server_poll_timeout,
unsigned int sockflags);
+/* Reenable uncrustify *INDENT-ON* */
void link_socket_init_phase2(struct link_socket *sock,
const struct frame *frame,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 4f194ad7..d585111b 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -209,10 +209,10 @@ int mbedtls_ssl_export_keys_cb(void *p_expkey, const unsigned char *ms,
memcpy(client_server_random + 32, server_random, 32);
const size_t ms_len = sizeof(ks_ssl->ctx->session->master);
- int ret = mbedtls_ssl_tls_prf(
- tls_prf_type, ms, ms_len, session->opt->ekm_label,
- client_server_random, sizeof(client_server_random),
- ks_ssl->exported_key_material, session->opt->ekm_size);
+ int ret = mbedtls_ssl_tls_prf(tls_prf_type, ms, ms_len,
+ session->opt->ekm_label, client_server_random,
+ sizeof(client_server_random), ks_ssl->exported_key_material,
+ session->opt->ekm_size);
if (!mbed_ok(ret))
{
--
2.26.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again.
2020-04-16 11:39 [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Arne Schwabe
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 2/3] Minor style change to improve code style Arne Schwabe
@ 2020-04-16 11:39 ` Arne Schwabe
2020-04-17 15:01 ` Antonio Quartulli
2020-04-19 10:40 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2020-04-16 14:10 ` [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Antonio Quartulli
` (3 subsequent siblings)
5 siblings, 2 replies; 20+ messages in thread
From: Arne Schwabe @ 2020-04-16 11:39 UTC (permalink / raw)
To: openvpn-devel
To bring everything back to the agreed upon style, run uncrustify once
more. Uncrustify version used:
Uncrustify-0.70.1_f
I double checked the result by running uncrustify (Uncrustify-0.69.0_f)
from Ubuntu focal/20.04 which does not do any further changes and
uncrustify 0.66.1_f from Ubuntu bionic/18.04
Signed-off-by: Arne Schwabe <arne@...1227...>
---
src/compat/compat-strsep.c | 2 +-
src/compat/compat.h | 3 ++-
src/openvpn/crypto.c | 9 +++++----
src/openvpn/cryptoapi.c | 5 +++--
src/openvpn/forward.c | 2 +-
src/openvpn/forward.h | 2 +-
src/openvpn/manage.c | 6 +++---
src/openvpn/misc.c | 2 +-
src/openvpn/mroute.c | 2 +-
src/openvpn/networking.h | 6 +++---
src/openvpn/networking_iproute2.c | 14 ++++++++++++++
src/openvpn/networking_sitnl.h | 2 +-
src/openvpn/openvpn.h | 2 +-
src/openvpn/options.c | 10 ++++++----
src/openvpn/options.h | 4 ++--
src/openvpn/proto.h | 2 +-
src/openvpn/push.c | 20 ++++++++++----------
src/openvpn/route.c | 2 +-
src/openvpn/ssl.c | 6 ++++--
src/openvpn/ssl.h | 1 +
src/openvpn/ssl_mbedtls.c | 15 ++++++++-------
src/openvpn/ssl_openssl.c | 28 ++++++++++++++--------------
src/openvpn/ssl_verify.c | 18 +++++++++---------
src/openvpn/ssl_verify.h | 3 ++-
src/openvpn/vlan.c | 4 ++--
src/openvpn/win32.h | 2 +-
26 files changed, 98 insertions(+), 74 deletions(-)
diff --git a/src/compat/compat-strsep.c b/src/compat/compat-strsep.c
index 42ff6414..e6518db6 100644
--- a/src/compat/compat-strsep.c
+++ b/src/compat/compat-strsep.c
@@ -58,4 +58,4 @@ strsep(char **stringp, const char *delim)
}
return begin;
}
-#endif
+#endif /* ifndef HAVE_STRSEP */
diff --git a/src/compat/compat.h b/src/compat/compat.h
index 592881df..a66a4235 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -71,7 +71,8 @@ int inet_pton(int af, const char *src, void *dst);
#endif
#ifndef HAVE_STRSEP
-char* strsep(char **stringp, const char *delim);
+char *strsep(char **stringp, const char *delim);
+
#endif
#endif /* COMPAT_H */
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 453cb20a..1678cba8 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -736,13 +736,14 @@ crypto_max_overhead(void)
+max_int(OPENVPN_MAX_HMAC_SIZE, OPENVPN_AEAD_TAG_LENGTH);
}
-static void warn_insecure_key_type(const char* ciphername, const cipher_kt_t *cipher)
+static void
+warn_insecure_key_type(const char *ciphername, const cipher_kt_t *cipher)
{
if (cipher_kt_insecure(cipher))
{
msg(M_WARN, "WARNING: INSECURE cipher (%s) with block size less than 128"
- " bit (%d bit). This allows attacks like SWEET32. Mitigate by "
- "using a --cipher with a larger block size (e.g. AES-256-CBC).",
+ " bit (%d bit). This allows attacks like SWEET32. Mitigate by "
+ "using a --cipher with a larger block size (e.g. AES-256-CBC).",
ciphername, cipher_kt_block_size(cipher)*8);
}
}
@@ -846,7 +847,7 @@ init_key_ctx(struct key_ctx *ctx, const struct key *key,
cipher_ctx_init(ctx->cipher, key->cipher, kt->cipher_length,
kt->cipher, enc);
- const char* ciphername = translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher));
+ const char *ciphername = translate_cipher_name_to_openvpn(cipher_kt_name(kt->cipher));
msg(D_HANDSHAKE, "%s: Cipher '%s' initialized with %d bit key",
prefix,
ciphername,
diff --git a/src/openvpn/cryptoapi.c b/src/openvpn/cryptoapi.c
index 30eba7b2..6c4df9e3 100644
--- a/src/openvpn/cryptoapi.c
+++ b/src/openvpn/cryptoapi.c
@@ -803,12 +803,13 @@ find_certificate_in_store(const char *cert_prop, HCERTSTORE cert_store)
}
blob.cbData = i;
}
- else {
+ else
+ {
msg(M_WARN, "WARNING: cryptoapicert: unsupported certificate specification <%s>", cert_prop);
goto out;
}
- while(true)
+ while (true)
{
int validity = 1;
/* this frees previous rv, if not NULL */
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index ea10f0bf..2082b9ea 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1278,7 +1278,7 @@ read_incoming_tun(struct context *c)
ASSERT(buf_init(&c->c2.buf, FRAME_HEADROOM(&c->c2.frame)));
ASSERT(buf_safe(&c->c2.buf, MAX_RW_SIZE_TUN(&c->c2.frame)));
c->c2.buf.len = read_tun(c->c1.tuntap, BPTR(&c->c2.buf), MAX_RW_SIZE_TUN(&c->c2.frame));
-#endif
+#endif /* ifdef _WIN32 */
#ifdef PACKET_TRUNCATION_CHECK
ipv4_packet_size_verify(BPTR(&c->c2.buf),
diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h
index b711ff00..ff898133 100644
--- a/src/openvpn/forward.h
+++ b/src/openvpn/forward.h
@@ -434,7 +434,7 @@ io_wait(struct context *c, const unsigned int flags)
c->c2.event_set_status = ret;
}
else
-#endif
+#endif /* ifdef _WIN32 */
{
/* slow path */
io_wait_dowork(c, flags);
diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
index 49864c0a..195941ca 100644
--- a/src/openvpn/manage.c
+++ b/src/openvpn/manage.c
@@ -3660,9 +3660,9 @@ management_query_pk_sig(struct management *man, const char *b64_data,
buf_write(&buf_data, ",", (int) strlen(","));
buf_write(&buf_data, algorithm, (int) strlen(algorithm));
}
- char* ret = management_query_multiline_flatten(man,
- (char *)buf_bptr(&buf_data), prompt, desc,
- &man->connection.ext_key_state, &man->connection.ext_key_input);
+ char *ret = management_query_multiline_flatten(man,
+ (char *)buf_bptr(&buf_data), prompt, desc,
+ &man->connection.ext_key_state, &man->connection.ext_key_input);
free_buf(&buf_data);
return ret;
}
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 1c17948c..a10888ed 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -146,7 +146,7 @@ auth_user_pass_mgmt(struct user_pass *up, const char *prefix, const unsigned int
}
return true;
}
-#endif
+#endif /* ifdef ENABLE_MANAGEMENT */
/*
* Get and store a username/password
diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
index bdb1b0c0..a7e78213 100644
--- a/src/openvpn/mroute.c
+++ b/src/openvpn/mroute.c
@@ -324,7 +324,7 @@ mroute_extract_addr_ether(struct mroute_addr *src,
break;
}
}
-#endif
+#endif /* ifdef ENABLE_PF */
}
return ret;
}
diff --git a/src/openvpn/networking.h b/src/openvpn/networking.h
index 5e6d898f..9c1d1696 100644
--- a/src/openvpn/networking.h
+++ b/src/openvpn/networking.h
@@ -31,8 +31,8 @@ struct context;
#include "networking_iproute2.h"
#else
/* define mock types to ensure code builds on any platform */
-typedef void * openvpn_net_ctx_t;
-typedef void * openvpn_net_iface_t;
+typedef void *openvpn_net_ctx_t;
+typedef void *openvpn_net_iface_t;
static inline int
net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx)
@@ -51,7 +51,7 @@ net_ctx_free(openvpn_net_ctx_t *ctx)
{
(void)ctx;
}
-#endif
+#endif /* ifdef ENABLE_SITNL */
#if defined(ENABLE_SITNL) || defined(ENABLE_IPROUTE)
diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c
index 0f9e899a..f3b9c614 100644
--- a/src/openvpn/networking_iproute2.c
+++ b/src/openvpn/networking_iproute2.c
@@ -43,7 +43,9 @@ net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx)
{
ctx->es = NULL;
if (c)
+ {
ctx->es = c->es;
+ }
ctx->gc = gc_new();
return 0;
@@ -207,10 +209,14 @@ net_route_v4_add(openvpn_net_ctx_t *ctx, const in_addr_t *dst, int prefixlen,
argv_printf(&argv, "%s route add %s/%d", iproute_path, dst_str, prefixlen);
if (metric > 0)
+ {
argv_printf_cat(&argv, "metric %d", metric);
+ }
if (iface)
+ {
argv_printf_cat(&argv, "dev %s", iface);
+ }
if (gw)
{
@@ -246,7 +252,9 @@ net_route_v6_add(openvpn_net_ctx_t *ctx, const struct in6_addr *dst,
}
if (metric > 0)
+ {
argv_printf_cat(&argv, "metric %d", metric);
+ }
argv_msg(D_ROUTE, &argv);
openvpn_execve_check(&argv, ctx->es, 0, "ERROR: Linux route -6 add command failed");
@@ -267,7 +275,9 @@ net_route_v4_del(openvpn_net_ctx_t *ctx, const in_addr_t *dst, int prefixlen,
argv_printf(&argv, "%s route del %s/%d", iproute_path, dst_str, prefixlen);
if (metric > 0)
+ {
argv_printf_cat(&argv, "metric %d", metric);
+ }
argv_msg(D_ROUTE, &argv);
openvpn_execve_check(&argv, ctx->es, 0, "ERROR: Linux route delete command failed");
@@ -296,7 +306,9 @@ net_route_v6_del(openvpn_net_ctx_t *ctx, const struct in6_addr *dst,
}
if (metric > 0)
+ {
argv_printf_cat(&argv, "metric %d", metric);
+ }
argv_msg(D_ROUTE, &argv);
openvpn_execve_check(&argv, ctx->es, 0, "ERROR: Linux route -6 del command failed");
@@ -314,7 +326,9 @@ net_route_v4_best_gw(openvpn_net_ctx_t *ctx, const in_addr_t *dst,
FILE *fp = fopen("/proc/net/route", "r");
if (!fp)
+ {
return -1;
+ }
char line[256];
int count = 0;
diff --git a/src/openvpn/networking_sitnl.h b/src/openvpn/networking_sitnl.h
index f39d426d..6396b06e 100644
--- a/src/openvpn/networking_sitnl.h
+++ b/src/openvpn/networking_sitnl.h
@@ -23,6 +23,6 @@
#define NETWORKING_SITNL_H_
typedef char openvpn_net_iface_t;
-typedef void * openvpn_net_ctx_t;
+typedef void *openvpn_net_ctx_t;
#endif /* NETWORKING_SITNL_H_ */
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index 900db7e1..595a9b1d 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -524,7 +524,7 @@ struct context
struct env_set *es; /**< Set of environment variables. */
- openvpn_net_ctx_t net_ctx; /**< Networking API opaque context */
+ openvpn_net_ctx_t net_ctx; /**< Networking API opaque context */
struct signal_info *sig; /**< Internal error signaling object. */
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 49df8df1..63dc53c3 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -1241,8 +1241,10 @@ print_vlan_accept(enum vlan_acceptable_frames mode)
{
case VLAN_ONLY_TAGGED:
return "tagged";
+
case VLAN_ONLY_UNTAGGED_OR_PRIORITY:
return "untagged";
+
case VLAN_ALL:
return "all";
}
@@ -1320,7 +1322,7 @@ show_p2mp_parms(const struct options *o)
SHOW_STR(port_share_port);
#endif
SHOW_BOOL(vlan_tagging);
- msg(D_SHOW_PARMS, " vlan_accept = %s", print_vlan_accept (o->vlan_accept));
+ msg(D_SHOW_PARMS, " vlan_accept = %s", print_vlan_accept(o->vlan_accept));
SHOW_INT(vlan_pvid);
#endif /* P2MP_SERVER */
@@ -5301,7 +5303,7 @@ add_option(struct options *options,
options->management_flags |= MF_EXTERNAL_CERT;
options->management_certificate = p[1];
}
-#endif
+#endif /* ifdef ENABLE_MANAGEMENT */
#ifdef MANAGEMENT_DEF_AUTH
else if (streq(p[0], "management-client-auth") && !p[1])
{
@@ -7711,8 +7713,8 @@ add_option(struct options *options,
}
else
{
- if (streq(p[1], "secret") || streq(p[1], "tls-auth") ||
- streq(p[1], "tls-crypt"))
+ if (streq(p[1], "secret") || streq(p[1], "tls-auth")
+ || streq(p[1], "tls-crypt"))
{
options->genkey_type = GENKEY_SECRET;
}
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 2f1f6faf..4c1737e1 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -222,8 +222,8 @@ struct options
bool show_curves;
bool genkey;
enum genkey_type genkey_type;
- const char* genkey_filename;
- const char* genkey_extra_data;
+ const char *genkey_filename;
+ const char *genkey_extra_data;
/* Networking parms */
int connect_retry_max;
diff --git a/src/openvpn/proto.h b/src/openvpn/proto.h
index c1ff3e14..c2517674 100644
--- a/src/openvpn/proto.h
+++ b/src/openvpn/proto.h
@@ -67,7 +67,7 @@ struct openvpn_ethhdr
struct openvpn_8021qhdr
{
uint8_t dest[OPENVPN_ETH_ALEN]; /* destination ethernet addr */
- uint8_t source[OPENVPN_ETH_ALEN]; /* source ethernet addr */
+ uint8_t source[OPENVPN_ETH_ALEN]; /* source ethernet addr */
uint16_t tpid; /* 802.1Q Tag Protocol Identifier */
#define OPENVPN_8021Q_MASK_PCP htons(0xE000) /* mask PCP out of pcp_cfi_vid */
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index aef00d34..39a906d4 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -72,19 +72,19 @@ receive_auth_failed(struct context *c, const struct buffer *buffer)
{
switch (auth_retry_get())
{
- case AR_NONE:
- c->sig->signal_received = SIGTERM; /* SOFT-SIGTERM -- Auth failure error */
- break;
+ case AR_NONE:
+ c->sig->signal_received = SIGTERM; /* SOFT-SIGTERM -- Auth failure error */
+ break;
- case AR_INTERACT:
- ssl_purge_auth(false);
+ case AR_INTERACT:
+ ssl_purge_auth(false);
- case AR_NOINTERACT:
- c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
- break;
+ case AR_NOINTERACT:
+ c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
+ break;
- default:
- ASSERT(0);
+ default:
+ ASSERT(0);
}
c->sig->signal_text = "auth-failure";
}
diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index e0f8d201..51f76318 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -2152,7 +2152,7 @@ delete_route(struct route_ipv4 *r,
#if !defined(TARGET_ANDROID)
const char *gateway;
#endif
-#else
+#else /* if !defined(TARGET_LINUX) */
int metric;
#endif
int is_local_route;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 56d0576a..80e0d5ac 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -466,7 +466,7 @@ ssl_set_auth_token(const char *token)
* Cleans an auth token and checks if it was active
*/
bool
-ssl_clean_auth_token (void)
+ssl_clean_auth_token(void)
{
bool wasdefined = auth_token.defined;
purge_user_pass(&auth_token, true);
@@ -2015,7 +2015,7 @@ tls_session_update_crypto_params(struct tls_session *session,
{
frame_remove_from_extra_frame(frame_fragment, crypto_max_overhead());
crypto_adjust_frame_parameters(frame_fragment, &session->opt->key_type,
- options->replay, packet_id_long_form);
+ options->replay, packet_id_long_form);
frame_set_mtu_dynamic(frame_fragment, options->ce.fragment, SET_MTU_UPPER_BOUND);
frame_print(frame_fragment, D_MTU_INFO, "Fragmentation MTU parms");
}
@@ -2411,7 +2411,9 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
* username/password
*/
if (auth_token.defined)
+ {
up = &auth_token;
+ }
if (!write_string(buf, up->username, -1))
{
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index f0a8ef54..2f6f7657 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -607,4 +607,5 @@ void
show_available_tls_ciphers(const char *cipher_list,
const char *cipher_list_tls13,
const char *tls_cert_profile);
+
#endif /* ifndef OPENVPN_SSL_H */
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index d585111b..1f91b785 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -191,12 +191,13 @@ tls_ctx_initialised(struct tls_root_ctx *ctx)
}
#ifdef HAVE_EXPORT_KEYING_MATERIAL
-int mbedtls_ssl_export_keys_cb(void *p_expkey, const unsigned char *ms,
- const unsigned char *kb, size_t maclen,
- size_t keylen, size_t ivlen,
- const unsigned char client_random[32],
- const unsigned char server_random[32],
- mbedtls_tls_prf_types tls_prf_type)
+int
+mbedtls_ssl_export_keys_cb(void *p_expkey, const unsigned char *ms,
+ const unsigned char *kb, size_t maclen,
+ size_t keylen, size_t ivlen,
+ const unsigned char client_random[32],
+ const unsigned char server_random[32],
+ mbedtls_tls_prf_types tls_prf_type)
{
struct tls_session *session = p_expkey;
struct key_state_ssl *ks_ssl = &session->key[KS_PRIMARY].ks_ssl;
@@ -1126,7 +1127,7 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
if (session->opt->ekm_size)
{
mbedtls_ssl_conf_export_keys_ext_cb(ks_ssl->ssl_config,
- mbedtls_ssl_export_keys_cb, session);
+ mbedtls_ssl_export_keys_cb, session);
}
#endif
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index d7bd6aa2..5955c6bd 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -683,7 +683,7 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
* so do nothing */
#endif
return;
-#else
+#else /* if OPENSSL_VERSION_NUMBER >= 0x10002000L */
/* For older OpenSSL we have to extract the curve from key on our own */
EC_KEY *eckey = NULL;
const EC_GROUP *ecgrp = NULL;
@@ -1173,7 +1173,7 @@ openvpn_extkey_rsa_finish(RSA *rsa)
* interface query
*/
const char *
-get_rsa_padding_name (const int padding)
+get_rsa_padding_name(const int padding)
{
switch (padding)
{
@@ -1190,14 +1190,14 @@ get_rsa_padding_name (const int padding)
/**
* Pass the input hash in 'dgst' to management and get the signature back.
- *
- * @param dgst hash to be signed
- * @param dgstlen len of data in dgst
- * @param sig On successful return signature is in sig.
- * @param siglen length of buffer sig
- * @param algorithm padding/hashing algorithm for the signature
*
- * @return signature length or -1 on error.
+ * @param dgst hash to be signed
+ * @param dgstlen len of data in dgst
+ * @param sig On successful return signature is in sig.
+ * @param siglen length of buffer sig
+ * @param algorithm padding/hashing algorithm for the signature
+ *
+ * @return signature length or -1 on error.
*/
static int
get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,
@@ -1239,7 +1239,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,
return -1;
}
- ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name (padding));
+ ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name(padding));
return (ret == len) ? ret : -1;
}
@@ -1314,7 +1314,7 @@ err:
}
#if ((OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) \
- || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
+ || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
&& !defined(OPENSSL_NO_EC)
/* called when EC_KEY is destroyed */
@@ -1475,7 +1475,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
}
}
#if ((OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) \
- || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
+ || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
&& !defined(OPENSSL_NO_EC)
else if (EVP_PKEY_id(pkey) == EVP_PKEY_EC)
{
@@ -2135,8 +2135,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
crypto_msg(M_FATAL, "Cannot create SSL object");
}
-#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) || \
- (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <= 0x2090000fL)
+#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) \
+ || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <= 0x2090000fL)
STACK_OF(SSL_CIPHER) *sk = SSL_get_ciphers(ssl);
#else
STACK_OF(SSL_CIPHER) *sk = SSL_get1_supported_ciphers(ssl);
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index da0966c5..9362b8e9 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -804,7 +804,7 @@ cleanup:
#endif
void
-auth_set_client_reason(struct tls_multi* multi, const char* client_reason)
+auth_set_client_reason(struct tls_multi *multi, const char *client_reason)
{
if (multi->client_reason)
{
@@ -1204,7 +1204,7 @@ verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,
static int
verify_user_pass_management(struct tls_session *session,
- struct tls_multi* multi,
+ struct tls_multi *multi,
const struct user_pass *up)
{
int retval = KMDA_ERROR;
@@ -1301,16 +1301,16 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
* for equality with AUTH_TOKEN_HMAC_OK
*/
msg(M_WARN, "TLS: Username/auth-token authentication "
- "succeeded for username '%s'",
+ "succeeded for username '%s'",
up->username);
- skip_auth = true;
+ skip_auth = true;
}
else
{
wipe_auth_token(multi);
ks->authenticated = false;
msg(M_WARN, "TLS: Username/auth-token authentication "
- "failed for username '%s'", up->username);
+ "failed for username '%s'", up->username);
return;
}
}
@@ -1335,12 +1335,12 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
}
/* check sizing of username if it will become our common name */
- if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) &&
- strlen(up->username)>TLS_USERNAME_LEN)
+ if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
+ && strlen(up->username)>TLS_USERNAME_LEN)
{
msg(D_TLS_ERRORS,
- "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
- TLS_USERNAME_LEN);
+ "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
+ TLS_USERNAME_LEN);
s1 = OPENVPN_PLUGIN_FUNC_ERROR;
}
/* auth succeeded? */
diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
index c54b89a6..21b37a0f 100644
--- a/src/openvpn/ssl_verify.h
+++ b/src/openvpn/ssl_verify.h
@@ -234,7 +234,8 @@ bool tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id
* @param multi The multi tls struct
* @param client_reason The string to send to the client as part of AUTH_FAILED
*/
-void auth_set_client_reason(struct tls_multi* multi, const char* client_reason);
+void auth_set_client_reason(struct tls_multi *multi, const char *client_reason);
+
#endif
static inline const char *
diff --git a/src/openvpn/vlan.c b/src/openvpn/vlan.c
index a5885de2..9290179d 100644
--- a/src/openvpn/vlan.c
+++ b/src/openvpn/vlan.c
@@ -58,7 +58,7 @@ static void
vlanhdr_set_vid(struct openvpn_8021qhdr *hdr, const uint16_t vid)
{
hdr->pcp_cfi_vid = (hdr->pcp_cfi_vid & ~OPENVPN_8021Q_MASK_VID)
- | (htons(vid) & OPENVPN_8021Q_MASK_VID);
+ | (htons(vid) & OPENVPN_8021Q_MASK_VID);
}
/*
@@ -135,7 +135,7 @@ vlan_decapsulate(const struct context *c, struct buffer *buf)
goto drop;
}
- /* vid == 0 means prio-tagged packet: don't drop and fall-through */
+ /* vid == 0 means prio-tagged packet: don't drop and fall-through */
case VLAN_ONLY_TAGGED:
case VLAN_ALL:
/* tagged frame can be accepted: extract vid and strip encapsulation */
diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
index 4b508c56..79504776 100644
--- a/src/openvpn/win32.h
+++ b/src/openvpn/win32.h
@@ -69,7 +69,7 @@ struct security_attributes
struct window_title
{
bool saved;
- char old_window_title [256];
+ char old_window_title[256];
};
struct rw_handle {
--
2.26.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-16 11:39 [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Arne Schwabe
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 2/3] Minor style change to improve code style Arne Schwabe
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again Arne Schwabe
@ 2020-04-16 14:10 ` Antonio Quartulli
2020-04-17 7:53 ` Arne Schwabe
` (2 subsequent siblings)
5 siblings, 0 replies; 20+ messages in thread
From: Antonio Quartulli @ 2020-04-16 14:10 UTC (permalink / raw)
To: Arne Schwabe <arne@
Hi,
On 16/04/2020 13:39, Arne Schwabe wrote:
> Signed-off-by: Arne Schwabe <arne@...1227...>
> ---
> src/openvpn/crypto.h | 16 +---------------
> src/openvpn/crypto_mbedtls.c | 19 +++++++++++++++++++
> src/openvpn/crypto_openssl.c | 5 +++++
> 3 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
> index 18a86ceb..dadf0a90 100644
> --- a/src/openvpn/crypto.h
> +++ b/src/openvpn/crypto.h
> @@ -528,21 +528,7 @@ void crypto_read_openvpn_key(const struct key_type *key_type,
> * As memcmp(), but constant-time.
> * Returns 0 when data is equal, non-zero otherwise.
> */
> -static inline int
> -memcmp_constant_time(const void *a, const void *b, size_t size)
> -{
> - const uint8_t *a1 = a;
> - const uint8_t *b1 = b;
> - int ret = 0;
> - size_t i;
> -
> - for (i = 0; i < size; i++)
> - {
> - ret |= *a1++ ^ *b1++;
> - }
> -
> - return ret;
> -}
> +int memcmp_constant_time(const void *a, const void *b, size_t size);
>
> static inline bool
> key_ctx_bi_defined(const struct key_ctx_bi *key)
> diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
> index 3e77fa9e..35072b73 100644
> --- a/src/openvpn/crypto_mbedtls.c
> +++ b/src/openvpn/crypto_mbedtls.c
> @@ -972,4 +972,23 @@ hmac_ctx_final(mbedtls_md_context_t *ctx, uint8_t *dst)
> ASSERT(0 == mbedtls_md_hmac_finish(ctx, dst));
> }
>
> +int
> +memcmp_constant_time(const void *a, const void *b, size_t size)
> +{
> + /* mbed TLS has a no const time memcmp function that it exposes
> + * via its APIs like OpenSSL does with CRYPTO_memcmp
'.' missing at the end of the sentence.
Actually, I'd just chop the last part and stop after 'memcmp'.
> + * Adapt the function that mbedtls itself uses in
> + * mbedtls_safer_memcmp as it considers that to be safe */
> + volatile const unsigned char *A = (volatile const unsigned char *) a;
no space after cast
> + volatile const unsigned char *B = (volatile const unsigned char *) b;
same
> + volatile unsigned char diff = 0;
> +
> + for (size_t i = 0; i < size; i++)
> + {
> + unsigned char x = A[i], y = B[i];
> + diff |= x ^ y;
After a discussion on IRC, it was noted that this conversion from
volatile to non-volatile was introduced by mbedTLS devs to avoid
warnings with the IAR compiler.
Since we don't want to change the function but use it as it is, we
should still add a comment to avoid somebody in the future from
"cleaning this up".
maybe something like:
/* this conversion was introduced by mbedTLS to suppress a IAR
* compiler warning. keep it as it is.
*/
> + }
> +
> + return diff;
> +}
> #endif /* ENABLE_CRYPTO_MBEDTLS */
> diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
> index a81dcfd8..9e7ea0ff 100644
> --- a/src/openvpn/crypto_openssl.c
> +++ b/src/openvpn/crypto_openssl.c
> @@ -1066,4 +1066,9 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
> HMAC_Final(ctx, dst, &in_hmac_len);
> }
>
> +int
> +memcmp_constant_time(const void *a, const void *b, size_t size)
> +{
> + return CRYPTO_memcmp(a, b, size);
> +}
> #endif /* ENABLE_CRYPTO_OPENSSL */
>
Cheers,
--
Antonio Quartulli
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-16 11:39 [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Arne Schwabe
` (2 preceding siblings ...)
2020-04-16 14:10 ` [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Antonio Quartulli
@ 2020-04-17 7:53 ` Arne Schwabe
2020-04-17 8:51 ` Gert Doering
2020-05-07 13:21 ` [Openvpn-devel] [PATCH applied] " Gert Doering
5 siblings, 0 replies; 20+ messages in thread
From: Arne Schwabe @ 2020-04-17 7:53 UTC (permalink / raw)
To: openvpn-devel
Signed-off-by: Arne Schwabe <arne@...1227...>
---
src/openvpn/crypto.h | 16 +---------------
src/openvpn/crypto_mbedtls.c | 20 ++++++++++++++++++++
src/openvpn/crypto_openssl.c | 5 +++++
3 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/src/openvpn/crypto.h b/src/openvpn/crypto.h
index 18a86ceb..dadf0a90 100644
--- a/src/openvpn/crypto.h
+++ b/src/openvpn/crypto.h
@@ -528,21 +528,7 @@ void crypto_read_openvpn_key(const struct key_type *key_type,
* As memcmp(), but constant-time.
* Returns 0 when data is equal, non-zero otherwise.
*/
-static inline int
-memcmp_constant_time(const void *a, const void *b, size_t size)
-{
- const uint8_t *a1 = a;
- const uint8_t *b1 = b;
- int ret = 0;
- size_t i;
-
- for (i = 0; i < size; i++)
- {
- ret |= *a1++ ^ *b1++;
- }
-
- return ret;
-}
+int memcmp_constant_time(const void *a, const void *b, size_t size);
static inline bool
key_ctx_bi_defined(const struct key_ctx_bi *key)
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index 3e77fa9e..1f6a23f8 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -972,4 +972,24 @@ hmac_ctx_final(mbedtls_md_context_t *ctx, uint8_t *dst)
ASSERT(0 == mbedtls_md_hmac_finish(ctx, dst));
}
+int
+memcmp_constant_time(const void *a, const void *b, size_t size)
+{
+ /* mbed TLS has a no const time memcmp function.
+ * Adapt the function mbedtls_safer_memcmp that mbedtls
+ * internally uses as it considers that to be safe. */
+ volatile const unsigned char *A = (volatile const unsigned char *)a;
+ volatile const unsigned char *B = (volatile const unsigned char *)b;
+ volatile unsigned char diff = 0;
+
+ for (size_t i = 0; i < size; i++)
+ {
+ /* this conversion was introduced by mbedTLS to suppress a IAR
+ * compiler warning. We keep it as it is. */
+ unsigned char x = A[i], y = B[i];
+ diff |= x ^ y;
+ }
+
+ return diff;
+}
#endif /* ENABLE_CRYPTO_MBEDTLS */
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index a81dcfd8..9e7ea0ff 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -1066,4 +1066,9 @@ hmac_ctx_final(HMAC_CTX *ctx, uint8_t *dst)
HMAC_Final(ctx, dst, &in_hmac_len);
}
+int
+memcmp_constant_time(const void *a, const void *b, size_t size)
+{
+ return CRYPTO_memcmp(a, b, size);
+}
#endif /* ENABLE_CRYPTO_OPENSSL */
--
2.26.0
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-16 11:39 [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Arne Schwabe
` (3 preceding siblings ...)
2020-04-17 7:53 ` Arne Schwabe
@ 2020-04-17 8:51 ` Gert Doering
2020-04-17 13:42 ` Antonio Quartulli
2020-05-07 13:21 ` [Openvpn-devel] [PATCH applied] " Gert Doering
5 siblings, 1 reply; 20+ messages in thread
From: Gert Doering @ 2020-04-17 8:51 UTC (permalink / raw)
To: Arne Schwabe <arne@; +Cc: openvpn-devel
[-- Attachment #1: Type: text/plain, Size: 1050 bytes --]
Hi,
On Thu, Apr 16, 2020 at 01:39:28PM +0200, Arne Schwabe wrote:
> index 18a86ceb..dadf0a90 100644
> --- a/src/openvpn/crypto.h
> +++ b/src/openvpn/crypto.h
> @@ -528,21 +528,7 @@ void crypto_read_openvpn_key(const struct key_type *key_type,
> * As memcmp(), but constant-time.
> * Returns 0 when data is equal, non-zero otherwise.
> */
> -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.
Also, keeping the "inline" part would be good... this is in the per-packet
path.
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 gert@...1296...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-17 8:51 ` Gert Doering
@ 2020-04-17 13:42 ` Antonio Quartulli
2020-04-17 15:36 ` Gert Doering
0 siblings, 1 reply; 20+ messages in thread
From: Antonio Quartulli @ 2020-04-17 13:42 UTC (permalink / raw)
To: Gert Doering <gert@; +Cc: openvpn-devel
On 17/04/2020 10:51, Gert Doering wrote:
> Hi,
>
> On Thu, Apr 16, 2020 at 01:39:28PM +0200, Arne Schwabe wrote:
>> index 18a86ceb..dadf0a90 100644
>> --- a/src/openvpn/crypto.h
>> +++ b/src/openvpn/crypto.h
>> @@ -528,21 +528,7 @@ void crypto_read_openvpn_key(const struct key_type *key_type,
>> * As memcmp(), but constant-time.
>> * Returns 0 when data is equal, non-zero otherwise.
>> */
>> -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 :)
On top of that, this is basically allowing us to re-use an existing
openssl API when possible.
>
> 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.....
Regards,
--
Antonio Quartulli
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 2/3] Minor style change to improve code style
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 2/3] Minor style change to improve code style Arne Schwabe
@ 2020-04-17 14:56 ` Antonio Quartulli
2020-04-19 10:32 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 0 replies; 20+ messages in thread
From: Antonio Quartulli @ 2020-04-17 14:56 UTC (permalink / raw)
To: Arne Schwabe <arne@
Hi,
On 16/04/2020 13:39, Arne Schwabe wrote:
> These are small manual changes that are done to improve the code
> style and also make the result of uncrustify better without mixing
> manual changes/automatic changes into a single commit.
>
> - Make prototype and function identical for gc_addspecial. Also fixes
> uncrustify misparsing the embedded function pointer decleration
> - Disallow uncrustify to reformat link_socket_init_phase1, which it
> messes up
> - Format the the parameters of a call of mbedtls_ssl_tls_prf to
> be more inline with the rest of our function calls with multiple
> arguments
>
> Signed-off-by: Arne Schwabe <arne@...1227...>
[CUT]
> -
> +/* *INDENT-OFF* uncrustify misparses this function declarion because of
> + * embedded #if/#endif tell it to skip this section */
> void
> link_socket_init_phase1(struct link_socket *sock,
> const char *local_host,
> @@ -327,6 +328,7 @@ link_socket_init_phase1(struct link_socket *sock,
> int mark,
> struct event_timeout *server_poll_timeout,
> unsigned int sockflags);
> +/* Reenable uncrustify *INDENT-ON* */
I hate the idea that we have to live with these markers in the code, but
it seems the most reasonable option for now.
Later on we should better refactor this function, because if it's
confusing for the parser...it means it's confusing -period-
>
> void link_socket_init_phase2(struct link_socket *sock,
> const struct frame *frame,
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index 4f194ad7..d585111b 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -209,10 +209,10 @@ int mbedtls_ssl_export_keys_cb(void *p_expkey, const unsigned char *ms,
> memcpy(client_server_random + 32, server_random, 32);
>
> const size_t ms_len = sizeof(ks_ssl->ctx->session->master);
> - int ret = mbedtls_ssl_tls_prf(
> - tls_prf_type, ms, ms_len, session->opt->ekm_label,
> - client_server_random, sizeof(client_server_random),
> - ks_ssl->exported_key_material, session->opt->ekm_size);
> + int ret = mbedtls_ssl_tls_prf(tls_prf_type, ms, ms_len,
> + session->opt->ekm_label, client_server_random,
> + sizeof(client_server_random), ks_ssl->exported_key_material,
> + session->opt->ekm_size);
>
> if (!mbed_ok(ret))
> {
>
Other than that it looks good!
Acked-by: Antonio Quartulli <antonio@...515...>
(I am no sure this can be merged without 1/3 applied, but I am acking
anyway)
Regards,
--
Antonio Quartulli
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again.
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again Arne Schwabe
@ 2020-04-17 15:01 ` Antonio Quartulli
2020-04-19 10:27 ` Gert Doering
2020-04-19 10:40 ` [Openvpn-devel] [PATCH applied] " Gert Doering
1 sibling, 1 reply; 20+ messages in thread
From: Antonio Quartulli @ 2020-04-17 15:01 UTC (permalink / raw)
To: Arne Schwabe <arne@
Hi,
On 16/04/2020 13:39, Arne Schwabe wrote:
[CUT]
> diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c
> index 49864c0a..195941ca 100644
> --- a/src/openvpn/manage.c
> +++ b/src/openvpn/manage.c
> @@ -3660,9 +3660,9 @@ management_query_pk_sig(struct management *man, const char *b64_data,
> buf_write(&buf_data, ",", (int) strlen(","));
> buf_write(&buf_data, algorithm, (int) strlen(algorithm));
> }
> - char* ret = management_query_multiline_flatten(man,
> - (char *)buf_bptr(&buf_data), prompt, desc,
> - &man->connection.ext_key_state, &man->connection.ext_key_input);
> + char *ret = management_query_multiline_flatten(man,
> + (char *)buf_bptr(&buf_data), prompt, desc,
why not moving some arguments on the first line ? One or two should fit.
(IMHO it can be done in this patch - it's still about restyling)
> + &man->connection.ext_key_state, &man->connection.ext_key_input);
> free_buf(&buf_data);
> return ret;
> }
> diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
> index 1c17948c..a10888ed 100644
> --- a/src/openvpn/misc.c
> +++ b/src/openvpn/misc.c
> @@ -146,7 +146,7 @@ auth_user_pass_mgmt(struct user_pass *up, const char *prefix, const unsigned int
> }
> return true;
> }
> -#endif
> +#endif /* ifdef ENABLE_MANAGEMENT */
>
> /*
> * Get and store a username/password
> diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
> index bdb1b0c0..a7e78213 100644
> --- a/src/openvpn/mroute.c
> +++ b/src/openvpn/mroute.c
> @@ -324,7 +324,7 @@ mroute_extract_addr_ether(struct mroute_addr *src,
> break;
> }
> }
> -#endif
> +#endif /* ifdef ENABLE_PF */
> }
> return ret;
> }
> diff --git a/src/openvpn/networking.h b/src/openvpn/networking.h
> index 5e6d898f..9c1d1696 100644
> --- a/src/openvpn/networking.h
> +++ b/src/openvpn/networking.h
> @@ -31,8 +31,8 @@ struct context;
> #include "networking_iproute2.h"
> #else
> /* define mock types to ensure code builds on any platform */
> -typedef void * openvpn_net_ctx_t;
> -typedef void * openvpn_net_iface_t;
> +typedef void *openvpn_net_ctx_t;
> +typedef void *openvpn_net_iface_t;
>
> static inline int
> net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx)
> @@ -51,7 +51,7 @@ net_ctx_free(openvpn_net_ctx_t *ctx)
> {
> (void)ctx;
> }
> -#endif
> +#endif /* ifdef ENABLE_SITNL */
>
> #if defined(ENABLE_SITNL) || defined(ENABLE_IPROUTE)
>
> diff --git a/src/openvpn/networking_iproute2.c b/src/openvpn/networking_iproute2.c
> index 0f9e899a..f3b9c614 100644
> --- a/src/openvpn/networking_iproute2.c
> +++ b/src/openvpn/networking_iproute2.c
> @@ -43,7 +43,9 @@ net_ctx_init(struct context *c, openvpn_net_ctx_t *ctx)
> {
> ctx->es = NULL;
> if (c)
> + {
> ctx->es = c->es;
> + }
> ctx->gc = gc_new();
>
> return 0;
> @@ -207,10 +209,14 @@ net_route_v4_add(openvpn_net_ctx_t *ctx, const in_addr_t *dst, int prefixlen,
> argv_printf(&argv, "%s route add %s/%d", iproute_path, dst_str, prefixlen);
>
> if (metric > 0)
> + {
> argv_printf_cat(&argv, "metric %d", metric);
> + }
>
> if (iface)
> + {
> argv_printf_cat(&argv, "dev %s", iface);
> + }
>
> if (gw)
> {
> @@ -246,7 +252,9 @@ net_route_v6_add(openvpn_net_ctx_t *ctx, const struct in6_addr *dst,
> }
>
> if (metric > 0)
> + {
> argv_printf_cat(&argv, "metric %d", metric);
> + }
>
> argv_msg(D_ROUTE, &argv);
> openvpn_execve_check(&argv, ctx->es, 0, "ERROR: Linux route -6 add command failed");
> @@ -267,7 +275,9 @@ net_route_v4_del(openvpn_net_ctx_t *ctx, const in_addr_t *dst, int prefixlen,
> argv_printf(&argv, "%s route del %s/%d", iproute_path, dst_str, prefixlen);
>
> if (metric > 0)
> + {
> argv_printf_cat(&argv, "metric %d", metric);
> + }
>
> argv_msg(D_ROUTE, &argv);
> openvpn_execve_check(&argv, ctx->es, 0, "ERROR: Linux route delete command failed");
> @@ -296,7 +306,9 @@ net_route_v6_del(openvpn_net_ctx_t *ctx, const struct in6_addr *dst,
> }
>
> if (metric > 0)
> + {
> argv_printf_cat(&argv, "metric %d", metric);
> + }
>
> argv_msg(D_ROUTE, &argv);
> openvpn_execve_check(&argv, ctx->es, 0, "ERROR: Linux route -6 del command failed");
> @@ -314,7 +326,9 @@ net_route_v4_best_gw(openvpn_net_ctx_t *ctx, const in_addr_t *dst,
>
> FILE *fp = fopen("/proc/net/route", "r");
> if (!fp)
> + {
> return -1;
> + }
>
> char line[256];
> int count = 0;
> diff --git a/src/openvpn/networking_sitnl.h b/src/openvpn/networking_sitnl.h
> index f39d426d..6396b06e 100644
> --- a/src/openvpn/networking_sitnl.h
> +++ b/src/openvpn/networking_sitnl.h
> @@ -23,6 +23,6 @@
> #define NETWORKING_SITNL_H_
>
> typedef char openvpn_net_iface_t;
> -typedef void * openvpn_net_ctx_t;
> +typedef void *openvpn_net_ctx_t;
>
> #endif /* NETWORKING_SITNL_H_ */
> diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
> index 900db7e1..595a9b1d 100644
> --- a/src/openvpn/openvpn.h
> +++ b/src/openvpn/openvpn.h
> @@ -524,7 +524,7 @@ struct context
>
> struct env_set *es; /**< Set of environment variables. */
>
> - openvpn_net_ctx_t net_ctx; /**< Networking API opaque context */
> + openvpn_net_ctx_t net_ctx; /**< Networking API opaque context */
>
> struct signal_info *sig; /**< Internal error signaling object. */
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 49df8df1..63dc53c3 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -1241,8 +1241,10 @@ print_vlan_accept(enum vlan_acceptable_frames mode)
> {
> case VLAN_ONLY_TAGGED:
> return "tagged";
> +
> case VLAN_ONLY_UNTAGGED_OR_PRIORITY:
> return "untagged";
> +
> case VLAN_ALL:
> return "all";
> }
> @@ -1320,7 +1322,7 @@ show_p2mp_parms(const struct options *o)
> SHOW_STR(port_share_port);
> #endif
> SHOW_BOOL(vlan_tagging);
> - msg(D_SHOW_PARMS, " vlan_accept = %s", print_vlan_accept (o->vlan_accept));
> + msg(D_SHOW_PARMS, " vlan_accept = %s", print_vlan_accept(o->vlan_accept));
> SHOW_INT(vlan_pvid);
> #endif /* P2MP_SERVER */
>
> @@ -5301,7 +5303,7 @@ add_option(struct options *options,
> options->management_flags |= MF_EXTERNAL_CERT;
> options->management_certificate = p[1];
> }
> -#endif
> +#endif /* ifdef ENABLE_MANAGEMENT */
> #ifdef MANAGEMENT_DEF_AUTH
> else if (streq(p[0], "management-client-auth") && !p[1])
> {
> @@ -7711,8 +7713,8 @@ add_option(struct options *options,
> }
> else
> {
> - if (streq(p[1], "secret") || streq(p[1], "tls-auth") ||
> - streq(p[1], "tls-crypt"))
> + if (streq(p[1], "secret") || streq(p[1], "tls-auth")
> + || streq(p[1], "tls-crypt"))
> {
> options->genkey_type = GENKEY_SECRET;
> }
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 2f1f6faf..4c1737e1 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -222,8 +222,8 @@ struct options
> bool show_curves;
> bool genkey;
> enum genkey_type genkey_type;
> - const char* genkey_filename;
> - const char* genkey_extra_data;
> + const char *genkey_filename;
> + const char *genkey_extra_data;
>
> /* Networking parms */
> int connect_retry_max;
> diff --git a/src/openvpn/proto.h b/src/openvpn/proto.h
> index c1ff3e14..c2517674 100644
> --- a/src/openvpn/proto.h
> +++ b/src/openvpn/proto.h
> @@ -67,7 +67,7 @@ struct openvpn_ethhdr
> struct openvpn_8021qhdr
> {
> uint8_t dest[OPENVPN_ETH_ALEN]; /* destination ethernet addr */
> - uint8_t source[OPENVPN_ETH_ALEN]; /* source ethernet addr */
> + uint8_t source[OPENVPN_ETH_ALEN]; /* source ethernet addr */
>
> uint16_t tpid; /* 802.1Q Tag Protocol Identifier */
> #define OPENVPN_8021Q_MASK_PCP htons(0xE000) /* mask PCP out of pcp_cfi_vid */
> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index aef00d34..39a906d4 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -72,19 +72,19 @@ receive_auth_failed(struct context *c, const struct buffer *buffer)
> {
> switch (auth_retry_get())
> {
> - case AR_NONE:
> - c->sig->signal_received = SIGTERM; /* SOFT-SIGTERM -- Auth failure error */
> - break;
> + case AR_NONE:
> + c->sig->signal_received = SIGTERM; /* SOFT-SIGTERM -- Auth failure error */
> + break;
>
> - case AR_INTERACT:
> - ssl_purge_auth(false);
> + case AR_INTERACT:
> + ssl_purge_auth(false);
>
> - case AR_NOINTERACT:
> - c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
> - break;
> + case AR_NOINTERACT:
> + c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
> + break;
>
> - default:
> - ASSERT(0);
> + default:
> + ASSERT(0);
> }
> c->sig->signal_text = "auth-failure";
> }
> diff --git a/src/openvpn/route.c b/src/openvpn/route.c
> index e0f8d201..51f76318 100644
> --- a/src/openvpn/route.c
> +++ b/src/openvpn/route.c
> @@ -2152,7 +2152,7 @@ delete_route(struct route_ipv4 *r,
> #if !defined(TARGET_ANDROID)
> const char *gateway;
> #endif
> -#else
> +#else /* if !defined(TARGET_LINUX) */
> int metric;
> #endif
> int is_local_route;
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 56d0576a..80e0d5ac 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -466,7 +466,7 @@ ssl_set_auth_token(const char *token)
> * Cleans an auth token and checks if it was active
> */
> bool
> -ssl_clean_auth_token (void)
> +ssl_clean_auth_token(void)
> {
> bool wasdefined = auth_token.defined;
> purge_user_pass(&auth_token, true);
> @@ -2015,7 +2015,7 @@ tls_session_update_crypto_params(struct tls_session *session,
> {
> frame_remove_from_extra_frame(frame_fragment, crypto_max_overhead());
> crypto_adjust_frame_parameters(frame_fragment, &session->opt->key_type,
> - options->replay, packet_id_long_form);
> + options->replay, packet_id_long_form);
> frame_set_mtu_dynamic(frame_fragment, options->ce.fragment, SET_MTU_UPPER_BOUND);
> frame_print(frame_fragment, D_MTU_INFO, "Fragmentation MTU parms");
> }
> @@ -2411,7 +2411,9 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
> * username/password
> */
> if (auth_token.defined)
> + {
> up = &auth_token;
> + }
>
> if (!write_string(buf, up->username, -1))
> {
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index f0a8ef54..2f6f7657 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -607,4 +607,5 @@ void
> show_available_tls_ciphers(const char *cipher_list,
> const char *cipher_list_tls13,
> const char *tls_cert_profile);
> +
> #endif /* ifndef OPENVPN_SSL_H */
> diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
> index d585111b..1f91b785 100644
> --- a/src/openvpn/ssl_mbedtls.c
> +++ b/src/openvpn/ssl_mbedtls.c
> @@ -191,12 +191,13 @@ tls_ctx_initialised(struct tls_root_ctx *ctx)
> }
>
> #ifdef HAVE_EXPORT_KEYING_MATERIAL
> -int mbedtls_ssl_export_keys_cb(void *p_expkey, const unsigned char *ms,
> - const unsigned char *kb, size_t maclen,
> - size_t keylen, size_t ivlen,
> - const unsigned char client_random[32],
> - const unsigned char server_random[32],
> - mbedtls_tls_prf_types tls_prf_type)
> +int
> +mbedtls_ssl_export_keys_cb(void *p_expkey, const unsigned char *ms,
> + const unsigned char *kb, size_t maclen,
> + size_t keylen, size_t ivlen,
> + const unsigned char client_random[32],
> + const unsigned char server_random[32],
> + mbedtls_tls_prf_types tls_prf_type)
> {
> struct tls_session *session = p_expkey;
> struct key_state_ssl *ks_ssl = &session->key[KS_PRIMARY].ks_ssl;
> @@ -1126,7 +1127,7 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
> if (session->opt->ekm_size)
> {
> mbedtls_ssl_conf_export_keys_ext_cb(ks_ssl->ssl_config,
> - mbedtls_ssl_export_keys_cb, session);
> + mbedtls_ssl_export_keys_cb, session);
> }
> #endif
>
> diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
> index d7bd6aa2..5955c6bd 100644
> --- a/src/openvpn/ssl_openssl.c
> +++ b/src/openvpn/ssl_openssl.c
> @@ -683,7 +683,7 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
> * so do nothing */
> #endif
> return;
> -#else
> +#else /* if OPENSSL_VERSION_NUMBER >= 0x10002000L */
> /* For older OpenSSL we have to extract the curve from key on our own */
> EC_KEY *eckey = NULL;
> const EC_GROUP *ecgrp = NULL;
> @@ -1173,7 +1173,7 @@ openvpn_extkey_rsa_finish(RSA *rsa)
> * interface query
> */
> const char *
> -get_rsa_padding_name (const int padding)
> +get_rsa_padding_name(const int padding)
> {
> switch (padding)
> {
> @@ -1190,14 +1190,14 @@ get_rsa_padding_name (const int padding)
>
> /**
> * Pass the input hash in 'dgst' to management and get the signature back.
> - *
> - * @param dgst hash to be signed
> - * @param dgstlen len of data in dgst
> - * @param sig On successful return signature is in sig.
> - * @param siglen length of buffer sig
> - * @param algorithm padding/hashing algorithm for the signature
> *
> - * @return signature length or -1 on error.
> + * @param dgst hash to be signed
> + * @param dgstlen len of data in dgst
> + * @param sig On successful return signature is in sig.
> + * @param siglen length of buffer sig
> + * @param algorithm padding/hashing algorithm for the signature
> + *
> + * @return signature length or -1 on error.
> */
> static int
> get_sig_from_man(const unsigned char *dgst, unsigned int dgstlen,
> @@ -1239,7 +1239,7 @@ rsa_priv_enc(int flen, const unsigned char *from, unsigned char *to, RSA *rsa,
> return -1;
> }
>
> - ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name (padding));
> + ret = get_sig_from_man(from, flen, to, len, get_rsa_padding_name(padding));
>
> return (ret == len) ? ret : -1;
> }
> @@ -1314,7 +1314,7 @@ err:
> }
>
> #if ((OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) \
> - || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
> + || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
> && !defined(OPENSSL_NO_EC)
>
> /* called when EC_KEY is destroyed */
> @@ -1475,7 +1475,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
> }
> }
> #if ((OPENSSL_VERSION_NUMBER > 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) \
> - || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
> + || LIBRESSL_VERSION_NUMBER > 0x2090000fL) \
> && !defined(OPENSSL_NO_EC)
> else if (EVP_PKEY_id(pkey) == EVP_PKEY_EC)
> {
> @@ -2135,8 +2135,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
> crypto_msg(M_FATAL, "Cannot create SSL object");
> }
>
> -#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) || \
> - (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <= 0x2090000fL)
> +#if (OPENSSL_VERSION_NUMBER < 0x1010000fL) \
> + || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER <= 0x2090000fL)
> STACK_OF(SSL_CIPHER) *sk = SSL_get_ciphers(ssl);
> #else
> STACK_OF(SSL_CIPHER) *sk = SSL_get1_supported_ciphers(ssl);
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index da0966c5..9362b8e9 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -804,7 +804,7 @@ cleanup:
> #endif
>
> void
> -auth_set_client_reason(struct tls_multi* multi, const char* client_reason)
> +auth_set_client_reason(struct tls_multi *multi, const char *client_reason)
> {
> if (multi->client_reason)
> {
> @@ -1204,7 +1204,7 @@ verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,
>
> static int
> verify_user_pass_management(struct tls_session *session,
> - struct tls_multi* multi,
> + struct tls_multi *multi,
> const struct user_pass *up)
> {
> int retval = KMDA_ERROR;
> @@ -1301,16 +1301,16 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
> * for equality with AUTH_TOKEN_HMAC_OK
> */
> msg(M_WARN, "TLS: Username/auth-token authentication "
> - "succeeded for username '%s'",
> + "succeeded for username '%s'",
> up->username);
> - skip_auth = true;
> + skip_auth = true;
> }
> else
> {
> wipe_auth_token(multi);
> ks->authenticated = false;
> msg(M_WARN, "TLS: Username/auth-token authentication "
> - "failed for username '%s'", up->username);
> + "failed for username '%s'", up->username);
> return;
> }
> }
> @@ -1335,12 +1335,12 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi,
> }
>
> /* check sizing of username if it will become our common name */
> - if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) &&
> - strlen(up->username)>TLS_USERNAME_LEN)
> + if ((session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME)
> + && strlen(up->username)>TLS_USERNAME_LEN)
> {
> msg(D_TLS_ERRORS,
> - "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
> - TLS_USERNAME_LEN);
> + "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters",
> + TLS_USERNAME_LEN);
> s1 = OPENVPN_PLUGIN_FUNC_ERROR;
> }
> /* auth succeeded? */
> diff --git a/src/openvpn/ssl_verify.h b/src/openvpn/ssl_verify.h
> index c54b89a6..21b37a0f 100644
> --- a/src/openvpn/ssl_verify.h
> +++ b/src/openvpn/ssl_verify.h
> @@ -234,7 +234,8 @@ bool tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id
> * @param multi The multi tls struct
> * @param client_reason The string to send to the client as part of AUTH_FAILED
> */
> -void auth_set_client_reason(struct tls_multi* multi, const char* client_reason);
> +void auth_set_client_reason(struct tls_multi *multi, const char *client_reason);
> +
> #endif
>
> static inline const char *
> diff --git a/src/openvpn/vlan.c b/src/openvpn/vlan.c
> index a5885de2..9290179d 100644
> --- a/src/openvpn/vlan.c
> +++ b/src/openvpn/vlan.c
> @@ -58,7 +58,7 @@ static void
> vlanhdr_set_vid(struct openvpn_8021qhdr *hdr, const uint16_t vid)
> {
> hdr->pcp_cfi_vid = (hdr->pcp_cfi_vid & ~OPENVPN_8021Q_MASK_VID)
> - | (htons(vid) & OPENVPN_8021Q_MASK_VID);
> + | (htons(vid) & OPENVPN_8021Q_MASK_VID);
> }
>
> /*
> @@ -135,7 +135,7 @@ vlan_decapsulate(const struct context *c, struct buffer *buf)
> goto drop;
> }
>
> - /* vid == 0 means prio-tagged packet: don't drop and fall-through */
> + /* vid == 0 means prio-tagged packet: don't drop and fall-through */
> case VLAN_ONLY_TAGGED:
> case VLAN_ALL:
> /* tagged frame can be accepted: extract vid and strip encapsulation */
> diff --git a/src/openvpn/win32.h b/src/openvpn/win32.h
> index 4b508c56..79504776 100644
> --- a/src/openvpn/win32.h
> +++ b/src/openvpn/win32.h
> @@ -69,7 +69,7 @@ struct security_attributes
> struct window_title
> {
> bool saved;
> - char old_window_title [256];
> + char old_window_title[256];
> };
>
> struct rw_handle {
>
I am not super happy with mis-aligning multi-line messages, but that's
how we have instructed uncrustify so far and we accept it for now.
@arne do you know if that bit is customizable?
By the way, the patch looks good.. except for that nitpick at the beginning.
--
Antonio Quartulli
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-17 13:42 ` Antonio Quartulli
@ 2020-04-17 15:36 ` Gert Doering
2020-04-26 9:25 ` Steffan Karger
0 siblings, 1 reply; 20+ messages in thread
From: Gert Doering @ 2020-04-17 15:36 UTC (permalink / raw)
To: Antonio Quartulli <a@; +Cc: Gert Doering <gert@
[-- Attachment #1: Type: text/plain, Size: 1650 bytes --]
Hi,
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).
> 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.
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 gert@...1296...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again.
2020-04-17 15:01 ` Antonio Quartulli
@ 2020-04-19 10:27 ` Gert Doering
2020-04-19 12:22 ` Antonio Quartulli
0 siblings, 1 reply; 20+ messages in thread
From: Gert Doering @ 2020-04-19 10:27 UTC (permalink / raw)
To: Antonio Quartulli <a@; +Cc: Arne Schwabe <arne@
[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]
Hi,
On Fri, Apr 17, 2020 at 05:01:29PM +0200, Antonio Quartulli wrote:
> > - char* ret = management_query_multiline_flatten(man,
> > - (char *)buf_bptr(&buf_data), prompt, desc,
> > - &man->connection.ext_key_state, &man->connection.ext_key_input);
> > + char *ret = management_query_multiline_flatten(man,
> > + (char *)buf_bptr(&buf_data), prompt, desc,
>
> why not moving some arguments on the first line ? One or two should fit.
> (IMHO it can be done in this patch - it's still about restyling)
The point of a "this is what uncrustify produces" patch is to produce
something which is immutable on future runs of uncrustify - so we can
check *new* code, without having to ignore other stuff that uncrustify
wants to change every time.
So, running uncrustify, and then changing things for a nicer look is
not solving the underlying issue.
(OTOH, if it turns out that some uncrustify option should be *changed*,
to produce nicer looking code, we can always do that after a quick round
of discussion in the meeting)
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 gert@...1296...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Openvpn-devel] [PATCH applied] Re: Minor style change to improve code style
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 2/3] Minor style change to improve code style Arne Schwabe
2020-04-17 14:56 ` Antonio Quartulli
@ 2020-04-19 10:32 ` Gert Doering
1 sibling, 0 replies; 20+ messages in thread
From: Gert Doering @ 2020-04-19 10:32 UTC (permalink / raw)
To: Arne Schwabe <arne@; +Cc: openvpn-devel
Your patch has been applied to the master branch.
(And yes...)
commit cbde07f474ae9e92b329475767c4660dd35b4ee4
Author: Arne Schwabe
Date: Thu Apr 16 13:39:29 2020 +0200
Minor style change to improve code style
Signed-off-by: Arne Schwabe <arne@...1227...>
Acked-by: Antonio Quartulli <antonio@...515...>
Message-Id: <20200416113930.15192-2-arne@...1227...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19748.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Openvpn-devel] [PATCH applied] Re: After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again.
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again Arne Schwabe
2020-04-17 15:01 ` Antonio Quartulli
@ 2020-04-19 10:40 ` Gert Doering
1 sibling, 0 replies; 20+ messages in thread
From: Gert Doering @ 2020-04-19 10:40 UTC (permalink / raw)
To: Arne Schwabe <arne@; +Cc: openvpn-devel
Acked-by: Gert Doering <gert@...1296...>
Verified the changes manually and by test compiling (Linux and FreeBSD)
- with "git show -w" one can see that most of it is simple whitespace
changes, and the rest is "change line wraps" and "add comment to #endif"
stuff.
The manage.c change is particularily ugly, but with a function name
that long and multiple (long) arguments, there is not really a way
that an automated tool can make this look *nice* (except if it had
a rule for "if the indended indent is > 50 chars, limit to 50" or
so - but then it would just be inconsistent).
Your patch has been applied to the master branch.
I've modified the commit message slightly (put the long text in the body,
add a shorter subject)
commit 9cf7b4925a54d93fbea1cadcf3dc0e11f3ce358f
Author: Arne Schwabe
Date: Thu Apr 16 13:39:30 2020 +0200
Another round of uncrustify code cleanup.
After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again.
Signed-off-by: Arne Schwabe <arne@...1227...>
Acked-by: Gert Doering <gert@...1296...>
Message-Id: <20200416113930.15192-3-arne@...1227...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19750.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again.
2020-04-19 10:27 ` Gert Doering
@ 2020-04-19 12:22 ` Antonio Quartulli
2020-04-19 15:46 ` Gert Doering
0 siblings, 1 reply; 20+ messages in thread
From: Antonio Quartulli @ 2020-04-19 12:22 UTC (permalink / raw)
To: Gert Doering <gert@; +Cc: openvpn-devel
Hi,
On 19/04/2020 12:27, Gert Doering wrote:
> Hi,
>
> On Fri, Apr 17, 2020 at 05:01:29PM +0200, Antonio Quartulli wrote:
>>> - char* ret = management_query_multiline_flatten(man,
>>> - (char *)buf_bptr(&buf_data), prompt, desc,
>>> - &man->connection.ext_key_state, &man->connection.ext_key_input);
>>> + char *ret = management_query_multiline_flatten(man,
>>> + (char *)buf_bptr(&buf_data), prompt, desc,
>>
>> why not moving some arguments on the first line ? One or two should fit.
>> (IMHO it can be done in this patch - it's still about restyling)
>
> The point of a "this is what uncrustify produces" patch is to produce
> something which is immutable on future runs of uncrustify - so we can
> check *new* code, without having to ignore other stuff that uncrustify
> wants to change every time.
>
> So, running uncrustify, and then changing things for a nicer look is
> not solving the underlying issue.
>
fully agreed. but in this case uncrustify is just fixing the alignment -
moving the arguments above would not make uncrustify unhappy because all
rules would still be respected.
Regards,
--
Antonio Quartulli
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again.
2020-04-19 12:22 ` Antonio Quartulli
@ 2020-04-19 15:46 ` Gert Doering
0 siblings, 0 replies; 20+ messages in thread
From: Gert Doering @ 2020-04-19 15:46 UTC (permalink / raw)
To: Antonio Quartulli <a@; +Cc: Gert Doering <gert@
[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]
Hi,
On Sun, Apr 19, 2020 at 02:22:37PM +0200, Antonio Quartulli wrote:
> >>> - char* ret = management_query_multiline_flatten(man,
> >>> - (char *)buf_bptr(&buf_data), prompt, desc,
> >>> - &man->connection.ext_key_state, &man->connection.ext_key_input);
> >>> + char *ret = management_query_multiline_flatten(man,
> >>> + (char *)buf_bptr(&buf_data), prompt, desc,
>
> fully agreed. but in this case uncrustify is just fixing the alignment -
> moving the arguments above would not make uncrustify unhappy because all
> rules would still be respected.
I think uncrustify would promptly move them down again... line length
rule.
Of course it's worth a try... we're at a good point for refactoring
and code cleanup.
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 gert@...1296...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-17 15:36 ` Gert Doering
@ 2020-04-26 9:25 ` Steffan Karger
2020-04-26 9:34 ` Gert Doering
0 siblings, 1 reply; 20+ messages in thread
From: Steffan Karger @ 2020-04-26 9:25 UTC (permalink / raw)
To: openvpn-devel
[-- Attachment #1.1: Type: text/plain, Size: 2095 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-26 9:25 ` Steffan Karger
@ 2020-04-26 9:34 ` Gert Doering
2020-04-26 9:49 ` Steffan Karger
2020-04-26 22:38 ` Arne Schwabe
0 siblings, 2 replies; 20+ messages in thread
From: Gert Doering @ 2020-04-26 9:34 UTC (permalink / raw)
To: Steffan Karger <steffan@; +Cc: openvpn-devel
[-- Attachment #1: Type: text/plain, Size: 2477 bytes --]
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 gert@...1296...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 630 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-26 9:34 ` Gert Doering
@ 2020-04-26 9:49 ` Steffan Karger
2020-04-26 22:38 ` Arne Schwabe
1 sibling, 0 replies; 20+ messages in thread
From: Steffan Karger @ 2020-04-26 9:49 UTC (permalink / raw)
To: Gert Doering <gert@; +Cc: openvpn-devel
[-- Attachment #1.1: Type: text/plain, Size: 1736 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible
2020-04-26 9:34 ` Gert Doering
2020-04-26 9:49 ` Steffan Karger
@ 2020-04-26 22:38 ` Arne Schwabe
1 sibling, 0 replies; 20+ messages in thread
From: Arne Schwabe @ 2020-04-26 22:38 UTC (permalink / raw)
To: Gert Doering <gert@; +Cc: openvpn-devel
[-- Attachment #1.1: Type: text/plain, Size: 1982 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* [Openvpn-devel] [PATCH applied] Re: Use crypto library functions for const time memcmp when possible
2020-04-16 11:39 [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Arne Schwabe
` (4 preceding siblings ...)
2020-04-17 8:51 ` Gert Doering
@ 2020-05-07 13:21 ` Gert Doering
5 siblings, 0 replies; 20+ messages in thread
From: Gert Doering @ 2020-05-07 13:21 UTC (permalink / raw)
To: Arne Schwabe <arne@; +Cc: openvpn-devel
Acked-by: Gert Doering <gert@...1296...>
I still feel it hurts my eyes, and is way overcomplicating things, but
if this is what mbedtls is using internally (why are they not exporting
it??!), it should be good enough for us. Further, as it's not being
used for AEAD anyway, I withdraw my "performance" argument (Steffan
could have just ACKed it... :-) ).
Stared at the code (awww!), test-built with openssl and mbedtls, passed
t_client tests.
We do not have a unit test for this, and crypto.c::test_crypto() actually
does the "compare bytes loop" manually (to be able to print differences).
Volunteers...?
Your patch has been applied to the master branch.
commit 4dddca52a8432095dd85ff652fae61a2aedb3785
Author: Arne Schwabe
Date: Thu Apr 16 13:39:28 2020 +0200
Use crypto library functions for const time memcmp when possible
Signed-off-by: Arne Schwabe <arne@...1227...>
Acked-by: Gert Doering <gert@...1296...>
Message-Id: <20200416113930.15192-1-arne@...1227...>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19749.html
Signed-off-by: Gert Doering <gert@...1296...>
--
kind regards,
Gert Doering
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2020-05-07 13:21 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-04-16 11:39 [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Arne Schwabe
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 2/3] Minor style change to improve code style Arne Schwabe
2020-04-17 14:56 ` Antonio Quartulli
2020-04-19 10:32 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2020-04-16 11:39 ` [Openvpn-devel] [PATCH v2 3/3] After the last big formatting patch a number of changes have been commited that do not conform with our style/uncrustify config. This has lead to the problem that running uncrustify on before sending PR some of the changes made by uncrustify need to be backed out again Arne Schwabe
2020-04-17 15:01 ` Antonio Quartulli
2020-04-19 10:27 ` Gert Doering
2020-04-19 12:22 ` Antonio Quartulli
2020-04-19 15:46 ` Gert Doering
2020-04-19 10:40 ` [Openvpn-devel] [PATCH applied] " Gert Doering
2020-04-16 14:10 ` [Openvpn-devel] [PATCH v2 1/3] Use crypto library functions for const time memcmp when possible Antonio Quartulli
2020-04-17 7:53 ` Arne Schwabe
2020-04-17 8:51 ` Gert Doering
2020-04-17 13:42 ` Antonio Quartulli
2020-04-17 15:36 ` Gert Doering
2020-04-26 9:25 ` Steffan Karger
2020-04-26 9:34 ` Gert Doering
2020-04-26 9:49 ` Steffan Karger
2020-04-26 22:38 ` Arne Schwabe
2020-05-07 13:21 ` [Openvpn-devel] [PATCH applied] " Gert Doering
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.