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. 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);