Attention is currently required from: flichtenheld, plaisthos, ralf_lici.
Hello flichtenheld, plaisthos, ralf_lici,
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 (#19).
Change subject: oob: Add probe request parsing and the probe-reply decision
......................................................................
oob: Add probe request parsing and the probe-reply decision
Add the message-level helpers the server needs to answer a probe:
oob_probe_request_find() for the probe request TLV,
oob_timestamp_in_window() for the replay/skew check on its timestamp,
and oob_probe_request_check(), which combines the two and classifies a
received probe as invalid, stale or OK.
The reader returns the probe request, whose request_id the reply
echoes, so a client can tell which of its probes a reply answers.
A stale probe, one whose timestamp is outside the window, is reported
separately rather than dropped: the timestamp lets a server drop
replayed requests, but a client with a skewed clock sends the same
thing, so the server path decides (a follow-up answers a few per
period). The window itself is the caller's choice; the server path
passes --hand-window.
These are pure functions exercised by unit tests; the server packet
path uses them in a follow-up and builds and sends the reply itself.
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, 271 insertions(+), 0 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/42/1742/19
diff --git a/src/openvpn/oob.c b/src/openvpn/oob.c
index 5f624be..954694a 100644
--- a/src/openvpn/oob.c
+++ b/src/openvpn/oob.c
@@ -83,3 +83,30 @@
r->flags = (uint16_t)buf_read_u16(buf);
return true;
}
+
+bool
+oob_probe_request_find(struct buffer *payload, struct oob_probe_request *req)
+{
+ struct buffer value;
+ return ctrl_msg_find_tlv(payload, TLV_TYPE_PROBE_REQUEST, &value)
+ && oob_probe_request_read(&value, req);
+}
+
+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;
+}
+
+enum oob_probe_verdict
+oob_probe_request_check(struct buffer *probe_payload, uint64_t now, uint64_t
window_secs,
+ struct oob_probe_request *req)
+{
+ if (!oob_probe_request_find(probe_payload, req))
+ {
+ return OOB_PROBE_INVALID;
+ }
+ return oob_timestamp_in_window(req->timestamp, now, window_secs) ?
OOB_PROBE_OK
+ :
OOB_PROBE_STALE;
+}
diff --git a/src/openvpn/oob.h b/src/openvpn/oob.h
index e3e4144..95b3afc 100644
--- a/src/openvpn/oob.h
+++ b/src/openvpn/oob.h
@@ -99,4 +99,49 @@
*/
bool oob_probe_reply_read(struct buffer *buf, struct oob_probe_reply *r);
+/**
+ * Scan the payload of a received OOB message for the probe request TLV.
+ * Unknown TLV types marked optional are skipped; an unknown mandatory one
+ * rejects the message. payload is consumed as it is read.
+ *
+ * @param payload buffer positioned at the start of the OOB message payload
+ * @param req filled with the parsed probe request on success
+ * @return true if a well-formed probe request was found, false otherwise
+ */
+bool oob_probe_request_find(struct buffer *payload, struct oob_probe_request
*req);
+
+/**
+ * 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 request timestamp rationale
+ * in the wire protocol specification).
+ *
+ * @param probe_ts timestamp from the probe request (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);
+
+enum oob_probe_verdict
+{
+ OOB_PROBE_INVALID, /**< no valid probe request: drop */
+ OOB_PROBE_STALE, /**< well-formed, timestamp outside the window */
+ OOB_PROBE_OK, /**< well-formed, timestamp within the window */
+};
+
+/**
+ * Classify a received probe request. Combines oob_probe_request_find() and
+ * oob_timestamp_in_window(). This is the transport-agnostic decision step;
+ * the caller decides what to do with a stale probe and builds the reply.
+ *
+ * @param probe_payload payload of the received OOB message, consumed
+ * @param now current time (UNIX seconds)
+ * @param window_secs acceptable timestamp skew, in either direction
+ * @param req filled with the probe request unless the verdict is
+ * OOB_PROBE_INVALID, for the reply to echo its
request_id
+ */
+enum oob_probe_verdict oob_probe_request_check(struct buffer *probe_payload,
uint64_t now,
+ uint64_t window_secs, struct
oob_probe_request *req);
+
#endif /* OOB_H */
diff --git a/tests/unit_tests/openvpn/test_oob.c
b/tests/unit_tests/openvpn/test_oob.c
index 4e06cfd..8af595d 100644
--- a/tests/unit_tests/openvpn/test_oob.c
+++ b/tests/unit_tests/openvpn/test_oob.c
@@ -338,6 +338,195 @@
gc_free(&gc);
}
+/* A payload of just a probe request is found by the scan, along with its
+ * request_id. */
+static void
+test_probe_request_find(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ const struct oob_probe_request in = {
+ .request_id = 42,
+ .timestamp = 0x1122334455667788ULL,
+ .flags = 0,
+ };
+ assert_true(oob_probe_request_write(&buf, &in));
+
+ struct oob_probe_request out = { 0 };
+ assert_true(oob_probe_request_find(&buf, &out));
+ assert_int_equal(out.request_id, 42);
+ assert_true(in.timestamp == out.timestamp);
+ assert_int_equal(in.flags, out.flags);
+
+ gc_free(&gc);
+}
+
+/* TLVs other than probe request are skipped, so the scan finds the
+ * probe request even when preceded by an unknown TLV. */
+static void
+test_probe_request_find_skips_unknown(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ /* An unknown but optional TLV (type 0x7ff) ... */
+ assert_true(ctrl_msg_tlv_write_header(&buf, 0x7ff, true, 4));
+ assert_true(buf_write_u32(&buf, 0xcafef00d));
+ /* ... followed by the real probe request */
+ const struct oob_probe_request in = { .timestamp = 42, .flags = 0 };
+ assert_true(oob_probe_request_write(&buf, &in));
+
+ struct oob_probe_request out = { 0 };
+ assert_true(oob_probe_request_find(&buf, &out));
+ assert_true(out.timestamp == 42);
+
+ gc_free(&gc);
+}
+
+/* An unknown TLV that is NOT marked optional carries something the sender
+ * requires us to act on: the whole message is rejected, whether that TLV comes
+ * before or after the one we were looking for. */
+static void
+test_probe_request_find_rejects_unknown_mandatory(void **state)
+{
+ struct gc_arena gc = gc_new();
+ const struct oob_probe_request in = { .timestamp = 42, .flags = 0 };
+
+ struct buffer before = alloc_buf_gc(128, &gc);
+ assert_true(ctrl_msg_tlv_write_header(&before, 0x7ff, false, 4));
+ assert_true(buf_write_u32(&before, 0xcafef00d));
+ assert_true(oob_probe_request_write(&before, &in));
+
+ struct buffer after = alloc_buf_gc(128, &gc);
+ assert_true(oob_probe_request_write(&after, &in));
+ assert_true(ctrl_msg_tlv_write_header(&after, 0x7ff, false, 4));
+ assert_true(buf_write_u32(&after, 0xcafef00d));
+
+ struct oob_probe_request out = { 0 };
+ assert_false(oob_probe_request_find(&before, &out));
+ assert_false(oob_probe_request_find(&after, &out));
+
+ gc_free(&gc);
+}
+
+/* A payload with no probe request must be rejected. */
+static void
+test_probe_request_find_missing(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ assert_true(ctrl_msg_tlv_write_header(&buf, 0x7ff, true, 4));
+ assert_true(buf_write_u32(&buf, 0));
+
+ struct oob_probe_request out = { 0 };
+ assert_false(oob_probe_request_find(&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_probe_request_find_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(ctrl_msg_tlv_write_header(&buf, 0x7ff, false, 16));
+
+ struct oob_probe_request out = { 0 };
+ assert_false(oob_probe_request_find(&buf, &out));
+
+ gc_free(&gc);
+}
+
+/* A payload carrying a probe reply instead holds no probe request. */
+static void
+test_probe_request_find_reply_tlv(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ const struct oob_probe_reply in = { .request_id = 42 };
+ assert_true(oob_probe_reply_write(&buf, &in));
+
+ struct oob_probe_request out = { 0 };
+ assert_false(oob_probe_request_find(&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 well-formed probe with an in-window timestamp is answered. */
+static void
+test_probe_request_check_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_request probe = { .request_id = 1, .timestamp =
now, .flags = 0 };
+ assert_true(oob_probe_request_write(&buf, &probe));
+
+ struct oob_probe_request req = { 0 };
+ assert_int_equal(oob_probe_request_check(&buf, now, 30, &req),
OOB_PROBE_OK);
+ assert_int_equal(req.request_id, 1);
+
+ gc_free(&gc);
+}
+
+/* A well-formed probe whose timestamp is outside the window is stale; the
+ * caller decides whether it still gets an answer. */
+static void
+test_probe_request_check_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_request probe = { .request_id = 7, .timestamp = now
- 1000, .flags = 0 };
+ assert_true(oob_probe_request_write(&buf, &probe));
+
+ struct oob_probe_request req = { 0 };
+ assert_int_equal(oob_probe_request_check(&buf, now, 30, &req),
OOB_PROBE_STALE);
+ assert_int_equal(req.request_id, 7); /* a stale probe may still be
answered */
+
+ gc_free(&gc);
+}
+
+/* A payload without a probe request is invalid (no reply). */
+static void
+test_probe_request_check_no_request(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ assert_true(ctrl_msg_tlv_write_header(&buf, 0x7ff, true, 4));
+ assert_true(buf_write_u32(&buf, 0));
+
+ struct oob_probe_request req;
+ assert_int_equal(oob_probe_request_check(&buf, 1000000, 30, &req),
OOB_PROBE_INVALID);
+
+ gc_free(&gc);
+}
+
/* The OOB tests run as a second group of pkt_testdriver; see test_pkt.c. */
int
run_oob_tests(void)
@@ -353,6 +542,16 @@
cmocka_unit_test(test_find_tlv_trailing_bytes),
cmocka_unit_test(test_find_tlv_skips_optional_after_wanted),
cmocka_unit_test(test_find_tlv_empty_payload),
+ cmocka_unit_test(test_probe_request_find),
+ cmocka_unit_test(test_probe_request_find_skips_unknown),
+ cmocka_unit_test(test_probe_request_find_rejects_unknown_mandatory),
+ cmocka_unit_test(test_probe_request_find_missing),
+ cmocka_unit_test(test_probe_request_find_truncated),
+ cmocka_unit_test(test_probe_request_find_reply_tlv),
+ cmocka_unit_test(test_timestamp_in_window),
+ cmocka_unit_test(test_probe_request_check_valid),
+ cmocka_unit_test(test_probe_request_check_stale),
+ cmocka_unit_test(test_probe_request_check_no_request),
};
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: 19
Gerrit-Owner: stipa <[email protected]>
Gerrit-Reviewer: flichtenheld <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-Reviewer: ralf_lici <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: ralf_lici <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
Gerrit-Attention: flichtenheld <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
