From 1866d335387eb4657bb323e3b4b2c74f92d65be8 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 15 Apr 2015 14:20:52 +0200 Subject: [PATCH 01/18] utils: Support __has_feature() macro on non-LLVM compilers by returning 0 --- src/libstrongswan/utils/utils.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/libstrongswan/utils/utils.h b/src/libstrongswan/utils/utils.h index c7ccd3afa..120b15950 100644 --- a/src/libstrongswan/utils/utils.h +++ b/src/libstrongswan/utils/utils.h @@ -112,6 +112,13 @@ void utils_deinit(); #define BUILD_ASSERT_ARRAY(a) \ BUILD_ASSERT(!__builtin_types_compatible_p(typeof(a), typeof(&(a)[0]))) +/** + * LLVM/Clang __has_feature support + */ +#ifndef __has_feature +# define __has_feature(x) 0 +#endif + /** * Debug macro to follow control flow */ From d840df185a0cdaae1aa1bd0e2f13a8af63edcf2e Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 15 Apr 2015 14:21:38 +0200 Subject: [PATCH 02/18] utils: Define ADDRESS_SANITIZER_EXCLUDE to exclude a function from sanitizer --- src/libstrongswan/utils/utils.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/libstrongswan/utils/utils.h b/src/libstrongswan/utils/utils.h index 120b15950..083f55344 100644 --- a/src/libstrongswan/utils/utils.h +++ b/src/libstrongswan/utils/utils.h @@ -119,6 +119,16 @@ void utils_deinit(); # define __has_feature(x) 0 #endif +/** + * Address santizer support + */ +#if __has_feature(address_sanitizer) || \ + (defined(__GNUC__) && defined(__SANITIZE_ADDRESS__)) +# define ADDRESS_SANITIZER_EXCLUDE __attribute__((no_sanitize_address)) +#else +# define ADDRESS_SANITIZER_EXCLUDE +#endif + /** * Debug macro to follow control flow */ From 7dce58135e1a8b8bde224c7da2fdcce7e4c83c17 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 8 Apr 2015 10:16:13 +0200 Subject: [PATCH 03/18] unit-tests: Exclude memory checks after-free from AddressSanitizer We explicitly test the memory we free()d if that got properly wiped, so suppress the warning from AddressSanitizer. --- src/libstrongswan/tests/suites/test_chunk.c | 53 ++++++++++++++++----- 1 file changed, 40 insertions(+), 13 deletions(-) diff --git a/src/libstrongswan/tests/suites/test_chunk.c b/src/libstrongswan/tests/suites/test_chunk.c index 3f7238152..93753c739 100644 --- a/src/libstrongswan/tests/suites/test_chunk.c +++ b/src/libstrongswan/tests/suites/test_chunk.c @@ -125,12 +125,27 @@ END_TEST * clear */ +ADDRESS_SANITIZER_EXCLUDE +static bool cleared(u_char *ptr) +{ + int i; + + for (i = 0; i < 64; i += 2) + { + if (ptr[i] != 0 && ptr[i] == i && + ptr[i+1] != 0 && ptr[i+1] == i+1) + { + return FALSE; + } + } + return TRUE; +} + START_TEST(test_chunk_clear) { chunk_t chunk; u_char *ptr; int i; - bool cleared = TRUE; chunk = chunk_empty; chunk_clear(&chunk); @@ -147,17 +162,8 @@ START_TEST(test_chunk_clear) * test directly, as it might allocate data at the freed area. comparing * two bytes at once reduces the chances of conflicts if memory got * overwritten already */ - for (i = 0; i < 64; i += 2) - { - if (ptr[i] != 0 && ptr[i] == i && - ptr[i+1] != 0 && ptr[i+1] == i+1) - { - cleared = FALSE; - break; - } - } + ck_assert(cleared(ptr)); assert_chunk_empty(chunk); - ck_assert(cleared); } END_TEST @@ -199,6 +205,27 @@ END_TEST * chunk_create_cat */ +ADDRESS_SANITIZER_EXCLUDE +bool chunk_equals_nosan(chunk_t a, chunk_t b) +{ + int i; + + /* cant use memcmp() or any function using it, as that is again + * sanitize-checked */ + if (a.len != b.len) + { + return FALSE; + } + for (i = 0; i < b.len; i++) + { + if (a.ptr[i] != b.ptr[i]) + { + return FALSE; + } + } + return TRUE; +} + START_TEST(test_chunk_create_cat) { chunk_t foo, bar; @@ -236,8 +263,8 @@ START_TEST(test_chunk_create_cat) ck_assert_int_eq(c.len, 6); ck_assert(chunk_equals(c, chunk_from_str("foobar"))); /* check memory area of cleared chunk */ - ck_assert(!chunk_equals(foo, chunk_create(ptra, 3))); - ck_assert(!chunk_equals(bar, chunk_create(ptrb, 3))); + ck_assert(!chunk_equals_nosan(foo, chunk_create(ptra, 3))); + ck_assert(!chunk_equals_nosan(bar, chunk_create(ptrb, 3))); } END_TEST From 432a846e669a44f041860875ede5f0d144e176e5 Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 8 Apr 2015 10:18:31 +0200 Subject: [PATCH 04/18] unit-tests: Disable AddressSanitizer for threading cleanup function As the cleanup function reads from the correct address on the parent frame, it is currently unclear why AddressSanitizer complains about that pointer dereference. --- src/libstrongswan/tests/suites/test_threading.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/libstrongswan/tests/suites/test_threading.c b/src/libstrongswan/tests/suites/test_threading.c index 90ebca62e..cd0101a51 100644 --- a/src/libstrongswan/tests/suites/test_threading.c +++ b/src/libstrongswan/tests/suites/test_threading.c @@ -1214,6 +1214,8 @@ START_TEST(test_cancel_point) } END_TEST +/* not sure why AddressSanitizer complains here, pointer looks fine */ +ADDRESS_SANITIZER_EXCLUDE static void close_fd_ptr(void *fd) { close(*(int*)fd); @@ -1253,6 +1255,8 @@ static void cancellation_read() } } +/* the AddressSaniziter complains about the fd_set here for some reason */ +ADDRESS_SANITIZER_EXCLUDE static void cancellation_select() { int sv[2]; From b18fbde41ebf39b3e45588f4a9c05b6da320b74f Mon Sep 17 00:00:00 2001 From: Martin Willi Date: Wed, 8 Apr 2015 10:20:23 +0200 Subject: [PATCH 05/18] test-runner: Properly clean up allocated test suites --- src/libstrongswan/tests/test_runner.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstrongswan/tests/test_runner.c b/src/libstrongswan/tests/test_runner.c index af1faf473..b598f7977 100644 --- a/src/libstrongswan/tests/test_runner.c +++ b/src/libstrongswan/tests/test_runner.c @@ -50,6 +50,7 @@ static void destroy_case(test_case_t *tcase) { array_destroy(tcase->functions); array_destroy(tcase->fixtures); + free(tcase); } /** From 5eeeb894d1bcfac590d6cd9b049345d09f24398a Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 13 Sep 2022 14:55:30 +0200 Subject: [PATCH 06/18] test-runner: Clean up collected run times --- src/libstrongswan/tests/test_runner.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libstrongswan/tests/test_runner.c b/src/libstrongswan/tests/test_runner.c index b598f7977..cfa3aabfe 100644 --- a/src/libstrongswan/tests/test_runner.c +++ b/src/libstrongswan/tests/test_runner.c @@ -739,6 +739,7 @@ static bool run_case(test_case_t *tcase, test_runner_init_t init, char *cfg, array_destroy(failures); array_destroy(warnings); DESTROY_IF(iterations); + free(times); return passed == array_count(tcase->functions); } From f2456376aeb1458c21de5b7f847b1567475f9cfb Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 13 Sep 2022 15:03:24 +0200 Subject: [PATCH 07/18] ike-rekey: Correctly destroy colliding rekey task Using DESTROY_IF() this way doesn't correctly check if the pointer is set. Fixes: 7ffeed01c0d7 ("ike-rekey: Remove collision task type checks") --- src/libcharon/sa/ikev2/tasks/ike_rekey.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libcharon/sa/ikev2/tasks/ike_rekey.c b/src/libcharon/sa/ikev2/tasks/ike_rekey.c index 85c47efc0..fac008715 100644 --- a/src/libcharon/sa/ikev2/tasks/ike_rekey.c +++ b/src/libcharon/sa/ikev2/tasks/ike_rekey.c @@ -449,7 +449,10 @@ METHOD(ike_rekey_t, collide, bool, "ignore"); break; } - DESTROY_IF(&this->collision->public.task); + if (this->collision) + { + this->collision->public.task.destroy(&this->collision->public.task); + } this->collision = rekey; return TRUE; } @@ -478,7 +481,10 @@ static void cleanup(private_ike_rekey_t *this) cur_sa = charon->bus->get_sa(charon->bus); DESTROY_IF(this->new_sa); charon->bus->set_sa(charon->bus, cur_sa); - DESTROY_IF(&this->collision->public.task); + if (this->collision) + { + this->collision->public.task.destroy(&this->collision->public.task); + } } METHOD(task_t, migrate, void, From 7217ff5fc567800ec3f90b4244b66d7949856ee8 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 13 Sep 2022 15:16:47 +0200 Subject: [PATCH 08/18] aesni: Fix out-of-bound read when loading 192-bit AES keys --- src/libstrongswan/plugins/aesni/aesni_key.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/libstrongswan/plugins/aesni/aesni_key.c b/src/libstrongswan/plugins/aesni/aesni_key.c index a9476124a..c05523590 100644 --- a/src/libstrongswan/plugins/aesni/aesni_key.c +++ b/src/libstrongswan/plugins/aesni/aesni_key.c @@ -142,9 +142,11 @@ static __m128i _mm_shuffle_i01(__m128i a, __m128i b) static void expand192(__m128i *key, __m128i *schedule) { __m128i t1, t2, t3; + u_char buf[16] = {}; schedule[0] = t1 = _mm_loadu_si128(key); - t2 = t3 = _mm_loadu_si128(key + 1); + memcpy(buf, key + 1, 8); + t2 = t3 = _mm_loadu_si128((__m128i*)buf); t2 = assist192(_mm_aeskeygenassist_si128(t2, 0x1), t2, &t1); schedule[1] = _mm_shuffle_i00(t3, t1); From 80b2c6cdc5d56f33c02c83f506644a02d482929c Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 13 Sep 2022 15:26:47 +0200 Subject: [PATCH 09/18] gcrypt: Return correct IV length (0) for ECB mode --- .../plugins/gcrypt/gcrypt_crypter.c | 27 ++++++++++++------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/src/libstrongswan/plugins/gcrypt/gcrypt_crypter.c b/src/libstrongswan/plugins/gcrypt/gcrypt_crypter.c index 81f615cde..8933c6978 100644 --- a/src/libstrongswan/plugins/gcrypt/gcrypt_crypter.c +++ b/src/libstrongswan/plugins/gcrypt/gcrypt_crypter.c @@ -45,7 +45,7 @@ struct private_gcrypt_crypter_t { /** * are we using counter mode? */ - bool ctr_mode; + int mode; /** * counter state @@ -62,13 +62,17 @@ struct private_gcrypt_crypter_t { */ static bool set_iv(private_gcrypt_crypter_t *this, chunk_t iv) { - if (this->ctr_mode) + if (this->mode == GCRY_CIPHER_MODE_CTR) { memcpy(this->ctr.iv, iv.ptr, sizeof(this->ctr.iv)); this->ctr.counter = htonl(1); return gcry_cipher_setctr(this->h, &this->ctr, sizeof(this->ctr)) == 0; } - return gcry_cipher_setiv(this->h, iv.ptr, iv.len) == 0; + if (iv.len) + { + return gcry_cipher_setiv(this->h, iv.ptr, iv.len) == 0; + } + return TRUE; } METHOD(crypter_t, decrypt, bool, @@ -108,7 +112,7 @@ METHOD(crypter_t, get_block_size, size_t, { size_t len = 0; - if (this->ctr_mode) + if (this->mode == GCRY_CIPHER_MODE_CTR) { /* counter mode does not need any padding */ return 1; } @@ -121,9 +125,14 @@ METHOD(crypter_t, get_iv_size, size_t, { size_t len = 0; - if (this->ctr_mode) + switch (this->mode) { - return sizeof(this->ctr.iv); + case GCRY_CIPHER_MODE_CTR: + return sizeof(this->ctr.iv); + case GCRY_CIPHER_MODE_ECB: + return 0; + default: + break; } gcry_cipher_algo_info(this->alg, GCRYCTL_GET_BLKLEN, NULL, &len); return len; @@ -135,7 +144,7 @@ METHOD(crypter_t, get_key_size, size_t, size_t len = 0; gcry_cipher_algo_info(this->alg, GCRYCTL_GET_KEYLEN, NULL, &len); - if (this->ctr_mode) + if (this->mode == GCRY_CIPHER_MODE_CTR) { return len + sizeof(this->ctr.nonce); } @@ -145,7 +154,7 @@ METHOD(crypter_t, get_key_size, size_t, METHOD(crypter_t, set_key, bool, private_gcrypt_crypter_t *this, chunk_t key) { - if (this->ctr_mode) + if (this->mode == GCRY_CIPHER_MODE_CTR) { /* last 4 bytes are the nonce */ memcpy(this->ctr.nonce, key.ptr + key.len - sizeof(this->ctr.nonce), @@ -308,7 +317,7 @@ gcrypt_crypter_t *gcrypt_crypter_create(encryption_algorithm_t algo, }, }, .alg = gcrypt_alg, - .ctr_mode = mode == GCRY_CIPHER_MODE_CTR, + .mode = mode, ); err = gcry_cipher_open(&this->h, gcrypt_alg, mode, 0); From bdc7f84a234550c8879d4e25e520c6ac66e4a37c Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 13 Sep 2022 16:10:55 +0200 Subject: [PATCH 10/18] unit-tests: Don't use test data on stack for TLS socket tests The stack of that function might not be valid anymore once data is read. --- src/libtls/tests/suites/test_socket.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/libtls/tests/suites/test_socket.c b/src/libtls/tests/suites/test_socket.c index 2c75d9efc..91ee58b97 100644 --- a/src/libtls/tests/suites/test_socket.c +++ b/src/libtls/tests/suites/test_socket.c @@ -647,6 +647,11 @@ static void run_echo_client(echo_server_config_t *config) server->destroy(server); } +/** + * Data for echo test + */ +static chunk_t echo_data = chunk_from_chars(0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08); + /** * Create server/peer configuration */ @@ -660,7 +665,7 @@ static echo_server_config_t *create_config(tls_version_t version, uint16_t port, .addr = "127.0.0.1", .port = port, .cauth = cauth, - .data = chunk_from_chars(0x01,0x02,0x03,0x04,0x05,0x06,0x07,0x08), + .data = echo_data, ); return config; } From 996f557c409e6b65b47b9c81761535e7fd093117 Mon Sep 17 00:00:00 2001 From: Thomas Egerer Date: Fri, 2 Sep 2022 11:54:05 +0000 Subject: [PATCH 11/18] unit-tests: Use allocated listener instead of stack object in exchange tests When using the statement expression and a stack object along with clang-11 and libasan, we get quite a lot of errors about reading invalid memory. This is due to clang making the actual listener_t local to the block, such that the access outside of the macros using _assert_payload is (correctly) considered an error. By using a heap allocated object, we can destroy it once the listener returns FALSE (cleaning up properly), and since bus_t does not touch the listener after that, we don't get any errors from libasan. Co-authored-by: Tobias Brunner --- src/libcharon/tests/suites/test_ike_mid_sync.c | 11 +++++++---- src/libcharon/tests/utils/exchange_test_asserts.c | 2 ++ src/libcharon/tests/utils/exchange_test_asserts.h | 10 ++++++---- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/libcharon/tests/suites/test_ike_mid_sync.c b/src/libcharon/tests/suites/test_ike_mid_sync.c index fa7592a6a..efe58c23f 100644 --- a/src/libcharon/tests/suites/test_ike_mid_sync.c +++ b/src/libcharon/tests/suites/test_ike_mid_sync.c @@ -22,7 +22,7 @@ #include #include -/** +/* * FIXME: Since we don't have the server side yet, this is kind of a hack!!! */ @@ -37,15 +37,18 @@ static bool add_notify(listener_t *listener, ike_sa_t *ike_sa, { message->add_notify(message, FALSE, IKEV2_MESSAGE_ID_SYNC_SUPPORTED, chunk_empty); + free(listener); return FALSE; } return TRUE; } + #define add_notify_to_ike_auth() ({ \ - listener_t _notify_listener = { \ + listener_t *_notify_listener; \ + INIT(_notify_listener, \ .message = add_notify, \ - }; \ - exchange_test_helper->add_listener(exchange_test_helper, &_notify_listener); \ + ); \ + exchange_test_helper->add_listener(exchange_test_helper, _notify_listener); \ }) /** diff --git a/src/libcharon/tests/utils/exchange_test_asserts.c b/src/libcharon/tests/utils/exchange_test_asserts.c index 8c39a6643..1a4fdda83 100644 --- a/src/libcharon/tests/utils/exchange_test_asserts.c +++ b/src/libcharon/tests/utils/exchange_test_asserts.c @@ -178,6 +178,8 @@ bool exchange_test_asserts_message(listener_t *listener, ike_sa_t *ike_sa, assert_message_rule(this, message, &this->rules[i]); } } + free(this->rules); + free(this); return FALSE; } return TRUE; diff --git a/src/libcharon/tests/utils/exchange_test_asserts.h b/src/libcharon/tests/utils/exchange_test_asserts.h index ac98dd452..a58832d73 100644 --- a/src/libcharon/tests/utils/exchange_test_asserts.h +++ b/src/libcharon/tests/utils/exchange_test_asserts.h @@ -350,16 +350,18 @@ bool exchange_test_asserts_message(listener_t *this, ike_sa_t *ike_sa, #define _assert_payload(dir, c, ...) ({ \ listener_message_rule_t _rules[] = { __VA_ARGS__ }; \ - listener_message_assert_t _listener = { \ + listener_message_assert_t *_listener; \ + INIT(_listener, \ .listener = { .message = exchange_test_asserts_message, }, \ .file = __FILE__, \ .line = __LINE__, \ .incoming = streq(dir, "IN") ? TRUE : FALSE, \ .count = c, \ - .rules = _rules, \ + .rules = malloc(sizeof(_rules)), \ .num_rules = countof(_rules), \ - }; \ - exchange_test_helper->add_listener(exchange_test_helper, &_listener.listener); \ + ); \ + memcpy(_listener->rules, _rules, sizeof(_rules)); \ + exchange_test_helper->add_listener(exchange_test_helper, &_listener->listener); \ }) /** From c9c8911478e1719b778025224747ad9896707c5f Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Wed, 14 Sep 2022 16:03:57 +0200 Subject: [PATCH 12/18] unit-tests: Don't link files from libimcv into the test executable This causes odr-violation errors with libasan as some symbols will be defined twice, once in the linked libimcv and once in the test executable itself. --- src/libimcv/Makefile.am | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/libimcv/Makefile.am b/src/libimcv/Makefile.am index 7be5da144..e4c1d48ba 100644 --- a/src/libimcv/Makefile.am +++ b/src/libimcv/Makefile.am @@ -199,25 +199,9 @@ TESTS = imcv_tests check_PROGRAMS = $(TESTS) imcv_tests_SOURCES = \ - ita/ita_attr_command.c \ - pa_tnc/pa_tnc_attr_manager.c \ - seg/seg_env.c seg/seg_contract.c \ - seg/seg_contract_manager.c \ - swid_gen/swid_gen.c \ - swima/swima_data_model.c \ - swima/swima_event.c \ - swima/swima_events.c \ - swima/swima_record.c \ - swima/swima_inventory.c \ - swima/swima_collector.c \ suites/test_imcv_seg.c \ suites/test_imcv_swima.c \ - ietf/ietf_attr_pa_tnc_error.c \ - ietf/swima/ietf_swima_attr_req.c \ - ietf/swima/ietf_swima_attr_sw_inv.c \ - ietf/swima/ietf_swima_attr_sw_ev.c \ - tcg/seg/tcg_seg_attr_seg_env.c \ - imcv.c imcv_tests.h imcv_tests.c + imcv_tests.h imcv_tests.c imcv_tests_CFLAGS = \ -I$(top_srcdir)/src/libimcv \ From ae9d110dd9eaaf2f414da7f19ce3f79f17652bef Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 13 Sep 2022 17:25:12 +0200 Subject: [PATCH 13/18] configure: Log if -Werror is enabled --- configure.ac | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index 0a083fbc0..b152456ba 100644 --- a/configure.ac +++ b/configure.ac @@ -1448,9 +1448,13 @@ fi # modify CFLAGS as needed, do this late so we don't affect configure checks CFLAGS="$CFLAGS -include $(pwd)/config.h" +AC_MSG_CHECKING([for use of -Werror and additional warnings]) WARN_CFLAGS= if test x$warnings = xtrue; then WARN_CFLAGS="-Werror -Wall -Wextra" + AC_MSG_RESULT([yes]) +else + AC_MSG_RESULT([no]) fi # disable some warnings, whether explicitly enabled above or by default # these are not compatible with our custom printf specifiers From eab9cd86612140990e9e7227014036c14a8c636c Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 15 Sep 2022 10:08:10 +0200 Subject: [PATCH 14/18] kernel-netlink: Fix compiler warnings with strncpy() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Normally, GCC sees that we terminate the destination with a zero byte. However, when using `-fsanitize=address`, there seems to be additional instrumentation code after strncpy() so GCC produces warnings like these: ‘__builtin_strncpy’ specified bound 16 equals destination size [-Wstringop-truncation] --- .../kernel_netlink/kernel_netlink_ipsec.c | 26 +++++++++---------- .../kernel_netlink/kernel_netlink_net.c | 2 +- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/libcharon/plugins/kernel_netlink/kernel_netlink_ipsec.c b/src/libcharon/plugins/kernel_netlink/kernel_netlink_ipsec.c index 6fac8c873..ca6ce175b 100644 --- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_ipsec.c +++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_ipsec.c @@ -1411,7 +1411,7 @@ static void netlink_find_offload_feature(const char *ifname) .cmd = ETHTOOL_GSSET_INFO, .sset_mask = 1ULL << ETH_SS_FEATURES, ); - strncpy(ifr.ifr_name, ifname, IFNAMSIZ); + strncpy(ifr.ifr_name, ifname, IFNAMSIZ-1); ifr.ifr_name[IFNAMSIZ-1] = '\0'; ifr.ifr_data = (void*)sset_info; @@ -1427,7 +1427,7 @@ static void netlink_find_offload_feature(const char *ifname) .cmd = ETHTOOL_GSTRINGS, .string_set = ETH_SS_FEATURES, ); - strncpy(ifr.ifr_name, ifname, IFNAMSIZ); + strncpy(ifr.ifr_name, ifname, IFNAMSIZ-1); ifr.ifr_name[IFNAMSIZ-1] = '\0'; ifr.ifr_data = (void*)cmd; @@ -1486,7 +1486,7 @@ static bool netlink_detect_offload(const char *ifname) .cmd = ETHTOOL_GFEATURES, .size = netlink_hw_offload.total_blocks, ); - strncpy(ifr.ifr_name, ifname, IFNAMSIZ); + strncpy(ifr.ifr_name, ifname, IFNAMSIZ-1); ifr.ifr_name[IFNAMSIZ-1] = '\0'; ifr.ifr_data = (void*)cmd; @@ -1778,8 +1778,8 @@ METHOD(kernel_ipsec_t, add_sa, status_t, } algo->alg_key_len = data->enc_key.len * 8; algo->alg_icv_len = icv_size; - strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)); - algo->alg_name[sizeof(algo->alg_name) - 1] = '\0'; + strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)-1); + algo->alg_name[sizeof(algo->alg_name)-1] = '\0'; memcpy(algo->alg_key, data->enc_key.ptr, data->enc_key.len); break; } @@ -1805,8 +1805,8 @@ METHOD(kernel_ipsec_t, add_sa, status_t, goto failed; } algo->alg_key_len = data->enc_key.len * 8; - strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)); - algo->alg_name[sizeof(algo->alg_name) - 1] = '\0'; + strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)-1); + algo->alg_name[sizeof(algo->alg_name)-1] = '\0'; memcpy(algo->alg_key, data->enc_key.ptr, data->enc_key.len); } } @@ -1862,8 +1862,8 @@ METHOD(kernel_ipsec_t, add_sa, status_t, } algo->alg_key_len = data->int_key.len * 8; algo->alg_trunc_len = trunc_len; - strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)); - algo->alg_name[sizeof(algo->alg_name) - 1] = '\0'; + strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)-1); + algo->alg_name[sizeof(algo->alg_name)-1] = '\0'; memcpy(algo->alg_key, data->int_key.ptr, data->int_key.len); } else @@ -1877,8 +1877,8 @@ METHOD(kernel_ipsec_t, add_sa, status_t, goto failed; } algo->alg_key_len = data->int_key.len * 8; - strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)); - algo->alg_name[sizeof(algo->alg_name) - 1] = '\0'; + strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)-1); + algo->alg_name[sizeof(algo->alg_name)-1] = '\0'; memcpy(algo->alg_key, data->int_key.ptr, data->int_key.len); } } @@ -1904,8 +1904,8 @@ METHOD(kernel_ipsec_t, add_sa, status_t, goto failed; } algo->alg_key_len = 0; - strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)); - algo->alg_name[sizeof(algo->alg_name) - 1] = '\0'; + strncpy(algo->alg_name, alg_name, sizeof(algo->alg_name)-1); + algo->alg_name[sizeof(algo->alg_name)-1] = '\0'; } if (data->encap) diff --git a/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c b/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c index 97309a6d2..7dcb15676 100644 --- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c +++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c @@ -1126,7 +1126,7 @@ static void process_link(private_kernel_netlink_net_t *this, ); this->ifaces->insert_last(this->ifaces, entry); } - strncpy(entry->ifname, name, IFNAMSIZ); + strncpy(entry->ifname, name, IFNAMSIZ-1); entry->ifname[IFNAMSIZ-1] = '\0'; entry->usable = charon->kernel->is_interface_usable(charon->kernel, name); From a7e8cb8f6146f10a324db1fe5a60638736ddb415 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 15 Sep 2022 10:49:42 +0200 Subject: [PATCH 15/18] tun-device: Fix compiler warning Only the second was reported by the compiler (depending on the version and similarly to the previous commit only with AddressSanitizer active). The strncpy() call for UTUN_CONTROL_NAME was simply wrong. --- src/libstrongswan/networking/tun_device.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libstrongswan/networking/tun_device.c b/src/libstrongswan/networking/tun_device.c index 77074b2ee..39fac4e0a 100644 --- a/src/libstrongswan/networking/tun_device.c +++ b/src/libstrongswan/networking/tun_device.c @@ -432,7 +432,7 @@ static bool init_tun(private_tun_device_t *this, const char *name_tmpl) } /* get a control identifier for the utun kernel extension */ - strncpy(info.ctl_name, UTUN_CONTROL_NAME, strlen(UTUN_CONTROL_NAME)); + strncpy(info.ctl_name, UTUN_CONTROL_NAME, sizeof(info.ctl_name)-1); if (ioctl(this->tunfd, CTLIOCGINFO, &info) < 0) { DBG1(DBG_LIB, "failed to ioctl tundevice: %s", strerror(errno)); @@ -466,7 +466,7 @@ static bool init_tun(private_tun_device_t *this, const char *name_tmpl) struct ifreq ifr; - strncpy(this->if_name, name_tmpl ?: "tun%d", IFNAMSIZ); + strncpy(this->if_name, name_tmpl ?: "tun%d", IFNAMSIZ-1); this->if_name[IFNAMSIZ-1] = '\0'; this->tunfd = open("/dev/net/tun", O_RDWR); From c4563abc2eddbb066507afe6a814f7ed6bf0d41e Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 13 Sep 2022 18:05:01 +0200 Subject: [PATCH 16/18] github: Use OpenSSL 3.0.5 for tests --- scripts/test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/test.sh b/scripts/test.sh index 692a9c17c..63d36f218 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -91,7 +91,7 @@ build_tss2() build_openssl() { - SSL_REV=3.0.2 + SSL_REV=3.0.5 SSL_PKG=openssl-$SSL_REV SSL_DIR=$DEPS_BUILD_DIR/$SSL_PKG SSL_SRC=https://www.openssl.org/source/$SSL_PKG.tar.gz From 8a57c2ab521e4f52a28b9fb185aedf37635f425e Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 15 Sep 2022 12:05:14 +0200 Subject: [PATCH 17/18] configure: Add an option to build with AddressSanitizer --- .lsan.suppressions | 7 ++++++ configure.ac | 24 ++++++++++++++++++- src/libcharon/plugins/vici/python/Makefile.am | 2 +- 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 .lsan.suppressions diff --git a/.lsan.suppressions b/.lsan.suppressions new file mode 100644 index 000000000..be166e53b --- /dev/null +++ b/.lsan.suppressions @@ -0,0 +1,7 @@ +leak:EVP_CIPHER_fetch +leak:EVP_KEYEXCH_fetch +leak:EVP_KEYMGMT_fetch +leak:EVP_RAND_fetch +leak:OSSL_DECODER_do_all_provided +leak:OSSL_ENCODER_do_all_provided +leak:OSSL_PROVIDER_load diff --git a/configure.ac b/configure.ac index b152456ba..dcb92da92 100644 --- a/configure.ac +++ b/configure.ac @@ -1,5 +1,5 @@ # -# Copyright (C) 2007-2017 Tobias Brunner +# Copyright (C) 2007-2022 Tobias Brunner # Copyright (C) 2006-2022 Andreas Steffen # Copyright (C) 2006-2014 Martin Willi # @@ -327,6 +327,7 @@ ARG_ENABL_SET([tss-trousers], [enable the use of the TrouSerS Trusted Software ARG_ENABL_SET([tss-tss2], [enable the use of the TSS 2.0 Trusted Software Stack]) # compile options +ARG_ENABL_SET([asan], [enable build with AddressSanitizer (ASan).]) ARG_ENABL_SET([coverage], [enable lcov coverage report generation.]) ARG_ENABL_SET([git-version], [use output of 'git describe' as version information in executables.]) ARG_ENABL_SET([leak-detective], [enable malloc hooks to find memory leaks.]) @@ -1378,6 +1379,27 @@ if test x$fuzzing = xtrue; then esac fi +if test x$asan = xtrue; then + # adding this here and not earlier or passed to the script avoids issues + # e.g. with libpthread (libasan provides stubs for its functions but no full + # implementation so configure does not detect that -lpthread is required + # when GCC is used, clang always adds -lpthread) + CFLAGS="$CFLAGS -fsanitize=address -fno-omit-frame-pointer" + # this is necessary so AddressSanitizer can resolve symbols e.g. for + # C++ exceptions that are used in libbotan + if test x$botan = xtrue; then + LDFLAGS="$LDFLAGS -lstdc++" + fi + if test x$openssl = xtrue; then + # we need to suppress some leaks with OpenSSL 3 as we don't deinitialze + # it properly + AC_SUBST(LSAN_OPTIONS, [suppressions=\${abs_top_srcdir}/.lsan.suppressions]) + # use this instead of AM_TESTS_ENVIRONMENT as we don't use the parallel + # test harness + AC_SUBST(TESTS_ENVIRONMENT, ['export LSAN_OPTIONS="$(LSAN_OPTIONS)";']) + fi +fi + if test x$ruby_gems = xtrue; then AC_PATH_PROG([GEM], [gem], [], [$PATH:/bin:/usr/bin:/usr/local/bin]) if test x$GEM = x; then diff --git a/src/libcharon/plugins/vici/python/Makefile.am b/src/libcharon/plugins/vici/python/Makefile.am index a2e7596e4..aa036c96c 100644 --- a/src/libcharon/plugins/vici/python/Makefile.am +++ b/src/libcharon/plugins/vici/python/Makefile.am @@ -40,7 +40,7 @@ install-exec-local: dist/vici-$(PYTHON_PACKAGE_VERSION)-py$(PYTHON_VERSION).egg endif if USE_TOX - TESTS_ENVIRONMENT = export TOX=$(TOX); + TESTS_ENVIRONMENT += export TOX=$(TOX); AM_TESTS_FD_REDIRECT = 1>&2 TESTS = tox.sh endif From d29af802bbb5c043123e0a876048bb2ed7fe727d Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Tue, 13 Sep 2022 17:23:55 +0200 Subject: [PATCH 18/18] github: Enable AddressSanitizer if leak-detective is disabled At least for the tests where it is available and works. It conflicts with the instrumentation used by the coverage and fuzzing (and possibly sonarcloud) tests, the toolchain for the Windows builds doesn't seem to support it, and on FreeBSD the test executables hang due to a compatibility issue with FreeBSD's qsort(), which has been fixed [1], but that has not made it into the clang version in the base system. For the custom OpenSSL build, debug symbols are enabled so we can suppress some leaks properly. [1] https://github.com/llvm/llvm-project/issues/46176 --- scripts/test.sh | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/scripts/test.sh b/scripts/test.sh index 63d36f218..9e102553a 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -96,7 +96,7 @@ build_openssl() SSL_DIR=$DEPS_BUILD_DIR/$SSL_PKG SSL_SRC=https://www.openssl.org/source/$SSL_PKG.tar.gz SSL_INS=$DEPS_PREFIX/ssl - SSL_OPT="shared no-tls no-dtls no-ssl3 no-zlib no-comp no-idea no-psk no-srp + SSL_OPT="-d shared no-tls no-dtls no-ssl3 no-zlib no-comp no-idea no-psk no-srp no-stdio no-tests enable-rfc3779 enable-ec_nistp_64_gcc_128" if test -d "$SSL_DIR"; then @@ -471,6 +471,21 @@ CONFIG="$CONFIG --enable-monolithic=${MONOLITHIC-no} --enable-leak-detective=${LEAK_DETECTIVE-no}" +case "$TEST" in + coverage|freebsd|fuzzing|sonarcloud|win*) + # don't use AddressSanitizer if it's not available or causes conflicts + CONFIG="$CONFIG --disable-asan" + ;; + *) + if [ "$ID" = "ubuntu" -a "$VERSION_ID" = "18.04" ]; then + # the libstdc++ workaround for libbotan doesn't work on Ubuntu 18.04 + CONFIG="$CONFIG --disable-asan" + elif [ "$LEAK_DETECTIVE" != "yes" ]; then + CONFIG="$CONFIG --enable-asan" + fi + ;; +esac + echo "$ ./autogen.sh" ./autogen.sh || exit $? echo "$ CC=$CC CFLAGS=\"$CFLAGS\" ./configure $CONFIG"