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

[Paul Wouters]

On Mon, 2 Oct 2017, Antony Antony wrote:

>> 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

This would skip restarting all instance connections. That would affect
all connections with narrowing even if they do not have right=%any and
would break any CP based connection on the initiator by ignoring
liveness failures.

I guess a slightly less bad check would be for rekey=no or right=%any
and skip those.

Ideally, I guess we need to somehow find a better way to mark a
connection as been created by a server for a roadwarrior client,
where we are not expected to initiate towards to client.

This could be done based on modecfgserver=yes ?

> 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.

Yeah, that requires updating the concept of hostpair to take into
account the ID as well as the IP.

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

Tuomo and I have talked about the restart keyword in the past. It seems
that DPD/liveness should only do hold or clear, and other properties
of the connection should either cause delete or re-initiate.

Paul