| Message ID | 20220728194733.27721-1-a@unstable.cc |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
Hi, On Thu, Jul 28, 2022 at 09:47:33PM +0200, Antonio Quartulli wrote: > DCO will try to install keys upon generating them, however, this happens > when parsing pushed cipher options (due to NCP). > > For this reason we need to postpone parsing pushed cipher options to *after* > the tunnel interface has been opened, otherwise we would have no DCO netdev > object to operate on. > > At the same time we split the parsing code, so that we can ensure that > the NEW_PEER call can happen after the received peer-id has been parsed > (it is required by all DCO API calls). > > Signed-off-by: Antonio Quartulli <a@unstable.cc> Some aspects of this patch I do not like, and others are broken, unfortunately. What I do not like is the naming of finish_options(), because I find it confusing in relation to the "first half" of that, do_deferred_options() - so maybe name this do_deferred_options_crypto()? But this is somewhat minor. What I'm not sure about is the order of things in multi.c - but that might be due to misunderstanding. But I wonder why you are calling "multi_client_generate_tls_keys()" *before* processing the incoming crypto options in "finish_options()", which call update_crypto_params() - is there an interdependency? Or does this not matter? The "broken" bit is: it breaks udp p2p TLS connections - the server will segfault. I guess this is due to moving p2p_ncp around... The crash manifests as follows: gdb openvpn gdb> run server.conf ... 2022-08-02 12:50:45 us=144084 net_iface_mtu_set: mtu 1500 for tun5 2022-08-02 12:50:45 us=144179 net_iface_up: set tun5 up 2022-08-02 12:50:45 us=144241 net_addr_v6_add: fd00:abcd:204:8::1/64 dev tun5 2022-08-02 12:50:45 us=144396 Data Channel MTU parms [ mss_fix:0 max_frag:0 tun_mtu:1500 headroom:136 payload:1736 tailroom:557 ET:0 ] 2022-08-02 12:50:45 us=144457 Local Options String (VER=V4): 'V4,dev-type tun,link-mtu 1545,tun-mtu 1500,proto UDPv4,tun-ipv6,ifconfig 10.204.8.2 10.204.8.1,comp-lzo,cipher BF-CBC,auth SHA1,keysize 128,secret' 2022-08-02 12:50:45 us=144480 Expected Remote Options String (VER=V4): 'V4,dev-type tun,link-mtu 1545,tun-mtu 1500,proto UDPv4,tun-ipv6,ifconfig 10.204.8.1 10.204.8.2,comp-lzo,cipher BF-CBC,auth SHA1,keysize 128,secret' 2022-08-02 12:50:45 us=144539 Socket Buffers: R=[212992->212992] S=[212992->212992] 2022-08-02 12:50:45 us=144568 setsockopt(IPV6_V6ONLY=0) 2022-08-02 12:50:45 us=144616 UDPv6 link local (bound): [AF_INET6][undef]:51204 2022-08-02 12:50:45 us=144644 UDPv6 link remote: [AF_UNSPEC] <waiting, client connects> 2022-08-02 12:52:13 us=388007 Peer Connection Initiated with [AF_INET6]::ffff:194.97.140.21:19880 Program received signal SIGSEGV, Segmentation fault0x00005555555d38e1 in check_session_cipher (session=session@entry=0x448, options=options@entry=0x7fffffffd4d0) at ssl_ncp.c:499 499 && streq(options->ciphername, session->opt->config_ciphername); (gdb) where #0 0x00005555555d38e1 in check_session_cipher (session=session@entry=0x448, options=options@entry=0x7fffffffd4d0) at ssl_ncp.c:499 #1 0x00005555555cab8b in tls_session_update_crypto_params (multi=0x0, session=0x448, options=0x7fffffffd4d0, frame=0x7fffffffe0a0, frame_fragment=0x0, lsi=0x55555562bc80) at ssl.c:1716 #2 0x000055555557899f in finish_options (c=<optimized out>) at init.c:2358 #3 0x0000555555578a65 in do_up (c=c@entry=0x7fffffffd4d0, pulled_options=pulled_options@entry=false, option_types_found=option_types_found@entry=0) at init.c:2131 #4 0x0000555555571326 in check_connection_established (c=0x7fffffffd4d0) at forward.c:311 #5 process_coarse_timers (c=0x7fffffffd4d0) at forward.c:634 #6 check_coarse_timers (c=0x7fffffffd4d0) at forward.c:717 #7 pre_select (c=c@entry=0x7fffffffd4d0) at forward.c:1823 #8 0x0000555555597b36 in tunnel_point_to_point (c=0x7fffffffd4d0) at openvpn.c:79 #9 openvpn_main (argc=2, argv=0x7fffffffe638) at openvpn.c:311 #10 0x00007ffff7cc21ea in ?? () from /lib64/libc.so.6 #11 0x00007ffff7cc229c in __libc_start_main () from /lib64/libc.so.6 #12 0x000055555555c0b1 in _start () gert
diff --git a/src/openvpn/init.c b/src/openvpn/init.c index 338d797b..db42d540 100644 --- a/src/openvpn/init.c +++ b/src/openvpn/init.c @@ -2093,14 +2093,6 @@ do_up(struct context *c, bool pulled_options, unsigned int option_types_found) return false; } } - else if (c->mode == MODE_POINT_TO_POINT) - { - if (!do_deferred_p2p_ncp(c)) - { - msg(D_TLS_ERRORS, "ERROR: Failed to apply P2P negotiated protocol options"); - return false; - } - } /* if --up-delay specified, open tun, do ifconfig, and run up script now */ if (c->options.up_delay || PULL_DEFINED(&c->options)) @@ -2127,6 +2119,20 @@ do_up(struct context *c, bool pulled_options, unsigned int option_types_found) } } + if (!pulled_options && c->mode == MODE_POINT_TO_POINT) + { + if (!do_deferred_p2p_ncp(c)) + { + msg(D_TLS_ERRORS, "ERROR: Failed to apply P2P negotiated protocol options"); + return false; + } + } + + if (!finish_options(c)) + { + return false; + } + if (c->c2.did_open_tun) { c->c1.pulled_options_digest_save = c->c2.pulled_options_digest; @@ -2332,23 +2338,30 @@ do_deferred_options(struct context *c, const unsigned int found) { return false; } - struct frame *frame_fragment = NULL; + } + + return true; +} + +bool +finish_options(struct context *c) +{ + struct frame *frame_fragment = NULL; #ifdef ENABLE_FRAGMENT - if (c->options.ce.fragment) - { - frame_fragment = &c->c2.frame_fragment; - } + if (c->options.ce.fragment) + { + frame_fragment = &c->c2.frame_fragment; + } #endif - struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; - if (!tls_session_update_crypto_params(c->c2.tls_multi, session, - &c->options, &c->c2.frame, - frame_fragment, - get_link_socket_info(c))) - { - msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options"); - return false; - } + struct tls_session *session = &c->c2.tls_multi->session[TM_ACTIVE]; + if (!tls_session_update_crypto_params(c->c2.tls_multi, session, + &c->options, &c->c2.frame, + frame_fragment, + get_link_socket_info(c))) + { + msg(D_TLS_ERRORS, "OPTIONS ERROR: failed to import crypto options"); + return false; } return true; diff --git a/src/openvpn/init.h b/src/openvpn/init.h index 5f412a33..9389e0db 100644 --- a/src/openvpn/init.h +++ b/src/openvpn/init.h @@ -97,6 +97,8 @@ void reset_coarse_timers(struct context *c); bool do_deferred_options(struct context *c, const unsigned int found); +bool finish_options(struct context *c); + void inherit_context_child(struct context *dest, const struct context *src); diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index c72575ae..34ab90b4 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2405,6 +2405,13 @@ multi_client_connect_late_setup(struct multi_context *m, { mi->context.c2.tls_multi->multi_state = CAS_FAILED; } + /* Continue processing options only if authentication hasn't failed. + * Otherwise it does not make sense and we may operate on a non-configured + * client instance */ + else + { + finish_options(&mi->context); + } /* send push reply if ready */ if (mi->context.c2.push_request_received)
DCO will try to install keys upon generating them, however, this happens when parsing pushed cipher options (due to NCP). For this reason we need to postpone parsing pushed cipher options to *after* the tunnel interface has been opened, otherwise we would have no DCO netdev object to operate on. At the same time we split the parsing code, so that we can ensure that the NEW_PEER call can happen after the received peer-id has been parsed (it is required by all DCO API calls). Signed-off-by: Antonio Quartulli <a@unstable.cc> --- Changes from v1: * removed error message in case of failure of finish_options(). The latter already warns the user about the failure - no need to print another generic message. --- src/openvpn/init.c | 57 ++++++++++++++++++++++++++++----------------- src/openvpn/init.h | 2 ++ src/openvpn/multi.c | 7 ++++++ 3 files changed, 44 insertions(+), 22 deletions(-)