Page MenuHomePhabricator

SWANCTL: DMVPN: ALL peers are deleted in swan when opennhrp tries to delete ONE peer
Open, LowPublic

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
runar created this task.Nov 30 2018, 10:13 PM
runar created this object in space S1 VyOS Public.
syncer assigned this task to c-po.
syncer triaged this task as Low priority.
runar added a comment.Dec 2 2018, 7:49 AM

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?

runar added a subscriber: syncer.Dec 2 2018, 9:50 AM

@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..:/

c-po added a comment.Dec 2 2018, 6:52 PM

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.

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