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

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, &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;
+    }
+
+    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