From be6f31a3390c2e271eb79db36339340b1010535f Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Wed, 31 Jan 2024 00:05:33 +0000 Subject: [PATCH 1/8] build: allow -Werror to be toggled for tests --- CMakeLists.txt | 3 +++ tests/unit/s2n_signature_algorithms_test.c | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ac12886c7a9..05f041120cb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -501,6 +501,9 @@ if (BUILD_TESTING) ) endif() target_compile_options(${test_case_name} PRIVATE -Wno-implicit-function-declaration -Wno-deprecated -D_POSIX_C_SOURCE=200809L -std=gnu99) + if (UNSAFE_TREAT_WARNINGS_AS_ERRORS) + target_compile_options(${test_case_name} PRIVATE -Werror ) + endif () if (S2N_LTO) target_compile_options(${test_case_name} PRIVATE -flto) endif() diff --git a/tests/unit/s2n_signature_algorithms_test.c b/tests/unit/s2n_signature_algorithms_test.c index 6d4145e850b..63bea8097bf 100644 --- a/tests/unit/s2n_signature_algorithms_test.c +++ b/tests/unit/s2n_signature_algorithms_test.c @@ -239,7 +239,7 @@ int main(int argc, char **argv) /* Test: ECDSA */ { const struct s2n_signature_scheme *expected = &s2n_ecdsa_sha1; - conn->handshake_params.client_cert_pkey_type = S2N_AUTHENTICATION_ECDSA; + conn->handshake_params.client_cert_pkey_type = S2N_PKEY_TYPE_ECDSA; EXPECT_SUCCESS(s2n_connection_set_config(conn, client_ecdsa_config)); /* TLS1.1 selects the default */ @@ -256,7 +256,7 @@ int main(int argc, char **argv) /* Test: RSA */ { const struct s2n_signature_scheme *expected = &s2n_rsa_pkcs1_md5_sha1; - conn->handshake_params.client_cert_pkey_type = S2N_AUTHENTICATION_RSA; + conn->handshake_params.client_cert_pkey_type = S2N_PKEY_TYPE_RSA; EXPECT_SUCCESS(s2n_connection_set_config(conn, client_rsa_config)); /* TLS1.1 selects the default */ From a17680da1a2ce78b701cc7028423baff60b57abd Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Wed, 31 Jan 2024 00:35:50 +0000 Subject: [PATCH 2/8] update cflags to match make default --- CMakeLists.txt | 7 +++++-- tests/unit/s2n_ktls_test.c | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 05f041120cb..bdb35e6932e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -500,9 +500,12 @@ if (BUILD_TESTING) find . -name '${test_case_name}.c.o' -exec objcopy --redefine-syms libcrypto.symbols {} \\\; ) endif() - target_compile_options(${test_case_name} PRIVATE -Wno-implicit-function-declaration -Wno-deprecated -D_POSIX_C_SOURCE=200809L -std=gnu99) + target_compile_options(${test_case_name} PRIVATE -Wall -Werror -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized + -Wshadow -Wcast-align -Wwrite-strings -fPIC -Wno-missing-braces + -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security + -Wno-implicit-function-declaration -Wno-deprecated -D_POSIX_C_SOURCE=200809L -std=gnu99) if (UNSAFE_TREAT_WARNINGS_AS_ERRORS) - target_compile_options(${test_case_name} PRIVATE -Werror ) + target_compile_options(${test_case_name} PRIVATE -Werror ) endif () if (S2N_LTO) target_compile_options(${test_case_name} PRIVATE -flto) diff --git a/tests/unit/s2n_ktls_test.c b/tests/unit/s2n_ktls_test.c index 5309911fbcb..58effec553f 100644 --- a/tests/unit/s2n_ktls_test.c +++ b/tests/unit/s2n_ktls_test.c @@ -184,7 +184,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(crypto_info.value.size, sizeof(crypto_info.ciphers.aes_gcm_128)); EXPECT_EQUAL(crypto_info.value.data, (uint8_t *) &crypto_info.ciphers.aes_gcm_128); s2n_ktls_crypto_info_tls12_aes_gcm_128 *value = - (s2n_ktls_crypto_info_tls12_aes_gcm_128 *) crypto_info.value.data; + (s2n_ktls_crypto_info_tls12_aes_gcm_128 *) (void *) crypto_info.value.data; EXPECT_EQUAL(test_key.size, sizeof(value->key)); EXPECT_BYTEARRAY_EQUAL(test_key.data, value->key, sizeof(value->key)); @@ -216,7 +216,7 @@ int main(int argc, char **argv) EXPECT_EQUAL(crypto_info.value.size, sizeof(crypto_info.ciphers.aes_gcm_256)); EXPECT_EQUAL(crypto_info.value.data, (uint8_t *) &crypto_info.ciphers.aes_gcm_256); s2n_ktls_crypto_info_tls12_aes_gcm_256 *value = - (s2n_ktls_crypto_info_tls12_aes_gcm_256 *) crypto_info.value.data; + (s2n_ktls_crypto_info_tls12_aes_gcm_256 *) (void *) crypto_info.value.data; EXPECT_EQUAL(test_key.size, sizeof(value->key)); EXPECT_BYTEARRAY_EQUAL(test_key.data, value->key, sizeof(value->key)); From 6c78ac7f69a62eb0237ac26b04e4a372fe67b282 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Wed, 31 Jan 2024 00:49:07 +0000 Subject: [PATCH 3/8] fix -Werror typo --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index bdb35e6932e..d459e561940 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -500,7 +500,7 @@ if (BUILD_TESTING) find . -name '${test_case_name}.c.o' -exec objcopy --redefine-syms libcrypto.symbols {} \\\; ) endif() - target_compile_options(${test_case_name} PRIVATE -Wall -Werror -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized + target_compile_options(${test_case_name} PRIVATE -Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow -Wcast-align -Wwrite-strings -fPIC -Wno-missing-braces -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security -Wno-implicit-function-declaration -Wno-deprecated -D_POSIX_C_SOURCE=200809L -std=gnu99) From 600e9b2fe16b2a7d8f68bd8cd3bc222ea5b4ff90 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Thu, 1 Feb 2024 02:11:14 +0000 Subject: [PATCH 4/8] remove unnecessary warning suppressions --- CMakeLists.txt | 11 ++++++----- tests/unit/s2n_x509_validator_test.c | 3 +-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index d459e561940..5d84f567c81 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -500,10 +500,11 @@ if (BUILD_TESTING) find . -name '${test_case_name}.c.o' -exec objcopy --redefine-syms libcrypto.symbols {} \\\; ) endif() - target_compile_options(${test_case_name} PRIVATE -Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized - -Wshadow -Wcast-align -Wwrite-strings -fPIC -Wno-missing-braces - -Wno-deprecated-declarations -Wno-unknown-pragmas -Wformat-security - -Wno-implicit-function-declaration -Wno-deprecated -D_POSIX_C_SOURCE=200809L -std=gnu99) + target_compile_options(${test_case_name} PRIVATE + -Wall -Wimplicit -Wunused -Wcomment -Wchar-subscripts -Wuninitialized + -Wshadow -Wcast-align -Wwrite-strings -Wformat-security + -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-deprecated + -fPIC -D_POSIX_C_SOURCE=200809L -std=gnu99) if (UNSAFE_TREAT_WARNINGS_AS_ERRORS) target_compile_options(${test_case_name} PRIVATE -Werror ) endif () @@ -549,7 +550,7 @@ if (BUILD_TESTING) # Based off the flags in tests/benchmark/Makefile target_compile_options(${benchmark_name} PRIVATE -pedantic -Wall -Werror -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow -Wcast-qual -Wcast-align -Wwrite-strings -Wno-deprecated-declarations - -Wno-unknown-pragmas -Wformat-security -Wno-missing-braces -fvisibility=hidden -Wno-unreachable-code + -Wno-unknown-pragmas -Wformat-security -fvisibility=hidden -Wno-unreachable-code -Wno-unused-but-set-variable) endforeach(benchmark) endif() diff --git a/tests/unit/s2n_x509_validator_test.c b/tests/unit/s2n_x509_validator_test.c index 163fb5528a7..72a1aa93960 100644 --- a/tests/unit/s2n_x509_validator_test.c +++ b/tests/unit/s2n_x509_validator_test.c @@ -13,11 +13,10 @@ * permissions and limitations under the License. */ +#include "crypto/s2n_openssl_x509.h" #include "s2n_test.h" #include "testlib/s2n_testlib.h" -DEFINE_POINTER_CLEANUP_FUNC(X509 *, X509_free); - static int mock_time(void *data, uint64_t *timestamp) { *timestamp = *(uint64_t *) data; From e8e676d6ba353a665c77c65148ea60e4b204ad20 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Fri, 2 Feb 2024 20:38:03 +0000 Subject: [PATCH 5/8] use new status toggle UNSAFE_TREAT_WARNINGS_AS_ERRORS is on by default, which is going to make it incredibly hard to roll out new changes. --- CMakeLists.txt | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5d84f567c81..e1c852da8e1 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -24,6 +24,8 @@ otherwise a crypto target needs to be defined." ON) option(UNSAFE_TREAT_WARNINGS_AS_ERRORS "Compiler warnings are treated as errors. Warnings may indicate danger points where you should verify with the S2N-TLS developers that the security of the library is not compromised. Turn this OFF to ignore warnings." ON) +option(S2N_WERROR_ALL "This option will cause all artifacts linked to libs2n to use the +-Werror setting." OFF) option(S2N_INTERN_LIBCRYPTO "This ensures that s2n-tls is compiled and deployed with a specific version of libcrypto by interning the code and hiding symbols. This also enables s2n-tls to be loaded in an application with an otherwise conflicting libcrypto version." OFF) @@ -136,7 +138,9 @@ target_compile_options(${PROJECT_NAME} PRIVATE -pedantic -std=gnu99 -Wall -Wimpl -Wno-missing-braces -Wsign-compare -Wno-strict-prototypes -Wa,--noexecstack ) -if (UNSAFE_TREAT_WARNINGS_AS_ERRORS) +if (S2N_WERROR_ALL) + target_compile_options(${PROJECT_NAME} PUBLIC -Werror) +elseif (UNSAFE_TREAT_WARNINGS_AS_ERRORS) target_compile_options(${PROJECT_NAME} PRIVATE -Werror ) endif () @@ -505,9 +509,6 @@ if (BUILD_TESTING) -Wshadow -Wcast-align -Wwrite-strings -Wformat-security -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-deprecated -fPIC -D_POSIX_C_SOURCE=200809L -std=gnu99) - if (UNSAFE_TREAT_WARNINGS_AS_ERRORS) - target_compile_options(${test_case_name} PRIVATE -Werror ) - endif () if (S2N_LTO) target_compile_options(${test_case_name} PRIVATE -flto) endif() From 87364ba5ed09e770c2a9b92c98b1bf247129601d Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Fri, 2 Feb 2024 21:56:02 +0000 Subject: [PATCH 6/8] remove usage of strcasestr extension --- tests/unit/s2n_build_test.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/unit/s2n_build_test.c b/tests/unit/s2n_build_test.c index 7e707954df4..395139d7ab8 100644 --- a/tests/unit/s2n_build_test.c +++ b/tests/unit/s2n_build_test.c @@ -23,6 +23,8 @@ #include "crypto/s2n_openssl.h" #include "s2n_test.h" +#define MAX_LIBCRYPTO_NAME_LEN 100 + int tokenize_s2n_libcrypto(char *s2n_libcrypto, char **name, char **version) { if (name == NULL || version == NULL || s2n_libcrypto == NULL) { @@ -44,6 +46,18 @@ int tokenize_s2n_libcrypto(char *s2n_libcrypto, char **name, char **version) return S2N_SUCCESS; } +S2N_RESULT s2n_test_lowercase_copy(const char *input, char* destination, size_t max_len) { + RESULT_ENSURE_REF(input); + RESULT_ENSURE_REF(destination); + + for (size_t i = 0; i < strlen(input); i++) { + RESULT_ENSURE_LT(i, max_len); + destination[i] = tolower(input[i]); + } + + return S2N_RESULT_OK; +} + int main() { BEGIN_TEST(); @@ -69,8 +83,9 @@ int main() END_TEST(); } - char s2n_libcrypto_copy[100] = { 0 }; - strncpy(s2n_libcrypto_copy, s2n_libcrypto, 99); + char s2n_libcrypto_copy[MAX_LIBCRYPTO_NAME_LEN] = { 0 }; + EXPECT_TRUE(strlen(s2n_libcrypto) < MAX_LIBCRYPTO_NAME_LEN); + EXPECT_OK(s2n_test_lowercase_copy(s2n_libcrypto, &s2n_libcrypto_copy[0], s2n_array_len(s2n_libcrypto_copy))); char *name = NULL; char *version = NULL; EXPECT_SUCCESS(tokenize_s2n_libcrypto(s2n_libcrypto_copy, &name, &version)); @@ -83,8 +98,9 @@ int main() EXPECT_TRUE(s2n_libcrypto_is_awslc()); } else { /* Any other library should have the name of the library (modulo case) in its version string. */ - const char *ssleay_version_text = SSLeay_version(SSLEAY_VERSION); - EXPECT_NOT_NULL(strcasestr(ssleay_version_text, name)); + char ssleay_version_text[MAX_LIBCRYPTO_NAME_LEN] = { 0 }; + EXPECT_OK(s2n_test_lowercase_copy(SSLeay_version(SSLEAY_VERSION), &ssleay_version_text[0], MAX_LIBCRYPTO_NAME_LEN)); + EXPECT_NOT_NULL(strstr(ssleay_version_text, name)); } }; From c4b050c58bc7a57f9a5495c0d1f99d89d2abb651 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Fri, 2 Feb 2024 22:44:22 +0000 Subject: [PATCH 7/8] address ci failures - clang format --- tests/unit/s2n_build_test.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/s2n_build_test.c b/tests/unit/s2n_build_test.c index 395139d7ab8..75195fe0d2a 100644 --- a/tests/unit/s2n_build_test.c +++ b/tests/unit/s2n_build_test.c @@ -46,7 +46,8 @@ int tokenize_s2n_libcrypto(char *s2n_libcrypto, char **name, char **version) return S2N_SUCCESS; } -S2N_RESULT s2n_test_lowercase_copy(const char *input, char* destination, size_t max_len) { +S2N_RESULT s2n_test_lowercase_copy(const char *input, char *destination, size_t max_len) +{ RESULT_ENSURE_REF(input); RESULT_ENSURE_REF(destination); From ba416ce405cd28c9d73e14305ab9f857ebb10ce3 Mon Sep 17 00:00:00 2001 From: James Mayclin Date: Tue, 6 Feb 2024 20:11:57 +0000 Subject: [PATCH 8/8] don't touch flags for benchmarks --- CMakeLists.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e1c852da8e1..0fe35d025a4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -551,7 +551,7 @@ if (BUILD_TESTING) # Based off the flags in tests/benchmark/Makefile target_compile_options(${benchmark_name} PRIVATE -pedantic -Wall -Werror -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow -Wcast-qual -Wcast-align -Wwrite-strings -Wno-deprecated-declarations - -Wno-unknown-pragmas -Wformat-security -fvisibility=hidden -Wno-unreachable-code + -Wno-unknown-pragmas -Wformat-security -Wno-missing-braces -fvisibility=hidden -Wno-unreachable-code -Wno-unused-but-set-variable) endforeach(benchmark) endif()