| Message ID | 20230301134455.2810114-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel] Only update frame calculation if we have a valid link sockets | expand |
Acked-by: Gert Doering <gert@greenie.muc.de> This is not really "high priority critical NULL pointer crash bug" important, because due to lucky circumstances the result from get_link_socket_info(c) on a NULL pointer is still NULL (because &->info is the first member of the struct) - so later consumers of *lsi will not crash ("if (lsi && lsi->...)"). It's still undefined behaviour, so let's not rely on this, and instead only update frame parameters *here* in the modes that have a link_sock available. Notable exception is "UDP child", but in this case there is another call chain from multi_connection_established() -> tls_session_update_crypto_params_do_work() -> frame_calculate_dynamic() which will do the updating later. (Discussed at lenght with Arne on IRC): I still find the whole session setup thing confusing (like, why is the TCP link_socket initialized before init_instance(), and UDP child only afterwards, leading to this situation in the first place?) - but this is not "bugfix, 2.6" land, more "refactor for 2.7". I have lightly tested this with GHA and the server test framework - which does not really *verify* all these frame / overhead, just "clients can still connect just fine, nothing crashes/fails". Your patch has been applied to the master and release/2.6 branch. commit 2d17869f8d9d8e27f64f1a7cd1514fbbb768807b (master) commit 75cc2fa6e15ce806415aed33d7608b8d9cc00e36 (release/2.6) Author: Arne Schwabe Date: Wed Mar 1 14:44:55 2023 +0100 Only update frame calculation if we have a valid link sockets Signed-off-by: Arne Schwabe <arne@rfc2549.org> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20230301134455.2810114-1-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26318.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
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.
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 <arne@rfc2549.org> --- src/openvpn/init.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)