Check for failures of exchange_establish_p{1,2}() and call the given

`finalize' function with the `fail' argument when this happen.

Introduce some sanity checks in exchange_free() to be able to call if
even if the data structure isn't completely initialized.

Plug memory leaks when exchange_establish() fails.  While here fix a
double free in one of the error paths.

Based on a diff from hshoexer@, ok stsp@, markus@

2 files changed, 57 insertions, 34 deletions

diff --git a/sbin/isakmpd/exchange.c b/sbin/isakmpd/exchange.c
index e06ef88f7cf..7a6575e1637 100644
--- a/sbin/isakmpd/exchange.c
+++ b/sbin/isakmpd/exchange.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: exchange.c,v 1.138 2016/03/10 07:32:16 yasuoka Exp $	 */
+/* $OpenBSD: exchange.c,v 1.139 2017/09/18 07:42:52 mpi Exp $	 */
 /* $EOM: exchange.c,v 1.143 2000/12/04 00:02:25 angelos Exp $	 */
 
 /*
@@ -529,6 +529,7 @@ exchange_enter(struct exchange *exchange)
 	}
 	bucket &= bucket_mask;
 	LIST_INSERT_HEAD(&exchange_tab[bucket], exchange, link);
+	exchange->linked = 1;
 }
 
 /*
@@ -703,7 +704,7 @@ exchange_establish_transaction(struct exchange *exchange, void *arg, int fail)
 }
 
 /* Establish a phase 1 exchange.  */
-void
+int
 exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
     char *name, void *args, void (*finalize)(struct exchange *, void *, int),
     void *arg, int stayalive)
@@ -738,7 +739,7 @@ exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
 			else {
 				log_print("exchange_establish_p1: "
 				    "DOI \"%s\" unsupported", str);
-				return;
+				return -1;
 			}
 
 			/* What exchange type do we want?  */
@@ -747,20 +748,19 @@ exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
 				log_print("exchange_establish_p1: "
 				    "no \"EXCHANGE_TYPE\" tag in [%s] section",
 				    tag);
-				return;
+				return -1;
 			}
 			type = constant_value(isakmp_exch_cst, str);
 			if (!type) {
 				log_print("exchange_establish_p1: "
 				    "unknown exchange type %s", str);
-				return;
+				return -1;
 			}
 		}
 	}
 	exchange = exchange_create(1, 1, doi, type);
 	if (!exchange) {
-		/* XXX Do something here?  */
-		return;
+		return -1;
 	}
 	if (name) {
 		exchange->name = strdup(name);
@@ -768,7 +768,7 @@ exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
 			log_error("exchange_establish_p1: "
 			    "strdup (\"%s\") failed", name);
 			exchange_free(exchange);
-			return;
+			return -1;
 		}
 	}
 	exchange->policy = name ? conf_get_str(name, "Configuration") : 0;
@@ -787,7 +787,7 @@ exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
 					    "calloc (1, %lu) failed",
 					    (unsigned long)sizeof(*node));
 					exchange_free(exchange);
-					return;
+					return -1;
 				}
 				/*
 				 * Insert this finalization inbetween
@@ -813,7 +813,7 @@ exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
 	if (!msg) {
 		log_print("exchange_establish_p1: message_alloc () failed");
 		exchange_free(exchange);
-		return;
+		return 0; /* exchange_free() runs finalize */
 	}
 	msg->exchange = exchange;
 
@@ -828,10 +828,9 @@ exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
 		sa_create(exchange, 0);
 		msg->isakmp_sa = TAILQ_FIRST(&exchange->sa_list);
 		if (!msg->isakmp_sa) {
-			/* XXX Do something more here?  */
 			message_free(msg);
 			exchange_free(exchange);
-			return;
+			return 0; /* exchange_free() runs finalize */
 		}
 		sa_reference(msg->isakmp_sa);
 
@@ -841,10 +840,11 @@ exchange_establish_p1(struct transport *t, u_int8_t type, u_int32_t doi,
 	msg->extra = args;
 
 	exchange_run(msg);
+	return 0;
 }
 
 /* Establish a phase 2 exchange.  XXX With just one SA for now.  */
-void
+int
 exchange_establish_p2(struct sa *isakmp_sa, u_int8_t type, char *name,
     void *args, void (*finalize)(struct exchange *, void *, int), void *arg)
 {
@@ -864,7 +864,7 @@ exchange_establish_p2(struct sa *isakmp_sa, u_int8_t type, char *name,
 		if (!tag) {
 			log_print("exchange_establish_p2: "
 			    "no configuration for peer \"%s\"", name);
-			return;
+			return -1;
 		}
 		seq = (u_int32_t)conf_get_num(name, "Acquire-ID", 0);
 
@@ -877,7 +877,7 @@ exchange_establish_p2(struct sa *isakmp_sa, u_int8_t type, char *name,
 		else {
 			log_print("exchange_establish_p2: "
 			    "DOI \"%s\" unsupported", str);
-			return;
+			return -1;
 		}
 
 		/* What exchange type do we want?  */
@@ -887,21 +887,20 @@ exchange_establish_p2(struct sa *isakmp_sa, u_int8_t type, char *name,
 				log_print("exchange_establish_p2: "
 				    "no \"EXCHANGE_TYPE\" tag in [%s] section",
 				    tag);
-				return;
+				return -1;
 			}
 			/* XXX IKE dependent.  */
 			type = constant_value(ike_exch_cst, str);
 			if (!type) {
 				log_print("exchange_establish_p2: unknown "
 				    "exchange type %s", str);
-				return;
+				return -1;
 			}
 		}
 	}
 	exchange = exchange_create(2, 1, doi, type);
 	if (!exchange) {
-		/* XXX Do something here?  */
-		return;
+		return -1;
 	}
 	if (name) {
 		exchange->name = strdup(name);
@@ -909,7 +908,7 @@ exchange_establish_p2(struct sa *isakmp_sa, u_int8_t type, char *name,
 			log_error("exchange_establish_p2: "
 			    "strdup (\"%s\") failed", name);
 			exchange_free(exchange);
-			return;
+			return -1;
 		}
 	}
 	exchange->policy = name ? conf_get_str(name, "Configuration") : 0;
@@ -936,7 +935,7 @@ exchange_establish_p2(struct sa *isakmp_sa, u_int8_t type, char *name,
 		for (i = 0; i < 1; i++)
 			if (sa_create(exchange, isakmp_sa->transport)) {
 				exchange_free(exchange);
-				return;
+				return 0; /* exchange_free() runs finalize */
 			}
 	}
 	msg = message_alloc(isakmp_sa->transport, 0, ISAKMP_HDR_SZ);
@@ -949,6 +948,8 @@ exchange_establish_p2(struct sa *isakmp_sa, u_int8_t type, char *name,
 	msg->exchange = exchange;
 
 	exchange_run(msg);
+
+	return 0;
 }
 
 /* Out of an incoming phase 1 message, setup an exchange.  */
@@ -1200,9 +1201,11 @@ exchange_free_aux(void *v_exch)
 	free(exchange->id_i);
 	free(exchange->id_r);
 	free(exchange->keystate);
-	if (exchange->doi && exchange->doi->free_exchange_data)
-		exchange->doi->free_exchange_data(exchange->data);
-	free(exchange->data);
+	if (exchange->data) {
+		if (exchange->doi && exchange->doi->free_exchange_data)
+			exchange->doi->free_exchange_data(exchange->data);
+		free(exchange->data);
+	}
 	free(exchange->name);
 	if (exchange->recv_cert) {
 		handler = cert_get(exchange->recv_certtype);
@@ -1223,7 +1226,10 @@ exchange_free_aux(void *v_exch)
 		kn_close(exchange->policy_id);
 
 	exchange_free_aca_list(exchange);
-	LIST_REMOVE(exchange, link);
+	if (exchange->linked) {
+		LIST_REMOVE(exchange, link);
+		exchange->linked = 0;
+	}
 
 	/* Tell potential finalize routine we never got there.  */
 	if (exchange->finalize)
@@ -1260,6 +1266,7 @@ exchange_upgrade_p1(struct message *msg)
 	struct exchange *exchange = msg->exchange;
 
 	LIST_REMOVE(exchange, link);
+	exchange->linked = 0;
 	GET_ISAKMP_HDR_RCOOKIE(msg->iov[0].iov_base, exchange->cookies +
 	    ISAKMP_HDR_ICOOKIE_LEN);
 	exchange_enter(exchange);
@@ -1746,6 +1753,8 @@ exchange_establish(char *name, void (*finalize)(struct exchange *, void *,
 		LOG_DBG((LOG_EXCHANGE, 40, "exchange_establish:"
 		    " returning in passive mode for exchange %s phase %d",
 		    name, phase));
+		if (finalize)
+			finalize(0, arg, 1);
 		return;
 	}
 
@@ -1772,10 +1781,13 @@ exchange_establish(char *name, void (*finalize)(struct exchange *, void *,
 		if (!transport) {
 			log_print("exchange_establish: transport \"%s\" for "
 			    "peer \"%s\" could not be created", trpt, name);
+			if (finalize)
+				finalize(0, arg, 1);
 			return;
 		}
-		exchange_establish_p1(transport, 0, 0, name, 0, finalize, arg,
-		    stayalive);
+		if (exchange_establish_p1(transport, 0, 0, name, 0, finalize,
+		    arg, stayalive) < 0 && finalize)
+			finalize(0, arg, 1);
 		break;
 
 	case 2:
@@ -1783,20 +1795,27 @@ exchange_establish(char *name, void (*finalize)(struct exchange *, void *,
 		if (!peer) {
 			log_print("exchange_establish: No ISAKMP-peer given "
 			    "for \"%s\"", name);
+			if (finalize)
+				finalize(0, arg, 1);
 			return;
 		}
 		isakmp_sa = sa_lookup_by_name(peer, 1);
 		if (!isakmp_sa) {
+			/* freed by exchange_establish_finalize() */
 			name = strdup(name);
 			if (!name) {
 				log_error("exchange_establish: "
 				    "strdup (\"%s\") failed", name);
+				if (finalize)
+					finalize(0, arg, 1);
 				return;
 			}
 			if (conf_get_num(peer, "Phase", 0) != 1) {
 				log_print("exchange_establish: "
 				    "[%s]:ISAKMP-peer's (%s) phase is not 1",
 				    name, peer);
+				if (finalize)
+					finalize(0, arg, 1);
 				free(name);
 				return;
 			}
@@ -1823,12 +1842,13 @@ exchange_establish(char *name, void (*finalize)(struct exchange *, void *,
 				/* Indicate failure */
 				if (finalize)
 					finalize(0, arg, 1);
-				free(name);
 			}
 			return;
-		} else
-			exchange_establish_p2(isakmp_sa, 0, name, 0, finalize,
-			    arg);
+		} else {
+			if (exchange_establish_p2(isakmp_sa, 0, name, 0,
+			    finalize, arg) < 0 && finalize)
+				finalize(0, arg, 1);
+		}
 		break;
 
 	default:
diff --git a/sbin/isakmpd/exchange.h b/sbin/isakmpd/exchange.h
index 35119c9fce8..e34f85d264a 100644
--- a/sbin/isakmpd/exchange.h
+++ b/sbin/isakmpd/exchange.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: exchange.h,v 1.34 2015/01/16 06:39:58 deraadt Exp $	 */
+/* $OpenBSD: exchange.h,v 1.35 2017/09/18 07:42:52 mpi Exp $	 */
 /* $EOM: exchange.h,v 1.28 2000/09/28 12:54:28 niklas Exp $	 */
 
 /*
@@ -55,6 +55,9 @@ struct exchange {
 	/* Link to exchanges with the same hash value.  */
 	LIST_ENTRY(exchange) link;
 
+	/* This exchange is linked to the global exchange list. */
+	int		linked;
+
 	/* A name of the SAs this exchange will result in.  XXX non unique?  */
 	char           *name;
 
@@ -229,10 +232,10 @@ extern void     exchange_free(struct exchange *);
 extern void     exchange_free_aca_list(struct exchange *);
 extern void     exchange_establish(char *name, void (*)(struct exchange *,
 		    void *, int), void *, int);
-extern void	exchange_establish_p1(struct transport *, u_int8_t, u_int32_t,
+extern int	exchange_establish_p1(struct transport *, u_int8_t, u_int32_t,
 		    char *, void *, void (*)(struct exchange *, void *, int),
 		    void *, int);
-extern void     exchange_establish_p2(struct sa *, u_int8_t, char *, void *,
+extern int      exchange_establish_p2(struct sa *, u_int8_t, char *, void *,
 		    void (*)(struct exchange *, void *, int), void *);
 extern int      exchange_gen_nonce(struct message *, size_t);
 extern void     exchange_init(void);