[M] Change in openvpn[master]: oob: Add client PROBE_REPLY parser

Attention is currently required from: plaisthos.

Hello plaisthos,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/1745?usp=email

to review the following change.


Change subject: oob: Add client PROBE_REPLY parser
......................................................................

oob: Add client PROBE_REPLY parser

Add oob_client_reply_read(), the client-side counterpart of
oob_server_probe_read(): it scans the TLV payload of a received PROBE_REPLY
for the probe_reply TLV, skipping other/future TLV types.

Factor the shared TLV scan used by both into a static oob_find_tlv() helper
so the two readers no longer duplicate the loop.

Change-Id: If04ce09d4c353f5384c0c48f48f63e05869a373f
Signed-off-by: Lev Stipakov <[email protected]>
---
M src/openvpn/oob.c
M src/openvpn/oob.h
M tests/unit_tests/openvpn/test_oob.c
3 files changed, 130 insertions(+), 15 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/45/1745/1

diff --git a/src/openvpn/oob.c b/src/openvpn/oob.c
index f7af0ef..9cc7c42 100644
--- a/src/openvpn/oob.c
+++ b/src/openvpn/oob.c
@@ -160,31 +160,28 @@
            && oob_probe_parameter_write(buf, param);
 }
 
-bool
-oob_server_probe_read(struct buffer *payload, struct oob_probe_parameter *param)
+/* Scan @p payload for the first TLV of type @p wanted_type, skipping any other
+ * (e.g. future) TLV types. On success @p payload is left positioned at that
+ * TLV's value and *value_len is its declared length. Returns false if the TLV
+ * is not found or a header/length is malformed. */
+static bool
+oob_find_tlv(struct buffer *payload, uint16_t wanted_type, uint16_t *value_len)
 {
-    if (!oob_msg_read_header(payload, OOB_MSG_SERVER_PROBE))
-    {
-        return false;
-    }
-
-    /* A TLV header is 4 bytes (type + length). Loop until we either find the
-     * probe_parameter or run out of well-formed TLVs. */
+    /* A TLV header is 4 bytes (type + length). */
     while (BLEN(payload) >= 4)
     {
         uint16_t type;
         bool optional;
-        uint16_t value_len;
-        if (!oob_tlv_read_header(payload, &type, &optional, &value_len))
+        if (!oob_tlv_read_header(payload, &type, &optional, value_len))
         {
             return false;
         }
-        if (type == OOB_TLV_PROBE_PARAMETER)
+        if (type == wanted_type)
         {
-            return oob_probe_parameter_read(payload, param, value_len);
+            return true;
         }
-        /* not the TLV we are looking for: skip its value and keep scanning */
-        if (!buf_advance(payload, value_len))
+        /* not the TLV we want: skip its value and keep scanning */
+        if (!buf_advance(payload, *value_len))
         {
             return false;
         }
@@ -193,12 +190,30 @@
 }
 
 bool
+oob_server_probe_read(struct buffer *payload, struct oob_probe_parameter *param)
+{
+    uint16_t value_len;
+    return oob_msg_read_header(payload, OOB_MSG_SERVER_PROBE)
+           && oob_find_tlv(payload, OOB_TLV_PROBE_PARAMETER, &value_len)
+           && oob_probe_parameter_read(payload, param, value_len);
+}
+
+bool
 oob_client_reply_write(struct buffer *buf, const struct oob_probe_reply *reply)
 {
     return oob_msg_write_header(buf, OOB_MSG_PROBE_REPLY) && oob_probe_reply_write(buf, reply);
 }
 
 bool
+oob_client_reply_read(struct buffer *payload, struct oob_probe_reply *reply)
+{
+    uint16_t value_len;
+    return oob_msg_read_header(payload, OOB_MSG_PROBE_REPLY)
+           && oob_find_tlv(payload, OOB_TLV_PROBE_REPLY, &value_len)
+           && oob_probe_reply_read(payload, reply, value_len);
+}
+
+bool
 oob_timestamp_in_window(uint64_t probe_ts, uint64_t now, uint64_t window_secs)
 {
     uint64_t diff = (now > probe_ts) ? (now - probe_ts) : (probe_ts - now);
diff --git a/src/openvpn/oob.h b/src/openvpn/oob.h
index 93c44b0..9040416 100644
--- a/src/openvpn/oob.h
+++ b/src/openvpn/oob.h
@@ -170,6 +170,19 @@
 bool oob_client_reply_write(struct buffer *buf, const struct oob_probe_reply *reply);
 
 /**
+ * Read a received OOB PROBE_REPLY: verify its message-type header, then scan
+ * for the probe_reply TLV; the client-side counterpart of
+ * oob_server_probe_read(). TLV types other than probe_reply are skipped.
+ * @p payload is consumed as it is read.
+ *
+ * @param payload  buffer positioned at the start of the OOB message payload
+ * @param reply    filled with the parsed probe_reply on success
+ * @return true if the header matched and a well-formed probe_reply was found,
+ *         false otherwise
+ */
+bool oob_client_reply_read(struct buffer *payload, struct oob_probe_reply *reply);
+
+/**
  * Check whether a probe timestamp is within an acceptable window around the
  * current time. Used to cheaply drop replayed or implausibly-timed probes
  * before doing any further work (see the probe_parameter timestamp rationale
diff --git a/tests/unit_tests/openvpn/test_oob.c b/tests/unit_tests/openvpn/test_oob.c
index a9998d2..1029399 100644
--- a/tests/unit_tests/openvpn/test_oob.c
+++ b/tests/unit_tests/openvpn/test_oob.c
@@ -346,6 +346,89 @@
     gc_free(&gc);
 }
 
+/* A PROBE_REPLY carrying a probe_reply is found by the client scan, with all
+ * fields surviving. */
+static void
+test_client_reply_read_finds_reply(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    struct oob_probe_reply in = {
+        .priority = 5,
+        .weight = 50,
+        .connect_lifetime = 120,
+        .flags = 1,
+    };
+    memcpy(in.peer_session_id.id, "SRVREPLY", SID_SIZE);
+    assert_true(oob_client_reply_write(&buf, &in));
+
+    struct oob_probe_reply out = { 0 };
+    assert_true(oob_client_reply_read(&buf, &out));
+    assert_memory_equal(out.peer_session_id.id, in.peer_session_id.id, SID_SIZE);
+    assert_int_equal(out.priority, in.priority);
+    assert_int_equal(out.weight, in.weight);
+    assert_int_equal(out.connect_lifetime, in.connect_lifetime);
+    assert_int_equal(out.flags, in.flags);
+
+    gc_free(&gc);
+}
+
+/* TLVs other than probe_reply are skipped. */
+static void
+test_client_reply_read_skips_unknown(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    assert_true(oob_msg_write_header(&buf, OOB_MSG_PROBE_REPLY));
+    assert_true(oob_tlv_write_header(&buf, 0x7ff, false, 4));
+    assert_true(buf_write_u32(&buf, 0xabad1dea));
+    struct oob_probe_reply in = { .priority = 7 };
+    assert_true(oob_probe_reply_write(&buf, &in));
+
+    struct oob_probe_reply out = { 0 };
+    assert_true(oob_client_reply_read(&buf, &out));
+    assert_int_equal(out.priority, 7);
+
+    gc_free(&gc);
+}
+
+/* A payload with no probe_reply is rejected. */
+static void
+test_client_reply_read_missing(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    assert_true(oob_msg_write_header(&buf, OOB_MSG_PROBE_REPLY));
+    assert_true(oob_tlv_write_header(&buf, 0x7ff, false, 4));
+    assert_true(buf_write_u32(&buf, 0));
+
+    struct oob_probe_reply out = { 0 };
+    assert_false(oob_client_reply_read(&buf, &out));
+
+    gc_free(&gc);
+}
+
+/* Likewise, a PROBE_REPLY reader rejects a payload with the wrong message
+ * type even when a valid probe_reply TLV follows. */
+static void
+test_client_reply_read_wrong_msg_type(void **state)
+{
+    struct gc_arena gc = gc_new();
+    struct buffer buf = alloc_buf_gc(128, &gc);
+
+    assert_true(oob_msg_write_header(&buf, OOB_MSG_SERVER_PROBE));
+    struct oob_probe_reply in = { .priority = 7 };
+    assert_true(oob_probe_reply_write(&buf, &in));
+
+    struct oob_probe_reply out = { 0 };
+    assert_false(oob_client_reply_read(&buf, &out));
+
+    gc_free(&gc);
+}
+
 int
 main(void)
 {
@@ -365,6 +448,10 @@
         cmocka_unit_test(test_build_probe_reply_valid),
         cmocka_unit_test(test_build_probe_reply_stale),
         cmocka_unit_test(test_build_probe_reply_no_parameter),
+        cmocka_unit_test(test_client_reply_read_finds_reply),
+        cmocka_unit_test(test_client_reply_read_skips_unknown),
+        cmocka_unit_test(test_client_reply_read_missing),
+        cmocka_unit_test(test_client_reply_read_wrong_msg_type),
     };
 
     return cmocka_run_group_tests_name("oob tests", tests, NULL, NULL);

-- 
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1745?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: If04ce09d4c353f5384c0c48f48f63e05869a373f
Gerrit-Change-Number: 1745
Gerrit-PatchSet: 1
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>

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