Attention is currently required from: Arne Schwabe, Frank Lichtenheld, Ralf
Lici.
Hello Arne Schwabe, Frank Lichtenheld, Ralf Lici,
I'd like you to reexamine a change. Please visit
https://gerrit.openvpn.net/c/openvpn/+/1742?usp=email
to look at the new patch set (#22).
Change subject: probe: Add probe request lookup and the probe-reply decision
......................................................................
probe: Add probe request lookup and the probe-reply decision
Add the message-level helpers a server needs before it can answer a
probe: probe_request_find() picks the probe request TLV out of the
payload of a received P_CONTROL_V2 message, probe_timestamp_in_window()
checks its timestamp against the window the wire protocol describes,
and probe_request_check() combines the two and classifies the message
as invalid, stale or OK.
The request itself is handed back, because the reply has to echo its
request_id, which is how a client tells which of its probes a reply
answers.
A stale request, one whose timestamp is outside the window, is reported
apart from an invalid one rather than dropped here. The timestamp is
what bounds how long a captured request can be replayed against the
server, but a client with an unsynchronised clock sends something that
looks just like a replay, so what to do about it is the caller's
decision: a follow-up answers a few of them per period. The width of
the window is the caller's too; the server path passes --hand-window.
These are pure functions, covered by unit tests. The server packet path
uses them in a follow-up, which also builds and sends the reply.
Change-Id: Iafa9efc1c156974ad9f5752e9de810a8c2f68c30
Signed-off-by: Lev Stipakov <[email protected]>
---
M src/openvpn/probe.c
M src/openvpn/probe.h
M tests/unit_tests/openvpn/test_pkt.c
M tests/unit_tests/openvpn/test_probe.c
M tests/unit_tests/openvpn/test_probe.h
5 files changed, 289 insertions(+), 1 deletion(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/42/1742/22
diff --git a/src/openvpn/probe.c b/src/openvpn/probe.c
index 2497b07..b59c654 100644
--- a/src/openvpn/probe.c
+++ b/src/openvpn/probe.c
@@ -92,3 +92,31 @@
}
return true;
}
+
+bool
+probe_request_find(struct buffer *payload, struct probe_request *req)
+{
+ struct buffer value;
+ return ctrl_msg_find_tlv(payload, TLV_TYPE_PROBE_REQUEST, &value)
+ && probe_request_read(&value, req);
+}
+
+bool
+probe_timestamp_in_window(uint64_t probe_ts, uint64_t now, uint64_t
window_secs)
+{
+ /* both clocks are unsigned and either may be ahead, so subtract the
+ * smaller from the larger rather than letting the difference wrap */
+ uint64_t diff = (now > probe_ts) ? (now - probe_ts) : (probe_ts - now);
+ return diff <= window_secs;
+}
+
+enum probe_verdict
+probe_request_check(struct buffer *payload, uint64_t now, uint64_t window_secs,
+ struct probe_request *req)
+{
+ if (!probe_request_find(payload, req))
+ {
+ return PROBE_INVALID;
+ }
+ return probe_timestamp_in_window(req->timestamp, now, window_secs) ?
PROBE_OK : PROBE_STALE;
+}
diff --git a/src/openvpn/probe.h b/src/openvpn/probe.h
index e8b0299..6acffbc 100644
--- a/src/openvpn/probe.h
+++ b/src/openvpn/probe.h
@@ -138,4 +138,61 @@
*/
bool probe_reply_read(struct buffer *buf, struct probe_reply *r);
+/**
+ * Find the probe request TLV in the payload of a received P_CONTROL_V2
+ * message and parse it.
+ *
+ * A TLV of another type is skipped if it is marked optional and rejects the
+ * whole message if it is not, as ctrl_msg_find_tlv() defines. payload is
+ * consumed as it is read.
+ *
+ * @param payload the message payload, positioned at the first TLV
+ * @param req filled with the probe request on success
+ * @return true if the payload holds exactly one well-formed probe request
+ */
+bool probe_request_find(struct buffer *payload, struct probe_request *req);
+
+/**
+ * Is the timestamp of a probe request close enough to our own clock?
+ *
+ * This is how a server bounds the age of the requests it answers, and with it
+ * for how long a captured request can be replayed against it. The window is
+ * the caller's to choose: it has to be wide enough for a client that has no
+ * proper clock synchronisation, so it cannot be tight.
+ *
+ * @param probe_ts the timestamp from the probe request, UNIX seconds
+ * @param now the current time, UNIX seconds
+ * @param window_secs the largest difference allowed, in either direction
+ * @return true if probe_ts and now are no more than window_secs apart
+ */
+bool probe_timestamp_in_window(uint64_t probe_ts, uint64_t now, uint64_t
window_secs);
+
+/** what probe_request_check() made of a received message */
+enum probe_verdict
+{
+ PROBE_INVALID, /**< it holds no probe request, so there is nothing to
answer */
+ PROBE_STALE, /**< a probe request, but its timestamp is outside the
window */
+ PROBE_OK, /**< a probe request whose timestamp is inside the window */
+};
+
+/**
+ * Classify the payload of a received P_CONTROL_V2 message:
probe_request_find()
+ * followed by probe_timestamp_in_window().
+ *
+ * A stale request is reported apart from an invalid one rather than folded
+ * into it, because the two deserve different treatment. A replayed request and
+ * one from a client with an unsynchronised clock are indistinguishable here,
so
+ * a server that dropped every stale request would lock that client out. What
to
+ * do instead is left to the caller, which also builds the reply.
+ *
+ * @param payload the message payload, consumed as it is read
+ * @param now the current time, UNIX seconds
+ * @param window_secs the largest difference allowed, in either direction
+ * @param req filled with the probe request unless the verdict is
+ * PROBE_INVALID, so that a reply can echo its request_id
+ * @return what the payload turned out to be
+ */
+enum probe_verdict probe_request_check(struct buffer *payload, uint64_t now,
uint64_t window_secs,
+ struct probe_request *req);
+
#endif /* ifndef PROBE_H */
diff --git a/tests/unit_tests/openvpn/test_pkt.c
b/tests/unit_tests/openvpn/test_pkt.c
index 75a1b31..0cb2ea5 100644
--- a/tests/unit_tests/openvpn/test_pkt.c
+++ b/tests/unit_tests/openvpn/test_pkt.c
@@ -856,7 +856,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_rejects_duplicate),
- cmocka_unit_test(test_find_tlv_empty_payload)
+ 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_reply_tlv),
+ cmocka_unit_test(test_probe_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("pkt tests", tests, NULL, NULL);
diff --git a/tests/unit_tests/openvpn/test_probe.c
b/tests/unit_tests/openvpn/test_probe.c
index 7cfb63d..a3a9d3a 100644
--- a/tests/unit_tests/openvpn/test_probe.c
+++ b/tests/unit_tests/openvpn/test_probe.c
@@ -400,3 +400,179 @@
gc_free(&gc);
}
+
+/* A payload of nothing but a probe request is found, fields and all. */
+void
+test_probe_request_find(void **state)
+{
+ struct gc_arena gc = gc_new();
+ struct buffer buf = alloc_buf_gc(128, &gc);
+
+ const struct probe_request in = {
+ .request_id = 0xdeadbeefcafef00dULL,
+ .timestamp = 0x1122334455667788ULL,
+ .flags = PROBE_REQUEST_FLAG_PROBE_ONLY,
+ };
+ assert_true(probe_request_write(&buf, &in));
+
+ struct probe_request out = { 0 };
+ assert_true(probe_request_find(&buf, &out));
+ assert_true(out.request_id == in.request_id);
+ assert_true(out.timestamp == in.timestamp);
+ assert_int_equal(out.flags, in.flags);
+
+ gc_free(&gc);
+}
+
+/* A TLV of another type that is marked optional is skipped, so the request is
+ * still found behind one. */
+void
+test_probe_request_find_skips_unknown(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, 0xcafef00d));
+ const struct probe_request in = { .timestamp = 42 };
+ assert_true(probe_request_write(&buf, &in));
+
+ struct probe_request out = { 0 };
+ assert_true(probe_request_find(&buf, &out));
+ assert_true(out.timestamp == 42);
+
+ gc_free(&gc);
+}
+
+/* A TLV we do not know that is not marked optional carries something its
+ * sender requires us to act on, so the message is rejected whether that TLV
+ * comes before or after the request. */
+void
+test_probe_request_find_rejects_unknown_mandatory(void **state)
+{
+ struct gc_arena gc = gc_new();
+ const struct probe_request in = { .timestamp = 42 };
+
+ 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(probe_request_write(&before, &in));
+
+ struct buffer after = alloc_buf_gc(128, &gc);
+ assert_true(probe_request_write(&after, &in));
+ assert_true(ctrl_msg_tlv_write_header(&after, 0x7ff, false, 4));
+ assert_true(buf_write_u32(&after, 0xcafef00d));
+
+ struct probe_request out = { 0 };
+ assert_false(probe_request_find(&before, &out));
+ assert_false(probe_request_find(&after, &out));
+
+ gc_free(&gc);
+}
+
+/* A payload that holds no probe request at all. */
+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 probe_request out = { 0 };
+ assert_false(probe_request_find(&buf, &out));
+
+ gc_free(&gc);
+}
+
+/* A probe reply is not a probe request: a server handed one must not answer
+ * it. The reply TLV is mandatory, so it rejects the message rather than
+ * being skipped. */
+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 probe_reply in = { .request_id = 42 };
+ assert_true(probe_reply_write(&buf, &in));
+
+ struct probe_request out = { 0 };
+ assert_false(probe_request_find(&buf, &out));
+
+ gc_free(&gc);
+}
+
+/* The window is inclusive and symmetric: a client clock that is behind is
+ * treated like one that is ahead. */
+void
+test_probe_timestamp_in_window(void **state)
+{
+ const uint64_t now = 1000000;
+ const uint64_t window = 30;
+
+ assert_true(probe_timestamp_in_window(now, now, window));
+ assert_true(probe_timestamp_in_window(now - window, now, window));
+ assert_true(probe_timestamp_in_window(now + window, now, window));
+ assert_false(probe_timestamp_in_window(now - window - 1, now, window));
+ assert_false(probe_timestamp_in_window(now + window + 1, now, window));
+
+ /* a timestamp of 0, and one near the end of the range, must not wrap into
+ * the window */
+ assert_false(probe_timestamp_in_window(0, now, window));
+ assert_false(probe_timestamp_in_window(UINT64_MAX, now, window));
+}
+
+/* A well-formed request with a timestamp inside the window. */
+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 probe_request in = { .request_id = 1, .timestamp = now };
+ assert_true(probe_request_write(&buf, &in));
+
+ struct probe_request req = { 0 };
+ assert_int_equal(probe_request_check(&buf, now, 30, &req), PROBE_OK);
+ assert_true(req.request_id == 1);
+
+ gc_free(&gc);
+}
+
+/* One whose timestamp is outside it is stale rather than invalid, and is
+ * still handed back so that the caller can answer it if it decides to. */
+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 probe_request in = { .request_id = 7, .timestamp = now - 1000
};
+ assert_true(probe_request_write(&buf, &in));
+
+ struct probe_request req = { 0 };
+ assert_int_equal(probe_request_check(&buf, now, 30, &req), PROBE_STALE);
+ assert_true(req.request_id == 7);
+
+ gc_free(&gc);
+}
+
+/* A payload with no request in it has nothing to answer. */
+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 probe_request req;
+ assert_int_equal(probe_request_check(&buf, 1000000, 30, &req),
PROBE_INVALID);
+
+ gc_free(&gc);
+}
diff --git a/tests/unit_tests/openvpn/test_probe.h
b/tests/unit_tests/openvpn/test_probe.h
index bc51721..d9d8f31 100644
--- a/tests/unit_tests/openvpn/test_probe.h
+++ b/tests/unit_tests/openvpn/test_probe.h
@@ -48,4 +48,22 @@
void test_find_tlv_rejects_duplicate(void **state);
void test_find_tlv_empty_payload(void **state);
+
+void test_probe_request_find(void **state);
+
+void test_probe_request_find_skips_unknown(void **state);
+
+void test_probe_request_find_rejects_unknown_mandatory(void **state);
+
+void test_probe_request_find_missing(void **state);
+
+void test_probe_request_find_reply_tlv(void **state);
+
+void test_probe_timestamp_in_window(void **state);
+
+void test_probe_request_check_valid(void **state);
+
+void test_probe_request_check_stale(void **state);
+
+void test_probe_request_check_no_request(void **state);
#endif
--
To view, visit https://gerrit.openvpn.net/c/openvpn/+/1742?usp=email
To unsubscribe, or for help writing mail filters, visit
https://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: 22
Gerrit-Owner: Lev Stipakov <[email protected]>
Gerrit-Reviewer: Arne Schwabe <[email protected]>
Gerrit-Reviewer: Frank Lichtenheld <[email protected]>
Gerrit-Reviewer: Ralf Lici <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: Ralf Lici <[email protected]>
Gerrit-Attention: Arne Schwabe <[email protected]>
Gerrit-Attention: Frank Lichtenheld <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
