[strongSwan-dev] [PATCH 1/2] Enforce IKE_SA uniqueness policy as initiator too

[Jan Blunck]

In cases that both peers of tunnel try to initiate the connection
simultaneously one of the peers ends up with having two IKE_SA in state
IKE_ESTABLISHED. Therefore lets call check_uniqueness() before we switch
state like it is already the case as a responder in build_r.
---
src/libcharon/sa/ikev2/tasks/ike_auth.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/src/libcharon/sa/ikev2/tasks/ike_auth.c b/src/libcharon/sa/ikev2/tasks/ike_auth.c
index e554ca3..a7ec37c 100644
--- a/src/libcharon/sa/ikev2/tasks/ike_auth.c
+++ b/src/libcharon/sa/ikev2/tasks/ike_auth.c
@@ -1140,6 +1140,14 @@ METHOD(task_t, process_i, status_t,
{
goto peer_auth_failed;
}
+ if (charon->ike_sa_manager->check_uniqueness(charon->ike_sa_manager,
+ this->ike_sa, FALSE))
+ {
+ DBG1(DBG_IKE, "cancelling IKE_SA setup due to uniqueness policy");
+ charon->bus->alert(charon->bus, ALERT_UNIQUE_KEEP);
+ send_auth_failed_informational(this, message);
+ return FAILED;
+ }
if (!charon->bus->authorize(charon->bus, TRUE))
{
DBG1(DBG_IKE, "final authorization hook forbids IKE_SA, "

[Jan Blunck]

This prevents the run of the updown scripts when the delete is executed.
---
src/libcharon/sa/ike_sa_manager.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/src/libcharon/sa/ike_sa_manager.c b/src/libcharon/sa/ike_sa_manager.c
index 9b9ad93..3cc0c45 100644
--- a/src/libcharon/sa/ike_sa_manager.c
+++ b/src/libcharon/sa/ike_sa_manager.c
@@ -1900,6 +1900,12 @@ static status_t enforce_replace(private_ike_sa_manager_t *this,
* explicitly. */
adopt_children_and_vips(duplicate, new);
}
+ DBG1(DBG_IKE, "deleting reauthenticated IKE_SA for peer '%Y' due to "
+ "uniqueness policy", other);
+
+ /* set rekeying state so we don't run updown */
+ duplicate->set_state(duplicate, IKE_REKEYING);
+
/* For IKEv1 we have to delay the delete for the old IKE_SA. Some
* peers need to complete the new SA first, otherwise the quick modes
* might get lost. For IKEv2 we do the same, as we want overlapping

[Tobias Brunner]

Post by Jan Blunck
This prevents the run of the updown scripts when the delete is executed.

I don't think this will work correctly. The updown script will run for
the newly established CHILD_SAs, but then not for the deleted ones. So
if the script does e.g. add firewall rules for every established SA
these won't all get removed if e.g. make-before-break reauthentication
is used. You might better implement some kind of refcounting in your
script so that it works with overlapping, duplicate CHILD_SAs.

Regards,
Tobias

[Tobias Brunner]

Hi Jan,

Post by Jan Blunck
In cases that both peers of tunnel try to initiate the connection
simultaneously one of the peers ends up with having two IKE_SA in state
IKE_ESTABLISHED. Therefore lets call check_uniqueness() before we switch
state like it is already the case as a responder in build_r.

Wouldn't this result in both peers aborting the IKE_SA they each initiated?

Peer A Peer B
IKE_SA_INIT (A) <--------------> IKE_SA_INIT (B)
IKE_SA_INIT (B) <--------------> IKE_SA_INIT (A)
IKE_AUTH (A) -----\ /----- IKE_AUTH (B)
SA B ESTABLISHED <-----\--/
IKE_AUTH (B) ----\ \-------> SA A ESTABLISHED
\ /---- IKE_AUTH (A)
\---/----> Finds duplicate A,
Finds duplicate B <--------/ aborts B
aborts A

Mutual duplicate checking would require something similar to what's done
to resolve rekey collisions (e.g. comparing nonces). But since that's
not standardized this would require some kind of private extension (it
only works if both peers behave the same otherwise you could still end
up without any SAs). And with current versions duplicate SAs aren't
really an issue anymore.

Regards,
Tobias