[Swan-dev] fixing Windows rekeying
Antony Antony
antony at phenome.org
Wed Apr 29 05:53:08 UTC 2020
Here is my attempt to fix it. I guess there more attempts Paul and Andrew
has their own? I didnt commit because there more happening around. May be
combine and take the best.
During rekey on the responder this patch validate TS before the crypto
starts. Which I think is way better. I have been thinking of the same for
initiator; when get the response to. May be that should be later fix, first
commmit the responder side clean up.
I used 4 test cases and Windows 10 Tuomo runs to validate.
ikev2-child-rekey-09-windows this should emulate what Windows 10 is doing
with rekey. It seems DH downgrade is fixed. This is based on logs provided
by Tuomo. Next 3 tests are more impairments to TS during rekey, emulating
other possible scenarios
ikev2-child-rekey-10-impair-rekey-initiate-subnet
ikev2-child-rekey-10-impair-rekey-respond-subnet
ikev2-child-rekey-10-impair-rekey-respond-supernet
Also regarding:
https://lists.libreswan.org/pipermail/swan-dev/2020-April/003754.html
Andrew is right the initiator does not call the new functions added in
7be41582a340. That is why it is removed. Initiator already call the score
fuction follow the last two test cases.
Also Tuomo has been testing this? any issues?
regards,
-antony
-------------- next part --------------
>From 4a6860c2dce178a591ee9855239a555a68c41fbb Mon Sep 17 00:00:00 2001
From: Antony Antony <antony at phenome.org>
Date: Sun, 19 Apr 2020 08:54:48 +0000
Subject: [PATCH] ikev2: rekey responder check use exising scoring logic
Fix Windows 10 rekey response. Windows during rekey request a wider TS than
in IKE_AUTH response.
Relax 7be41582a3 check, and respond with same TS as responded in IKE_AUTH.
RFC7296 allow requesting a superset TS than IKE_AUTH response, still respond
with same TS as in IKE_AUTH response. The predecessor RFC, 5996, possibly
allow changing the TS during rekey.
Fixes: 7be41582a340 "(IKEv2: Verify (not ignore) expected TSi/TSr payloads for IPsec rekeys)"
diff --git a/programs/pluto/ikev2_parent.c b/programs/pluto/ikev2_parent.c
index 9caaf9c222..bad5e8dcf6 100644
--- a/programs/pluto/ikev2_parent.c
+++ b/programs/pluto/ikev2_parent.c
@@ -1251,10 +1251,10 @@ stf_status ikev2_IKE_SA_process_SA_INIT_response_notification(struct ike_sa *ike
}
/*
- * Need to wind things back to the point that
- * the Message ID counter code thinks this is
- * an initial outgoing message.
- */
+ * Need to wind things back to the point that
+ * the Message ID counter code thinks this is
+ * an initial outgoing message.
+ */
v2_msgid_init_ike(ike);
/*
* Stop retransmits!
@@ -4480,6 +4480,13 @@ stf_status ikev2_child_inIoutR(struct ike_sa *ike,
/* already logged; already recorded */
return STF_FAIL;
}
+ if (!child_rekey_responder_ts_verify(child, md)) {
+ record_v2N_response(ike->sa.st_logger, ike, md,
+ v2N_TS_UNACCEPTABLE, NULL/*no data*/,
+ ENCRYPTED_PAYLOAD);
+ return STF_FAIL;
+ }
+
pexpect(child->sa.st_ipsec_pred != SOS_NOBODY);
break;
case STATE_V2_NEW_CHILD_R0:
@@ -4884,20 +4891,6 @@ static stf_status ikev2_child_out_tail(struct ike_sa *ike, struct child_sa *chil
return ret; /* abort building the response message */
}
- /*
- * RFC 7296 https://tools.ietf.org/html/rfc7296#section-2.8
- * "when rekeying, the new Child SA SHOULD NOT have different Traffic
- * Selectors and algorithms than the old one."
- */
- if (child->sa.st_state->kind == STATE_V2_REKEY_CHILD_R0) {
- if (!child_rekey_ts_verify(child, request_md)) {
- /* logged; but not recorded */
- record_v2N_response(child->sa.st_logger, ike, request_md, v2N_TS_UNACCEPTABLE,
- NULL, ENCRYPTED_PAYLOAD);
- return STF_FAIL;
- }
- }
-
/* note: pst: parent; md->st: child */
/* const unsigned int len = pbs_offset(&sk.pbs); */
diff --git a/programs/pluto/ikev2_ts.c b/programs/pluto/ikev2_ts.c
index 90e63319d1..dd2d024e73 100644
--- a/programs/pluto/ikev2_ts.c
+++ b/programs/pluto/ikev2_ts.c
@@ -54,26 +54,6 @@ enum fit {
END_WIDER_THAN_TS,
};
-
-static bool ts_in_tslist(struct traffic_selectors *haystack,
- struct traffic_selector *needle)
-{
- for (unsigned int i = 0; i < haystack->nr; i++) {
- struct traffic_selector hay = haystack->ts[i];
-
- if (needle->ts_type == hay.ts_type &&
- needle->ipprotoid == hay.ipprotoid &&
- needle->startport == hay.startport &&
- needle->endport == hay.endport &&
- sameaddr(&needle->net.start, &hay.net.start) &&
- sameaddr(&needle->net.end, &hay.net.end))
- {
- return TRUE;
- }
- }
- return FALSE;
-}
-
static const char *fit_string(enum fit fit)
{
switch (fit) {
@@ -1247,40 +1227,51 @@ bool v2_process_ts_response(struct child_sa *child,
}
/*
- * RFC 7296 https://tools.ietf.org/html/rfc7296#section-2.8
+ * When rekeying responder only respond with the original TS!
+ * During rekey Windows 10 is known to ask a superset TS than
+ * the initially negotiated TS.
+ *
+ * RFC 7296 #1.3.3 "The Traffic Selectors for traffic to be sent
+ * on that SA are specified in the TS payloads in the response,
+ * which may be a subset of what the initiator of the Child SA proposed."
+ *
+ * However, the rekey initiator, when it is the original initiator of
+ * the Child SA, may request a super set. And responder should
+ * respond with same set as initially negotiated, ie RFC 7296 #2.8
* "when rekeying, the new Child SA SHOULD NOT have different Traffic
* Selectors and algorithms than the old one."
- *
- * We already matched the right connection by the SPI of v2N_REKEY_SA
+ * NOTE: Also note RFC 7296 #1.7. for the above change.
+ * Significant Differences between RFC 4306 and RFC 5996
*/
-bool child_rekey_ts_verify(struct child_sa *child, struct msg_digest *md)
+
+bool child_rekey_responder_ts_verify(struct child_sa *child, struct msg_digest *md)
{
- if (!pexpect(child->sa.st_state->kind == STATE_V2_REKEY_CHILD_R0 ||
- child->sa.st_state->kind == STATE_V2_REKEY_CHILD_I1)) {
+ if (!pexpect(child->sa.st_state->kind == STATE_V2_REKEY_CHILD_R0))
return false;
- }
+ const struct connection *c = child->sa.st_connection;
struct traffic_selectors their_tsis = { .nr = 0, };
struct traffic_selectors their_tsrs = { .nr = 0, };
- enum message_role md_role = v2_msg_role(md);
if (!v2_parse_tss(md, &their_tsis, &their_tsrs)) {
log_state(RC_LOG_SERIOUS, &child->sa, "received malformed TSi/TSr payload(s)");
return false;
}
- struct traffic_selector ts_this = ikev2_end_to_ts(&child->sa.st_connection->spd.this);
- struct traffic_selector ts_that = ikev2_end_to_ts(&child->sa.st_connection->spd.that);
+ const struct ends ends = {
+ .i = &c->spd.that,
+ .r = &c->spd.this,
+ };
- if (!ts_in_tslist(&their_tsis, (md_role == MESSAGE_REQUEST) ? &ts_that : &ts_this)) {
- log_state(RC_LOG_SERIOUS, &child->sa,
- "received TSi payload does not contain existing IPsec SA traffic Selectors");
- return false;
- }
+ enum fit fitness = END_NARROWER_THAN_TS;
+
+ struct best_score score = score_ends(fitness, c, &ends, &their_tsis,
+ &their_tsrs);
- if (!ts_in_tslist(&their_tsrs, (md_role == MESSAGE_REQUEST) ? &ts_this : &ts_that)) {
- log_state(RC_LOG_SERIOUS, &child->sa, "received TSr payload(s) does not contain existing IPsec SA Traffic Selectors");
+ if (!score.ok) {
+ loglog(RC_LOG_SERIOUS, "Rekey: received Traffic Selectors does not contain existing IPsec SA Traffic Selectors");
return false;
}
+
return true;
}
diff --git a/programs/pluto/ikev2_ts.h b/programs/pluto/ikev2_ts.h
index 4ed4b79b6a..e90650ac8b 100644
--- a/programs/pluto/ikev2_ts.h
+++ b/programs/pluto/ikev2_ts.h
@@ -49,6 +49,6 @@ stf_status v2_emit_ts_payloads(const struct child_sa *cst,
pb_stream *outpbs,
const struct connection *c);
-bool child_rekey_ts_verify(struct child_sa *child, struct msg_digest *md);
+bool child_rekey_responder_ts_verify(struct child_sa *child, struct msg_digest *md);
#endif
More information about the Swan-dev
mailing list