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?
>
> 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);
>
