Menu
▾
▴
openvpn-devel
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:47:53
|
This allows us to skip waiting for the first PUSH_REQUEST message from
the client to send the response.
Signed-off-by: Arne Schwabe <ar...@rf...>
---
src/openvpn/multi.c | 12 ++++++++++--
src/openvpn/ssl.c | 15 +++++++++++++--
src/openvpn/ssl.h | 7 +++++++
3 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 0095c871..88ba9db2 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1795,10 +1795,18 @@ multi_client_set_protocol_options(struct context *c)
{
int proto = 0;
int r = sscanf(optstr, "IV_PROTO=%d", &proto);
- if ((r == 1) && (proto >= 2))
+ if (r == 1)
{
- tls_multi->use_peer_id = true;
+ if (proto >= 2)
+ {
+ tls_multi->use_peer_id = true;
+ }
+ if (proto & IV_PROTO_REQUEST_PUSH)
+ {
+ c->c2.push_request_received = true;
+ }
}
+
}
/* Select cipher if client supports Negotiable Crypto Parameters */
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 54a23011..04d78a81 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
buf_printf(&out, "IV_PLAT=win\n");
#endif
- /* support for P_DATA_V2 */
- buf_printf(&out, "IV_PROTO=2\n");
+ /* support for P_DATA_V2*/
+ int iv_proto = IV_PROTO_DATA_V2;
+
+ /* support for receiving push_reply before sending
+ * push request, also signal that the client wants
+ * to get push-reply messages without without requiring a round
+ * trip for a push request message*/
+ if(session->opt->pull)
+ {
+ iv_proto |= IV_PROTO_REQUEST_PUSH;
+ }
+
+ buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
/* support for Negotiable Crypto Parameters */
if (session->opt->ncp_enabled
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 58a9b0d4..86fb6853 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -99,6 +99,13 @@
/* Maximum length of OCC options string passed as part of auth handshake */
#define TLS_OPTIONS_LEN 512
+/* Definitions of the bits in the IV_PROTO bitfield */
+#define IV_PROTO_DATA_V2 (1<<1) /**< Support P_DATA_V2 */
+#define IV_PROTO_REQUEST_PUSH (1<<2) /**< Assume client will send a push
+ * request and server does not need
+ * to wait for a push-request to send
+ * a push-reply */
+
/* Default field in X509 to be username */
#define X509_USERNAME_FIELD_DEFAULT "CN"
--
2.26.2
|
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:47:51
|
The NCP rework introduced a regression of sending a --cipher
command as part of the push message when the client does not
support NCP. This is is more a cosmetic issue since the client
will log that as warning in the log and ignore it.
Signed-off-by: Arne Schwabe <ar...@rf...>
---
src/openvpn/push.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 2183b74a..1c4f2033 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -472,9 +472,15 @@ prepare_push_reply(struct context *c, struct gc_arena *gc,
/*
* Push the selected cipher, at this point the cipher has been
- * already negotiated and been fixed
+ * already negotiated and been fixed.
+ *
+ * We avoid pushing the cipher to clients not supporting NCP
+ * to avoid error messages in their logs
*/
- push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
+ if (tls_peer_supports_ncp(c->c2.tls_multi->peer_info))
+ {
+ push_option_fmt(gc, push_list, M_USAGE, "cipher %s", o->ciphername);
+ }
return true;
}
--
2.26.2
|
|
From: Gert D. <ge...@gr...> - 2020-07-22 07:41:57
|
Acked-by: Gert Doering <ge...@gr...>
Verify by staring at the testbed. 2.2/2.3 clients or 2.4 or master
with --ncp-disable won't get cipher pushed, the rest will.
Works!
Your patch has been applied to the master branch.
commit 4b59e2644a978074f0eed492d6541ba7b30b01a7
Author: Arne Schwabe
Date: Fri Jul 17 15:47:37 2020 +0200
Avoid sending --cipher to clients not supporting NCP
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@rf...>
URL: https://www.mail-archive.com/ope...@li.../msg20437.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:47:50
|
Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
v1 op codes and give a good warning message if we encounter
them in the legal op codes pre-check.
Signed-off-by: Arne Schwabe <ar...@rf...>
---
doc/doxygen/doc_control_processor.h | 6 +-
doc/doxygen/doc_key_generation.h | 6 +-
doc/doxygen/doc_protocol_overview.h | 2 +-
src/openvpn/forward.c | 2 +-
src/openvpn/helper.c | 5 -
src/openvpn/init.c | 1 -
src/openvpn/options.c | 35 +----
src/openvpn/options.h | 4 -
src/openvpn/ssl.c | 230 ++++------------------------
src/openvpn/ssl.h | 19 +--
src/openvpn/ssl_common.h | 1 -
11 files changed, 42 insertions(+), 269 deletions(-)
diff --git a/doc/doxygen/doc_control_processor.h b/doc/doxygen/doc_control_processor.h
index f87324cc..1bbf2d2d 100644
--- a/doc/doxygen/doc_control_processor.h
+++ b/doc/doxygen/doc_control_processor.h
@@ -175,11 +175,7 @@
* appropriate messages to be sent.
*
* @par Functions which control data channel key generation
- * - Key method 1 key exchange functions:
- * - \c key_method_1_write(), generates and processes key material to
- * be sent to the remote OpenVPN peer.
- * - \c key_method_1_read(), processes key material received from the
- * remote OpenVPN peer.
+ * - Key method 1 key exchange functions were removed from OpenVPN 2.5
* - Key method 2 key exchange functions:
* - \c key_method_2_write(), generates and processes key material to
* be sent to the remote OpenVPN peer.
diff --git a/doc/doxygen/doc_key_generation.h b/doc/doxygen/doc_key_generation.h
index efe61155..4bb9c708 100644
--- a/doc/doxygen/doc_key_generation.h
+++ b/doc/doxygen/doc_key_generation.h
@@ -131,11 +131,7 @@ S_ACTIVE S_ACTIVE
* control_processor Control Channel Processor module's\endlink \c
* tls_process() function and control the %key generation and exchange
* process as follows:
- * - %Key method 1:
- * - \c key_method_1_write(): generate random material locally, and load
- * as "sending" keys.
- * - \c key_method_1_read(): read random material received from remote
- * peer, and load as "receiving" keys.
+ * - %Key method 1 has been removed in OpenVPN 2.5
* - %Key method 2:
* - \c key_method_2_write(): generate random material locally, and if
* in server mode generate %key expansion.
diff --git a/doc/doxygen/doc_protocol_overview.h b/doc/doxygen/doc_protocol_overview.h
index 3f48b18a..08212223 100644
--- a/doc/doxygen/doc_protocol_overview.h
+++ b/doc/doxygen/doc_protocol_overview.h
@@ -150,7 +150,7 @@
*
* @subsection network_protocol_control_plaintext Structure of plaintext control channel messages
*
- * - %Key method 1:
+ * - %Key method 1 (support removed in OpenVPN 2.5):
* - Cipher %key length in bytes (1 byte).
* - Cipher %key (n bytes).
* - HMAC %key length in bytes (1 byte).
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 5c4370a8..698451d1 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1100,7 +1100,7 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
floated, &ad_start))
{
/* Restore pre-NCP frame parameters */
- if (is_hard_reset(opcode, c->options.key_method))
+ if (is_hard_reset_method2(opcode))
{
c->c2.frame = c->c2.frame_initial;
#ifdef ENABLE_FRAGMENT
diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 6e9cc63c..a1d03070 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -490,11 +490,6 @@ helper_client_server(struct options *o)
*/
if (o->client)
{
- if (o->key_method != 2)
- {
- msg(M_USAGE, "--client requires --key-method 2");
- }
-
o->pull = true;
o->tls_client = true;
}
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index e9c01629..b96d1471 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2852,7 +2852,6 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
to.ssl_ctx = c->c1.ks.ssl_ctx;
to.key_type = c->c1.ks.key_type;
to.server = options->tls_server;
- to.key_method = options->key_method;
to.replay = options->replay;
to.replay_window = options->replay_window;
to.replay_time = options->replay_time;
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 15173db2..0025c526 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -881,7 +881,6 @@ init_options(struct options *o, const bool init_gc)
#ifdef ENABLE_PREDICTION_RESISTANCE
o->use_prediction_resistance = false;
#endif
- o->key_method = 2;
o->tls_timeout = 2;
o->renegotiate_bytes = -1;
o->renegotiate_seconds = 3600;
@@ -1715,7 +1714,6 @@ show_settings(const struct options *o)
SHOW_BOOL(tls_server);
SHOW_BOOL(tls_client);
- SHOW_INT(key_method);
SHOW_STR(ca_file);
SHOW_STR(ca_path);
SHOW_STR(dh_file);
@@ -2376,10 +2374,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
{
msg(M_USAGE, "--ccd-exclusive must be used with --client-config-dir");
}
- if (options->key_method != 2)
- {
- msg(M_USAGE, "--mode server requires --key-method 2");
- }
if (options->auth_token_generate && !options->renegotiate_seconds)
{
msg(M_USAGE, "--auth-gen-token needs a non-infinite "
@@ -2546,13 +2540,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
"may accept clients which do not present a certificate");
}
- if (options->key_method == 1)
- {
- msg(M_WARN, "WARNING: --key-method 1 is deprecated and will be removed "
- "in OpenVPN 2.5. By default --key-method 2 will be used if not set "
- "in the configuration file, which is the recommended approach.");
- }
-
const int tls_version_max =
(options->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
& SSLF_TLS_VERSION_MAX_MASK;
@@ -2794,7 +2781,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
MUST_BE_UNDEF(push_peer_info);
MUST_BE_UNDEF(tls_exit);
MUST_BE_UNDEF(crl_file);
- MUST_BE_UNDEF(key_method);
MUST_BE_UNDEF(ns_cert_type);
MUST_BE_UNDEF(remote_cert_ku[0]);
MUST_BE_UNDEF(remote_cert_eku);
@@ -3823,10 +3809,7 @@ options_string(const struct options *o,
* tls-auth/tls-crypt does not match. Removing tls-auth here would
* break stuff, so leaving that in place. */
- if (o->key_method > 1)
- {
- buf_printf(&out, ",key-method %d", o->key_method);
- }
+ buf_printf(&out, ",key-method %d", KEY_METHOD_2);
}
if (remote)
@@ -8477,22 +8460,6 @@ add_option(struct options *options,
VERIFY_PERMISSION(OPT_P_GENERAL);
options->tls_crypt_v2_verify_script = p[1];
}
- else if (streq(p[0], "key-method") && p[1] && !p[2])
- {
- int key_method;
-
- VERIFY_PERMISSION(OPT_P_GENERAL);
- key_method = atoi(p[1]);
- if (key_method < KEY_METHOD_MIN || key_method > KEY_METHOD_MAX)
- {
- msg(msglevel, "key_method parameter (%d) must be >= %d and <= %d",
- key_method,
- KEY_METHOD_MIN,
- KEY_METHOD_MAX);
- goto err;
- }
- options->key_method = key_method;
- }
else if (streq(p[0], "x509-track") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 1b038c91..3546bab3 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -572,10 +572,6 @@ struct options
#ifdef ENABLE_CRYPTOAPI
const char *cryptoapi_cert;
#endif
-
- /* data channel key exchange method */
- int key_method;
-
/* Per-packet timeout on control channel */
int tls_timeout;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 00b97352..4144217d 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -861,23 +861,12 @@ print_key_id(struct tls_multi *multi, struct gc_arena *gc)
}
bool
-is_hard_reset(int op, int key_method)
+is_hard_reset_method2(int op)
{
- if (!key_method || key_method == 1)
+ if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
+ || op == P_CONTROL_HARD_RESET_CLIENT_V3)
{
- if (op == P_CONTROL_HARD_RESET_CLIENT_V1 || op == P_CONTROL_HARD_RESET_SERVER_V1)
- {
- return true;
- }
- }
-
- if (!key_method || key_method >= 2)
- {
- if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
- || op == P_CONTROL_HARD_RESET_CLIENT_V3)
- {
- return true;
- }
+ return true;
}
return false;
@@ -1097,23 +1086,14 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session)
}
/* Are we a TLS server or client? */
- ASSERT(session->opt->key_method >= 1);
- if (session->opt->key_method == 1)
+ if (session->opt->server)
{
- session->initial_opcode = session->opt->server ?
- P_CONTROL_HARD_RESET_SERVER_V1 : P_CONTROL_HARD_RESET_CLIENT_V1;
+ session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
}
- else /* session->opt->key_method >= 2 */
+ else
{
- if (session->opt->server)
- {
- session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
- }
- else
- {
- session->initial_opcode = session->opt->tls_crypt_v2 ?
- P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
- }
+ session->initial_opcode = session->opt->tls_crypt_v2 ?
+ P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
}
/* Initialize control channel authentication parameters */
@@ -1993,9 +1973,9 @@ tls_session_update_crypto_params(struct tls_session *session,
}
else
{
- /* Very hacky workaround and quick fix for frame calculation
- * different when adjusting frame size when the original and new cipher
- * are identical to avoid a regression with client without NCP */
+ /* Very hacky workaround and quick fix for frame calculation
+ * different when adjusting frame size when the original and new cipher
+ * are identical to avoid a regression with client without NCP */
return tls_session_generate_data_channel_keys(session);
}
@@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
return str;
}
-/*
- * Handle the reading and writing of key data to and from
- * the TLS control channel (cleartext).
- */
-
-static bool
-key_method_1_write(struct buffer *buf, struct tls_session *session)
-{
- struct key key;
- struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
-
- ASSERT(session->opt->key_method == 1);
- ASSERT(buf_init(buf, 0));
-
- generate_key_random(&key, &session->opt->key_type);
- if (!check_key(&key, &session->opt->key_type))
- {
- msg(D_TLS_ERRORS, "TLS Error: Bad encrypting key generated");
- return false;
- }
-
- if (!write_key(&key, &session->opt->key_type, buf))
- {
- msg(D_TLS_ERRORS, "TLS Error: write_key failed");
- return false;
- }
-
- init_key_ctx(&ks->crypto_options.key_ctx_bi.encrypt, &key,
- &session->opt->key_type, OPENVPN_OP_ENCRYPT,
- "Data Channel Encrypt");
- secure_memzero(&key, sizeof(key));
-
- /* send local options string */
- {
- const char *local_options = local_options_string(session);
- const int optlen = strlen(local_options) + 1;
- if (!buf_write(buf, local_options, optlen))
- {
- msg(D_TLS_ERRORS, "TLS Error: KM1 write options failed");
- return false;
- }
- }
-
- return true;
-}
-
static bool
push_peer_info(struct buffer *buf, struct tls_session *session)
{
@@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
* push request, also signal that the client wants
* to get push-reply messages without without requiring a round
* trip for a push request message*/
- if(session->opt->pull)
+ if (session->opt->pull)
{
iv_proto |= IV_PROTO_REQUEST_PUSH;
}
@@ -2394,7 +2328,6 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
{
struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
- ASSERT(session->opt->key_method == 2);
ASSERT(buf_init(buf, 0));
/* write a uint32 0 */
@@ -2404,7 +2337,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
}
/* write key_method + flags */
- if (!buf_write_u8(buf, (session->opt->key_method & KEY_METHOD_MASK)))
+ if (!buf_write_u8(buf, KEY_METHOD_2))
{
goto error;
}
@@ -2506,73 +2439,11 @@ error:
return false;
}
-static bool
-key_method_1_read(struct buffer *buf, struct tls_session *session)
-{
- int status;
- struct key key;
- struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
-
- ASSERT(session->opt->key_method == 1);
-
- if (!session->verified)
- {
- msg(D_TLS_ERRORS,
- "TLS Error: Certificate verification failed (key-method 1)");
- goto error;
- }
-
- status = read_key(&key, &session->opt->key_type, buf);
- if (status != 1)
- {
- msg(D_TLS_ERRORS,
- "TLS Error: Error reading data channel key from plaintext buffer");
- goto error;
- }
-
- if (!check_key(&key, &session->opt->key_type))
- {
- msg(D_TLS_ERRORS, "TLS Error: Bad decrypting key received from peer");
- goto error;
- }
-
- if (buf->len < 1)
- {
- msg(D_TLS_ERRORS, "TLS Error: Missing options string");
- goto error;
- }
-
-#ifdef ENABLE_OCC
- /* compare received remote options string
- * with our locally computed options string */
- if (!session->opt->disable_occ
- && !options_cmp_equal_safe((char *) BPTR(buf), session->opt->remote_options, buf->len))
- {
- options_warning_safe((char *) BPTR(buf), session->opt->remote_options, buf->len);
- }
-#endif
-
- buf_clear(buf);
-
- init_key_ctx(&ks->crypto_options.key_ctx_bi.decrypt, &key,
- &session->opt->key_type, OPENVPN_OP_DECRYPT,
- "Data Channel Decrypt");
- secure_memzero(&key, sizeof(key));
- ks->authenticated = KS_AUTH_TRUE;
- return true;
-
-error:
- buf_clear(buf);
- secure_memzero(&key, sizeof(key));
- return false;
-}
-
static bool
key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_session *session)
{
struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
- int key_method_flags;
bool username_status, password_status;
struct gc_arena gc = gc_new();
@@ -2582,8 +2453,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
/* allocate temporary objects */
ALLOC_ARRAY_CLEAR_GC(options, char, TLS_OPTIONS_LEN, &gc);
- ASSERT(session->opt->key_method == 2);
-
/* discard leading uint32 */
if (!buf_advance(buf, 4))
{
@@ -2593,7 +2462,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
}
/* get key method */
- key_method_flags = buf_read_u8(buf);
+ int key_method_flags = buf_read_u8(buf);
if ((key_method_flags & KEY_METHOD_MASK) != 2)
{
msg(D_TLS_ERRORS,
@@ -3003,23 +2872,9 @@ tls_process(struct tls_multi *multi,
if (!buf->len && ((ks->state == S_START && !session->opt->server)
|| (ks->state == S_GOT_KEY && session->opt->server)))
{
- if (session->opt->key_method == 1)
+ if (!key_method_2_write(buf, session))
{
- if (!key_method_1_write(buf, session))
- {
- goto error;
- }
- }
- else if (session->opt->key_method == 2)
- {
- if (!key_method_2_write(buf, session))
- {
- goto error;
- }
- }
- else
- {
- ASSERT(0);
+ goto error;
}
state_change = true;
@@ -3033,23 +2888,9 @@ tls_process(struct tls_multi *multi,
&& ((ks->state == S_SENT_KEY && !session->opt->server)
|| (ks->state == S_START && session->opt->server)))
{
- if (session->opt->key_method == 1)
+ if (!key_method_2_read(buf, multi, session))
{
- if (!key_method_1_read(buf, session))
- {
- goto error;
- }
- }
- else if (session->opt->key_method == 2)
- {
- if (!key_method_2_read(buf, multi, session))
- {
- goto error;
- }
- }
- else
- {
- ASSERT(0);
+ goto error;
}
state_change = true;
@@ -3463,6 +3304,11 @@ tls_pre_decrypt(struct tls_multi *multi,
/* verify legal opcode */
if (op < P_FIRST_OPCODE || op > P_LAST_OPCODE)
{
+ if (op == P_CONTROL_HARD_RESET_CLIENT_V1
+ || op == P_CONTROL_HARD_RESET_SERVER_V1)
+ {
+ msg(D_TLS_ERRORS, "Peer tried unsupported key-method 1");
+ }
msg(D_TLS_ERRORS,
"TLS Error: unknown opcode received from %s op=%d",
print_link_socket_actual(from, &gc), op);
@@ -3470,14 +3316,12 @@ tls_pre_decrypt(struct tls_multi *multi,
}
/* hard reset ? */
- if (is_hard_reset(op, 0))
+ if (is_hard_reset_method2(op))
{
/* verify client -> server or server -> client connection */
- if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
- || op == P_CONTROL_HARD_RESET_CLIENT_V2
+ if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
|| op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
- || ((op == P_CONTROL_HARD_RESET_SERVER_V1
- || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
+ || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
{
msg(D_TLS_ERRORS,
"TLS Error: client->client or server->server connection attempted from %s",
@@ -3542,19 +3386,11 @@ tls_pre_decrypt(struct tls_multi *multi,
* Initial packet received.
*/
- if (i == TM_SIZE && is_hard_reset(op, 0))
+ if (i == TM_SIZE && is_hard_reset_method2(op))
{
struct tls_session *session = &multi->session[TM_ACTIVE];
struct key_state *ks = &session->key[KS_PRIMARY];
- if (!is_hard_reset(op, multi->opt.key_method))
- {
- msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
- multi->opt.key_method,
- packet_opcode_name(op));
- goto error;
- }
-
/*
* If we have no session currently in progress, the initial packet will
* open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
@@ -3594,7 +3430,7 @@ tls_pre_decrypt(struct tls_multi *multi,
}
}
- if (i == TM_SIZE && is_hard_reset(op, 0))
+ if (i == TM_SIZE && is_hard_reset_method2(op))
{
/*
* No match with existing sessions,
@@ -3614,14 +3450,6 @@ tls_pre_decrypt(struct tls_multi *multi,
goto error;
}
- if (!is_hard_reset(op, multi->opt.key_method))
- {
- msg(D_TLS_ERRORS, "TLS ERROR: new session local/remote key_method mismatch, local key_method=%d, op=%s",
- multi->opt.key_method,
- packet_opcode_name(op));
- goto error;
- }
-
if (!read_control_auth(buf, &session->tls_wrap, from,
session->opt))
{
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 86fb6853..b4ce407c 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -66,8 +66,10 @@
/* indicates key_method >= 2 and client-specific tls-crypt key */
#define P_CONTROL_HARD_RESET_CLIENT_V3 10 /* initial key from client, forget previous state */
-/* define the range of legal opcodes */
-#define P_FIRST_OPCODE 1
+/* define the range of legal opcodes
+ * Since we do no longer support key-method 1 we consider
+ * the v1 op codes invalid */
+#define P_FIRST_OPCODE 3
#define P_LAST_OPCODE 10
/*
@@ -109,11 +111,7 @@
/* Default field in X509 to be username */
#define X509_USERNAME_FIELD_DEFAULT "CN"
-/*
- * Range of key exchange methods
- */
-#define KEY_METHOD_MIN 1
-#define KEY_METHOD_MAX 2
+#define KEY_METHOD_2 2
/* key method taken from lower 4 bits */
#define KEY_METHOD_MASK 0x0F
@@ -585,12 +583,11 @@ void show_tls_performance_stats(void);
void extract_x509_field_test(void);
/**
- * Given a key_method, return true if opcode represents the required form of
- * hard_reset.
+ * Given a key_method, return true if opcode represents the one of the
+ * hard_reset op codes for key-method 2
*
- * If key_method == 0, return true if any form of hard reset is used.
*/
-bool is_hard_reset(int op, int key_method);
+bool is_hard_reset_method2(int op);
void delayed_auth_pass_purge(void);
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index e0b3ee56..d904c31f 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -262,7 +262,6 @@ struct tls_options
#endif
/* from command line */
- int key_method;
bool replay;
bool single_session;
#ifdef ENABLE_OCC
--
2.26.2
|
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:47:55
|
By default OpenSSL 1.1+ only allows signatures and ecdh/ecdhx from the
default list of X25519:secp256r1:X448:secp521r1:secp384r1. In
TLS1.3 key exchange is independent from the signature/key of the
certificates, so allowing all groups per default is not a sensible
choice anymore and instead a shorter list is reasonable.
However, when using certificates with exotic curves that are not on
the group list, the signatures of these certificates will no longer
be accepted.
The tls-groups option allows to modify the group list to account
for these corner cases.
Patch V2: Uses local gc_arena instead of malloc/free, reword commit
message. Fix other typos/clarify messages
Patch V3: Style fixes, adjust code to changes from mbed tls session
fix
Patch V5: Fix compilation with OpenSSL 1.0.2
Signed-off-by: Arne Schwabe <ar...@rf...>
---
README.ec | 7 ++--
configure.ac | 1 +
doc/man-sections/encryption-options.rst | 6 +--
doc/man-sections/tls-options.rst | 27 +++++++++++-
src/openvpn/openssl_compat.h | 6 +++
src/openvpn/options.c | 10 ++++-
src/openvpn/options.h | 1 +
src/openvpn/ssl.c | 6 +++
src/openvpn/ssl_backend.h | 10 +++++
src/openvpn/ssl_mbedtls.c | 46 +++++++++++++++++++++
src/openvpn/ssl_mbedtls.h | 1 +
src/openvpn/ssl_openssl.c | 55 ++++++++++++++++++++++++-
12 files changed, 167 insertions(+), 9 deletions(-)
diff --git a/README.ec b/README.ec
index 32938017..61f23b2e 100644
--- a/README.ec
+++ b/README.ec
@@ -12,14 +12,15 @@ OpenVPN 2.4.0 and newer automatically initialize ECDH parameters. When ECDSA is
used for authentication, the curve used for the server certificate will be used
for ECDH too. When autodetection fails (e.g. when using RSA certificates)
OpenVPN lets the crypto library decide if possible, or falls back to the
-secp384r1 curve.
+secp384r1 curve. The list of groups/curves that the crypto library will choose
+from can be set with the --tls-groups <grouplist> option.
An administrator can force an OpenVPN/OpenSSL server to use a specific curve
using the --ecdh-curve <curvename> option with one of the curves listed as
-available by the --show-curves option. Clients will use the same curve as
+available by the --show-groups option. Clients will use the same curve as
selected by the server.
-Note that not all curves listed by --show-curves are available for use with TLS;
+Note that not all curves listed by --show-groups are available for use with TLS;
in that case connecting will fail with a 'no shared cipher' TLS error.
Authentication (ECDSA)
diff --git a/configure.ac b/configure.ac
index a8535b70..dee2c4f2 100644
--- a/configure.ac
+++ b/configure.ac
@@ -927,6 +927,7 @@ if test "${with_crypto_library}" = "openssl"; then
OpenSSL_version \
SSL_CTX_get_default_passwd_cb \
SSL_CTX_get_default_passwd_cb_userdata \
+ SSL_CTX_set1_groups \
SSL_CTX_set_security_level \
X509_get0_notBefore \
X509_get0_notAfter \
diff --git a/doc/man-sections/encryption-options.rst b/doc/man-sections/encryption-options.rst
index a59f636c..ee34f14e 100644
--- a/doc/man-sections/encryption-options.rst
+++ b/doc/man-sections/encryption-options.rst
@@ -27,9 +27,9 @@ SSL Library information
(Standalone) Show currently available hardware-based crypto acceleration
engines supported by the OpenSSL library.
---show-curves
- (Standalone) Show all available elliptic curves to use with the
- ``--ecdh-curve`` option.
+--show-groups
+ (Standalone) Show all available elliptic curves/groups to use with the
+ ``--ecdh-curve`` and ``tls-groups`` options.
Generating key material
-----------------------
diff --git a/doc/man-sections/tls-options.rst b/doc/man-sections/tls-options.rst
index 92b832cd..ccc90ac9 100644
--- a/doc/man-sections/tls-options.rst
+++ b/doc/man-sections/tls-options.rst
@@ -338,6 +338,31 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
Use ``--tls-crypt`` instead if you want to use the key file to not only
authenticate, but also encrypt the TLS control channel.
+--tls-groups list
+ A list of allowable groups/curves in order of preference.
+
+ Set the allowed elictipic curves/groups for the TLS session.
+ These groups are allowed to be used in signatures and key exchange.
+
+ mbed TLS currently allows all known curves per default.
+
+ OpenSSL 1.1+ restricts the list per default to
+ ::
+
+ "X25519:secp256r1:X448:secp521r1:secp384r1".
+
+ If you use certificates that use non-standard curves, you
+ might need to add them here. If you do not force the ecdh curve
+ by using ``--ecdh-curve``, the groups for ecdh will also be picked
+ from this list.
+
+ OpenVPN maps the curve name `secp256r1` to `prime256v1` to allow
+ specifying the same tls-groups option for mbed TLS and OpenSSL.
+
+ Warning: this option not only affects eliptic curve certificates
+ but also the key exchange in TLS 1.3 and using this option improperly
+ will disable TLS 1.3.
+
--tls-cert-profile profile
Set the allowed cryptographic algorithms for certificates according to
``profile``.
@@ -368,7 +393,7 @@ certificates and keys: https://github.com/OpenVPN/easy-rsa
OpenVPN will migrate to 'preferred' as default in the future. Please
ensure that your keys already comply.
-*WARNING:* ``--tls-ciphers`` and ``--tls-ciphersuites``
+*WARNING:* ``--tls-ciphers``, ``--tls-ciphersuites`` and ``tls-groups``
These options are expert features, which - if used correctly - can
improve the security of your VPN connection. But it is also easy to
unwittingly use them to carefully align a gun with your foot, or just
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index d35251fb..eb6c9c90 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -183,6 +183,12 @@ SSL_CTX_get_default_passwd_cb(SSL_CTX *ctx)
}
#endif
+/* This function is implemented as macro, so the configure check for the
+ * function may fail, so we check for both variants here */
+#if !defined(HAVE_SSL_CTX_SET1_GROUPS) && !defined(SSL_CTX_set1_groups)
+#define SSL_CTX_set1_groups SSL_CTX_set1_curves
+#endif
+
#if !defined(HAVE_X509_GET0_PUBKEY)
/**
* Get the public key from a X509 certificate
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index a35d91e7..15173db2 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8058,7 +8058,7 @@ add_option(struct options *options,
VERIFY_PERMISSION(OPT_P_GENERAL);
options->show_tls_ciphers = true;
}
- else if (streq(p[0], "show-curves") && !p[1])
+ else if ((streq(p[0], "show-curves") || streq(p[0], "show-groups")) && !p[1])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
options->show_curves = true;
@@ -8066,6 +8066,9 @@ add_option(struct options *options,
else if (streq(p[0], "ecdh-curve") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
+ msg(M_WARN, "Consider setting groups/curves preference with "
+ "tls-groups instead of forcing a specific curve with "
+ "ecdh-curve.");
options->ecdh_curve = p[1];
}
else if (streq(p[0], "tls-server") && !p[1])
@@ -8236,6 +8239,11 @@ add_option(struct options *options,
VERIFY_PERMISSION(OPT_P_GENERAL);
options->cipher_list_tls13 = p[1];
}
+ else if (streq(p[0], "tls-groups") && p[1] && !p[2])
+ {
+ VERIFY_PERMISSION(OPT_P_GENERAL);
+ options->tls_groups = p[1];
+ }
else if (streq(p[0], "crl-verify") && p[1] && ((p[2] && streq(p[2], "dir"))
|| !p[2]))
{
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index c37006d3..1b038c91 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -542,6 +542,7 @@ struct options
bool pkcs12_file_inline;
const char *cipher_list;
const char *cipher_list_tls13;
+ const char *tls_groups;
const char *tls_cert_profile;
const char *ecdh_curve;
const char *tls_verify;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 04d78a81..00b97352 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -630,6 +630,12 @@ init_ssl(const struct options *options, struct tls_root_ctx *new_ctx)
tls_ctx_restrict_ciphers(new_ctx, options->cipher_list);
tls_ctx_restrict_ciphers_tls13(new_ctx, options->cipher_list_tls13);
+ /* Set the allow groups/curves for TLS if we want to override them */
+ if (options->tls_groups)
+ {
+ tls_ctx_set_tls_groups(new_ctx, options->tls_groups);
+ }
+
if (!tls_ctx_set_options(new_ctx, options->ssl_flags))
{
goto err;
diff --git a/src/openvpn/ssl_backend.h b/src/openvpn/ssl_backend.h
index a1770bd4..75692797 100644
--- a/src/openvpn/ssl_backend.h
+++ b/src/openvpn/ssl_backend.h
@@ -198,6 +198,16 @@ void tls_ctx_restrict_ciphers_tls13(struct tls_root_ctx *ctx, const char *cipher
*/
void tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile);
+/**
+ * Set the allowed (eliptic curve) group allowed for signatures and
+ * key exchange.
+ *
+ * @param ctx TLS context to restrict, must be valid.
+ * @param groups List of groups that will be allowed, in priority,
+ * separated by :
+ */
+void tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups);
+
/**
* Check our certificate notBefore and notAfter fields, and warn if the cert is
* either not yet valid or has expired. Note that this is a non-fatal error,
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index 977ff5c3..9b10e9e4 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -176,6 +176,11 @@ tls_ctx_free(struct tls_root_ctx *ctx)
free(ctx->allowed_ciphers);
}
+ if (ctx->groups)
+ {
+ free(ctx->groups);
+ }
+
CLEAR(*ctx);
ctx->initialised = false;
@@ -343,6 +348,42 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
}
}
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+ ASSERT(ctx);
+ struct gc_arena gc = gc_new();
+
+ /* Get number of groups and allocate an array in ctx */
+ int groups_count = get_num_elements(groups, ':');
+ ALLOC_ARRAY_CLEAR(ctx->groups, mbedtls_ecp_group_id, groups_count + 1)
+
+ /* Parse allowed ciphers, getting IDs */
+ int i = 0;
+ char *tmp_groups = string_alloc(groups, &gc);
+
+ const char *token;
+ while ((token = strsep(&tmp_groups, ":")))
+ {
+ const mbedtls_ecp_curve_info *ci =
+ mbedtls_ecp_curve_info_from_name(token);
+ if (!ci)
+ {
+ msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+ }
+ else
+ {
+ ctx->groups[i] = ci->grp_id;
+ i++;
+ }
+ token = strsep(&tmp_groups, ":");
+ }
+ ctx->groups[i] = MBEDTLS_ECP_DP_NONE;
+
+ gc_free(&gc);
+}
+
+
void
tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
{
@@ -1043,6 +1084,11 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
mbedtls_ssl_conf_ciphersuites(ks_ssl->ssl_config, ssl_ctx->allowed_ciphers);
}
+ if (ssl_ctx->groups)
+ {
+ mbedtls_ssl_conf_curves(ks_ssl->ssl_config, ssl_ctx->groups);
+ }
+
/* Disable record splitting (for now). OpenVPN assumes records are sent
* unfragmented, and changing that will require thorough review and
* testing. Since OpenVPN is not susceptible to BEAST, we can just
diff --git a/src/openvpn/ssl_mbedtls.h b/src/openvpn/ssl_mbedtls.h
index 92381f1a..0525134f 100644
--- a/src/openvpn/ssl_mbedtls.h
+++ b/src/openvpn/ssl_mbedtls.h
@@ -105,6 +105,7 @@ struct tls_root_ctx {
#endif
struct external_context external_key; /**< External key context */
int *allowed_ciphers; /**< List of allowed ciphers for this connection */
+ mbedtls_ecp_group_id *groups; /**< List of allowed groups for this connection */
mbedtls_x509_crt_profile cert_profile; /**< Allowed certificate types */
};
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 14d52bfa..5ba74402 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -563,6 +563,57 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
#endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
}
+void
+tls_ctx_set_tls_groups(struct tls_root_ctx *ctx, const char *groups)
+{
+ ASSERT(ctx);
+ struct gc_arena gc = gc_new();
+ /* This method could be as easy as
+ * SSL_CTX_set1_groups_list(ctx->ctx, groups)
+ * but OpenSSL does not like the name secp256r1 for prime256v1
+ * This is one of the important curves.
+ * To support the same name for OpenSSL and mbedTLS, we do
+ * this dance.
+ */
+
+ int groups_count = get_num_elements(groups, ':');
+
+ int *glist;
+ /* Allocate an array for them */
+ ALLOC_ARRAY_CLEAR_GC(glist, int, groups_count, &gc);
+
+ /* Parse allowed ciphers, getting IDs */
+ int glistlen = 0;
+ char *tmp_groups = string_alloc(groups, &gc);
+
+ const char *token;
+ while ((token = strsep(&tmp_groups, ":")))
+ {
+ if (streq(token, "secp256r1"))
+ {
+ token = "prime256v1";
+ }
+ int nid = OBJ_sn2nid(token);
+
+ if (nid == 0)
+ {
+ msg(M_WARN, "Warning unknown curve/group specified: %s", token);
+ }
+ else
+ {
+ glist[glistlen] = nid;
+ glistlen++;
+ }
+ }
+
+ if (!SSL_CTX_set1_groups(ctx->ctx, glist, glistlen))
+ {
+ crypto_msg(M_FATAL, "Failed to set allowed TLS group list: %s",
+ groups);
+ }
+ gc_free(&gc);
+}
+
void
tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
{
@@ -2135,6 +2186,8 @@ show_available_tls_ciphers_list(const char *cipher_list,
void
show_available_curves(void)
{
+ printf("Consider using openssl 'ecparam -list_curves' as\n"
+ "alternative to running this command.\n");
#ifndef OPENSSL_NO_EC
EC_builtin_curve *curves = NULL;
size_t crv_len = 0;
@@ -2144,7 +2197,7 @@ show_available_curves(void)
ALLOC_ARRAY(curves, EC_builtin_curve, crv_len);
if (EC_get_builtin_curves(curves, crv_len))
{
- printf("Available Elliptic curves:\n");
+ printf("\nAvailable Elliptic curves/groups:\n");
for (n = 0; n < crv_len; n++)
{
const char *sname;
--
2.26.2
|
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:47:56
|
All supported crypto libraries have AEAD support and with our
ncp/de facto default cipher AES-256-GCM we do not want to support
the obscure corner case of a library with disabled AEAD.
Signed-off-by: Arne Schwabe <ar...@rf...>
---
configure.ac | 7 ++-----
src/openvpn/crypto.c | 11 -----------
src/openvpn/crypto_mbedtls.c | 14 --------------
src/openvpn/crypto_openssl.c | 16 ----------------
src/openvpn/crypto_openssl.h | 4 ----
src/openvpn/options.c | 6 ------
6 files changed, 2 insertions(+), 56 deletions(-)
diff --git a/configure.ac b/configure.ac
index d9ad80b1..a8535b70 100644
--- a/configure.ac
+++ b/configure.ac
@@ -905,11 +905,10 @@ if test "${with_crypto_library}" = "openssl"; then
AC_DEFINE([HAVE_OPENSSL_ENGINE], [1], [OpenSSL engine support available])
fi
- have_crypto_aead_modes="yes"
AC_CHECK_FUNC(
[EVP_aes_256_gcm],
,
- [have_crypto_aead_modes="no"]
+ [AC_MSG_ERROR([OpenSSL check for AES-256-GCM support failed])]
)
# All supported OpenSSL version (>= 1.0.2)
@@ -1003,14 +1002,13 @@ elif test "${with_crypto_library}" = "mbedtls"; then
[AC_MSG_ERROR([mbed TLS 2.y.z required])]
)
- have_crypto_aead_modes="yes"
AC_CHECK_FUNCS(
[ \
mbedtls_cipher_write_tag \
mbedtls_cipher_check_tag \
],
,
- [have_crypto_aead_modes="no"; break]
+ [AC_MSG_ERROR([mbed TLS check for AEAD support failed])]
)
have_export_keying_material="yes"
@@ -1225,7 +1223,6 @@ test "${enable_pf}" = "yes" && AC_DEFINE([ENABLE_PF], [1], [Enable internal pack
test "${enable_strict_options}" = "yes" && AC_DEFINE([ENABLE_STRICT_OPTIONS_CHECK], [1], [Enable strict options check between peers])
test "${enable_crypto_ofb_cfb}" = "yes" && AC_DEFINE([ENABLE_OFB_CFB_MODE], [1], [Enable OFB and CFB cipher modes])
-test "${have_crypto_aead_modes}" = "yes" && AC_DEFINE([HAVE_AEAD_CIPHER_MODES], [1], [Use crypto library])
if test "${have_export_keying_material}" = "yes"; then
AC_DEFINE(
[HAVE_EXPORT_KEYING_MATERIAL], [1],
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index bbf47ef7..e92a0dc1 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -64,7 +64,6 @@ static void
openvpn_encrypt_aead(struct buffer *buf, struct buffer work,
struct crypto_options *opt)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
struct gc_arena gc;
int outlen = 0;
const struct key_ctx *ctx = &opt->key_ctx_bi.encrypt;
@@ -152,9 +151,6 @@ err:
buf->len = 0;
gc_free(&gc);
return;
-#else /* HAVE_AEAD_CIPHER_MODES */
- ASSERT(0);
-#endif /* ifdef HAVE_AEAD_CIPHER_MODES */
}
static void
@@ -361,7 +357,6 @@ openvpn_decrypt_aead(struct buffer *buf, struct buffer work,
struct crypto_options *opt, const struct frame *frame,
const uint8_t *ad_start)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
static const char error_prefix[] = "AEAD Decrypt error";
struct packet_id_net pin = { 0 };
const struct key_ctx *ctx = &opt->key_ctx_bi.decrypt;
@@ -482,10 +477,6 @@ error_exit:
buf->len = 0;
gc_free(&gc);
return false;
-#else /* HAVE_AEAD_CIPHER_MODES */
- ASSERT(0);
- return false;
-#endif /* ifdef HAVE_AEAD_CIPHER_MODES */
}
/*
@@ -1104,7 +1095,6 @@ test_crypto(struct crypto_options *co, struct frame *frame)
/* init work */
ASSERT(buf_init(&work, FRAME_HEADROOM(frame)));
-#ifdef HAVE_AEAD_CIPHER_MODES
/* init implicit IV */
{
const cipher_kt_t *cipher =
@@ -1126,7 +1116,6 @@ test_crypto(struct crypto_options *co, struct frame *frame)
co->key_ctx_bi.decrypt.implicit_iv_len = impl_iv_len;
}
}
-#endif /* ifdef HAVE_AEAD_CIPHER_MODES */
msg(M_INFO, "Entering " PACKAGE_NAME " crypto self-test mode.");
for (i = 1; i <= TUN_MTU_SIZE(frame); ++i)
diff --git a/src/openvpn/crypto_mbedtls.c b/src/openvpn/crypto_mbedtls.c
index bb752557..19a87eb4 100644
--- a/src/openvpn/crypto_mbedtls.c
+++ b/src/openvpn/crypto_mbedtls.c
@@ -530,12 +530,10 @@ cipher_kt_block_size(const mbedtls_cipher_info_t *cipher_kt)
int
cipher_kt_tag_size(const mbedtls_cipher_info_t *cipher_kt)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
if (cipher_kt && cipher_kt_mode_aead(cipher_kt))
{
return OPENVPN_AEAD_TAG_LENGTH;
}
-#endif
return 0;
}
@@ -632,7 +630,6 @@ cipher_ctx_iv_length(const mbedtls_cipher_context_t *ctx)
int
cipher_ctx_get_tag(cipher_ctx_t *ctx, uint8_t *tag, int tag_len)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
if (tag_len > SIZE_MAX)
{
return 0;
@@ -644,9 +641,6 @@ cipher_ctx_get_tag(cipher_ctx_t *ctx, uint8_t *tag, int tag_len)
}
return 1;
-#else /* ifdef HAVE_AEAD_CIPHER_MODES */
- ASSERT(0);
-#endif /* HAVE_AEAD_CIPHER_MODES */
}
int
@@ -688,7 +682,6 @@ cipher_ctx_reset(mbedtls_cipher_context_t *ctx, const uint8_t *iv_buf)
int
cipher_ctx_update_ad(cipher_ctx_t *ctx, const uint8_t *src, int src_len)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
if (src_len > SIZE_MAX)
{
return 0;
@@ -700,9 +693,6 @@ cipher_ctx_update_ad(cipher_ctx_t *ctx, const uint8_t *src, int src_len)
}
return 1;
-#else /* ifdef HAVE_AEAD_CIPHER_MODES */
- ASSERT(0);
-#endif /* HAVE_AEAD_CIPHER_MODES */
}
int
@@ -741,7 +731,6 @@ int
cipher_ctx_final_check_tag(mbedtls_cipher_context_t *ctx, uint8_t *dst,
int *dst_len, uint8_t *tag, size_t tag_len)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
size_t olen = 0;
if (MBEDTLS_DECRYPT != ctx->operation)
@@ -773,9 +762,6 @@ cipher_ctx_final_check_tag(mbedtls_cipher_context_t *ctx, uint8_t *dst,
}
return 1;
-#else /* ifdef HAVE_AEAD_CIPHER_MODES */
- ASSERT(0);
-#endif /* HAVE_AEAD_CIPHER_MODES */
}
void
diff --git a/src/openvpn/crypto_openssl.c b/src/openvpn/crypto_openssl.c
index 161a189e..30c705b9 100644
--- a/src/openvpn/crypto_openssl.c
+++ b/src/openvpn/crypto_openssl.c
@@ -301,9 +301,7 @@ show_available_ciphers(void)
#ifdef ENABLE_OFB_CFB_MODE
|| cipher_kt_mode_ofb_cfb(cipher)
#endif
-#ifdef HAVE_AEAD_CIPHER_MODES
|| cipher_kt_mode_aead(cipher)
-#endif
))
{
cipher_list[num_ciphers++] = cipher;
@@ -732,7 +730,6 @@ cipher_kt_mode_ofb_cfb(const cipher_kt_t *cipher)
bool
cipher_kt_mode_aead(const cipher_kt_t *cipher)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
if (cipher)
{
switch (EVP_CIPHER_nid(cipher))
@@ -746,7 +743,6 @@ cipher_kt_mode_aead(const cipher_kt_t *cipher)
return true;
}
}
-#endif
return false;
}
@@ -806,11 +802,7 @@ cipher_ctx_iv_length(const EVP_CIPHER_CTX *ctx)
int
cipher_ctx_get_tag(EVP_CIPHER_CTX *ctx, uint8_t *tag_buf, int tag_size)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
return EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_GET_TAG, tag_size, tag_buf);
-#else
- ASSERT(0);
-#endif
}
int
@@ -841,16 +833,12 @@ cipher_ctx_reset(EVP_CIPHER_CTX *ctx, const uint8_t *iv_buf)
int
cipher_ctx_update_ad(EVP_CIPHER_CTX *ctx, const uint8_t *src, int src_len)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
int len;
if (!EVP_CipherUpdate(ctx, NULL, &len, src, src_len))
{
crypto_msg(M_FATAL, "%s: EVP_CipherUpdate() failed", __func__);
}
return 1;
-#else /* ifdef HAVE_AEAD_CIPHER_MODES */
- ASSERT(0);
-#endif
}
int
@@ -874,7 +862,6 @@ int
cipher_ctx_final_check_tag(EVP_CIPHER_CTX *ctx, uint8_t *dst, int *dst_len,
uint8_t *tag, size_t tag_len)
{
-#ifdef HAVE_AEAD_CIPHER_MODES
ASSERT(tag_len < SIZE_MAX);
if (!EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_GCM_SET_TAG, tag_len, tag))
{
@@ -882,9 +869,6 @@ cipher_ctx_final_check_tag(EVP_CIPHER_CTX *ctx, uint8_t *dst, int *dst_len,
}
return cipher_ctx_final(ctx, dst, dst_len);
-#else /* ifdef HAVE_AEAD_CIPHER_MODES */
- ASSERT(0);
-#endif
}
void
diff --git a/src/openvpn/crypto_openssl.h b/src/openvpn/crypto_openssl.h
index 4694ee08..e6f8f537 100644
--- a/src/openvpn/crypto_openssl.h
+++ b/src/openvpn/crypto_openssl.h
@@ -61,13 +61,9 @@ typedef HMAC_CTX hmac_ctx_t;
/** Cipher is in CFB mode */
#define OPENVPN_MODE_CFB EVP_CIPH_CFB_MODE
-#ifdef HAVE_AEAD_CIPHER_MODES
-
/** Cipher is in GCM mode */
#define OPENVPN_MODE_GCM EVP_CIPH_GCM_MODE
-#endif /* HAVE_AEAD_CIPHER_MODES */
-
/** Cipher should encrypt */
#define OPENVPN_OP_ENCRYPT 1
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index a20b27c9..a35d91e7 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -104,9 +104,7 @@ const char title_string[] =
" [MH/RECVDA]"
#endif
#endif
-#ifdef HAVE_AEAD_CIPHER_MODES
" [AEAD]"
-#endif
" built on " __DATE__
;
@@ -871,11 +869,7 @@ init_options(struct options *o, const bool init_gc)
o->scheduled_exit_interval = 5;
#endif
o->ciphername = "BF-CBC";
-#ifdef HAVE_AEAD_CIPHER_MODES /* IV_NCP=2 requires GCM support */
o->ncp_enabled = true;
-#else
- o->ncp_enabled = false;
-#endif
o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
o->authname = "SHA1";
o->prng_hash = "SHA1";
--
2.26.2
|
|
From: Steffan K. <ste...@fo...> - 2020-07-20 11:54:52
|
Hi, On 17-07-2020 15:47, Arne Schwabe wrote: > All supported crypto libraries have AEAD support and with our > ncp/de facto default cipher AES-256-GCM we do not want to support > the obscure corner case of a library with disabled AEAD. Again: feature-ACK, but some comments. config-msvc.h still has a #define HAVE_AEAD_CIPHER_MODES 1 and ssl_openssl.c has two occurances of #ifdef EVP_CIPH_FLAG_AEAD_CIPHER which I think can go too now. -Steffan |
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:47:53
|
The change in name signals that data-ciphers is the preferred way to
configure data channel (and not --cipher). The data prefix is chosen
to avoid ambiguity and make it distinct from tls-cipher for the TLS
ciphers.
Signed-off-by: Arne Schwabe <ar...@rf...>
---
Changes.rst | 13 ++++++++++---
doc/man-sections/protocol-options.rst | 11 +++++++----
doc/man-sections/server-options.rst | 4 ++--
sample/sample-config-files/client.conf | 2 +-
src/openvpn/multi.c | 4 ++--
src/openvpn/options.c | 5 +++--
src/openvpn/ssl_ncp.c | 4 ++--
7 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/Changes.rst b/Changes.rst
index 6e283270..2158c8e7 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -14,12 +14,19 @@ ChaCha20-Poly1305 cipher support
channel.
Improved Data channel cipher negotiation
+ The option ``ncp-ciphers`` has been renamed to ``data-ciphers``.
+ The old name is still accepted. The change in name signals that
+ ``data-ciphers`` is the preferred way to configure data channel
+ ciphers and the data prefix is chosen to avoid the ambiguity that
+ exists with ``--cipher`` for the data cipher and ``tls-cipher``
+ for the TLS ciphers.
+
OpenVPN clients will now signal all supported ciphers from the
- ``ncp-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
- servers will select the first common cipher from the ``ncp-ciphers``
+ ``data-ciphers`` option to the server via ``IV_CIPHERS``. OpenVPN
+ servers will select the first common cipher from the ``data-ciphers``
list instead of blindly pushing the first cipher of the list. This
allows to use a configuration like
- ``ncp-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
+ ``data-ciphers ChaCha20-Poly1305:AES-256-GCM`` on the server that
prefers ChaCha20-Poly1305 but uses it only if the client supports it.
Asynchronous (deferred) authentication support for auth-pam plugin.
diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index 923d2da0..051f1d32 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -62,7 +62,7 @@ configured in a compatible way between both the local and remote side.
The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
Block Chaining mode. When cipher negotiation (NCP) is allowed,
OpenVPN 2.4 and newer on both client and server side will automatically
- upgrade to :code:`AES-256-GCM`. See ``--ncp-ciphers`` and
+ upgrade to :code:`AES-256-GCM`. See ``--data-ciphers`` and
``--ncp-disable`` for more details on NCP.
Using :code:`BF-CBC` is no longer recommended, because of its 64-bit
@@ -169,7 +169,7 @@ configured in a compatible way between both the local and remote side.
non-standard key lengths, and a larger key may offer no real guarantee
of greater security, or may even reduce security.
---ncp-ciphers cipher-list
+--data-ciphers cipher-list
Restrict the allowed ciphers to be negotiated to the ciphers in
``cipher-list``. ``cipher-list`` is a colon-separated list of ciphers,
and defaults to :code:`AES-256-GCM:AES-128-GCM`.
@@ -189,9 +189,9 @@ configured in a compatible way between both the local and remote side.
Additionally, to allow for more smooth transition, if NCP is enabled,
OpenVPN will inherit the cipher of the peer if that cipher is different
from the local ``--cipher`` setting, but the peer cipher is one of the
- ciphers specified in ``--ncp-ciphers``. E.g. a non-NCP client (<=v2.3,
+ ciphers specified in ``--data-ciphers``. E.g. a non-NCP client (<=v2.3,
or with --ncp-disabled set) connecting to a NCP server (v2.4+) with
- ``--cipher BF-CBC`` and ``--ncp-ciphers AES-256-GCM:AES-256-CBC`` set can
+ ``--cipher BF-CBC`` and ``--data-ciphers AES-256-GCM:AES-256-CBC`` set can
either specify ``--cipher BF-CBC`` or ``--cipher AES-256-CBC`` and both
will work.
@@ -201,6 +201,9 @@ configured in a compatible way between both the local and remote side.
This list is restricted to be 127 chars long after conversion to OpenVPN
ciphers.
+ This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
+ to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
+
--ncp-disable
Disable "Negotiable Crypto Parameters". This completely disables cipher
negotiation.
diff --git a/doc/man-sections/server-options.rst b/doc/man-sections/server-options.rst
index c24aec0b..74ad5e18 100644
--- a/doc/man-sections/server-options.rst
+++ b/doc/man-sections/server-options.rst
@@ -473,8 +473,8 @@ fast hardware. SSL/TLS authentication must be used in this mode.
*AES-GCM-128* and *AES-GCM-256*.
:code:`IV_CIPHERS=<ncp-ciphers>`
- The client pushes the list of configured ciphers with the
- ``--ciphers`` option to the server.
+ The client announces the list of supported ciphers configured with the
+ ``--data-ciphers`` option to the server.
:code:`IV_GUI_VER=<gui_id> <version>`
The UI version of a UI if one is running, for example
diff --git a/sample/sample-config-files/client.conf b/sample/sample-config-files/client.conf
index 7f2f30a3..47ca4099 100644
--- a/sample/sample-config-files/client.conf
+++ b/sample/sample-config-files/client.conf
@@ -112,7 +112,7 @@ tls-auth ta.key 1
# then you must also specify it here.
# Note that v2.4 client/server will automatically
# negotiate AES-256-GCM in TLS mode.
-# See also the ncp-cipher option in the manpage
+# See also the data-ciphers option in the manpage
cipher AES-256-CBC
# Enable compression on the VPN link.
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 88ba9db2..d2549bca 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1827,7 +1827,7 @@ multi_client_set_protocol_options(struct context *c)
else
{
/*
- * Push the first cipher from --ncp-ciphers to the client that
+ * Push the first cipher from --data-ciphers to the client that
* the client announces to be supporting.
*/
char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
@@ -1847,7 +1847,7 @@ multi_client_set_protocol_options(struct context *c)
{
msg(M_INFO, "PUSH: No common cipher between server and "
"client. Expect this connection not to work. Server "
- "ncp-ciphers: '%s', client supported ciphers '%s'",
+ "data-ciphers: '%s', client supported ciphers '%s'",
o->ncp_ciphers, peer_ciphers);
}
else
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 31e33ae3..896abcde 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -536,7 +536,7 @@ static const char usage_message[] =
"--cipher alg : Encrypt packets with cipher algorithm alg\n"
" (default=%s).\n"
" Set alg=none to disable encryption.\n"
- "--ncp-ciphers list : List of ciphers that are allowed to be negotiated.\n"
+ "--data-ciphers list : List of ciphers that are allowed to be negotiated.\n"
"--ncp-disable : (DEPRECATED) Disable cipher negotiation.\n"
"--prng alg [nsl] : For PRNG, use digest algorithm alg, and\n"
" nonce_secret_len=nsl. Set alg=none to disable PRNG.\n"
@@ -7866,7 +7866,8 @@ add_option(struct options *options,
VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
options->ciphername = p[1];
}
- else if (streq(p[0], "ncp-ciphers") && p[1] && !p[2])
+ else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
+ && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_INSTANCE);
options->ncp_ciphers = p[1];
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index e057a40b..6760884e 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -111,7 +111,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
const cipher_kt_t *ktc = cipher_kt_get(token);
if (!ktc)
{
- msg(M_WARN, "Unsupported cipher in --ncp-ciphers: %s", token);
+ msg(M_WARN, "Unsupported cipher in --data-ciphers: %s", token);
error_found = true;
}
else
@@ -130,7 +130,7 @@ mutate_ncp_cipher_list(const char *list, struct gc_arena *gc)
if (!(buf_forward_capacity(&new_list) >
strlen(ovpn_cipher_name) + 2))
{
- msg(M_WARN, "Length of --ncp-ciphers is over the "
+ msg(M_WARN, "Length of --data-ciphers is over the "
"limit of 127 chars");
error_found = true;
}
--
2.26.2
|
|
From: Gert D. <ge...@gr...> - 2020-07-27 08:08:30
|
Your patch has been applied to the master branch.
I have not done more than basic compile-time testing and a quick
glance at "git show" here.
Patch 10/9 with the message will follow right away.
commit 30d19c6ebee05de4cc35155a6c7742ccbf021a82
Author: Arne Schwabe
Date: Fri Jul 17 15:47:38 2020 +0200
Rename ncp-ciphers to data-ciphers
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: Steffan Karger <ste...@fo...>
Message-Id: <202...@rf...>
URL: https://www.mail-archive.com/ope...@li.../msg20444.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:47:57
|
Commit 037669f3dd already made occ being unconditionally on. This commit
only removes the #ifdefs
Signed-off-by: Arne Schwabe <ar...@rf...>
---
src/openvpn/forward.c | 8 --------
src/openvpn/init.c | 16 +---------------
src/openvpn/occ.c | 9 ---------
src/openvpn/occ.h | 3 ---
src/openvpn/openvpn.h | 7 +------
src/openvpn/options.c | 30 ------------------------------
src/openvpn/options.h | 8 --------
src/openvpn/sig.c | 6 ------
src/openvpn/sig.h | 3 ---
src/openvpn/ssl.c | 21 +--------------------
src/openvpn/ssl_common.h | 4 ----
src/openvpn/syshead.h | 5 -----
12 files changed, 3 insertions(+), 117 deletions(-)
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 698451d1..3d462d0a 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -822,7 +822,6 @@ process_coarse_timers(struct context *c)
}
#endif
-#ifdef ENABLE_OCC
/* Should we send an OCC_REQUEST message? */
check_send_occ_req(c);
@@ -834,7 +833,6 @@ process_coarse_timers(struct context *c)
{
process_explicit_exit_notification_timer_wakeup(c);
}
-#endif
/* Should we ping the remote? */
check_ping_send(c);
@@ -983,14 +981,12 @@ read_incoming_link(struct context *c)
}
else
{
-#ifdef ENABLE_OCC
if (event_timeout_defined(&c->c2.explicit_exit_notification_interval))
{
msg(D_STREAM_ERRORS, "Connection reset during exit notification period, ignoring [%d]", status);
management_sleep(1);
}
else
-#endif
{
register_signal(c, SIGUSR1, "connection-reset"); /* SOFT-SIGUSR1 -- TCP connection reset */
msg(D_STREAM_ERRORS, "Connection reset, restarting [%d]", status);
@@ -1214,13 +1210,11 @@ process_incoming_link_part2(struct context *c, struct link_socket_info *lsi, con
c->c2.buf.len = 0; /* drop packet */
}
-#ifdef ENABLE_OCC
/* Did we just receive an OCC packet? */
if (is_occ_msg(&c->c2.buf))
{
process_received_occ_msg(c);
}
-#endif
buffer_turnover(orig_buf, &c->c2.to_tun, &c->c2.buf, &c->c2.buffers->read_link_buf);
@@ -1992,10 +1986,8 @@ pre_select(struct context *c)
/* check for incoming configuration info on the control channel */
check_incoming_control_channel(c);
-#ifdef ENABLE_OCC
/* Should we send an OCC message? */
check_send_occ_msg(c);
-#endif
#ifdef ENABLE_FRAGMENT
/* Should we deliver a datagram fragment to remote? */
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index b96d1471..1ea4735d 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -1419,7 +1419,6 @@ do_init_timers(struct context *c, bool deferred)
/* initialize connection establishment timer */
event_timeout_init(&c->c2.wait_for_connect, 1, now);
-#ifdef ENABLE_OCC
/* initialize occ timers */
if (c->options.occ
@@ -1433,7 +1432,6 @@ do_init_timers(struct context *c, bool deferred)
{
event_timeout_init(&c->c2.occ_mtu_load_test_interval, OCC_MTU_LOAD_INTERVAL_SECONDS, now);
}
-#endif
/* initialize packet_id persistence timer */
if (c->options.packet_id_file)
@@ -2279,7 +2277,6 @@ do_deferred_options(struct context *c, const unsigned int found)
msg(D_PUSH, "OPTIONS IMPORT: timers and/or timeouts modified");
}
-#ifdef ENABLE_OCC
if (found & OPT_P_EXPLICIT_NOTIFY)
{
if (!proto_is_udp(c->options.ce.proto) && c->options.ce.explicit_exit_notification)
@@ -2292,7 +2289,6 @@ do_deferred_options(struct context *c, const unsigned int found)
msg(D_PUSH, "OPTIONS IMPORT: explicit notify parm(s) modified");
}
}
-#endif
#ifdef USE_COMP
if (found & OPT_P_COMP)
@@ -2901,9 +2897,7 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
to.xmit_hold = true;
}
-#ifdef ENABLE_OCC
to.disable_occ = !options->occ;
-#endif
to.verify_command = options->tls_verify;
to.verify_export_cert = options->tls_export_cert;
@@ -3193,7 +3187,7 @@ do_init_frame(struct context *c)
c->c2.frame_fragment_initial = c->c2.frame_fragment;
#endif
-#if defined(ENABLE_FRAGMENT) && defined(ENABLE_OCC)
+#if defined(ENABLE_FRAGMENT)
/*
* MTU advisories
*/
@@ -3478,7 +3472,6 @@ do_print_data_channel_mtu_parms(struct context *c)
#endif
}
-#ifdef ENABLE_OCC
/*
* Get local and remote options compatibility strings.
*/
@@ -3510,7 +3503,6 @@ do_compute_occ_strings(struct context *c)
gc_free(&gc);
}
-#endif /* ifdef ENABLE_OCC */
/*
* These things can only be executed once per program instantiation.
@@ -3586,7 +3578,6 @@ do_close_tls(struct context *c)
c->c2.tls_multi = NULL;
}
-#ifdef ENABLE_OCC
/* free options compatibility strings */
if (c->c2.options_string_local)
{
@@ -3597,7 +3588,6 @@ do_close_tls(struct context *c)
free(c->c2.options_string_remote);
}
c->c2.options_string_local = c->c2.options_string_remote = NULL;
-#endif
if (c->c2.pulled_options_state)
{
@@ -4256,13 +4246,11 @@ init_instance(struct context *c, const struct env_set *env, const unsigned int f
do_open_ifconfig_pool_persist(c);
}
-#ifdef ENABLE_OCC
/* reset OCC state */
if (c->mode == CM_P2P || child)
{
c->c2.occ_op = occ_reset_op();
}
-#endif
/* our wait-for-i/o objects, different for posix vs. win32 */
if (c->mode == CM_P2P)
@@ -4362,13 +4350,11 @@ init_instance(struct context *c, const struct env_set *env, const unsigned int f
/* print MTU info */
do_print_data_channel_mtu_parms(c);
-#ifdef ENABLE_OCC
/* get local and remote options compatibility strings */
if (c->mode == CM_P2P || child)
{
do_compute_occ_strings(c);
}
-#endif
/* initialize output speed limiter */
if (c->mode == CM_P2P)
diff --git a/src/openvpn/occ.c b/src/openvpn/occ.c
index 70c578fb..3ff351aa 100644
--- a/src/openvpn/occ.c
+++ b/src/openvpn/occ.c
@@ -29,8 +29,6 @@
#include "syshead.h"
-#ifdef ENABLE_OCC
-
#include "occ.h"
#include "forward.h"
#include "memdbg.h"
@@ -424,10 +422,3 @@ process_received_occ_msg(struct context *c)
}
c->c2.buf.len = 0; /* don't pass packet on */
}
-
-#else /* ifdef ENABLE_OCC */
-static void
-dummy(void)
-{
-}
-#endif /* ifdef ENABLE_OCC */
diff --git a/src/openvpn/occ.h b/src/openvpn/occ.h
index e3abd8cb..504c8c43 100644
--- a/src/openvpn/occ.h
+++ b/src/openvpn/occ.h
@@ -24,8 +24,6 @@
#ifndef OCC_H
#define OCC_H
-#ifdef ENABLE_OCC
-
#include "forward.h"
/* OCC_STRING_SIZE must be set to sizeof (occ_magic) */
@@ -155,5 +153,4 @@ check_send_occ_msg(struct context *c)
}
}
-#endif /* ifdef ENABLE_OCC */
#endif /* ifndef OCC_H */
diff --git a/src/openvpn/openvpn.h b/src/openvpn/openvpn.h
index ccc7f118..a4191a3b 100644
--- a/src/openvpn/openvpn.h
+++ b/src/openvpn/openvpn.h
@@ -325,7 +325,6 @@ struct context_2
struct event_timeout inactivity_interval;
int inactivity_bytes;
-#ifdef ENABLE_OCC
/* the option strings must match across peers */
char *options_string_local;
char *options_string_remote;
@@ -333,7 +332,6 @@ struct context_2
int occ_op; /* INIT to -1 */
int occ_n_tries;
struct event_timeout occ_interval;
-#endif
/*
* Keep track of maximum packet size received so far
@@ -345,13 +343,12 @@ struct context_2
int max_send_size_local; /* max packet size sent */
int max_send_size_remote; /* max packet size sent by remote */
-#ifdef ENABLE_OCC
+
/* remote wants us to send back a load test packet of this size */
int occ_mtu_load_size;
struct event_timeout occ_mtu_load_test_interval;
int occ_mtu_load_n_tries;
-#endif
/*
* TLS-mode crypto objects.
@@ -438,13 +435,11 @@ struct context_2
/* indicates that the do_up_delay function has run */
bool do_up_ran;
-#ifdef ENABLE_OCC
/* indicates that we have received a SIGTERM when
* options->explicit_exit_notification is enabled,
* but we have not exited yet */
time_t explicit_exit_notification_time_wait;
struct event_timeout explicit_exit_notification_interval;
-#endif
/* environmental variables to pass to scripts */
struct env_set *es;
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 0025c526..31e33ae3 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -277,9 +277,7 @@ static const char usage_message[] =
" 'no' -- Never send DF (Don't Fragment) frames\n"
" 'maybe' -- Use per-route hints\n"
" 'yes' -- Always DF (Don't Fragment)\n"
-#ifdef ENABLE_OCC
"--mtu-test : Empirically measure and report MTU.\n"
-#endif
#ifdef ENABLE_FRAGMENT
"--fragment max : Enable internal datagram fragmentation so that no UDP\n"
" datagrams are sent which are larger than max bytes.\n"
@@ -350,9 +348,7 @@ static const char usage_message[] =
"--status file n : Write operational status to file every n seconds.\n"
"--status-version [n] : Choose the status file format version number.\n"
" Currently, n can be 1, 2, or 3 (default=1).\n"
-#ifdef ENABLE_OCC
"--disable-occ : Disable options consistency check between peers.\n"
-#endif
#ifdef ENABLE_DEBUG
"--gremlin mask : Special stress testing mode (for debugging only).\n"
#endif
@@ -522,10 +518,8 @@ static const char usage_message[] =
"--allow-recursive-routing : When this option is set, OpenVPN will not drop\n"
" incoming tun packets with same destination as host.\n"
#endif /* if P2MP */
-#ifdef ENABLE_OCC
"--explicit-exit-notify [n] : On exit/restart, send exit signal to\n"
" server/remote. n = # of retries, default=1.\n"
-#endif
"\n"
"Data Channel Encryption Options (must be compatible between peers):\n"
"(These options are meaningful for both Static Key & TLS-mode)\n"
@@ -832,9 +826,7 @@ init_options(struct options *o, const bool init_gc)
o->resolve_retry_seconds = RESOLV_RETRY_INFINITE;
o->resolve_in_advance = false;
o->proto_force = -1;
-#ifdef ENABLE_OCC
o->occ = true;
-#endif
#ifdef ENABLE_MANAGEMENT
o->management_log_history_cache = 250;
o->management_echo_buffer_size = 100;
@@ -1483,9 +1475,7 @@ show_connection_entry(const struct connection_entry *o)
#endif
SHOW_INT(mssfix);
-#ifdef ENABLE_OCC
SHOW_INT(explicit_exit_notification);
-#endif
SHOW_STR(tls_auth_file);
SHOW_PARM(key_direction, keydirection2ascii(o->key_direction, false, true),
@@ -1579,9 +1569,7 @@ show_settings(const struct options *o)
#ifdef ENABLE_FEATURE_SHAPER
SHOW_INT(shaper);
#endif
-#ifdef ENABLE_OCC
SHOW_INT(mtu_test);
-#endif
SHOW_BOOL(mlock);
@@ -1633,9 +1621,7 @@ show_settings(const struct options *o)
SHOW_INT(status_file_version);
SHOW_INT(status_file_update_freq);
-#ifdef ENABLE_OCC
SHOW_BOOL(occ);
-#endif
SHOW_INT(rcvbuf);
SHOW_INT(sndbuf);
#if defined(TARGET_LINUX) && HAVE_DECL_SO_MARK
@@ -2079,12 +2065,10 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
msg(M_USAGE, "only one of --tun-mtu or --link-mtu may be defined (note that --ifconfig implies --link-mtu %d)", LINK_MTU_DEFAULT);
}
-#ifdef ENABLE_OCC
if (!proto_is_udp(ce->proto) && options->mtu_test)
{
msg(M_USAGE, "--mtu-test only makes sense with --proto udp");
}
-#endif
/* will we be pulling options from server? */
#if P2MP
@@ -2217,12 +2201,10 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
}
#endif
-#ifdef ENABLE_OCC
if (!proto_is_udp(ce->proto) && ce->explicit_exit_notification)
{
msg(M_USAGE, "--explicit-exit-notify can only be used with --proto udp");
}
-#endif
if (!ce->remote && ce->proto == PROTO_TCP_CLIENT)
{
@@ -3587,9 +3569,6 @@ pre_pull_restore(struct options *o, struct gc_arena *gc)
}
#endif /* if P2MP */
-
-#ifdef ENABLE_OCC
-
/**
* Calculate the link-mtu to advertise to our peer. The actual value is not
* relevant, because we will possibly perform data channel cipher negotiation
@@ -3619,7 +3598,6 @@ calc_options_string_link_mtu(const struct options *o, const struct frame *frame)
}
return link_mtu;
}
-
/*
* Build an options string to represent data channel encryption options.
* This string must match exactly between peers. The keysize is checked
@@ -4027,8 +4005,6 @@ options_string_version(const char *s, struct gc_arena *gc)
return BSTR(&out);
}
-#endif /* ENABLE_OCC */
-
char *
options_string_extract_option(const char *options_string,const char *opt_name,
struct gc_arena *gc)
@@ -6028,13 +6004,11 @@ add_option(struct options *options,
VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION);
options->ce.mtu_discover_type = translate_mtu_discover_type_name(p[1]);
}
-#ifdef ENABLE_OCC
else if (streq(p[0], "mtu-test") && !p[1])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
options->mtu_test = true;
}
-#endif
else if (streq(p[0], "nice") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_NICE);
@@ -6345,7 +6319,6 @@ add_option(struct options *options,
VERIFY_PERMISSION(OPT_P_TIMER);
options->ping_timer_remote = true;
}
-#ifdef ENABLE_OCC
else if (streq(p[0], "explicit-exit-notify") && !p[2])
{
VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_CONNECTION|OPT_P_EXPLICIT_NOTIFY);
@@ -6358,7 +6331,6 @@ add_option(struct options *options,
options->ce.explicit_exit_notification = 1;
}
}
-#endif
else if (streq(p[0], "persist-tun") && !p[1])
{
VERIFY_PERMISSION(OPT_P_PERSIST);
@@ -6682,13 +6654,11 @@ add_option(struct options *options,
}
}
-#ifdef ENABLE_OCC
else if (streq(p[0], "disable-occ") && !p[1])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
options->occ = false;
}
-#endif
#if P2MP
else if (streq(p[0], "server") && p[1] && p[2] && !p[4])
{
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 3546bab3..c5df2d18 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -262,9 +262,7 @@ struct options
int proto_force;
-#ifdef ENABLE_OCC
bool mtu_test;
-#endif
#ifdef ENABLE_MEMSTATS
char *memstats_fn;
@@ -375,10 +373,8 @@ struct options
bool allow_pull_fqdn; /* as a client, allow server to push a FQDN for certain parameters */
struct client_nat_option_list *client_nat;
-#ifdef ENABLE_OCC
/* Enable options consistency check between peers */
bool occ;
-#endif
#ifdef ENABLE_MANAGEMENT
const char *management_addr;
@@ -756,8 +752,6 @@ void show_settings(const struct options *o);
bool string_defined_equal(const char *s1, const char *s2);
-#ifdef ENABLE_OCC
-
const char *options_string_version(const char *s, struct gc_arena *gc);
char *options_string(const struct options *o,
@@ -775,8 +769,6 @@ bool options_cmp_equal(char *actual, const char *expected);
void options_warning(char *actual, const char *expected);
-#endif
-
/**
* Given an OpenVPN options string, extract the value of an option.
*
diff --git a/src/openvpn/sig.c b/src/openvpn/sig.c
index 6e3379fe..24a2878f 100644
--- a/src/openvpn/sig.c
+++ b/src/openvpn/sig.c
@@ -330,7 +330,6 @@ print_status(const struct context *c, struct status_output *so)
gc_free(&gc);
}
-#ifdef ENABLE_OCC
/*
* Handle the triggering and time-wait of explicit
* exit notification.
@@ -367,7 +366,6 @@ process_explicit_exit_notification_timer_wakeup(struct context *c)
}
}
}
-#endif /* ifdef ENABLE_OCC */
/*
* Process signals
@@ -395,14 +393,12 @@ static bool
process_sigterm(struct context *c)
{
bool ret = true;
-#ifdef ENABLE_OCC
if (c->options.ce.explicit_exit_notification
&& !c->c2.explicit_exit_notification_time_wait)
{
process_explicit_exit_notification_init(c);
ret = false;
}
-#endif
return ret;
}
@@ -415,7 +411,6 @@ static bool
ignore_restart_signals(struct context *c)
{
bool ret = false;
-#ifdef ENABLE_OCC
if ( (c->sig->signal_received == SIGUSR1 || c->sig->signal_received == SIGHUP)
&& event_timeout_defined(&c->c2.explicit_exit_notification_interval) )
{
@@ -434,7 +429,6 @@ ignore_restart_signals(struct context *c)
ret = false;
}
}
-#endif
return ret;
}
diff --git a/src/openvpn/sig.h b/src/openvpn/sig.h
index 887d8332..59f30fd0 100644
--- a/src/openvpn/sig.h
+++ b/src/openvpn/sig.h
@@ -81,11 +81,8 @@ bool process_signal(struct context *c);
void register_signal(struct context *c, int sig, const char *text);
-#ifdef ENABLE_OCC
void process_explicit_exit_notification_timer_wakeup(struct context *c);
-#endif
-
#ifdef _WIN32
static inline void
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 4144217d..cb18121a 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -64,21 +64,6 @@
#include "memdbg.h"
-#ifndef ENABLE_OCC
-static const char ssl_default_options_string[] = "V0 UNDEF";
-#endif
-
-
-static inline const char *
-local_options_string(const struct tls_session *session)
-{
-#ifdef ENABLE_OCC
- return session->opt->local_options;
-#else
- return ssl_default_options_string;
-#endif
-}
-
#ifdef MEASURE_TLS_HANDSHAKE_STATS
static int tls_handshake_success; /* GLOBAL */
@@ -1319,11 +1304,9 @@ tls_multi_init_set_options(struct tls_multi *multi,
const char *local,
const char *remote)
{
-#ifdef ENABLE_OCC
/* initialize options string */
multi->opt.local_options = local;
multi->opt.remote_options = remote;
-#endif
}
/*
@@ -2350,7 +2333,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
/* write options string */
{
- if (!write_string(buf, local_options_string(session), TLS_OPTIONS_LEN))
+ if (!write_string(buf, session->opt->local_options, TLS_OPTIONS_LEN))
{
goto error;
}
@@ -2543,7 +2526,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
verify_final_auth_checks(multi, session);
}
-#ifdef ENABLE_OCC
/* check options consistency */
if (!session->opt->disable_occ
&& !options_cmp_equal(options, session->opt->remote_options))
@@ -2555,7 +2537,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
ks->authenticated = KS_AUTH_FALSE;
}
}
-#endif
buf_clear(buf);
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index d904c31f..9f777750 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -254,19 +254,15 @@ struct tls_options
/* if true, don't xmit until first packet from peer is received */
bool xmit_hold;
-#ifdef ENABLE_OCC
/* local and remote options strings
* that must match between client and server */
const char *local_options;
const char *remote_options;
-#endif
/* from command line */
bool replay;
bool single_session;
-#ifdef ENABLE_OCC
bool disable_occ;
-#endif
int mode;
bool pull;
int push_peer_info_detail;
diff --git a/src/openvpn/syshead.h b/src/openvpn/syshead.h
index cafe4719..8342eae0 100644
--- a/src/openvpn/syshead.h
+++ b/src/openvpn/syshead.h
@@ -569,11 +569,6 @@ socket_defined(const socket_descriptor_t sd)
#define UNIX_SOCK_SUPPORT 0
#endif
-/*
- * Should we include OCC (options consistency check) code?
- */
-#define ENABLE_OCC
-
/*
* Should we include NTLM proxy functionality
*/
--
2.26.2
|
|
From: Gert D. <ge...@gr...> - 2020-07-18 10:00:36
Attachments:
signature.asc
|
Hi,
On Fri, Jul 17, 2020 at 03:47:36PM +0200, Arne Schwabe wrote:
> Commit 037669f3dd already made occ being unconditionally on. This commit
> only removes the #ifdefs
>
> Signed-off-by: Arne Schwabe <ar...@rf...>
This generally looks good, and does what it says on the tin, so
Acked-By: Gert Doering <ge...@gr...>
I have tried to apply it out of sequence, but that leads to compile-time
errors
ld: ssl.o: in function `key_method_1_write':
.../ssl.c:2239: undefined reference to `local_options_string'
... so the "remove key-method 1" patch must be applied first.
(This is not a show-stopper, just an explanation why I ACK this and
not merge it right away)
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 ge...@gr...
|
|
From: Gert D. <ge...@gr...> - 2020-07-21 19:50:18
|
NOW I can finally merge this, since key-method v1 is gone
and this compiles without unresolveds \o/
Stared-at-code, test compiled, ship.
Your patch has been applied to the master branch.
commit ba66faad5608233f792c3679ebade09ff324a4b3
Author: Arne Schwabe
Date: Fri Jul 17 15:47:36 2020 +0200
Remove ENABLE_OCC #define
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: Gert Doering <ge...@gr...>
Message-Id: <202...@rf...>
URL: https://www.mail-archive.com/ope...@li.../msg20442.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:47:59
|
OpenSSL 1.0.1 was supported until 2016-12-31. Rhel6/Centos6 still
use this version but considering that RHEL7 and RHEL8 are already
out, these versions can also stay with OpenVPN 2.4.
All the supported Debian based distributions also come with at
least 1.0.2.
We (accidently) unconditionally compiled some key exporter code on
OpenSSL 1.0.2+ without problems. So always compile the whole
key exporter feature for OpenSSL.
This also allows the tls groups commit to be applied without
adding ifdefs to disable that functionality on OpenSSL 1.0.1
Signed-off-by: Arne Schwabe <ar...@rf...>
---
.travis.yml | 8 -----
Changes.rst | 2 ++
INSTALL | 9 +++---
configure.ac | 14 +++------
src/openvpn/crypto.c | 7 -----
src/openvpn/openssl_compat.h | 14 ---------
src/openvpn/options.c | 2 +-
src/openvpn/ssl_mbedtls.c | 2 +-
src/openvpn/ssl_openssl.c | 60 ++----------------------------------
9 files changed, 16 insertions(+), 102 deletions(-)
diff --git a/.travis.yml b/.travis.yml
index 925d09ea..101ff096 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -35,10 +35,6 @@ jobs:
env: SSLLIB="openssl" RUN_COVERITY="1"
os: linux
compiler: gcc
- - name: gcc | openssl-1.0.1u
- env: SSLLIB="openssl" OPENSSL_VERSION="1.0.1u"
- os: linux
- compiler: gcc
- name: gcc | openssl-1.1.1d
env: SSLLIB="openssl" OPENSSL_VERSION="1.1.1d"
os: linux
@@ -87,10 +83,6 @@ jobs:
env: SSLLIB="mbedtls"
os: osx
compiler: clang
- - name: mingw64 | openssl-1.0.1u
- env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32 OPENSSL_VERSION="1.0.1u"
- os: linux
- compiler: ": Win64 build only"
- name: mingw64 | openssl-1.1.1d
env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32 OPENSSL_VERSION="1.1.1d"
os: linux
diff --git a/Changes.rst b/Changes.rst
index 18b03e47..6e283270 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -34,6 +34,8 @@ https://community.openvpn.net/openvpn/wiki/DeprecatedOptions
With the improved and matured data channel cipher negotiation, the use
of ``ncp-disable`` should not be necessary anymore.
+- Support for building with OpenSSL 1.0.1 has been removed. The minimum
+ supported OpenSSL version is now 1.0.2.
Overview of changes in 2.4
==========================
diff --git a/INSTALL b/INSTALL
index de0eb518..fde0b7cd 100644
--- a/INSTALL
+++ b/INSTALL
@@ -71,12 +71,13 @@ REQUIRES:
(1) TUN and/or TAP driver to allow user-space programs to control
a virtual point-to-point IP or Ethernet device. See
TUN/TAP Driver Configuration section below for more info.
-
-OPTIONAL (but recommended):
- (1) OpenSSL library, necessary for encryption, version 1.0.1 or higher
+ (2) OpenSSL library, necessary for encryption, version 1.0.2 or higher
required, available from http://www.openssl.org/
- (2) mbed TLS library, an alternative for encryption, version 2.0 or higher
+ or
+ (3) mbed TLS library, an alternative for encryption, version 2.0 or higher
required, available from https://tls.mbed.org/
+
+OPTIONAL:
(3) LZO real-time compression library, required for link compression,
available from http://www.oberhumer.com/opensource/lzo/
OpenBSD users can use ports or packages to install lzo, but remember
diff --git a/configure.ac b/configure.ac
index 45148892..d9ad80b1 100644
--- a/configure.ac
+++ b/configure.ac
@@ -846,7 +846,7 @@ if test "${with_crypto_library}" = "openssl"; then
# if the user did not explicitly specify flags, try to autodetect
PKG_CHECK_MODULES(
[OPENSSL],
- [openssl >= 1.0.1],
+ [openssl >= 1.0.2],
[have_openssl="yes"],
[] # If this fails, we will do another test next
)
@@ -861,7 +861,7 @@ if test "${with_crypto_library}" = "openssl"; then
# If pkgconfig check failed or OPENSSL_CFLAGS/OPENSSL_LIBS env vars
# are used, check the version directly in the OpenSSL include file
if test "${have_openssl}" != "yes"; then
- AC_MSG_CHECKING([additionally if OpenSSL is available and version >= 1.0.1])
+ AC_MSG_CHECKING([additionally if OpenSSL is available and version >= 1.0.2])
AC_COMPILE_IFELSE(
[AC_LANG_PROGRAM(
[[
@@ -869,7 +869,7 @@ if test "${with_crypto_library}" = "openssl"; then
]],
[[
/* Version encoding: MNNFFPPS - see opensslv.h for details */
-#if OPENSSL_VERSION_NUMBER < 0x10001000L
+#if OPENSSL_VERSION_NUMBER < 0x10002000L
#error OpenSSL too old
#endif
]]
@@ -912,12 +912,9 @@ if test "${with_crypto_library}" = "openssl"; then
[have_crypto_aead_modes="no"]
)
+ # All supported OpenSSL version (>= 1.0.2)
+ # have this feature
have_export_keying_material="yes"
- AC_CHECK_FUNC(
- [SSL_export_keying_material],
- ,
- [have_export_keying_material="no"]
- )
AC_CHECK_FUNCS(
[ \
@@ -938,7 +935,6 @@ if test "${with_crypto_library}" = "openssl"; then
X509_STORE_get0_objects \
X509_OBJECT_free \
X509_OBJECT_get_type \
- EVP_PKEY_id \
EVP_PKEY_get0_RSA \
EVP_PKEY_get0_DSA \
EVP_PKEY_get0_EC_KEY \
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index 1ce98184..bbf47ef7 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -428,13 +428,6 @@ openvpn_decrypt_aead(struct buffer *buf, struct buffer work,
tag_ptr = BPTR(buf);
ASSERT(buf_advance(buf, tag_size));
dmsg(D_PACKET_CONTENT, "DECRYPT MAC: %s", format_hex(tag_ptr, tag_size, 0, &gc));
-#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER < 0x10001040L
- /* OpenSSL <= 1.0.1c bug requires set tag before processing ciphertext */
- if (!EVP_CIPHER_CTX_ctrl(ctx->cipher, EVP_CTRL_GCM_SET_TAG, tag_size, tag_ptr))
- {
- CRYPT_ERROR("setting tag failed");
- }
-#endif
if (buf->len < 1)
{
diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h
index 4ac8f24d..d35251fb 100644
--- a/src/openvpn/openssl_compat.h
+++ b/src/openvpn/openssl_compat.h
@@ -271,20 +271,6 @@ EVP_PKEY_get0_EC_KEY(EVP_PKEY *pkey)
}
#endif
-#if !defined(HAVE_EVP_PKEY_ID)
-/**
- * Get the PKEY type
- *
- * @param pkey Public key object
- * @return The key type
- */
-static inline int
-EVP_PKEY_id(const EVP_PKEY *pkey)
-{
- return pkey ? pkey->type : EVP_PKEY_NONE;
-}
-#endif
-
#if !defined(HAVE_EVP_PKEY_GET0_DSA)
/**
* Get the DSA object of a public key
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index b6b8d769..a20b27c9 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -8671,7 +8671,7 @@ add_option(struct options *options,
options->keying_material_exporter_label = p[1];
options->keying_material_exporter_length = ekm_length;
}
-#endif /* if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10001000 */
+#endif /* HAVE_EXPORT_KEYING_MATERIAL */
else if (streq(p[0], "allow-recursive-routing") && !p[1])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c
index f518f593..977ff5c3 100644
--- a/src/openvpn/ssl_mbedtls.c
+++ b/src/openvpn/ssl_mbedtls.c
@@ -1108,7 +1108,7 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl,
}
}
-#if HAVE_EXPORT_KEYING_MATERIAL
+#ifdef HAVE_EXPORT_KEYING_MATERIAL
/* Initialize keying material exporter */
if (session->opt->ekm_size)
{
diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c
index 07d422c9..14d52bfa 100644
--- a/src/openvpn/ssl_openssl.c
+++ b/src/openvpn/ssl_openssl.c
@@ -164,7 +164,6 @@ key_state_export_keying_material(struct key_state_ssl *ssl,
{
if (session->opt->ekm_size > 0)
{
-#if (OPENSSL_VERSION_NUMBER >= 0x10001000)
unsigned int size = session->opt->ekm_size;
struct gc_arena gc = gc_new();
unsigned char *ekm = (unsigned char *) gc_malloc(size, true, &gc);
@@ -188,7 +187,6 @@ key_state_export_keying_material(struct key_state_ssl *ssl,
setenv_del(session->opt->es, "exported_keying_material");
}
gc_free(&gc);
-#endif /* if (OPENSSL_VERSION_NUMBER >= 0x10001000) */
}
}
@@ -559,7 +557,7 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile)
#else /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
if (profile)
{
- msg(M_WARN, "WARNING: OpenSSL 1.0.1 does not support --tls-cert-profile"
+ msg(M_WARN, "WARNING: OpenSSL 1.0.2 does not support --tls-cert-profile"
", ignoring user-set profile: '%s'", profile);
}
#endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */
@@ -573,19 +571,11 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
ASSERT(ctx);
-#if (OPENSSL_VERSION_NUMBER >= 0x10002000L && !defined(LIBRESSL_VERSION_NUMBER)) \
- || LIBRESSL_VERSION_NUMBER >= 0x2070000fL
- /* OpenSSL 1.0.2 and up */
cert = SSL_CTX_get0_certificate(ctx->ctx);
-#else
- /* OpenSSL 1.0.1 and earlier need an SSL object to get at the certificate */
- SSL *ssl = SSL_new(ctx->ctx);
- cert = SSL_get_certificate(ssl);
-#endif
if (cert == NULL)
{
- goto cleanup; /* Nothing to check if there is no certificate */
+ return; /* Nothing to check if there is no certificate */
}
ret = X509_cmp_time(X509_get0_notBefore(cert), NULL);
@@ -607,13 +597,6 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx)
{
msg(M_WARN, "WARNING: Your certificate has expired!");
}
-
-cleanup:
-#if OPENSSL_VERSION_NUMBER < 0x10002000L \
- || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x2070000fL)
- SSL_free(ssl);
-#endif
- return;
}
void
@@ -680,7 +663,6 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
}
else
{
-#if OPENSSL_VERSION_NUMBER >= 0x10002000L
#if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER))
/* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter
@@ -691,29 +673,6 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name
* so do nothing */
#endif
return;
-#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;
- EVP_PKEY *pkey = NULL;
-
- /* Little hack to get private key ref from SSL_CTX, yay OpenSSL... */
- SSL *ssl = SSL_new(ctx->ctx);
- if (!ssl)
- {
- crypto_msg(M_FATAL, "SSL_new failed");
- }
- pkey = SSL_get_privatekey(ssl);
- SSL_free(ssl);
-
- msg(D_TLS_DEBUG, "Extracting ECDH curve from private key");
-
- if (pkey != NULL && (eckey = EVP_PKEY_get1_EC_KEY(pkey)) != NULL
- && (ecgrp = EC_KEY_get0_group(eckey)) != NULL)
- {
- nid = EC_GROUP_get_curve_name(ecgrp);
- }
-#endif /* if OPENSSL_VERSION_NUMBER >= 0x10002000L */
}
/* Translate NID back to name , just for kicks */
@@ -1462,15 +1421,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
ASSERT(NULL != ctx);
-#if (OPENSSL_VERSION_NUMBER >= 0x10002000L && !defined(LIBRESSL_VERSION_NUMBER)) \
- || LIBRESSL_VERSION_NUMBER >= 0x2070000fL
- /* OpenSSL 1.0.2 and up */
X509 *cert = SSL_CTX_get0_certificate(ctx->ctx);
-#else
- /* OpenSSL 1.0.1 and earlier need an SSL object to get at the certificate */
- SSL *ssl = SSL_new(ctx->ctx);
- X509 *cert = SSL_get_certificate(ssl);
-#endif
ASSERT(NULL != cert);
@@ -1510,13 +1461,6 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx)
ret = 0;
cleanup:
-#if OPENSSL_VERSION_NUMBER < 0x10002000L \
- || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x2070000fL)
- if (ssl)
- {
- SSL_free(ssl);
- }
-#endif
if (ret)
{
crypto_msg(M_FATAL, "Cannot enable SSL external private key capability");
--
2.26.2
|
|
From: Steffan K. <ste...@fo...> - 2020-07-20 11:41:55
|
Hi, On 17-07-2020 15:47, Arne Schwabe wrote: > OpenSSL 1.0.1 was supported until 2016-12-31. Rhel6/Centos6 still > use this version but considering that RHEL7 and RHEL8 are already > out, these versions can also stay with OpenVPN 2.4. > > All the supported Debian based distributions also come with at > least 1.0.2. > > We (accidently) unconditionally compiled some key exporter code on > OpenSSL 1.0.2+ without problems. So always compile the whole > key exporter feature for OpenSSL. > > This also allows the tls groups commit to be applied without > adding ifdefs to disable that functionality on OpenSSL 1.0.1 > > Signed-off-by: Arne Schwabe <ar...@rf...> > --- > .travis.yml | 8 ----- > Changes.rst | 2 ++ > INSTALL | 9 +++--- > configure.ac | 14 +++------ > src/openvpn/crypto.c | 7 ----- > src/openvpn/openssl_compat.h | 14 --------- > src/openvpn/options.c | 2 +- > src/openvpn/ssl_mbedtls.c | 2 +- > src/openvpn/ssl_openssl.c | 60 ++---------------------------------- > 9 files changed, 16 insertions(+), 102 deletions(-) > > diff --git a/.travis.yml b/.travis.yml > index 925d09ea..101ff096 100644 > --- a/.travis.yml > +++ b/.travis.yml > @@ -35,10 +35,6 @@ jobs: > env: SSLLIB="openssl" RUN_COVERITY="1" > os: linux > compiler: gcc > - - name: gcc | openssl-1.0.1u > - env: SSLLIB="openssl" OPENSSL_VERSION="1.0.1u" > - os: linux > - compiler: gcc > - name: gcc | openssl-1.1.1d > env: SSLLIB="openssl" OPENSSL_VERSION="1.1.1d" > os: linux > @@ -87,10 +83,6 @@ jobs: > env: SSLLIB="mbedtls" > os: osx > compiler: clang > - - name: mingw64 | openssl-1.0.1u > - env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32 OPENSSL_VERSION="1.0.1u" > - os: linux > - compiler: ": Win64 build only" > - name: mingw64 | openssl-1.1.1d > env: SSLLIB="openssl" CHOST=x86_64-w64-mingw32 OPENSSL_VERSION="1.1.1d" > os: linux > diff --git a/Changes.rst b/Changes.rst > index 18b03e47..6e283270 100644 > --- a/Changes.rst > +++ b/Changes.rst > @@ -34,6 +34,8 @@ https://community.openvpn.net/openvpn/wiki/DeprecatedOptions > With the improved and matured data channel cipher negotiation, the use > of ``ncp-disable`` should not be necessary anymore. > > +- Support for building with OpenSSL 1.0.1 has been removed. The minimum > + supported OpenSSL version is now 1.0.2. > > Overview of changes in 2.4 > ========================== > diff --git a/INSTALL b/INSTALL > index de0eb518..fde0b7cd 100644 > --- a/INSTALL > +++ b/INSTALL > @@ -71,12 +71,13 @@ REQUIRES: > (1) TUN and/or TAP driver to allow user-space programs to control > a virtual point-to-point IP or Ethernet device. See > TUN/TAP Driver Configuration section below for more info. > - > -OPTIONAL (but recommended): > - (1) OpenSSL library, necessary for encryption, version 1.0.1 or higher > + (2) OpenSSL library, necessary for encryption, version 1.0.2 or higher > required, available from http://www.openssl.org/ > - (2) mbed TLS library, an alternative for encryption, version 2.0 or higher > + or > + (3) mbed TLS library, an alternative for encryption, version 2.0 or higher > required, available from https://tls.mbed.org/ > + > +OPTIONAL: > (3) LZO real-time compression library, required for link compression, > available from http://www.oberhumer.com/opensource/lzo/ > OpenBSD users can use ports or packages to install lzo, but remember > diff --git a/configure.ac b/configure.ac > index 45148892..d9ad80b1 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -846,7 +846,7 @@ if test "${with_crypto_library}" = "openssl"; then > # if the user did not explicitly specify flags, try to autodetect > PKG_CHECK_MODULES( > [OPENSSL], > - [openssl >= 1.0.1], > + [openssl >= 1.0.2], > [have_openssl="yes"], > [] # If this fails, we will do another test next > ) > @@ -861,7 +861,7 @@ if test "${with_crypto_library}" = "openssl"; then > # If pkgconfig check failed or OPENSSL_CFLAGS/OPENSSL_LIBS env vars > # are used, check the version directly in the OpenSSL include file > if test "${have_openssl}" != "yes"; then > - AC_MSG_CHECKING([additionally if OpenSSL is available and version >= 1.0.1]) > + AC_MSG_CHECKING([additionally if OpenSSL is available and version >= 1.0.2]) > AC_COMPILE_IFELSE( > [AC_LANG_PROGRAM( > [[ > @@ -869,7 +869,7 @@ if test "${with_crypto_library}" = "openssl"; then > ]], > [[ > /* Version encoding: MNNFFPPS - see opensslv.h for details */ > -#if OPENSSL_VERSION_NUMBER < 0x10001000L > +#if OPENSSL_VERSION_NUMBER < 0x10002000L > #error OpenSSL too old > #endif > ]] > @@ -912,12 +912,9 @@ if test "${with_crypto_library}" = "openssl"; then > [have_crypto_aead_modes="no"] > ) > > + # All supported OpenSSL version (>= 1.0.2) > + # have this feature This comment is space-indented, while the surrounding code is tab-indented. > have_export_keying_material="yes" > - AC_CHECK_FUNC( > - [SSL_export_keying_material], > - , > - [have_export_keying_material="no"] > - ) > > AC_CHECK_FUNCS( > [ \ > @@ -938,7 +935,6 @@ if test "${with_crypto_library}" = "openssl"; then > X509_STORE_get0_objects \ > X509_OBJECT_free \ > X509_OBJECT_get_type \ > - EVP_PKEY_id \ > EVP_PKEY_get0_RSA \ > EVP_PKEY_get0_DSA \ > EVP_PKEY_get0_EC_KEY \ > diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c > index 1ce98184..bbf47ef7 100644 > --- a/src/openvpn/crypto.c > +++ b/src/openvpn/crypto.c > @@ -428,13 +428,6 @@ openvpn_decrypt_aead(struct buffer *buf, struct buffer work, > tag_ptr = BPTR(buf); > ASSERT(buf_advance(buf, tag_size)); > dmsg(D_PACKET_CONTENT, "DECRYPT MAC: %s", format_hex(tag_ptr, tag_size, 0, &gc)); > -#if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER < 0x10001040L > - /* OpenSSL <= 1.0.1c bug requires set tag before processing ciphertext */ > - if (!EVP_CIPHER_CTX_ctrl(ctx->cipher, EVP_CTRL_GCM_SET_TAG, tag_size, tag_ptr)) > - { > - CRYPT_ERROR("setting tag failed"); > - } > -#endif > > if (buf->len < 1) > { > diff --git a/src/openvpn/openssl_compat.h b/src/openvpn/openssl_compat.h > index 4ac8f24d..d35251fb 100644 > --- a/src/openvpn/openssl_compat.h > +++ b/src/openvpn/openssl_compat.h > @@ -271,20 +271,6 @@ EVP_PKEY_get0_EC_KEY(EVP_PKEY *pkey) > } > #endif > > -#if !defined(HAVE_EVP_PKEY_ID) > -/** > - * Get the PKEY type > - * > - * @param pkey Public key object > - * @return The key type > - */ > -static inline int > -EVP_PKEY_id(const EVP_PKEY *pkey) > -{ > - return pkey ? pkey->type : EVP_PKEY_NONE; > -} > -#endif > - > #if !defined(HAVE_EVP_PKEY_GET0_DSA) > /** > * Get the DSA object of a public key > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index b6b8d769..a20b27c9 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -8671,7 +8671,7 @@ add_option(struct options *options, > options->keying_material_exporter_label = p[1]; > options->keying_material_exporter_length = ekm_length; > } > -#endif /* if defined(ENABLE_CRYPTO_OPENSSL) && OPENSSL_VERSION_NUMBER >= 0x10001000 */ > +#endif /* HAVE_EXPORT_KEYING_MATERIAL */ > else if (streq(p[0], "allow-recursive-routing") && !p[1]) > { > VERIFY_PERMISSION(OPT_P_GENERAL); > diff --git a/src/openvpn/ssl_mbedtls.c b/src/openvpn/ssl_mbedtls.c > index f518f593..977ff5c3 100644 > --- a/src/openvpn/ssl_mbedtls.c > +++ b/src/openvpn/ssl_mbedtls.c > @@ -1108,7 +1108,7 @@ key_state_ssl_init(struct key_state_ssl *ks_ssl, > } > } > > -#if HAVE_EXPORT_KEYING_MATERIAL > +#ifdef HAVE_EXPORT_KEYING_MATERIAL > /* Initialize keying material exporter */ > if (session->opt->ekm_size) > { > diff --git a/src/openvpn/ssl_openssl.c b/src/openvpn/ssl_openssl.c > index 07d422c9..14d52bfa 100644 > --- a/src/openvpn/ssl_openssl.c > +++ b/src/openvpn/ssl_openssl.c > @@ -164,7 +164,6 @@ key_state_export_keying_material(struct key_state_ssl *ssl, > { > if (session->opt->ekm_size > 0) > { > -#if (OPENSSL_VERSION_NUMBER >= 0x10001000) > unsigned int size = session->opt->ekm_size; > struct gc_arena gc = gc_new(); > unsigned char *ekm = (unsigned char *) gc_malloc(size, true, &gc); > @@ -188,7 +187,6 @@ key_state_export_keying_material(struct key_state_ssl *ssl, > setenv_del(session->opt->es, "exported_keying_material"); > } > gc_free(&gc); > -#endif /* if (OPENSSL_VERSION_NUMBER >= 0x10001000) */ > } > } > > @@ -559,7 +557,7 @@ tls_ctx_set_cert_profile(struct tls_root_ctx *ctx, const char *profile) > #else /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */ > if (profile) > { > - msg(M_WARN, "WARNING: OpenSSL 1.0.1 does not support --tls-cert-profile" > + msg(M_WARN, "WARNING: OpenSSL 1.0.2 does not support --tls-cert-profile" > ", ignoring user-set profile: '%s'", profile); > } > #endif /* ifdef HAVE_SSL_CTX_SET_SECURITY_LEVEL */ > @@ -573,19 +571,11 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) > > ASSERT(ctx); > > -#if (OPENSSL_VERSION_NUMBER >= 0x10002000L && !defined(LIBRESSL_VERSION_NUMBER)) \ > - || LIBRESSL_VERSION_NUMBER >= 0x2070000fL > - /* OpenSSL 1.0.2 and up */ > cert = SSL_CTX_get0_certificate(ctx->ctx); > -#else > - /* OpenSSL 1.0.1 and earlier need an SSL object to get at the certificate */ > - SSL *ssl = SSL_new(ctx->ctx); > - cert = SSL_get_certificate(ssl); > -#endif > > if (cert == NULL) > { > - goto cleanup; /* Nothing to check if there is no certificate */ > + return; /* Nothing to check if there is no certificate */ > } > > ret = X509_cmp_time(X509_get0_notBefore(cert), NULL); > @@ -607,13 +597,6 @@ tls_ctx_check_cert_time(const struct tls_root_ctx *ctx) > { > msg(M_WARN, "WARNING: Your certificate has expired!"); > } > - > -cleanup: > -#if OPENSSL_VERSION_NUMBER < 0x10002000L \ > - || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x2070000fL) > - SSL_free(ssl); > -#endif > - return; > } > > void > @@ -680,7 +663,6 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name > } > else > { > -#if OPENSSL_VERSION_NUMBER >= 0x10002000L > #if (OPENSSL_VERSION_NUMBER < 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)) > > /* OpenSSL 1.0.2 and newer can automatically handle ECDH parameter > @@ -691,29 +673,6 @@ tls_ctx_load_ecdh_params(struct tls_root_ctx *ctx, const char *curve_name > * so do nothing */ > #endif > return; > -#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; > - EVP_PKEY *pkey = NULL; > - > - /* Little hack to get private key ref from SSL_CTX, yay OpenSSL... */ > - SSL *ssl = SSL_new(ctx->ctx); > - if (!ssl) > - { > - crypto_msg(M_FATAL, "SSL_new failed"); > - } > - pkey = SSL_get_privatekey(ssl); > - SSL_free(ssl); > - > - msg(D_TLS_DEBUG, "Extracting ECDH curve from private key"); > - > - if (pkey != NULL && (eckey = EVP_PKEY_get1_EC_KEY(pkey)) != NULL > - && (ecgrp = EC_KEY_get0_group(eckey)) != NULL) > - { > - nid = EC_GROUP_get_curve_name(ecgrp); > - } > -#endif /* if OPENSSL_VERSION_NUMBER >= 0x10002000L */ > } > > /* Translate NID back to name , just for kicks */ > @@ -1462,15 +1421,7 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx) > > ASSERT(NULL != ctx); > > -#if (OPENSSL_VERSION_NUMBER >= 0x10002000L && !defined(LIBRESSL_VERSION_NUMBER)) \ > - || LIBRESSL_VERSION_NUMBER >= 0x2070000fL > - /* OpenSSL 1.0.2 and up */ > X509 *cert = SSL_CTX_get0_certificate(ctx->ctx); > -#else > - /* OpenSSL 1.0.1 and earlier need an SSL object to get at the certificate */ > - SSL *ssl = SSL_new(ctx->ctx); > - X509 *cert = SSL_get_certificate(ssl); > -#endif > > ASSERT(NULL != cert); > > @@ -1510,13 +1461,6 @@ tls_ctx_use_management_external_key(struct tls_root_ctx *ctx) > > ret = 0; > cleanup: > -#if OPENSSL_VERSION_NUMBER < 0x10002000L \ > - || (defined(LIBRESSL_VERSION_NUMBER) && LIBRESSL_VERSION_NUMBER < 0x2070000fL) > - if (ssl) > - { > - SSL_free(ssl); > - } > -#endif > if (ret) > { > crypto_msg(M_FATAL, "Cannot enable SSL external private key capability"); > Otherwise this now looks good to me. So if the whitespace can fixed when committing: Acked-by: Steffan Karger <ste...@fo...> -Steffan |
|
From: Gert D. <ge...@gr...> - 2020-07-20 19:48:24
|
Your patch has been applied to the master branch.
Whitespace fixed on-the-go.
Sanity-tested on Linux / 1.1.1g and FreeBSD / 1.0.2s (client only).
commit ec7d0e8e0f8cd8f1c5fab58c795a59828eba6ae7
Author: Arne Schwabe
Date: Fri Jul 17 15:47:32 2020 +0200
Drop support for OpenSSL 1.0.1
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: Steffan Karger <ste...@fo...>
Message-Id: <202...@rf...>
URL: https://www.mail-archive.com/ope...@li.../msg20441.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
|
From: Arne S. <ar...@rf...> - 2020-07-17 13:48:01
|
This reworks the NCP logic to be more strict about what is
considered an acceptable result of an NCP negotiation. It also
us to finally drop BF-CBC support by default.
All new behaviour is currently limited to server/client
mode with pull enabled. P2p mode without pull does not change.
New Server behaviour:
- when a client announces its supported ciphers through either
OCC or IV_CIPHER/IV_NCP we reject the client with a
AUTH_FAILED message if we have no common cipher.
- When a client does not announce any cipher in either
OCC or NCP we by reject it unless fallback-cipher is
specified in either ccd or config.
New client behaviour:
- When no cipher is pushed (or a cipher we refused to support)
and we also cannot support the server's server announced in
OCC we fail the connection and log why
- If fallback-cipher is specified we will in the case that
cipher is missing from occ use the fallback cipher instead
of failing the connection
Both client and server behaviour:
- We only announce --cipher xyz in occ if we are willing
to support that cipher.
It means that we only announce the fallback-cipher if
it is also contained in --data-ciphers
Compatiblity behaviour:
In 2.5 both client and server will automatically set
fallback-cipher xyz if --cipher xyz is in the config and
also add append the cipher to the end of data-ciphers.
We log a warn user about this and point to --data-ciphers and
--fallback-cipher. This also happens if the configuration
contains an explicit --cipher BF-CBC.
If --cipher is not set, we only warn that previous versions
allowed BF-CBC and point how to reenable BF-CBC. This might
break very few config where someone connects a very old
client to a 2.5 server but at some point we need to drop
the BF-CBC and those affected use will already have a the
scary SWEET32 warning in their logs.
In short: If --cipher is explicitly set 2.6 will work the same as
2.4 did. When --cipher is not set, BF-CBC support is dropped and
we warn about it.
Examples how breaking the default BF-CBC will be logged:
Client side:
- Client connecting to server that does not push cipher but
has --cipher in OCC
OPTIONS ERROR: failed to negotiate cipher with server. Add the server's cipher ('BF-CBC') to --data-ciphers (currently 'AES-256-GCM:AES-128-CBC') if you want to connect to this server.
- Client connecting to a server that does not support OCC:
OPTIONS ERROR: failed to negotioate cipher with server. Configure --fallback-cipher if you want connect to this server.
Server Side:
- Server has a client only supporting BF-CBC connecting:
styx/IP PUSH: No common cipher between server and client. Server data-ciphers: 'CHACHA20-POLY1305:AES-128-GCM:AES-256-GCM:AES-256-CBC:AES-128-CBC', client supports cipher 'BF-CBC'.
- Client without OCC:
styx/IP PUSH:No NCP or OCC cipher data received from peer.
styx/IP Use --fallback-cipher with the cipher the client is using if you want to allow the client to connect
In all reject cases on the client:
AUTH: Received control message: AUTH_FAILED,Data channel cipher negotiation failed (no shared cipher)
Signed-off-by: Arne Schwabe <ar...@rf...>
---
doc/man-sections/protocol-options.rst | 16 ++-
src/openvpn/crypto.c | 3 +-
src/openvpn/init.c | 25 ++++-
src/openvpn/multi.c | 136 ++++++++++++++++----------
src/openvpn/options.c | 109 +++++++++++++++++----
src/openvpn/options.h | 2 +
src/openvpn/ssl.c | 11 ++-
src/openvpn/ssl_ncp.c | 20 ++--
src/openvpn/ssl_ncp.h | 10 +-
tests/unit_tests/openvpn/test_ncp.c | 26 +++--
10 files changed, 253 insertions(+), 105 deletions(-)
diff --git a/doc/man-sections/protocol-options.rst b/doc/man-sections/protocol-options.rst
index 051f1d32..1b53400b 100644
--- a/doc/man-sections/protocol-options.rst
+++ b/doc/man-sections/protocol-options.rst
@@ -57,6 +57,9 @@ configured in a compatible way between both the local and remote side.
http://www.cs.ucsd.edu/users/mihir/papers/hmac.html
--cipher alg
+ This option is deprecated for server-client mode and ``--data-ciphers``
+ or rarely `--fallback-cipher`` should be used instead.
+
Encrypt data channel packets with cipher algorithm ``alg``.
The default is :code:`BF-CBC`, an abbreviation for Blowfish in Cipher
@@ -183,8 +186,9 @@ configured in a compatible way between both the local and remote side.
``--server`` ), or if ``--pull`` is specified (client-side, implied by
setting --client).
- If both peers support and do not disable NCP, the negotiated cipher will
- override the cipher specified by ``--cipher``.
+ If no common cipher is found is found during cipher negotiation, the
+ connection is terminated. To support old clients/server that do not
+ provide any cipher support see ``fallback-cipher``.
Additionally, to allow for more smooth transition, if NCP is enabled,
OpenVPN will inherit the cipher of the peer if that cipher is different
@@ -204,6 +208,14 @@ configured in a compatible way between both the local and remote side.
This option was called ``ncp-ciphers`` in OpenVPN 2.4 but has been renamed
to ``data-ciphers`` in OpenVPN 2.5 to more accurately reflect its meaning.
+--fallback-cipher alg
+
+ Configure a cipher that is used to fall back to if the peer does announce
+ the cipher in OCC.
+
+ This option should normally not be needed. It only exists to allow
+ connecting to old servers or if supporting old clients as server.
+
--ncp-disable
Disable "Negotiable Crypto Parameters". This completely disables cipher
negotiation.
diff --git a/src/openvpn/crypto.c b/src/openvpn/crypto.c
index e92a0dc1..accab850 100644
--- a/src/openvpn/crypto.c
+++ b/src/openvpn/crypto.c
@@ -727,7 +727,8 @@ warn_insecure_key_type(const char *ciphername, const cipher_kt_t *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).",
+ "using a --cipher with a larger block size (e.g. AES-256-CBC). "
+ "Support for these insecure cipher will be removed in OpenVPN 2.6.",
ciphername, cipher_kt_block_size(cipher)*8);
}
}
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 1ea4735d..7cae817c 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2374,7 +2374,30 @@ do_deferred_options(struct context *c, const unsigned int found)
{
/* If the server did not push a --cipher, we will switch to the
* remote cipher if it is in our ncp-ciphers list */
- tls_poor_mans_ncp(&c->options, c->c2.tls_multi->remote_ciphername);
+ bool useremotecipher = tls_poor_mans_ncp(&c->options,
+ c->c2.tls_multi->remote_ciphername);
+
+ if (!useremotecipher && !c->options.enable_ncp_fallback)
+ {
+ /* Give appropiate error message */
+ if (c->c2.tls_multi->remote_ciphername)
+ {
+ msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotiate "
+ "cipher with server. Add the server's "
+ "cipher ('%s') to --data-ciphers (currently '%s') if "
+ "you want to connect to this server.",
+ c->c2.tls_multi->remote_ciphername,
+ c->options.ncp_ciphers);
+ }
+ else
+ {
+ msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to negotioate "
+ "cipher with server. Configure "
+ "--fallback-cipher if you want connect "
+ "to this server.");
+ }
+ return false;
+ }
}
struct frame *frame_fragment = NULL;
#ifdef ENABLE_FRAGMENT
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index d2549bca..58a07e34 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1780,7 +1780,7 @@ multi_client_connect_setenv(struct multi_context *m,
* - choosen cipher
* - peer id
*/
-static void
+static bool
multi_client_set_protocol_options(struct context *c)
{
@@ -1810,56 +1810,86 @@ multi_client_set_protocol_options(struct context *c)
}
/* Select cipher if client supports Negotiable Crypto Parameters */
- if (o->ncp_enabled)
+ if (!o->ncp_enabled)
{
- /* if we have already created our key, we cannot *change* our own
- * cipher -> so log the fact and push the "what we have now" cipher
- * (so the client is always told what we expect it to use)
- */
- const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
- if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+ return true;
+ }
+
+ /* if we have already created our key, we cannot *change* our own
+ * cipher -> so log the fact and push the "what we have now" cipher
+ * (so the client is always told what we expect it to use)
+ */
+ const struct tls_session *session = &tls_multi->session[TM_ACTIVE];
+ if (session->key[KS_PRIMARY].crypto_options.key_ctx_bi.initialized)
+ {
+ msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
+ "server has already generated data channel keys, "
+ "re-sending previously negotiated cipher '%s'",
+ o->ciphername );
+ return true;
+ }
+
+ /*
+ * Push the first cipher from --data-ciphers to the client that
+ * the client announces to be supporting.
+ */
+ char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, peer_info,
+ tls_multi->remote_ciphername,
+ &o->gc);
+
+ if (push_cipher)
+ {
+ o->ciphername = push_cipher;
+ return true;
+ }
+
+ /* NCP cipher negotiation failed. Try to figure out why exactly it
+ * failed and give good error messages and potentially do a fallback
+ * for non NCP clients */
+ struct gc_arena gc = gc_new();
+ bool ret = false;
+
+ const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
+ /* If we are in a situation where we know the client ciphers, there is no
+ * reason to fall back to a cipher that will not be accepted by the other
+ * side, in this situation we fail the auth*/
+ if (strlen(peer_ciphers) > 0)
+ {
+ msg(M_INFO, "PUSH: No common cipher between server and client. "
+ "Server data-ciphers: '%s', client supported ciphers '%s'",
+ o->ncp_ciphers, peer_ciphers);
+ }
+ else if (tls_multi->remote_ciphername)
+ {
+ msg(M_INFO, "PUSH: No common cipher between server and client. "
+ "Server data-ciphers: '%s', client supports cipher '%s'",
+ o->ncp_ciphers, tls_multi->remote_ciphername);
+ }
+ else
+ {
+ msg(M_INFO, "PUSH: No NCP or OCC cipher data received from peer.");
+
+ if (o->enable_ncp_fallback && !tls_multi->remote_ciphername)
{
- msg( M_INFO, "PUSH: client wants to negotiate cipher (NCP), but "
- "server has already generated data channel keys, "
- "re-sending previously negotiated cipher '%s'",
- o->ciphername );
+ msg(M_INFO, "Using data channel cipher '%s' since "
+ "--fallback-cipher is set.", o->ciphername);
+ ret = true;
}
else
{
- /*
- * Push the first cipher from --data-ciphers to the client that
- * the client announces to be supporting.
- */
- char *push_cipher = ncp_get_best_cipher(o->ncp_ciphers, o->ciphername,
- peer_info,
- tls_multi->remote_ciphername,
- &o->gc);
-
- if (push_cipher)
- {
- o->ciphername = push_cipher;
- }
- else
- {
- struct gc_arena gc = gc_new();
- const char *peer_ciphers = tls_peer_ncp_list(peer_info, &gc);
- if (strlen(peer_ciphers) > 0)
- {
- msg(M_INFO, "PUSH: No common cipher between server and "
- "client. Expect this connection not to work. Server "
- "data-ciphers: '%s', client supported ciphers '%s'",
- o->ncp_ciphers, peer_ciphers);
- }
- else
- {
- msg(M_INFO, "No NCP data received from peer, falling back "
- "to --cipher '%s'. Peer reports in OCC --cipher '%s'",
- o->ciphername, np(tls_multi->remote_ciphername));
- }
- gc_free(&gc);
- }
+ msg(M_INFO, "Use --fallback-cipher with the cipher the client is "
+ "using if you want to allow the client "
+ "to connect");
}
}
+ if (!ret)
+ {
+ auth_set_client_reason(tls_multi, "Data channel cipher negotiation failed "
+ "(no shared cipher)");
+ }
+
+ gc_free(&gc);
+ return ret;
}
static enum client_connect_return
@@ -2068,7 +2098,7 @@ multi_client_connect_late_setup(struct multi_context *m,
if (!mi->context.c2.push_ifconfig_defined)
{
msg(D_MULTI_ERRORS, "MULTI: no dynamic or static remote"
- "--ifconfig address is available for %s",
+ "--ifconfig address is available for %s",
multi_instance_string(mi, false, &gc));
}
@@ -2084,7 +2114,7 @@ multi_client_connect_late_setup(struct multi_context *m,
/* JYFIXME -- this should cause the connection to fail */
msg(D_MULTI_ERRORS, "MULTI ERROR: primary virtual IP for %s (%s)"
- "violates tunnel network/netmask constraint (%s/%s)",
+ "violates tunnel network/netmask constraint (%s/%s)",
multi_instance_string(mi, false, &gc),
print_in_addr_t(mi->context.c2.push_ifconfig_local, 0, &gc),
ifconfig_constraint_network, ifconfig_constraint_netmask);
@@ -2133,7 +2163,7 @@ multi_client_connect_late_setup(struct multi_context *m,
else if (mi->context.options.iroutes)
{
msg(D_MULTI_ERRORS, "MULTI: --iroute options rejected for %s -- iroute"
- "only works with tun-style tunnels",
+ "only works with tun-style tunnels",
multi_instance_string(mi, false, &gc));
}
@@ -2145,11 +2175,15 @@ multi_client_connect_late_setup(struct multi_context *m,
mi->context.c2.context_auth = CAS_SUCCEEDED;
/* authentication complete, calculate dynamic client specific options */
- multi_client_set_protocol_options(&mi->context);
-
- /* Generate data channel keys */
- if (!multi_client_generate_tls_keys(&mi->context))
+ if (!multi_client_set_protocol_options(&mi->context))
+ {
+ mi->context.c2.context_auth = CAS_FAILED;
+ }
+ /* Generate data channel keys only if setting protocol options
+ * has not failed */
+ else if (!multi_client_generate_tls_keys(&mi->context))
{
+
mi->context.c2.context_auth = CAS_FAILED;
}
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 896abcde..0c129e42 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -860,7 +860,6 @@ init_options(struct options *o, const bool init_gc)
#if P2MP
o->scheduled_exit_interval = 5;
#endif
- o->ciphername = "BF-CBC";
o->ncp_enabled = true;
o->ncp_ciphers = "AES-256-GCM:AES-128-GCM";
o->authname = "SHA1";
@@ -3042,6 +3041,69 @@ options_postprocess_verify(const struct options *o)
}
}
+static void
+options_postprocess_cipher(struct options *o)
+{
+ if (!o->pull && !(o->mode == MODE_SERVER))
+ {
+ /* we are in the classic P2P mode */
+ /* cipher negotiation (NCP) currently assumes --pull or --mode server */
+ o->ncp_enabled = false;
+ msg( M_WARN, "Dynamic cipher negioation is disabled since neither "
+ "P2MP client nor server mode is enabled" );
+
+ /* If the cipher is not set default to BF-CBC, we will warn that this
+ * is deprecated on cipher initialisation, no need to warn here as
+ * well */
+ if (!o->ciphername)
+ {
+ o->ciphername = "BF-CBC";
+ }
+ return;
+ }
+
+ /* M2P mode */
+ if (!o->ciphername)
+ {
+ msg(M_WARN, "--cipher is not set. Previous OpenVPN version defaulted to "
+ "BF-CBC as fallback when dynamic cipher negotiation failed "
+ " in this case. If you need this fallback please "
+ "add '--fallback-cipher BF-CBC' to your configuration "
+ "and/or add BF-CBC to --data-ciphers");
+
+ /* We still need to set the ciphername to BF-CBC since various other
+ * parts of OpenVPN assert that the ciphername is set */
+ o->ciphername = "BF-CBC";
+
+ if (!o->ncp_enabled)
+ {
+ msg(M_USAGE, "--ncp-disable needs an explicit --cipher or "
+ "--fallback-cipher config option");
+ }
+ }
+ else if (!o->enable_ncp_fallback
+ && !tls_item_in_cipher_list(o->ciphername, o->ncp_ciphers))
+ {
+ msg(M_WARN, "DEPRECATED OPTION: --cipher set to '%s' but missing in"
+ " --ncp-ciphers (%s). Future OpenVPN version will "
+ "ignore --cipher for dynamic cipher negotiations. "
+ "Add '%s' to --data-ciphers or change --cipher '%s' to "
+ "fallback-cipher '%s' to silence this warning.",
+ o->ciphername, o->ncp_ciphers, o->ciphername,
+ o->ciphername, o->ciphername);
+ o->enable_ncp_fallback = true;
+
+ /* Append the --cipher to ncp_ciphers to allow it in NCP */
+ char *ncp_ciphers = gc_malloc(strlen(o->ncp_ciphers)
+ +strlen(o->ciphername) + 1, false, &o->gc);
+
+ strcpy(ncp_ciphers, o->ncp_ciphers);
+ strcat(ncp_ciphers, ":");
+ strcat(ncp_ciphers, o->ciphername);
+ o->ncp_ciphers = ncp_ciphers;
+ }
+}
+
static void
options_postprocess_mutate(struct options *o)
{
@@ -3054,6 +3116,7 @@ options_postprocess_mutate(struct options *o)
helper_keepalive(o);
helper_tcp_nodelay(o);
+ options_postprocess_cipher(o);
options_postprocess_mutate_invariant(o);
if (o->ncp_enabled)
@@ -3114,16 +3177,6 @@ options_postprocess_mutate(struct options *o)
"include this in your server configuration");
o->dh_file = NULL;
}
-
- /* cipher negotiation (NCP) currently assumes --pull or --mode server */
- if (o->ncp_enabled
- && !(o->pull || o->mode == MODE_SERVER) )
- {
- msg( M_WARN, "disabling NCP mode (--ncp-disable) because not "
- "in P2MP client or server mode" );
- o->ncp_enabled = false;
- }
-
#if ENABLE_MANAGEMENT
if (o->http_proxy_override)
{
@@ -3659,14 +3712,22 @@ options_string(const struct options *o,
*/
buf_printf(&out, ",dev-type %s", dev_type_string(o->dev, o->dev_type));
- buf_printf(&out, ",link-mtu %u", (unsigned int) calc_options_string_link_mtu(o, frame));
+ if (o->ciphername)
+ {
+ /* the link-mtu that we send has only a meaning if have a fixed
+ * cipher (p2p) or have a fallback cipher for older non ncp
+ * clients. If we do have a fallback cipher, do not send it */
+ buf_printf(&out, ",link-mtu %u",
+ (unsigned int) calc_options_string_link_mtu(o, frame));
+ }
buf_printf(&out, ",tun-mtu %d", PAYLOAD_SIZE(frame));
buf_printf(&out, ",proto %s", proto_remote(o->ce.proto, remote));
+ bool p2p_nopull = o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o);
/* send tun_ipv6 only in peer2peer mode - in client/server mode, it
* is usually pushed by the server, triggering a non-helpful warning
*/
- if (o->ifconfig_ipv6_local && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
+ if (o->ifconfig_ipv6_local && p2p_nopull)
{
buf_printf(&out, ",tun-ipv6");
}
@@ -3696,7 +3757,7 @@ options_string(const struct options *o,
}
}
- if (tt && o->mode == MODE_POINT_TO_POINT && !PULL_DEFINED(o))
+ if (tt && p2p_nopull)
{
const char *ios = ifconfig_options_string(tt, remote, o->ifconfig_nowarn, gc);
if (ios && strlen(ios))
@@ -3752,8 +3813,15 @@ options_string(const struct options *o,
init_key_type(&kt, o->ciphername, o->authname, o->keysize, true,
false);
-
- buf_printf(&out, ",cipher %s", cipher_kt_name(kt.cipher));
+ /* Only announce the cipher to our peer if we are willing to
+ * support it */
+ const char *ciphername = cipher_kt_name(kt.cipher);
+ if (p2p_nopull || !o->ncp_enabled
+ || (o->ncp_enabled
+ && tls_item_in_cipher_list(ciphername, o->ncp_ciphers)))
+ {
+ buf_printf(&out, ",cipher %s", ciphername);
+ }
buf_printf(&out, ",auth %s", md_kt_name(kt.digest));
buf_printf(&out, ",keysize %d", kt.cipher_length * 8);
if (o->shared_secret_file)
@@ -3871,7 +3939,8 @@ options_warning_safe_scan2(const int msglevel,
|| strprefix(p1, "keydir ")
|| strprefix(p1, "proto ")
|| strprefix(p1, "tls-auth ")
- || strprefix(p1, "tun-ipv6"))
+ || strprefix(p1, "tun-ipv6")
+ || strprefix(p1, "cipher "))
{
return;
}
@@ -7866,6 +7935,12 @@ add_option(struct options *options,
VERIFY_PERMISSION(OPT_P_NCP|OPT_P_INSTANCE);
options->ciphername = p[1];
}
+ else if (streq(p[0], "fallback-cipher") && p[1] && !p[2])
+ {
+ VERIFY_PERMISSION(OPT_P_INSTANCE);
+ options->ciphername = p[1];
+ options->enable_ncp_fallback = true;
+ }
else if ((streq(p[0], "data-ciphers") || streq(p[0], "ncp-ciphers"))
&& p[1] && !p[2])
{
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index c5df2d18..877e9396 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -503,6 +503,8 @@ struct options
bool shared_secret_file_inline;
int key_direction;
const char *ciphername;
+ bool enable_ncp_fallback; /**< If defined fall back to
+ * ciphername if NCP fails */
bool ncp_enabled;
const char *ncp_ciphers;
const char *authname;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index cb18121a..6cf9fe5f 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1932,13 +1932,14 @@ tls_session_update_crypto_params(struct tls_session *session,
return true;
}
- if (!session->opt->server
- && 0 != strcmp(options->ciphername, session->opt->config_ciphername)
+ bool cipher_allowed_as_fallback = options->enable_ncp_fallback
+ && streq(options->ciphername, session->opt->config_ciphername);
+
+ if (!session->opt->server && !cipher_allowed_as_fallback
&& !tls_item_in_cipher_list(options->ciphername, options->ncp_ciphers))
{
- msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s or %s",
- options->ciphername, session->opt->config_ciphername,
- options->ncp_ciphers);
+ msg(D_TLS_ERRORS, "Error: pushed cipher not allowed - %s not in %s",
+ options->ciphername, options->ncp_ciphers);
/* undo cipher push, abort connection setup */
options->ciphername = session->opt->config_ciphername;
return false;
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index 6760884e..68542880 100644
--- a/src/openvpn/ssl_ncp.c
+++ b/src/openvpn/ssl_ncp.c
@@ -211,9 +211,8 @@ tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
}
char *
-ncp_get_best_cipher(const char *server_list, const char *server_cipher,
- const char *peer_info, const char *remote_cipher,
- struct gc_arena *gc)
+ncp_get_best_cipher(const char *server_list, const char *peer_info,
+ const char *remote_cipher, struct gc_arena *gc)
{
/*
* The gc of the parameter is tied to the VPN session, create a
@@ -243,15 +242,6 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
}
token = strsep(&tmp_ciphers, ":");
}
- /* We have not found a common cipher, as a last resort check if the
- * server cipher can be used
- */
- if (token == NULL
- && (tls_item_in_cipher_list(server_cipher, peer_ncp_list)
- || streq(server_cipher, remote_cipher)))
- {
- token = server_cipher;
- }
char *ret = NULL;
if (token != NULL)
@@ -263,16 +253,18 @@ ncp_get_best_cipher(const char *server_list, const char *server_cipher,
return ret;
}
-void
+bool
tls_poor_mans_ncp(struct options *o, const char *remote_ciphername)
{
- if (o->ncp_enabled && remote_ciphername
+ if (remote_ciphername
&& 0 != strcmp(o->ciphername, remote_ciphername))
{
if (tls_item_in_cipher_list(remote_ciphername, o->ncp_ciphers))
{
o->ciphername = string_alloc(remote_ciphername, &o->gc);
msg(D_TLS_DEBUG_LOW, "Using peer cipher '%s'", o->ciphername);
+ return true;
}
}
+ return false;
}
diff --git a/src/openvpn/ssl_ncp.h b/src/openvpn/ssl_ncp.h
index d00c222d..98f80286 100644
--- a/src/openvpn/ssl_ncp.h
+++ b/src/openvpn/ssl_ncp.h
@@ -44,10 +44,13 @@ tls_peer_supports_ncp(const char *peer_info);
* "Poor man's NCP": Use peer cipher if it is an allowed (NCP) cipher.
* Allows non-NCP peers to upgrade their cipher individually.
*
+ * Returns true if we switched to the peer's cipher
+ *
* Make sure to call tls_session_update_crypto_params() after calling this
* function.
*/
-void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
+bool
+tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
/**
* Iterates through the ciphers in server_list and return the first
@@ -67,9 +70,8 @@ void tls_poor_mans_ncp(struct options *o, const char *remote_ciphername);
* cipher
*/
char *
-ncp_get_best_cipher(const char *server_list, const char *server_cipher,
- const char *peer_info, const char *remote_cipher,
- struct gc_arena *gc);
+ncp_get_best_cipher(const char *server_list, const char *peer_info,
+ const char *remote_cipher, struct gc_arena *gc);
/**
diff --git a/tests/unit_tests/openvpn/test_ncp.c b/tests/unit_tests/openvpn/test_ncp.c
index 19432410..ea950030 100644
--- a/tests/unit_tests/openvpn/test_ncp.c
+++ b/tests/unit_tests/openvpn/test_ncp.c
@@ -139,21 +139,29 @@ test_poor_man(void **state)
char *best_cipher;
const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM";
+ const char *serverlistbfcbc = "CHACHA20_POLY1305:AES-128-GCM:BF-CBC";
- best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+ best_cipher = ncp_get_best_cipher(serverlist,
+ "IV_YOLO=NO\nIV_BAR=7",
+ "BF-CBC", &gc);
+
+ assert_ptr_equal(best_cipher, NULL);
+
+
+ best_cipher = ncp_get_best_cipher(serverlistbfcbc,
"IV_YOLO=NO\nIV_BAR=7",
"BF-CBC", &gc);
assert_string_equal(best_cipher, "BF-CBC");
- best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+
+ best_cipher = ncp_get_best_cipher(serverlist,
"IV_NCP=1\nIV_BAR=7",
"AES-128-GCM", &gc);
assert_string_equal(best_cipher, "AES-128-GCM");
- best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
- NULL,
+ best_cipher = ncp_get_best_cipher(serverlist, NULL,
"AES-128-GCM", &gc);
assert_string_equal(best_cipher, "AES-128-GCM");
@@ -170,29 +178,27 @@ test_ncp_best(void **state)
const char *serverlist = "CHACHA20_POLY1305:AES-128-GCM:AES-256-GCM";
- best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+ best_cipher = ncp_get_best_cipher(serverlist,
"IV_YOLO=NO\nIV_NCP=2\nIV_BAR=7",
"BF-CBC", &gc);
assert_string_equal(best_cipher, "AES-128-GCM");
/* Best cipher is in --cipher of client */
- best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
- "IV_NCP=2\nIV_BAR=7",
+ best_cipher = ncp_get_best_cipher(serverlist, "IV_NCP=2\nIV_BAR=7",
"CHACHA20_POLY1305", &gc);
assert_string_equal(best_cipher, "CHACHA20_POLY1305");
/* Best cipher is in --cipher of client */
- best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
- "IV_CIPHERS=AES-128-GCM",
+ best_cipher = ncp_get_best_cipher(serverlist, "IV_CIPHERS=AES-128-GCM",
"AES-256-CBC", &gc);
assert_string_equal(best_cipher, "AES-128-GCM");
/* IV_NCP=2 should be ignored if IV_CIPHERS is sent */
- best_cipher = ncp_get_best_cipher(serverlist, "BF-CBC",
+ best_cipher = ncp_get_best_cipher(serverlist,
"IV_FOO=7\nIV_CIPHERS=AES-256-GCM\nIV_NCP=2",
"AES-256-CBC", &gc);
--
2.26.2
|
|
From: Antonio Q. <a...@un...> - 2020-07-17 14:04:15
|
Hi,
On 17/07/2020 15:47, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
>
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
> src/openvpn/multi.c | 12 ++++++++++--
> src/openvpn/ssl.c | 15 +++++++++++++--
> src/openvpn/ssl.h | 7 +++++++
> 3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 0095c871..88ba9db2 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1795,10 +1795,18 @@ multi_client_set_protocol_options(struct context *c)
> {
> int proto = 0;
> int r = sscanf(optstr, "IV_PROTO=%d", &proto);
> - if ((r == 1) && (proto >= 2))
> + if (r == 1)
> {
> - tls_multi->use_peer_id = true;
> + if (proto >= 2)
I thought it was agreed (but I may be wrong) to substitute this check
with a bitwise AND, since this variable is now treated as a bitfield.
Regards,
> + {
> + tls_multi->use_peer_id = true;
> + }
> + if (proto & IV_PROTO_REQUEST_PUSH)
> + {
> + c->c2.push_request_received = true;
> + }
> }
> +
> }
>
> /* Select cipher if client supports Negotiable Crypto Parameters */
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 54a23011..04d78a81 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
> buf_printf(&out, "IV_PLAT=win\n");
> #endif
>
> - /* support for P_DATA_V2 */
> - buf_printf(&out, "IV_PROTO=2\n");
> + /* support for P_DATA_V2*/
> + int iv_proto = IV_PROTO_DATA_V2;
> +
> + /* support for receiving push_reply before sending
> + * push request, also signal that the client wants
> + * to get push-reply messages without without requiring a round
> + * trip for a push request message*/
> + if(session->opt->pull)
> + {
> + iv_proto |= IV_PROTO_REQUEST_PUSH;
> + }
> +
> + buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
>
> /* support for Negotiable Crypto Parameters */
> if (session->opt->ncp_enabled
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 58a9b0d4..86fb6853 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -99,6 +99,13 @@
> /* Maximum length of OCC options string passed as part of auth handshake */
> #define TLS_OPTIONS_LEN 512
>
> +/* Definitions of the bits in the IV_PROTO bitfield */
> +#define IV_PROTO_DATA_V2 (1<<1) /**< Support P_DATA_V2 */
> +#define IV_PROTO_REQUEST_PUSH (1<<2) /**< Assume client will send a push
> + * request and server does not need
> + * to wait for a push-request to send
> + * a push-reply */
> +
> /* Default field in X509 to be username */
> #define X509_USERNAME_FIELD_DEFAULT "CN"
>
>
--
Antonio Quartulli
|
|
From: Steffan K. <ste...@fo...> - 2020-07-20 08:15:07
|
Hi,
On 17-07-2020 15:47, Arne Schwabe wrote:
> This allows us to skip waiting for the first PUSH_REQUEST message from
> the client to send the response.
Feature-ACK, clever use of existing infra. Some comments though:
This commit message could use a bit more information. In particular, it
would be good to explain how the various OpenVPN versions currently
interpret IV_PROTO values, and why the change from a version-like
interpretation to a bitfield will not cause issues.
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
> src/openvpn/multi.c | 12 ++++++++++--
> src/openvpn/ssl.c | 15 +++++++++++++--
> src/openvpn/ssl.h | 7 +++++++
> 3 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 0095c871..88ba9db2 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -1795,10 +1795,18 @@ multi_client_set_protocol_options(struct context *c)
> {
> int proto = 0;
> int r = sscanf(optstr, "IV_PROTO=%d", &proto);
> - if ((r == 1) && (proto >= 2))
> + if (r == 1)
> {
> - tls_multi->use_peer_id = true;
> + if (proto >= 2)
> + {
> + tls_multi->use_peer_id = true;
> + }
Wouldn't checking for proto & IV_PROTO_REQUEST_PUSH suffice? Any 2.3/2.4
client will only ever send "2", and newer clients will know this is a
bitfield now.
This works fine, but - in particular without an explaining comment -
first checking for a value, then continue as a bitfield is somewhat
surprising.
> + if (proto & IV_PROTO_REQUEST_PUSH)
> + {
> + c->c2.push_request_received = true;
> + }
> }
> +
> }
>
> /* Select cipher if client supports Negotiable Crypto Parameters */
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 54a23011..04d78a81 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -2299,8 +2299,19 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
> buf_printf(&out, "IV_PLAT=win\n");
> #endif
>
> - /* support for P_DATA_V2 */
> - buf_printf(&out, "IV_PROTO=2\n");
> + /* support for P_DATA_V2*/
> + int iv_proto = IV_PROTO_DATA_V2;
> +
> + /* support for receiving push_reply before sending
> + * push request, also signal that the client wants
> + * to get push-reply messages without without requiring a round
> + * trip for a push request message*/
> + if(session->opt->pull)
This needs a space behind the if.
> + {
> + iv_proto |= IV_PROTO_REQUEST_PUSH;
> + }
> +
> + buf_printf(&out, "IV_PROTO=%d\n", iv_proto);
>
> /* support for Negotiable Crypto Parameters */
> if (session->opt->ncp_enabled
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 58a9b0d4..86fb6853 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -99,6 +99,13 @@
> /* Maximum length of OCC options string passed as part of auth handshake */
> #define TLS_OPTIONS_LEN 512
>
> +/* Definitions of the bits in the IV_PROTO bitfield */
> +#define IV_PROTO_DATA_V2 (1<<1) /**< Support P_DATA_V2 */
> +#define IV_PROTO_REQUEST_PUSH (1<<2) /**< Assume client will send a push
> + * request and server does not need
> + * to wait for a push-request to send
> + * a push-reply */
> +
Style nit: would this not be more readable if you place the (rather
long) comment above the value, instead of behind?
Also: any particular reason to no use 1<<0 ? If we should not use that
for some reason, let's mark it as reserved. Otherwise: maybe just use it?
-Steffan
|
|
From: David S. <op...@sf...> - 2020-07-20 13:16:52
Attachments:
signature.asc
|
On 17/07/2020 15:47, Arne Schwabe wrote:
> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
>
> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
> v1 op codes and give a good warning message if we encounter
> them in the legal op codes pre-check.
>
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
> doc/doxygen/doc_control_processor.h | 6 +-
> doc/doxygen/doc_key_generation.h | 6 +-
> doc/doxygen/doc_protocol_overview.h | 2 +-
> src/openvpn/forward.c | 2 +-
> src/openvpn/helper.c | 5 -
> src/openvpn/init.c | 1 -
> src/openvpn/options.c | 35 +----
> src/openvpn/options.h | 4 -
> src/openvpn/ssl.c | 230 ++++------------------------
> src/openvpn/ssl.h | 19 +--
> src/openvpn/ssl_common.h | 1 -
> 11 files changed, 42 insertions(+), 269 deletions(-)
[...snip...]
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 00b97352..4144217d 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
[...snip...]
> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
> return str;
> }
>
> -/*
> - * Handle the reading and writing of key data to and from
> - * the TLS control channel (cleartext).
> - */
Is it needed to remove this comment? Or move it after the push_peer_info()
function?
> -static bool
> -key_method_1_write(struct buffer *buf, struct tls_session *session)
> -{
[...snip...]
> -}
> -
> static bool
> push_peer_info(struct buffer *buf, struct tls_session *session)
> {
> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
> * push request, also signal that the client wants
> * to get push-reply messages without without requiring a round
> * trip for a push request message*/
> - if(session->opt->pull)
> + if (session->opt->pull)
> {
> iv_proto |= IV_PROTO_REQUEST_PUSH;
> }
> @@ -2394,7 +2328,6 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
[...snip...]
> @@ -3470,14 +3316,12 @@ tls_pre_decrypt(struct tls_multi *multi,
> }
>
> /* hard reset ? */
> - if (is_hard_reset(op, 0))
> + if (is_hard_reset_method2(op))
> {
> /* verify client -> server or server -> client connection */
> - if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
> - || op == P_CONTROL_HARD_RESET_CLIENT_V2
> + if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
> || op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
> - || ((op == P_CONTROL_HARD_RESET_SERVER_V1
> - || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
> + || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
> {
> msg(D_TLS_ERRORS,
> "TLS Error: client->client or server->server connection attempted from %s",
> @@ -3542,19 +3386,11 @@ tls_pre_decrypt(struct tls_multi *multi,
> * Initial packet received.
> */
>
> - if (i == TM_SIZE && is_hard_reset(op, 0))
> + if (i == TM_SIZE && is_hard_reset_method2(op))
> {
> struct tls_session *session = &multi->session[TM_ACTIVE];
> struct key_state *ks = &session->key[KS_PRIMARY];
>
> - if (!is_hard_reset(op, multi->opt.key_method))
> - {
> - msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
> - multi->opt.key_method,
> - packet_opcode_name(op));
> - goto error;
> - }
> -
> /*
> * If we have no session currently in progress, the initial packet will
> * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
> @@ -3594,7 +3430,7 @@ tls_pre_decrypt(struct tls_multi *multi,
> }
> }
>
> - if (i == TM_SIZE && is_hard_reset(op, 0))
> + if (i == TM_SIZE && is_hard_reset_method2(op))
Do we need this if() block? Considering it is exactly the same if() statement
as the previous if() block. I would suggest merging them, but you might hit a
challenge with reuse of struct tls_session *session, where one is referring to
TM_ACTIVE vs TM_UNTRUSTED.
> {
> /*
> * No match with existing sessions,
> @@ -3614,14 +3450,6 @@ tls_pre_decrypt(struct tls_multi *multi,
> goto error;
> }
>
> - if (!is_hard_reset(op, multi->opt.key_method))
> - {
> - msg(D_TLS_ERRORS, "TLS ERROR: new session local/remote key_method mismatch, local key_method=%d, op=%s",
> - multi->opt.key_method,
> - packet_opcode_name(op));
> - goto error;
> - }
> -
> if (!read_control_auth(buf, &session->tls_wrap, from,
> session->opt))
> {
I had already started my own approach of removing --key-method when I was made
aware of this patch. Considering the size of it, my own barely tested changes
is about 90% similar to this patch. But this patch has some improvements I
didn't consider in my work.
I've done a quick 'make check' as well, and it works fine. There are just a
couple of minor things to consider, otherwise I will happily ACK this patch.
--
kind regards,
David Sommerseth
OpenVPN Inc
|
|
From: Arne S. <ar...@rf...> - 2020-07-20 13:22:30
Attachments:
signature.asc
|
Am 20.07.20 um 15:16 schrieb David Sommerseth:
> On 17/07/2020 15:47, Arne Schwabe wrote:
>> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
>>
>> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
>> v1 op codes and give a good warning message if we encounter
>> them in the legal op codes pre-check.
>>
>> Signed-off-by: Arne Schwabe <ar...@rf...>
>> ---
>> doc/doxygen/doc_control_processor.h | 6 +-
>> doc/doxygen/doc_key_generation.h | 6 +-
>> doc/doxygen/doc_protocol_overview.h | 2 +-
>> src/openvpn/forward.c | 2 +-
>> src/openvpn/helper.c | 5 -
>> src/openvpn/init.c | 1 -
>> src/openvpn/options.c | 35 +----
>> src/openvpn/options.h | 4 -
>> src/openvpn/ssl.c | 230 ++++------------------------
>> src/openvpn/ssl.h | 19 +--
>> src/openvpn/ssl_common.h | 1 -
>> 11 files changed, 42 insertions(+), 269 deletions(-)
>
> [...snip...]
>
>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
>> index 00b97352..4144217d 100644
>> --- a/src/openvpn/ssl.c
>> +++ b/src/openvpn/ssl.c
>
> [...snip...]
>
>> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
>> return str;
>> }
>>
>> -/*
>> - * Handle the reading and writing of key data to and from
>> - * the TLS control channel (cleartext).
>> - */
>
> Is it needed to remove this comment? Or move it after the push_peer_info()
> function?
Yeah, we can move the comment.
>
>> -static bool
>> -key_method_1_write(struct buffer *buf, struct tls_session *session)
>> -{
> [...snip...]
>> -}
>> -
>> static bool
>> push_peer_info(struct buffer *buf, struct tls_session *session)
>> {
>> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>> * push request, also signal that the client wants
>> * to get push-reply messages without without requiring a round
>> * trip for a push request message*/
>> - if(session->opt->pull)
>> + if (session->opt->pull)
>> {
>> iv_proto |= IV_PROTO_REQUEST_PUSH;
>> }
>> @@ -2394,7 +2328,6 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
>
> [...snip...]
>
>> @@ -3470,14 +3316,12 @@ tls_pre_decrypt(struct tls_multi *multi,
>> }
>>
>> /* hard reset ? */
>> - if (is_hard_reset(op, 0))
>> + if (is_hard_reset_method2(op))
>> {
>> /* verify client -> server or server -> client connection */
>> - if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
>> - || op == P_CONTROL_HARD_RESET_CLIENT_V2
>> + if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
>> || op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
>> - || ((op == P_CONTROL_HARD_RESET_SERVER_V1
>> - || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
>> + || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
>> {
>> msg(D_TLS_ERRORS,
>> "TLS Error: client->client or server->server connection attempted from %s",
>> @@ -3542,19 +3386,11 @@ tls_pre_decrypt(struct tls_multi *multi,
>> * Initial packet received.
>> */
>>
>> - if (i == TM_SIZE && is_hard_reset(op, 0))
>> + if (i == TM_SIZE && is_hard_reset_method2(op))
>> {
>> struct tls_session *session = &multi->session[TM_ACTIVE];
>> struct key_state *ks = &session->key[KS_PRIMARY];
>>
>> - if (!is_hard_reset(op, multi->opt.key_method))
>> - {
>> - msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
>> - multi->opt.key_method,
>> - packet_opcode_name(op));
>> - goto error;
>> - }
>> -
>> /*
>> * If we have no session currently in progress, the initial packet will
>> * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
>> @@ -3594,7 +3430,7 @@ tls_pre_decrypt(struct tls_multi *multi,
>> }
>> }
>>
>> - if (i == TM_SIZE && is_hard_reset(op, 0))
>> + if (i == TM_SIZE && is_hard_reset_method2(op))
>
> Do we need this if() block? Considering it is exactly the same if() statement
> as the previous if() block. I would suggest merging them, but you might hit a
> challenge with reuse of struct tls_session *session, where one is referring to
> TM_ACTIVE vs TM_UNTRUSTED.
That is something that might be possible but I would consider outside of
patch. The first block actually modifies i (i = TM_ACTIVE;), so that
needs a separate patch with an explaination why/how we simplify the
logic here.
Arne
|
|
From: David S. <op...@sf...> - 2020-07-20 18:50:08
Attachments:
signature.asc
|
On 20/07/2020 15:22, Arne Schwabe wrote:
> Am 20.07.20 um 15:16 schrieb David Sommerseth:
>> On 17/07/2020 15:47, Arne Schwabe wrote:
>>> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
>>>
>>> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
>>> v1 op codes and give a good warning message if we encounter
>>> them in the legal op codes pre-check.
>>>
>>> Signed-off-by: Arne Schwabe <ar...@rf...>
>>> ---
>>> doc/doxygen/doc_control_processor.h | 6 +-
>>> doc/doxygen/doc_key_generation.h | 6 +-
>>> doc/doxygen/doc_protocol_overview.h | 2 +-
>>> src/openvpn/forward.c | 2 +-
>>> src/openvpn/helper.c | 5 -
>>> src/openvpn/init.c | 1 -
>>> src/openvpn/options.c | 35 +----
>>> src/openvpn/options.h | 4 -
>>> src/openvpn/ssl.c | 230 ++++------------------------
>>> src/openvpn/ssl.h | 19 +--
>>> src/openvpn/ssl_common.h | 1 -
>>> 11 files changed, 42 insertions(+), 269 deletions(-)
>>
>> [...snip...]
>>
>>> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
>>> index 00b97352..4144217d 100644
>>> --- a/src/openvpn/ssl.c
>>> +++ b/src/openvpn/ssl.c
>>
>> [...snip...]
>>
>>> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
>>> return str;
>>> }
>>>
>>> -/*
>>> - * Handle the reading and writing of key data to and from
>>> - * the TLS control channel (cleartext).
>>> - */
>>
>> Is it needed to remove this comment? Or move it after the push_peer_info()
>> function?
>
> Yeah, we can move the comment.
Yes, please ... Since it's not a bad comment, I don't like loosing helpful
pointers in comments :)
>>
>>> -static bool
>>> -key_method_1_write(struct buffer *buf, struct tls_session *session)
>>> -{
>> [...snip...]
>>> -}
>>> -
>>> static bool
>>> push_peer_info(struct buffer *buf, struct tls_session *session)
>>> {
>>> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
>>> * push request, also signal that the client wants
>>> * to get push-reply messages without without requiring a round
>>> * trip for a push request message*/
>>> - if(session->opt->pull)
>>> + if (session->opt->pull)
>>> {
>>> iv_proto |= IV_PROTO_REQUEST_PUSH;
>>> }
>>> @@ -2394,7 +2328,6 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
>>
>> [...snip...]
>>
>>> @@ -3470,14 +3316,12 @@ tls_pre_decrypt(struct tls_multi *multi,
>>> }
>>>
>>> /* hard reset ? */
>>> - if (is_hard_reset(op, 0))
>>> + if (is_hard_reset_method2(op))
>>> {
>>> /* verify client -> server or server -> client connection */
>>> - if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
>>> - || op == P_CONTROL_HARD_RESET_CLIENT_V2
>>> + if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
>>> || op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
>>> - || ((op == P_CONTROL_HARD_RESET_SERVER_V1
>>> - || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
>>> + || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
>>> {
>>> msg(D_TLS_ERRORS,
>>> "TLS Error: client->client or server->server connection attempted from %s",
>>> @@ -3542,19 +3386,11 @@ tls_pre_decrypt(struct tls_multi *multi,
>>> * Initial packet received.
>>> */
>>>
>>> - if (i == TM_SIZE && is_hard_reset(op, 0))
>>> + if (i == TM_SIZE && is_hard_reset_method2(op))
>>> {
>>> struct tls_session *session = &multi->session[TM_ACTIVE];
>>> struct key_state *ks = &session->key[KS_PRIMARY];
>>>
>>> - if (!is_hard_reset(op, multi->opt.key_method))
>>> - {
>>> - msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
>>> - multi->opt.key_method,
>>> - packet_opcode_name(op));
>>> - goto error;
>>> - }
>>> -
>>> /*
>>> * If we have no session currently in progress, the initial packet will
>>> * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
>>> @@ -3594,7 +3430,7 @@ tls_pre_decrypt(struct tls_multi *multi,
>>> }
>>> }
>>>
>>> - if (i == TM_SIZE && is_hard_reset(op, 0))
>>> + if (i == TM_SIZE && is_hard_reset_method2(op))
>>
>> Do we need this if() block? Considering it is exactly the same if() statement
>> as the previous if() block. I would suggest merging them, but you might hit a
>> challenge with reuse of struct tls_session *session, where one is referring to
>> TM_ACTIVE vs TM_UNTRUSTED.
>
> That is something that might be possible but I would consider outside of
> patch. The first block actually modifies i (i = TM_ACTIVE;), so that
> needs a separate patch with an explaination why/how we simplify the
> logic here.
Alright, I can see the argument of not touching it now.
Could we then just add a comment to these two sections for now, indicating why
they have been kept separate despite the if() statement being identical?
Something like "This section processes {TM_ACTIVE, TM_UNTRUSTED} sessions" and
elaborate a little bit around that, which may help refactoring this code later on.
--
kind regards,
David Sommerseth
OpenVPN Inc
|
|
From: Arne S. <ar...@rf...> - 2020-07-21 10:01:44
|
Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
v1 op codes and give a good warning message if we encounter
them in the legal op codes pre-check.
Patch V3: Add a bit more comments in the existing methods.
Signed-off-by: Arne Schwabe <ar...@rf...>
---
doc/doxygen/doc_control_processor.h | 6 +-
doc/doxygen/doc_key_generation.h | 6 +-
doc/doxygen/doc_protocol_overview.h | 2 +-
src/openvpn/forward.c | 2 +-
src/openvpn/helper.c | 5 -
src/openvpn/init.c | 1 -
src/openvpn/options.c | 35 +---
src/openvpn/options.h | 4 -
src/openvpn/ssl.c | 240 +++++-----------------------
src/openvpn/ssl.h | 19 +--
src/openvpn/ssl_common.h | 1 -
11 files changed, 53 insertions(+), 268 deletions(-)
diff --git a/doc/doxygen/doc_control_processor.h b/doc/doxygen/doc_control_processor.h
index f87324cc..1bbf2d2d 100644
--- a/doc/doxygen/doc_control_processor.h
+++ b/doc/doxygen/doc_control_processor.h
@@ -175,11 +175,7 @@
* appropriate messages to be sent.
*
* @par Functions which control data channel key generation
- * - Key method 1 key exchange functions:
- * - \c key_method_1_write(), generates and processes key material to
- * be sent to the remote OpenVPN peer.
- * - \c key_method_1_read(), processes key material received from the
- * remote OpenVPN peer.
+ * - Key method 1 key exchange functions were removed from OpenVPN 2.5
* - Key method 2 key exchange functions:
* - \c key_method_2_write(), generates and processes key material to
* be sent to the remote OpenVPN peer.
diff --git a/doc/doxygen/doc_key_generation.h b/doc/doxygen/doc_key_generation.h
index efe61155..4bb9c708 100644
--- a/doc/doxygen/doc_key_generation.h
+++ b/doc/doxygen/doc_key_generation.h
@@ -131,11 +131,7 @@ S_ACTIVE S_ACTIVE
* control_processor Control Channel Processor module's\endlink \c
* tls_process() function and control the %key generation and exchange
* process as follows:
- * - %Key method 1:
- * - \c key_method_1_write(): generate random material locally, and load
- * as "sending" keys.
- * - \c key_method_1_read(): read random material received from remote
- * peer, and load as "receiving" keys.
+ * - %Key method 1 has been removed in OpenVPN 2.5
* - %Key method 2:
* - \c key_method_2_write(): generate random material locally, and if
* in server mode generate %key expansion.
diff --git a/doc/doxygen/doc_protocol_overview.h b/doc/doxygen/doc_protocol_overview.h
index 3f48b18a..08212223 100644
--- a/doc/doxygen/doc_protocol_overview.h
+++ b/doc/doxygen/doc_protocol_overview.h
@@ -150,7 +150,7 @@
*
* @subsection network_protocol_control_plaintext Structure of plaintext control channel messages
*
- * - %Key method 1:
+ * - %Key method 1 (support removed in OpenVPN 2.5):
* - Cipher %key length in bytes (1 byte).
* - Cipher %key (n bytes).
* - HMAC %key length in bytes (1 byte).
diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
index 5c4370a8..698451d1 100644
--- a/src/openvpn/forward.c
+++ b/src/openvpn/forward.c
@@ -1100,7 +1100,7 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
floated, &ad_start))
{
/* Restore pre-NCP frame parameters */
- if (is_hard_reset(opcode, c->options.key_method))
+ if (is_hard_reset_method2(opcode))
{
c->c2.frame = c->c2.frame_initial;
#ifdef ENABLE_FRAGMENT
diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 6e9cc63c..a1d03070 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -490,11 +490,6 @@ helper_client_server(struct options *o)
*/
if (o->client)
{
- if (o->key_method != 2)
- {
- msg(M_USAGE, "--client requires --key-method 2");
- }
-
o->pull = true;
o->tls_client = true;
}
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index e9c01629..b96d1471 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2852,7 +2852,6 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
to.ssl_ctx = c->c1.ks.ssl_ctx;
to.key_type = c->c1.ks.key_type;
to.server = options->tls_server;
- to.key_method = options->key_method;
to.replay = options->replay;
to.replay_window = options->replay_window;
to.replay_time = options->replay_time;
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7da04b6f..14d4c911 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -881,7 +881,6 @@ init_options(struct options *o, const bool init_gc)
#ifdef ENABLE_PREDICTION_RESISTANCE
o->use_prediction_resistance = false;
#endif
- o->key_method = 2;
o->tls_timeout = 2;
o->renegotiate_bytes = -1;
o->renegotiate_seconds = 3600;
@@ -1719,7 +1718,6 @@ show_settings(const struct options *o)
SHOW_BOOL(tls_server);
SHOW_BOOL(tls_client);
- SHOW_INT(key_method);
SHOW_STR_INLINE(ca_file);
SHOW_STR(ca_path);
SHOW_STR(dh_file);
@@ -2380,10 +2378,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
{
msg(M_USAGE, "--ccd-exclusive must be used with --client-config-dir");
}
- if (options->key_method != 2)
- {
- msg(M_USAGE, "--mode server requires --key-method 2");
- }
if (options->auth_token_generate && !options->renegotiate_seconds)
{
msg(M_USAGE, "--auth-gen-token needs a non-infinite "
@@ -2550,13 +2544,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
"may accept clients which do not present a certificate");
}
- if (options->key_method == 1)
- {
- msg(M_WARN, "WARNING: --key-method 1 is deprecated and will be removed "
- "in OpenVPN 2.5. By default --key-method 2 will be used if not set "
- "in the configuration file, which is the recommended approach.");
- }
-
const int tls_version_max =
(options->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
& SSLF_TLS_VERSION_MAX_MASK;
@@ -2798,7 +2785,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
MUST_BE_UNDEF(push_peer_info);
MUST_BE_UNDEF(tls_exit);
MUST_BE_UNDEF(crl_file);
- MUST_BE_UNDEF(key_method);
MUST_BE_UNDEF(ns_cert_type);
MUST_BE_UNDEF(remote_cert_ku[0]);
MUST_BE_UNDEF(remote_cert_eku);
@@ -3827,10 +3813,7 @@ options_string(const struct options *o,
* tls-auth/tls-crypt does not match. Removing tls-auth here would
* break stuff, so leaving that in place. */
- if (o->key_method > 1)
- {
- buf_printf(&out, ",key-method %d", o->key_method);
- }
+ buf_printf(&out, ",key-method %d", KEY_METHOD_2);
}
if (remote)
@@ -8476,22 +8459,6 @@ add_option(struct options *options,
VERIFY_PERMISSION(OPT_P_GENERAL);
options->tls_crypt_v2_verify_script = p[1];
}
- else if (streq(p[0], "key-method") && p[1] && !p[2])
- {
- int key_method;
-
- VERIFY_PERMISSION(OPT_P_GENERAL);
- key_method = atoi(p[1]);
- if (key_method < KEY_METHOD_MIN || key_method > KEY_METHOD_MAX)
- {
- msg(msglevel, "key_method parameter (%d) must be >= %d and <= %d",
- key_method,
- KEY_METHOD_MIN,
- KEY_METHOD_MAX);
- goto err;
- }
- options->key_method = key_method;
- }
else if (streq(p[0], "x509-track") && p[1] && !p[2])
{
VERIFY_PERMISSION(OPT_P_GENERAL);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 1b038c91..3546bab3 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -572,10 +572,6 @@ struct options
#ifdef ENABLE_CRYPTOAPI
const char *cryptoapi_cert;
#endif
-
- /* data channel key exchange method */
- int key_method;
-
/* Per-packet timeout on control channel */
int tls_timeout;
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 00b97352..ed35f792 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -861,23 +861,12 @@ print_key_id(struct tls_multi *multi, struct gc_arena *gc)
}
bool
-is_hard_reset(int op, int key_method)
+is_hard_reset_method2(int op)
{
- if (!key_method || key_method == 1)
+ if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
+ || op == P_CONTROL_HARD_RESET_CLIENT_V3)
{
- if (op == P_CONTROL_HARD_RESET_CLIENT_V1 || op == P_CONTROL_HARD_RESET_SERVER_V1)
- {
- return true;
- }
- }
-
- if (!key_method || key_method >= 2)
- {
- if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
- || op == P_CONTROL_HARD_RESET_CLIENT_V3)
- {
- return true;
- }
+ return true;
}
return false;
@@ -1097,23 +1086,14 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session)
}
/* Are we a TLS server or client? */
- ASSERT(session->opt->key_method >= 1);
- if (session->opt->key_method == 1)
+ if (session->opt->server)
{
- session->initial_opcode = session->opt->server ?
- P_CONTROL_HARD_RESET_SERVER_V1 : P_CONTROL_HARD_RESET_CLIENT_V1;
+ session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
}
- else /* session->opt->key_method >= 2 */
+ else
{
- if (session->opt->server)
- {
- session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
- }
- else
- {
- session->initial_opcode = session->opt->tls_crypt_v2 ?
- P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
- }
+ session->initial_opcode = session->opt->tls_crypt_v2 ?
+ P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
}
/* Initialize control channel authentication parameters */
@@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
return str;
}
-/*
- * Handle the reading and writing of key data to and from
- * the TLS control channel (cleartext).
- */
-
-static bool
-key_method_1_write(struct buffer *buf, struct tls_session *session)
-{
- struct key key;
- struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
-
- ASSERT(session->opt->key_method == 1);
- ASSERT(buf_init(buf, 0));
-
- generate_key_random(&key, &session->opt->key_type);
- if (!check_key(&key, &session->opt->key_type))
- {
- msg(D_TLS_ERRORS, "TLS Error: Bad encrypting key generated");
- return false;
- }
-
- if (!write_key(&key, &session->opt->key_type, buf))
- {
- msg(D_TLS_ERRORS, "TLS Error: write_key failed");
- return false;
- }
-
- init_key_ctx(&ks->crypto_options.key_ctx_bi.encrypt, &key,
- &session->opt->key_type, OPENVPN_OP_ENCRYPT,
- "Data Channel Encrypt");
- secure_memzero(&key, sizeof(key));
-
- /* send local options string */
- {
- const char *local_options = local_options_string(session);
- const int optlen = strlen(local_options) + 1;
- if (!buf_write(buf, local_options, optlen))
- {
- msg(D_TLS_ERRORS, "TLS Error: KM1 write options failed");
- return false;
- }
- }
-
- return true;
-}
-
static bool
push_peer_info(struct buffer *buf, struct tls_session *session)
{
@@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
* push request, also signal that the client wants
* to get push-reply messages without without requiring a round
* trip for a push request message*/
- if(session->opt->pull)
+ if (session->opt->pull)
{
iv_proto |= IV_PROTO_REQUEST_PUSH;
}
@@ -2389,12 +2323,15 @@ error:
return ret;
}
+/**
+ * Handle the writing of key data, peer-info, username/password, OCC
+ * to the TLS control channel (cleartext).
+ */
static bool
key_method_2_write(struct buffer *buf, struct tls_session *session)
{
struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
- ASSERT(session->opt->key_method == 2);
ASSERT(buf_init(buf, 0));
/* write a uint32 0 */
@@ -2404,7 +2341,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
}
/* write key_method + flags */
- if (!buf_write_u8(buf, (session->opt->key_method & KEY_METHOD_MASK)))
+ if (!buf_write_u8(buf, KEY_METHOD_2))
{
goto error;
}
@@ -2506,73 +2443,15 @@ error:
return false;
}
-static bool
-key_method_1_read(struct buffer *buf, struct tls_session *session)
-{
- int status;
- struct key key;
- struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
-
- ASSERT(session->opt->key_method == 1);
-
- if (!session->verified)
- {
- msg(D_TLS_ERRORS,
- "TLS Error: Certificate verification failed (key-method 1)");
- goto error;
- }
-
- status = read_key(&key, &session->opt->key_type, buf);
- if (status != 1)
- {
- msg(D_TLS_ERRORS,
- "TLS Error: Error reading data channel key from plaintext buffer");
- goto error;
- }
-
- if (!check_key(&key, &session->opt->key_type))
- {
- msg(D_TLS_ERRORS, "TLS Error: Bad decrypting key received from peer");
- goto error;
- }
-
- if (buf->len < 1)
- {
- msg(D_TLS_ERRORS, "TLS Error: Missing options string");
- goto error;
- }
-
-#ifdef ENABLE_OCC
- /* compare received remote options string
- * with our locally computed options string */
- if (!session->opt->disable_occ
- && !options_cmp_equal_safe((char *) BPTR(buf), session->opt->remote_options, buf->len))
- {
- options_warning_safe((char *) BPTR(buf), session->opt->remote_options, buf->len);
- }
-#endif
-
- buf_clear(buf);
-
- init_key_ctx(&ks->crypto_options.key_ctx_bi.decrypt, &key,
- &session->opt->key_type, OPENVPN_OP_DECRYPT,
- "Data Channel Decrypt");
- secure_memzero(&key, sizeof(key));
- ks->authenticated = KS_AUTH_TRUE;
- return true;
-
-error:
- buf_clear(buf);
- secure_memzero(&key, sizeof(key));
- return false;
-}
-
+/**
+ * Handle reading key data, peer-info, username/password, OCC
+ * from the TLS control channel (cleartext).
+ */
static bool
key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_session *session)
{
struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
- int key_method_flags;
bool username_status, password_status;
struct gc_arena gc = gc_new();
@@ -2582,8 +2461,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
/* allocate temporary objects */
ALLOC_ARRAY_CLEAR_GC(options, char, TLS_OPTIONS_LEN, &gc);
- ASSERT(session->opt->key_method == 2);
-
/* discard leading uint32 */
if (!buf_advance(buf, 4))
{
@@ -2593,7 +2470,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
}
/* get key method */
- key_method_flags = buf_read_u8(buf);
+ int key_method_flags = buf_read_u8(buf);
if ((key_method_flags & KEY_METHOD_MASK) != 2)
{
msg(D_TLS_ERRORS,
@@ -3003,23 +2880,9 @@ tls_process(struct tls_multi *multi,
if (!buf->len && ((ks->state == S_START && !session->opt->server)
|| (ks->state == S_GOT_KEY && session->opt->server)))
{
- if (session->opt->key_method == 1)
- {
- if (!key_method_1_write(buf, session))
- {
- goto error;
- }
- }
- else if (session->opt->key_method == 2)
- {
- if (!key_method_2_write(buf, session))
- {
- goto error;
- }
- }
- else
+ if (!key_method_2_write(buf, session))
{
- ASSERT(0);
+ goto error;
}
state_change = true;
@@ -3033,23 +2896,9 @@ tls_process(struct tls_multi *multi,
&& ((ks->state == S_SENT_KEY && !session->opt->server)
|| (ks->state == S_START && session->opt->server)))
{
- if (session->opt->key_method == 1)
- {
- if (!key_method_1_read(buf, session))
- {
- goto error;
- }
- }
- else if (session->opt->key_method == 2)
- {
- if (!key_method_2_read(buf, multi, session))
- {
- goto error;
- }
- }
- else
+ if (!key_method_2_read(buf, multi, session))
{
- ASSERT(0);
+ goto error;
}
state_change = true;
@@ -3463,6 +3312,11 @@ tls_pre_decrypt(struct tls_multi *multi,
/* verify legal opcode */
if (op < P_FIRST_OPCODE || op > P_LAST_OPCODE)
{
+ if (op == P_CONTROL_HARD_RESET_CLIENT_V1
+ || op == P_CONTROL_HARD_RESET_SERVER_V1)
+ {
+ msg(D_TLS_ERRORS, "Peer tried unsupported key-method 1");
+ }
msg(D_TLS_ERRORS,
"TLS Error: unknown opcode received from %s op=%d",
print_link_socket_actual(from, &gc), op);
@@ -3470,14 +3324,12 @@ tls_pre_decrypt(struct tls_multi *multi,
}
/* hard reset ? */
- if (is_hard_reset(op, 0))
+ if (is_hard_reset_method2(op))
{
/* verify client -> server or server -> client connection */
- if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
- || op == P_CONTROL_HARD_RESET_CLIENT_V2
+ if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
|| op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
- || ((op == P_CONTROL_HARD_RESET_SERVER_V1
- || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
+ || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
{
msg(D_TLS_ERRORS,
"TLS Error: client->client or server->server connection attempted from %s",
@@ -3539,22 +3391,14 @@ tls_pre_decrypt(struct tls_multi *multi,
}
/*
- * Initial packet received.
+ * Hard reset and session id does not match any session in
+ * multi->session: Possible initial packet
*/
-
- if (i == TM_SIZE && is_hard_reset(op, 0))
+ if (i == TM_SIZE && is_hard_reset_method2(op))
{
struct tls_session *session = &multi->session[TM_ACTIVE];
struct key_state *ks = &session->key[KS_PRIMARY];
- if (!is_hard_reset(op, multi->opt.key_method))
- {
- msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
- multi->opt.key_method,
- packet_opcode_name(op));
- goto error;
- }
-
/*
* If we have no session currently in progress, the initial packet will
* open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
@@ -3594,7 +3438,11 @@ tls_pre_decrypt(struct tls_multi *multi,
}
}
- if (i == TM_SIZE && is_hard_reset(op, 0))
+ /*
+ * If we detected new session in the last if block, i has
+ * changed to TM_ACTIVE, so check the condition again.
+ */
+ if (i == TM_SIZE && is_hard_reset_method2(op))
{
/*
* No match with existing sessions,
@@ -3614,14 +3462,6 @@ tls_pre_decrypt(struct tls_multi *multi,
goto error;
}
- if (!is_hard_reset(op, multi->opt.key_method))
- {
- msg(D_TLS_ERRORS, "TLS ERROR: new session local/remote key_method mismatch, local key_method=%d, op=%s",
- multi->opt.key_method,
- packet_opcode_name(op));
- goto error;
- }
-
if (!read_control_auth(buf, &session->tls_wrap, from,
session->opt))
{
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index 81f8810e..4f4f4bd5 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -66,8 +66,10 @@
/* indicates key_method >= 2 and client-specific tls-crypt key */
#define P_CONTROL_HARD_RESET_CLIENT_V3 10 /* initial key from client, forget previous state */
-/* define the range of legal opcodes */
-#define P_FIRST_OPCODE 1
+/* define the range of legal opcodes
+ * Since we do no longer support key-method 1 we consider
+ * the v1 op codes invalid */
+#define P_FIRST_OPCODE 3
#define P_LAST_OPCODE 10
/*
@@ -118,11 +120,7 @@
/* Default field in X509 to be username */
#define X509_USERNAME_FIELD_DEFAULT "CN"
-/*
- * Range of key exchange methods
- */
-#define KEY_METHOD_MIN 1
-#define KEY_METHOD_MAX 2
+#define KEY_METHOD_2 2
/* key method taken from lower 4 bits */
#define KEY_METHOD_MASK 0x0F
@@ -594,12 +592,11 @@ void show_tls_performance_stats(void);
void extract_x509_field_test(void);
/**
- * Given a key_method, return true if opcode represents the required form of
- * hard_reset.
+ * Given a key_method, return true if opcode represents the one of the
+ * hard_reset op codes for key-method 2
*
- * If key_method == 0, return true if any form of hard reset is used.
*/
-bool is_hard_reset(int op, int key_method);
+bool is_hard_reset_method2(int op);
void delayed_auth_pass_purge(void);
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index e0b3ee56..d904c31f 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -262,7 +262,6 @@ struct tls_options
#endif
/* from command line */
- int key_method;
bool replay;
bool single_session;
#ifdef ENABLE_OCC
--
2.26.2
|
|
From: tincanteksup <tin...@gm...> - 2020-07-21 10:40:08
|
1x typo
On 21/07/2020 11:01, Arne Schwabe wrote:
> Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients.
>
> Patch V2: Fix style. Make V1 op codes illegal, remove all code handling
> v1 op codes and give a good warning message if we encounter
> them in the legal op codes pre-check.
>
> Patch V3: Add a bit more comments in the existing methods.
>
> Signed-off-by: Arne Schwabe <ar...@rf...>
> ---
> doc/doxygen/doc_control_processor.h | 6 +-
> doc/doxygen/doc_key_generation.h | 6 +-
> doc/doxygen/doc_protocol_overview.h | 2 +-
> src/openvpn/forward.c | 2 +-
> src/openvpn/helper.c | 5 -
> src/openvpn/init.c | 1 -
> src/openvpn/options.c | 35 +---
> src/openvpn/options.h | 4 -
> src/openvpn/ssl.c | 240 +++++-----------------------
> src/openvpn/ssl.h | 19 +--
> src/openvpn/ssl_common.h | 1 -
> 11 files changed, 53 insertions(+), 268 deletions(-)
>
> diff --git a/doc/doxygen/doc_control_processor.h b/doc/doxygen/doc_control_processor.h
> index f87324cc..1bbf2d2d 100644
> --- a/doc/doxygen/doc_control_processor.h
> +++ b/doc/doxygen/doc_control_processor.h
> @@ -175,11 +175,7 @@
> * appropriate messages to be sent.
> *
> * @par Functions which control data channel key generation
> - * - Key method 1 key exchange functions:
> - * - \c key_method_1_write(), generates and processes key material to
> - * be sent to the remote OpenVPN peer.
> - * - \c key_method_1_read(), processes key material received from the
> - * remote OpenVPN peer.
> + * - Key method 1 key exchange functions were removed from OpenVPN 2.5
> * - Key method 2 key exchange functions:
> * - \c key_method_2_write(), generates and processes key material to
> * be sent to the remote OpenVPN peer.
> diff --git a/doc/doxygen/doc_key_generation.h b/doc/doxygen/doc_key_generation.h
> index efe61155..4bb9c708 100644
> --- a/doc/doxygen/doc_key_generation.h
> +++ b/doc/doxygen/doc_key_generation.h
> @@ -131,11 +131,7 @@ S_ACTIVE S_ACTIVE
> * control_processor Control Channel Processor module's\endlink \c
> * tls_process() function and control the %key generation and exchange
> * process as follows:
> - * - %Key method 1:
> - * - \c key_method_1_write(): generate random material locally, and load
> - * as "sending" keys.
> - * - \c key_method_1_read(): read random material received from remote
> - * peer, and load as "receiving" keys.
> + * - %Key method 1 has been removed in OpenVPN 2.5
> * - %Key method 2:
> * - \c key_method_2_write(): generate random material locally, and if
> * in server mode generate %key expansion.
> diff --git a/doc/doxygen/doc_protocol_overview.h b/doc/doxygen/doc_protocol_overview.h
> index 3f48b18a..08212223 100644
> --- a/doc/doxygen/doc_protocol_overview.h
> +++ b/doc/doxygen/doc_protocol_overview.h
> @@ -150,7 +150,7 @@
> *
> * @subsection network_protocol_control_plaintext Structure of plaintext control channel messages
> *
> - * - %Key method 1:
> + * - %Key method 1 (support removed in OpenVPN 2.5):
> * - Cipher %key length in bytes (1 byte).
> * - Cipher %key (n bytes).
> * - HMAC %key length in bytes (1 byte).
> diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c
> index 5c4370a8..698451d1 100644
> --- a/src/openvpn/forward.c
> +++ b/src/openvpn/forward.c
> @@ -1100,7 +1100,7 @@ process_incoming_link_part1(struct context *c, struct link_socket_info *lsi, boo
> floated, &ad_start))
> {
> /* Restore pre-NCP frame parameters */
> - if (is_hard_reset(opcode, c->options.key_method))
> + if (is_hard_reset_method2(opcode))
> {
> c->c2.frame = c->c2.frame_initial;
> #ifdef ENABLE_FRAGMENT
> diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
> index 6e9cc63c..a1d03070 100644
> --- a/src/openvpn/helper.c
> +++ b/src/openvpn/helper.c
> @@ -490,11 +490,6 @@ helper_client_server(struct options *o)
> */
> if (o->client)
> {
> - if (o->key_method != 2)
> - {
> - msg(M_USAGE, "--client requires --key-method 2");
> - }
> -
> o->pull = true;
> o->tls_client = true;
> }
> diff --git a/src/openvpn/init.c b/src/openvpn/init.c
> index e9c01629..b96d1471 100644
> --- a/src/openvpn/init.c
> +++ b/src/openvpn/init.c
> @@ -2852,7 +2852,6 @@ do_init_crypto_tls(struct context *c, const unsigned int flags)
> to.ssl_ctx = c->c1.ks.ssl_ctx;
> to.key_type = c->c1.ks.key_type;
> to.server = options->tls_server;
> - to.key_method = options->key_method;
> to.replay = options->replay;
> to.replay_window = options->replay_window;
> to.replay_time = options->replay_time;
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 7da04b6f..14d4c911 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -881,7 +881,6 @@ init_options(struct options *o, const bool init_gc)
> #ifdef ENABLE_PREDICTION_RESISTANCE
> o->use_prediction_resistance = false;
> #endif
> - o->key_method = 2;
> o->tls_timeout = 2;
> o->renegotiate_bytes = -1;
> o->renegotiate_seconds = 3600;
> @@ -1719,7 +1718,6 @@ show_settings(const struct options *o)
>
> SHOW_BOOL(tls_server);
> SHOW_BOOL(tls_client);
> - SHOW_INT(key_method);
> SHOW_STR_INLINE(ca_file);
> SHOW_STR(ca_path);
> SHOW_STR(dh_file);
> @@ -2380,10 +2378,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
> {
> msg(M_USAGE, "--ccd-exclusive must be used with --client-config-dir");
> }
> - if (options->key_method != 2)
> - {
> - msg(M_USAGE, "--mode server requires --key-method 2");
> - }
> if (options->auth_token_generate && !options->renegotiate_seconds)
> {
> msg(M_USAGE, "--auth-gen-token needs a non-infinite "
> @@ -2550,13 +2544,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
> "may accept clients which do not present a certificate");
> }
>
> - if (options->key_method == 1)
> - {
> - msg(M_WARN, "WARNING: --key-method 1 is deprecated and will be removed "
> - "in OpenVPN 2.5. By default --key-method 2 will be used if not set "
> - "in the configuration file, which is the recommended approach.");
> - }
> -
> const int tls_version_max =
> (options->ssl_flags >> SSLF_TLS_VERSION_MAX_SHIFT)
> & SSLF_TLS_VERSION_MAX_MASK;
> @@ -2798,7 +2785,6 @@ options_postprocess_verify_ce(const struct options *options, const struct connec
> MUST_BE_UNDEF(push_peer_info);
> MUST_BE_UNDEF(tls_exit);
> MUST_BE_UNDEF(crl_file);
> - MUST_BE_UNDEF(key_method);
> MUST_BE_UNDEF(ns_cert_type);
> MUST_BE_UNDEF(remote_cert_ku[0]);
> MUST_BE_UNDEF(remote_cert_eku);
> @@ -3827,10 +3813,7 @@ options_string(const struct options *o,
> * tls-auth/tls-crypt does not match. Removing tls-auth here would
> * break stuff, so leaving that in place. */
>
> - if (o->key_method > 1)
> - {
> - buf_printf(&out, ",key-method %d", o->key_method);
> - }
> + buf_printf(&out, ",key-method %d", KEY_METHOD_2);
> }
>
> if (remote)
> @@ -8476,22 +8459,6 @@ add_option(struct options *options,
> VERIFY_PERMISSION(OPT_P_GENERAL);
> options->tls_crypt_v2_verify_script = p[1];
> }
> - else if (streq(p[0], "key-method") && p[1] && !p[2])
> - {
> - int key_method;
> -
> - VERIFY_PERMISSION(OPT_P_GENERAL);
> - key_method = atoi(p[1]);
> - if (key_method < KEY_METHOD_MIN || key_method > KEY_METHOD_MAX)
> - {
> - msg(msglevel, "key_method parameter (%d) must be >= %d and <= %d",
> - key_method,
> - KEY_METHOD_MIN,
> - KEY_METHOD_MAX);
> - goto err;
> - }
> - options->key_method = key_method;
> - }
> else if (streq(p[0], "x509-track") && p[1] && !p[2])
> {
> VERIFY_PERMISSION(OPT_P_GENERAL);
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 1b038c91..3546bab3 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -572,10 +572,6 @@ struct options
> #ifdef ENABLE_CRYPTOAPI
> const char *cryptoapi_cert;
> #endif
> -
> - /* data channel key exchange method */
> - int key_method;
> -
> /* Per-packet timeout on control channel */
> int tls_timeout;
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 00b97352..ed35f792 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -861,23 +861,12 @@ print_key_id(struct tls_multi *multi, struct gc_arena *gc)
> }
>
> bool
> -is_hard_reset(int op, int key_method)
> +is_hard_reset_method2(int op)
> {
> - if (!key_method || key_method == 1)
> + if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
> + || op == P_CONTROL_HARD_RESET_CLIENT_V3)
> {
> - if (op == P_CONTROL_HARD_RESET_CLIENT_V1 || op == P_CONTROL_HARD_RESET_SERVER_V1)
> - {
> - return true;
> - }
> - }
> -
> - if (!key_method || key_method >= 2)
> - {
> - if (op == P_CONTROL_HARD_RESET_CLIENT_V2 || op == P_CONTROL_HARD_RESET_SERVER_V2
> - || op == P_CONTROL_HARD_RESET_CLIENT_V3)
> - {
> - return true;
> - }
> + return true;
> }
>
> return false;
> @@ -1097,23 +1086,14 @@ tls_session_init(struct tls_multi *multi, struct tls_session *session)
> }
>
> /* Are we a TLS server or client? */
> - ASSERT(session->opt->key_method >= 1);
> - if (session->opt->key_method == 1)
> + if (session->opt->server)
> {
> - session->initial_opcode = session->opt->server ?
> - P_CONTROL_HARD_RESET_SERVER_V1 : P_CONTROL_HARD_RESET_CLIENT_V1;
> + session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
> }
> - else /* session->opt->key_method >= 2 */
> + else
> {
> - if (session->opt->server)
> - {
> - session->initial_opcode = P_CONTROL_HARD_RESET_SERVER_V2;
> - }
> - else
> - {
> - session->initial_opcode = session->opt->tls_crypt_v2 ?
> - P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
> - }
> + session->initial_opcode = session->opt->tls_crypt_v2 ?
> + P_CONTROL_HARD_RESET_CLIENT_V3 : P_CONTROL_HARD_RESET_CLIENT_V2;
> }
>
> /* Initialize control channel authentication parameters */
> @@ -2225,52 +2205,6 @@ read_string_alloc(struct buffer *buf)
> return str;
> }
>
> -/*
> - * Handle the reading and writing of key data to and from
> - * the TLS control channel (cleartext).
> - */
> -
> -static bool
> -key_method_1_write(struct buffer *buf, struct tls_session *session)
> -{
> - struct key key;
> - struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
> -
> - ASSERT(session->opt->key_method == 1);
> - ASSERT(buf_init(buf, 0));
> -
> - generate_key_random(&key, &session->opt->key_type);
> - if (!check_key(&key, &session->opt->key_type))
> - {
> - msg(D_TLS_ERRORS, "TLS Error: Bad encrypting key generated");
> - return false;
> - }
> -
> - if (!write_key(&key, &session->opt->key_type, buf))
> - {
> - msg(D_TLS_ERRORS, "TLS Error: write_key failed");
> - return false;
> - }
> -
> - init_key_ctx(&ks->crypto_options.key_ctx_bi.encrypt, &key,
> - &session->opt->key_type, OPENVPN_OP_ENCRYPT,
> - "Data Channel Encrypt");
> - secure_memzero(&key, sizeof(key));
> -
> - /* send local options string */
> - {
> - const char *local_options = local_options_string(session);
> - const int optlen = strlen(local_options) + 1;
> - if (!buf_write(buf, local_options, optlen))
> - {
> - msg(D_TLS_ERRORS, "TLS Error: KM1 write options failed");
> - return false;
> - }
> - }
> -
> - return true;
> -}
> -
> static bool
> push_peer_info(struct buffer *buf, struct tls_session *session)
> {
> @@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session *session)
> * push request, also signal that the client wants
> * to get push-reply messages without without requiring a round
> * trip for a push request message*/
> - if(session->opt->pull)
> + if (session->opt->pull)
> {
> iv_proto |= IV_PROTO_REQUEST_PUSH;
> }
> @@ -2389,12 +2323,15 @@ error:
> return ret;
> }
>
> +/**
> + * Handle the writing of key data, peer-info, username/password, OCC
> + * to the TLS control channel (cleartext).
> + */
> static bool
> key_method_2_write(struct buffer *buf, struct tls_session *session)
> {
> struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
>
> - ASSERT(session->opt->key_method == 2);
> ASSERT(buf_init(buf, 0));
>
> /* write a uint32 0 */
> @@ -2404,7 +2341,7 @@ key_method_2_write(struct buffer *buf, struct tls_session *session)
> }
>
> /* write key_method + flags */
> - if (!buf_write_u8(buf, (session->opt->key_method & KEY_METHOD_MASK)))
> + if (!buf_write_u8(buf, KEY_METHOD_2))
> {
> goto error;
> }
> @@ -2506,73 +2443,15 @@ error:
> return false;
> }
>
> -static bool
> -key_method_1_read(struct buffer *buf, struct tls_session *session)
> -{
> - int status;
> - struct key key;
> - struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
> -
> - ASSERT(session->opt->key_method == 1);
> -
> - if (!session->verified)
> - {
> - msg(D_TLS_ERRORS,
> - "TLS Error: Certificate verification failed (key-method 1)");
> - goto error;
> - }
> -
> - status = read_key(&key, &session->opt->key_type, buf);
> - if (status != 1)
> - {
> - msg(D_TLS_ERRORS,
> - "TLS Error: Error reading data channel key from plaintext buffer");
> - goto error;
> - }
> -
> - if (!check_key(&key, &session->opt->key_type))
> - {
> - msg(D_TLS_ERRORS, "TLS Error: Bad decrypting key received from peer");
> - goto error;
> - }
> -
> - if (buf->len < 1)
> - {
> - msg(D_TLS_ERRORS, "TLS Error: Missing options string");
> - goto error;
> - }
> -
> -#ifdef ENABLE_OCC
> - /* compare received remote options string
> - * with our locally computed options string */
> - if (!session->opt->disable_occ
> - && !options_cmp_equal_safe((char *) BPTR(buf), session->opt->remote_options, buf->len))
> - {
> - options_warning_safe((char *) BPTR(buf), session->opt->remote_options, buf->len);
> - }
> -#endif
> -
> - buf_clear(buf);
> -
> - init_key_ctx(&ks->crypto_options.key_ctx_bi.decrypt, &key,
> - &session->opt->key_type, OPENVPN_OP_DECRYPT,
> - "Data Channel Decrypt");
> - secure_memzero(&key, sizeof(key));
> - ks->authenticated = KS_AUTH_TRUE;
> - return true;
> -
> -error:
> - buf_clear(buf);
> - secure_memzero(&key, sizeof(key));
> - return false;
> -}
> -
> +/**
> + * Handle reading key data, peer-info, username/password, OCC
> + * from the TLS control channel (cleartext).
> + */
> static bool
> key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_session *session)
> {
> struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */
>
> - int key_method_flags;
> bool username_status, password_status;
>
> struct gc_arena gc = gc_new();
> @@ -2582,8 +2461,6 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
> /* allocate temporary objects */
> ALLOC_ARRAY_CLEAR_GC(options, char, TLS_OPTIONS_LEN, &gc);
>
> - ASSERT(session->opt->key_method == 2);
> -
> /* discard leading uint32 */
> if (!buf_advance(buf, 4))
> {
> @@ -2593,7 +2470,7 @@ key_method_2_read(struct buffer *buf, struct tls_multi *multi, struct tls_sessio
> }
>
> /* get key method */
> - key_method_flags = buf_read_u8(buf);
> + int key_method_flags = buf_read_u8(buf);
> if ((key_method_flags & KEY_METHOD_MASK) != 2)
> {
> msg(D_TLS_ERRORS,
> @@ -3003,23 +2880,9 @@ tls_process(struct tls_multi *multi,
> if (!buf->len && ((ks->state == S_START && !session->opt->server)
> || (ks->state == S_GOT_KEY && session->opt->server)))
> {
> - if (session->opt->key_method == 1)
> - {
> - if (!key_method_1_write(buf, session))
> - {
> - goto error;
> - }
> - }
> - else if (session->opt->key_method == 2)
> - {
> - if (!key_method_2_write(buf, session))
> - {
> - goto error;
> - }
> - }
> - else
> + if (!key_method_2_write(buf, session))
> {
> - ASSERT(0);
> + goto error;
> }
>
> state_change = true;
> @@ -3033,23 +2896,9 @@ tls_process(struct tls_multi *multi,
> && ((ks->state == S_SENT_KEY && !session->opt->server)
> || (ks->state == S_START && session->opt->server)))
> {
> - if (session->opt->key_method == 1)
> - {
> - if (!key_method_1_read(buf, session))
> - {
> - goto error;
> - }
> - }
> - else if (session->opt->key_method == 2)
> - {
> - if (!key_method_2_read(buf, multi, session))
> - {
> - goto error;
> - }
> - }
> - else
> + if (!key_method_2_read(buf, multi, session))
> {
> - ASSERT(0);
> + goto error;
> }
>
> state_change = true;
> @@ -3463,6 +3312,11 @@ tls_pre_decrypt(struct tls_multi *multi,
> /* verify legal opcode */
> if (op < P_FIRST_OPCODE || op > P_LAST_OPCODE)
> {
> + if (op == P_CONTROL_HARD_RESET_CLIENT_V1
> + || op == P_CONTROL_HARD_RESET_SERVER_V1)
> + {
> + msg(D_TLS_ERRORS, "Peer tried unsupported key-method 1");
> + }
> msg(D_TLS_ERRORS,
> "TLS Error: unknown opcode received from %s op=%d",
> print_link_socket_actual(from, &gc), op);
> @@ -3470,14 +3324,12 @@ tls_pre_decrypt(struct tls_multi *multi,
> }
>
> /* hard reset ? */
> - if (is_hard_reset(op, 0))
> + if (is_hard_reset_method2(op))
> {
> /* verify client -> server or server -> client connection */
> - if (((op == P_CONTROL_HARD_RESET_CLIENT_V1
> - || op == P_CONTROL_HARD_RESET_CLIENT_V2
> + if (((op == P_CONTROL_HARD_RESET_CLIENT_V2
> || op == P_CONTROL_HARD_RESET_CLIENT_V3) && !multi->opt.server)
> - || ((op == P_CONTROL_HARD_RESET_SERVER_V1
> - || op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
> + || ((op == P_CONTROL_HARD_RESET_SERVER_V2) && multi->opt.server))
> {
> msg(D_TLS_ERRORS,
> "TLS Error: client->client or server->server connection attempted from %s",
> @@ -3539,22 +3391,14 @@ tls_pre_decrypt(struct tls_multi *multi,
> }
>
> /*
> - * Initial packet received.
> + * Hard reset and session id does not match any session in
> + * multi->session: Possible initial packet
> */
> -
> - if (i == TM_SIZE && is_hard_reset(op, 0))
> + if (i == TM_SIZE && is_hard_reset_method2(op))
> {
> struct tls_session *session = &multi->session[TM_ACTIVE];
> struct key_state *ks = &session->key[KS_PRIMARY];
>
> - if (!is_hard_reset(op, multi->opt.key_method))
> - {
> - msg(D_TLS_ERRORS, "TLS ERROR: initial packet local/remote key_method mismatch, local key_method=%d, op=%s",
> - multi->opt.key_method,
> - packet_opcode_name(op));
> - goto error;
> - }
> -
> /*
> * If we have no session currently in progress, the initial packet will
> * open a new session in TM_ACTIVE rather than TM_UNTRUSTED.
> @@ -3594,7 +3438,11 @@ tls_pre_decrypt(struct tls_multi *multi,
> }
> }
>
> - if (i == TM_SIZE && is_hard_reset(op, 0))
> + /*
> + * If we detected new session in the last if block, i has
block, i has ->
block, it has
> + * changed to TM_ACTIVE, so check the condition again.
> + */
> + if (i == TM_SIZE && is_hard_reset_method2(op))
> {
> /*
> * No match with existing sessions,
> @@ -3614,14 +3462,6 @@ tls_pre_decrypt(struct tls_multi *multi,
> goto error;
> }
>
> - if (!is_hard_reset(op, multi->opt.key_method))
> - {
> - msg(D_TLS_ERRORS, "TLS ERROR: new session local/remote key_method mismatch, local key_method=%d, op=%s",
> - multi->opt.key_method,
> - packet_opcode_name(op));
> - goto error;
> - }
> -
> if (!read_control_auth(buf, &session->tls_wrap, from,
> session->opt))
> {
> diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
> index 81f8810e..4f4f4bd5 100644
> --- a/src/openvpn/ssl.h
> +++ b/src/openvpn/ssl.h
> @@ -66,8 +66,10 @@
> /* indicates key_method >= 2 and client-specific tls-crypt key */
> #define P_CONTROL_HARD_RESET_CLIENT_V3 10 /* initial key from client, forget previous state */
>
> -/* define the range of legal opcodes */
> -#define P_FIRST_OPCODE 1
> +/* define the range of legal opcodes
> + * Since we do no longer support key-method 1 we consider
> + * the v1 op codes invalid */
> +#define P_FIRST_OPCODE 3
> #define P_LAST_OPCODE 10
>
> /*
> @@ -118,11 +120,7 @@
> /* Default field in X509 to be username */
> #define X509_USERNAME_FIELD_DEFAULT "CN"
>
> -/*
> - * Range of key exchange methods
> - */
> -#define KEY_METHOD_MIN 1
> -#define KEY_METHOD_MAX 2
> +#define KEY_METHOD_2 2
>
> /* key method taken from lower 4 bits */
> #define KEY_METHOD_MASK 0x0F
> @@ -594,12 +592,11 @@ void show_tls_performance_stats(void);
> void extract_x509_field_test(void);
>
> /**
> - * Given a key_method, return true if opcode represents the required form of
> - * hard_reset.
> + * Given a key_method, return true if opcode represents the one of the
> + * hard_reset op codes for key-method 2
> *
> - * If key_method == 0, return true if any form of hard reset is used.
> */
> -bool is_hard_reset(int op, int key_method);
> +bool is_hard_reset_method2(int op);
>
> void delayed_auth_pass_purge(void);
>
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index e0b3ee56..d904c31f 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -262,7 +262,6 @@ struct tls_options
> #endif
>
> /* from command line */
> - int key_method;
> bool replay;
> bool single_session;
> #ifdef ENABLE_OCC
>
|
|
From: David S. <op...@sf...> - 2020-07-21 17:27:07
|
On 21/07/2020 12:01, Arne Schwabe wrote: > Key-method 1 is only needed to talk to pre OpenVPN 2.0 clients. > > Patch V2: Fix style. Make V1 op codes illegal, remove all code handling > v1 op codes and give a good warning message if we encounter > them in the legal op codes pre-check. > > Patch V3: Add a bit more comments in the existing methods. > > Signed-off-by: Arne Schwabe <ar...@rf...> > --- > doc/doxygen/doc_control_processor.h | 6 +- > doc/doxygen/doc_key_generation.h | 6 +- > doc/doxygen/doc_protocol_overview.h | 2 +- > src/openvpn/forward.c | 2 +- > src/openvpn/helper.c | 5 - > src/openvpn/init.c | 1 - > src/openvpn/options.c | 35 +--- > src/openvpn/options.h | 4 - > src/openvpn/ssl.c | 240 +++++----------------------- > src/openvpn/ssl.h | 19 +-- > src/openvpn/ssl_common.h | 1 - > 11 files changed, 53 insertions(+), 268 deletions(-) > This LGTM now. Thanks for the updates and adjustments! I've done light local build testing (applying just this patch on git master commit 08469ca1eccc). Builds fine, 'make check' looks good. Acked-By: David Sommerseth <da...@op...> -- kind regards, David Sommerseth OpenVPN Inc |
|
From: Gert D. <ge...@gr...> - 2020-07-21 19:41:40
|
Your patch has been applied to the master branch.
I have run a t_client test on FreeBSD/OpenSSL and Linux/mbedTLS, and
a full server side test. Just to be sure. This is surprisingly large
changes in crypto code... the changes look good, but...!
All tests pass :-)
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 5 5a 5b 5c 5v1 5v2 5v3 5w1 5w2 5w3 5w4 5x1 5x2 5x3 5x4 6 7 7x 8 8a 9 2f 4b.
Test sets failed: none.
I have ignored that hunk from the patch:
@@ -2312,7 +2246,7 @@ push_peer_info(struct buffer *buf, struct tls_session
+*session)
* push request, also signal that the client wants
* to get push-reply messages without without requiring a round
* trip for a push request message*/
- if(session->opt->pull)
+ if (session->opt->pull)
{
iv_proto |= IV_PROTO_REQUEST_PUSH;
}
because it fixes whitespace in code that is not there yet - so I'll
fix the whitespace on the fly when applying *that* patch (if we're
not at v8 by then, with the whitespace fix included).
commit 36bef1b52b49ebbc3790635be230e2f30f0532a7
Author: Arne Schwabe
Date: Tue Jul 21 12:01:28 2020 +0200
Remove key-method 1
Signed-off-by: Arne Schwabe <ar...@rf...>
Acked-by: David Sommerseth <da...@op...>
Message-Id: <202...@rf...>
URL: https://www.mail-archive.com/ope...@li.../msg20516.html
Signed-off-by: Gert Doering <ge...@gr...>
--
kind regards,
Gert Doering
|
Thanks for helping keep SourceForge clean.
X