From 06ced3fd84e4e28f2774c4156f8032a2bed736a2 Mon Sep 17 00:00:00 2001 From: Arne Schwabe Date: Sat, 8 Aug 2026 17:49:28 +0200 Subject: [PATCH] Allow extracing other integers from peer info and add a unit test for it This also ensure that we only really extract the variable we are looking for. The old code would also consider UV_IV_NCP to be IV_NCP. Change-Id: Iad94e7a9d0b3be8a8db09e9a20eaac6041470ab6 Signed-off-by: Arne Schwabe Acked-by: Gert Doering Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1800 Message-Id: <20260808154933.17540-1-gert@greenie.muc.de> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg38241.html Signed-off-by: Gert Doering --- CMakeLists.txt | 3 +- src/openvpn/push.c | 4 +- src/openvpn/ssl_ncp.c | 14 +--- src/openvpn/ssl_util.c | 80 +++++++++++++------ src/openvpn/ssl_util.h | 31 ++++++- tests/unit_tests/openvpn/Makefile.am | 3 +- .../unit_tests/openvpn/test_push_update_msg.c | 7 +- tests/unit_tests/openvpn/test_ssl.c | 67 +++++++++++++++- 8 files changed, 159 insertions(+), 50 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3ff27343c..aabbcb67c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -916,7 +916,8 @@ if (BUILD_TESTING) src/openvpn/options_util.c src/openvpn/otime.c src/openvpn/list.c - ) + src/openvpn/ssl_util.c + ) target_sources(test_argv PRIVATE tests/unit_tests/openvpn/mock_get_random.c diff --git a/src/openvpn/push.c b/src/openvpn/push.c index 633c007dc..ce2baf238 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -718,9 +718,9 @@ prepare_push_reply(struct context *c, struct gc_arena *gc, struct push_list *pus /* Push our mtu to the peer if it supports pushable MTUs */ int client_max_mtu = 0; - const char *iv_mtu = extract_var_peer_info(tls_multi->peer_info, "IV_MTU=", gc); + unsigned int iv_mtu = peer_info_extract_uint(tls_multi->peer_info, "IV_MTU="); - if (iv_mtu && sscanf(iv_mtu, "%d", &client_max_mtu) == 1) + if (iv_mtu != 0) { push_option_fmt(gc, push_list, M_USAGE, "tun-mtu %d", o->ce.tun_mtu); if (client_max_mtu < o->ce.tun_mtu) diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c index b450de82e..8b0f28dfb 100644 --- a/src/openvpn/ssl_ncp.c +++ b/src/openvpn/ssl_ncp.c @@ -55,20 +55,10 @@ * Return the Negotiable Crypto Parameters version advertised in the peer info * string, or 0 if none specified. */ -static int +static unsigned int tls_peer_info_ncp_ver(const char *peer_info) { - const char *ncpstr = peer_info ? strstr(peer_info, "IV_NCP=") : NULL; - if (ncpstr) - { - int ncp = 0; - int r = sscanf(ncpstr, "IV_NCP=%d", &ncp); - if (r == 1) - { - return ncp; - } - } - return 0; + return peer_info_extract_uint(peer_info, "IV_NCP="); } /** diff --git a/src/openvpn/ssl_util.c b/src/openvpn/ssl_util.c index 2f01f8a3e..ba185c142 100644 --- a/src/openvpn/ssl_util.c +++ b/src/openvpn/ssl_util.c @@ -27,26 +27,56 @@ #include "openvpn.h" #include "ssl_util.h" -char * -extract_var_peer_info(const char *peer_info, const char *var, struct gc_arena *gc) +/** + * Looks for the string field at the start of peer_info or + * after any new line and returns a pointer to the start of the field + * or NULL if not found. + */ +static const char * +peer_info_extract_varstr(const char *peer_info, const char *field) { if (!peer_info) { return NULL; } - const char *var_start = strstr(peer_info, var); + while (peer_info) + { + /* peer info starts with this field */ + if (strncmp(peer_info, field, strlen(field)) == 0) + { + return peer_info; + } + + /* skip to the next line */ + peer_info = strchr(peer_info, '\n'); + if (peer_info) + { + /* skip the '\n' itself */ + peer_info++; + } + } + return NULL; +} + +char * +extract_var_peer_info(const char *peer_info, const char *var, struct gc_arena *gc) +{ + const char *var_start = peer_info_extract_varstr(peer_info, var); if (!var_start) { - /* variable not found in peer info */ + /* variable hasn't been found in peer info */ return NULL; } + /* skip over the variable name (includes the = as per API)*/ var_start += strlen(var); - const char *var_end = strstr(var_start, "\n"); + + /* returns the location of the next '\n' or the end of the + * string if the variable is already on the last line */ + const char *var_end = strchr(var_start, '\n'); if (!var_end) { - /* var is at end of the peer_info list and no '\n' follows */ var_end = var_start + strlen(var_start); } @@ -56,38 +86,40 @@ extract_var_peer_info(const char *peer_info, const char *var, struct gc_arena *g return var_value; } + unsigned int -extract_iv_proto(const char *peer_info) +peer_info_extract_int(const char *peer_info, const char *field, const char *format, unsigned int default_value) { - const char *optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL; + const char *optstr = peer_info_extract_varstr(peer_info, field); if (optstr) { - int proto = 0; - int r = sscanf(optstr, "IV_PROTO=%d", &proto); - if (r == 1 && proto > 0) + optstr += strlen(field); + + int value = 0; + int r = sscanf(optstr, format, &value); + if (r == 1 && value >= 0) { - return proto; + return value; } } - return 0; + return default_value; +} + +unsigned int +peer_info_extract_uint(const char *peer_info, const char *field) +{ + return peer_info_extract_int(peer_info, field, "%u", 0); } uint32_t extract_asymmetric_peer_id(const char *peer_info) { - for (const char *p = peer_info; p && (p = strstr(p, "ID=")); p += 3) + uint32_t peer_id = peer_info_extract_int(peer_info, "ID=", "%x", MAX_PEER_ID); + if (peer_id < MAX_PEER_ID) { - /* only accept "ID=" at the start of a line, so it does not match - * substrings like "UV_ID=" or "GUID=" */ - if (p == peer_info || p[-1] == '\n') - { - uint32_t peer_id = 0; - if (sscanf(p, "ID=%x", &peer_id) == 1 && peer_id < MAX_PEER_ID) - { - return peer_id; - } - } + return peer_id; } + return MAX_PEER_ID; } diff --git a/src/openvpn/ssl_util.h b/src/openvpn/ssl_util.h index 1a69e6fa9..6f9f65b0a 100644 --- a/src/openvpn/ssl_util.h +++ b/src/openvpn/ssl_util.h @@ -32,12 +32,34 @@ #include "buffer.h" +/** + * Extracts the named integer variable and returns its value or default_value + * if it cannot be extracted. + * + * @param peer_info peer info string to search in + * @param field name of the field to be extracted including the = + * @param format sscanf/printf format string of the type to extract + * @param default_value default value to return if the field cannot be extracted + */ +unsigned int +peer_info_extract_int(const char *peer_info, const char *field, const char *format, unsigned int default_value); + +/** + * Extracts the named integer variable and returns its value or 0 + * if it cannot be extracted. + * + * @param peer_info peer info string to search in + * @param field name of the field to be extracted + */ +unsigned int +peer_info_extract_uint(const char *peer_info, const char *field); + /** * Extracts a variable from peer info, the returned string will be allocated * using the supplied gc_arena * * @param peer_info The peer's peer_info - * @param var The variable *including* =, e.g. IV_CIPHERS= + * @param var The variable including =, e.g. IV_CIPHERS= * @param gc GC arena to allocate return value in * * @return The content of the variable as NULL terminated string or NULL if the @@ -51,8 +73,11 @@ char *extract_var_peer_info(const char *peer_info, const char *var, struct gc_ar * * @param peer_info peer info string to search for IV_PROTO */ -unsigned int extract_iv_proto(const char *peer_info); - +static inline unsigned int +extract_iv_proto(const char *peer_info) +{ + return peer_info_extract_uint(peer_info, "IV_PROTO="); +} /** * Extracts the ID variable and returns its value or diff --git a/tests/unit_tests/openvpn/Makefile.am b/tests/unit_tests/openvpn/Makefile.am index 9174ed02c..38adfd768 100644 --- a/tests/unit_tests/openvpn/Makefile.am +++ b/tests/unit_tests/openvpn/Makefile.am @@ -400,7 +400,8 @@ push_update_msg_testdriver_SOURCES = test_push_update_msg.c \ $(top_srcdir)/src/openvpn/platform.c \ $(top_srcdir)/src/openvpn/options_util.c \ $(top_srcdir)/src/openvpn/otime.c \ - $(top_srcdir)/src/openvpn/list.c + $(top_srcdir)/src/openvpn/list.c \ + $(top_srcdir)/src/openvpn/ssl_util.c socket_testdriver_CFLAGS = \ -I$(top_srcdir)/include -I$(top_srcdir)/src/compat -I$(top_srcdir)/src/openvpn \ diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c index 219b47665..389fa0959 100644 --- a/tests/unit_tests/openvpn/test_push_update_msg.c +++ b/tests/unit_tests/openvpn/test_push_update_msg.c @@ -188,12 +188,6 @@ mroute_extract_openvpn_sockaddr(struct mroute_addr *addr, { return true; } - -unsigned int -extract_iv_proto(const char *peer_info) -{ - return IV_PROTO_PUSH_UPDATE; -} #endif /* ifdef ENABLE_MANAGEMENT */ /* tests */ @@ -650,6 +644,7 @@ setup2(void **state) m->instances = calloc(1, sizeof(struct multi_instance *)); struct multi_instance *mi = calloc(1, sizeof(struct multi_instance)); mi->context.c2.tls_multi = calloc(1, sizeof(struct tls_multi)); + mi->context.c2.tls_multi->peer_info = "IV_PROTO=4096"; // IV_PROTO_PUSH_UPDATE *(m->instances) = mi; m->top.options.disable_dco = true; *state = m; diff --git a/tests/unit_tests/openvpn/test_ssl.c b/tests/unit_tests/openvpn/test_ssl.c index d473d6770..3e84fb8a7 100644 --- a/tests/unit_tests/openvpn/test_ssl.c +++ b/tests/unit_tests/openvpn/test_ssl.c @@ -48,7 +48,9 @@ #include "buffer.h" #include "cert_data.h" #include "packet_id.h" +#include "ssl_util.h" #include "ssl_verify.h" +#include "openvpn.h" /* Mock function to be allowed to include win32.c which is required for * getting the temp directory */ @@ -884,6 +886,68 @@ crypto_test_print_cert_details(void **state) free_certificate(cert); } +void +ssl_test_extract_peer_info(void **state) +{ + const char *peer_info_normal = + "IV_VER=2.6_git\nIV_PLAT=mac\nIV_TCPNL=1\nIV_NCP=2\n" + "IV_CIPHERS=AES-256-GCM:AES-128-GCM:CHACHA20-POLY1305\n" + "IV_PROTO=94\nIV_LZO_STUB=1\nIV_COMP_STUB=1\nP=78\nIV_COMP_STUBv2=1\n" + "IV_SSL=OpenSSL_3.0.5_5_Jul_2022\nIV_LZ4v2=1"; + + const char *empty = ""; + const char *invalid_proto = "IV_PROTO=seven\nIV_SSL=7\nP=300\nID=xyz"; + const char *test_prefix = "UV_IV_PROTO=773\nNP=112\nPD=8\n"; + + + assert_int_equal(extract_iv_proto(peer_info_normal), 94); + assert_int_equal(extract_iv_proto(empty), 0); + assert_int_equal(extract_iv_proto(invalid_proto), 0); + /* This should not pick up the UV_IV_PROTO that has the extra prefix */ + assert_int_equal(extract_iv_proto(test_prefix), 0); + + assert_int_equal(peer_info_extract_uint(peer_info_normal, "IV_COMP_STUB="), 1); + assert_int_equal(peer_info_extract_int(empty, "IV_COMP_STUB=", "%d", 0xfe0d0d), 0xfe0d0d); + assert_int_equal(peer_info_extract_uint(invalid_proto, "IV_COMP_STUB="), 0); + + assert_int_equal(peer_info_extract_int(test_prefix, "NP=", "%d", 23), 112); + assert_int_equal(peer_info_extract_uint(test_prefix, "PD="), 8); + assert_int_equal(peer_info_extract_int(test_prefix, "P=", "%x", 0xfe0d0d), 0xfe0d0d); + assert_int_equal(peer_info_extract_uint(peer_info_normal, "P="), 78); + assert_int_equal(peer_info_extract_uint(test_prefix, "UV_IV_PROTO="), 773); + + struct gc_arena gc = gc_new(); + + const char *peer_ciphers = extract_var_peer_info(peer_info_normal, "IV_CIPHERS=", &gc); + assert_string_equal(peer_ciphers, "AES-256-GCM:AES-128-GCM:CHACHA20-POLY1305"); + + /* with the extra = this should not extract anything */ + const char *proto = extract_var_peer_info(peer_info_normal, "IV_PROTO==", &gc); + assert_null(proto); + + proto = extract_var_peer_info(peer_info_normal, "IV_PROTO=", &gc); + assert_string_equal(proto, "94"); + + + const char *double_eq_info = "FOO=7\nFOO==new"; + const char *foo_eq = extract_var_peer_info(double_eq_info, "FOO=", &gc); + const char *foo = extract_var_peer_info(double_eq_info, "FOO", &gc); + const char *foo_2eq = extract_var_peer_info(double_eq_info, "FOO==", &gc); + + assert_string_equal(foo, "=7"); + assert_string_equal(foo_eq, "7"); + assert_string_equal(foo_2eq, "new"); + + assert_int_equal(extract_asymmetric_peer_id(double_eq_info), MAX_PEER_ID); + assert_int_equal(extract_asymmetric_peer_id(peer_info_normal), MAX_PEER_ID); + assert_int_equal(extract_asymmetric_peer_id(invalid_proto), MAX_PEER_ID); + assert_int_equal(extract_asymmetric_peer_id("ID=f7"), 0xf7); + assert_int_equal(extract_asymmetric_peer_id("X=foo\nID=12ab"), 0x12ab); + assert_int_equal(extract_asymmetric_peer_id("X=foo\nID=34dd\nY=bar"), 0x34dd); + assert_int_equal(extract_asymmetric_peer_id("X=foo\nID=12345678"), MAX_PEER_ID); + + gc_free(&gc); +} int main(void) @@ -908,7 +972,8 @@ main(void) cmocka_unit_test(test_data_channel_roundtrip_bf_cbc), cmocka_unit_test(test_data_channel_known_vectors_epoch), cmocka_unit_test(test_data_channel_known_vectors_shortpktid), - cmocka_unit_test(crypto_test_print_cert_details) + cmocka_unit_test(crypto_test_print_cert_details), + cmocka_unit_test(ssl_test_extract_peer_info) }; -- 2.47.3