[Swan-dev] crash after pluto: Fix addresspool reference count

[wolfgang at linogate.de]

On Sat, 7 Oct 2017 11:40:19 +0200, Antony Antony wrote
> Hi Wolfgang,
> I couldn't stay away from this mystery since I already spend days on 
> it.
> 
> One line summary, I can reproduce lsw299. And need to define some 
> things before fixing it. There is a partial workaround to get 
> connections established.
> 
> I applied patch to the test case and forked it to xauth-pluto-25-lsw299.
> Now thinking what the fix should be.
> 
> >From a higher level the issue is triggered due to a double assignment of IP
> address. The addresspool and password file assign the same address. 
> I think this behavior is not handled, and lead to crash. Though 
> pluto should not crash, this is arguably a a wrong configuration.
> 
> To test the theory assign IP address in the password file to one 
> outside the pool. Connections come up. However, 'ipsec stop' would 
> hit an assert, which might be an issue with the original patch.
> 
> -echo "xroad:xOzlFlqtwJIu2:east-any:192.0.2.100" >> /etc/ipsec.d/passwd
> +echo "xroad:xOzlFlqtwJIu2:east-any:192.0.2.201" >> /etc/ipsec.d/passwd
> 
> Could you please test this? Atleast connections would come up. (no 
> IPsec stop or --delete yet).
> 
> A simple ,in theory, possibly hard to get right, fix would be no 
> support for double, or multiple 1. And fix "ipsec stop" by changing 
> rel_lease_addr to handle an address from file properly.
> 
> Uniqueness of assigned address would be a nice thing. One could 
> possibly argue Uniqueness is not necessary with netkey stack and a 
> typical access vpn would work fine without uniqueness. However,
>  debugging would get complicated, everything will be decided on SPI 
> + IP, when IP addresses are not unique.
> 
> Another approach was possibly mentioned before. If the address from 
> the file overlap with the addresspool, no matter it is assigned in 
> the addresspool or not, do not again it. However, it would be hard 
> to get this right in the entire life time of pluto. Address from the 
> file could overlap with another pool, from another connection, and 
> cause a duplicate assignment. One would think lets check all 
> addresspools known to pluto at the time of assignment from file. 
> This assume no addresspools are added later on:) A bit more compute 
> intensive approach would be, also check all existing leases when 
> adding new addresspool.
> 
> Pluto do not allow addresspool overlap. When adding a new 
> addresspool, connection with a pool, there is a check for overlap 
> with other addresspools and refuse the overlaping connection. We 
> could extend the overlap check to to also check assigned address 
> from files. Note pools can be shared between connections if they are 
> exact size. One complexity is the xauth file may change on its own. 
> I am not sure what this means.
> 
> Thanks again the patch. Crash is no more a mystery! And we have 
> example how to use this feature.
> 
> On Fri, Oct 06, 2017 at 09:29:33PM +0200, wolfgang at linogate.de wrote:
> > I have added a patch for you for the xauth-pluto-22 test to reproduce 
> > lsw299
> > with v3.21 and it also triggers the rel_lease_addr crash with my actual patch.
> 
> I am think rel_lease_addr need fixing. Warning I didn't not look at 
> the code only focused on re-producing.
> 
> > The actual problems seems to be when installing a new addresspool from
> > ikev1_xauth.c.
> > This code is initially from me and I think when I implemented it I overlooked
> > that the pool is shared and not copied for the instance.
> > I can look to rework it next week.
> 
> Added test with ipsec stop, xauth-pluto-26.
> 
> I wonder ipsec stop , with password file, is yet to implemented. I 
> need to double check.  xauth-pluto-26 has just one client, address 
> assignment from the file, there is a pool defined. The tunnel come 
> up fine, however, pluto seems to crash on "ipsec stop". I will stop 
> here for now.
> 
> ABORT: ASSERTION FAILED: pool->pool_refcount > 0 (in 
> unreference_addresspool() at addresspool.c:441)
> 
> -antony

I also couldn't stay away and found some time today to look into it. I have
added a solution and two test cases to lsw299, which I think worked now properly.

We use this feature for years without problems. Sure it is not optimal, but it
works. The static address pool is only temporary installed to assign the user
defined static ip to the client and deleted once the instance is gone.

Having multiple address pools on one connection would be a nice thing, but I
think it is not easy to implement.

Overlapping ip addresses in global and static pools are configuration problems
and the log clearly show the user that he need to configure to separate pools.

Wolfgang