From 834f9deeb62622509504a76776dc060a242f6a54 Mon Sep 17 00:00:00 2001 From: markus Date: Fri, 9 May 2014 06:37:24 +0000 Subject: [PATCH] get rid of redundant {csa,flow}_{src,dst}id pointers, so we don't need to update it on rekey (fixes use-after-free); ok mikeb@ --- sbin/iked/iked.h | 10 +++------- sbin/iked/ikev2.c | 31 ++----------------------------- sbin/iked/pfkey.c | 23 +++++++++++++++-------- 3 files changed, 20 insertions(+), 44 deletions(-) diff --git a/sbin/iked/iked.h b/sbin/iked/iked.h index a1665f07aec..0e240f62748 100644 --- a/sbin/iked/iked.h +++ b/sbin/iked/iked.h @@ -1,4 +1,4 @@ -/* $OpenBSD: iked.h,v 1.80 2014/05/09 06:29:46 markus Exp $ */ +/* $OpenBSD: iked.h,v 1.81 2014/05/09 06:37:24 markus Exp $ */ /* * Copyright (c) 2010-2013 Reyk Floeter @@ -146,9 +146,6 @@ struct iked_flow { u_int8_t flow_ipproto; u_int8_t flow_type; - struct iked_id *flow_srcid; - struct iked_id *flow_dstid; - struct iked_addr *flow_local; /* outer source */ struct iked_addr *flow_peer; /* outer dest */ struct iked_sa *flow_ikesa; /* parent SA */ @@ -179,9 +176,6 @@ struct iked_childsa { struct ibuf *csa_integrkey; /* auth key */ u_int16_t csa_integrid; /* auth xform id */ - struct iked_id *csa_srcid; - struct iked_id *csa_dstid; - struct iked_addr *csa_local; /* outer source */ struct iked_addr *csa_peer; /* outer dest */ struct iked_sa *csa_ikesa; /* parent SA */ @@ -417,6 +411,8 @@ struct iked_sa { struct iked_id sa_rid; /* responder id */ struct iked_id sa_icert; /* initiator cert */ struct iked_id sa_rcert; /* responder cert */ +#define IKESA_SRCID(x) ((x)->sa_hdr.sh_initiator ? &(x)->sa_iid : &(x)->sa_rid) +#define IKESA_DSTID(x) ((x)->sa_hdr.sh_initiator ? &(x)->sa_rid : &(x)->sa_iid) char *sa_eapid; /* EAP identity */ struct iked_id sa_eap; /* EAP challenge */ diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c index 37feb432a82..d5953cb7c29 100644 --- a/sbin/iked/ikev2.c +++ b/sbin/iked/ikev2.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ikev2.c,v 1.111 2014/05/09 06:29:46 markus Exp $ */ +/* $OpenBSD: ikev2.c,v 1.112 2014/05/09 06:37:24 markus Exp $ */ /* * Copyright (c) 2010-2013 Reyk Floeter @@ -4171,7 +4171,6 @@ ikev2_childsa_negotiate(struct iked *env, struct iked_sa *sa, struct iked_transform *xform, *encrxf = NULL, *integrxf = NULL; struct iked_childsa *csa, *csb; struct iked_flow *flow, *saflow, *flowa, *flowb; - struct iked_id *peerid, *localid; struct ibuf *keymat = NULL, *seed = NULL, *dhsecret = NULL; struct group *group; u_int32_t spi = 0; @@ -4182,15 +4181,7 @@ ikev2_childsa_negotiate(struct iked *env, struct iked_sa *sa, if (!sa_stateok(sa, IKEV2_STATE_VALID)) return (-1); - if (sa->sa_hdr.sh_initiator) { - peerid = &sa->sa_rid; - localid = &sa->sa_iid; - } else { - peerid = &sa->sa_iid; - localid = &sa->sa_rid; - } - - if (ikev2_sa_tag(sa, peerid) == -1) + if (ikev2_sa_tag(sa, IKESA_DSTID(sa)) == -1) return (-1); /* We need to determine the key material length first */ @@ -4289,8 +4280,6 @@ ikev2_childsa_negotiate(struct iked *env, struct iked_sa *sa, memcpy(flowa, flow, sizeof(*flow)); flowa->flow_dir = IPSP_DIRECTION_OUT; flowa->flow_saproto = prop->prop_protoid; - flowa->flow_srcid = localid; - flowa->flow_dstid = peerid; flowa->flow_local = &sa->sa_local; flowa->flow_peer = &sa->sa_peer; flowa->flow_ikesa = sa; @@ -4330,8 +4319,6 @@ ikev2_childsa_negotiate(struct iked *env, struct iked_sa *sa, csa->csa_saproto = prop->prop_protoid; csa->csa_ikesa = sa; - csa->csa_srcid = localid; - csa->csa_dstid = peerid; csa->csa_spi.spi_protoid = prop->prop_protoid; csa->csa_esn = esn; @@ -4481,13 +4468,6 @@ ikev2_ipcomp_enable(struct iked *env, struct iked_sa *sa) /* install IPCOMP SAs */ csa->csa_ikesa = sa; csa->csa_saproto = IKEV2_SAPROTO_IPCOMP; - if (sa->sa_hdr.sh_initiator) { - csa->csa_dstid = &sa->sa_rid; - csa->csa_srcid = &sa->sa_iid; - } else { - csa->csa_dstid = &sa->sa_iid; - csa->csa_srcid = &sa->sa_rid; - } csa->csa_spi.spi_size = 2; csa->csa_spi.spi = sa->sa_cpi_out; csa->csa_peerspi = sa->sa_cpi_in; @@ -4557,13 +4537,6 @@ ikev2_ipcomp_enable(struct iked *env, struct iked_sa *sa) /* setup ESP flows for gateways */ flowa->flow_dir = IPSP_DIRECTION_OUT; flowa->flow_saproto = IKEV2_SAPROTO_ESP; - if (sa->sa_hdr.sh_initiator) { - flowa->flow_dstid = &sa->sa_rid; - flowa->flow_srcid = &sa->sa_iid; - } else { - flowa->flow_dstid = &sa->sa_iid; - flowa->flow_srcid = &sa->sa_rid; - } flowa->flow_local = &sa->sa_local; flowa->flow_peer = &sa->sa_peer; memcpy(&flowa->flow_src, &sa->sa_local, sizeof(sa->sa_local)); diff --git a/sbin/iked/pfkey.c b/sbin/iked/pfkey.c index afe2d607829..b92138cb7f6 100644 --- a/sbin/iked/pfkey.c +++ b/sbin/iked/pfkey.c @@ -1,4 +1,4 @@ -/* $OpenBSD: pfkey.c,v 1.36 2014/05/09 06:29:46 markus Exp $ */ +/* $OpenBSD: pfkey.c,v 1.37 2014/05/09 06:37:24 markus Exp $ */ /* * Copyright (c) 2010-2013 Reyk Floeter @@ -312,11 +312,11 @@ pfkey_flow(int sd, u_int8_t satype, u_int8_t action, struct iked_flow *flow) (sizeof(sa_peer) + ROUNDUP(speer.ss_len)) / 8; /* local id */ - sa_srcid = pfkey_id2ident(flow->flow_srcid, + sa_srcid = pfkey_id2ident(IKESA_SRCID(flow->flow_ikesa), SADB_EXT_IDENTITY_SRC); /* peer id */ - sa_dstid = pfkey_id2ident(flow->flow_dstid, + sa_dstid = pfkey_id2ident(IKESA_DSTID(flow->flow_ikesa), SADB_EXT_IDENTITY_DST); } @@ -578,11 +578,18 @@ pfkey_sa(int sd, u_int8_t satype, u_int8_t action, struct iked_childsa *sa) if (satype == SADB_X_SATYPE_IPCOMP) sadb.sadb_sa_encrypt = SADB_X_CALG_DEFLATE; - /* local id */ - sa_srcid = pfkey_id2ident(sa->csa_srcid, SADB_EXT_IDENTITY_SRC); - - /* peer id */ - sa_dstid = pfkey_id2ident(sa->csa_dstid, SADB_EXT_IDENTITY_DST); + /* Note that we need to swap the IDs for incoming SAs (SADB_UPDATE) */ + if (action != SADB_UPDATE) { + sa_srcid = pfkey_id2ident( + IKESA_SRCID(sa->csa_ikesa), SADB_EXT_IDENTITY_SRC); + sa_dstid = pfkey_id2ident( + IKESA_DSTID(sa->csa_ikesa), SADB_EXT_IDENTITY_DST); + } else { + sa_srcid = pfkey_id2ident( + IKESA_DSTID(sa->csa_ikesa), SADB_EXT_IDENTITY_SRC); + sa_dstid = pfkey_id2ident( + IKESA_SRCID(sa->csa_ikesa), SADB_EXT_IDENTITY_DST); + } tag = sa->csa_ikesa->sa_tag; if (tag != NULL && *tag != '\0') { -- 2.20.1