[Openvpn-devel] [L] Change in openvpn[master]: oob: Add SERVER_PROBE parsing and the probe-reply decision

Attention is currently required from: flichtenheld, stipa.

Hello flichtenheld, plaisthos, 

I'd like you to reexamine a change. Please visit

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

to look at the new patch set (#9).

The following approvals got outdated and were removed:
Code-Review+2 by flichtenheld


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, 289 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/42/1742/9

diff --git a/src/openvpn/oob.c b/src/openvpn/oob.c
index c807171..930b213 100644
--- a/src/openvpn/oob.c
+++ b/src/openvpn/oob.c
@@ -81,3 +81,54 @@
     r->flags = buf_read_u32(buf, NULL);
     return true;
 }
+
+bool
+oob_server_probe_write(struct buffer *buf, const struct oob_probe_parameter *param)
+{
+    return buf_write_u16(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 (!ctrl_msg_read_header(payload, OOB_MSG_SERVER_PROBE))
+    {
+        return false;
+    }
+
+    struct buffer value;
+    if (!ctrl_msg_find_tlv(payload, OOB_TLV_PROBE_PARAMETER, &value))
+    {
+        return false;
+    }
+
+    return oob_probe_parameter_read(&value, param);
+}
+
+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, &param))
+    {
+        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;
+    }
+
+    /* the caller's advertised values are kept; only the echo is ours to fill */
+    reply->peer_session_id = *peer_sid;
+    return true;
+}
diff --git a/src/openvpn/oob.h b/src/openvpn/oob.h
index 0a9a4bb..1afabbd 100644
--- a/src/openvpn/oob.h
+++ b/src/openvpn/oob.h
@@ -107,4 +107,58 @@
  */
 bool oob_probe_reply_read(struct buffer *buf, struct oob_probe_reply *r);
 
+/**
+ * Write a complete SERVER_PROBE message (message-type header + probe_parameter
+ * TLV) to 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. 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.
+ *
+ * reply carries the values the server advertises (priority, weight,
+ * max_latency_diff, connect_lifetime, flags) in, and is deliberately not
+ * cleared: on success only the peer's session id is filled in, leaving the
+ * reply 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          in: the values to advertise; out: the reply to send
+ * @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 aa4eb23..371cbc6 100644
--- a/tests/unit_tests/openvpn/test_oob.c
+++ b/tests/unit_tests/openvpn/test_oob.c
@@ -259,6 +259,181 @@
     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(buf_write_u16(&buf, OOB_MSG_SERVER_PROBE));
+    assert_true(ctrl_msg_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(buf_write_u16(&buf, OOB_MSG_SERVER_PROBE));
+    assert_true(ctrl_msg_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(buf_write_u16(&buf, OOB_MSG_SERVER_PROBE));
+    assert_true(ctrl_msg_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(buf_write_u16(&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);
+
+    /* left as the caller set them: the function only fills the session id */
+    struct oob_probe_reply reply = { 0 };
+    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 = { 0 };
+    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(buf_write_u16(&buf, OOB_MSG_SERVER_PROBE));
+    assert_true(ctrl_msg_tlv_write_header(&buf, 0x7ff, false, 4));
+    assert_true(buf_write_u32(&buf, 0));
+
+    struct session_id peer = { 0 };
+    struct oob_probe_reply reply = { 0 };
+    assert_false(oob_build_probe_reply(&buf, 1000000, 30, &peer, &reply));
+
+    gc_free(&gc);
+}
+
 int
 main(void)
 {
@@ -271,6 +446,15 @@
         cmocka_unit_test(test_probe_parameter_too_short),
         cmocka_unit_test(test_find_tlv_value_truncated),
         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: newpatchset
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: Iafa9efc1c156974ad9f5752e9de810a8c2f68c30
Gerrit-Change-Number: 1742
Gerrit-PatchSet: 9
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
Gerrit-Attention: stipa <[email protected]>

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