From: markus <markus@openbsd.org>
Date: Tue, 6 May 2014 09:48:40 +0000 (+0000)
Subject: cleanup IKE-SA tree handling (fixes repeated-insert & double-remove)
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=4697d2e3bd014797ed8b327306077d52897f4db3;p=openbsd

cleanup IKE-SA tree handling (fixes repeated-insert & double-remove)

sa_new() always re-inserts an SA into the SA tree. in case of a key
collision it would try to free the new SA. While doing that it would
accidentially free the existing SA, since config_free_sa() does
RB_REMOVE() uncoditionally.  This change fixes this by:
a) moving the responsibility for RB_REMOVE() to CALLER of config_free_sa() and
b) by calling config_free_sa() instead of sa_free() from sa_new()
It also changes to code to NEVER re-add an SA to the tree. So
RB_INSERT() is ONLY called once per SA. The code also makes sure
that there is always a KEY defined for this tree (ispi).

ok mikeb@
---

diff --git a/sbin/iked/config.c b/sbin/iked/config.c
index 5c33685bbb9..1c6ee7c4756 100644
--- a/sbin/iked/config.c
+++ b/sbin/iked/config.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: config.c,v 1.28 2014/05/06 07:24:37 markus Exp $	*/
+/*	$OpenBSD: config.c,v 1.29 2014/05/06 09:48:40 markus Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -78,8 +78,6 @@ config_getspi(void)
 void
 config_free_sa(struct iked *env, struct iked_sa *sa)
 {
-	(void)RB_REMOVE(iked_sas, &env->sc_sas, sa);
-
 	timer_del(env, &sa->sa_timer);
 
 	config_free_proposals(&sa->sa_proposals, 0);
@@ -471,6 +469,7 @@ config_getreset(struct iked *env, struct imsg *imsg)
 		for (sa = RB_MIN(iked_sas, &env->sc_sas);
 		    sa != NULL; sa = nextsa) {
 			nextsa = RB_NEXT(iked_sas, &env->sc_sas, sa);
+			RB_REMOVE(iked_sas, &env->sc_sas, sa);
 			config_free_sa(env, sa);
 		}
 	}
diff --git a/sbin/iked/ikev2.c b/sbin/iked/ikev2.c
index 0d1774eeadd..02fdf026d09 100644
--- a/sbin/iked/ikev2.c
+++ b/sbin/iked/ikev2.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: ikev2.c,v 1.105 2014/05/06 08:17:58 markus Exp $	*/
+/*	$OpenBSD: ikev2.c,v 1.106 2014/05/06 09:48:40 markus Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -661,7 +661,8 @@ ikev2_init_recv(struct iked *env, struct iked_message *msg,
 		    betoh64(hdr->ike_ispi), betoh64(hdr->ike_rspi), 1,
 		    NULL)) == NULL || sa != msg->msg_sa) {
 			log_debug("%s: invalid new SA", __func__);
-			sa_free(env, sa);
+			if (sa)
+				sa_free(env, sa);
 		}
 		break;
 	case IKEV2_EXCHANGE_IKE_AUTH:
diff --git a/sbin/iked/policy.c b/sbin/iked/policy.c
index 561272931d4..e5c553ec69e 100644
--- a/sbin/iked/policy.c
+++ b/sbin/iked/policy.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: policy.c,v 1.32 2014/04/29 11:51:13 markus Exp $	*/
+/*	$OpenBSD: policy.c,v 1.33 2014/05/06 09:48:40 markus Exp $	*/
 
 /*
  * Copyright (c) 2010-2013 Reyk Floeter <reyk@openbsd.org>
@@ -310,12 +310,29 @@ sa_new(struct iked *env, u_int64_t ispi, u_int64_t rspi,
 	if ((ispi == 0 && rspi == 0) ||
 	    (sa = sa_lookup(env, ispi, rspi, initiator)) == NULL) {
 		/* Create new SA */
+		if (!initiator && ispi == 0) {
+			log_debug("%s: cannot create responder IKE SA w/o ispi",
+			    __func__);
+			return (NULL);
+		}
 		sa = config_new_sa(env, initiator);
+		if (sa == NULL) {
+			log_debug("%s: failed to allocate IKE SA", __func__);
+			return (NULL);
+		}
+		if (!initiator)
+			sa->sa_hdr.sh_ispi = ispi;
+		old = RB_INSERT(iked_sas, &env->sc_sas, sa);
+		if (old && old != sa) {
+			log_warnx("%s: duplicate IKE SA", __func__);
+			config_free_sa(env, sa);
+			return (NULL);
+		}
 	}
-	if (sa == NULL) {
-		log_debug("%s: failed to get sa", __func__);
-		return (NULL);
-	}
+	/* Update rspi in the initator case */
+	if (initiator && sa->sa_hdr.sh_rspi == 0 && rspi)
+		sa->sa_hdr.sh_rspi = rspi;
+
 	if (sa->sa_policy == NULL)
 		sa->sa_policy = pol;
 	else
@@ -345,19 +362,6 @@ sa_new(struct iked *env, u_int64_t ispi, u_int64_t rspi,
 		return (NULL);
 	}
 
-	if (sa->sa_hdr.sh_ispi == 0)
-		sa->sa_hdr.sh_ispi = ispi;
-	if (sa->sa_hdr.sh_rspi == 0)
-		sa->sa_hdr.sh_rspi = rspi;
-
-	/* Re-insert node into the tree */
-	old = RB_INSERT(iked_sas, &env->sc_sas, sa);
-	if (old && old != sa) {
-		log_debug("%s: duplicate ikesa", __func__);
-		sa_free(env, sa);
-		return (NULL);
-	}
-
 	return (sa);
 }
 
@@ -368,6 +372,7 @@ sa_free(struct iked *env, struct iked_sa *sa)
 	    print_spi(sa->sa_hdr.sh_ispi, 8),
 	    print_spi(sa->sa_hdr.sh_rspi, 8));
 
+	RB_REMOVE(iked_sas, &env->sc_sas, sa);
 	config_free_sa(env, sa);
 }