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/+/1742?usp=email
to review the following change.
Change subject: oob: Add SERVER_PROBE parsing and the probe-reply decision
......................................................................
oob: Add SERVER_PROBE parsing and the probe-reply decision
Add the message-level helpers the server needs to answer a probe:
- oob_server_probe_read() scans the TLV payload of a received OOB
SERVER_PROBE for the probe_parameter TLV, skipping other/future TLV
types and rejecting malformed or truncated input.
- oob_timestamp_in_window() performs the cheap replay/skew check on the
probe timestamp described in the wire protocol specification.
- oob_build_probe_reply() composes the two: given a received SERVER_PROBE
it scans for the probe_parameter, drops the probe if it is missing or
its timestamp is outside the acceptable window, and otherwise populates
the probe_reply (echoing the peer's session id) to send back.
These are pure functions exercised by unit tests; the server packet path
calls oob_build_probe_reply() in a follow-up, which performs the actual send.
Change-Id: Iafa9efc1c156974ad9f5752e9de810a8c2f68c30
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, 303 insertions(+), 0 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/42/1742/1
diff --git a/src/openvpn/oob.c b/src/openvpn/oob.c
index 9f123e2..58d47c0 100644
--- a/src/openvpn/oob.c
+++ b/src/openvpn/oob.c
@@ -152,3 +152,71 @@
r->max_latency_diff = (uint16_t)max_latency_diff;
return oob_skip_trailing(buf, value_len, OOB_PROBE_REPLY_LEN);
}
+
+bool
+oob_server_probe_write(struct buffer *buf, const struct oob_probe_parameter
*param)
+{
+ return oob_msg_write_header(buf, OOB_MSG_SERVER_PROBE)
+ && oob_probe_parameter_write(buf, param);
+}
+
+bool
+oob_server_probe_read(struct buffer *payload, struct oob_probe_parameter
*param)
+{
+ 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. */
+ while (BLEN(payload) >= 4)
+ {
+ uint16_t type;
+ bool optional;
+ uint16_t value_len;
+ if (!oob_tlv_read_header(payload, &type, &optional, &value_len))
+ {
+ return false;
+ }
+ if (type == OOB_TLV_PROBE_PARAMETER)
+ {
+ return oob_probe_parameter_read(payload, param, value_len);
+ }
+ /* not the TLV we are looking for: skip its value and keep scanning */
+ if (!buf_advance(payload, value_len))
+ {
+ return false;
+ }
+ }
+ return false;
+}
+
+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);
+ return diff <= window_secs;
+}
+
+bool
+oob_build_probe_reply(struct buffer *probe_payload, uint64_t now, uint64_t
window_secs,
+ const struct session_id *peer_sid, struct
oob_probe_reply *reply)
+{
+ struct oob_probe_parameter param;
+ if (!oob_server_probe_read(probe_payload, ¶m))
+ {
+ return false;
+ }
+
+ /* Drop replayed or implausibly-timed probes before doing any more work. */
+ if (!oob_timestamp_in_window(param.timestamp, now, window_secs))
+ {
+ return false;
+ }
+
+ memset(reply, 0, sizeof(*reply));
+ reply->peer_session_id = *peer_sid;
+ /* priority/weight/connect_lifetime/flags left at 0 for now */
+ return true;
+}
diff --git a/src/openvpn/oob.h b/src/openvpn/oob.h
index c32a7db..48fcfa1 100644
--- a/src/openvpn/oob.h
+++ b/src/openvpn/oob.h
@@ -144,4 +144,56 @@
*/
bool oob_probe_reply_read(struct buffer *buf, struct oob_probe_reply *r,
uint16_t value_len);
+/**
+ * Write a complete SERVER_PROBE message (message-type header + probe_parameter
+ * TLV) to @p buf. Sent by the client.
+ */
+bool oob_server_probe_write(struct buffer *buf, const struct
oob_probe_parameter *param);
+
+/**
+ * Read a received OOB SERVER_PROBE: verify its message-type header, then scan
+ * for the probe_parameter TLV. TLV types other than probe_parameter are
+ * skipped, so the scan tolerates additional/future TLVs. @p payload is
consumed
+ * as it is read.
+ *
+ * @param payload buffer positioned at the start of the OOB message payload
+ * @param param filled with the parsed probe_parameter on success
+ * @return true if the header matched and a well-formed probe_parameter was
+ * found, false otherwise
+ */
+bool oob_server_probe_read(struct buffer *payload, struct oob_probe_parameter
*param);
+
+/**
+ * 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
+ * in the wire protocol specification).
+ *
+ * @param probe_ts timestamp from the probe_parameter (UNIX seconds)
+ * @param now current time (UNIX seconds)
+ * @param window_secs maximum allowed difference, in either direction
+ * @return true if |now - probe_ts| <= window_secs
+ */
+bool oob_timestamp_in_window(uint64_t probe_ts, uint64_t now, uint64_t
window_secs);
+
+/**
+ * Process the TLV payload of a received SERVER_PROBE and decide whether to
+ * answer it. Combines oob_server_probe_read() and oob_timestamp_in_window():
+ * the probe is dropped (false returned) if it has no valid probe_parameter or
+ * its timestamp is outside the acceptable window. On success @p reply is
+ * populated with the peer's session id echoed back and the remaining fields
+ * zeroed, ready to be wrapped and sent.
+ *
+ * This is the transport-agnostic decision step; the caller performs the send.
+ *
+ * @param probe_payload TLV payload of the received OOB SERVER_PROBE
+ * @param now current time (UNIX seconds)
+ * @param window_secs acceptable timestamp skew, in either direction
+ * @param peer_sid session id of the requesting peer (echoed in the
reply)
+ * @param reply filled with the reply to send on success
+ * @return true if a reply should be sent, false to silently drop the probe
+ */
+bool oob_build_probe_reply(struct buffer *probe_payload, uint64_t now,
uint64_t window_secs,
+ const struct session_id *peer_sid, struct
oob_probe_reply *reply);
+
#endif /* OOB_H */
diff --git a/tests/unit_tests/openvpn/test_oob.c
b/tests/unit_tests/openvpn/test_oob.c
index 40dcaf9..a9998d2 100644
--- a/tests/unit_tests/openvpn/test_oob.c
+++ b/tests/unit_tests/openvpn/test_oob.c
@@ -172,6 +172,180 @@
gc_free(&gc);
}
+/* A SERVER_PROBE carrying just a probe_parameter is found by the scan. */
+static void
+test_server_probe_read_finds_parameter(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ const struct oob_probe_parameter in = {
+ .timestamp = 0x1122334455667788ULL,
+ .flags = 0,
+ };
+ assert_true(oob_server_probe_write(&buf, &in));
+
+ struct oob_probe_parameter out = { 0 };
+ assert_true(oob_server_probe_read(&buf, &out));
+ assert_true(in.timestamp == out.timestamp);
+ assert_int_equal(in.flags, out.flags);
+
+ gc_free(&gc);
+}
+
+/* TLVs other than probe_parameter are skipped, so the scan finds the
+ * probe_parameter even when preceded by an unknown TLV. */
+static void
+test_server_probe_read_skips_unknown(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ /* SERVER_PROBE message header, then an unknown TLV (type 0x7ff) ... */
+ assert_true(oob_msg_write_header(&buf, OOB_MSG_SERVER_PROBE));
+ assert_true(oob_tlv_write_header(&buf, 0x7ff, false, 4));
+ assert_true(buf_write_u32(&buf, 0xcafef00d));
+ /* ... followed by the real probe_parameter */
+ const struct oob_probe_parameter in = { .timestamp = 42, .flags = 0 };
+ assert_true(oob_probe_parameter_write(&buf, &in));
+
+ struct oob_probe_parameter out = { 0 };
+ assert_true(oob_server_probe_read(&buf, &out));
+ assert_true(out.timestamp == 42);
+
+ gc_free(&gc);
+}
+
+/* A payload with no probe_parameter must be rejected. */
+static void
+test_server_probe_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_SERVER_PROBE));
+ assert_true(oob_tlv_write_header(&buf, 0x7ff, false, 4));
+ assert_true(buf_write_u32(&buf, 0));
+
+ struct oob_probe_parameter out = { 0 };
+ assert_false(oob_server_probe_read(&buf, &out));
+
+ gc_free(&gc);
+}
+
+/* A TLV whose declared length runs past the buffer must be rejected, not
+ * read out of bounds. */
+static void
+test_server_probe_read_truncated(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ /* TLV header claims a 16-byte value but no value bytes follow */
+ assert_true(oob_msg_write_header(&buf, OOB_MSG_SERVER_PROBE));
+ assert_true(oob_tlv_write_header(&buf, 0x7ff, false, 16));
+
+ struct oob_probe_parameter out = { 0 };
+ assert_false(oob_server_probe_read(&buf, &out));
+
+ gc_free(&gc);
+}
+
+/* A SERVER_PROBE reader rejects a payload carrying a different message type
+ * (here a PROBE_REPLY's), even if it contains a valid probe_parameter TLV. */
+static void
+test_server_probe_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_PROBE_REPLY));
+ const struct oob_probe_parameter in = { .timestamp = 42, .flags = 0 };
+ assert_true(oob_probe_parameter_write(&buf, &in));
+
+ struct oob_probe_parameter out = { 0 };
+ assert_false(oob_server_probe_read(&buf, &out));
+
+ gc_free(&gc);
+}
+
+/* Timestamp window check accepts values within the window (either direction)
+ * and rejects values outside it. */
+static void
+test_timestamp_in_window(void **state)
+{
+ const uint64_t now = 1000000;
+ const uint64_t window = 30;
+
+ assert_true(oob_timestamp_in_window(now, now, window));
+ assert_true(oob_timestamp_in_window(now - window, now, window)); /*
boundary, past */
+ assert_true(oob_timestamp_in_window(now + window, now, window)); /*
boundary, future */
+ assert_false(oob_timestamp_in_window(now - window - 1, now, window)); /*
too old */
+ assert_false(oob_timestamp_in_window(now + window + 1, now, window)); /*
too far ahead */
+}
+
+/* A valid, in-window SERVER_PROBE yields a reply that echoes the peer's
+ * session id and zeroes the remaining fields. */
+static void
+test_build_probe_reply_valid(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ const uint64_t now = 1000000;
+ const struct oob_probe_parameter probe = { .timestamp = now, .flags = 0 };
+ assert_true(oob_server_probe_write(&buf, &probe));
+
+ struct session_id peer;
+ memcpy(peer.id, "PEER1234", SID_SIZE);
+
+ struct oob_probe_reply reply;
+ assert_true(oob_build_probe_reply(&buf, now, 30, &peer, &reply));
+ assert_memory_equal(reply.peer_session_id.id, peer.id, SID_SIZE);
+ assert_int_equal(reply.priority, 0);
+ assert_int_equal(reply.weight, 0);
+ assert_int_equal(reply.connect_lifetime, 0);
+ assert_int_equal(reply.flags, 0);
+
+ gc_free(&gc);
+}
+
+/* A probe whose timestamp is outside the window is dropped (no reply). */
+static void
+test_build_probe_reply_stale(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ const uint64_t now = 1000000;
+ const struct oob_probe_parameter probe = { .timestamp = now - 1000, .flags
= 0 };
+ assert_true(oob_server_probe_write(&buf, &probe));
+
+ struct session_id peer = { 0 };
+ struct oob_probe_reply reply;
+ assert_false(oob_build_probe_reply(&buf, now, 30, &peer, &reply));
+
+ gc_free(&gc);
+}
+
+/* A payload without a probe_parameter is dropped (no reply). */
+static void
+test_build_probe_reply_no_parameter(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));
+ assert_true(oob_tlv_write_header(&buf, 0x7ff, false, 4));
+ assert_true(buf_write_u32(&buf, 0));
+
+ struct session_id peer = { 0 };
+ struct oob_probe_reply reply;
+ assert_false(oob_build_probe_reply(&buf, 1000000, 30, &peer, &reply));
+
+ gc_free(&gc);
+}
+
int
main(void)
{
@@ -182,6 +356,15 @@
cmocka_unit_test(test_probe_parameter_forward_compat),
cmocka_unit_test(test_probe_parameter_too_short),
cmocka_unit_test(test_tlv_header_truncated),
+ cmocka_unit_test(test_server_probe_read_finds_parameter),
+ cmocka_unit_test(test_server_probe_read_skips_unknown),
+ cmocka_unit_test(test_server_probe_read_missing),
+ cmocka_unit_test(test_server_probe_read_truncated),
+ cmocka_unit_test(test_server_probe_read_wrong_msg_type),
+ cmocka_unit_test(test_timestamp_in_window),
+ 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),
};
return cmocka_run_group_tests_name("oob tests", tests, NULL, NULL);
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1742?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: Iafa9efc1c156974ad9f5752e9de810a8c2f68c30
Gerrit-Change-Number: 1742
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
