[Swan-dev] crash during testing xauth (2) when processing dpd event

[Antony Antony]

On Sat, Sep 30, 2017 at 08:18:11PM -0400, D. Hugh Redelmeier wrote:
> Same context as (1).
> 
> testing/pluto/xauth-pluto-17 failed east:CORE,output-different road:output-different
> 
> Core was generated by `/usr/local/libexec/ipsec/pluto --leak-detective --config /etc/ipsec.conf --nofo'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  restart_connections_by_peer (c=c at entry=0x7f5b64649998) at /source/programs/pluto/initiate.c:434
> 434                     if (same_host(dnshostname, &host_addr,
> #0  restart_connections_by_peer (c=c at entry=0x7f5b64649998) at /source/programs/pluto/initiate.c:434
> #1  0x00007f5b62997791 in liveness_action (c=0x7f5b64649998, ikev2=<optimized out>) at /source/programs/pluto/connections.c:4401
> #2  0x00007f5b629bc946 in p2_dpd_outI1 (p2st=0x7f5b6464bb18) at /source/programs/pluto/ikev1_dpd.c:362
> #3  dpd_event (st=st at entry=0x7f5b6464bb18) at /source/programs/pluto/ikev1_dpd.c:378
> #4  0x00007f5b629a9658 in timer_event_cb (fd=<optimized out>, event=<optimized out>, arg=<optimized out>) at /source/programs/pluto/timer.c:894
> #5  0x00007f5b607183cc in event_process_active_single_queue (activeq=0x7f5b6463ae70, base=0x7f5b6463aa50) at event.c:1350
> #6  event_process_active (base=<optimized out>) at event.c:1420
> #7  event_base_loop (base=0x7f5b6463aa50, flags=flags at entry=0) at event.c:1621
> #8  0x00007f5b629a673d in main_loop () at /source/programs/pluto/server.c:813
> #9  call_server () at /source/programs/pluto/server.c:946
> #10 0x00007f5b629720d6 in main (argc=<optimized out>, argv=<optimized out>) at /source/programs/pluto/plutomain.c:1812
> 
> At the point of the crash, c points to a connection that is all 0xef
> bytes.  You can see this with the GDB command "p/x c[0]".  That
> suggests that something has deleted this connection.
> 
> Earlier in the same function invocation, c is NOT all 0xef.  Otherwise
> the code would have crashed earlier.  For example, the expression
> c->dnshostname is dereferenced in a call to clone_str.
> 
> The pointer hp points at a bunch of 0xef bytes.  This cannot have been
> the case when it was initialized at the start of the function: it is
> dereferenced to initialize d in the first for loop.
> 
> c_kind is CK_INSTANCE.  So the preceding if-body was not executed.
> dnshostname is NULL;
> d is 0xef...ef
> host_addr is 192.1.3.209
> 
> Theory: the ??? comment is right and terminate_connection has deleted
> the connection (and host pair).

Two years later the bug surfaced. Good catch!!

> Who thinks that they should fix this?

here is my proposed fix. Any comments? I tested using xauth-pluto-17

I am, still, suspecious of restart code. If there are multiple connections 
from same NAT GW it would restart all of them when one dpd fails. Probably 
for another day. Lets fix this crash first.

Also the test is weired. The combination IKEv1 aggressive mode, xauth , 
%any, dpdaction=%restart. A bad combination except for testing!

-antony
-------------- next part --------------
>From 9c73066c50f1abe8a7a5085dcf1b7b329dfc1a39 Mon Sep 17 00:00:00 2001
From: Antony Antony <antony at phenome.org>
Date: Mon, 2 Oct 2017 01:08:37 +0200
Subject: [PATCH] dpd: do not restart CK_INSTATNCE conn with dpdaction%restart

terminate_connection() deletes a CK_INSTATNCE and hp is dangling,
so nothing to restart. 
Note: dpdaction=%restart and %any should not allowed.
---
 programs/pluto/initiate.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/programs/pluto/initiate.c b/programs/pluto/initiate.c
index 6526caa24..987ac9f17 100644
--- a/programs/pluto/initiate.c
+++ b/programs/pluto/initiate.c
@@ -415,8 +415,6 @@ void restart_connections_by_peer(struct connection *const c)
 		if (same_host(dnshostname, &host_addr,
 				d->dnshostname, &d->spd.that.host_addr))
 		{
-			/* This might delete c if CK_INSTANCE */
-			/* ??? is there a chance hp becomes dangling? */
 			terminate_connection(d->name);
 		}
 		d = next;
@@ -428,13 +426,13 @@ void restart_connections_by_peer(struct connection *const c)
 		/* host_pair/host_addr changes with dynamic dns */
 		hp = c->host_pair;
 		host_addr = c->spd.that.host_addr;
-	}
 
-	for (d = hp->connections; d != NULL; d = d->hp_next) {
-		if (same_host(dnshostname, &host_addr,
-				d->dnshostname, &d->spd.that.host_addr))
-			initiate_connection(d->name, NULL_FD, LEMPTY,
-					pcim_demand_crypto, NULL);
+		for (d = hp->connections; d != NULL; d = d->hp_next) {
+			if (same_host(dnshostname, &host_addr,
+					d->dnshostname, &d->spd.that.host_addr))
+				initiate_connection(d->name, NULL_FD, LEMPTY,
+						pcim_demand_crypto, NULL);
+		}
 	}
 	pfreeany(dnshostname);
 }
-- 
2.13.5