On Thu, Nov 21, 2019 at 01:15:50PM +0100, Tobias Heider wrote:
> Hi,
>
> this is a somewhat bigger diff that tries to separate the
> state-changing message handling parts from the message parsing in iked.
>
> Touching the SA structure in the parser is generally a bad idea
> as it means we're changing global state before the message
> is fully validated (or decrypted/integrity checked).
> The main offenders in iked are the "Notify" and "Certificate Request"
> payloads.
> With the diff, the parsed message content is attached to the
> iked_message structure (temporarily) and the SA (global state)
> is modified only when the full message is considered valid.
>
> This fixes a few problems:
> Broken message could change global state, now they only change state
> when we consider them not broken.
> The evaluation of Notify/Certreq contents depended on the order
> of the payloads, which is problematic because IKEv2 does not put any
> restrictions on the order of payloads. With the diff applied,
> message contents are evaluated in a fixed order.
>
> Functionally there should be no difference, with or without the diff,
> in use-cases that have worked before and I haven't found any regressions
> in my tests. I would appreciate if people running iked could apply and
> test this diff in their setups, just to be sure.
>
> Tobias
sthen@ ran into a problem because I limited the number of CERTREQs
to 1 + ocsp (which apparently is not enough).
Here is an updated diff which allows multiple CERTREQs.
diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h
index bc64f5b3a7b..60122a7f639 100644
--- a/sbin/iked/iked.h
+++ b/sbin/iked/iked.h
@@ -497,6 +497,13 @@ RB_HEAD(iked_sas, iked_sa);
RB_HEAD(iked_addrpool, iked_sa);
RB_HEAD(iked_addrpool6, iked_sa);
+struct iked_certreq {
+ struct ibuf *cr_data;
+ uint8_t cr_type;
+ SLIST_ENTRY(iked_certreq) cr_entry;
+};
+SLIST_HEAD(iked_certreqs, iked_certreq);
+
struct iked_message {
struct ibuf *msg_data;
size_t msg_offset;
@@ -529,6 +536,7 @@ struct iked_message {
/* Parsed information */
struct iked_proposals msg_proposals;
+ struct iked_certreqs msg_certreqs;
struct iked_spi msg_rekey;
struct ibuf *msg_nonce; /* dh NONCE */
uint16_t msg_dhgroup; /* dh group */
@@ -537,6 +545,10 @@ struct iked_message {
struct iked_id msg_id;
struct iked_id msg_cert;
struct ibuf *msg_cookie;
+ uint16_t msg_group;
+ uint16_t msg_cpi;
+ uint8_t msg_transform;
+ uint16_t msg_flags;
/* MOBIKE */
int msg_update_sa_addresses;
@@ -554,6 +566,19 @@ struct iked_message {
#define IKED_RETRANSMIT_TRIES 5 /* try 5 times */
};
+#define IKED_MSG_NAT_SRC_IP 0x01
+#define IKED_MSG_NAT_DST_IP 0x02
+
+#define IKED_MSG_FLAGS_FRAGMENTATION 0x0001
+#define IKED_MSG_FLAGS_MOBIKE 0x0002
+#define IKED_MSG_FLAGS_SIGSHA2 0x0004
+#define IKED_MSG_FLAGS_CHILD_SA_NOT_FOUND 0x0008
+#define IKED_MSG_FLAGS_NO_ADDITIONAL_SAS 0x0010
+#define IKED_MSG_FLAGS_AUTHENTICATION_FAILED 0x0020
+#define IKED_MSG_FLAGS_INVALID_KE 0x0040
+#define IKED_MSG_FLAGS_IPCOMP_SUPPORTED 0x0080
+
+
struct iked_user {
char usr_name[LOGIN_NAME_MAX];
char usr_pass[IKED_PASSWORD_SIZE];
diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index e4d37665368..90bedf91abd 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -82,6 +82,8 @@ 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_handle_certreq(struct iked*, struct iked_message *);
+
int ikev2_send_create_child_sa(struct iked *, struct iked_sa *,
struct iked_spi *, uint8_t);
int ikev2_ikesa_enable(struct iked *, struct iked_sa *, struct iked_sa *);
@@ -118,6 +120,8 @@ int ikev2_match_proposals(struct iked_proposal *, struct
iked_proposal *,
int ikev2_valid_proposal(struct iked_proposal *,
struct iked_transform **, struct iked_transform **, int *);
+int ikev2_handle_notifies(struct iked *, struct iked_message *);
+
ssize_t ikev2_add_proposals(struct iked *, struct iked_sa *, struct
ibuf *,
struct iked_proposals *, uint8_t, int, int, int);
ssize_t ikev2_add_cp(struct iked *, struct iked_sa *, struct ibuf *);
@@ -845,6 +849,9 @@ ikev2_init_recv(struct iked *env, struct iked_message *msg,
if (!ikev2_msg_frompeer(msg))
return;
+ if (ikev2_handle_notifies(env, msg) != 0)
+ return;
+
if (sa && msg->msg_nat_detected && sa->sa_natt == 0 &&
(sock = ikev2_msg_getsocket(env,
sa->sa_local.addr_af, 1)) != NULL) {
@@ -881,6 +888,9 @@ ikev2_init_recv(struct iked *env, struct iked_message *msg,
__func__));
break;
}
+ if (ikev2_handle_certreq(env, msg) != 0)
+ return;
+
(void)ikev2_init_auth(env, msg);
break;
case IKEV2_EXCHANGE_IKE_AUTH:
@@ -2354,6 +2364,9 @@ ikev2_resp_recv(struct iked *env, struct iked_message
*msg,
if (!ikev2_msg_frompeer(msg))
return;
+ if (ikev2_handle_notifies(env, msg) != 0)
+ return;
+
if ((sa = msg->msg_sa) == NULL)
return;
@@ -2398,6 +2411,9 @@ ikev2_resp_recv(struct iked *env, struct iked_message
*msg,
sa->sa_policy->pol_auth.auth_eap)
sa_state(env, sa, IKEV2_STATE_EAP);
+ if (ikev2_handle_certreq(env, msg) != 0)
+ return;
+
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);
@@ -2423,6 +2439,102 @@ ikev2_resp_recv(struct iked *env, struct iked_message
*msg,
}
}
+int
+ikev2_handle_notifies(struct iked *env, struct iked_message *msg)
+{
+ struct iked_sa *sa;
+ uint16_t group;
+
+ if ((sa = msg->msg_sa) == NULL)
+ return (-1);
+
+ if (msg->msg_flags & IKED_MSG_FLAGS_CHILD_SA_NOT_FOUND)
+ sa->sa_stateflags &= ~IKED_REQ_CHILDSA;
+
+ if ((msg->msg_flags & IKED_MSG_FLAGS_FRAGMENTATION) && env->sc_frag) {
+ log_debug("%s: fragmentation enabled", __func__);
+ sa->sa_frag = 1;
+ }
+
+ if ((msg->msg_flags & IKED_MSG_FLAGS_MOBIKE) && env->sc_mobike) {
+ log_debug("%s: mobike enabled", __func__);
+ sa->sa_mobike = 1;
+ /* enforce natt */
+ sa->sa_natt = 1;
+ }
+
+ if ((msg->msg_flags & IKED_MSG_FLAGS_NO_ADDITIONAL_SAS)
+ && sa->sa_stateflags & IKED_REQ_CHILDSA) {
+ /* This makes sense for Child SAs only atm */
+ ikev2_disable_rekeying(env, sa);
+ sa->sa_stateflags &= ~IKED_REQ_CHILDSA;
+ }
+
+ if (msg->msg_flags & IKED_MSG_FLAGS_AUTHENTICATION_FAILED) {
+ log_debug("%s: AUTHENTICATION_FAILED, closing SA", __func__);
+ ikev2_ike_sa_setreason(sa,
+ "authentication failed notification from peer");
+ sa_state(env, sa, IKEV2_STATE_CLOSED);
+ msg->msg_sa = NULL;
+ return (-1);
+ }
+
+ if (msg->msg_flags & IKED_MSG_FLAGS_INVALID_KE) {
+ /* XXX chould also happen for PFS */
+ group = betoh16(msg->msg_group);
+ if (!sa->sa_hdr.sh_initiator) {
+ log_debug("%s: not an initiator", __func__);
+ ikev2_ike_sa_setreason(sa,
+ "received invalid KE as responder");
+ sa_state(env, sa, IKEV2_STATE_CLOSED);
+ msg->msg_sa = NULL;
+ return (-1);
+ }
+ if (group_getid(group) == NULL) {
+ log_debug("%s: unable to select DH group %u", __func__,
+ group);
+ ikev2_ike_sa_setreason(sa,
+ "unable to select DH group");
+ sa_state(env, sa, IKEV2_STATE_CLOSED);
+ msg->msg_sa = NULL;
+ return (-1);
+ }
+ msg->msg_policy->pol_peerdh = group;
+ log_debug("%s: responder selected DH group %u", __func__,
+ group);
+ ikev2_ike_sa_setreason(sa,
+ "reinitiating with new DH group");
+ sa_state(env, sa, IKEV2_STATE_CLOSED);
+ msg->msg_sa = NULL;
+
+ /*
+ * XXX should also happen for PFS so we have to check state.
+ */
+ timer_set(env, &env->sc_inittmr, ikev2_init_ike_sa, NULL);
+ timer_add(env, &env->sc_inittmr, IKED_INITIATOR_INITIAL);
+ return (-1);
+ }
+
+ if (msg->msg_flags & IKED_MSG_FLAGS_IPCOMP_SUPPORTED) {
+ /* we only support deflate */
+ if ((msg->msg_policy->pol_flags & IKED_POLICY_IPCOMP) &&
+ (msg->msg_transform == IKEV2_IPCOMP_DEFLATE)) {
+ sa->sa_ipcomp = msg->msg_transform;
+ sa->sa_cpi_out = betoh16(msg->msg_cpi);
+ }
+ }
+
+ if (msg->msg_nat_detected & IKED_MSG_NAT_DST_IP) {
+ /* Send keepalive, since we are behind a NAT-gw */
+ sa->sa_usekeepalive = 1;
+ }
+
+ /* Signature hash algorithm */
+ if (msg->msg_flags & IKED_MSG_FLAGS_SIGSHA2)
+ sa->sa_sigsha2 = 1;
+ return (0);
+}
+
int
ikev2_resp_ike_sa_init(struct iked *env, struct iked_message *msg)
{
@@ -2735,6 +2847,36 @@ ikev2_send_init_error(struct iked *env, struct
iked_message *msg)
return (ret);
}
+int
+ikev2_handle_certreq(struct iked* env, struct iked_message *msg)
+{
+ struct iked_certreq *cr;
+ struct iked_sa *sa;
+
+ if ((sa = msg->msg_sa) == NULL)
+ return (-1);
+
+ while ((cr = SLIST_FIRST(&msg->msg_certreqs))) {
+ /* Optional certreq for PSK */
+ if (sa->sa_hdr.sh_initiator)
+ sa->sa_stateinit |= IKED_REQ_CERT;
+ else
+ sa->sa_statevalid |= IKED_REQ_CERT;
+
+ ca_setreq(env, sa, &sa->sa_policy->pol_localid, cr->cr_type,
+ ibuf_data(cr->cr_data),
+ ibuf_length(cr->cr_data),
+ PROC_CERT);
+
+ ibuf_release(cr->cr_data);
+ SLIST_REMOVE_HEAD(&msg->msg_certreqs, cr_entry);
+ free(cr);
+ }
+
+ return (0);
+}
+
+
int
ikev2_resp_ike_auth(struct iked *env, struct iked_sa *sa)
{
diff --git a/sbin/iked/ikev2_msg.c b/sbin/iked/ikev2_msg.c
index d36e9b3b2d7..9f51110ada5 100644
--- a/sbin/iked/ikev2_msg.c
+++ b/sbin/iked/ikev2_msg.c
@@ -94,6 +94,7 @@ ikev2_msg_cb(int fd, short event, void *arg)
return;
TAILQ_INIT(&msg.msg_proposals);
+ SLIST_INIT(&msg.msg_certreqs);
msg.msg_fd = fd;
if (hdr.ike_version == IKEV1_VERSION)
@@ -181,6 +182,8 @@ ikev2_msg_copy(struct iked *env, struct iked_message *msg)
void
ikev2_msg_cleanup(struct iked *env, struct iked_message *msg)
{
+ struct iked_certreq *cr;
+
if (msg == msg->msg_parent) {
ibuf_release(msg->msg_nonce);
ibuf_release(msg->msg_ke);
@@ -199,6 +202,11 @@ ikev2_msg_cleanup(struct iked *env, struct iked_message
*msg)
msg->msg_cookie2 = NULL;
config_free_proposals(&msg->msg_proposals, 0);
+ while ((cr = SLIST_FIRST(&msg->msg_certreqs))) {
+ ibuf_release(cr->cr_data);
+ SLIST_REMOVE_HEAD(&msg->msg_certreqs, cr_entry);
+ free(cr);
+ }
}
if (msg->msg_data != NULL) {
diff --git a/sbin/iked/ikev2_pld.c b/sbin/iked/ikev2_pld.c
index e92bd3be2a7..8147eb47fb5 100644
--- a/sbin/iked/ikev2_pld.c
+++ b/sbin/iked/ikev2_pld.c
@@ -792,7 +792,8 @@ ikev2_pld_cert(struct iked *env, struct ikev2_payload *pld,
certid = &msg->msg_parent->msg_cert;
if (certid->id_type) {
- log_debug("%s: duplicate cert payload", __func__);
+ log_info("%s: multiple cert payloads not supported",
+ SPI_SA(msg->msg_sa, __func__));
return (-1);
}
@@ -826,8 +827,8 @@ int
ikev2_pld_certreq(struct iked *env, struct ikev2_payload *pld,
struct iked_message *msg, size_t offset, size_t left)
{
- struct iked_sa *sa = msg->msg_sa;
struct ikev2_cert cert;
+ struct iked_certreq *cr;
uint8_t *buf;
ssize_t len;
uint8_t *msgbuf = ibuf_data(msg->msg_data);
@@ -848,25 +849,27 @@ ikev2_pld_certreq(struct iked *env, struct ikev2_payload
*pld,
return (0);
if (cert.cert_type == IKEV2_CERT_X509_CERT) {
- if (!len)
+ if (len == 0) {
+ log_info("%s: invalid length 0", __func__);
return (0);
+ }
if ((len % SHA_DIGEST_LENGTH) != 0) {
- log_debug("%s: invalid certificate request", __func__);
+ log_info("%s: invalid certificate request",
+ __func__);
return (-1);
}
}
- if (msg->msg_sa == NULL)
+ if ((cr = calloc(1, sizeof(struct iked_certreq))) == NULL) {
+ log_info("%s: failed to allocate certreq.", __func__);
return (-1);
-
- /* Optional certreq for PSK */
- if (sa->sa_hdr.sh_initiator)
- sa->sa_stateinit |= IKED_REQ_CERT;
- else
- sa->sa_statevalid |= IKED_REQ_CERT;
-
- ca_setreq(env, sa, &sa->sa_policy->pol_localid,
- cert.cert_type, buf, len, PROC_CERT);
+ }
+ if ((cr->cr_data = ibuf_new(buf, len)) == NULL) {
+ log_info("%s: failed to allocate buffer.", __func__);
+ return (-1);
+ }
+ cr->cr_type = cert.cert_type;
+ SLIST_INSERT_HEAD(&msg->msg_parent->msg_certreqs, cr, cr_entry);
return (0);
}
@@ -989,10 +992,7 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
uint64_t spi64;
struct iked_spi *rekey;
uint16_t type;
- uint16_t group;
- uint16_t cpi;
uint16_t signature_hash;
- uint8_t transform;
if (ikev2_validate_notify(msg, offset, left, &n))
return (-1);
@@ -1025,11 +1025,12 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
if (memcmp(buf, md, len) != 0) {
log_debug("%s: %s detected NAT", __func__,
print_map(type, ikev2_n_map));
- msg->msg_parent->msg_nat_detected = 1;
- /* Send keepalive, since we are behind a NAT-gw */
- if (msg->msg_sa != NULL &&
- type == IKEV2_N_NAT_DETECTION_DESTINATION_IP)
- msg->msg_sa->sa_usekeepalive = 1;
+ if (type == IKEV2_N_NAT_DETECTION_SOURCE_IP)
+ msg->msg_parent->msg_nat_detected
+ |= IKED_MSG_NAT_SRC_IP;
+ else
+ msg->msg_parent->msg_nat_detected
+ |= IKED_MSG_NAT_DST_IP;
}
print_hex(md, 0, sizeof(md));
/* remember for MOBIKE */
@@ -1061,11 +1062,8 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
return (-1);
}
}
- log_debug("%s: AUTHENTICATION_FAILED, closing SA", __func__);
- ikev2_ike_sa_setreason(msg->msg_sa,
- "authentication failed notification from peer");
- sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSED);
- msg->msg_sa = NULL;
+ msg->msg_parent->msg_flags
+ |= IKED_MSG_FLAGS_AUTHENTICATION_FAILED;
break;
case IKEV2_N_INVALID_KE_PAYLOAD:
if (sa_stateok(msg->msg_sa, IKEV2_STATE_VALID) &&
@@ -1074,36 +1072,14 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
__func__);
return (-1);
}
- if (len != sizeof(group)) {
+ if (len != sizeof(msg->msg_parent->msg_group)) {
log_debug("%s: malformed payload: group size mismatch"
- " (%zu != %zu)", __func__, len, sizeof(group));
- return (-1);
- }
- /* XXX chould also happen for PFS */
- if (!msg->msg_sa->sa_hdr.sh_initiator) {
- log_debug("%s: not an initiator", __func__);
- sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSED);
- msg->msg_sa = NULL;
+ " (%zu != %zu)", __func__, len,
+ sizeof(msg->msg_parent->msg_group));
return (-1);
}
- memcpy(&group, buf, len);
- group = betoh16(group);
- if (group_getid(group) == NULL) {
- log_debug("%s: unable to select DH group %u", __func__,
- group);
- return (-1);
- }
- msg->msg_policy->pol_peerdh = group;
- log_debug("%s: responder selected DH group %u", __func__,
- group);
- sa_state(env, msg->msg_sa, IKEV2_STATE_CLOSED);
- msg->msg_sa = NULL;
-
- /*
- * XXX should also happen for PFS so we have to check state.
- */
- timer_set(env, &env->sc_inittmr, ikev2_init_ike_sa, NULL);
- timer_add(env, &env->sc_inittmr, IKED_INITIATOR_INITIAL);
+ memcpy(&msg->msg_parent->msg_group, buf, len);
+ msg->msg_parent->msg_flags |= IKED_MSG_FLAGS_INVALID_KE;
break;
case IKEV2_N_NO_ADDITIONAL_SAS:
if (!msg->msg_e) {
@@ -1111,11 +1087,7 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
__func__);
return (-1);
}
- /* This makes sense for Child SAs only atm */
- if (msg->msg_sa->sa_stateflags & IKED_REQ_CHILDSA) {
- ikev2_disable_rekeying(env, msg->msg_sa);
- msg->msg_sa->sa_stateflags &= ~IKED_REQ_CHILDSA;
- }
+ msg->msg_parent->msg_flags |= IKED_MSG_FLAGS_NO_ADDITIONAL_SAS;
break;
case IKEV2_N_REKEY_SA:
if (!msg->msg_e) {
@@ -1159,21 +1131,25 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
__func__);
return (-1);
}
- if (len < sizeof(cpi) + sizeof(transform)) {
+ if (len < sizeof(msg->msg_parent->msg_cpi) +
+ sizeof(msg->msg_parent->msg_transform)) {
log_debug("%s: ignoring malformed ipcomp notification",
__func__);
return (0);
}
- memcpy(&cpi, buf, sizeof(cpi));
- memcpy(&transform, buf + sizeof(cpi), sizeof(transform));
- log_debug("%s: cpi 0x%x, transform %s, len %zu", __func__,
- betoh16(cpi), print_map(transform, ikev2_ipcomp_map), len);
- /* we only support deflate */
- if ((msg->msg_policy->pol_flags & IKED_POLICY_IPCOMP) &&
- (transform == IKEV2_IPCOMP_DEFLATE)) {
- msg->msg_sa->sa_ipcomp = transform;
- msg->msg_sa->sa_cpi_out = betoh16(cpi);
- }
+ memcpy(&msg->msg_parent->msg_cpi, buf,
+ sizeof(msg->msg_parent->msg_cpi));
+ memcpy(&msg->msg_parent->msg_transform,
+ buf + sizeof(msg->msg_parent->msg_cpi),
+ sizeof(msg->msg_parent->msg_transform));
+
+ log_debug("%s: %s cpi 0x%x, transform %s, len %zu", __func__,
+ msg->msg_parent->msg_response ? "res" : "req",
+ betoh16(msg->msg_parent->msg_cpi),
+ print_map(msg->msg_parent->msg_transform,
+ ikev2_ipcomp_map), len);
+
+ msg->msg_parent->msg_flags |= IKED_MSG_FLAGS_IPCOMP_SUPPORTED;
break;
case IKEV2_N_CHILD_SA_NOT_FOUND:
if (!msg->msg_e) {
@@ -1181,8 +1157,7 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
__func__);
return (-1);
}
- /* Stop expecting response */
- msg->msg_sa->sa_stateflags &= ~IKED_REQ_CHILDSA;
+ msg->msg_parent->msg_flags |= IKED_MSG_FLAGS_CHILD_SA_NOT_FOUND;
break;
case IKEV2_N_MOBIKE_SUPPORTED:
if (!msg->msg_e) {
@@ -1195,13 +1170,7 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
" notification: %zu", __func__, len);
return (0);
}
- if (!env->sc_mobike) {
- log_debug("%s: mobike disabled", __func__);
- return (0);
- }
- msg->msg_sa->sa_mobike = 1;
- /* enforce natt */
- msg->msg_sa->sa_natt = 1;
+ msg->msg_parent->msg_flags |= IKED_MSG_FLAGS_MOBIKE;
break;
case IKEV2_N_UPDATE_SA_ADDRESSES:
if (!msg->msg_e) {
@@ -1276,11 +1245,7 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
" notification: %zu", __func__, len);
return (0);
}
- if (!env->sc_frag) {
- log_debug("%s: fragmentation disabled", __func__);
- return (0);
- }
- msg->msg_sa->sa_frag = 1;
+ msg->msg_parent->msg_flags |= IKED_MSG_FLAGS_FRAGMENTATION;
break;
case IKEV2_N_SIGNATURE_HASH_ALGORITHMS:
if (msg->msg_e) {
@@ -1309,7 +1274,8 @@ ikev2_pld_notify(struct iked *env, struct ikev2_payload
*pld,
len -= sizeof(signature_hash);
buf += sizeof(signature_hash);
if (signature_hash == IKEV2_SIGHASH_SHA2_256)
- msg->msg_sa->sa_sigsha2 = 1;
+ msg->msg_parent->msg_flags
+ |= IKED_MSG_FLAGS_SIGSHA2;
}
break;
}
