openvpn-devel Mailing List for OpenVPN (Page 11)

Hi, On 03/03/2023 12:27, Antonio Quartulli wrote: > Hi, > > On 03/03/2023 12:05, Kristof Provost via Openvpn-devel wrote: >> From: Kristof Provost <kp...@Fr...> >> >> FreeBSD's if_ovpn will never emit this as a peer deletion reason >> (because it doesn't support TCP), but this allows us to align the >> defines between Linux and FreeBSD, and remove a Linux-specific case from >> process_incoming_del_peer(). > > SoB missing Sorry for being very condensed, however the patch looks good and gets my ACK: Acked-by: Antonio Quartulli <a...@un...> I normally put my ACK under the SoB, so when I couldn't find it, my brain just threw that exception :-P Cheers, > >> --- >>   src/openvpn/dco_freebsd.h | 1 + >>   src/openvpn/multi.c       | 3 --- >>   2 files changed, 1 insertion(+), 3 deletions(-) >> >> diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h >> index 2e35f3ac..970beca0 100644 >> --- a/src/openvpn/dco_freebsd.h >> +++ b/src/openvpn/dco_freebsd.h >> @@ -41,6 +41,7 @@ enum ovpn_del_reason_t { >>       OVPN_DEL_PEER_REASON_EXPIRED, >>       OVPN_DEL_PEER_REASON_TRANSPORT_ERROR, >>       OVPN_DEL_PEER_REASON_USERSPACE, >> +    OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT, >>   }; >>   typedef struct dco_context { >> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c >> index f2559016..99123c39 100644 >> --- a/src/openvpn/multi.c >> +++ b/src/openvpn/multi.c >> @@ -3244,12 +3244,9 @@ process_incoming_del_peer(struct multi_context >> *m, struct multi_instance *mi, >>               reason = "ovpn-dco: transport error"; >>               break; >> -#ifdef TARGET_LINUX >> -        /* FIXME: this is linux-only today and breaks FreeBSD >> compilation */ >>           case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: >>               reason = "ovpn-dco: transport disconnected"; >>               break; >> -#endif >>           case OVPN_DEL_PEER_REASON_USERSPACE: >>               /* We assume that is ourselves. Unfortunately, sometimes >> these > -- Antonio Quartulli 

Hi, On 03/03/2023 12:05, Kristof Provost via Openvpn-devel wrote: > From: Kristof Provost <kp...@Fr...> > > FreeBSD's if_ovpn will never emit this as a peer deletion reason > (because it doesn't support TCP), but this allows us to align the > defines between Linux and FreeBSD, and remove a Linux-specific case from > process_incoming_del_peer(). SoB missing > --- > src/openvpn/dco_freebsd.h | 1 + > src/openvpn/multi.c | 3 --- > 2 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h > index 2e35f3ac..970beca0 100644 > --- a/src/openvpn/dco_freebsd.h > +++ b/src/openvpn/dco_freebsd.h > @@ -41,6 +41,7 @@ enum ovpn_del_reason_t { > OVPN_DEL_PEER_REASON_EXPIRED, > OVPN_DEL_PEER_REASON_TRANSPORT_ERROR, > OVPN_DEL_PEER_REASON_USERSPACE, > + OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT, > }; > > typedef struct dco_context { > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index f2559016..99123c39 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -3244,12 +3244,9 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, > reason = "ovpn-dco: transport error"; > break; > > -#ifdef TARGET_LINUX > - /* FIXME: this is linux-only today and breaks FreeBSD compilation */ > case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: > reason = "ovpn-dco: transport disconnected"; > break; > -#endif > > case OVPN_DEL_PEER_REASON_USERSPACE: > /* We assume that is ourselves. Unfortunately, sometimes these -- Antonio Quartulli 

From: Kristof Provost <kp...@Fr...> FreeBSD's if_ovpn will never emit this as a peer deletion reason (because it doesn't support TCP), but this allows us to align the defines between Linux and FreeBSD, and remove a Linux-specific case from process_incoming_del_peer(). --- src/openvpn/dco_freebsd.h | 1 + src/openvpn/multi.c | 3 --- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/openvpn/dco_freebsd.h b/src/openvpn/dco_freebsd.h index 2e35f3ac..970beca0 100644 --- a/src/openvpn/dco_freebsd.h +++ b/src/openvpn/dco_freebsd.h @@ -41,6 +41,7 @@ enum ovpn_del_reason_t { OVPN_DEL_PEER_REASON_EXPIRED, OVPN_DEL_PEER_REASON_TRANSPORT_ERROR, OVPN_DEL_PEER_REASON_USERSPACE, + OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT, }; typedef struct dco_context { diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f2559016..99123c39 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -3244,12 +3244,9 @@ process_incoming_del_peer(struct multi_context *m, struct multi_instance *mi, reason = "ovpn-dco: transport error"; break; -#ifdef TARGET_LINUX - /* FIXME: this is linux-only today and breaks FreeBSD compilation */ case OVPN_DEL_PEER_REASON_TRANSPORT_DISCONNECT: reason = "ovpn-dco: transport disconnected"; break; -#endif case OVPN_DEL_PEER_REASON_USERSPACE: /* We assume that is ourselves. Unfortunately, sometimes these -- 2.39.2 

On 28/02/2023 05:41, Heiko Hund wrote: > No DNS resolver currently supports this and it is not possible to > emulate the behavior without the chance of errors. Finding the > effective default system DNS server(s) to specify the exclude > DNS routes is not trivial and cannot be verified to be correct > without resolver internal knowledge. So, it is better to not > support this instead of supporting it, but incorrectly. > > Signed-off-by: Heiko Hund <he...@is...> > --- > doc/man-sections/client-options.rst | 14 +++++--------- > src/openvpn/dns.c | 13 ++----------- > src/openvpn/dns.h | 7 ------- > src/openvpn/options.c | 16 ---------------- > 4 files changed, 7 insertions(+), 43 deletions(-) I've only glared at the code and quickly done a few compile tests. LGTM. Change itself also makes sense. Acked-By: David Sommerseth <da...@op...> -- kind regards, David Sommerseth OpenVPN Inc 

Acked-by: Gert Doering <ge...@gr...> Thanks for spotting & fixing this, and apologies for still not having a FreeBSD 14 buildslave. Indeed, the existing configure.ac hard breaks my FreeBSD 14 setup (which I didn't look at for a while, "because it works")... checking for nvlist_create in -lnv... no configure: WARNING: Name/Value pair library not found. configure: error: DCO support can't be enabled (because "./configure --enable-dco" -> succeed or error out). With "enable DCO on auto", the existing code does checking for nvlist_create in -lnv... no configure: WARNING: Name/Value pair library not found. configure: WARNING: DCO support disabled and proceeds to build a binary with no DCO! With this patch applied, "no arguments" or "--enable-dco" both succeed... checking for net/if_ovpn.h... yes configure: Enabled ovpn-dco support for FreeBSD on earlier FreeBSD versions (7.4), it just disables DCO, and proceeds happily - as it should be. configure: WARNING: DCO header not found. configure: WARNING: DCO support disabled Your patch has been applied to the master and release/2.6 branch. commit 6f261673dee26ae8cfdf58f77038098d4f81d84a (master) commit 86fb085b6d2582916ef59b4bd8bd5e4a072964a3 (release/2.6) Author: Kristof Provost Date: Wed Mar 1 10:18:48 2023 +0100 configure: improve FreeBSD DCO check Signed-off-by: Kristof Provost <kpr...@ne...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@ne...> URL: https://www.mail-archive.com/ope...@li.../msg26314.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Acked-by: Gert Doering <ge...@gr...> Straightforward :-) - and we really shouldn't divide by zero.. I have adjusted the message to read "--fragment ..." (with dashes), because that's what we seem to do in other option-related error messages. Your patch has been applied to the master and release/2.6 branch. commit 78e504210add19343e65f5c5b80be9ea6e9e95ab (master) commit b9a9de156bc3ad517bfc6d1042ad0ef0350b638e (release/2.6) Author: Kristof Provost Date: Wed Mar 1 10:18:51 2023 +0100 options.c: enforce a minimal fragment size Signed-off-by: Kristof Provost <kpr...@ne...> Acked-by: Gert Doering <ge...@gr...> Message-Id: <202...@ne...> URL: https://www.mail-archive.com/ope...@li.../msg26313.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

The control messages coming from auth pending should always be on the session that triggered them (i.e. INITIAL or ACTIVE) and not always on the active session. Rework the code path that trigger those messsages from management and plugin/script to specify the TLS session. We only support the two TLS sessions that are supposed to be active. TLS sessions in any lame slot (TM_LAME or KS_LAME) are not considered to be candidates for sending messages as these slots only serve to keep key material around. Unfortunately, this fix requires the management interface to be changed to allow including the specific session the messages should to go to. As there are very few users of this interface with auth-pending, I made this a hard change instead of adding hacky workaround code that is not always working correctly anyway. send_control_channel_string will continue to only use the primary session and key but the current users of that (push replys and exit notification) already require the established session to be the active one, so there no changes needed at the moment. Signed-off-by: Arne Schwabe <ar...@rf...> --- Changes.rst | 3 +++ doc/management-notes.txt | 13 +++++++++---- src/openvpn/forward.c | 12 ++++++------ src/openvpn/forward.h | 11 ++++++----- src/openvpn/manage.c | 13 ++++++++----- src/openvpn/manage.h | 3 ++- src/openvpn/multi.c | 20 +++++++++++++++++++- src/openvpn/push.c | 27 +++++++++++++++++++-------- src/openvpn/push.h | 8 +++++--- src/openvpn/ssl_verify.c | 9 +++++---- 10 files changed, 82 insertions(+), 37 deletions(-) diff --git a/Changes.rst b/Changes.rst index c5335ce93..43f312fc6 100644 --- a/Changes.rst +++ b/Changes.rst @@ -223,6 +223,9 @@ User-visible Changes compatibility with older versions. See the manual page on the ``--compat-mode`` for details. +- The ``client-pending-auth`` management command now requires also the + key id. The management version has been changed to 5 to indicate this change. + Common errors with OpenSSL 3.0 and OpenVPN 2.6 ---------------------------------------------- Both OpenVPN 2.6 and OpenSSL 3.0 tighten the security considerable, so some diff --git a/doc/management-notes.txt b/doc/management-notes.txt index 34f301db7..5c51bc997 100644 --- a/doc/management-notes.txt +++ b/doc/management-notes.txt @@ -613,10 +613,10 @@ COMMAND -- client-pending-auth (OpenVPN 2.5 or higher) Instruct OpenVPN server to send AUTH_PENDING and INFO_PRE message to signal a pending authenticating to the client. A pending auth means -that the connecting requires extra authentication like a one time +that connecting requires extra authentication like a one time password or doing a single sign on via web. - client-pending-auth {CID} {EXTRA} {TIMEOUT} + client-pending-auth {CID} {KID} {EXTRA} {TIMEOUT} The server will send AUTH_PENDING and INFO_PRE,{EXTRA} to the client. If the client supports accepting keywords to AUTH_PENDING (announced via IV_PROTO), @@ -639,11 +639,16 @@ Both client and server limit the maximum timeout to the smaller value of half th For the format of {EXTRA} see below. For OpenVPN server this is a stateless operation and needs to be followed by a client-deny/client-auth[-nt] command -(that is the result of the out of band authentication). +(that is the result of the out-of-band authentication). + +Note that the {KID} argument has been added in management version 5 to +correctly allow specifing the pending client authentication the message is + +tying together this message strictly to the authentication Before issuing a client-pending-auth to a client instead of a client-auth/client-deny, the server should check the IV_SSO -environment variable for whether the method is supported. Currently +environment variable for whether the method is supported. Currently, defined methods are crtext for challenge/response using text (e.g., TOTP), openurl (deprecated) and webauth for opening a URL in the client to continue authentication. A client supporting webauth and diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 9bb099097..8fcd703c4 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -366,20 +366,20 @@ check_connection_established(struct context *c) } bool -send_control_channel_string_dowork(struct tls_multi *multi, +send_control_channel_string_dowork(struct tls_session *session, const char *str, int msglevel) { struct gc_arena gc = gc_new(); bool stat; - ASSERT(multi); - struct key_state *ks = get_key_scan(multi, 0); + ASSERT(session); + struct key_state *ks = &session->key[KS_PRIMARY]; /* buffered cleartext write onto TLS control channel */ stat = tls_send_payload(ks, (uint8_t *) str, strlen(str) + 1); msg(msglevel, "SENT CONTROL [%s]: '%s' (status=%d)", - tls_common_name(multi, false), + session->common_name ? session->common_name : "UNDEF", sanitize_control_message(str, &gc), (int) stat); @@ -399,8 +399,8 @@ send_control_channel_string(struct context *c, const char *str, int msglevel) { if (c->c2.tls_multi) { - bool ret = send_control_channel_string_dowork(c->c2.tls_multi, - str, msglevel); + struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; + bool ret = send_control_channel_string_dowork(session, str, msglevel); reschedule_multi_process(c); return ret; diff --git a/src/openvpn/forward.h b/src/openvpn/forward.h index 7376bca23..e19115ea1 100644 --- a/src/openvpn/forward.h +++ b/src/openvpn/forward.h @@ -265,21 +265,22 @@ send_control_channel_string(struct context *c, const char *str, int msglevel); /* * Send a string to remote over the TLS control channel. - * Used for push/pull messages, passing username/password, - * etc. + * Used for push/pull messages, auth pending and other clear text + * control messages. * * This variant does not schedule the actual sending of the message * The caller needs to ensure that it is scheduled or call * send_control_channel_string * - * @param multi - The tls_multi structure of the VPN tunnel associated - * with the packet. + * @param session - The session structure of the VPN tunnel associated + * with the packet. The method will always use the + * primary key (KS_PRIMARY) for sending the message * @param str - The message to be sent * @param msglevel - Message level to use for logging */ bool -send_control_channel_string_dowork(struct tls_multi *multi, +send_control_channel_string_dowork(struct tls_session *session, const char *str, int msglevel); diff --git a/src/openvpn/manage.c b/src/openvpn/manage.c index db88e3479..05358af45 100644 --- a/src/openvpn/manage.c +++ b/src/openvpn/manage.c @@ -1042,22 +1042,25 @@ parse_uint(const char *str, const char *what, unsigned int *uint) * * @param man The management interface struct * @param cid_str The CID in string form + * @param kid_str The key ID in string form * @param extra The string to be send to the client containing * the information of the additional steps */ static void man_client_pending_auth(struct management *man, const char *cid_str, - const char *extra, const char *timeout_str) + const char *kid_str, const char *extra, + const char *timeout_str) { unsigned long cid = 0; + unsigned int kid = 0; unsigned int timeout = 0; - if (parse_cid(cid_str, &cid) + if (parse_cid(cid_str, &cid) && parse_uint(kid_str, "KID", &kid) && parse_uint(timeout_str, "TIMEOUT", &timeout)) { if (man->persist.callback.client_pending_auth) { bool ret = (*man->persist.callback.client_pending_auth) - (man->persist.callback.arg, cid, extra, timeout); + (man->persist.callback.arg, cid, kid, extra, timeout); if (ret) { @@ -1594,9 +1597,9 @@ man_dispatch_command(struct management *man, struct status_output *so, const cha } else if (streq(p[0], "client-pending-auth")) { - if (man_need(man, p, 3, 0)) + if (man_need(man, p, 4, 0)) { - man_client_pending_auth(man, p[1], p[2], p[3]); + man_client_pending_auth(man, p[1], p[2], p[3], p[4]); } } else if (streq(p[0], "rsa-sig")) diff --git a/src/openvpn/manage.h b/src/openvpn/manage.h index 2ced90835..07317a402 100644 --- a/src/openvpn/manage.h +++ b/src/openvpn/manage.h @@ -52,7 +52,7 @@ #include "socket.h" #include "mroute.h" -#define MANAGEMENT_VERSION 4 +#define MANAGEMENT_VERSION 5 #define MANAGEMENT_N_PASSWORD_RETRIES 3 #define MANAGEMENT_LOG_HISTORY_INITIAL_SIZE 100 #define MANAGEMENT_ECHO_BUFFER_SIZE 100 @@ -194,6 +194,7 @@ struct management_callback struct buffer_list *cc_config); /* ownership transferred */ bool (*client_pending_auth) (void *arg, const unsigned long cid, + const unsigned int kid, const char *extra, unsigned int timeout); char *(*get_peer_info) (void *arg, const unsigned long cid); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index f25590168..14ec39dc5 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -4025,15 +4025,33 @@ management_kill_by_cid(void *arg, const unsigned long cid, const char *kill_msg) static bool management_client_pending_auth(void *arg, const unsigned long cid, + const unsigned int mda_key_id, const char *extra, unsigned int timeout) { struct multi_context *m = (struct multi_context *) arg; struct multi_instance *mi = lookup_by_cid(m, cid); + if (mi) { + struct tls_multi *multi = mi->context.c2.tls_multi; + struct tls_session *session; + + if (multi->session[TM_INITIAL].key[KS_PRIMARY].mda_key_id == mda_key_id) + { + session = &multi->session[TM_INITIAL]; + } + else if (multi->session[TM_ACTIVE].key[KS_PRIMARY].mda_key_id == mda_key_id) + { + session = &multi->session[TM_ACTIVE]; + } + else + { + return false; + } + /* sends INFO_PRE and AUTH_PENDING messages to client */ - bool ret = send_auth_pending_messages(mi->context.c2.tls_multi, extra, + bool ret = send_auth_pending_messages(multi, session, extra, timeout); reschedule_multi_process(&mi->context); multi_schedule_context_wakeup(m, mi); diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 4d64ad1af..3475cbda8 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -412,7 +412,16 @@ send_auth_failed(struct context *c, const char *client_reason) { buf_printf(&buf, ",%s", client_reason); } - send_control_channel_string(c, BSTR(&buf), D_PUSH); + + /* We kill the whole session, send the AUTH_FAILED to any TLS session + * that might be active */ + send_control_channel_string_dowork(&c->c2.tls_multi->session[TM_INITIAL], + BSTR(&buf), D_PUSH); + send_control_channel_string_dowork(&c->c2.tls_multi->session[TM_ACTIVE], + BSTR(&buf), D_PUSH); + + reschedule_multi_process(c); + } gc_free(&gc); @@ -420,10 +429,11 @@ send_auth_failed(struct context *c, const char *client_reason) bool -send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra, - unsigned int timeout) +send_auth_pending_messages(struct tls_multi *tls_multi, + struct tls_session *session, + const char *extra, unsigned int timeout) { - struct key_state *ks = get_key_scan(tls_multi, 0); + struct key_state *ks = &session->key[KS_PRIMARY]; static const char info_pre[] = "INFO_PRE,"; @@ -440,7 +450,7 @@ send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra, struct gc_arena gc = gc_new(); if ((proto & IV_PROTO_AUTH_PENDING_KW) == 0) { - send_control_channel_string_dowork(tls_multi, "AUTH_PENDING", D_PUSH); + send_control_channel_string_dowork(session, "AUTH_PENDING", D_PUSH); } else { @@ -451,7 +461,7 @@ send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra, struct buffer buf = alloc_buf_gc(len, &gc); buf_printf(&buf, auth_pre); buf_printf(&buf, "%u", timeout); - send_control_channel_string_dowork(tls_multi, BSTR(&buf), D_PUSH); + send_control_channel_string_dowork(session, BSTR(&buf), D_PUSH); } size_t len = strlen(extra) + 1 + sizeof(info_pre); @@ -464,7 +474,7 @@ send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra, struct buffer buf = alloc_buf_gc(len, &gc); buf_printf(&buf, info_pre); buf_printf(&buf, "%s", extra); - send_control_channel_string_dowork(tls_multi, BSTR(&buf), D_PUSH); + send_control_channel_string_dowork(session, BSTR(&buf), D_PUSH); ks->auth_deferred_expire = now + timeout; @@ -736,6 +746,7 @@ send_push_reply_auth_token(struct tls_multi *multi) { struct gc_arena gc = gc_new(); struct push_list push_list = { 0 }; + struct tls_session *session = &multi->session[TM_ACTIVE]; prepare_auth_token_push_reply(multi, &gc, &push_list); @@ -746,7 +757,7 @@ send_push_reply_auth_token(struct tls_multi *multi) /* Construct a mimimal control channel push reply message */ struct buffer buf = alloc_buf_gc(PUSH_BUNDLE_SIZE, &gc); buf_printf(&buf, "%s,%s", push_reply_cmd, e->option); - send_control_channel_string_dowork(multi, BSTR(&buf), D_PUSH); + send_control_channel_string_dowork(session, BSTR(&buf), D_PUSH); gc_free(&gc); } diff --git a/src/openvpn/push.h b/src/openvpn/push.h index 5e594a30a..f43ab0966 100644 --- a/src/openvpn/push.h +++ b/src/openvpn/push.h @@ -78,16 +78,18 @@ void send_auth_failed(struct context *c, const char *client_reason); * more details on message format */ bool -send_auth_pending_messages(struct tls_multi *tls_multi, const char *extra, +send_auth_pending_messages(struct tls_multi *tls_multi, + struct tls_session *session, const char *extra, unsigned int timeout); void send_restart(struct context *c, const char *kill_msg); /** * Sends a push reply message only containin the auth-token to update - * the auth-token on the client + * the auth-token on the client. Always pushes to the active session * - * @param multi - The tls_multi structure belonging to the instance to push to + * @param multi - The \c tls_multi structure belonging to the instance + * to push to */ void send_push_reply_auth_token(struct tls_multi *multi); diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 996aee01f..1b589f1a6 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -916,7 +916,8 @@ check_auth_pending_method(const char *peer_info, const char *method) */ static bool key_state_check_auth_pending_file(struct auth_deferred_status *ads, - struct tls_multi *multi) + struct tls_multi *multi, + struct tls_session *session) { bool ret = true; if (ads->auth_pending_file) @@ -965,7 +966,7 @@ key_state_check_auth_pending_file(struct auth_deferred_status *ads, } else { - send_auth_pending_messages(multi, BSTR(extra_buf), timeout); + send_auth_pending_messages(multi, session, BSTR(extra_buf), timeout); } } @@ -1390,7 +1391,7 @@ verify_user_pass_script(struct tls_session *session, struct tls_multi *multi, /* Check if we the plugin has written the pending auth control * file and send the pending auth to the client */ if (!key_state_check_auth_pending_file(&ks->script_auth, - multi)) + multi, session)) { retval = OPENVPN_PLUGIN_FUNC_ERROR; key_state_rm_auth_control_files(&ks->script_auth); @@ -1514,7 +1515,7 @@ verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi, { /* Check if the plugin has written the pending auth control * file and send the pending auth to the client */ - if (!key_state_check_auth_pending_file(&ks->plugin_auth, multi)) + if (!key_state_check_auth_pending_file(&ks->plugin_auth, multi, session)) { retval = OPENVPN_PLUGIN_FUNC_ERROR; } -- 2.37.1 (Apple Git-137.1) 

Currently, this function and other parts of OpenVPN assume that multi->session[TM_ACTIVE].key[KS_PRIMARY] is always the right session to send control message. This assumption was only achieve through complicated session moving and shuffling in our state machine in the past. The old logic basically also always assumed that control messages are always for fully authenticated clients. This assumption was never really true (see AUTH_FAILED message) but has been broken even more by auth-pending. Cleaning up the state machine transitions in 7dcde87b7a broke this assumption even more. This change now allows to specify the key_state/TLS session that is used to send the control message. Signed-off-by: Arne Schwabe <ar...@rf...> --- src/openvpn/forward.c | 5 ++++- src/openvpn/ssl.c | 7 ++----- src/openvpn/ssl.h | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/openvpn/forward.c b/src/openvpn/forward.c index 257c7c75c..9bb099097 100644 --- a/src/openvpn/forward.c +++ b/src/openvpn/forward.c @@ -372,8 +372,11 @@ send_control_channel_string_dowork(struct tls_multi *multi, struct gc_arena gc = gc_new(); bool stat; + ASSERT(multi); + struct key_state *ks = get_key_scan(multi, 0); + /* buffered cleartext write onto TLS control channel */ - stat = tls_send_payload(multi, (uint8_t *) str, strlen(str) + 1); + stat = tls_send_payload(ks, (uint8_t *) str, strlen(str) + 1); msg(msglevel, "SENT CONTROL [%s]: '%s' (status=%d)", tls_common_name(multi, false), diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index 016bdc57f..b84f23c62 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -3988,18 +3988,15 @@ tls_post_encrypt(struct tls_multi *multi, struct buffer *buf) */ bool -tls_send_payload(struct tls_multi *multi, +tls_send_payload(struct key_state *ks, const uint8_t *data, int size) { - struct key_state *ks; bool ret = false; tls_clear_error(); - ASSERT(multi); - - ks = get_key_scan(multi, 0); + ASSERT(ks); if (ks->state >= S_ACTIVE) { diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h index b0a2823fb..7ea13b920 100644 --- a/src/openvpn/ssl.h +++ b/src/openvpn/ssl.h @@ -414,7 +414,7 @@ void ssl_put_auth_challenge(const char *cr_str); /* * Send a payload over the TLS control channel */ -bool tls_send_payload(struct tls_multi *multi, +bool tls_send_payload(struct key_state *ks, const uint8_t *data, int size); -- 2.37.1 (Apple Git-137.1) 

Without this, we will caculate a pointer to the linksocket relative to a null pointer in get_link_socket_info, which itself does not crash and the pointer seems not to be accessed later, so we do not get a crash here. This is still not the correct behaviour and the undefined behaviour sanitiser from llvm/clang finds this. Change-Id: I82a20ac72f60f8770ea1b4ab0c8cdea31868abe7 Signed-off-by: Arne Schwabe <ar...@rf...> --- src/openvpn/init.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 622239f6b..e6f14f72d 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -4541,14 +4541,15 @@ init_instance(struct context *c, const struct env_set *env, const unsigned int f if (c->mode == CM_P2P || c->mode == CM_TOP || c->mode == CM_CHILD_TCP) { link_socket_init_phase2(c); - } - /* Update dynamic frame calculation as exact transport socket information - * (IP vs IPv6) may be only available after socket phase2 has finished. - * This is only needed for --static or no crypto, NCP will recalculate this - * in tls_session_update_crypto_params (P2MP) */ - frame_calculate_dynamic(&c->c2.frame, &c->c1.ks.key_type, &c->options, - get_link_socket_info(c)); + + /* Update dynamic frame calculation as exact transport socket information + * (IP vs IPv6) may be only available after socket phase2 has finished. + * This is only needed for --static or no crypto, NCP will recalculate this + * in tls_session_update_crypto_params (P2MP) */ + frame_calculate_dynamic(&c->c2.frame, &c->c1.ks.key_type, &c->options, + get_link_socket_info(c)); + } /* * Actually do UID/GID downgrade, and chroot, if requested. -- 2.37.1 (Apple Git-137.1) 

Explicitly say that the version specified is the one of the peer and not the version we try to emulate. Change-Id: I3bd27a8d34d8cb4896a3b78508b7d16911571543 Signed-off-by: Arne Schwabe <ar...@rf...> --- doc/man-sections/generic-options.rst | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/doc/man-sections/generic-options.rst b/doc/man-sections/generic-options.rst index c827651d6..ae20a261a 100644 --- a/doc/man-sections/generic-options.rst +++ b/doc/man-sections/generic-options.rst @@ -53,10 +53,16 @@ which mode OpenVPN is configured as. need for /dev/urandom to be available. --compat-mode version - This option provides a way to alter the default of OpenVPN to be more - compatible with the version ``version`` specified. All of the changes - this option does can also be achieved using individual configuration - options. + This option provides a convient way to alter the default of OpenVPN + to be more compatible with the version ``version`` specified. All of + the changes this option does can also be achieved using individual + configuration options. + + The version specified is the version of OpenVPN peer OepnVPN should try + to compatible with. In general OpenVPN should be compatible with the last + two previous version without this option. E.g. OpenVPN 2.6.0 should be + compatible with 2.5.x and 2.4.x without this option. However, there might + be some edge cases that still require this option even in these cases. Note: Using this option reverts defaults to no longer recommended values and should be avoided if possible. @@ -67,12 +73,15 @@ which mode OpenVPN is configured as. - 2.5.x or lower: ``--allow-compression asym`` is automatically added to the configuration if no other compression options are present. - 2.4.x or lower: The cipher in ``--cipher`` is appended to - ``--data-ciphers`` + ``--data-ciphers``. - 2.3.x or lower: ``--data-cipher-fallback`` is automatically added with - the same cipher as ``--cipher`` + the same cipher as ``--cipher``. - 2.3.6 or lower: ``--tls-version-min 1.0`` is added to the configuration when ``--tls-version-min`` is not explicitly set. + If not needed, this is option should be avoided. Setting this option can + lower security or disable features like data-channel offloading. + --config file Load additional config options from ``file`` where each line corresponds to one command line option, but with the leading :code:`--` removed. -- 2.37.1 (Apple Git-137.1) 

I have not tested this beyond "compile on mingw", which didn't warn before the patch either. But it looks innocent enough, and if Lev confirms it actually works, good enough. Your patch has been applied to the master and release/2.6 branch. commit 9c52e0c610ef1229561c2d038ca41fe2cbefe8da (master) commit 27dac5061cfeff75470dca11e07dadb1fb0ad180 (release/2.6) Author: Arne Schwabe Date: Tue Feb 14 14:43:23 2023 +0100 Use proper print format/casting when converting msg_channel handle Signed-off-by: Arne Schwabe <ar...@rf...> Acked-by: Lev Stipakov <lst...@gm...> Message-Id: <202...@rf...> URL: https://www.mail-archive.com/ope...@li.../msg26255.html Signed-off-by: Gert Doering <ge...@gr...> -- kind regards, Gert Doering 

Hi, Sadly MSVC doesn't show those warnings even on L4. Change looks good. I couldn't (yet) test on x86 since I (hopefully not for long) lost access to the x86 machine, but arm64 works just fine. Acked-by: Lev Stipakov <lst...@gm...> ti 14. helmik. 2023 klo 15.44 Arne Schwabe (ar...@rf...) kirjoitti: > > The current casting triggers a warning on 32bit: > > init.c:1842:66: error: cast from pointer to integer of different size [-Werror=pointer-to-int-cast] > > Use the proper printf format specifier for printing a pointer avoiding > the cast alltogether. > > In options.c use a cast to intptr_t before converting to a handle to > avoid having to ifdef atoll/atol for 32/64 bit. > > Signed-off-by: Arne Schwabe <ar...@rf...> > --- > src/openvpn/init.c | 3 ++- > src/openvpn/options.c | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/src/openvpn/init.c b/src/openvpn/init.c > index 0ad2c7e09..0d50d9189 100644 > --- a/src/openvpn/init.c > +++ b/src/openvpn/init.c > @@ -1839,7 +1839,8 @@ do_open_tun(struct context *c, int *error_flags) > #ifdef _WIN32 > /* store (hide) interactive service handle in tuntap_options */ > c->c1.tuntap->options.msg_channel = c->options.msg_channel; > - msg(D_ROUTE, "interactive service msg_channel=%" PRIu64, (unsigned long long) c->options.msg_channel); > + msg(D_ROUTE, "interactive service msg_channel=%" PRIuPTR, > + (intptr_t) c->options.msg_channel); > #endif > > /* allocate route list structure */ > diff --git a/src/openvpn/options.c b/src/openvpn/options.c > index c1ddb0262..679528187 100644 > --- a/src/openvpn/options.c > +++ b/src/openvpn/options.c > @@ -7882,7 +7882,7 @@ add_option(struct options *options, > #ifdef _WIN32 > VERIFY_PERMISSION(OPT_P_GENERAL); > HANDLE process = GetCurrentProcess(); > - HANDLE handle = (HANDLE) atoll(p[1]); > + HANDLE handle = (HANDLE) ((intptr_t) atoll(p[1])); > if (!DuplicateHandle(process, handle, process, &options->msg_channel, 0, > FALSE, DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) > { > -- > 2.37.1 (Apple Git-137.1) > > > > _______________________________________________ > Openvpn-devel mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openvpn-devel -- -Lev 

From: Kristof Provost <kp...@Fr...> The libnv check doesn't work as expected on FreeBSD 14.x, because FreeBSD has namespaced libnv to avoid conflicts with libnvpair. This means that the naive check generated by AC_CHECK_LIB() fails to detect libnv even though it's present. Instead check for the if_ovpn.h header. This is a more accurate check anyway, as libnv is present on FreeBSD versions prior to 14 (which do not support DCO). Signed-off-by: Kristof Provost <kpr...@ne...> --- configure.ac | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 4c271464..67f680b2 100644 --- a/configure.ac +++ b/configure.ac @@ -832,9 +832,7 @@ if test "$enable_dco" != "no"; then	fi	;;	*-*-freebsd*) -	AC_CHECK_LIB( -	[nv], -	[nvlist_create], +	AC_CHECK_HEADERS([net/if_ovpn.h],	[ LIBS="${LIBS} -lnv" AC_DEFINE(ENABLE_DCO, 1, [Enable data channel offload for FreeBSD]) @@ -842,7 +840,7 @@ if test "$enable_dco" != "no"; then	],	[ enable_dco="no" - AC_MSG_WARN([Name/Value pair library not found.]) + AC_MSG_WARN([DCO header not found.])	]	)	if test "$enable_dco" = "no"; then -- 2.39.2 

From: Kristof Provost <kp...@Fr...> Very low values for 'fragment' can result in a division by zero in optimal_fragment_size() (because it rounds max_frag_size down with FRAG_SIZE_ROUND_MASK). Enforce a minimal fragment size of 68 bytes, based on RFC 791 ("Every internet module must be able to forward a datagram of 68 octets without further fragmentation.") Signed-off-by: Kristof Provost <kpr...@ne...> --- src/openvpn/options.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/openvpn/options.c b/src/openvpn/options.c index 9105449c..9f79da09 100644 --- a/src/openvpn/options.c +++ b/src/openvpn/options.c @@ -6549,6 +6549,12 @@ add_option(struct options *options, VERIFY_PERMISSION(OPT_P_MTU|OPT_P_CONNECTION); options->ce.fragment = positive_atoi(p[1]); + if (options->ce.fragment < 68) + { + msg(msglevel, "fragment needs to be at least 68"); + goto err; + } + if (p[2] && streq(p[2], "mtu")) { options->ce.fragment_encap = true; -- 2.39.2