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 5c33685..1c6ee7c 100644 (file)
--- 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 0d1774e..02fdf02 100644 (file)
--- 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 5612729..e5c553e 100644 (file)
--- 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);
 }