From 0a4a61597205e7175656ee7d07d8f67a2fa9aada Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Wed, 10 May 2023 12:12:12 -0400 Subject: [PATCH 1/5] Add Openssh integration tests to CI (#942) --- .../github_ci_linux_x86_omnibus.yaml | 8 ++ .../linux-x86/openssh_integration.yml | 9 ++ tests/ci/run_openssh_integration.sh | 117 ++++++++++++++++++ 3 files changed, 134 insertions(+) create mode 100644 tests/ci/codebuild/linux-x86/openssh_integration.yml create mode 100755 tests/ci/run_openssh_integration.sh 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 6f04155c23..2b5b400574 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 @@ -353,6 +353,14 @@ batch: compute-type: BUILD_GENERAL1_LARGE image: 620771051181.dkr.ecr.us-west-2.amazonaws.com/aws-lc-docker-images-linux-x86:ubuntu-20.04_clang-9x_latest + - identifier: openssh_integration + buildspec: ./tests/ci/codebuild/linux-x86/openssh_integration.yml + env: + type: LINUX_CONTAINER + privileged-mode: false + compute-type: BUILD_GENERAL1_LARGE + image: 620771051181.dkr.ecr.us-west-2.amazonaws.com/aws-lc-docker-images-linux-x86:amazonlinux-2023_clang-15x_sanitizer_latest + - identifier: postgres_integration buildspec: ./tests/ci/codebuild/linux-x86/postgres_integration.yml env: diff --git a/tests/ci/codebuild/linux-x86/openssh_integration.yml b/tests/ci/codebuild/linux-x86/openssh_integration.yml new file mode 100644 index 0000000000..84e29f216d --- /dev/null +++ b/tests/ci/codebuild/linux-x86/openssh_integration.yml @@ -0,0 +1,9 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 OR ISC + +version: 0.2 + +phases: + build: + commands: + - ./tests/ci/run_openssh_integration.sh diff --git a/tests/ci/run_openssh_integration.sh b/tests/ci/run_openssh_integration.sh new file mode 100755 index 0000000000..623e1b0542 --- /dev/null +++ b/tests/ci/run_openssh_integration.sh @@ -0,0 +1,117 @@ +#!/bin/bash -exu +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 OR ISC + +source tests/ci/common_posix_setup.sh + +# Set up environment. + +# ROOT +# | +# - AWS_LC_DIR +# | +# - aws-lc +# | +# - SCRATCH_FOLDER +# | +# - openssh-portable +# - AWS_LC_BUILD_FOLDER +# - AWS_LC_INSTALL_FOLDER +# - OPENSSH_INSTALL_FOLDER + +# Assumes script is executed from the root of aws-lc directory +AWS_LC_DIR=$(pwd) +pushd .. +ROOT=$(pwd) +popd + +SCRATCH_FOLDER="${ROOT}/SCRATCH_AWSLC_OPENSSH_INTERN_TEST" +AWS_LC_BUILD_FOLDER="${SCRATCH_FOLDER}/aws-lc-build" +AWS_LC_INSTALL_FOLDER="${SCRATCH_FOLDER}/aws-lc-install" +OPENSSH_WORKSPACE_FOLDER="${SCRATCH_FOLDER}/openssh-portable" +OPENSSH_INSTALL_FOLDER="${SCRATCH_FOLDER}/openssh-install" + +NINJA_COMMAND=ninja +if ! ${NINJA_COMMAND} --version; then + NINJA_COMMAND=ninja-build +fi + +# Make script execution idempotent. +rm -rf "${SCRATCH_FOLDER}" +mkdir -p "${SCRATCH_FOLDER}" +pushd "${SCRATCH_FOLDER}" + +# Test helper functions. + +function aws_lc_build() { + export GOPROXY=direct + ${CMAKE_COMMAND} "${AWS_LC_DIR}" -GNinja "-B${AWS_LC_BUILD_FOLDER}" "-DCMAKE_INSTALL_PREFIX=${AWS_LC_INSTALL_FOLDER}" "$@" + ${NINJA_COMMAND} -C "${AWS_LC_BUILD_FOLDER}" install + ls -R "${AWS_LC_INSTALL_FOLDER}" + rm -rf "${AWS_LC_BUILD_FOLDER:?}"/* +} + +function install_aws_lc() { + AWS_LC_LIB_FOLDER=$(readlink -f "${AWS_LC_INSTALL_FOLDER}"/lib*) + # This installs AWS-LC as the "libcrypto" for the system + echo "${AWS_LC_LIB_FOLDER}" > /etc/ld.so.conf.d/aws-lc.conf + rm -f /etc/ld.so.cache + ldconfig +} + +function openssh_build() { + pushd "${OPENSSH_WORKSPACE_FOLDER}" + autoreconf + # The RSA_meth_XXX functions are not implemented by AWS-LC, and the implementation provided by OpenSSH also doesn't compile for us. + # Fortunately, these functions are only needed for pkcs11 support, which is disabled for our build. + # See: https://github.com/openssh/openssh-portable/pull/385 + export CFLAGS="-DAWS_LC_INTERNAL_IGNORE_BN_SET_FLAGS=1 -DHAVE_RSA_METH_FREE=1 -DHAVE_RSA_METH_DUP=1 -DHAVE_RSA_METH_SET1_NAME=1 -DHAVE_RSA_METH_SET_PRIV_ENC=1 -DHAVE_RSA_METH_SET_PRIV_DEC=1" + ./configure --with-ssl-dir="${AWS_LC_INSTALL_FOLDER}" --prefix="${OPENSSH_INSTALL_FOLDER}" --disable-pkcs11 + make install + ls -R "${OPENSSH_INSTALL_FOLDER}" + popd +} + +function checkout_openssh_branch() { + pushd "${OPENSSH_WORKSPACE_FOLDER}" + make clean + git clean -f -d + git checkout --track origin/"$1" + popd +} + +function openssh_run_tests() { + pushd "${OPENSSH_WORKSPACE_FOLDER}" + if ! id -u sshd; then + useradd sshd + fi + export TEST_SSH_UNSAFE_PERMISSIONS=1 + export SKIP_LTESTS="$@" + make tests + popd +} + +mkdir -p "${AWS_LC_BUILD_FOLDER}" "${AWS_LC_INSTALL_FOLDER}" "${OPENSSH_INSTALL_FOLDER}" + +# Get latest OpenSSH version. +git clone https://github.com/openssh/openssh-portable.git +ls + +# Buld AWS-LC as a shared library +aws_lc_build -DBUILD_SHARED_LIBS=1 +install_aws_lc + +CODEBUILD_SKIPPED_TESTS="agent-subprocess forwarding multiplex forward-control agent-restrict connection-timeout" + +# Using default branch. Build openssh and run tests. +openssh_build +openssh_run_tests "${CODEBUILD_SKIPPED_TESTS}" + +# Using branch V_8_9 +checkout_openssh_branch V_8_9 +openssh_build +# In v8.9, the "percent" test requires the 'openssl' cli command +openssh_run_tests "percent ${CODEBUILD_SKIPPED_TESTS}" + +popd + From be1de131ed52f292d2a3827febb7b41973152d65 Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Wed, 10 May 2023 11:40:16 -0700 Subject: [PATCH 2/5] add support for X509_get0_pubkey (#1000) mySQL consumes the X509_get0_pubkey API when building with OpenSSL1.1.1 and uses the X509_get_pubkey when built with OpenSSL1.0.2. X509_get0_pubkey seems to have been added in OpenSSL1.1.1 and is very similar to X509_get_pubkey (which exists in AWS-LC). X509_get_pubkey attempts to decode the public key for certificate x. If successful it returns the public key as an EVP_PKEY pointer with its reference count incremented: this means the returned key must be freed up after use. X509_get0_pubkey is similar except it does not increment the reference count of the returned EVP_PKEY so it must not be freed up after use. --- crypto/x509/internal.h | 5 ++++ crypto/x509/x509_cmp.c | 7 +++++ crypto/x509/x509_test.cc | 17 ++++++++++++ crypto/x509/x_pubkey.c | 58 +++++++++++++++++++++++++--------------- include/openssl/x509.h | 7 +++++ 5 files changed, 73 insertions(+), 21 deletions(-) diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index 8056917730..b610c57239 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -365,6 +365,11 @@ ASN1_TYPE *ASN1_generate_v3(const char *str, const X509V3_CTX *cnf); int X509_CERT_AUX_print(BIO *bp, X509_CERT_AUX *x, int indent); +// X509_PUBKEY_get0 decodes the public key in |key| and returns an |EVP_PKEY| +// on success, or NULL on error. It is similar to |X509_PUBKEY_get|, but it +// directly returns the reference to |pkey| of |key|. This means that the +// caller must not free the result after use. +EVP_PKEY *X509_PUBKEY_get0(X509_PUBKEY *key); // RSA-PSS functions. diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c index b696b9491e..52d6ac33ae 100644 --- a/crypto/x509/x509_cmp.c +++ b/crypto/x509/x509_cmp.c @@ -233,6 +233,13 @@ X509 *X509_find_by_subject(const STACK_OF(X509) *sk, X509_NAME *name) { return NULL; } +EVP_PKEY *X509_get0_pubkey(const X509 *x) { + if ((x == NULL) || (x->cert_info == NULL)) { + return NULL; + } + return (X509_PUBKEY_get0(x->cert_info->key)); +} + EVP_PKEY *X509_get_pubkey(X509 *x) { if ((x == NULL) || (x->cert_info == NULL)) { return NULL; diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 91499b7aca..38e3c23d4d 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -6173,3 +6173,20 @@ TEST(X509Test, SortRDN) { 0x02, 0x41, 0x42}; EXPECT_EQ(Bytes(kExpected), Bytes(der, der_len)); } + +TEST(X509Test, TestDecode) { + bssl::UniquePtr cert(CertFromPEM(kExamplePSSCert)); + ASSERT_TRUE(cert); + + // |X509_get0_pubkey| directly returns a reference to the decoded |pkey|, so + // it can't be freed. + EVP_PKEY *pkey1 = X509_get0_pubkey(cert.get()); + ASSERT_TRUE(pkey1); + ASSERT_TRUE(X509_verify(cert.get(), pkey1)); + + // |X509_get_pubkey| returns the decoded |pkey| with its reference count + // updated, so we must free it. + bssl::UniquePtr pkey2(X509_get_pubkey(cert.get())); + ASSERT_TRUE(pkey2); + ASSERT_TRUE(X509_verify(cert.get(), pkey2.get())); +} diff --git a/crypto/x509/x_pubkey.c b/crypto/x509/x_pubkey.c index 6a6a9750cf..659053d928 100644 --- a/crypto/x509/x_pubkey.c +++ b/crypto/x509/x_pubkey.c @@ -130,22 +130,18 @@ int X509_PUBKEY_set(X509_PUBKEY **x, EVP_PKEY *pkey) { // not. static struct CRYPTO_STATIC_MUTEX g_pubkey_lock = CRYPTO_STATIC_MUTEX_INIT; -EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key) { - EVP_PKEY *ret = NULL; - uint8_t *spki = NULL; - - if (key == NULL) { - goto error; - } - +// x509_pubkey_decode decodes |key| into |pkey|. It returns one on success and +// zero on error. +static int x509_pubkey_decode(EVP_PKEY **pkey, X509_PUBKEY *key) { CRYPTO_STATIC_MUTEX_lock_read(&g_pubkey_lock); if (key->pkey != NULL) { CRYPTO_STATIC_MUTEX_unlock_read(&g_pubkey_lock); - EVP_PKEY_up_ref(key->pkey); - return key->pkey; + *pkey = key->pkey; + return 1; } CRYPTO_STATIC_MUTEX_unlock_read(&g_pubkey_lock); + uint8_t *spki = NULL; // Re-encode the |X509_PUBKEY| to DER and parse it. int spki_len = i2d_X509_PUBKEY(key, &spki); if (spki_len < 0) { @@ -153,8 +149,8 @@ EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key) { } CBS cbs; CBS_init(&cbs, spki, (size_t)spki_len); - ret = EVP_parse_public_key(&cbs); - if (ret == NULL || CBS_len(&cbs) != 0) { + *pkey = EVP_parse_public_key(&cbs); + if (*pkey == NULL || CBS_len(&cbs) != 0) { OPENSSL_PUT_ERROR(X509, X509_R_PUBLIC_KEY_DECODE_ERROR); goto error; } @@ -163,21 +159,41 @@ EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key) { CRYPTO_STATIC_MUTEX_lock_write(&g_pubkey_lock); if (key->pkey) { CRYPTO_STATIC_MUTEX_unlock_write(&g_pubkey_lock); - EVP_PKEY_free(ret); - ret = key->pkey; + EVP_PKEY_free(*pkey); + *pkey = key->pkey; } else { - key->pkey = ret; + key->pkey = *pkey; CRYPTO_STATIC_MUTEX_unlock_write(&g_pubkey_lock); } - OPENSSL_free(spki); - EVP_PKEY_up_ref(ret); - return ret; - + return 1; error: OPENSSL_free(spki); - EVP_PKEY_free(ret); - return NULL; + return 0; +} + +EVP_PKEY *X509_PUBKEY_get0(X509_PUBKEY *key) { + if (key == NULL) { + OPENSSL_PUT_ERROR(X509, ERR_R_PASSED_NULL_PARAMETER); + return NULL; + } + + EVP_PKEY *ret = NULL; + if (!x509_pubkey_decode(&ret, key)) { + EVP_PKEY_free(ret); + return NULL; + }; + return ret; +} + +EVP_PKEY *X509_PUBKEY_get(X509_PUBKEY *key) { + EVP_PKEY *ret = X509_PUBKEY_get0(key); + + if (ret != NULL && !EVP_PKEY_up_ref(ret)) { + OPENSSL_PUT_ERROR(X509, ERR_R_INTERNAL_ERROR); + ret = NULL; + } + return ret; } int X509_PUBKEY_set0_param(X509_PUBKEY *pub, ASN1_OBJECT *obj, int param_type, diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 67ff61ac41..d7f1571907 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -193,6 +193,13 @@ OPENSSL_EXPORT X509_NAME *X509_get_subject_name(const X509 *x509); // object. OPENSSL_EXPORT X509_PUBKEY *X509_get_X509_PUBKEY(const X509 *x509); +// X509_get0_pubkey returns |x509|'s public key as an |EVP_PKEY|, or NULL if the +// public key was unsupported or could not be decoded. It is similar to +// |X509_get_pubkey|, but it does not increment the reference count of the +// returned |EVP_PKEY|. This means that the caller must not free the result after +// use. +OPENSSL_EXPORT EVP_PKEY *X509_get0_pubkey(const X509 *x); + // X509_get_pubkey returns |x509|'s public key as an |EVP_PKEY|, or NULL if the // public key was unsupported or could not be decoded. This function returns a // reference to the |EVP_PKEY|. The caller must release the result with From 26befb9f25941605d1fefb3d8efe98ffbed826c6 Mon Sep 17 00:00:00 2001 From: Sean McGrail <549813+skmcgrail@users.noreply.github.com> Date: Thu, 11 May 2023 15:29:22 -0700 Subject: [PATCH 3/5] Fix incorrect lambda CodeBuild policy permission (#1001) --- tests/ci/cdk/cdk.context.json | 2 +- tests/ci/cdk/cdk/components.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/ci/cdk/cdk.context.json b/tests/ci/cdk/cdk.context.json index f6f26c2608..a409f9dd7b 100644 --- a/tests/ci/cdk/cdk.context.json +++ b/tests/ci/cdk/cdk.context.json @@ -2,7 +2,7 @@ "acknowledged-issue-numbers": [ 19836 ], - "availability-zones:account=431229050501:region=us-west-2": [ + "availability-zones:account=620771051181:region=us-west-2": [ "us-west-2a", "us-west-2b", "us-west-2c", diff --git a/tests/ci/cdk/cdk/components.py b/tests/ci/cdk/cdk/components.py index 39b2b5b3e1..58881bd495 100644 --- a/tests/ci/cdk/cdk/components.py +++ b/tests/ci/cdk/cdk/components.py @@ -36,7 +36,7 @@ def __init__(self, scope: Construct, id: str, *, project: codebuild.IProject) -> actions=[ "codebuild:BatchGetBuildBatches", "codebuild:ListBuildBatchesForProject", - "codebuild:StopBuild", + "codebuild:StopBuildBatch", ], resources=[project.project_arn])) From 24cb055cb48ae15b9b924ec939f4b993380723f7 Mon Sep 17 00:00:00 2001 From: Lily Barrowman Date: Fri, 12 May 2023 09:23:16 -0700 Subject: [PATCH 4/5] Check HWCAP_CPUID before reading MIDR_EL1 on aarch64 (#1006) Valgrind does not support emulating the MIDR_EL1 aarch64 system register, and will crash if an attempt is made to read it. The fix is to simply check HWCAP_CPUID before reading MIDR_EL1. --------- Co-authored-by: Lily Barrowman --- crypto/fipsmodule/cpucap/cpu_aarch64_linux.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/crypto/fipsmodule/cpucap/cpu_aarch64_linux.c b/crypto/fipsmodule/cpucap/cpu_aarch64_linux.c index ef93ff06a8..b30249e294 100644 --- a/crypto/fipsmodule/cpucap/cpu_aarch64_linux.c +++ b/crypto/fipsmodule/cpucap/cpu_aarch64_linux.c @@ -97,6 +97,7 @@ void OPENSSL_cpuid_setup(void) { static const unsigned long kSHA256 = 1 << 6; static const unsigned long kSHA512 = 1 << 21; static const unsigned long kSHA3 = 1 << 17; + static const unsigned long kCPUID = 1 << 11; uint64_t OPENSSL_arm_midr = 0; @@ -127,11 +128,16 @@ void OPENSSL_cpuid_setup(void) { OPENSSL_armcap_P |= ARMV8_SHA3; } - // Check if the CPU model is Neoverse V1, - // which has a wide crypto/SIMD pipeline. - OPENSSL_arm_midr = armv8_cpuid_probe(); - if (MIDR_IS_CPU_MODEL(OPENSSL_arm_midr, ARM_CPU_IMP_ARM, ARM_CPU_PART_V1)) { - OPENSSL_armcap_P |= ARMV8_NEOVERSE_V1; + // Before calling armv8_cpuid_probe and reading from MIDR_EL1 check that it + // is supported. As of Valgrind 3.21 trying to read from that register will + // cause Valgrind to crash. + if (hwcap & kCPUID) { + // Check if the CPU model is Neoverse V1, + // which has a wide crypto/SIMD pipeline. + OPENSSL_arm_midr = armv8_cpuid_probe(); + if (MIDR_IS_CPU_MODEL(OPENSSL_arm_midr, ARM_CPU_IMP_ARM, ARM_CPU_PART_V1)) { + OPENSSL_armcap_P |= ARMV8_NEOVERSE_V1; + } } // OPENSSL_armcap is a 32-bit, unsigned value which may start with "0x" to From a7577d2326da58525ebd6033f42392779c0746b2 Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Mon, 15 May 2023 13:04:18 -0400 Subject: [PATCH 5/5] Only build acvp/modulewrapper with BUILD_TESTING (#1012) --- CMakeLists.txt | 3 ++- tests/ci/run_fips_tests.sh | 3 +++ util/fipstools/CMakeLists.txt | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 8e3ed51ce8..846c25ca40 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -827,6 +827,8 @@ if(BUILD_TESTING) set(MEM_TEST_EXEC mem_test) set(MEM_SET_TEST_EXEC mem_set_test) endif() + + add_subdirectory(util/fipstools/acvp/modulewrapper) endif() add_subdirectory(crypto) @@ -835,7 +837,6 @@ if(BUILD_LIBSSL) add_subdirectory(tool) endif() add_subdirectory(util/fipstools) -add_subdirectory(util/fipstools/acvp/modulewrapper) if(FUZZ) if(LIBFUZZER_FROM_DEPS) diff --git a/tests/ci/run_fips_tests.sh b/tests/ci/run_fips_tests.sh index f5e512059f..901661e3c5 100755 --- a/tests/ci/run_fips_tests.sh +++ b/tests/ci/run_fips_tests.sh @@ -11,6 +11,9 @@ fips_build_and_test -DCMAKE_BUILD_TYPE=Release -DBUILD_SHARED_LIBS=1 if [[ ("$(uname -s)" == 'Linux'*) && (("$(uname -p)" == 'x86_64'*) || ("$(uname -p)" == 'aarch64'*)) ]]; then echo "Testing AWS-LC static library in FIPS Release mode." fips_build_and_test -DCMAKE_BUILD_TYPE=Release + + # These build parameters may be needed by our aws-lc-fips-sys Rust package + run_build -DFIPS=1 -DBUILD_LIBSSL=OFF -DBUILD_TESTING=OFF fi # The AL2 version of Clang does not have all of the required artifacts for address sanitizer, see P45594051 diff --git a/util/fipstools/CMakeLists.txt b/util/fipstools/CMakeLists.txt index 87abf0ab1c..ace581a2dc 100644 --- a/util/fipstools/CMakeLists.txt +++ b/util/fipstools/CMakeLists.txt @@ -1,4 +1,4 @@ -if(FIPS) +if(FIPS AND BUILD_TESTING) add_executable( test_fips