[Openvpn-devel] [PATCH v7] Allow extracing other integers from peer info and add a unit test for it

Gert Doering <[email protected]>
Newsgroups net.sourceforge.lists.openvpn-devel
Message-ID <[email protected]> 
From: Arne Schwabe <[email protected]>

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]>
Acked-by: Gert Doering <[email protected]>
Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1800
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/1800
This mail reflects revision 7 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <[email protected]>

        
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..ba185c1 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 @@
     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..6f9f65b 100644
--- a/src/openvpn/ssl_util.h
+++ b/src/openvpn/ssl_util.h
@@ -33,11 +33,33 @@
 #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 @@
  *
  * @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 d473d67..3e84fb8 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 @@
     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 @@
         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)
 
     };
 


_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.