Page MenuHomeVyOS Platform

SWANCTL: DMVPN: ALL peers are deleted in swan when opennhrp tries to delete ONE peer
Needs testing, NormalPublic

Description

There is a serius bug in the strongswan patch for dmvpn.
The fault happens when opennhrp tries to tear down a connection (swanctl -t).
While opennhrp specifies both source -S and destination -R options swanctl actually deletes all the peers

to recreate a setup:
set up 3 nodes and configure dmvpn between them.
Then on one of the nodes stop opennhrp. then run:

swanctl -i -c dmvpn -S 10.0.0.1 -R 10.0.0.2
swanctl -i -c dmvpn -S 10.0.0.1 -R 10.0.0.3
This will show as two created session in swanctl

swanctl -t -S 10.0.0.1 -R 10.0.0.2
Running this should delete the session between 10.0.0.1 and 10.0.0.2, but actually deletes both sessions to 10.0.0.2 and 10.0.0.3

Details

Difficulty level
Unknown (require assessment)
Version
vyos-1.2.0-rc9
Why the issue appeared?
Will be filled on close
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Bug (incorrect behavior)

Event Timeline

runar created this object in space S1 VyOS Public.
syncer triaged this task as Low priority.
syncer edited projects, added VyOS 1.3 Equuleus; removed VyOS-1.2.0-GA.

I've been trying to get a dev environment for vyos-strongswan up and running for a couple of days now but are unable to compile it.. right now i'm stuck with the compile system not finding my libsoup-2.4 package :/

Do we have a known working docker build env. For this package somewhere?

@syncer, this is a quite serious security issue and a deal breaker for dmvpn. As we have earlier stated that dmvpn is working now (http://blog.vyos.net/vyos-development-news-in-august-and-september) i think this needs to be fixed before 1.2LTS ... OR. We need to make a new statement that states that dmvpn will be broken in 1.2LTS..

As for fimding the fault, i've not been able to compile strongswan yet, and i think i need help to get a working dev.env. up and running..:/

What about Dockerfile in vyos-build?

@c-po i've updated the Dockerfile and added build notes in README.md to build the vyos-strongswan module in this PR: https://github.com/vyos/vyos-build/pull/31 . please test it out

The fault is found in the vyos-strongswan codeset,

The code that compares the src and dst adresses is in the original patch treated as strings, but is an object. this makes the string compare function always return a match. i've changed that out with the compare functions inside the object and it seems to work

diff:

diff --git a/src/libcharon/plugins/vici/vici_control.c b/src/libcharon/plugins/vici/vici_control.c
index 9539e2fe..d71953bb 100644
--- a/src/libcharon/plugins/vici/vici_control.c
+++ b/src/libcharon/plugins/vici/vici_control.c

@@ -362,19 +364,35 @@ CALLBACK(terminate, vici_message_t*,
                }
                else if (ike && streq(ike, ike_sa->get_name(ike_sa)))
                {
                        current = ike_sa->get_unique_id(ike_sa);
                        array_insert(ids, ARRAY_TAIL, &current);
                }
                else if (ike_id && ike_id == ike_sa->get_unique_id(ike_sa))
                {
                        array_insert(ids, ARRAY_TAIL, &ike_id);
                }
                else if (my_host && other_host)
                {
-                       if (my_host && !streq(my_host, ike_sa->get_my_host(ike_sa)) && other_host && !streq(other_host, ike_sa->get_other_host(ike_sa)))
+                       if (!my_host->ip_equals(my_host, ike_sa->get_my_host(ike_sa)) || !other_host->ip_equals(other_host, ike_sa->get_other_host(ike_sa)))
                        {
                                continue;
                        }
                        current = ike_sa->get_unique_id(ike_sa);
                        array_insert(ids, ARRAY_TAIL, &current);
                }

but for now the vyos-strongswan repo is upgraded to 5.7.2 and all patches are for now reverted by @UnicronNL so i'm unable to debug any further.

Until these patches are back in the repo dmvpn is broken.

c-po removed c-po as the assignee of this task.Mar 22 2019, 8:37 PM
c-po added a subscriber: c-po.

I an confirm as well this is happening in 1.2.2. Is there anyway to cronjob a restart of the process to re-establish connectivity to the hub as a workaround?

@UnicronNL could you apply my patch to the codebase?

In T1070#41443, @runar wrote:

@UnicronNL could you apply my patch to the codebase?

This would be awesome. DMVPN is 100% broken without it. The tunnels will stay up for a decent amount of time giving the appearance that everything is working. I tested 1d 18h hours before then tunnel from spoke to spoke went to tear down.

Once it is torn down it removes all tunnels and stays down until you modify the config or reboot.

Aug 21 10:30:22 vyos charon: 16[IKE] received DELETE for ESP CHILD_SA with SPI ffbeba19
Aug 21 10:30:22 vyos charon: 16[IKE] CHILD_SA not found, ignored
Aug 21 10:30:22 vyos charon: 08[NET] received packet: from x.x.x.x[4500] to 10.10.100.200[4500] (92 bytes)
Aug 21 10:30:22 vyos charon: 08[ENC] parsed INFORMATIONAL_V1 request 2933871230 [ HASH D ]
Aug 21 10:30:22 vyos charon: 08[IKE] received DELETE for IKE_SA dmvpn-DMVPN-tun0[42]
Aug 21 10:30:22 vyos charon: 08[IKE] deleting IKE_SA dmvpn-DMVPN-tun0[42] between 10.10.100.200[10.10.100.200]...x.x.x.x[x.x.x.x]

vyos@vyos:~$ show vpn ipsec sa
Connection State Up Bytes In/Out Remote address Remote ID Proposal


dmvpn-DMVPN-tun0 down N/A N/A N/A N/A N/A

@UnicronNL , no need to apply the patch, it is already applied to the codebase. this issue needs to be something else

syncer changed the task status from Open to Needs testing.Dec 31 2019, 4:01 AM
syncer assigned this task to c-po.
syncer raised the priority of this task from Low to Normal.
syncer edited projects, added VyOS 1.3 Equuleus; removed VyOS-1.2.0-GA.
syncer added subscribers: Viacheslav, Unknown Object (User).

@Dmitry @Viacheslav can you look into this

i will propose to move away from opennhrp to frr backed nhrp
http://docs.frrouting.org/en/latest/nhrpd.html

c-po removed c-po as the assignee of this task.Jun 14 2020, 3:36 PM
c-po set Is it a breaking change? to Unspecified (possibly destroys the router).
Unknown Object (User) added a subscriber: Unknown Object (User).Aug 20 2020, 6:04 PM
erkin set Issue type to Bug (incorrect behavior).Aug 31 2021, 7:13 PM
dmbaturin edited projects, added Restricted Project; removed test.Feb 15 2024, 11:24 AM