[Openvpn-devel] [M] Change in openvpn[master]: Allow extracing other integers from peer info and add a unit test for it

plaisthos has uploaded this change for review. ( http://gerrit.openvpn.net/c/openvpn/+/1800?usp=email )


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 src/openvpn/ssl_verify.c
M tests/unit_tests/openvpn/test_push_update_msg.c
M tests/unit_tests/openvpn/test_ssl.c
8 files changed, 124 insertions(+), 41 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/00/1800/1

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 7473f15..b886414 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -906,6 +906,7 @@
         tests/unit_tests/openvpn/mock_msg.c
         tests/unit_tests/openvpn/mock_get_random.c
         src/openvpn/options_util.c
+        src/openvpn/ssl_util.c
         src/openvpn/otime.c
         src/openvpn/list.c
         )
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 633c007d..88b499f 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -718,7 +718,7 @@
 
     /* 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);
+    const char *iv_mtu = extract_var_peer_info(tls_multi->peer_info, "IV_MTU", gc);
 
     if (iv_mtu && sscanf(iv_mtu, "%d", &client_max_mtu) == 1)
     {
diff --git a/src/openvpn/ssl_ncp.c b/src/openvpn/ssl_ncp.c
index b450de8..8148396 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", 0);
 }
 
 /**
@@ -227,7 +217,7 @@
 tls_peer_ncp_list(const char *peer_info, struct gc_arena *gc)
 {
     /* Check if the peer sends the IV_CIPHERS list */
-    const char *iv_ciphers = extract_var_peer_info(peer_info, "IV_CIPHERS=", gc);
+    const char *iv_ciphers = extract_var_peer_info(peer_info, "IV_CIPHERS", gc);
     if (iv_ciphers)
     {
         return iv_ciphers;
@@ -358,7 +348,7 @@
 {
     /* we use a local gc arena to keep the temporary strings needed by strsep */
     struct gc_arena gc_local = gc_new();
-    const char *peer_ciphers = extract_var_peer_info(peer_info, "IV_CIPHERS=", &gc_local);
+    const char *peer_ciphers = extract_var_peer_info(peer_info, "IV_CIPHERS", &gc_local);
 
     if (!peer_ciphers)
     {
diff --git a/src/openvpn/ssl_util.c b/src/openvpn/ssl_util.c
index 2f01f8a..a4c2910 100644
--- a/src/openvpn/ssl_util.c
+++ b/src/openvpn/ssl_util.c
@@ -27,6 +27,32 @@
 #include "openvpn.h"
 #include "ssl_util.h"
 
+static const char *
+peer_info_extract_varstr(const char *peer_info, const char *field)
+{
+    char buf[32];
+    snprintf(buf, sizeof(buf), "%s=", field);
+
+    const char *optstr = NULL;
+
+    /* peer info starts with this field */
+    if (peer_info && strncmp(peer_info, buf, strlen(buf)) == 0)
+    {
+        optstr = peer_info;
+    }
+    else
+    {
+        snprintf(buf, sizeof(buf), "\n%s=", field);
+        optstr = peer_info ? strstr(peer_info, buf) : NULL;
+        if (optstr)
+        {
+            /* skip the \n */
+            optstr++;
+        }
+    }
+    return optstr;
+}
+
 char *
 extract_var_peer_info(const char *peer_info, const char *var, struct gc_arena *gc)
 {
@@ -35,14 +61,15 @@
         return NULL;
     }
 
-    const char *var_start = strstr(peer_info, var);
+    const char *var_start = peer_info_extract_varstr(peer_info, var);
     if (!var_start)
     {
         /* variable not found in peer info */
         return NULL;
     }
 
-    var_start += strlen(var);
+    /* one extra byte forward for the = */
+    var_start += strlen(var) + 1;
     const char *var_end = strstr(var_start, "\n");
     if (!var_end)
     {
@@ -56,36 +83,35 @@
     return var_value;
 }
 
+
 unsigned int
-extract_iv_proto(const char *peer_info)
+peer_info_extract_uint(const char *peer_info, const char *field, 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)
     {
+        char buf[32];
+        snprintf(buf, sizeof(buf), "%s=%%d", field);
         int proto = 0;
-        int r = sscanf(optstr, "IV_PROTO=%d", &proto);
+        int r = sscanf(optstr, buf, &proto);
         if (r == 1 && proto > 0)
         {
             return proto;
         }
     }
-    return 0;
+    return default_value;
 }
 
 uint32_t
 extract_asymmetric_peer_id(const char *peer_info)
 {
-    for (const char *p = peer_info; p && (p = strstr(p, "ID=")); p += 3)
+    const char *optstr = peer_info_extract_varstr(peer_info, "ID");
+    if (optstr)
     {
-        /* 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(optstr, "ID=%x", &peer_id) == 1 && peer_id < MAX_PEER_ID)
         {
-            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..49faefa 100644
--- a/src/openvpn/ssl_util.h
+++ b/src/openvpn/ssl_util.h
@@ -33,11 +33,22 @@
 #include "buffer.h"
 
 /**
+ * Extracts the named variable and returns its value or default_value
+ * if it cannot be extracted.
+ *
+ * @param peer_info     peer info string to search in
+ * @parm field          name of the field to be extracted
+ * @parm default_value  value to return if not found or cannot be extracted
+ */
+unsigned int
+peer_info_extract_uint(const char *peer_info, const char *field, unsigned int default_value);
+
+/**
  * 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 *excluding* =, 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 +62,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", 0);
+}
 
 /**
  * Extracts the ID variable and returns its value or
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 162f68f..3317380 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -861,7 +861,7 @@
 {
     struct gc_arena gc = gc_new();
 
-    char *iv_sso = extract_var_peer_info(peer_info, "IV_SSO=", &gc);
+    char *iv_sso = extract_var_peer_info(peer_info, "IV_SSO", &gc);
     if (!iv_sso)
     {
         gc_free(&gc);
diff --git a/tests/unit_tests/openvpn/test_push_update_msg.c b/tests/unit_tests/openvpn/test_push_update_msg.c
index 219b476..8f42483 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 */
diff --git a/tests/unit_tests/openvpn/test_ssl.c b/tests/unit_tests/openvpn/test_ssl.c
index 0e9cecf..9c2a99d 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,61 @@
     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", 0), 1);
+    assert_int_equal(peer_info_extract_uint(empty, "IV_COMP_STUB", 0xfe0d0d), 0xfe0d0d);
+    assert_int_equal(peer_info_extract_uint(invalid_proto, "IV_COMP_STUB", 5), 5);
+
+    assert_int_equal(peer_info_extract_uint(test_prefix, "NP", 23), 112);
+    assert_int_equal(peer_info_extract_uint(test_prefix, "PD", 23), 8);
+    assert_int_equal(peer_info_extract_uint(test_prefix, "P", 0xfe0d0d), 0xfe0d0d);
+    assert_int_equal(peer_info_extract_uint(peer_info_normal, "P", 23), 78);
+    assert_int_equal(peer_info_extract_uint(test_prefix, "UV_IV_PROTO", 23), 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);
+
+    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);
+    assert_string_equal(foo, "7");
+    assert_string_equal(foo_eq, "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 +910,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: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iad94e7a9d0b3be8a8db09e9a20eaac6041470ab6
Gerrit-Change-Number: 1800
Gerrit-PatchSet: 1
Gerrit-Owner: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel