bus: Prevent redundant down event on rekeyed CHILD_SA delete timeout

If a CHILD_SA is rekeyed using a CREATE_CHILD_SA request, a subsequent
DELETE for the old CHILD_SA may time out. Before sending this DELETE,
a CHILD_REKEYED state CHILD_SA set from child_rekey::process_i() is
immediately set to CHILD_DELETING from child_delete::build_i(). If the
IKE_SA dies due to a retransmission timeout of this DELETE, a redundant
child-down event is issued for the rekeyed CHILD_SA that has already seen a
child-rekey event.

A reproducer shows the following log and events:

    [CFG] vici rekey CHILD_SA #533
    [IKE] establishing CHILD_SA XXX{534} reqid 20
    [ENC] generating CREATE_CHILD_SA request 0 [ N(REKEY_SA) SA No KE TSi TSr ]
    [ENC] parsed CREATE_CHILD_SA response 0 [ SA No TSi TSr ]
    [IKE] rekeyed CHILD_SA XXX{533} with SPIs ca997de6_i cd27d4fe_o with XXX{534} with SPIs ced1cd01_i c460a7c9_o
     Event: child-rekey
      [OLD SA] state: REKEYING, spi-in: ca997de6
      [NEW SA] state: INSTALLED, spi-in: ced1cd01
    [IKE] closing CHILD_SA XXX{533} with SPIs ca997de6_i (352 bytes) cd27d4fe_o (264 bytes) and TS 0.0.0.0/0 === 10.11.9.40/29
    [IKE] sending DELETE for ESP CHILD_SA with SPI ca997de6
    [ENC] generating INFORMATIONAL request 1 [ D ]
    [IKE] retransmit 1 of request with message ID 1
    [IKE] retransmit 2 of request with message ID 1
    [IKE] retransmit 3 of request with message ID 1
    [IKE] retransmit 4 of request with message ID 1
    [IKE] giving up after 4 retransmits
     Event: child-updown
      [SA] state: DELETING, spi-in: ca997de6
     Event: child-updown
      [SA] state: INSTALLED, spi-in: ced1cd01

To prevent the redundant child-down event for the successfully rekeyed CHILD_SA,
check if a DELETING CHILD_SA has already removed its outbound state due to
having been rekeyed before issuing the child-down event.

Add a new exchange test exercising that a delete timeout after rekeying does
not cause a duplicate child-down event.

diff --git a/src/libcharon/bus/bus.c b/src/libcharon/bus/bus.c
index 99387d5e06f8077682a99741c20021b088fc8acc..4ab3991cae5a8c8163498b90e30b676b23cfad86 100644 (file)
--- a/src/libcharon/bus/bus.c
+++ b/src/libcharon/bus/bus.c
@@ -830,11 +830,23 @@ METHOD(bus_t, ike_updown, void,
                enumerator = ike_sa->create_child_sa_enumerator(ike_sa);
                while (enumerator->enumerate(enumerator, (void**)&child_sa))
                {
-                       if (child_sa->get_state(child_sa) != CHILD_REKEYED &&
-                               child_sa->get_state(child_sa) != CHILD_DELETED)
+                       switch (child_sa->get_state(child_sa))
                        {
-                               child_updown(this, child_sa, FALSE);
+                               case CHILD_REKEYED:
+                               case CHILD_DELETED:
+                                       continue;
+                               case CHILD_DELETING:
+                                       if (child_sa->get_outbound_state(child_sa) ==
+                                               CHILD_OUTBOUND_NONE)
+                                       {
+                                               /* deleting CHILD_SA has been rekeyed, omit event */
+                                               continue;
+                                       }
+                                       break;
+                               default:
+                                       break;
                        }
+                       child_updown(this, child_sa, FALSE);
                }
                enumerator->destroy(enumerator);
        }

diff --git a/src/libcharon/tests/suites/test_child_rekey.c b/src/libcharon/tests/suites/test_child_rekey.c
index b61f31c7cb179d8475ada76d2261821281bb3336..1c81e75e2bcdfc929c1b5979dc9e7b2a61e41009 100644 (file)
--- a/src/libcharon/tests/suites/test_child_rekey.c
+++ b/src/libcharon/tests/suites/test_child_rekey.c
@@ -1089,6 +1089,64 @@ START_TEST(test_regular_responder_incorrect_delete)
 }
 END_TEST
 
+/**
+ * Check that a delete timeout after rekeying does not cause a duplicate
+ * child-down event for the rekeyed SA.
+ */
+START_TEST(test_regular_delete_timeout)
+{
+       ike_sa_t *a, *b;
+       message_t *msg;
+       status_t s;
+
+       exchange_test_helper->establish_sa(exchange_test_helper, &a, &b, NULL);
+       initiate_rekey(a, 1);
+       assert_ipsec_sas_installed(a, 1, 2);
+
+       /* this should never get called as this results in a successful rekeying */
+       assert_hook_not_called(child_updown);
+
+       /* CREATE_CHILD_SA { N(REKEY_SA), SA, Ni, [KEi,] TSi, TSr } --> */
+       assert_hook_not_called(child_rekey);
+       assert_notify(IN, REKEY_SA);
+       exchange_test_helper->process_message(exchange_test_helper, b, NULL);
+       assert_child_sa_state(b, 2, CHILD_REKEYED, CHILD_OUTBOUND_INSTALLED);
+       assert_child_sa_state(b, 4, CHILD_INSTALLED, CHILD_OUTBOUND_REGISTERED);
+       assert_ipsec_sas_installed(b, 1, 2, 4);
+       assert_hook();
+
+       /* <-- CREATE_CHILD_SA { SA, Nr, [KEr,] TSi, TSr } */
+       assert_hook_rekey(child_rekey, 1, 3);
+       assert_no_notify(IN, REKEY_SA);
+       exchange_test_helper->process_message(exchange_test_helper, a, NULL);
+       assert_child_sa_state(a, 3, CHILD_INSTALLED, CHILD_OUTBOUND_INSTALLED);
+       assert_ipsec_sas_installed(a, 1, 3, 4);
+       assert_hook();
+       assert_hook();
+
+       /* trigger retransmits until the request times out */
+       assert_hook_updown(child_updown, FALSE);
+       msg = exchange_test_helper->sender->dequeue(exchange_test_helper->sender);
+       while (msg)
+       {
+               charon->bus->set_sa(charon->bus, a);
+               s = a->retransmit(a, msg->get_message_id(msg));
+               charon->bus->set_sa(charon->bus, NULL);
+               msg->destroy(msg);
+               if (s == DESTROY_ME)
+               {
+                       break;
+               }
+               msg = exchange_test_helper->sender->dequeue(
+                                                                                               exchange_test_helper->sender);
+       }
+       assert_hook();
+
+       call_ikesa(a, destroy);
+       call_ikesa(b, destroy);
+}
+END_TEST
+
 /**
  * Both peers initiate the CHILD_SA rekeying concurrently and should handle
  * the collision properly depending on the nonces.
@@ -4368,6 +4426,7 @@ Suite *child_rekey_suite_create()
        tcase_add_test(tc, test_regular_responder_delete);
        tcase_add_test(tc, test_regular_responder_lost_sa);
        tcase_add_test(tc, test_regular_responder_incorrect_delete);
+       tcase_add_test(tc, test_regular_delete_timeout);
        suite_add_tcase(s, tc);
 
        tc = tcase_create("collisions rekey");