On Fri, Sep 01, 2017 at 11:58:46AM +0200, Martin Pieuchot wrote:
> On 22/08/17(Tue) 10:50, Martin Pieuchot wrote:
> > 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?
>
> Anyone?
The original code is clearly lacking error handling, and I cannot
spot a mistake in the error handling you've added.
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);
> >
>
