From 12aa06ccc98cf8a912eb2ea54f02d234f2f8c173 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Tue, 14 Jan 2020 19:52:54 +0000 Subject: [PATCH] Fix double-free in remote->local forwardings. This bug applies to both the new stream-based agent forwarding, and ordinary remote->local TCP port forwardings, because it was introduced by the preliminary infrastructure in commit 09954a87c. new_connection() and sk_new() accept a SockAddr *, and take ownership of it. So it's a mistake to make an address, connect to it, and then sk_addr_free() it: the free will decrement its reference count to zero, and then the Socket made by the connection will be holding a stale pointer. But that's exactly what I was doing in the version of portfwdmgr_connect() that I rewrote in that refactoring. And then I made the same error again in commit ae1148267 in the Unix stream-based agent forwarding. Now both fixed. Rather than remove the sk_addr_free() to make the code look more like it used to, I've instead solved the problem by adding an sk_addr_dup() at the point of making the connection. The idea is that that should be more robust, in that it will still do the right thing if portfwdmgr_connect_socket should later change so as not to call its connect helper function at all. The new Windows stream-based agent forwarding is unaffected by this bug, because it calls new_named_pipe_client() with a pathname in string format, without first wrapping it into a SockAddr. --- portfwd.c | 5 +++-- unix/uxagentc.c | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/portfwd.c b/portfwd.c index 8e5f087c..2f3ee6e5 100644 --- a/portfwd.c +++ b/portfwd.c @@ -1129,8 +1129,9 @@ struct portfwdmgr_connect_ctx { static Socket *portfwdmgr_connect_helper(void *vctx, Plug *plug) { struct portfwdmgr_connect_ctx *ctx = (struct portfwdmgr_connect_ctx *)vctx; - return new_connection(ctx->addr, ctx->canonical_hostname, ctx->port, - false, true, false, false, plug, ctx->conf); + return new_connection(sk_addr_dup(ctx->addr), ctx->canonical_hostname, + ctx->port, false, true, false, false, plug, + ctx->conf); } /* diff --git a/unix/uxagentc.c b/unix/uxagentc.c index b937e7e4..cc6063fd 100644 --- a/unix/uxagentc.c +++ b/unix/uxagentc.c @@ -137,7 +137,7 @@ struct agent_connect_ctx { Socket *agent_connect(void *vctx, Plug *plug) { agent_connect_ctx *ctx = (agent_connect_ctx *)vctx; - return sk_new(ctx->addr, 0, false, false, false, false, plug); + return sk_new(sk_addr_dup(ctx->addr), 0, false, false, false, false, plug); } agent_connect_ctx *agent_get_connect_ctx(void)