On Wed, Nov 22, 2017 at 05:26:24PM +0100, Patrick Wildt wrote:
> On 2017/06/25 21:44, Tim Stewart wrote:
> > My first patch did, in fact, break Child SAs rekeying. I have a new
> > patch at the end of this message that simply restricts DH group
> > negotiation to IKE SAs (I *think* that DH group guessing only applies to
> > IKE SAs, and perhaps only the IKE_SA_INIT exchange, but I'm still
> > working through the RFC). This may not be the ultimate solution, but it
> > does allow us to move forward.
>
> Reading RFC 7296 it looks like throwing INVALID_KE_PAYLOAD is fine for
> both establishing the IKE SA and rekeying the Child SAs. If we select a
> proposal from the msg that uses a different DH group than the one that's
> used in the KEi (in the same msg) we need to throw INVALID_KE_PAYLOAD.
>
> Since all messages subsequent to the initial exchange must be encrypted,
> the INVALID_KE_PAYLOAD message on rekeying Child SAs must be encrypted.
> Apparently with the previous diff the Child SA rekeying failed. This is
> because the code sends the INVALID_KE_PAYLOAD response unencrypted.
>
> Also I have found inconsistencies in handling INVALID_KE_PAYLOAD with us
> acting as initiator. I will take a look at both cases and will follow
> up.
>
> Patrick
Hi,
This diff adds suport to rejecting IKE SA messages. This means that we
can reply to IKE SA INIT messages with no proposal chosen, as we already
do for Child SAs. For that the error "adding" is done in a new function
shared by both send error handlers. We need two "send error" functions
because the init error is unencrypted, while all later ones are not.
Now we can add more cases, like Child SA not found or that the DH group
is not what we expect. I think that is quite an improvement.
One "issue" is retransmits. The RFC specifies that IKEv2 is a reliable
protocol. When a packet is lost, the initiator has to retransmit the
request and the responder must retransmit the saved reply. On IKE SA
INIT error we don't save our response, which means that we will handle
the request as if we never had it. The RFC says we shouldn't do that,
because DoS protection and so on. What we should do is keep our reply
around and check if it is indeed a retransmit. If so we reply with the
same reply, thus saving resources. If it is an attempt to create a new
IKE SA, drop the old SA so that iked(8) can attempt to create a new SA.
Thoughts?
Patrick
diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
index b536d58e157..050277a2530 100644
--- a/sbin/iked/iked.h
+++ b/sbin/iked/iked.h
@@ -502,6 +502,7 @@ struct iked_message {
struct iked_proposals msg_proposals;
struct iked_spi msg_rekey;
struct ibuf *msg_nonce; /* dh NONCE */
+ uint16_t msg_dhgroup; /* dh group */
struct ibuf *msg_ke; /* dh key exchange */
struct iked_id msg_auth; /* AUTH payload */
struct iked_id msg_id;
diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index e908b6230a5..234f8329791 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -76,6 +76,7 @@ int ikev2_resp_ike_eap(struct iked *, struct iked_sa *,
struct ibuf *);
int ikev2_send_auth_failed(struct iked *, struct iked_sa *);
int ikev2_send_error(struct iked *, struct iked_sa *,
struct iked_message *, uint8_t);
+int ikev2_send_init_error(struct iked *, struct iked_message *);
int ikev2_send_create_child_sa(struct iked *, struct iked_sa *,
struct iked_spi *, uint8_t);
@@ -125,6 +126,7 @@ ssize_t ikev2_add_certreq(struct ibuf *, struct
ikev2_payload **, ssize_t,
ssize_t ikev2_add_ipcompnotify(struct iked *, struct ibuf *,
struct ikev2_payload **, ssize_t, struct iked_sa *);
ssize_t ikev2_add_ts_payload(struct ibuf *, unsigned int, struct
iked_sa *);
+ssize_t ikev2_add_error(struct iked *, struct ibuf *, struct
iked_message *);
int ikev2_add_data(struct ibuf *, void *, size_t);
int ikev2_add_buf(struct ibuf *buf, struct ibuf *);
@@ -446,6 +448,23 @@ ikev2_recv(struct iked *env, struct iked_message *msg)
if ((m = ikev2_msg_lookup(env, &sa->sa_requests, msg, hdr)))
ikev2_msg_dispose(env, &sa->sa_requests, m);
} else {
+ /*
+ * IKE_SA_INIT is special since it always uses the message id 0.
+ * Even when the message was rejected, and the new message has
+ * different proposals, the id will be the same. To discern
+ * retransmits and new messages, the RFC suggests to compare the
+ * the messages.
+ */
+ if (sa->sa_state == IKEV2_STATE_CLOSED && sa->sa_1stmsg &&
+ hdr->ike_exchange == IKEV2_EXCHANGE_IKE_SA_INIT &&
+ msg->msg_msgid == 0 &&
+ (ibuf_length(msg->msg_data) != ibuf_length(sa->sa_1stmsg) ||
+ memcmp(ibuf_data(msg->msg_data), ibuf_data(sa->sa_1stmsg),
+ ibuf_length(sa->sa_1stmsg)) != 0)) {
+ sa_free(env, sa);
+ msg->msg_sa = sa = NULL;
+ goto done;
+ }
if (msg->msg_msgid < sa->sa_msgid)
return;
if (flag)
@@ -2121,6 +2140,9 @@ ikev2_resp_recv(struct iked *env, struct iked_message
*msg,
case IKEV2_EXCHANGE_IKE_SA_INIT:
if (ikev2_sa_responder(env, sa, NULL, msg) != 0) {
log_debug("%s: failed to get IKE SA keys", __func__);
+ if (msg->msg_error == 0)
+ msg->msg_error = IKEV2_N_NO_PROPOSAL_CHOSEN;
+ ikev2_send_init_error(env, msg);
sa_state(env, sa, IKEV2_STATE_CLOSED);
return;
}
@@ -2143,13 +2165,17 @@ ikev2_resp_recv(struct iked *env, struct iked_message
*msg,
if (ikev2_ike_auth_recv(env, sa, msg) != 0) {
log_debug("%s: failed to send auth response", __func__);
+ ikev2_send_error(env, sa, msg, hdr->ike_exchange);
sa_state(env, sa, IKEV2_STATE_CLOSED);
return;
}
break;
case IKEV2_EXCHANGE_CREATE_CHILD_SA:
- if (ikev2_resp_create_child_sa(env, msg) != 0)
+ if (ikev2_resp_create_child_sa(env, msg) != 0) {
+ if (msg->msg_error == 0)
+ msg->msg_error = IKEV2_N_NO_PROPOSAL_CHOSEN;
ikev2_send_error(env, sa, msg, hdr->ike_exchange);
+ }
break;
case IKEV2_EXCHANGE_INFORMATIONAL:
if (sa_stateok(sa, IKEV2_STATE_AUTH_REQUEST) &&
@@ -2269,7 +2295,6 @@ ikev2_resp_ike_sa_init(struct iked *env, struct
iked_message *msg)
goto done;
}
- resp.msg_sa = NULL; /* Don't save the response */
ret = ikev2_msg_send(env, &resp);
done:
@@ -2316,31 +2341,88 @@ ikev2_send_auth_failed(struct iked *env, struct iked_sa
*sa)
return (ret);
}
-int
-ikev2_send_error(struct iked *env, struct iked_sa *sa,
- struct iked_message *msg, uint8_t exchange)
+ssize_t
+ikev2_add_error(struct iked *env, struct ibuf *buf, struct iked_message *msg)
{
struct ikev2_notify *n;
- struct ibuf *buf = NULL;
- int ret = -1;
+ struct iked_spi *rekey;
+ uint16_t group;
+ uint32_t spi32;
+ uint64_t spi64;
+ size_t len;
+ uint8_t *ptr;
switch (msg->msg_error) {
+ case IKEV2_N_CHILD_SA_NOT_FOUND:
+ break;
case IKEV2_N_NO_PROPOSAL_CHOSEN:
break;
- case 0:
- return (0);
+ case IKEV2_N_INVALID_KE_PAYLOAD:
+ break;
default:
return (-1);
}
- /* Notify payload */
+ len = sizeof(*n);
+ if ((ptr = ibuf_advance(buf, len)) == NULL)
+ return (-1);
+ n = (struct ikev2_notify *)ptr;
+ n->n_type = htobe16(msg->msg_error);
+ switch (msg->msg_error) {
+ case IKEV2_N_CHILD_SA_NOT_FOUND:
+ rekey = &msg->msg_rekey;
+ switch (rekey->spi_size) {
+ case 4:
+ spi32 = htobe32(rekey->spi);
+ if (ibuf_add(buf, &spi32, sizeof(spi32)) != 0)
+ return (-1);
+ len += sizeof(spi32);
+ break;
+ case 8:
+ spi64 = htobe64(rekey->spi);
+ if (ibuf_add(buf, &spi64, sizeof(spi64)) != 0)
+ return (-1);
+ len += sizeof(spi64);
+ break;
+ default:
+ log_debug("%s: invalid SPI size %d", __func__,
+ rekey->spi_size);
+ return (-1);
+ }
+ n->n_protoid = rekey->spi_protoid;
+ n->n_spisize = rekey->spi_size;
+ break;
+ case IKEV2_N_INVALID_KE_PAYLOAD:
+ group = htobe16(msg->msg_dhgroup);
+ if (ibuf_add(buf, &group, sizeof(group)) != 0)
+ return (-1);
+ len += sizeof(group);
+ n->n_protoid = 0;
+ n->n_spisize = 0;
+ break;
+ default:
+ n->n_protoid = 0;
+ n->n_spisize = 0;
+ break;
+ }
+ log_debug("%s: done", __func__);
+
+ return (len);
+}
+
+int
+ikev2_send_error(struct iked *env, struct iked_sa *sa,
+ struct iked_message *msg, uint8_t exchange)
+{
+ struct ibuf *buf = NULL;
+ ssize_t len;
+ int ret = -1;
+
+ if (msg->msg_error == 0)
+ return (0);
if ((buf = ibuf_static()) == NULL)
goto done;
- if ((n = ibuf_advance(buf, sizeof(*n))) == NULL)
+ if ((len = ikev2_add_error(env, buf, msg)) == 0)
goto done;
- n->n_protoid = 0;
- n->n_spisize = 0;
- n->n_type = htobe16(msg->msg_error);
-
ret = ikev2_send_ike_e(env, sa, buf, IKEV2_PAYLOAD_NOTIFY,
exchange, 1);
done:
@@ -2348,6 +2430,63 @@ ikev2_send_error(struct iked *env, struct iked_sa *sa,
return (ret);
}
+/*
+ * Variant of ikev2_send_error() that can be used before encryption
+ * is enabled. Based on ikev2_resp_ike_sa_init() code.
+ */
+int
+ikev2_send_init_error(struct iked *env, struct iked_message *msg)
+{
+ struct iked_message resp;
+ struct ike_header *hdr;
+ struct ikev2_payload *pld;
+ struct iked_sa *sa = msg->msg_sa;
+ struct ibuf *buf;
+ ssize_t len = 0;
+ int ret = -1;
+
+ if (sa->sa_hdr.sh_initiator) {
+ log_debug("%s: called by initiator", __func__);
+ return (-1);
+ }
+ if (msg->msg_error == 0)
+ return (0);
+
+ if ((buf = ikev2_msg_init(env, &resp,
+ &msg->msg_peer, msg->msg_peerlen,
+ &msg->msg_local, msg->msg_locallen, 1)) == NULL)
+ goto done;
+
+ resp.msg_sa = sa;
+ resp.msg_fd = msg->msg_fd;
+ resp.msg_natt = msg->msg_natt;
+ resp.msg_msgid = 0;
+
+ /* IKE header */
+ if ((hdr = ikev2_add_header(buf, sa, resp.msg_msgid,
+ IKEV2_PAYLOAD_NOTIFY, IKEV2_EXCHANGE_IKE_SA_INIT,
+ IKEV2_FLAG_RESPONSE)) == NULL)
+ goto done;
+
+ /* NOTIFY payload */
+ if ((pld = ikev2_add_payload(buf)) == NULL)
+ goto done;
+ if ((len = ikev2_add_error(env, buf, msg)) == 0)
+ goto done;
+ if (ikev2_next_payload(pld, len, IKEV2_PAYLOAD_NONE) == -1)
+ goto done;
+ if (ikev2_set_header(hdr, ibuf_size(buf) - sizeof(*hdr)) == -1)
+ goto done;
+
+ (void)ikev2_pld_parse(env, hdr, &resp, 0);
+ ret = ikev2_msg_send(env, &resp);
+
+ done:
+ ikev2_msg_cleanup(env, &resp);
+
+ return (ret);
+}
+
int
ikev2_resp_ike_auth(struct iked *env, struct iked_sa *sa)
{
@@ -3425,14 +3564,14 @@ ikev2_resp_create_child_sa(struct iked *env, struct
iked_message *msg)
log_debug("%s: CHILD SA %s wasn't found",
__func__, print_spi(rekey->spi,
rekey->spi_size));
- /* XXX send notification to peer */
+ msg->msg_error = IKEV2_N_CHILD_SA_NOT_FOUND;
goto fail;
}
if (!csa->csa_loaded || !csa->csa_peersa ||
!csa->csa_peersa->csa_loaded) {
log_debug("%s: SA is not loaded or no peer SA",
__func__);
- /* XXX send notification to peer */
+ msg->msg_error = IKEV2_N_CHILD_SA_NOT_FOUND;
goto fail;
}
csa->csa_rekey = 1;
@@ -4121,6 +4260,16 @@ ikev2_sa_responder_dh(struct iked_kex *kex, struct
iked_proposals *proposals,
}
}
+ /* Look for dhgroup mismatch during an IKE SA negotiation */
+ if (msg->msg_dhgroup != kex->kex_dhgroup->id) {
+ log_debug("%s: want dh %s, KE has %s", __func__,
+ print_map(kex->kex_dhgroup->id, ikev2_xformdh_map),
+ print_map(msg->msg_dhgroup, ikev2_xformdh_map));
+ msg->msg_error = IKEV2_N_INVALID_KE_PAYLOAD;
+ msg->msg_dhgroup = kex->kex_dhgroup->id;
+ return (-1);
+ }
+
if (!ibuf_length(kex->kex_dhrexchange)) {
if ((kex->kex_dhrexchange = ibuf_new(NULL,
dh_getlen(kex->kex_dhgroup))) == NULL) {
diff --git a/sbin/iked/ikev2_pld.c b/sbin/iked/ikev2_pld.c
index 5724520f696..6b55f548fdc 100644
--- a/sbin/iked/ikev2_pld.c
+++ b/sbin/iked/ikev2_pld.c
@@ -703,6 +703,7 @@ ikev2_pld_ke(struct iked *env, struct ikev2_payload *pld,
log_debug("%s: failed to get exchange", __func__);
return (-1);
}
+ msg->msg_parent->msg_dhgroup = betoh16(kex.kex_dhgroup);
}
return (0);
