From c8ef580168e407dae9e2283b6b14808806098c15 Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Wed, 19 Feb 2025 11:46:52 -0800 Subject: [PATCH 1/6] Fix C++98 compatibility in our header files (#2193) We encountered this when trying to compile Xtrabackup on gcc versions lower than 10. Apparently scoped enums are a C++11 feature, which the compiler can't understand without additional linker flags. We also encountered this in 42a8dba, which prompted us to move it to `ssl.h`. Upon closer examination, I don't think the C++11 usage is a necessity especially with the pure C fallback that was included. Predeclaring enums reduces compilation dependencies and build times, but our build seems to work fine without it. Removing this would let us encounter less build issues with projects that aren't expecting C++ in our header files. Asides from the issue with scoped enums mentioned above, we decided to check for C++98 compatibility in CI and fix any potential issues with `-Wpedantic` while we're at it. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- include/openssl/aead.h | 2 +- include/openssl/bn.h | 2 +- include/openssl/crypto.h | 2 +- include/openssl/curve25519.h | 2 +- include/openssl/ec.h | 2 +- include/openssl/service_indicator.h | 2 +- include/openssl/ssl.h | 62 ++++++++----------- .../github_ci_linux_x86_omnibus.yaml | 10 +++ tests/ci/codebuild/linux-x86/pre-push.yml | 2 +- tests/ci/run_posix_tests.sh | 10 --- ...99_gcc_test.sh => c99_cplusplus98_test.sh} | 14 +++-- .../coding_guidelines_test.sh | 8 --- 12 files changed, 52 insertions(+), 66 deletions(-) rename tests/coding_guidelines/{c99_gcc_test.sh => c99_cplusplus98_test.sh} (59%) delete mode 100755 tests/coding_guidelines/coding_guidelines_test.sh diff --git a/include/openssl/aead.h b/include/openssl/aead.h index 7f9b262379..64df91ecfe 100644 --- a/include/openssl/aead.h +++ b/include/openssl/aead.h @@ -437,7 +437,7 @@ OPENSSL_EXPORT const EVP_AEAD *EVP_aead_aes_256_gcm_tls13(void); // evp_aead_direction_t denotes the direction of an AEAD operation. enum evp_aead_direction_t { evp_aead_open, - evp_aead_seal, + evp_aead_seal }; // EVP_AEAD_CTX_init_with_direction calls |EVP_AEAD_CTX_init| for normal diff --git a/include/openssl/bn.h b/include/openssl/bn.h index ad539dad20..f96eae79bb 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -769,7 +769,7 @@ OPENSSL_EXPORT int BN_generate_prime_ex(BIGNUM *ret, int bits, int safe, enum bn_primality_result_t { bn_probably_prime, bn_composite, - bn_non_prime_power_composite, + bn_non_prime_power_composite }; // BN_enhanced_miller_rabin_primality_test tests whether |w| is probably a prime diff --git a/include/openssl/crypto.h b/include/openssl/crypto.h index 0e81efcdf9..0f39e8ab1a 100644 --- a/include/openssl/crypto.h +++ b/include/openssl/crypto.h @@ -130,7 +130,7 @@ enum fips_counter_t { fips_counter_evp_aes_128_ctr = 2, fips_counter_evp_aes_256_ctr = 3, - fips_counter_max = 3, + fips_counter_max = 3 }; // FIPS_read_counter returns a counter of the number of times the specific diff --git a/include/openssl/curve25519.h b/include/openssl/curve25519.h index 430ec09f27..95511a920d 100644 --- a/include/openssl/curve25519.h +++ b/include/openssl/curve25519.h @@ -178,7 +178,7 @@ OPENSSL_EXPORT void ED25519_keypair_from_seed(uint8_t out_public_key[ED25519_PUB // must be “Alice” and the other be “Bob”. enum spake2_role_t { spake2_role_alice, - spake2_role_bob, + spake2_role_bob }; // SPAKE2_CTX_new creates a new |SPAKE2_CTX| (which can only be used for a diff --git a/include/openssl/ec.h b/include/openssl/ec.h index 0c1b961c79..e00ed304af 100644 --- a/include/openssl/ec.h +++ b/include/openssl/ec.h @@ -92,7 +92,7 @@ typedef enum { // POINT_CONVERSION_HYBRID indicates that the point is encoded as z||x||y, // where z specifies which solution of the quadratic equation y is. - POINT_CONVERSION_HYBRID = 6, + POINT_CONVERSION_HYBRID = 6 } point_conversion_form_t; diff --git a/include/openssl/service_indicator.h b/include/openssl/service_indicator.h index ba2f4c449d..5f8f61180f 100644 --- a/include/openssl/service_indicator.h +++ b/include/openssl/service_indicator.h @@ -32,7 +32,7 @@ OPENSSL_EXPORT const char* awslc_version_string(void); enum FIPSStatus { AWSLC_NOT_APPROVED = 0, - AWSLC_APPROVED = 1, + AWSLC_APPROVED = 1 }; #if defined(AWSLC_FIPS) diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index a6aa02caa3..e2659a1b2c 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -166,24 +166,6 @@ struct timeval; extern "C" { #endif -#if defined(__cplusplus) -// enums can be predeclared, but only in C++ and only if given an explicit type. -// C doesn't support setting an explicit type for enums thus a #define is used -// to do this only for C++. However, the ABI type between C and C++ need to have -// equal sizes, which is confirmed in a unittest. -#define BORINGSSL_ENUM_INT : int -enum ssl_early_data_reason_t BORINGSSL_ENUM_INT; -enum ssl_encryption_level_t BORINGSSL_ENUM_INT; -enum ssl_private_key_result_t BORINGSSL_ENUM_INT; -enum ssl_renegotiate_mode_t BORINGSSL_ENUM_INT; -enum ssl_select_cert_result_t BORINGSSL_ENUM_INT; -enum ssl_select_cert_result_t BORINGSSL_ENUM_INT; -enum ssl_ticket_aead_result_t BORINGSSL_ENUM_INT; -enum ssl_verify_result_t BORINGSSL_ENUM_INT; -#else -#define BORINGSSL_ENUM_INT -#endif - // SSL implementation. @@ -1276,8 +1258,7 @@ OPENSSL_EXPORT int SSL_set_chain_and_key( SSL *ssl, CRYPTO_BUFFER *const *certs, size_t num_certs, EVP_PKEY *privkey, const SSL_PRIVATE_KEY_METHOD *privkey_method); - -// SSL_get0_chain returns the list of |CRYPTO_BUFFER|s that were set by +// SSL_CTX_get0_chain returns the list of |CRYPTO_BUFFER|s that were set by // |SSL_set_chain_and_key|, unless they have been discarded. Reference counts // are not incremented by this call. The return value may be |NULL| if no chain // has been set. @@ -1356,7 +1337,7 @@ OPENSSL_EXPORT int SSL_use_PrivateKey_file(SSL *ssl, const char *file, OPENSSL_EXPORT int SSL_CTX_use_certificate_chain_file(SSL_CTX *ctx, const char *file); -// SSL_CTX_use_certificate_chain_file configures certificates for |ssl|. It +// SSL_use_certificate_chain_file configures certificates for |ssl|. It // reads the contents of |file| as a PEM-encoded leaf certificate followed // optionally by the certificate chain to send to the peer. It returns one on // success and zero on failure. @@ -1384,10 +1365,10 @@ OPENSSL_EXPORT void *SSL_CTX_get_default_passwd_cb_userdata(const SSL_CTX *ctx); // Custom private keys. -enum ssl_private_key_result_t BORINGSSL_ENUM_INT { +enum ssl_private_key_result_t { ssl_private_key_success, ssl_private_key_retry, - ssl_private_key_failure, + ssl_private_key_failure }; // ssl_private_key_method_st (aka |SSL_PRIVATE_KEY_METHOD|) describes private @@ -2563,7 +2544,7 @@ OPENSSL_EXPORT int SSL_CTX_set_tlsext_ticket_key_cb( // ssl_ticket_aead_result_t enumerates the possible results from decrypting a // ticket with an |SSL_TICKET_AEAD_METHOD|. -enum ssl_ticket_aead_result_t BORINGSSL_ENUM_INT { +enum ssl_ticket_aead_result_t { // ssl_ticket_aead_success indicates that the ticket was successfully // decrypted. ssl_ticket_aead_success, @@ -2576,7 +2557,7 @@ enum ssl_ticket_aead_result_t BORINGSSL_ENUM_INT { ssl_ticket_aead_ignore_ticket, // ssl_ticket_aead_error indicates that a fatal error occured and the // handshake should be terminated. - ssl_ticket_aead_error, + ssl_ticket_aead_error }; // ssl_ticket_aead_method_st (aka |SSL_TICKET_AEAD_METHOD|) contains methods @@ -2689,23 +2670,30 @@ OPENSSL_EXPORT int SSL_set1_groups_list(SSL *ssl, const char *groups); #define SSL_GROUP_SECP521R1 25 #define SSL_GROUP_X25519 29 +// SSL_GROUP_SECP256R1_KYBER768_DRAFT00 is defined at // https://datatracker.ietf.org/doc/html/draft-kwiatkowski-tls-ecdhe-kyber #define SSL_GROUP_SECP256R1_KYBER768_DRAFT00 0x639A +// SSL_GROUP_X25519_KYBER768_DRAFT00 is defined at // https://datatracker.ietf.org/doc/html/draft-tls-westerbaan-xyber768d00 #define SSL_GROUP_X25519_KYBER768_DRAFT00 0x6399 +// SSL_GROUP_SECP256R1_MLKEM768 is defined at // https://datatracker.ietf.org/doc/html/draft-kwiatkowski-tls-ecdhe-mlkem.html #define SSL_GROUP_SECP256R1_MLKEM768 0x11EB + +// SSL_GROUP_X25519_MLKEM768 is defined at +// https://datatracker.ietf.org/doc/html/draft-kwiatkowski-tls-ecdhe-mlkem.html #define SSL_GROUP_X25519_MLKEM768 0x11EC -// PQ and hybrid group IDs are not yet standardized. Current IDs are driven by -// community consensus and are defined at +// The following PQ and hybrid group IDs are not yet standardized. Current IDs +// are driven by community consensus and are defined at: // https://github.com/open-quantum-safe/oqs-provider/blob/main/oqs-template/oqs-kem-info.md #define SSL_GROUP_KYBER512_R3 0x023A #define SSL_GROUP_KYBER768_R3 0x023C #define SSL_GROUP_KYBER1024_R3 0x023D +// The following are defined at // https://datatracker.ietf.org/doc/html/draft-connolly-tls-mlkem-key-agreement.html #define SSL_GROUP_MLKEM768 0x0768 #define SSL_GROUP_MLKEM1024 0x1024 @@ -2904,10 +2892,10 @@ OPENSSL_EXPORT void SSL_set_verify(SSL *ssl, int mode, int (*callback)(int ok, X509_STORE_CTX *store_ctx)); -enum ssl_verify_result_t BORINGSSL_ENUM_INT { +enum ssl_verify_result_t { ssl_verify_ok, ssl_verify_invalid, - ssl_verify_retry, + ssl_verify_retry }; // SSL_CTX_set_custom_verify configures certificate verification. |mode| is one @@ -3840,11 +3828,11 @@ OPENSSL_EXPORT int SSL_delegated_credential_used(const SSL *ssl); // ssl_encryption_level_t represents a specific QUIC encryption level used to // transmit handshake messages. -enum ssl_encryption_level_t BORINGSSL_ENUM_INT { +enum ssl_encryption_level_t { ssl_encryption_initial = 0, ssl_encryption_early_data, ssl_encryption_handshake, - ssl_encryption_application, + ssl_encryption_application }; // ssl_quic_method_st (aka |SSL_QUIC_METHOD|) describes custom QUIC hooks. @@ -4111,7 +4099,7 @@ OPENSSL_EXPORT int32_t SSL_get_ticket_age_skew(const SSL *ssl); // An ssl_early_data_reason_t describes why 0-RTT was accepted or rejected. // These values are persisted to logs. Entries should not be renumbered and // numeric values should never be reused. -enum ssl_early_data_reason_t BORINGSSL_ENUM_INT { +enum ssl_early_data_reason_t { // The handshake has not progressed far enough for the 0-RTT status to be // known. ssl_early_data_unknown = 0, @@ -4145,7 +4133,7 @@ enum ssl_early_data_reason_t BORINGSSL_ENUM_INT { // The value of the largest entry. ssl_early_data_unsupported_with_custom_extension = 15, ssl_early_data_reason_max_value = - ssl_early_data_unsupported_with_custom_extension, + ssl_early_data_unsupported_with_custom_extension }; // SSL_get_early_data_reason returns details why 0-RTT was accepted or rejected @@ -4658,12 +4646,12 @@ OPENSSL_EXPORT void SSL_CTX_set_current_time_cb( // such as HTTP/1.1, and not others, such as HTTP/2. OPENSSL_EXPORT void SSL_set_shed_handshake_config(SSL *ssl, int enable); -enum ssl_renegotiate_mode_t BORINGSSL_ENUM_INT { +enum ssl_renegotiate_mode_t { ssl_renegotiate_never = 0, ssl_renegotiate_once, ssl_renegotiate_freely, ssl_renegotiate_ignore, - ssl_renegotiate_explicit, + ssl_renegotiate_explicit }; // SSL_set_renegotiate_mode configures how |ssl|, a client, reacts to @@ -4795,7 +4783,7 @@ struct ssl_early_callback_ctx { // ssl_select_cert_result_t enumerates the possible results from selecting a // certificate with |select_certificate_cb|. -enum ssl_select_cert_result_t BORINGSSL_ENUM_INT { +enum ssl_select_cert_result_t { // ssl_select_cert_success indicates that the certificate selection was // successful. ssl_select_cert_success = 1, @@ -4804,7 +4792,7 @@ enum ssl_select_cert_result_t BORINGSSL_ENUM_INT { ssl_select_cert_retry = 0, // ssl_select_cert_error indicates that a fatal error occured and the // handshake should be terminated. - ssl_select_cert_error = -1, + ssl_select_cert_error = -1 }; // SSL_early_callback_ctx_extension_get searches the extensions in diff --git a/tests/ci/cdk/cdk/codebuild/github_ci_linux_x86_omnibus.yaml b/tests/ci/cdk/cdk/codebuild/github_ci_linux_x86_omnibus.yaml index ff2c84a34c..3f4e0cad5d 100644 --- a/tests/ci/cdk/cdk/codebuild/github_ci_linux_x86_omnibus.yaml +++ b/tests/ci/cdk/cdk/codebuild/github_ci_linux_x86_omnibus.yaml @@ -14,6 +14,16 @@ batch: compute-type: BUILD_GENERAL1_SMALL image: 620771051181.dkr.ecr.us-west-2.amazonaws.com/aws-lc-docker-images-linux-x86:ubuntu-20.04_clang-8x_latest + - identifier: c99_cplusplus98_checker # The checker script runs on gcc. + buildspec: ./tests/ci/codebuild/common/run_simple_target.yml + env: + type: LINUX_CONTAINER + privileged-mode: false + compute-type: BUILD_GENERAL1_SMALL + image: 620771051181.dkr.ecr.us-west-2.amazonaws.com/aws-lc-docker-images-linux-x86:ubuntu-22.04_gcc-12x_latest + variables: + AWS_LC_CI_TARGET: "tests/coding_guidelines/c99_cplusplus98_test.sh" + - identifier: ubuntu1604_gcc5x_x86 buildspec: ./tests/ci/codebuild/common/run_simple_target.yml env: diff --git a/tests/ci/codebuild/linux-x86/pre-push.yml b/tests/ci/codebuild/linux-x86/pre-push.yml index 783fd1d332..687077f8ac 100644 --- a/tests/ci/codebuild/linux-x86/pre-push.yml +++ b/tests/ci/codebuild/linux-x86/pre-push.yml @@ -13,5 +13,5 @@ phases: - ./tests/check_objects_and_errors.sh - go run ./tests/check_licenses.go - ./tests/check_generated_src.sh - - ./tests/coding_guidelines/coding_guidelines_test.sh + - ./tests/coding_guidelines/style.sh - (cd util && go run ./doc.go) diff --git a/tests/ci/run_posix_tests.sh b/tests/ci/run_posix_tests.sh index 31a308fe94..9d9121902f 100755 --- a/tests/ci/run_posix_tests.sh +++ b/tests/ci/run_posix_tests.sh @@ -38,16 +38,6 @@ build_and_test -DDISABLE_PERL=ON -DENABLE_DILITHIUM=ON echo "Testing building with AArch64 Data-Independent Timing (DIT) on." build_and_test -DENABLE_DATA_INDEPENDENT_TIMING=ON -DCMAKE_BUILD_TYPE=Release -DENABLE_DILITHIUM=ON -if [[ "${AWSLC_C99_TEST}" == "1" ]]; then - echo "Testing the C99 compatability of AWS-LC headers." - ./tests/coding_guidelines/c99_gcc_test.sh -fi - -if [[ "${AWSLC_CODING_GUIDELINES_TEST}" == "1" ]]; then - echo "Testing that AWS-LC is compliant with the coding guidelines." - source ./tests/coding_guidelines/coding_guidelines_test.sh -fi - # Lightly verify that uncommon build options does not break the build. Fist # define a list of typical build options to verify the special build option with build_options_to_test=("" "-DBUILD_SHARED_LIBS=1" "-DCMAKE_BUILD_TYPE=Release" "-DBUILD_SHARED_LIBS=1 -DCMAKE_BUILD_TYPE=Release" "-DDISABLE_PERL=ON -DDISABLE_GO=ON") diff --git a/tests/coding_guidelines/c99_gcc_test.sh b/tests/coding_guidelines/c99_cplusplus98_test.sh similarity index 59% rename from tests/coding_guidelines/c99_gcc_test.sh rename to tests/coding_guidelines/c99_cplusplus98_test.sh index aff5e2c42d..02b348ab65 100755 --- a/tests/coding_guidelines/c99_gcc_test.sh +++ b/tests/coding_guidelines/c99_cplusplus98_test.sh @@ -24,8 +24,7 @@ INCLUDE_FILES=`ls $INCLUDE_DIR/openssl/*.h | grep -v $INCLUDE_DIR/openssl/arm_ar # some non-ISO practices, but not all — only those for which ISO C requires a # diagnostic, and some others for which diagnostics have been added." # https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html - -${CC} -std=c99 -c -I${INCLUDE_DIR} -include ${INCLUDE_FILES} -Wpedantic -fsyntax-only -Werror +${CC} -std=c99 -c -I${INCLUDE_DIR} $(echo ${INCLUDE_FILES} | sed 's/[^ ]* */-include &/g') -Wpedantic -fsyntax-only -Werror ./tests/compiler_features_tests/builtin_swap_check.c # AWS C SDKs conforms to C99. They set `C_STANDARD 99` which will set the # flag `-std=gnu99` @@ -34,5 +33,12 @@ ${CC} -std=c99 -c -I${INCLUDE_DIR} -include ${INCLUDE_FILES} -Wpedantic -fsyntax # https://cmake.org/cmake/help/latest/prop_tgt/C_STANDARD.html # # the c99 and gnu99 modes are different, so let's test both. - -${CC} -std=gnu99 -c -I${INCLUDE_DIR} -include ${INCLUDE_FILES} -Wpedantic -fsyntax-only -Werror +${CC} -std=gnu99 -c -I${INCLUDE_DIR} $(echo ${INCLUDE_FILES} | sed 's/[^ ]* */-include &/g') -Wpedantic -fsyntax-only -Werror ./tests/compiler_features_tests/builtin_swap_check.c + +# Our SSL headers use C++, but older compilers do not have the C++11 flag enabled by +# default. Not all consuming applications that use older compilers have enabled the +# C++11 feature flag. To ensure a smoother integration process for migrating +# applications, we should ensure that the default settings of older C++ compilers +# work with our header files. +${CXX} -std=c++98 -c -I${INCLUDE_DIR} $(echo ${INCLUDE_FILES} | sed 's/[^ ]* */-include &/g') -Wpedantic -fsyntax-only -Werror ./tests/compiler_features_tests/builtin_swap_check.c +${CXX} -std=gnu++98 -c -I${INCLUDE_DIR} $(echo ${INCLUDE_FILES} | sed 's/[^ ]* */-include &/g') -Wpedantic -fsyntax-only -Werror ./tests/compiler_features_tests/builtin_swap_check.c diff --git a/tests/coding_guidelines/coding_guidelines_test.sh b/tests/coding_guidelines/coding_guidelines_test.sh deleted file mode 100755 index 2be22f9698..0000000000 --- a/tests/coding_guidelines/coding_guidelines_test.sh +++ /dev/null @@ -1,8 +0,0 @@ -#!/usr/bin/env bash -# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -# SPDX-License-Identifier: Apache-2.0 OR ISC - -set -exo pipefail - -./tests/coding_guidelines/style.sh -./tests/coding_guidelines/c99_gcc_test.sh From c8993cf4ea83e2f57660faf8cc130a65132041e2 Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Wed, 19 Feb 2025 13:32:51 -0800 Subject: [PATCH 2/6] Enable RSA keygen becnhmarks by default (#2206) ### Description of changes: The current filter logic in the speed tool is a bit weird. It's not possible to run all the benchmarks because RSA key gen was off by default. If you pass in the filter "RSAKeyGen" you get the keygen benchmarks and all the other RSA benchmarks. This change updates it to be treated the same as all the other benchmarks. The RSA key gen benchmarks are still a little weird because they override the timeout which is why they are slower and take more time. For now the canary use case is fine. ### Testing: The CI will run this benchmark with all the libcryptos we expect to support. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- tool/speed.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tool/speed.cc b/tool/speed.cc index 11264a0e56..3c67795c9d 100644 --- a/tool/speed.cc +++ b/tool/speed.cc @@ -417,8 +417,7 @@ static bool SpeedRSA(const std::string &selected) { } static bool SpeedRSAKeyGen(bool is_fips, const std::string &selected) { - // Don't run this by default because it's so slow. - if (selected != "RSAKeyGen") { + if (!selected.empty() && selected.find("RSAKeyGen") == std::string::npos) { return true; } From a5eff8ed6752c0d8813212ed25bcee86dbbdae74 Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Wed, 19 Feb 2025 13:58:50 -0800 Subject: [PATCH 3/6] Update pairwise consistency test failures to support gracefully continiung (#2201) ### Description of changes: Currently AWS_LC_FIPS_failure always aborts the process and it is impossible to continue after calling that function. In a future change this will be optional and AWS-LC will not abort and the overall API call should return a failure. This change updates the current pairwise consistency test spots to theoretically return a failure. ### Call-outs: This PR does not change the behavior, if there is a PWCT failure the process still aborts. ### Testing: With the current AWS_LC_FIPS_failure implementation it is impossible to test this change. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- crypto/fipsmodule/ec/ec_key.c | 3 +-- crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c | 7 ++++--- crypto/fipsmodule/ml_kem/ml_kem_ref/kem.c | 1 - 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/crypto/fipsmodule/ec/ec_key.c b/crypto/fipsmodule/ec/ec_key.c index 24128f40d5..d7aea95f1f 100644 --- a/crypto/fipsmodule/ec/ec_key.c +++ b/crypto/fipsmodule/ec/ec_key.c @@ -550,9 +550,8 @@ int EC_KEY_generate_key_fips(EC_KEY *eckey) { #if defined(AWSLC_FIPS) AWS_LC_FIPS_failure("EC keygen checks failed"); -#else - return 0; #endif + return 0; } int EC_KEY_get_ex_new_index(long argl, void *argp, CRYPTO_EX_unused *unused, diff --git a/crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c b/crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c index 9722f6490b..c0474c5b33 100644 --- a/crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c +++ b/crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c @@ -51,7 +51,7 @@ static int ml_dsa_keypair_pct(ml_dsa_params *params, * array of CRYPTO_SECRETKEYBYTES bytes) * - const uint8_t *rnd: pointer to random seed * - * Returns 0 (success) + * Returns 0 (success) -1 on failure or abort depending on FIPS mode **************************************************/ int ml_dsa_keypair_internal(ml_dsa_params *params, uint8_t *pk, @@ -114,6 +114,7 @@ int ml_dsa_keypair_internal(ml_dsa_params *params, // Abort in case of PCT failure. if (!ml_dsa_keypair_pct(params, pk, sk)) { AWS_LC_FIPS_failure("ML-DSA keygen PCT failed"); + return -1; } #endif return 0; @@ -138,9 +139,9 @@ int ml_dsa_keypair(ml_dsa_params *params, uint8_t *pk, uint8_t *sk) { if (!RAND_bytes(seed, ML_DSA_SEEDBYTES)) { return -1; } - ml_dsa_keypair_internal(params, pk, sk, seed); + int result = ml_dsa_keypair_internal(params, pk, sk, seed); OPENSSL_cleanse(seed, sizeof(seed)); - return 0; + return result; } /************************************************* diff --git a/crypto/fipsmodule/ml_kem/ml_kem_ref/kem.c b/crypto/fipsmodule/ml_kem/ml_kem_ref/kem.c index 7f74bc8365..3aaf55ef00 100644 --- a/crypto/fipsmodule/ml_kem/ml_kem_ref/kem.c +++ b/crypto/fipsmodule/ml_kem/ml_kem_ref/kem.c @@ -59,7 +59,6 @@ int crypto_kem_keypair_derand(ml_kem_params *params, memcpy(sk+params->secret_key_bytes-KYBER_SYMBYTES, coins+KYBER_SYMBYTES, KYBER_SYMBYTES); #if defined(AWSLC_FIPS) - // Abort in case of PCT failure. if (keygen_pct(params, pk, sk)) { return -1; } From 82a16b62fa5370686ecd543c787cac442039afec Mon Sep 17 00:00:00 2001 From: Sean McGrail <549813+skmcgrail@users.noreply.github.com> Date: Wed, 19 Feb 2025 15:32:37 -0800 Subject: [PATCH 4/6] Simplify IsFlag check logic (#2209) ### Issues: Resolves P194739376 ### Description of changes: There was some logically dead code that was unnecessary and would never be reached. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- tool/args.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tool/args.cc b/tool/args.cc index fdcaf9e5b5..9581430cd9 100644 --- a/tool/args.cc +++ b/tool/args.cc @@ -24,7 +24,7 @@ #include "internal.h" bool IsFlag(const std::string& arg) { - return arg.length() > 1 && (arg[0] == '-' || (arg.length() > 2 && arg[0] == '-' && arg[1] == '-')); + return arg.length() > 1 && arg[0] == '-'; } bool ParseKeyValueArguments(args_map_t &out_args, From adc59f03d8c64e72d46ebb90a9ca4d2dcba2d8fb Mon Sep 17 00:00:00 2001 From: Shubham Mittal <107728331+smittals2@users.noreply.github.com> Date: Wed, 19 Feb 2025 18:11:27 -0800 Subject: [PATCH 5/6] Remove access() call from Snapsafe detection (#2197) ### Description of changes: This change uses stat() instead of access() to check for the existence of the Sysgenid device. This is more accurate to ascertain whether the device exists on a machine. access() could fail for various other reasons such as missing permissions. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- crypto/fipsmodule/rand/snapsafe_detect.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/crypto/fipsmodule/rand/snapsafe_detect.c b/crypto/fipsmodule/rand/snapsafe_detect.c index dee46c3aa1..430445d06c 100644 --- a/crypto/fipsmodule/rand/snapsafe_detect.c +++ b/crypto/fipsmodule/rand/snapsafe_detect.c @@ -9,6 +9,7 @@ #include #include #include +#include #include #include "../delocate.h" @@ -21,28 +22,18 @@ DEFINE_STATIC_ONCE(aws_snapsafe_init) DEFINE_BSS_GET(volatile uint32_t *, sgc_addr) DEFINE_BSS_GET(int, snapsafety_state) -// aws_snapsafe_check_kernel_support returns 1 if the special sysgenid device -// file exists and 0 otherwise. -static int aws_snapsafe_check_kernel_support(void) { - // This file-exist method is generally brittle. But for our purpose, this - // should be more than fine. - if (access(CRYPTO_get_sysgenid_path(), F_OK) != 0) { - return 0; - } - return 1; -} - static void do_aws_snapsafe_init(void) { - *snapsafety_state_bss_get() = SNAPSAFETY_STATE_NOT_SUPPORTED; *sgc_addr_bss_get() = NULL; + *snapsafety_state_bss_get() = SNAPSAFETY_STATE_NOT_SUPPORTED; - if (aws_snapsafe_check_kernel_support() != 1) { + struct stat buff; + if (stat(CRYPTO_get_sysgenid_path(), &buff) != 0) { return; } - *snapsafety_state_bss_get() = SNAPSAFETY_STATE_FAILED_INITIALISE; + *snapsafety_state_bss_get() = SNAPSAFETY_STATE_FAILED_INITIALISE; int fd_sgc = open(CRYPTO_get_sysgenid_path(), O_RDONLY); - if (fd_sgc == -1) { + if (fd_sgc < 0) { return; } From becf5785c131012bb5a64f3da6cdb117ddc0f431 Mon Sep 17 00:00:00 2001 From: Shubham Mittal <107728331+smittals2@users.noreply.github.com> Date: Wed, 19 Feb 2025 18:12:11 -0800 Subject: [PATCH 6/6] Prepare release v1.46.1 (#2210) ### Description of changes: New patch release to pull in snapsafe changes. ### Call-outs: Point out areas that need special attention or support during the review process. Discuss architecture or design changes. ### Testing: How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license. --- crypto/fipsmodule/service_indicator/service_indicator_test.cc | 4 ++-- include/openssl/base.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crypto/fipsmodule/service_indicator/service_indicator_test.cc b/crypto/fipsmodule/service_indicator/service_indicator_test.cc index c9153c1a80..6b18a7e0b5 100644 --- a/crypto/fipsmodule/service_indicator/service_indicator_test.cc +++ b/crypto/fipsmodule/service_indicator/service_indicator_test.cc @@ -5296,7 +5296,7 @@ TEST(ServiceIndicatorTest, ED25519SigGenVerify) { // Since this is running in FIPS mode it should end in FIPS // Update this when the AWS-LC version number is modified TEST(ServiceIndicatorTest, AWSLCVersionString) { - ASSERT_STREQ(awslc_version_string(), "AWS-LC FIPS 1.46.0"); + ASSERT_STREQ(awslc_version_string(), "AWS-LC FIPS 1.46.1"); } #else @@ -5339,6 +5339,6 @@ TEST(ServiceIndicatorTest, BasicTest) { // Since this is not running in FIPS mode it shouldn't end in FIPS // Update this when the AWS-LC version number is modified TEST(ServiceIndicatorTest, AWSLCVersionString) { - ASSERT_STREQ(awslc_version_string(), "AWS-LC 1.46.0"); + ASSERT_STREQ(awslc_version_string(), "AWS-LC 1.46.1"); } #endif // AWSLC_FIPS diff --git a/include/openssl/base.h b/include/openssl/base.h index 8416520aa1..5f39445fcc 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h @@ -122,7 +122,7 @@ extern "C" { // ServiceIndicatorTest.AWSLCVersionString // Note: there are two versions of this test. Only one test is compiled // depending on FIPS mode. -#define AWSLC_VERSION_NUMBER_STRING "1.46.0" +#define AWSLC_VERSION_NUMBER_STRING "1.46.1" #if defined(BORINGSSL_SHARED_LIBRARY)