Patrick Wildt <patr...@blueri.se> writes:
> On Mon, Nov 27, 2017 at 06:12:22PM +0100, Patrick Wildt wrote:
>> On Mon, Nov 27, 2017 at 04:21:08PM +0100, Patrick Wildt wrote:
>> > 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
>
> Updated diff also responds with INVALID_KE_PAYLOAD when the responder
> has a lower SA lifetime and initiates the CHILD_SA_CREATE.
>
> Handling rejections (as in receiving INVALID_KE_PAYLOAD) is still a ToDo
> I want to tackle next.
Patrick, thanks for running with this. I see from the CVS log that a
version of this patch and other patches have been applied. I will
update my system and do some testing.
> ok?
>
> diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
> index a0059682bc8..01f8ac00506 100644
> --- a/sbin/iked/iked.h
> +++ b/sbin/iked/iked.h
> @@ -508,6 +508,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 b505f763153..b8cef9061fc 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 *);
>
> @@ -455,6 +457,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)
> @@ -825,7 +844,11 @@ ikev2_init_recv(struct iked *env, struct iked_message
> *msg,
> (void)ikev2_ike_auth_recv(env, sa, msg);
> break;
> case IKEV2_EXCHANGE_CREATE_CHILD_SA:
> - (void)ikev2_init_create_child_sa(env, msg);
> + if (ikev2_init_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:
> sa->sa_stateflags &= ~IKED_REQ_INF;
> @@ -2232,6 +2255,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;
> }
> @@ -2254,13 +2280,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 (msg->msg_update_sa_addresses)
> @@ -2376,7 +2406,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:
> @@ -2423,31 +2452,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:
> @@ -2455,6 +2541,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)
> {
> @@ -3540,14 +3683,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;
> @@ -4105,6 +4248,16 @@ ikev2_sa_initiator_dh(struct iked_sa *sa, struct
> iked_message *msg,
> if (msg == NULL)
> return (0);
>
> + /* Look for dhgroup mismatch during an IKE SA negotiation */
> + if (msg->msg_dhgroup != sa->sa_dhgroup->id) {
> + log_debug("%s: want dh %s, KE has %s", __func__,
> + print_map(sa->sa_dhgroup->id, ikev2_xformdh_map),
> + print_map(msg->msg_dhgroup, ikev2_xformdh_map));
> + msg->msg_error = IKEV2_N_INVALID_KE_PAYLOAD;
> + msg->msg_dhgroup = sa->sa_dhgroup->id;
> + return (-1);
> + }
> +
> if (!ibuf_length(sa->sa_dhrexchange)) {
> if (!ibuf_length(msg->msg_ke)) {
> log_debug("%s: invalid peer dh exchange", __func__);
> @@ -4236,6 +4389,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 38a05e8ff0d..32ecb349674 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);
--
Tim Stewart
-----------
Mail: t...@stoo.org
Matrix: @tim:stoo.org
