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
