By reviewing my last isakmpd(8) diff to fix a use-after-free, hshoexer@
pointed out that if exchange_establish() fails, `arg' is leaked. This is
not a new issue. However it generally happens under memory pressure,
and when you're under memory pressure leaking memory is not a clever
thing to do.
In order to prevent this memory leak, we have to:
- check for failures of exchange_establish_p{1,2}() and call the given
`finalize' function with the `fail' argument when this happen.
Because now the various finalize functions correctly free their
corresponding argument.
- introduce some sanity checks in exchange_free() to be able to call
if even if the data structure isn't completely initialized.
Diff below does that. It also includes a fix for a double free in one
of the error paths of exchange_establish().
ok?
Index: exchange.c
===================================================================
RCS file: /cvs/src/sbin/isakmpd/exchange.c,v
retrieving revision 1.138
diff -u -p -r1.138 exchange.c
--- exchange.c 10 Mar 2016 07:32:16 -0000 1.138
+++ exchange.c 22 Aug 2017 08:45:21 -0000
@@ -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 ex
}
/* 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 *
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 *
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 *
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 *
"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 *
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 *
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 *
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_
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_
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_
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_
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_
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_
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 (*fi
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 (*fi
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 (*fi
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 (*fi
/* 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:
Index: exchange.h
===================================================================
RCS file: /cvs/src/sbin/isakmpd/exchange.h,v
retrieving revision 1.34
diff -u -p -r1.34 exchange.h
--- exchange.h 16 Jan 2015 06:39:58 -0000 1.34
+++ exchange.h 6 Aug 2017 13:54:41 -0000
@@ -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 exc
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);
