Attention is currently required from: cron2, plaisthos.
Hello cron2,
I'd like you to reexamine a change. Please visit
http://gerrit.openvpn.net/c/openvpn/+/1800?usp=email
to look at the new patch set (#4).
The following approvals got outdated and were removed:
Code-Review-1 by cron2
Change subject: Allow extracing other integers from peer info and add a unit
test for it
......................................................................
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 <[email protected]>
---
M CMakeLists.txt
M src/openvpn/push.c
M src/openvpn/ssl_ncp.c
M src/openvpn/ssl_util.c
M src/openvpn/ssl_util.h
M tests/unit_tests/openvpn/Makefile.am
M tests/unit_tests/openvpn/test_push_update_msg.c
M tests/unit_tests/openvpn/test_ssl.c
8 files changed, 158 insertions(+), 54 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/00/1800/4
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 7473f15..21bcb91 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -908,7 +908,8 @@
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 633c007d..ce2baf2 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -718,9 +718,9 @@
/* 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 b450de8..8b0f28d 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 2f01f8a..e6af145 100644
--- a/src/openvpn/ssl_util.c
+++ b/src/openvpn/ssl_util.c
@@ -27,28 +27,55 @@
#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");
- if (!var_end)
- {
- /* var is at end of the peer_info list and no '\n' follows */
- var_end = var_start + strlen(var_start);
- }
+
+ /* 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 = strchrnul(var_start, '\n');
+
char *var_value = string_alloc(var_start, gc);
/* NULL terminate the copy at the right position */
@@ -56,38 +83,40 @@
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 1a69e6f..751af85 100644
--- a/src/openvpn/ssl_util.h
+++ b/src/openvpn/ssl_util.h
@@ -33,11 +33,31 @@
#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
+ */
+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 +71,11 @@
*
* @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 d861ef9..5ccf3ce 100644
--- a/tests/unit_tests/openvpn/Makefile.am
+++ b/tests/unit_tests/openvpn/Makefile.am
@@ -395,7 +395,8 @@
$(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 219b476..389fa09 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 @@
{
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 @@
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 0e9cecf..03544ea 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 */
@@ -829,6 +831,68 @@
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)
@@ -853,7 +917,8 @@
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)
};
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1800?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iad94e7a9d0b3be8a8db09e9a20eaac6041470ab6
Gerrit-Change-Number: 1800
Gerrit-PatchSet: 4
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-Reviewer: cron2 <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: cron2 <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
