From 0cd46df37761b24f8d172e6af7c1884b5a7f4b89 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 10 Jan 2024 16:38:54 +0100 Subject: [PATCH 1/6] atomics: Add a ref_get() variant returning non-zero on overflows This is useful for users using ref_get() for unique identifier allocation, but the zero value has special meaning. --- src/libstrongswan/tests/suites/test_utils.c | 24 ++++++++++++++++++ src/libstrongswan/utils/utils/atomics.h | 27 +++++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/src/libstrongswan/tests/suites/test_utils.c b/src/libstrongswan/tests/suites/test_utils.c index e4463f2d0..4662fa886 100644 --- a/src/libstrongswan/tests/suites/test_utils.c +++ b/src/libstrongswan/tests/suites/test_utils.c @@ -256,6 +256,26 @@ START_TEST(test_round) } END_TEST +/******************************************************************************* + * ref_get/put + */ + +START_TEST(test_refs) +{ + refcount_t r = 0xfffffffe; + + ck_assert_int_eq(ref_cur(&r), 0xfffffffe); + ck_assert_int_eq(ref_get(&r), 0xffffffff); + ck_assert_int_eq(ref_get_nonzero(&r), 1); + ck_assert_int_eq(ref_get_nonzero(&r), 2); + ck_assert_int_eq(ref_cur(&r), 2); + ck_assert(!ref_put(&r)); + ck_assert_int_eq(ref_cur(&r), 1); + ck_assert(ref_put(&r)); + ck_assert_int_eq(ref_cur(&r), 0); +} +END_TEST + /******************************************************************************* * streq */ @@ -1272,6 +1292,10 @@ Suite *utils_suite_create() tcase_add_test(tc, test_round); suite_add_tcase(s, tc); + tc = tcase_create("refcount"); + tcase_add_test(tc, test_refs); + suite_add_tcase(s, tc); + tc = tcase_create("string helper"); tcase_add_loop_test(tc, test_streq, 0, countof(streq_data)); tcase_add_loop_test(tc, test_strneq, 0, countof(strneq_data)); diff --git a/src/libstrongswan/utils/utils/atomics.h b/src/libstrongswan/utils/utils/atomics.h index 0fcd99e18..58f1cd92e 100644 --- a/src/libstrongswan/utils/utils/atomics.h +++ b/src/libstrongswan/utils/utils/atomics.h @@ -124,6 +124,33 @@ bool cas_ptr(void **ptr, void *oldval, void *newval); #endif /* HAVE_GCC_ATOMIC_OPERATIONS */ +/** + * Get a new reference, but skip zero on overflow. + * + * If a reference counter is used to allocate unique identifiers, the + * refcount value may overflow if it is never decremented. The 0 identifier + * may have special semantics, hence returning can be problematic for some + * users. + * + * This call does an additional ref_get() if ref_get() overflows and returns + * zero. This ensures that zero is never returned, in the assumption that it + * has special meaning. + * + * @param ref pointer to ref counter + * @return new value of ref + */ +static inline refcount_t ref_get_nonzero(refcount_t *ref) +{ + refcount_t v; + + v = ref_get(ref); + if (v == 0) + { + v = ref_get(ref); + } + return v; +} + /** * Initialize atomics utility functions */ From cdf865e0b806d5311208d78c32361c912b1e0534 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 10 Jan 2024 16:54:17 +0100 Subject: [PATCH 2/6] kernel-netlink: Handle Netlink sequence number counter overflows gracefully A refcount variable is used to allocate sequential unique identifiers for Netlink sequence numbers, subject to overflows. The risk of an overflow has so far not been considered practical, as it requires 2^32 netlink requests. It seems that this issue is not only theoretical. A host with thousands of tunnels doing aggressive rekeying and/or aggressive status checking (via vici list-sas) may trigger the overflow after a few weeks uptime. The consequences are rather devastating: Once the refcount overflows, a Netlink request is sent with sequence number 0. This request is answered by the kernel, but can't be matched to the request, resulting in the error: "received unknown netlink seq 0, ignored". Without Netlink timeouts, the thread indefinitely waits for a response while holding the Netlink mutex, bringing all threads to a halt. So at all costs avoid zero sequence numbers. Also, start at sequence number 1 instead of the arbitrary 201, so the same range is used on start and after an overflow. --- src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c b/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c index 72c13a697..339d0b9e6 100644 --- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c +++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_shared.c @@ -514,7 +514,7 @@ METHOD(netlink_socket_t, netlink_send, status_t, uintptr_t seq; u_int try; - seq = ref_get(&this->seq); + seq = ref_get_nonzero(&this->seq); for (try = 0; try <= this->retries; ++try) { @@ -694,7 +694,6 @@ netlink_socket_t *netlink_socket_create(int protocol, enum_name_t *names, .send_ack = _netlink_send_ack, .destroy = _destroy, }, - .seq = 200, .mutex = mutex_create(MUTEX_TYPE_RECURSIVE), .socket = socket(AF_NETLINK, SOCK_RAW, protocol), .entries = hashtable_create(hashtable_hash_ptr, hashtable_equals_ptr, 4), From f634a3300c9611b5ce4be945d6a606f5a0a1c5bb Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 10 Jan 2024 17:27:00 +0100 Subject: [PATCH 3/6] ike-sa: Handle IKE_SA unique identifier refcount overflow gracefully IKE_SA unique identifier allocation starts at 1. If the counter overflows, a unique ID of 0 is assigned to an IKE_SA, which may have unclear consequences. Overflowing the unique ID counter is theoretical for most setups, but on a Gateway terminating 100'000 tunnels and rekeying the IKE_SA every 60s overflows the counter after a month uptime. So avoid a 0 unique identifier by using ref_get_nonzero(). --- src/libcharon/sa/ike_sa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcharon/sa/ike_sa.c b/src/libcharon/sa/ike_sa.c index 654508e4b..758a4cdba 100644 --- a/src/libcharon/sa/ike_sa.c +++ b/src/libcharon/sa/ike_sa.c @@ -3229,7 +3229,7 @@ ike_sa_t * ike_sa_create(ike_sa_id_t *ike_sa_id, bool initiator, .my_auths = array_create(0, 0), .other_auths = array_create(0, 0), .attributes = array_create(sizeof(attribute_entry_t), 0), - .unique_id = ref_get(&unique_id), + .unique_id = ref_get_nonzero(&unique_id), .keepalive_interval = lib->settings->get_time(lib->settings, "%s.keep_alive", KEEPALIVE_INTERVAL, lib->ns), .keepalive_dpd_margin = lib->settings->get_time(lib->settings, From 4aac88fadd7053ae15a4ff3e9039d979d8141211 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 10 Jan 2024 17:31:49 +0100 Subject: [PATCH 4/6] child-sa: Handle CHILD_SA unique identifier refcount overflow gracefully CHILD_SA unique identifier allocation starts at 1. If the counter overflows, a unique ID of 0 is assigned to an CHILD_SA, which may have unclear consequences. Overflowing the unique ID counter is theoretical for most setups, but on a Gateway terminating 100'000 tunnels and rekeying CHILD_SAs every 60s overflows the counter after a month uptime. So avoid a 0 unique identifier by using ref_get_nonzero(). --- src/libcharon/sa/child_sa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c index 494c36b2e..aeb46ed43 100644 --- a/src/libcharon/sa/child_sa.c +++ b/src/libcharon/sa/child_sa.c @@ -2096,7 +2096,7 @@ child_sa_t *child_sa_create(host_t *me, host_t *other, child_cfg_t *config, .close_action = config->get_close_action(config), .dpd_action = config->get_dpd_action(config), .reqid = config->get_reqid(config), - .unique_id = ref_get(&unique_id), + .unique_id = ref_get_nonzero(&unique_id), .mark_in = config->get_mark(config, TRUE), .mark_out = config->get_mark(config, FALSE), .if_id_in = config->get_if_id(config, TRUE) ?: data->if_id_in_def, From dde40bcb9eedcb8b3d42c49aa29f98833ba3f720 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Fri, 16 Feb 2024 10:42:43 +0100 Subject: [PATCH 5/6] child-sa: Move unique mark allocation to a separate helper function This aligns the code with unique interface ID allocation, which uses a helper function for the same purpose and mechanic as well. --- src/libcharon/sa/child_sa.c | 24 ++----------- src/libstrongswan/ipsec/ipsec_types.c | 28 +++++++++++++++ src/libstrongswan/ipsec/ipsec_types.h | 12 +++++++ src/libstrongswan/tests/suites/test_utils.c | 39 +++++++++++++++++++++ 4 files changed, 81 insertions(+), 22 deletions(-) diff --git a/src/libcharon/sa/child_sa.c b/src/libcharon/sa/child_sa.c index aeb46ed43..97ee88acb 100644 --- a/src/libcharon/sa/child_sa.c +++ b/src/libcharon/sa/child_sa.c @@ -2038,7 +2038,7 @@ child_sa_t *child_sa_create(host_t *me, host_t *other, child_cfg_t *config, child_sa_create_t *data) { private_child_sa_t *this; - static refcount_t unique_id = 0, unique_mark = 0; + static refcount_t unique_id = 0; INIT(this, .public = { @@ -2127,27 +2127,7 @@ child_sa_t *child_sa_create(host_t *me, host_t *other, child_cfg_t *config, } allocate_unique_if_ids(&this->if_id_in, &this->if_id_out); - - if (MARK_IS_UNIQUE(this->mark_in.value) || - MARK_IS_UNIQUE(this->mark_out.value)) - { - refcount_t mark = 0; - bool unique_dir = this->mark_in.value == MARK_UNIQUE_DIR || - this->mark_out.value == MARK_UNIQUE_DIR; - - if (!unique_dir) - { - mark = ref_get(&unique_mark); - } - if (MARK_IS_UNIQUE(this->mark_in.value)) - { - this->mark_in.value = unique_dir ? ref_get(&unique_mark) : mark; - } - if (MARK_IS_UNIQUE(this->mark_out.value)) - { - this->mark_out.value = unique_dir ? ref_get(&unique_mark) : mark; - } - } + allocate_unique_marks(&this->mark_in.value, &this->mark_out.value); if (!this->reqid) { diff --git a/src/libstrongswan/ipsec/ipsec_types.c b/src/libstrongswan/ipsec/ipsec_types.c index bd8a4a52c..6f10adf70 100644 --- a/src/libstrongswan/ipsec/ipsec_types.c +++ b/src/libstrongswan/ipsec/ipsec_types.c @@ -150,6 +150,34 @@ bool mark_from_string(const char *value, mark_op_t ops, mark_t *mark) return TRUE; } +/* + * Described in header + */ +void allocate_unique_marks(uint32_t *in, uint32_t *out) +{ + static refcount_t unique_mark = 0; + + if (MARK_IS_UNIQUE(*in) || MARK_IS_UNIQUE(*out)) + { + refcount_t mark = 0; + bool unique_dir = *in == MARK_UNIQUE_DIR || + *out == MARK_UNIQUE_DIR; + + if (!unique_dir) + { + mark = ref_get(&unique_mark); + } + if (MARK_IS_UNIQUE(*in)) + { + *in = unique_dir ? ref_get(&unique_mark) : mark; + } + if (MARK_IS_UNIQUE(*out)) + { + *out = unique_dir ? ref_get(&unique_mark) : mark; + } + } +} + /* * Described in header */ diff --git a/src/libstrongswan/ipsec/ipsec_types.h b/src/libstrongswan/ipsec/ipsec_types.h index 5320c46ab..b3a26ce8e 100644 --- a/src/libstrongswan/ipsec/ipsec_types.h +++ b/src/libstrongswan/ipsec/ipsec_types.h @@ -242,6 +242,18 @@ enum mark_op_t { */ bool mark_from_string(const char *value, mark_op_t ops, mark_t *mark); +/** + * Allocate up to two unique marks depending on the given values. + * + * If the given values are MARK_UNIQUE, the values get replaced by a single + * unique mark. If the given values are MARK_UNIQUE_DIR, the values get + * replaced by a single unique mark for each direction. + * + * @param[out] in inbound interface ID + * @param[out] out outbound interface ID + */ +void allocate_unique_marks(uint32_t *in, uint32_t *out); + /** * Special interface ID values to allocate a unique ID for each CHILD_SA/dir */ diff --git a/src/libstrongswan/tests/suites/test_utils.c b/src/libstrongswan/tests/suites/test_utils.c index 4662fa886..bbaca6deb 100644 --- a/src/libstrongswan/tests/suites/test_utils.c +++ b/src/libstrongswan/tests/suites/test_utils.c @@ -1124,6 +1124,41 @@ START_TEST(test_mark_from_string) } END_TEST +/******************************************************************************* + * allocate_unique_marks + */ + +static struct { + uint32_t in; + uint32_t out; + uint32_t exp_in; + uint32_t exp_out; +} unique_mark_data[] = { + {0, 0, 0, 0 }, + {42, 42, 42, 42 }, + {42, 1337, 42, 1337 }, + /* each call increases the internal counter by 1 or 2*/ + {MARK_UNIQUE, 42, 1, 42 }, + {42, MARK_UNIQUE, 42, 2 }, + {MARK_UNIQUE_DIR, 42, 3, 42 }, + {42, MARK_UNIQUE_DIR, 42, 4 }, + {MARK_UNIQUE, MARK_UNIQUE, 5, 5 }, + {MARK_UNIQUE_DIR, MARK_UNIQUE, 6, 7 }, + {MARK_UNIQUE, MARK_UNIQUE_DIR, 8, 9 }, + {MARK_UNIQUE_DIR, MARK_UNIQUE_DIR, 10, 11 }, +}; + +START_TEST(test_allocate_unique_marks) +{ + uint32_t mark_in = unique_mark_data[_i].in, + mark_out = unique_mark_data[_i].out; + + allocate_unique_marks(&mark_in, &mark_out); + ck_assert_int_eq(mark_in, unique_mark_data[_i].exp_in); + ck_assert_int_eq(mark_out, unique_mark_data[_i].exp_out); +} +END_TEST + /******************************************************************************* * if_id_from_string */ @@ -1364,6 +1399,10 @@ Suite *utils_suite_create() tcase_add_loop_test(tc, test_mark_from_string, 0, countof(mark_data)); suite_add_tcase(s, tc); + tc = tcase_create("allocate_unique_marks"); + tcase_add_loop_test(tc, test_allocate_unique_marks, 0, countof(unique_mark_data)); + suite_add_tcase(s, tc); + tc = tcase_create("if_id_from_string"); tcase_add_loop_test(tc, test_if_id_from_string, 0, countof(if_id_data)); suite_add_tcase(s, tc); From 1a740bf3f3e20c8a7ff88ca9dcf043fb051670a7 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Fri, 16 Feb 2024 10:59:11 +0100 Subject: [PATCH 6/6] child-sa: Handle refcount overflow for unique mark/if_id allocation gracefully The refcount_t for allocating unique marks and interface IDs may overflow or hit the special value for unique marks/if_ids, in the worst case not setting it on CHILD_SAs that should have one. As (potentially two) marks/if_ids are allocated only for newly created CHILD_SAs, but not for rekeying, this not very likely. Still, if a setup uses aggressive re-authentication and or re-creates CHILD_SAs every minute, a gateway with 100'000 tunnels may hit the overflow within a month uptime. --- src/libstrongswan/ipsec/ipsec_types.c | 52 +++++++++++++++++++++------ 1 file changed, 41 insertions(+), 11 deletions(-) diff --git a/src/libstrongswan/ipsec/ipsec_types.c b/src/libstrongswan/ipsec/ipsec_types.c index 6f10adf70..7d93f275a 100644 --- a/src/libstrongswan/ipsec/ipsec_types.c +++ b/src/libstrongswan/ipsec/ipsec_types.c @@ -150,30 +150,44 @@ bool mark_from_string(const char *value, mark_op_t ops, mark_t *mark) return TRUE; } +/** + * Allocate a unique mark that is non-zero and not one of the special values. + */ +static uint32_t allocate_unique_mark() +{ + static refcount_t unique_mark = 0; + uint32_t m; + + m = ref_get_nonzero(&unique_mark); + while (MARK_IS_UNIQUE(m)) + { + m = ref_get_nonzero(&unique_mark); + } + return m; +} + /* * Described in header */ void allocate_unique_marks(uint32_t *in, uint32_t *out) { - static refcount_t unique_mark = 0; - if (MARK_IS_UNIQUE(*in) || MARK_IS_UNIQUE(*out)) { - refcount_t mark = 0; + uint32_t mark = 0; bool unique_dir = *in == MARK_UNIQUE_DIR || *out == MARK_UNIQUE_DIR; if (!unique_dir) { - mark = ref_get(&unique_mark); + mark = allocate_unique_mark(); } if (MARK_IS_UNIQUE(*in)) { - *in = unique_dir ? ref_get(&unique_mark) : mark; + *in = unique_dir ? allocate_unique_mark() : mark; } if (MARK_IS_UNIQUE(*out)) { - *out = unique_dir ? ref_get(&unique_mark) : mark; + *out = unique_dir ? allocate_unique_mark() : mark; } } } @@ -219,30 +233,46 @@ bool if_id_from_string(const char *value, uint32_t *if_id) return TRUE; } +/** + * Allocate a unique interface ID that is non-zero and not one of the special + * values. + */ +static uint32_t allocate_unique_if_id() +{ + static refcount_t unique_if_id = 0; + uint32_t if_id; + + if_id = ref_get_nonzero(&unique_if_id); + while (IF_ID_IS_UNIQUE(if_id)) + { + if_id = ref_get_nonzero(&unique_if_id); + } + return if_id; +} + /* * Described in header */ void allocate_unique_if_ids(uint32_t *in, uint32_t *out) { - static refcount_t unique_if_id = 0; if (IF_ID_IS_UNIQUE(*in) || IF_ID_IS_UNIQUE(*out)) { - refcount_t if_id = 0; + uint32_t if_id = 0; bool unique_dir = *in == IF_ID_UNIQUE_DIR || *out == IF_ID_UNIQUE_DIR; if (!unique_dir) { - if_id = ref_get(&unique_if_id); + if_id = allocate_unique_if_id(); } if (IF_ID_IS_UNIQUE(*in)) { - *in = unique_dir ? ref_get(&unique_if_id) : if_id; + *in = unique_dir ? allocate_unique_if_id() : if_id; } if (IF_ID_IS_UNIQUE(*out)) { - *out = unique_dir ? ref_get(&unique_if_id) : if_id; + *out = unique_dir ? allocate_unique_if_id() : if_id; } } }