From 3324473943d14bc97ffb450e8e146f8ae42a488f Mon Sep 17 00:00:00 2001 From: Nevine Ebeid <66388554+nebeid@users.noreply.github.com> Date: Fri, 31 May 2024 16:38:23 -0400 Subject: [PATCH 01/27] Cleanse the right amount of bytes in HMAC. (#1613) EVP_MAX_MD_BLOCK_SIZE is the block size in bytes. This commit partially reverts "Zeroize data immediately after use for FIPS (#911)", commit c7a9fd0dd20c0e35a5b7b98f22b78f16c8c34567. Prior to it, EVP_MAX_MD_BLOCK_SIZE was divided by 8 in a 64-bit word array initialisation in hmac.c --- crypto/fipsmodule/digest/internal.h | 1 - crypto/fipsmodule/hmac/hmac.c | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/crypto/fipsmodule/digest/internal.h b/crypto/fipsmodule/digest/internal.h index 7b93754f5a..148e467077 100644 --- a/crypto/fipsmodule/digest/internal.h +++ b/crypto/fipsmodule/digest/internal.h @@ -63,7 +63,6 @@ extern "C" { #endif -#define EVP_MAX_MD_BLOCK_SIZE_BYTES (EVP_MAX_MD_BLOCK_SIZE / 8) struct env_md_st { // type contains a NID identifing the digest function. (For example, diff --git a/crypto/fipsmodule/hmac/hmac.c b/crypto/fipsmodule/hmac/hmac.c index 00edf495c9..0e576c026a 100644 --- a/crypto/fipsmodule/hmac/hmac.c +++ b/crypto/fipsmodule/hmac/hmac.c @@ -289,8 +289,8 @@ int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, size_t key_len, FIPS_service_indicator_lock_state(); int result = 0; - uint64_t pad[EVP_MAX_MD_BLOCK_SIZE_BYTES] = {0}; - uint64_t key_block[EVP_MAX_MD_BLOCK_SIZE_BYTES] = {0}; + uint64_t pad[EVP_MAX_MD_BLOCK_SIZE / sizeof(uint64_t)] = {0}; + uint64_t key_block[EVP_MAX_MD_BLOCK_SIZE / sizeof(uint64_t)] = {0}; if (block_size < key_len) { // Long keys are hashed. if (!methods->init(&ctx->md_ctx) || @@ -322,8 +322,8 @@ int HMAC_Init_ex(HMAC_CTX *ctx, const void *key, size_t key_len, result = 1; end: - OPENSSL_cleanse(pad, EVP_MAX_MD_BLOCK_SIZE_BYTES); - OPENSSL_cleanse(key_block, EVP_MAX_MD_BLOCK_SIZE_BYTES); + OPENSSL_cleanse(pad, EVP_MAX_MD_BLOCK_SIZE); + OPENSSL_cleanse(key_block, EVP_MAX_MD_BLOCK_SIZE); FIPS_service_indicator_unlock_state(); if (result != 1) { // We're in some error state, so return our context to a known and well defined zero state. From 94e91d96d311404a0eaaca6039bfd7e6577c6dff Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Fri, 31 May 2024 16:38:41 -0400 Subject: [PATCH 02/27] Pin aws-lc-rs integ to nightly-2024-05-22 (#1612) ### Description of changes: * Pin to a working Rust nightly toolchain. 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. --- .github/workflows/aws-lc-rs.yml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/aws-lc-rs.yml b/.github/workflows/aws-lc-rs.yml index ac68f8df2a..51f18012c0 100644 --- a/.github/workflows/aws-lc-rs.yml +++ b/.github/workflows/aws-lc-rs.yml @@ -10,6 +10,7 @@ concurrency: env: GOPROXY: https://proxy.golang.org,direct AWS_LC_SYS_CMAKE_BUILDER: 1 + RUST_NIGHTLY_TOOLCHAIN: nightly-2024-05-22 jobs: standard: if: github.repository_owner == 'aws' @@ -20,11 +21,11 @@ jobs: repository: awslabs/aws-lc-rs path: ./aws-lc-rs submodules: false - - uses: actions-rs/toolchain@v1 + - uses: dtolnay/rust-toolchain@master with: # Our aws-lc-sys generation scripts require nightly. - toolchain: nightly - override: true + toolchain: ${{ env.RUST_NIGHTLY_TOOLCHAIN }} + - run: rustup override set $RUST_NIGHTLY_TOOLCHAIN - uses: actions-rs/cargo@v1 with: command: install From a8ed881cbf918cdbdb2a97805242d274898f6754 Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Mon, 3 Jun 2024 13:22:18 -0400 Subject: [PATCH 03/27] Fix NTP integ test (#1616) ### Issues: Resolves #P132890351 ### Description of changes: * Fix NTP integration test 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. --- .../0001-Fix-MD5-and-Shake128-usage.patch | 74 +++++++++++++++++++ tests/ci/integration/ntp_patch/digests.patch | 11 --- tests/ci/integration/run_ntp_integration.sh | 2 +- 3 files changed, 75 insertions(+), 12 deletions(-) create mode 100644 tests/ci/integration/ntp_patch/0001-Fix-MD5-and-Shake128-usage.patch delete mode 100644 tests/ci/integration/ntp_patch/digests.patch diff --git a/tests/ci/integration/ntp_patch/0001-Fix-MD5-and-Shake128-usage.patch b/tests/ci/integration/ntp_patch/0001-Fix-MD5-and-Shake128-usage.patch new file mode 100644 index 0000000000..820e8cde62 --- /dev/null +++ b/tests/ci/integration/ntp_patch/0001-Fix-MD5-and-Shake128-usage.patch @@ -0,0 +1,74 @@ +From 96ed539aad785b12756cd8513309eff631d39951 Mon Sep 17 00:00:00 2001 +From: Justin Smith +Date: Mon, 3 Jun 2024 06:59:44 -0400 +Subject: [PATCH] Fix MD5 and Shake128 usage + +--- + include/ntp_md5.h | 7 ++++++- + sntp/crypto.c | 19 ++++++++++++++----- + 2 files changed, 20 insertions(+), 6 deletions(-) + +diff --git a/include/ntp_md5.h b/include/ntp_md5.h +index 22caff3..29a4235 100644 +--- a/include/ntp_md5.h ++++ b/include/ntp_md5.h +@@ -9,13 +9,18 @@ + /* Use the system MD5 or fall back on libisc's */ + # if defined HAVE_MD5_H && defined HAVE_MD5INIT + # include +-# else ++# elif !defined(OPENSSL) + # include "isc/md5.h" + typedef isc_md5_t MD5_CTX; + # define MD5_DIGEST_LENGTH ISC_MD5_DIGESTLENGTH + # define MD5Init(c) isc_md5_init(c) + # define MD5Update(c, p, s) isc_md5_update(c, (const void *)p, s) + # define MD5Final(d, c) isc_md5_final((c), (d)) /* swapped */ ++# else ++#include ++# define MD5Init(c) MD5_Init(c) ++# define MD5Update(c, p, s) MD5_Update(c, p, s) ++# define MD5Final(d, c) MD5_Final((d), (c)) + # endif + + # define KEY_TYPE_MD5 NID_md5 +diff --git a/sntp/crypto.c b/sntp/crypto.c +index 1be2ea3..ea3f7e0 100644 +--- a/sntp/crypto.c ++++ b/sntp/crypto.c +@@ -10,6 +10,7 @@ + #include "crypto.h" + #include + #include "isc/string.h" ++#include "openssl/md5.h" + + struct key *key_ptr; + size_t key_cnt = 0; +@@ -101,11 +102,19 @@ compute_mac( + macname); + goto mac_fail; + } +- if (!EVP_DigestFinal(ctx, digest, &len)) { +- msyslog(LOG_ERR, "make_mac: MAC %s Digest Final failed.", +- macname); +- len = 0; +- } ++ if (EVP_MD_flags(ctx->digest) & EVP_MD_FLAG_XOF) { ++ // The callers expect the hash to always contain 16 bytes ++ len = MD5_DIGEST_LENGTH; ++ if (!EVP_DigestFinalXOF(ctx, digest, len)) { ++ msyslog(LOG_ERR, "make_mac: MAC %s Digest Final failed.", macname); ++ len = 0; ++ } ++ } else { ++ if (!EVP_DigestFinal(ctx, digest, &len)) { ++ msyslog(LOG_ERR, "make_mac: MAC %s Digest Final failed.", macname); ++ len = 0; ++ } ++ } + #else /* !OPENSSL */ + (void)key_type; /* unused, so try to prevent compiler from croaks */ + if (!EVP_DigestInit(ctx, EVP_get_digestbynid(key_type))) { +-- +2.39.3 (Apple Git-145) + diff --git a/tests/ci/integration/ntp_patch/digests.patch b/tests/ci/integration/ntp_patch/digests.patch deleted file mode 100644 index a0d71403f6..0000000000 --- a/tests/ci/integration/ntp_patch/digests.patch +++ /dev/null @@ -1,11 +0,0 @@ ---- a/tests/libntp/digests.c -+++ b/tests/libntp/digests.c -@@ -238,7 +238,7 @@ - void test_Digest_MDC2(void); - void test_Digest_MDC2(void) - { --#ifdef OPENSSL -+#if defined(OPENSSL) && !defined(OPENSSL_NO_MDC2) - u_char expectedA[MAX_MAC_LEN] = - { - 0, 0, 0, KEYID_A, diff --git a/tests/ci/integration/run_ntp_integration.sh b/tests/ci/integration/run_ntp_integration.sh index 4b7c11ed68..eb0b9f2857 100755 --- a/tests/ci/integration/run_ntp_integration.sh +++ b/tests/ci/integration/run_ntp_integration.sh @@ -16,7 +16,7 @@ source tests/ci/common_posix_setup.sh # - AWS_LC_INSTALL_FOLDER # Assumes script is executed from the root of aws-lc directory -SCRATCH_FOLDER="${SRC_ROOT}/NTP_BUILD_ROOT" +SCRATCH_FOLDER="${SRC_ROOT}/../NTP_BUILD_ROOT" NTP_WEBSITE_URL="https://downloads.nwtime.org/ntp/" # - curl fetches the HTML content of the website, From bb4082699ed2f0d68d08d76b134a08265963c96b Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Mon, 3 Jun 2024 16:15:32 -0400 Subject: [PATCH 04/27] Remove special aarch64 valgrind logic (#1618) ### Issues: Resolves #P132890850 ### Description of changes: * Fix Valgrind test. The constants to configure static CPU capabilities on aarch64 are no longer needed. ### Testing: * Tested on Ubuntu 24.04 and Amazon Linux 2023 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. --- tests/ci/common_posix_setup.sh | 40 ++++------------------------------ 1 file changed, 4 insertions(+), 36 deletions(-) diff --git a/tests/ci/common_posix_setup.sh b/tests/ci/common_posix_setup.sh index c80ddbb9e4..5b7ce7f7a6 100644 --- a/tests/ci/common_posix_setup.sh +++ b/tests/ci/common_posix_setup.sh @@ -29,22 +29,6 @@ if [[ "${KERNEL_NAME}" == "Darwin" || "${KERNEL_NAME}" =~ .*BSD ]]; then else # Assume KERNEL_NAME is Linux. NUM_CPU_THREADS=$(grep -c ^processor /proc/cpuinfo) - if [[ $PLATFORM == "aarch64" ]]; then - CPU_PART=$(grep -Po -m 1 'CPU part.*:\s\K.*' /proc/cpuinfo) - NUM_CPU_PART=$(grep -c $CPU_PART /proc/cpuinfo) - # Set capabilities via the static flags for valgrind tests. - # This is because valgrind reports the instruction - # mrs %0, MIDR_EL1 - # which fetches the CPU part number, as illegal. - # For some reason, valgrind also reports SHA512 instructions illegal, - # so the SHA512 capability is not included below. - VALGRIND_STATIC_CAP_FLAGS="-DOPENSSL_STATIC_ARMCAP -DOPENSSL_STATIC_ARMCAP_NEON" - VALGRIND_STATIC_CAP_FLAGS+=" -DOPENSSL_STATIC_ARMCAP_AES -DOPENSSL_STATIC_ARMCAP_PMULL " - VALGRIND_STATIC_CAP_FLAGS+=" -DOPENSSL_STATIC_ARMCAP_SHA1 -DOPENSSL_STATIC_ARMCAP_SHA256 " - if [[ $NUM_CPU_PART == $NUM_CPU_THREADS ]] && [[ ${CPU_PART} =~ 0x[dD]40 ]]; then - VALGRIND_STATIC_CAP_FLAGS+=" -DOPENSSL_STATIC_ARMCAP_SHA3 -DOPENSSL_STATIC_ARMCAP_NEOVERSE_V1" - fi - fi fi # Pick cmake3 if possible. We don't know of any OS that installs a cmake3 @@ -160,31 +144,15 @@ function fips_build_and_test { } function build_and_test_valgrind { - if [[ $PLATFORM == "aarch64" ]]; then - run_build "$@" -DCMAKE_C_FLAGS="$VALGRIND_STATIC_CAP_FLAGS" - run_cmake_custom_target 'run_tests_valgrind' - - # Disable all capabilities and run again - # (We don't use the env. variable OPENSSL_armcap because it is currently - # restricted to the case of runtime discovery of capabilities - # in cpu_aarch64_linux.c) - run_build "$@" -DCMAKE_C_FLAGS="-DOPENSSL_STATIC_ARMCAP" - run_cmake_custom_target 'run_tests_valgrind' - else - run_build "$@" - run_cmake_custom_target 'run_tests_valgrind' - fi + run_build "$@" + run_cmake_custom_target 'run_tests_valgrind' } function build_and_test_ssl_runner_valgrind { export AWS_LC_GO_TEST_TIMEOUT="60m" - if [[ $PLATFORM == "aarch64" ]]; then - run_build "$@" -DCMAKE_C_FLAGS="$VALGRIND_STATIC_CAP_FLAGS" - else - run_build "$@" - fi - run_cmake_custom_target 'run_ssl_runner_tests_valgrind' + run_build "$@" + run_cmake_custom_target 'run_ssl_runner_tests_valgrind' } function build_and_test_with_sde { From 8258d73d5e56d764456663af10c414c118436071 Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Mon, 3 Jun 2024 13:22:35 -0700 Subject: [PATCH 05/27] add back ASN1_dup with tests (#1591) `ASN1_dup` was removed in 419144a in favor of `ASN1_Item_dup`. This shouldn't normally be called directly, but Ruby happens to consume the API in several instances. I've optimized the function to allocate memory with `i2d` directly, instead of the ancient OpenSSL allocate and pass into behavior. Also added some tests for verification along with a do-not-use warning. --- crypto/asn1/a_dup.c | 20 +++++++++++++ crypto/asn1/asn1_test.cc | 64 ++++++++++++++++++++++++++++++++++++++++ include/openssl/asn1.h | 15 ++++++++-- 3 files changed, 96 insertions(+), 3 deletions(-) diff --git a/crypto/asn1/a_dup.c b/crypto/asn1/a_dup.c index b37a5c61b9..d052a44a8c 100644 --- a/crypto/asn1/a_dup.c +++ b/crypto/asn1/a_dup.c @@ -59,6 +59,26 @@ #include #include +void *ASN1_dup(i2d_of_void *i2d, d2i_of_void *d2i, void *input) { + if (i2d == NULL || d2i == NULL || input == NULL) { + OPENSSL_PUT_ERROR(ASN1, ERR_R_PASSED_NULL_PARAMETER); + return NULL; + } + + // Size and allocate |buf|. + unsigned char *buf = NULL; + int buf_len = i2d(input, &buf); + if (buf == NULL || buf_len < 0) { + return NULL; + } + + // |buf| needs to be converted to |const| to be passed in. + const unsigned char *temp_input = buf; + char *ret = d2i(NULL, &temp_input, buf_len); + OPENSSL_free(buf); + return ret; +} + // ASN1_ITEM version of dup: this follows the model above except we don't // need to allocate the buffer. At some point this could be rewritten to // directly dup the underlying structure instead of doing and encode and diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 1e0f453cc1..c8cea4613c 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -2412,6 +2412,70 @@ TEST(ASN1Test, LargeString) { #endif } + +// Wrapper functions are needed to get around Control Flow Integrity Sanitizers. +static int i2d_ASN1_TYPE_void(const void *a, unsigned char **out) { + return i2d_ASN1_TYPE((ASN1_TYPE *)a, out); +} +static void *d2i_ASN1_TYPE_void(void **a, const unsigned char **in, long len) { + return d2i_ASN1_TYPE((ASN1_TYPE **)a, in, len); +} +static int i2d_ECPrivateKey_void(const void *a, unsigned char **out) { + return i2d_ECPrivateKey((EC_KEY *)a, out); +} +static void *d2i_ECPrivateKey_void(void **a, const unsigned char **in, long len) { + return d2i_ECPrivateKey((EC_KEY **)a, in, len); +} +static int i2d_X509_PUBKEY_void(const void *a, unsigned char **out) { + return i2d_X509_PUBKEY((X509_PUBKEY *)a, out); +} +static void *d2i_X509_PUBKEY_void(void **a, const unsigned char **in, long len) { + return d2i_X509_PUBKEY((X509_PUBKEY **)a, in, len); +} + +TEST(ASN1Test, ASN1Dup) { + const uint8_t *tag = kTag128; + bssl::UniquePtr asn1( + d2i_ASN1_TYPE(nullptr, &tag, sizeof(kTag128))); + ASSERT_TRUE(asn1); + EXPECT_EQ(128, asn1->type); + bssl::UniquePtr asn1_copy((ASN1_TYPE *)ASN1_dup( + i2d_ASN1_TYPE_void, d2i_ASN1_TYPE_void, asn1.get())); + ASSERT_TRUE(asn1_copy); + EXPECT_EQ(ASN1_TYPE_cmp(asn1.get(), asn1_copy.get()), 0); + + bssl::UniquePtr key(EC_KEY_new_by_curve_name(NID_X9_62_prime256v1)); + ASSERT_TRUE(key); + ASSERT_TRUE(EC_KEY_generate_key(key.get())); + bssl::UniquePtr key_copy((EC_KEY *)ASN1_dup( + i2d_ECPrivateKey_void, d2i_ECPrivateKey_void, key.get())); + ASSERT_TRUE(key_copy); + EXPECT_EQ(BN_cmp(EC_KEY_get0_private_key(key.get()), + EC_KEY_get0_private_key(key_copy.get())), + 0); + EXPECT_EQ(EC_GROUP_cmp(EC_KEY_get0_group(key.get()), + EC_KEY_get0_group(key_copy.get()), nullptr), + 0); + EXPECT_EQ(EC_POINT_cmp(EC_KEY_get0_group(key_copy.get()), + EC_KEY_get0_public_key(key.get()), + EC_KEY_get0_public_key(key_copy.get()), nullptr), + 0); + + bssl::UniquePtr evp_pkey(EVP_PKEY_new()); + X509_PUBKEY *tmp_key = nullptr; + ASSERT_TRUE(evp_pkey); + ASSERT_TRUE(EVP_PKEY_set1_EC_KEY(evp_pkey.get(), key.get())); + ASSERT_TRUE(X509_PUBKEY_set(&tmp_key, evp_pkey.get())); + bssl::UniquePtr x509_pubkey(tmp_key); + bssl::UniquePtr x509_pubkey_copy((X509_PUBKEY *)ASN1_dup( + i2d_X509_PUBKEY_void, d2i_X509_PUBKEY_void, x509_pubkey.get())); + ASSERT_TRUE(x509_pubkey_copy); + EXPECT_EQ( + ASN1_STRING_cmp(X509_PUBKEY_get0_public_key(x509_pubkey.get()), + X509_PUBKEY_get0_public_key(x509_pubkey_copy.get())), + 0); +} + // The ASN.1 macros do not work on Windows shared library builds, where usage of // |OPENSSL_EXPORT| is a bit stricter. #if !defined(OPENSSL_WINDOWS) || !defined(BORINGSSL_SHARED_LIBRARY) diff --git a/include/openssl/asn1.h b/include/openssl/asn1.h index 581b87369b..69bb666a0d 100644 --- a/include/openssl/asn1.h +++ b/include/openssl/asn1.h @@ -275,8 +275,7 @@ int i2d_SAMPLE(const SAMPLE *in, uint8_t **outp); // CHECKED_I2D_OF casts a given pointer to i2d_of_void* and statically checks // that it was a pointer to |type|'s |i2d| function. -#define CHECKED_I2D_OF(type, i2d) \ - ((i2d_of_void*) (1 ? i2d : ((I2D_OF(type))0))) +#define CHECKED_I2D_OF(type, i2d) ((i2d_of_void *)(1 ? i2d : ((I2D_OF(type))0))) // The following typedefs are sometimes used for pointers to functions like // |d2i_SAMPLE| and |i2d_SAMPLE|. Note, however, that these act on |void*|. @@ -391,6 +390,16 @@ OPENSSL_EXPORT ASN1_VALUE *ASN1_item_d2i(ASN1_VALUE **out, OPENSSL_EXPORT int ASN1_item_i2d(ASN1_VALUE *val, unsigned char **outp, const ASN1_ITEM *it); +// ASN1_dup returns a newly-allocated copy of |x| by re-encoding with |i2d| and +// |d2i|. |i2d| and |d2i| must be the corresponding type functions of |x|. NULL +// is returned on error. +// +// WARNING: DO NOT USE. Casting the result of this function to the wrong type, +// or passing a pointer of the wrong type into this function, are potentially +// exploitable memory errors. Prefer directly calling |i2d| and |d2i| or other +// type-specific functions. +OPENSSL_EXPORT void *ASN1_dup(i2d_of_void *i2d, d2i_of_void *d2i, void *x); + // ASN1_item_dup returns a newly-allocated copy of |x|, or NULL on error. |x| // must be an object of |it|'s C type. // @@ -443,7 +452,7 @@ OPENSSL_EXPORT int ASN1_i2d_bio(i2d_of_void *i2d, BIO *out, void *in); // forces the user to use undefined C behavior and will cause failures when // running against undefined behavior sanitizers in clang. #define ASN1_i2d_bio_of(type, i2d, out, in) \ - (ASN1_i2d_bio(CHECKED_I2D_OF(type, i2d), out, CHECKED_PTR_OF(type, in))) + (ASN1_i2d_bio(CHECKED_I2D_OF(type, i2d), out, CHECKED_PTR_OF(type, in))) // ASN1_item_unpack parses |oct|'s contents as |it|'s ASN.1 type. It returns a // newly-allocated instance of |it|'s C type on success, or NULL on error. From a98f017dd829929135d146b43de839df1eee00de Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 6 Dec 2023 22:18:48 -0500 Subject: [PATCH 06/27] Fuzz more extension parsers in the cert parser If we're going to rewrite the parsers later, let's cover them more thoroughly. Change-Id: Iab4bbb886da5e42caf4a6eff77cfedca8a33f085 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64629 Reviewed-by: Bob Beck Commit-Queue: David Benjamin Auto-Submit: David Benjamin (cherry picked from commit 811de7adb2b1742fe10ebcaf3b8dda66301c3cc1) --- fuzz/cert.cc | 50 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 9 deletions(-) diff --git a/fuzz/cert.cc b/fuzz/cert.cc index e433450c34..2f4a54702c 100644 --- a/fuzz/cert.cc +++ b/fuzz/cert.cc @@ -19,24 +19,56 @@ #include "../crypto/x509/internal.h" extern "C" int LLVMFuzzerTestOneInput(const uint8_t *buf, size_t len) { - X509 *x509 = d2i_X509(NULL, &buf, len); - if (x509 != NULL) { + bssl::UniquePtr x509(d2i_X509(nullptr, &buf, len)); + if (x509 != nullptr) { // Extract the public key. - EVP_PKEY_free(X509_get_pubkey(x509)); + EVP_PKEY_free(X509_get_pubkey(x509.get())); // Fuzz some deferred parsing. - x509v3_cache_extensions(x509); + x509v3_cache_extensions(x509.get()); - // Reserialize the structure. - uint8_t *der = NULL; - i2d_X509(x509, &der); + // Fuzz every supported extension. + for (int i = 0; i < X509_get_ext_count(x509.get()); i++) { + const X509_EXTENSION *ext = X509_get_ext(x509.get(), i); + void *parsed = X509V3_EXT_d2i(ext); + if (parsed != nullptr) { + int nid = OBJ_obj2nid(X509_EXTENSION_get_object(ext)); + BSSL_CHECK(nid != NID_undef); + + // Reserialize the extension. This should succeed if we were able to + // parse it. + // TODO(crbug.com/boringssl/352): Ideally we would also assert that + // |new_ext| is identical to |ext|, but our parser is not strict enough. + bssl::UniquePtr new_ext( + X509V3_EXT_i2d(nid, X509_EXTENSION_get_critical(ext), parsed)); + BSSL_CHECK(new_ext != nullptr); + + // This can only fail if |ext| was not a supported type, but then + // |X509V3_EXT_d2i| should have failed. + BSSL_CHECK(X509V3_EXT_free(nid, parsed)); + } + } + + // Reserialize |x509|. This should succeed if we were able to parse it. + // TODO(crbug.com/boringssl/352): Ideally we would also assert the output + // matches the input, but our parser is not strict enough. + uint8_t *der = nullptr; + int der_len = i2d_X509(x509.get(), &der); + BSSL_CHECK(der_len > 0); + OPENSSL_free(der); + + // Reserialize |x509|'s TBSCertificate without reusing the cached encoding. + // TODO(crbug.com/boringssl/352): Ideally we would also assert the output + // matches the input TBSCertificate, but our parser is not strict enough. + der = nullptr; + der_len = i2d_re_X509_tbs(x509.get(), &der); + BSSL_CHECK(der_len > 0); OPENSSL_free(der); BIO *bio = BIO_new(BIO_s_mem()); - X509_print(bio, x509); + X509_print(bio, x509.get()); BIO_free(bio); } - X509_free(x509); ERR_clear_error(); return 0; } From 5aefe649cb511916457f8635011980bb5ee5ca8f Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 6 Dec 2023 22:33:21 -0500 Subject: [PATCH 07/27] Document functions that export verification internals It is a little silly to have documented obscure verification internals and not verification itself yet, but these were pretty easy. X509_check_purpose and X509_check_trust should also go here, but I'll do those when I've tackled more of X509_PURPOSE and X509_TRUST. I haven't moved X509_CHECK_FLAG_* because those should go with X509_VERIFY_PARAM_set_set_hostflags. X509_check_akid seems to have no callers, so I unexported that one. Bug: 426 Change-Id: I5af1824346db27fd52773ae27e943df89c1d5a87 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64630 Auto-Submit: David Benjamin Commit-Queue: Bob Beck Reviewed-by: Bob Beck (cherry picked from commit 9e40481d5bfe08e3e6daa5335c2cf076e1187ec1) --- crypto/x509/internal.h | 4 + crypto/x509/v3_purp.c | 10 +-- crypto/x509/v3_utl.c | 22 ++++-- include/openssl/ssl.h | 25 +++--- include/openssl/x509.h | 173 +++++++++++++++++++++++++++++++---------- 5 files changed, 164 insertions(+), 70 deletions(-) diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index 7830d0872a..cc963005cf 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -567,6 +567,10 @@ GENERAL_NAMES *v2i_GENERAL_NAMES(const X509V3_EXT_METHOD *method, const X509V3_CTX *ctx, const STACK_OF(CONF_VALUE) *nval); +// TODO(https://crbug.com/boringssl/407): Make |issuer| const once the +// |X509_NAME| issue is resolved. +int X509_check_akid(X509 *issuer, const AUTHORITY_KEYID *akid); + #if defined(__cplusplus) } // extern C diff --git a/crypto/x509/v3_purp.c b/crypto/x509/v3_purp.c index 4eb4ee830d..eaf9ff9479 100644 --- a/crypto/x509/v3_purp.c +++ b/crypto/x509/v3_purp.c @@ -597,14 +597,6 @@ static int check_purpose_timestamp_sign(const X509_PURPOSE *xp, const X509 *x, static int no_check(const X509_PURPOSE *xp, const X509 *x, int ca) { return 1; } -// Various checks to see if one certificate issued the second. This can be -// used to prune a set of possible issuer certificates which have been looked -// up using some simple method such as by subject name. These are: 1. Check -// issuer_name(subject) == subject_name(issuer) 2. If akid(subject) exists -// check it matches issuer 3. If key_usage(issuer) exists check it supports -// certificate signing returns 0 for OK, positive for reason for mismatch, -// reasons match codes for X509_verify_cert() - int X509_check_issued(X509 *issuer, X509 *subject) { if (X509_NAME_cmp(X509_get_subject_name(issuer), X509_get_issuer_name(subject))) { @@ -627,7 +619,7 @@ int X509_check_issued(X509 *issuer, X509 *subject) { return X509_V_OK; } -int X509_check_akid(X509 *issuer, AUTHORITY_KEYID *akid) { +int X509_check_akid(X509 *issuer, const AUTHORITY_KEYID *akid) { if (!akid) { return X509_V_OK; } diff --git a/crypto/x509/v3_utl.c b/crypto/x509/v3_utl.c index 7428014dff..47f412993e 100644 --- a/crypto/x509/v3_utl.c +++ b/crypto/x509/v3_utl.c @@ -942,6 +942,9 @@ static int do_check_string(const ASN1_STRING *a, int cmp_type, equal_fn equal, } if (rv > 0 && peername) { *peername = OPENSSL_strndup((char *)a->data, a->length); + if (*peername == NULL) { + return -1; + } } } else { int astrlen; @@ -960,13 +963,16 @@ static int do_check_string(const ASN1_STRING *a, int cmp_type, equal_fn equal, } if (rv > 0 && peername) { *peername = OPENSSL_strndup((char *)astr, astrlen); + if (*peername == NULL) { + return -1; + } } OPENSSL_free(astr); } return rv; } -static int do_x509_check(X509 *x, const char *chk, size_t chklen, +static int do_x509_check(const X509 *x, const char *chk, size_t chklen, unsigned int flags, int check_type, char **peername) { int cnid = NID_undef; int alt_type; @@ -1033,8 +1039,8 @@ static int do_x509_check(X509 *x, const char *chk, size_t chklen, return 0; } -int X509_check_host(X509 *x, const char *chk, size_t chklen, unsigned int flags, - char **peername) { +int X509_check_host(const X509 *x, const char *chk, size_t chklen, + unsigned int flags, char **peername) { if (chk == NULL) { return -2; } @@ -1050,7 +1056,7 @@ int X509_check_host(X509 *x, const char *chk, size_t chklen, unsigned int flags, return do_x509_check(x, chk, chklen, flags, GEN_DNS, peername); } -int X509_check_email(X509 *x, const char *chk, size_t chklen, +int X509_check_email(const X509 *x, const char *chk, size_t chklen, unsigned int flags) { if (chk == NULL) { return -2; @@ -1067,15 +1073,15 @@ int X509_check_email(X509 *x, const char *chk, size_t chklen, return do_x509_check(x, chk, chklen, flags, GEN_EMAIL, NULL); } -int X509_check_ip(X509 *x, const unsigned char *chk, size_t chklen, +int X509_check_ip(const X509 *x, const unsigned char *chk, size_t chklen, unsigned int flags) { if (chk == NULL) { return -2; } - return do_x509_check(x, (char *)chk, chklen, flags, GEN_IPADD, NULL); + return do_x509_check(x, (const char *)chk, chklen, flags, GEN_IPADD, NULL); } -int X509_check_ip_asc(X509 *x, const char *ipasc, unsigned int flags) { +int X509_check_ip_asc(const X509 *x, const char *ipasc, unsigned int flags) { unsigned char ipout[16]; size_t iplen; @@ -1086,7 +1092,7 @@ int X509_check_ip_asc(X509 *x, const char *ipasc, unsigned int flags) { if (iplen == 0) { return -2; } - return do_x509_check(x, (char *)ipout, iplen, flags, GEN_IPADD, NULL); + return do_x509_check(x, (const char *)ipout, iplen, flags, GEN_IPADD, NULL); } // Convert IP addresses both IPv4 and IPv6 into an OCTET STRING compatible diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index e85894e46d..b4dbbd3889 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2915,8 +2915,23 @@ OPENSSL_EXPORT int (*SSL_get_verify_callback(const SSL *ssl))( // ineffective. Simply checking that a host has some certificate from a CA is // rarely meaningful—you have to check that the CA believed that the host was // who you expect to be talking to. +// +// By default, both subject alternative names and the subject's common name +// attribute are checked. The latter has long been deprecated, so callers should +// call |SSL_set_hostflags| with |X509_CHECK_FLAG_NEVER_CHECK_SUBJECT| to use +// the standard behavior. https://crbug.com/boringssl/464 tracks fixing the +// default. OPENSSL_EXPORT int SSL_set1_host(SSL *ssl, const char *hostname); +// SSL_set_hostflags calls |X509_VERIFY_PARAM_set_hostflags| on the +// |X509_VERIFY_PARAM| associated with this |SSL*|. |flags| should be some +// combination of the |X509_CHECK_*| constants. +// +// |X509_V_FLAG_X509_STRICT| is always ON by default and +// |X509_V_FLAG_ALLOW_PROXY_CERTS| is always OFF. Both are non-configurable. +// See |x509.h| for more details. +OPENSSL_EXPORT void SSL_set_hostflags(SSL *ssl, unsigned flags); + // SSL_CTX_set_verify_depth sets the maximum depth of a certificate chain // accepted in verification. This number does not include the leaf, so a depth // of 1 allows the leaf and one CA certificate. @@ -3093,16 +3108,6 @@ OPENSSL_EXPORT int SSL_set_verify_algorithm_prefs(SSL *ssl, const uint16_t *prefs, size_t num_prefs); -// SSL_set_hostflags calls |X509_VERIFY_PARAM_set_hostflags| on the -// |X509_VERIFY_PARAM| associated with this |SSL*|. The |flags| argument -// should be one of the |X509_CHECK_*| constants. -// -// |X509_V_FLAG_X509_STRICT| is always ON by default and -// |X509_V_FLAG_ALLOW_PROXY_CERTS| is always OFF. Both are non-configurable. -// See |x509.h| for more details. -OPENSSL_EXPORT void SSL_set_hostflags(SSL *ssl, unsigned flags); - - // Client certificate CA list. // // When requesting a client certificate, a server may advertise a list of diff --git a/include/openssl/x509.h b/include/openssl/x509.h index c47d624869..852c1dc78d 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -3041,6 +3041,119 @@ OPENSSL_EXPORT int ASN1_item_sign_ctx(const ASN1_ITEM *it, X509_ALGOR *algor1, EVP_MD_CTX *ctx); +// Verification internals. +// +// The following functions expose portions of certificate validation. They are +// exported for compatibility with existing callers, or to support some obscure +// use cases. Most callers, however, will not need these functions and should +// instead use |X509_STORE_CTX| APIs. + +// X509_supported_extension returns one if |ex| is a critical X.509 certificate +// extension, supported by |X509_verify_cert|, and zero otherwise. +// +// Note this function only reports certificate extensions (as opposed to CRL or +// CRL extensions), and only extensions that are expected to be marked critical. +// Additionally, |X509_verify_cert| checks for unsupported critical extensions +// internally, so most callers will not need to call this function separately. +OPENSSL_EXPORT int X509_supported_extension(const X509_EXTENSION *ex); + +// X509_check_ca returns one if |x509| may be considered a CA certificate, +// according to basic constraints and key usage extensions. Otherwise, it +// returns zero. If |x509| is an X509v1 certificate, and thus has no extensions, +// it is considered eligible. +// +// This function returning one does not indicate that |x509| is trusted, only +// that it is eligible to be a CA. +// +// TODO(crbug.com/boringssl/407): |x509| should be const. +OPENSSL_EXPORT int X509_check_ca(X509 *x509); + +// X509_check_issued checks if |issuer| and |subject|'s name, authority key +// identifier, and key usage fields allow |issuer| to have issued |subject|. It +// returns |X509_V_OK| on success and an |X509_V_ERR_*| value otherwise. +// +// This function does not check the signature on |subject|. Rather, it is +// intended to prune the set of possible issuer certificates during +// path-building. +// +// TODO(crbug.com/boringssl/407): Both parameters should be const. +OPENSSL_EXPORT int X509_check_issued(X509 *issuer, X509 *subject); + +// NAME_CONSTRAINTS_check checks if |x509| satisfies name constraints in |nc|. +// It returns |X509_V_OK| on success and some |X509_V_ERR_*| constant on error. +// +// TODO(crbug.com/boringssl/407): Both parameters should be const. +OPENSSL_EXPORT int NAME_CONSTRAINTS_check(X509 *x509, NAME_CONSTRAINTS *nc); + +// X509_check_host checks if |x509| matches the DNS name |chk|. It returns one +// on match, zero on mismatch, or a negative number on error. |flags| should be +// some combination of |X509_CHECK_FLAG_*| and modifies the behavior. On match, +// if |out_peername| is non-NULL, it additionally sets |*out_peername| to a +// newly-allocated, NUL-terminated string containing the DNS name or wildcard in +// the certificate which matched. The caller must then free |*out_peername| with +// |OPENSSL_free| when done. +// +// By default, both subject alternative names and the subject's common name +// attribute are checked. The latter has long been deprecated, so callers should +// include |X509_CHECK_FLAG_NEVER_CHECK_SUBJECT| in |flags| to use the standard +// behavior. https://crbug.com/boringssl/464 tracks fixing the default. +// +// This function does not check if |x509| is a trusted certificate, only if, +// were it trusted, it would match |chk|. +// +// WARNING: This function differs from the usual calling convention and may +// return either 0 or a negative number on error. +// +// TODO(davidben): Make the error case also return zero. +OPENSSL_EXPORT int X509_check_host(const X509 *x509, const char *chk, + size_t chklen, unsigned int flags, + char **out_peername); + +// X509_check_email checks if |x509| matches the email address |chk|. It returns +// one on match, zero on mismatch, or a negative number on error. |flags| should +// be some combination of |X509_CHECK_FLAG_*| and modifies the behavior. +// +// By default, both subject alternative names and the subject's email address +// attribute are checked. The |X509_CHECK_FLAG_NEVER_CHECK_SUBJECT| flag may be +// used to change this behavior. +// +// This function does not check if |x509| is a trusted certificate, only if, +// were it trusted, it would match |chk|. +// +// WARNING: This function differs from the usual calling convention and may +// return either 0 or a negative number on error. +// +// TODO(davidben): Make the error case also return zero. +OPENSSL_EXPORT int X509_check_email(const X509 *x509, const char *chk, + size_t chklen, unsigned int flags); + +// X509_check_ip checks if |x509| matches the IP address |chk|. The IP address +// is represented in byte form and should be 4 bytes for an IPv4 address and 16 +// bytes for an IPv6 address. It returns one on match, zero on mismatch, or a +// negative number on error. |flags| should be some combination of +// |X509_CHECK_FLAG_*| and modifies the behavior. +// +// This function does not check if |x509| is a trusted certificate, only if, +// were it trusted, it would match |chk|. +// +// WARNING: This function differs from the usual calling convention and may +// return either 0 or a negative number on error. +// +// TODO(davidben): Make the error case also return zero. +OPENSSL_EXPORT int X509_check_ip(const X509 *x509, const uint8_t *chk, + size_t chklen, unsigned int flags); + +// X509_check_ip_asc behaves like |X509_check_ip| except the IP address is +// specified in textual form in |ipasc|. +// +// WARNING: This function differs from the usual calling convention and may +// return either 0 or a negative number on error. +// +// TODO(davidben): Make the error case also return zero. +OPENSSL_EXPORT int X509_check_ip_asc(const X509 *x509, const char *ipasc, + unsigned int flags); + + // X.509 information. // // |X509_INFO| is the return type for |PEM_X509_INFO_read_bio|, defined in @@ -4068,11 +4181,17 @@ OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_policies( // |namelen| should be set to the length of |name|. It may be zero if |name| is // NUL-terminated, but this is only maintained for backwards compatibility with // OpenSSL. +// +// By default, both subject alternative names and the subject's common name +// attribute are checked. The latter has long been deprecated, so callers should +// call |X509_VERIFY_PARAM_set_hostflags| with +// |X509_CHECK_FLAG_NEVER_CHECK_SUBJECT| to use the standard behavior. +// https://crbug.com/boringssl/464 tracks fixing the default. OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_host(X509_VERIFY_PARAM *param, const char *name, size_t namelen); -// X509_VERIFY_PARAM_add1_host |name| to the list of names checked by +// X509_VERIFY_PARAM_add1_host adds |name| to the list of names checked by // |param|. If any configured DNS name matches the certificate, verification // succeeds. Any previous names set via |X509_VERIFY_PARAM_set1_host| or // |X509_VERIFY_PARAM_add1_host| are retained, no change is made if |name| is @@ -4081,6 +4200,12 @@ OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_host(X509_VERIFY_PARAM *param, // |namelen| should be set to the length of |name|. It may be zero if |name| is // NUL-terminated, but this is only maintained for backwards compatibility with // OpenSSL. +// +// By default, both subject alternative names and the subject's common name +// attribute are checked. The latter has long been deprecated, so callers should +// call |X509_VERIFY_PARAM_set_hostflags| with +// |X509_CHECK_FLAG_NEVER_CHECK_SUBJECT| to use the standard behavior. +// https://crbug.com/boringssl/464 tracks fixing the default. OPENSSL_EXPORT int X509_VERIFY_PARAM_add1_host(X509_VERIFY_PARAM *param, const char *name, size_t name_len); @@ -4095,6 +4220,10 @@ OPENSSL_EXPORT void X509_VERIFY_PARAM_set_hostflags(X509_VERIFY_PARAM *param, // |emaillen| should be set to the length of |email|. It may be zero if |email| // is NUL-terminated, but this is only maintained for backwards compatibility // with OpenSSL. +// +// By default, both subject alternative names and the subject's email address +// attribute are checked. The |X509_CHECK_FLAG_NEVER_CHECK_SUBJECT| flag may be +// used to change this behavior. OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_email(X509_VERIFY_PARAM *param, const char *email, size_t emaillen); @@ -4371,8 +4500,6 @@ DECLARE_ASN1_FUNCTIONS(ISSUING_DIST_POINT) OPENSSL_EXPORT int DIST_POINT_set_dpname(DIST_POINT_NAME *dpn, X509_NAME *iname); -OPENSSL_EXPORT int NAME_CONSTRAINTS_check(X509 *x, NAME_CONSTRAINTS *nc); - // TODO(https://crbug.com/boringssl/407): This is not const because it contains // an |X509_NAME|. DECLARE_ASN1_FUNCTIONS(ACCESS_DESCRIPTION) @@ -4561,21 +4688,9 @@ OPENSSL_EXPORT X509_EXTENSION *X509V3_EXT_i2d(int ext_nid, int crit, OPENSSL_EXPORT int X509V3_add1_i2d(STACK_OF(X509_EXTENSION) **x, int nid, void *value, int crit, unsigned long flags); -OPENSSL_EXPORT int X509_check_ca(X509 *x); OPENSSL_EXPORT int X509_check_purpose(X509 *x, int id, int ca); -// X509_supported_extension returns one if |ex| is a critical X.509 certificate -// extension, supported by |X509_verify_cert|, and zero otherwise. -// -// Note this function only reports certificate extensions (as opposed to CRL or -// CRL extensions), and only extensions that are expected to be marked critical. -// Additionally, |X509_verify_cert| checks for unsupported critical extensions -// internally, so most callers will not need to call this function separately. -OPENSSL_EXPORT int X509_supported_extension(const X509_EXTENSION *ex); - OPENSSL_EXPORT int X509_PURPOSE_set(int *p, int purpose); -OPENSSL_EXPORT int X509_check_issued(X509 *issuer, X509 *subject); -OPENSSL_EXPORT int X509_check_akid(X509 *issuer, AUTHORITY_KEYID *akid); OPENSSL_EXPORT int X509_PURPOSE_get_count(void); OPENSSL_EXPORT const X509_PURPOSE *X509_PURPOSE_get0(int idx); @@ -4625,34 +4740,6 @@ OPENSSL_EXPORT int X509_PURPOSE_get_id(const X509_PURPOSE *); // sub-domains. #define X509_CHECK_FLAG_SINGLE_LABEL_SUBDOMAINS 0 -// X509_check_host checks if |x| has a Common Name or Subject Alternate name -// that matches the |chk| string up to |chklen|. If |chklen| is 0 -// X509_check_host will calculate the length using strlen. It is encouraged to -// always pass in the length of |chk| and rely on higher level parsing to ensure -// strlen is not called on a string that does not contain a null terminator. -// If a match is found X509_check_host returns 1, if |peername| is not null -// it is updated to point to the matching name in |x|. -OPENSSL_EXPORT int X509_check_host(X509 *x, const char *chk, size_t chklen, - unsigned int flags, char **peername); - -// X509_check_email checks if the email address in |x| matches |chk| string up -// to |chklen|. If |chklen| is 0 X509_check_email will calculate the length -// using strlen. It is encouraged to always pass in the length of |chk| and rely -// on higher level parsing to ensure strlen is not called on a string that does -// not contain a null terminator. If the certificate matches X509_check_email -// returns 1. -OPENSSL_EXPORT int X509_check_email(X509 *x, const char *chk, size_t chklen, - unsigned int flags); - -// X509_check_ip checks if the IPv4 or IPv6 address in |x| matches |chk| up -// to |chklen|. X509_check_ip does not attempt to determine the length of |chk| -// if 0 is passed in for |chklen|. If the certificate matches X509_check_ip -// returns 1. -OPENSSL_EXPORT int X509_check_ip(X509 *x, const unsigned char *chk, - size_t chklen, unsigned int flags); -OPENSSL_EXPORT int X509_check_ip_asc(X509 *x, const char *ipasc, - unsigned int flags); - #if defined(__cplusplus) } // extern C From b012876f1eb17e4189a3af646fdd8564175b4297 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 6 Dec 2023 23:07:11 -0500 Subject: [PATCH 08/27] Document and fix up name hashing functions These return 32-bit hashes, so they should return a platform-independent uint32_t. I've categorized X509_issuer_name_hash and friends under "convenience" functions. X509_NAME_hash and X509_NAME_hash_old are as yet unclassified. Since the hash function is only relevant to X509_LOOKUP_hash_dir, I'm thinking I'll put them with that, once that's organized. While I'm here, simplify the implementations of these functions. The hash operation itself can be made infallible and allocation-free easily. However the function itself is still fallible (and non-const, and not thread-safe) due to the cached encoding mess. X509Test.NameHash captures existing hash values, so we'd notice if this changed the output. Update-Note: This is source-compatible for C/C++, including with -Wconversion, but some bindings need a patch in cl/588632028 to be compatible. Bug: 426 Change-Id: I9bfd3f1093ab15c44d8cb2d81d53aeb3d6e49fc9 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64647 Commit-Queue: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit c77d0f8c776c89195d4095d51f2eaebb621887a6) --- crypto/x509/by_dir.c | 10 ++++--- crypto/x509/internal.h | 1 - crypto/x509/x509_cmp.c | 57 ++++++++++++++++------------------------ crypto/x509/x509_test.cc | 4 +-- include/openssl/x509.h | 40 ++++++++++++++++++++++------ 5 files changed, 62 insertions(+), 50 deletions(-) diff --git a/crypto/x509/by_dir.c b/crypto/x509/by_dir.c index 3807f1f80b..28bdc70271 100644 --- a/crypto/x509/by_dir.c +++ b/crypto/x509/by_dir.c @@ -54,6 +54,7 @@ * copied and put under another distribution licence * [including the GNU Public Licence.] */ +#include #include #include #include @@ -68,7 +69,7 @@ #include "internal.h" typedef struct lookup_dir_hashes_st { - unsigned long hash; + uint32_t hash; int suffix; } BY_DIR_HASH; @@ -246,8 +247,8 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name, int ok = 0; size_t i; int j, k; - unsigned long h; - unsigned long hash_array[2]; + uint32_t h; + uint32_t hash_array[2]; int hash_index; BUF_MEM *b = NULL; X509_OBJECT stmp, *tmp; @@ -309,7 +310,8 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name, hent = NULL; } for (;;) { - snprintf(b->data, b->max, "%s/%08lx.%s%d", ent->dir, h, postfix, k); + snprintf(b->data, b->max, "%s/%08" PRIx32 ".%s%d", ent->dir, h, postfix, + k); #ifndef OPENSSL_NO_POSIX_IO #if defined(_WIN32) && !defined(stat) #define stat _stat diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index cc963005cf..7e3cb21faf 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -96,7 +96,6 @@ struct X509_name_st { STACK_OF(X509_NAME_ENTRY) *entries; int modified; // true if 'bytes' needs to be built BUF_MEM *bytes; - // unsigned long hash; Keep the hash around for lookups unsigned char *canon_enc; int canon_enclen; } /* X509_NAME */; diff --git a/crypto/x509/x509_cmp.c b/crypto/x509/x509_cmp.c index d598d6f98b..ac24a13d11 100644 --- a/crypto/x509/x509_cmp.c +++ b/crypto/x509/x509_cmp.c @@ -60,7 +60,9 @@ #include #include #include +#include #include +#include #include #include @@ -88,11 +90,11 @@ X509_NAME *X509_get_issuer_name(const X509 *a) { return a->cert_info->issuer; } -unsigned long X509_issuer_name_hash(X509 *x) { - return (X509_NAME_hash(x->cert_info->issuer)); +uint32_t X509_issuer_name_hash(X509 *x) { + return X509_NAME_hash(x->cert_info->issuer); } -unsigned long X509_issuer_name_hash_old(X509 *x) { +uint32_t X509_issuer_name_hash_old(X509 *x) { return (X509_NAME_hash_old(x->cert_info->issuer)); } @@ -108,12 +110,12 @@ const ASN1_INTEGER *X509_get0_serialNumber(const X509 *x509) { return x509->cert_info->serialNumber; } -unsigned long X509_subject_name_hash(X509 *x) { - return (X509_NAME_hash(x->cert_info->subject)); +uint32_t X509_subject_name_hash(X509 *x) { + return X509_NAME_hash(x->cert_info->subject); } -unsigned long X509_subject_name_hash_old(X509 *x) { - return (X509_NAME_hash_old(x->cert_info->subject)); +uint32_t X509_subject_name_hash_old(X509 *x) { + return X509_NAME_hash_old(x->cert_info->subject); } // Compare two certificates: they must be identical for this to work. NB: @@ -165,44 +167,29 @@ int X509_NAME_cmp(const X509_NAME *a, const X509_NAME *b) { return OPENSSL_memcmp(a->canon_enc, b->canon_enc, a->canon_enclen); } -unsigned long X509_NAME_hash(X509_NAME *x) { - unsigned long ret = 0; - unsigned char md[SHA_DIGEST_LENGTH]; - - // Make sure X509_NAME structure contains valid cached encoding - i2d_X509_NAME(x, NULL); - if (!EVP_Digest(x->canon_enc, x->canon_enclen, md, NULL, EVP_sha1(), NULL)) { +uint32_t X509_NAME_hash(X509_NAME *x) { + // Make sure the X509_NAME structure contains a valid cached encoding. + if (i2d_X509_NAME(x, NULL) < 0) { return 0; } - ret = (((unsigned long)md[0]) | ((unsigned long)md[1] << 8L) | - ((unsigned long)md[2] << 16L) | ((unsigned long)md[3] << 24L)) & - 0xffffffffL; - return ret; + uint8_t md[SHA_DIGEST_LENGTH]; + SHA1(x->canon_enc, x->canon_enclen, md); + return CRYPTO_load_u32_le(md); } // I now DER encode the name and hash it. Since I cache the DER encoding, // this is reasonably efficient. -unsigned long X509_NAME_hash_old(X509_NAME *x) { - EVP_MD_CTX md_ctx; - unsigned long ret = 0; - unsigned char md[16]; - - // Make sure X509_NAME structure contains valid cached encoding - i2d_X509_NAME(x, NULL); - EVP_MD_CTX_init(&md_ctx); - // EVP_MD_CTX_set_flags(&md_ctx, EVP_MD_CTX_FLAG_NON_FIPS_ALLOW); - if (EVP_DigestInit_ex(&md_ctx, EVP_md5(), NULL) && - EVP_DigestUpdate(&md_ctx, x->bytes->data, x->bytes->length) && - EVP_DigestFinal_ex(&md_ctx, md, NULL)) { - ret = (((unsigned long)md[0]) | ((unsigned long)md[1] << 8L) | - ((unsigned long)md[2] << 16L) | ((unsigned long)md[3] << 24L)) & - 0xffffffffL; +uint32_t X509_NAME_hash_old(X509_NAME *x) { + // Make sure the X509_NAME structure contains a valid cached encoding. + if (i2d_X509_NAME(x, NULL) < 0) { + return 0; } - EVP_MD_CTX_cleanup(&md_ctx); - return ret; + uint8_t md[SHA_DIGEST_LENGTH]; + MD5((const uint8_t *)x->bytes->data, x->bytes->length, md); + return CRYPTO_load_u32_le(md); } X509 *X509_find_by_issuer_and_serial(const STACK_OF(X509) *sk, X509_NAME *name, diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 3458b92a32..7ca4e78768 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -3092,8 +3092,8 @@ TEST(X509Test, X509NameSet) { TEST(X509Test, NameHash) { struct { std::vector name_der; - unsigned long hash; - unsigned long hash_old; + uint32_t hash; + uint32_t hash_old; } kTests[] = { // SEQUENCE { // SET { diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 852c1dc78d..95f70c7f63 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2947,6 +2947,22 @@ OPENSSL_EXPORT int X509_subject_name_cmp(const X509 *a, const X509 *b); // CRL, only the issuer fields using |X509_NAME_cmp|. OPENSSL_EXPORT int X509_CRL_cmp(const X509_CRL *a, const X509_CRL *b); +// X509_issuer_name_hash returns the hash of |x509|'s issuer name with +// |X509_NAME_hash|. +OPENSSL_EXPORT uint32_t X509_issuer_name_hash(X509 *x509); + +// X509_subject_name_hash returns the hash of |x509|'s subject name with +// |X509_NAME_hash|. +OPENSSL_EXPORT uint32_t X509_subject_name_hash(X509 *x509); + +// X509_issuer_name_hash_old returns the hash of |x509|'s issuer name with +// |X509_NAME_hash_old|. +OPENSSL_EXPORT uint32_t X509_issuer_name_hash_old(X509 *x509); + +// X509_subject_name_hash_old returns the hash of |x509|'s usjbect name with +// |X509_NAME_hash_old|. +OPENSSL_EXPORT uint32_t X509_subject_name_hash_old(X509 *x509); + // ex_data functions. // @@ -3569,16 +3585,24 @@ OPENSSL_EXPORT const char *X509_get_default_private_dir(void); OPENSSL_EXPORT int X509_TRUST_set(int *t, int trust); -OPENSSL_EXPORT unsigned long X509_issuer_name_hash(X509 *a); - -OPENSSL_EXPORT unsigned long X509_subject_name_hash(X509 *x); +OPENSSL_EXPORT int X509_cmp(const X509 *a, const X509 *b); -OPENSSL_EXPORT unsigned long X509_issuer_name_hash_old(X509 *a); -OPENSSL_EXPORT unsigned long X509_subject_name_hash_old(X509 *x); +// X509_NAME_hash returns a hash of |name|, or zero on error. This is the new +// hash used by |X509_LOOKUP_hash_dir|. +// +// TODO(https://crbug.com/boringssl/407): This should be const and thread-safe +// but currently is neither, notably if |name| was modified from its parsed +// value. +OPENSSL_EXPORT uint32_t X509_NAME_hash(X509_NAME *name); -OPENSSL_EXPORT int X509_cmp(const X509 *a, const X509 *b); -OPENSSL_EXPORT unsigned long X509_NAME_hash(X509_NAME *x); -OPENSSL_EXPORT unsigned long X509_NAME_hash_old(X509_NAME *x); +// X509_NAME_hash_old returns a hash of |name|, or zero on error. This is the +// legacy hash used by |X509_LOOKUP_hash_dir|, which is still supported for +// compatibility. +// +// TODO(https://crbug.com/boringssl/407): This should be const and thread-safe +// but currently is neither, notably if |name| was modified from its parsed +// value. +OPENSSL_EXPORT uint32_t X509_NAME_hash_old(X509_NAME *name); OPENSSL_EXPORT int X509_CRL_match(const X509_CRL *a, const X509_CRL *b); From 04aaa057e01d46dc7dd0c22abac8cfaba4ab56cf Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 12 Dec 2023 11:48:10 -0500 Subject: [PATCH 09/27] Assume the Arm assembler can handle ADR It's 2023. We shouldn't need to be counting offsets from PC anymore. Instead, let the assembler figure this out with an ADR instruction. Additionally, since it's easy, in chacha-armv4.pl, avoid depending on the exact offset between code and data. We still depend on the code and data being close enough to fit within ADR's (very tight) bounds however. (E.g. an ADR of K256 inside sha256_block_data_order_armv8 would not work because K256 is too far away.) I have not removed the offset dependency in the SHA-2 files yet as they're a bit thorny and .Lsha256_block_data_order-K256 does not seem to work on Apple's 32-bit Arm assembler. (We probably should drop 32-bit Arm assembly on Apple platforms. It doesn't really exist anymore.) Once the armcap references are gone, that will be more straightforward. Update-Note: If 32-bit Arm assembly no longer builds, let us know and tell us what your toolchain is. Change-Id: Ie191781fed98d53c3b986b2f535132b970d79f98 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64747 Auto-Submit: David Benjamin Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 547221f5dc50e62543733cd65bd71bd97785afb6) --- crypto/chacha/asm/chacha-armv4.pl | 11 +++-------- crypto/fipsmodule/sha/asm/sha256-armv4.pl | 6 ++---- crypto/fipsmodule/sha/asm/sha512-armv4.pl | 6 ++---- generated-src/ios-arm/crypto/chacha/chacha-armv4.S | 11 +++-------- .../ios-arm/crypto/fipsmodule/sha256-armv4.S | 6 ++---- .../ios-arm/crypto/fipsmodule/sha512-armv4.S | 6 ++---- generated-src/linux-arm/crypto/chacha/chacha-armv4.S | 11 +++-------- .../linux-arm/crypto/fipsmodule/sha256-armv4.S | 6 ++---- .../linux-arm/crypto/fipsmodule/sha512-armv4.S | 6 ++---- 9 files changed, 21 insertions(+), 48 deletions(-) diff --git a/crypto/chacha/asm/chacha-armv4.pl b/crypto/chacha/asm/chacha-armv4.pl index fc08e90bb0..9899862bfc 100755 --- a/crypto/chacha/asm/chacha-armv4.pl +++ b/crypto/chacha/asm/chacha-armv4.pl @@ -201,7 +201,7 @@ sub ROUND { .long 1,0,0,0 #if __ARM_MAX_ARCH__>=7 .LOPENSSL_armcap: -.word OPENSSL_armcap_P-.LChaCha20_ctr32 +.word OPENSSL_armcap_P-.Lsigma #else .word -1 #endif @@ -213,11 +213,7 @@ sub ROUND { .LChaCha20_ctr32: ldr r12,[sp,#0] @ pull pointer to counter and nonce stmdb sp!,{r0-r2,r4-r11,lr} -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r14,pc,#16 @ ChaCha20_ctr32 -#else - adr r14,.LChaCha20_ctr32 -#endif + adr r14,.Lsigma cmp r2,#0 @ len==0? #ifdef __thumb2__ itt eq @@ -227,7 +223,7 @@ sub ROUND { #if __ARM_MAX_ARCH__>=7 cmp r2,#192 @ test len bls .Lshort - ldr r4,[r14,#-32] + ldr r4,[r14,#32] ldr r4,[r14,r4] # ifdef __APPLE__ ldr r4,[r4] @@ -238,7 +234,6 @@ sub ROUND { #endif ldmia r12,{r4-r7} @ load counter and nonce sub sp,sp,#4*(16) @ off-load area - sub r14,r14,#64 @ .Lsigma stmdb sp!,{r4-r7} @ copy counter and nonce ldmia r3,{r4-r11} @ load key ldmia r14,{r0-r3} @ load sigma diff --git a/crypto/fipsmodule/sha/asm/sha256-armv4.pl b/crypto/fipsmodule/sha/asm/sha256-armv4.pl index 4d12f4c397..ff509a6eb0 100644 --- a/crypto/fipsmodule/sha/asm/sha256-armv4.pl +++ b/crypto/fipsmodule/sha/asm/sha256-armv4.pl @@ -229,11 +229,7 @@ sub BODY_16_XX { .type sha256_block_data_order,%function sha256_block_data_order: .Lsha256_block_data_order: -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r3,pc,#8 @ sha256_block_data_order -#else adr r3,.Lsha256_block_data_order -#endif #if __ARM_MAX_ARCH__>=7 && !defined(__KERNEL__) ldr r12,.LOPENSSL_armcap ldr r12,[r3,r12] @ OPENSSL_armcap_P @@ -248,6 +244,8 @@ sub BODY_16_XX { add $len,$inp,$len,lsl#6 @ len to point at the end of inp stmdb sp!,{$ctx,$inp,$len,r4-r11,lr} ldmia $ctx,{$A,$B,$C,$D,$E,$F,$G,$H} + @ TODO(davidben): When the OPENSSL_armcap logic above is removed, + @ replace this with a simple ADR. sub $Ktbl,r3,#256+32 @ K256 sub sp,sp,#16*4 @ alloca(X[16]) .Loop: diff --git a/crypto/fipsmodule/sha/asm/sha512-armv4.pl b/crypto/fipsmodule/sha/asm/sha512-armv4.pl index 61d14aea26..8da171dbc5 100644 --- a/crypto/fipsmodule/sha/asm/sha512-armv4.pl +++ b/crypto/fipsmodule/sha/asm/sha512-armv4.pl @@ -290,11 +290,7 @@ () .type sha512_block_data_order,%function sha512_block_data_order: .Lsha512_block_data_order: -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r3,pc,#8 @ sha512_block_data_order -#else adr r3,.Lsha512_block_data_order -#endif #if __ARM_MAX_ARCH__>=7 && !defined(__KERNEL__) ldr r12,.LOPENSSL_armcap ldr r12,[r3,r12] @ OPENSSL_armcap_P @@ -306,6 +302,8 @@ () #endif add $len,$inp,$len,lsl#7 @ len to point at the end of inp stmdb sp!,{r4-r12,lr} + @ TODO(davidben): When the OPENSSL_armcap logic above is removed, + @ replace this with a simple ADR. sub $Ktbl,r3,#672 @ K512 sub sp,sp,#9*8 diff --git a/generated-src/ios-arm/crypto/chacha/chacha-armv4.S b/generated-src/ios-arm/crypto/chacha/chacha-armv4.S index bd836b60a4..9aa23e1c48 100644 --- a/generated-src/ios-arm/crypto/chacha/chacha-armv4.S +++ b/generated-src/ios-arm/crypto/chacha/chacha-armv4.S @@ -31,7 +31,7 @@ Lone: .long 1,0,0,0 #if __ARM_MAX_ARCH__>=7 LOPENSSL_armcap: -.word OPENSSL_armcap_P-LChaCha20_ctr32 +.word OPENSSL_armcap_P-Lsigma #else .word -1 #endif @@ -46,11 +46,7 @@ _ChaCha20_ctr32: LChaCha20_ctr32: ldr r12,[sp,#0] @ pull pointer to counter and nonce stmdb sp!,{r0,r1,r2,r4-r11,lr} -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r14,pc,#16 @ _ChaCha20_ctr32 -#else - adr r14,LChaCha20_ctr32 -#endif + adr r14,Lsigma cmp r2,#0 @ len==0? #ifdef __thumb2__ itt eq @@ -60,7 +56,7 @@ LChaCha20_ctr32: #if __ARM_MAX_ARCH__>=7 cmp r2,#192 @ test len bls Lshort - ldr r4,[r14,#-32] + ldr r4,[r14,#32] ldr r4,[r14,r4] # ifdef __APPLE__ ldr r4,[r4] @@ -71,7 +67,6 @@ Lshort: #endif ldmia r12,{r4,r5,r6,r7} @ load counter and nonce sub sp,sp,#4*(16) @ off-load area - sub r14,r14,#64 @ Lsigma stmdb sp!,{r4,r5,r6,r7} @ copy counter and nonce ldmia r3,{r4,r5,r6,r7,r8,r9,r10,r11} @ load key ldmia r14,{r0,r1,r2,r3} @ load sigma diff --git a/generated-src/ios-arm/crypto/fipsmodule/sha256-armv4.S b/generated-src/ios-arm/crypto/fipsmodule/sha256-armv4.S index cfe8de2d9b..31747007d3 100644 --- a/generated-src/ios-arm/crypto/fipsmodule/sha256-armv4.S +++ b/generated-src/ios-arm/crypto/fipsmodule/sha256-armv4.S @@ -103,11 +103,7 @@ LOPENSSL_armcap: #endif _sha256_block_data_order: Lsha256_block_data_order: -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r3,pc,#8 @ _sha256_block_data_order -#else adr r3,Lsha256_block_data_order -#endif #if __ARM_MAX_ARCH__>=7 && !defined(__KERNEL__) ldr r12,LOPENSSL_armcap ldr r12,[r3,r12] @ OPENSSL_armcap_P @@ -122,6 +118,8 @@ Lsha256_block_data_order: add r2,r1,r2,lsl#6 @ len to point at the end of inp stmdb sp!,{r0,r1,r2,r4-r11,lr} ldmia r0,{r4,r5,r6,r7,r8,r9,r10,r11} + @ TODO(davidben): When the OPENSSL_armcap logic above is removed, + @ replace this with a simple ADR. sub r14,r3,#256+32 @ K256 sub sp,sp,#16*4 @ alloca(X[16]) Loop: diff --git a/generated-src/ios-arm/crypto/fipsmodule/sha512-armv4.S b/generated-src/ios-arm/crypto/fipsmodule/sha512-armv4.S index 2b1cd5004a..fb737619a4 100644 --- a/generated-src/ios-arm/crypto/fipsmodule/sha512-armv4.S +++ b/generated-src/ios-arm/crypto/fipsmodule/sha512-armv4.S @@ -150,11 +150,7 @@ LOPENSSL_armcap: #endif _sha512_block_data_order: Lsha512_block_data_order: -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r3,pc,#8 @ _sha512_block_data_order -#else adr r3,Lsha512_block_data_order -#endif #if __ARM_MAX_ARCH__>=7 && !defined(__KERNEL__) ldr r12,LOPENSSL_armcap ldr r12,[r3,r12] @ OPENSSL_armcap_P @@ -166,6 +162,8 @@ Lsha512_block_data_order: #endif add r2,r1,r2,lsl#7 @ len to point at the end of inp stmdb sp!,{r4,r5,r6,r7,r8,r9,r10,r11,r12,lr} + @ TODO(davidben): When the OPENSSL_armcap logic above is removed, + @ replace this with a simple ADR. sub r14,r3,#672 @ K512 sub sp,sp,#9*8 diff --git a/generated-src/linux-arm/crypto/chacha/chacha-armv4.S b/generated-src/linux-arm/crypto/chacha/chacha-armv4.S index 4494c50b8e..6d3e84bc5e 100644 --- a/generated-src/linux-arm/crypto/chacha/chacha-armv4.S +++ b/generated-src/linux-arm/crypto/chacha/chacha-armv4.S @@ -31,7 +31,7 @@ .long 1,0,0,0 #if __ARM_MAX_ARCH__>=7 .LOPENSSL_armcap: -.word OPENSSL_armcap_P-.LChaCha20_ctr32 +.word OPENSSL_armcap_P-.Lsigma #else .word -1 #endif @@ -44,11 +44,7 @@ ChaCha20_ctr32: .LChaCha20_ctr32: ldr r12,[sp,#0] @ pull pointer to counter and nonce stmdb sp!,{r0,r1,r2,r4-r11,lr} -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r14,pc,#16 @ ChaCha20_ctr32 -#else - adr r14,.LChaCha20_ctr32 -#endif + adr r14,.Lsigma cmp r2,#0 @ len==0? #ifdef __thumb2__ itt eq @@ -58,7 +54,7 @@ ChaCha20_ctr32: #if __ARM_MAX_ARCH__>=7 cmp r2,#192 @ test len bls .Lshort - ldr r4,[r14,#-32] + ldr r4,[r14,#32] ldr r4,[r14,r4] # ifdef __APPLE__ ldr r4,[r4] @@ -69,7 +65,6 @@ ChaCha20_ctr32: #endif ldmia r12,{r4,r5,r6,r7} @ load counter and nonce sub sp,sp,#4*(16) @ off-load area - sub r14,r14,#64 @ .Lsigma stmdb sp!,{r4,r5,r6,r7} @ copy counter and nonce ldmia r3,{r4,r5,r6,r7,r8,r9,r10,r11} @ load key ldmia r14,{r0,r1,r2,r3} @ load sigma diff --git a/generated-src/linux-arm/crypto/fipsmodule/sha256-armv4.S b/generated-src/linux-arm/crypto/fipsmodule/sha256-armv4.S index 28ff5978b8..610a35afc0 100644 --- a/generated-src/linux-arm/crypto/fipsmodule/sha256-armv4.S +++ b/generated-src/linux-arm/crypto/fipsmodule/sha256-armv4.S @@ -101,11 +101,7 @@ K256: .type sha256_block_data_order,%function sha256_block_data_order: .Lsha256_block_data_order: -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r3,pc,#8 @ sha256_block_data_order -#else adr r3,.Lsha256_block_data_order -#endif #if __ARM_MAX_ARCH__>=7 && !defined(__KERNEL__) ldr r12,.LOPENSSL_armcap ldr r12,[r3,r12] @ OPENSSL_armcap_P @@ -120,6 +116,8 @@ sha256_block_data_order: add r2,r1,r2,lsl#6 @ len to point at the end of inp stmdb sp!,{r0,r1,r2,r4-r11,lr} ldmia r0,{r4,r5,r6,r7,r8,r9,r10,r11} + @ TODO(davidben): When the OPENSSL_armcap logic above is removed, + @ replace this with a simple ADR. sub r14,r3,#256+32 @ K256 sub sp,sp,#16*4 @ alloca(X[16]) .Loop: diff --git a/generated-src/linux-arm/crypto/fipsmodule/sha512-armv4.S b/generated-src/linux-arm/crypto/fipsmodule/sha512-armv4.S index 4003168827..135d23338e 100644 --- a/generated-src/linux-arm/crypto/fipsmodule/sha512-armv4.S +++ b/generated-src/linux-arm/crypto/fipsmodule/sha512-armv4.S @@ -148,11 +148,7 @@ K512: .type sha512_block_data_order,%function sha512_block_data_order: .Lsha512_block_data_order: -#if __ARM_ARCH<7 && !defined(__thumb2__) - sub r3,pc,#8 @ sha512_block_data_order -#else adr r3,.Lsha512_block_data_order -#endif #if __ARM_MAX_ARCH__>=7 && !defined(__KERNEL__) ldr r12,.LOPENSSL_armcap ldr r12,[r3,r12] @ OPENSSL_armcap_P @@ -164,6 +160,8 @@ sha512_block_data_order: #endif add r2,r1,r2,lsl#7 @ len to point at the end of inp stmdb sp!,{r4,r5,r6,r7,r8,r9,r10,r11,r12,lr} + @ TODO(davidben): When the OPENSSL_armcap logic above is removed, + @ replace this with a simple ADR. sub r14,r3,#672 @ K512 sub sp,sp,#9*8 From ec99b829dc2093e29ac6363d80e8f8e944ff112d Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Tue, 12 Dec 2023 13:58:22 -0500 Subject: [PATCH 10/27] Disable 32-bit Arm assembly optimizations on iOS The last iOS version that supported 32-bit was iOS 10, which I don't believe any of our consumers support anymore. (Chromium does not, neither does google/oss-policies-info.) Our iOS CI coverage comes from Chromium, so this means we don't actually have any test coverage for 32-bit iOS, only compile coverage. In addition to lacking any test coverage, 32-bit Arm assembly is more platform-dependent than one might expect, between different limitiations on patterns for PC-relative loads and lots of assembler quirks around what kinds of label expressions (which show up in PC-relative loads a lot) are allowed. Finally, since iOS in that era did not do runtime detection of features and relied on compiling a binary multiple times, the 32-bit assembly would never enable AES acceleration anyway, so it's not as impactful as on other platforms. Between all that, it's no longer worth enabling this. Disable it in target.h which, with the all the recent build simplifications, should be sufficient to disable this code. Update-Note: iOS on 32-bit Arm now disables assembly. This is unlikely to impact anyone. As far as I can tell, 32-bit Arm for iOS thoroughly does not exist anymore. Change-Id: If31208b42047377ad1b4fb0af6fee17334f18330 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64748 Auto-Submit: David Benjamin Reviewed-by: Bob Beck Commit-Queue: Bob Beck (cherry picked from commit fcec1397a411cbe4e27bd1428dad63adf207dc33) --- include/openssl/target.h | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/include/openssl/target.h b/include/openssl/target.h index 8bb421cb48..feb0bdfd4c 100644 --- a/include/openssl/target.h +++ b/include/openssl/target.h @@ -212,6 +212,12 @@ #endif #endif +// Disable 32-bit Arm assembly on Apple platforms. The last iOS version that +// supported 32-bit Arm was iOS 10. +#if defined(OPENSSL_APPLE) && defined(OPENSSL_ARM) +#define OPENSSL_ASM_INCOMPATIBLE +#endif + #if defined(OPENSSL_ASM_INCOMPATIBLE) #undef OPENSSL_ASM_INCOMPATIBLE #if !defined(OPENSSL_NO_ASM) From 7682ed1d26a60b90b243a1070afcc0439ffc9c14 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Wed, 13 Dec 2023 10:33:17 -0500 Subject: [PATCH 11/27] Fix X509_ATTRIBUTE_set1_data with negative attributes One of the WARNINGs in this function is unambiguously a bug. Just fix it. In doing so, add a helper for the ASN1_STRING -> ASN1_TYPE conversion and make this function easier to follow. Also document the attrtype == 0 case. I missed that one when enumerating this overcomplicated calling convention. Move that check earlier so we don't try to process the input. It's ignored anyway. Change-Id: Ia6e2dcb7c69488b6e2e58a68c3b701040ed3d4ef Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64827 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: David Benjamin (cherry picked from commit 23d58423169a2d473a6028228e46f68d1e65f503) --- crypto/asn1/a_strex.c | 20 ++------- crypto/asn1/a_type.c | 20 ++++++++- crypto/asn1/internal.h | 4 ++ crypto/x509/x509_att.c | 67 +++++++++++++++-------------- crypto/x509/x509_test.cc | 91 ++++++++++++++++++++++++++-------------- include/openssl/x509.h | 20 ++++----- 6 files changed, 132 insertions(+), 90 deletions(-) diff --git a/crypto/asn1/a_strex.c b/crypto/asn1/a_strex.c index 7bcbdfb29d..06af54d566 100644 --- a/crypto/asn1/a_strex.c +++ b/crypto/asn1/a_strex.c @@ -67,6 +67,8 @@ #include #include +#include "../bytestring/internal.h" +#include "../internal.h" #include "internal.h" @@ -256,22 +258,8 @@ static int do_dump(unsigned long flags, BIO *out, const ASN1_STRING *str) { // Placing the ASN1_STRING in a temporary ASN1_TYPE allows the DER encoding // to readily obtained. ASN1_TYPE t; - t.type = str->type; - // Negative INTEGER and ENUMERATED values are the only case where - // |ASN1_STRING| and |ASN1_TYPE| types do not match. - // - // TODO(davidben): There are also some type fields which, in |ASN1_TYPE|, do - // not correspond to |ASN1_STRING|. It is unclear whether those are allowed - // in |ASN1_STRING| at all, or what the space of allowed types is. - // |ASN1_item_ex_d2i| will never produce such a value so, for now, we say - // this is an invalid input. But this corner of the library in general - // should be more robust. - if (t.type == V_ASN1_NEG_INTEGER) { - t.type = V_ASN1_INTEGER; - } else if (t.type == V_ASN1_NEG_ENUMERATED) { - t.type = V_ASN1_ENUMERATED; - } - t.value.asn1_string = (ASN1_STRING *)str; + OPENSSL_memset(&t, 0, sizeof(ASN1_TYPE)); + asn1_type_set0_string(&t, (ASN1_STRING *)str); unsigned char *der_buf = NULL; int der_len = i2d_ASN1_TYPE(&t, &der_buf); if (der_len < 0) { diff --git a/crypto/asn1/a_type.c b/crypto/asn1/a_type.c index 0c2fd136d3..af603a3e8e 100644 --- a/crypto/asn1/a_type.c +++ b/crypto/asn1/a_type.c @@ -56,7 +56,8 @@ #include -#include +#include + #include #include #include @@ -89,6 +90,23 @@ const void *asn1_type_value_as_pointer(const ASN1_TYPE *a) { } } +void asn1_type_set0_string(ASN1_TYPE *a, ASN1_STRING *str) { + // |ASN1_STRING| types are almost the same as |ASN1_TYPE| types, except that + // the negative flag is not reflected into |ASN1_TYPE|. + int type = str->type; + if (type == V_ASN1_NEG_INTEGER) { + type = V_ASN1_INTEGER; + } else if (type == V_ASN1_NEG_ENUMERATED) { + type = V_ASN1_ENUMERATED; + } + + // These types are not |ASN1_STRING| types and use a different + // representation when stored in |ASN1_TYPE|. + assert(type != V_ASN1_NULL && type != V_ASN1_OBJECT && + type != V_ASN1_BOOLEAN); + ASN1_TYPE_set(a, type, str); +} + void asn1_type_cleanup(ASN1_TYPE *a) { switch (a->type) { case V_ASN1_NULL: diff --git a/crypto/asn1/internal.h b/crypto/asn1/internal.h index 56b534fd29..c05ead243a 100644 --- a/crypto/asn1/internal.h +++ b/crypto/asn1/internal.h @@ -218,6 +218,10 @@ void asn1_encoding_clear(ASN1_ENCODING *enc); // a pointer. const void *asn1_type_value_as_pointer(const ASN1_TYPE *a); +// asn1_type_set0_string sets |a|'s value to the object represented by |str| and +// takes ownership of |str|. +void asn1_type_set0_string(ASN1_TYPE *a, ASN1_STRING *str); + // asn1_type_cleanup releases memory associated with |a|'s value, without // freeing |a| itself. void asn1_type_cleanup(ASN1_TYPE *a); diff --git a/crypto/x509/x509_att.c b/crypto/x509/x509_att.c index 062168eaf7..e3d0c6a595 100644 --- a/crypto/x509/x509_att.c +++ b/crypto/x509/x509_att.c @@ -137,54 +137,57 @@ int X509_ATTRIBUTE_set1_object(X509_ATTRIBUTE *attr, const ASN1_OBJECT *obj) { int X509_ATTRIBUTE_set1_data(X509_ATTRIBUTE *attr, int attrtype, const void *data, int len) { - ASN1_TYPE *ttmp = NULL; - ASN1_STRING *stmp = NULL; - int atype = 0; if (!attr) { return 0; } + + if (attrtype == 0) { + // Do nothing. This is used to create an empty value set in + // |X509_ATTRIBUTE_create_by_*|. This is invalid, but supported by OpenSSL. + return 1; + } + + ASN1_TYPE *typ = ASN1_TYPE_new(); + if (typ == NULL) { + return 0; + } + + // This function is several functions in one. if (attrtype & MBSTRING_FLAG) { - stmp = ASN1_STRING_set_by_NID(NULL, data, len, attrtype, - OBJ_obj2nid(attr->object)); - if (!stmp) { + // |data| is an encoded string. We must decode and re-encode it to |attr|'s + // preferred ASN.1 type. Note |len| may be -1, in which case + // |ASN1_STRING_set_by_NID| calls |strlen| automatically. + ASN1_STRING *str = ASN1_STRING_set_by_NID(NULL, data, len, attrtype, + OBJ_obj2nid(attr->object)); + if (str == NULL) { OPENSSL_PUT_ERROR(X509, ERR_R_ASN1_LIB); - return 0; - } - atype = stmp->type; - } else if (len != -1) { - if (!(stmp = ASN1_STRING_type_new(attrtype))) { goto err; } - if (!ASN1_STRING_set(stmp, data, len)) { + asn1_type_set0_string(typ, str); + } else if (len != -1) { + // |attrtype| must be a valid |ASN1_STRING| type. |data| and |len| is a + // value in the corresponding |ASN1_STRING| representation. + ASN1_STRING *str = ASN1_STRING_type_new(attrtype); + if (str == NULL || !ASN1_STRING_set(str, data, len)) { + ASN1_STRING_free(str); goto err; } - atype = attrtype; - } - // This is a bit naughty because the attribute should really have at - // least one value but some types use and zero length SET and require - // this. - if (attrtype == 0) { - ASN1_STRING_free(stmp); - return 1; - } - if (!(ttmp = ASN1_TYPE_new())) { - goto err; - } - if ((len == -1) && !(attrtype & MBSTRING_FLAG)) { - if (!ASN1_TYPE_set1(ttmp, attrtype, data)) { + asn1_type_set0_string(typ, str); + } else { + // |attrtype| must be a valid |ASN1_TYPE| type. |data| is a pointer to an + // object of the corresponding type. + if (!ASN1_TYPE_set1(typ, attrtype, data)) { goto err; } - } else { - ASN1_TYPE_set(ttmp, atype, stmp); - stmp = NULL; } - if (!sk_ASN1_TYPE_push(attr->set, ttmp)) { + + if (!sk_ASN1_TYPE_push(attr->set, typ)) { goto err; } return 1; + err: - ASN1_TYPE_free(ttmp); - ASN1_STRING_free(stmp); + ASN1_TYPE_free(typ); return 0; } diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 7ca4e78768..3c2b63e78c 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -4342,46 +4342,62 @@ TEST(X509Test, X509AlgorExtract) { // Test the various |X509_ATTRIBUTE| creation functions. TEST(X509Test, Attribute) { - // The friendlyName attribute has a BMPString value. See RFC 2985, - // section 5.5.1. + // The expected attribute values are: + // 1. BMPString U+2603 + // 2. BMPString "test" + // 3. INTEGER -1 (not valid for friendlyName) static const uint8_t kTest1[] = {0x26, 0x03}; // U+2603 SNOWMAN static const uint8_t kTest1UTF8[] = {0xe2, 0x98, 0x83}; static const uint8_t kTest2[] = {0, 't', 0, 'e', 0, 's', 0, 't'}; - auto check_attribute = [&](X509_ATTRIBUTE *attr, bool has_test2) { + constexpr uint32_t kTest1Mask = 1 << 0; + constexpr uint32_t kTest2Mask = 1 << 1; + constexpr uint32_t kTest3Mask = 1 << 2; + auto check_attribute = [&](X509_ATTRIBUTE *attr, uint32_t mask) { EXPECT_EQ(NID_friendlyName, OBJ_obj2nid(X509_ATTRIBUTE_get0_object(attr))); - EXPECT_EQ(has_test2 ? 2 : 1, X509_ATTRIBUTE_count(attr)); - - // The first attribute should contain |kTest1|. - const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, 0); - ASSERT_TRUE(value); - EXPECT_EQ(V_ASN1_BMPSTRING, value->type); - EXPECT_EQ(Bytes(kTest1), - Bytes(ASN1_STRING_get0_data(value->value.bmpstring), - ASN1_STRING_length(value->value.bmpstring))); - - // |X509_ATTRIBUTE_get0_data| requires the type match. - EXPECT_FALSE( - X509_ATTRIBUTE_get0_data(attr, 0, V_ASN1_OCTET_STRING, nullptr)); - const ASN1_BMPSTRING *bmpstring = static_cast( - X509_ATTRIBUTE_get0_data(attr, 0, V_ASN1_BMPSTRING, nullptr)); - ASSERT_TRUE(bmpstring); - EXPECT_EQ(Bytes(kTest1), Bytes(ASN1_STRING_get0_data(bmpstring), - ASN1_STRING_length(bmpstring))); - - if (has_test2) { - value = X509_ATTRIBUTE_get0_type(attr, 1); + int idx = 0; + if (mask & kTest1Mask) { + // The first attribute should contain |kTest1|. + const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, idx); + ASSERT_TRUE(value); + EXPECT_EQ(V_ASN1_BMPSTRING, value->type); + EXPECT_EQ(Bytes(kTest1), + Bytes(ASN1_STRING_get0_data(value->value.bmpstring), + ASN1_STRING_length(value->value.bmpstring))); + + // |X509_ATTRIBUTE_get0_data| requires the type match. + EXPECT_FALSE( + X509_ATTRIBUTE_get0_data(attr, idx, V_ASN1_OCTET_STRING, nullptr)); + const ASN1_BMPSTRING *bmpstring = static_cast( + X509_ATTRIBUTE_get0_data(attr, idx, V_ASN1_BMPSTRING, nullptr)); + ASSERT_TRUE(bmpstring); + EXPECT_EQ(Bytes(kTest1), Bytes(ASN1_STRING_get0_data(bmpstring), + ASN1_STRING_length(bmpstring))); + idx++; + } + + if (mask & kTest2Mask) { + const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, idx); ASSERT_TRUE(value); EXPECT_EQ(V_ASN1_BMPSTRING, value->type); EXPECT_EQ(Bytes(kTest2), Bytes(ASN1_STRING_get0_data(value->value.bmpstring), ASN1_STRING_length(value->value.bmpstring))); - } else { - EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, 1)); + idx++; } - EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, 2)); + if (mask & kTest3Mask) { + const ASN1_TYPE *value = X509_ATTRIBUTE_get0_type(attr, idx); + ASSERT_TRUE(value); + EXPECT_EQ(V_ASN1_INTEGER, value->type); + int64_t v; + ASSERT_TRUE(ASN1_INTEGER_get_int64(&v, value->value.integer)); + EXPECT_EQ(v, -1); + idx++; + } + + EXPECT_FALSE(X509_ATTRIBUTE_get0_type(attr, idx)); }; bssl::UniquePtr str(ASN1_STRING_type_new(V_ASN1_BMPSTRING)); @@ -4393,7 +4409,7 @@ TEST(X509Test, Attribute) { X509_ATTRIBUTE_create(NID_friendlyName, V_ASN1_BMPSTRING, str.get())); ASSERT_TRUE(attr); str.release(); // |X509_ATTRIBUTE_create| takes ownership on success. - check_attribute(attr.get(), /*has_test2=*/false); + check_attribute(attr.get(), kTest1Mask); // Test the |MBSTRING_*| form of |X509_ATTRIBUTE_set1_data|. attr.reset(X509_ATTRIBUTE_new()); @@ -4402,12 +4418,19 @@ TEST(X509Test, Attribute) { X509_ATTRIBUTE_set1_object(attr.get(), OBJ_nid2obj(NID_friendlyName))); ASSERT_TRUE(X509_ATTRIBUTE_set1_data(attr.get(), MBSTRING_UTF8, kTest1UTF8, sizeof(kTest1UTF8))); - check_attribute(attr.get(), /*has_test2=*/false); + check_attribute(attr.get(), kTest1Mask); // Test the |ASN1_STRING| form of |X509_ATTRIBUTE_set1_data|. ASSERT_TRUE(X509_ATTRIBUTE_set1_data(attr.get(), V_ASN1_BMPSTRING, kTest2, sizeof(kTest2))); - check_attribute(attr.get(), /*has_test2=*/true); + check_attribute(attr.get(), kTest1Mask | kTest2Mask); + + // The |ASN1_STRING| form of |X509_ATTRIBUTE_set1_data| should correctly + // handle negative integers. + const uint8_t kOne = 1; + ASSERT_TRUE( + X509_ATTRIBUTE_set1_data(attr.get(), V_ASN1_NEG_INTEGER, &kOne, 1)); + check_attribute(attr.get(), kTest1Mask | kTest2Mask | kTest3Mask); // Test the |ASN1_TYPE| form of |X509_ATTRIBUTE_set1_data|. attr.reset(X509_ATTRIBUTE_new()); @@ -4419,7 +4442,13 @@ TEST(X509Test, Attribute) { ASSERT_TRUE(ASN1_STRING_set(str.get(), kTest1, sizeof(kTest1))); ASSERT_TRUE( X509_ATTRIBUTE_set1_data(attr.get(), V_ASN1_BMPSTRING, str.get(), -1)); - check_attribute(attr.get(), /*has_test2=*/false); + check_attribute(attr.get(), kTest1Mask); + + // An |attrtype| of zero leaves the attribute empty. + attr.reset(X509_ATTRIBUTE_create_by_NID( + nullptr, NID_friendlyName, /*attrtype=*/0, /*data=*/nullptr, /*len=*/0)); + ASSERT_TRUE(attr); + check_attribute(attr.get(), 0); } // Test that, by default, |X509_V_FLAG_TRUSTED_FIRST| is set, which means we'll diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 95f70c7f63..14acf3012e 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -2182,21 +2182,21 @@ OPENSSL_EXPORT int X509_ATTRIBUTE_set1_object(X509_ATTRIBUTE *attr, // X509_ATTRIBUTE_set1_data appends a value to |attr|'s value set and returns // one on success or zero on error. The value is determined as follows: // -// If |attrtype| is a |MBSTRING_*| constant, the value is an ASN.1 string. The -// string is determined by decoding |len| bytes from |data| in the encoding -// specified by |attrtype|, and then re-encoding it in a form appropriate for -// |attr|'s type. If |len| is -1, |strlen(data)| is used instead. See -// |ASN1_STRING_set_by_NID| for details. +// If |attrtype| is zero, this function returns one and does nothing. This form +// may be used when calling |X509_ATTRIBUTE_create_by_*| to create an attribute +// with an empty value set. Such attributes are invalid, but OpenSSL supports +// creating them. +// +// Otherwise, if |attrtype| is a |MBSTRING_*| constant, the value is an ASN.1 +// string. The string is determined by decoding |len| bytes from |data| in the +// encoding specified by |attrtype|, and then re-encoding it in a form +// appropriate for |attr|'s type. If |len| is -1, |strlen(data)| is used +// instead. See |ASN1_STRING_set_by_NID| for details. // // Otherwise, if |len| is not -1, the value is an ASN.1 string. |attrtype| is an // |ASN1_STRING| type value and the |len| bytes from |data| are copied as the // type-specific representation of |ASN1_STRING|. See |ASN1_STRING| for details. // -// WARNING: If this form is used to construct a negative INTEGER or ENUMERATED, -// |attrtype| includes the |V_ASN1_NEG| flag for |ASN1_STRING|, but the function -// forgets to clear the flag for |ASN1_TYPE|. This matches OpenSSL but is -// probably a bug. For now, do not use this form with negative values. -// // Otherwise, if |len| is -1, the value is constructed by passing |attrtype| and // |data| to |ASN1_TYPE_set1|. That is, |attrtype| is an |ASN1_TYPE| type value, // and |data| is cast to the corresponding pointer type. From 6635445c8171d27a5697fdb854815265e5b46dd8 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 11 Dec 2023 01:18:00 -0500 Subject: [PATCH 12/27] Change certificate depth limit to match OpenSSL and document OpenSSL 1.1.0 included d9b8b89bec4480de3a10bdaf9425db371c19145b, which a cleanup change to X509_verify_cert. This cleanup changed the semanitcs of the depth limit. Previously, the depth limit omitted the leaf but included the trust anchor. Now it omits both. We forked a little before 1.0.2, so we still had the old behavior. Now that the new behavior is well-established, switch to new one. Bump BORINGSSL_API_VERSION so callers can detect one or the other as needed. Document the new semantics. Also fix up some older docs where I implied -1 was unlimited depth. Negative numbers were actually enforced as negative numbers (which means only explicitly-trusted self-signed certs worked). Update-Note: The new semantics increase the limit by 1 compared to the old ones. Thus this change should only accept more chains than previously and be relatively safe. It also makes us more OpenSSL-compatible. Envoy will need a tweak because they unit test the boundary condition for the depth limit. Bug: 426 Fixed: 459 Change-Id: Ifaa108b8135ea3d875f2ac1f2a3b2cd8a22aa323 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64707 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit b251d813ec615e7ef01d82073f94960eb13b1e0a) --- crypto/x509/internal.h | 2 +- crypto/x509/x509_test.cc | 61 +++++++++++++++++++++++++++++++--------- crypto/x509/x509_vfy.c | 30 +++++++++++--------- include/openssl/base.h | 2 +- include/openssl/ssl.h | 8 +++--- include/openssl/x509.h | 22 +++++++++++---- 6 files changed, 85 insertions(+), 40 deletions(-) diff --git a/crypto/x509/internal.h b/crypto/x509/internal.h index 7e3cb21faf..9c7f2117e7 100644 --- a/crypto/x509/internal.h +++ b/crypto/x509/internal.h @@ -346,7 +346,7 @@ struct x509_store_ctx_st { X509_STORE_CTX_check_crl_fn check_crl; // Check CRL validity // The following is built up - int valid; // if 0, rebuild chain + int last_untrusted; // index of last untrusted cert STACK_OF(X509) *chain; // chain of X509s - built up and trusted diff --git a/crypto/x509/x509_test.cc b/crypto/x509/x509_test.cc index 3c2b63e78c..ce9409f1db 100644 --- a/crypto/x509/x509_test.cc +++ b/crypto/x509/x509_test.cc @@ -12,6 +12,8 @@ * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ +#include + #include #include #include @@ -1614,33 +1616,37 @@ TEST(X509Test, TestVerify) { // though in different ways. for (bool trusted_first : {true, false}) { SCOPED_TRACE(trusted_first); - std::function configure_callback; - if (!trusted_first) { + bool override_depth = false; + int depth = -1; + auto configure_callback = [&](X509_VERIFY_PARAM *param) { // Note we need the callback to clear the flag. Setting |flags| to zero // only skips setting new flags. - configure_callback = [&](X509_VERIFY_PARAM *param) { + if (!trusted_first) { X509_VERIFY_PARAM_clear_flags(param, X509_V_FLAG_TRUSTED_FIRST); - }; - } + } + if (override_depth) { + X509_VERIFY_PARAM_set_depth(param, depth); + } + }; // No trust anchors configured. - ASSERT_EQ(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, + EXPECT_EQ(X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, Verify(leaf.get(), /*roots=*/{}, /*intermediates=*/{}, /*crls=*/{}, /*flags=*/0, configure_callback)); - ASSERT_EQ( + EXPECT_EQ( X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, Verify(leaf.get(), /*roots=*/{}, {intermediate.get()}, /*crls=*/{}, /*flags=*/0, configure_callback)); // Each chain works individually. - ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {intermediate.get()}, + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {intermediate.get()}, /*crls=*/{}, /*flags=*/0, configure_callback)); - ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {cross_signing_root.get()}, + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {cross_signing_root.get()}, {intermediate.get(), root_cross_signed.get()}, /*crls=*/{}, /*flags=*/0, configure_callback)); // When both roots are available, we pick one or the other. - ASSERT_EQ(X509_V_OK, + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {cross_signing_root.get(), root.get()}, {intermediate.get(), root_cross_signed.get()}, /*crls=*/{}, /*flags=*/0, configure_callback)); @@ -1649,7 +1655,7 @@ TEST(X509Test, TestVerify) { // the cross-sign in the intermediates. With |trusted_first|, we // preferentially stop path-building at |intermediate|. Without // |trusted_first|, the "altchains" logic repairs it. - ASSERT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, + EXPECT_EQ(X509_V_OK, Verify(leaf.get(), {root.get()}, {intermediate.get(), root_cross_signed.get()}, /*crls=*/{}, /*flags=*/0, configure_callback)); @@ -1660,7 +1666,7 @@ TEST(X509Test, TestVerify) { // This test exists to confirm our current behavior, but these modes are // just workarounds for not having an actual path-building verifier. If we // fix it, this test can be removed. - ASSERT_EQ(trusted_first ? X509_V_OK + EXPECT_EQ(trusted_first ? X509_V_OK : X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, Verify(leaf.get(), {root.get()}, {intermediate.get(), root_cross_signed.get()}, /*crls=*/{}, @@ -1668,18 +1674,45 @@ TEST(X509Test, TestVerify) { // |forgery| is signed by |leaf_no_key_usage|, but is rejected because the // leaf is not a CA. - ASSERT_EQ(X509_V_ERR_INVALID_CA, + EXPECT_EQ(X509_V_ERR_INVALID_CA, Verify(forgery.get(), {intermediate_self_signed.get()}, {leaf_no_key_usage.get()}, /*crls=*/{}, /*flags=*/0, configure_callback)); // Test that one cannot skip Basic Constraints checking with a contorted set // of roots and intermediates. This is a regression test for CVE-2015-1793. - ASSERT_EQ(X509_V_ERR_INVALID_CA, + EXPECT_EQ(X509_V_ERR_INVALID_CA, Verify(forgery.get(), {intermediate_self_signed.get(), root_cross_signed.get()}, {leaf_no_key_usage.get(), intermediate.get()}, /*crls=*/{}, /*flags=*/0, configure_callback)); + + // Test depth limits. |configure_callback| looks at |override_depth| and + // |depth|. Negative numbers have historically worked, so test those too. + for (int d : {-4, -3, -2, -1, 0, 1, 2, 3, 4, INT_MAX - 3, INT_MAX - 2, + INT_MAX - 1, INT_MAX}) { + SCOPED_TRACE(d); + override_depth = true; + depth = d; + // A chain with a leaf, two intermediates, and a root is depth two. + EXPECT_EQ( + depth >= 2 ? X509_V_OK : X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, + Verify(leaf.get(), {cross_signing_root.get()}, + {intermediate.get(), root_cross_signed.get()}, + /*crls=*/{}, /*flags=*/0, configure_callback)); + + // A chain with a leaf, a root, and no intermediates is depth zero. + EXPECT_EQ( + depth >= 0 ? X509_V_OK : X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY, + Verify(root_cross_signed.get(), {cross_signing_root.get()}, {}, + /*crls=*/{}, /*flags=*/0, configure_callback)); + + // An explicitly trusted self-signed certificate is unaffected by depth + // checks. + EXPECT_EQ(X509_V_OK, + Verify(cross_signing_root.get(), {cross_signing_root.get()}, {}, + /*crls=*/{}, /*flags=*/0, configure_callback)); + } } } diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c index 19698f06cf..0cf343fda7 100644 --- a/crypto/x509/x509_vfy.c +++ b/crypto/x509/x509_vfy.c @@ -55,6 +55,7 @@ * [including the GNU Public Licence.] */ #include +#include #include #include @@ -160,11 +161,11 @@ static X509 *lookup_cert_match(X509_STORE_CTX *ctx, X509 *x) { } int X509_verify_cert(X509_STORE_CTX *ctx) { - X509 *x, *xtmp, *xtmp2, *chain_ss = NULL; + X509 *xtmp, *xtmp2, *chain_ss = NULL; int bad_chain = 0; X509_VERIFY_PARAM *param = ctx->param; - int depth, i, ok = 0; - int num, j, retry, trust; + int i, ok = 0; + int j, retry, trust; STACK_OF(X509) *sktmp = NULL; if (ctx->cert == NULL) { @@ -207,17 +208,17 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { goto end; } - num = (int)sk_X509_num(ctx->chain); - x = sk_X509_value(ctx->chain, num - 1); - depth = param->depth; + int num = (int)sk_X509_num(ctx->chain); + X509 *x = sk_X509_value(ctx->chain, num - 1); + // |param->depth| does not include the leaf certificate or the trust anchor, + // so the maximum size is 2 more. + int max_chain = param->depth >= INT_MAX - 2 ? INT_MAX : param->depth + 2; for (;;) { - // If we have enough, we break - if (depth < num) { - break; // FIXME: If this happens, we should take - // note of it and, if appropriate, use the - // X509_V_ERR_CERT_CHAIN_TOO_LONG error code - // later. + if (num >= max_chain) { + // FIXME: If this happens, we should take note of it and, if appropriate, + // use the X509_V_ERR_CERT_CHAIN_TOO_LONG error code later. + break; } int is_self_signed; @@ -321,8 +322,9 @@ int X509_verify_cert(X509_STORE_CTX *ctx) { } // We now lookup certs from the certificate store for (;;) { - // If we have enough, we break - if (depth < num) { + if (num >= max_chain) { + // FIXME: If this happens, we should take note of it and, if + // appropriate, use the X509_V_ERR_CERT_CHAIN_TOO_LONG error code later. break; } if (!cert_self_signed(x, &is_self_signed)) { diff --git a/include/openssl/base.h b/include/openssl/base.h index a759dfa9b7..5866918944 100644 --- a/include/openssl/base.h +++ b/include/openssl/base.h @@ -114,7 +114,7 @@ extern "C" { // A consumer may use this symbol in the preprocessor to temporarily build // against multiple revisions of BoringSSL at the same time. It is not // recommended to do so for longer than is necessary. -#define AWSLC_API_VERSION 28 +#define AWSLC_API_VERSION 29 // This string tracks the most current production release version on Github // https://github.com/aws/aws-lc/releases. diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index b4dbbd3889..364fbbd96e 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -2933,13 +2933,13 @@ OPENSSL_EXPORT int SSL_set1_host(SSL *ssl, const char *hostname); OPENSSL_EXPORT void SSL_set_hostflags(SSL *ssl, unsigned flags); // SSL_CTX_set_verify_depth sets the maximum depth of a certificate chain -// accepted in verification. This number does not include the leaf, so a depth -// of 1 allows the leaf and one CA certificate. +// accepted in verification. This count excludes both the target certificate and +// the trust anchor (root certificate). OPENSSL_EXPORT void SSL_CTX_set_verify_depth(SSL_CTX *ctx, int depth); // SSL_set_verify_depth sets the maximum depth of a certificate chain accepted -// in verification. This number does not include the leaf, so a depth of 1 -// allows the leaf and one CA certificate. +// in verification. This count excludes both the target certificate and the +// trust anchor (root certificate). OPENSSL_EXPORT void SSL_set_verify_depth(SSL *ssl, int depth); // SSL_CTX_get_verify_depth returns the maximum depth of a certificate accepted diff --git a/include/openssl/x509.h b/include/openssl/x509.h index 14acf3012e..bbd6f1311b 100644 --- a/include/openssl/x509.h +++ b/include/openssl/x509.h @@ -3671,8 +3671,14 @@ typedef int (*X509_STORE_CTX_get_crl_fn)(X509_STORE_CTX *ctx, X509_CRL **crl, X509 *x); typedef int (*X509_STORE_CTX_check_crl_fn)(X509_STORE_CTX *ctx, X509_CRL *crl); +// X509_STORE_set_depth configures |store| to, by default, limit certificate +// chains to |depth| intermediate certificates. This count excludes both the +// target certificate and the trust anchor (root certificate). OPENSSL_EXPORT int X509_STORE_set_depth(X509_STORE *store, int depth); +// X509_STORE_CTX_set_depth configures |ctx| to, by default, limit certificate +// chains to |depth| intermediate certificates. This count excludes both the +// target certificate and the trust anchor (root certificate). OPENSSL_EXPORT void X509_STORE_CTX_set_depth(X509_STORE_CTX *ctx, int depth); #define X509_STORE_CTX_set_app_data(ctx, data) \ @@ -3922,14 +3928,12 @@ OPENSSL_EXPORT int X509_STORE_set1_param(X509_STORE *store, // // As of writing these late defaults are a depth limit (see // |X509_VERIFY_PARAM_set_depth|) and the |X509_V_FLAG_TRUSTED_FIRST| flag. This -// warning does not apply if the parameters were set in |store|. That is, -// callers may safely set a concrete depth limit in |store|, but unlimited depth -// must be configured at |X509_STORE_CTX|. +// warning does not apply if the parameters were set in |store|. // // TODO(crbug.com/boringssl/441): This behavior is very surprising. Can we -// remove this notion of late defaults? A depth limit of 100 can probably be -// applied unconditionally. |X509_V_FLAG_TRUSTED_FIRST| is mostly a workaround -// for poor path-building. +// remove this notion of late defaults? The unsettable value at |X509_STORE| is +// -1, which rejects everything but explicitly-trusted self-signed certificates. +// |X509_V_FLAG_TRUSTED_FIRST| is mostly a workaround for poor path-building. OPENSSL_EXPORT X509_VERIFY_PARAM *X509_STORE_get0_param(X509_STORE *store); // X509_STORE_set_verify_cb acts like |X509_STORE_CTX_set_verify_cb| but sets @@ -4170,6 +4174,10 @@ OPENSSL_EXPORT int X509_VERIFY_PARAM_set_purpose(X509_VERIFY_PARAM *param, int purpose); OPENSSL_EXPORT int X509_VERIFY_PARAM_set_trust(X509_VERIFY_PARAM *param, int trust); + +// X509_VERIFY_PARAM_set_depth configures |param| to limit certificate chains to +// |depth| intermediate certificates. This count excludes both the target +// certificate and the trust anchor (root certificate). OPENSSL_EXPORT void X509_VERIFY_PARAM_set_depth(X509_VERIFY_PARAM *param, int depth); @@ -4270,6 +4278,8 @@ OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_ip(X509_VERIFY_PARAM *param, OPENSSL_EXPORT int X509_VERIFY_PARAM_set1_ip_asc(X509_VERIFY_PARAM *param, const char *ipasc); +// X509_VERIFY_PARAM_get_depth returns the maximum depth configured in |param|. +// See |X509_VERIFY_PARAM_set_depth|. OPENSSL_EXPORT int X509_VERIFY_PARAM_get_depth(const X509_VERIFY_PARAM *param); typedef void *(*X509V3_EXT_NEW)(void); From 96b9299b9aa853e7fd380567978bfc37c357dcb0 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 15 Dec 2023 18:28:54 -0500 Subject: [PATCH 13/27] Skip emitting empty
 blocks in documentation

We sometimes have decl-less comments interspersed in headers. Avoid the
empty patch of blue when they get rendered.

Change-Id: I6668946d07349febd6d28bbe7d4fd8d2426adca9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64927
Auto-Submit: David Benjamin 
Commit-Queue: David Benjamin 
Reviewed-by: Bob Beck 
Commit-Queue: Bob Beck 
(cherry picked from commit 56112cc01f8be29578d52c1c2af6f481c3d77bd2)
---
 util/doc.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/doc.go b/util/doc.go
index a76a4d0912..0449a210ab 100644
--- a/util/doc.go
+++ b/util/doc.go
@@ -649,7 +649,7 @@ func generate(outPath string, config *Config) (map[string]string, error) {
           {{range .Comment}}
             

{{. | markupPipeWords | newlinesToBR | markupFirstWord | markupRFC}}

{{end}} -
{{.Decl}}
+ {{if .Decl}}
{{.Decl}}
{{end}} {{end}} From c19cdbfef92f1f4db6e21f041635c3f980b11828 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 15 Dec 2023 18:23:43 -0500 Subject: [PATCH 14/27] Add conf.h to the documentation output We don't want anyone using it, but we probably should render all our documented headers. I've grouped it with the rest of the legacy X.509 stack. Change-Id: If01dfd19c71598f47a40cd334b0fd7ef3e00685d Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64928 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 9c821af02f2c38ac9ba58708164ccb6c4d7654bc) --- include/openssl/conf.h | 5 ++++- util/doc.config | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/include/openssl/conf.h b/include/openssl/conf.h index e02f76939d..2a829ae9e2 100644 --- a/include/openssl/conf.h +++ b/include/openssl/conf.h @@ -67,7 +67,9 @@ extern "C" { #endif -// Config files look like: +// Config files. +// +// This library handles OpenSSL's config files, which look like: // // # Comment // @@ -82,6 +84,7 @@ extern "C" { // untrusted input as a config file risks string injection and denial of service // vulnerabilities. + struct conf_value_st { char *section; char *name; diff --git a/util/doc.config b/util/doc.config index 68c739e20c..340130e8c1 100644 --- a/util/doc.config +++ b/util/doc.config @@ -58,6 +58,7 @@ "Name": "Legacy ASN.1 and X.509 implementation (documentation in progress)", "Headers": [ "include/openssl/asn1.h", + "include/openssl/conf.h", "include/openssl/x509.h" ] },{ From e20e087ef6cd30c658b94cdddab715a7e61385b3 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 16 Dec 2023 21:07:46 -0500 Subject: [PATCH 15/27] Restore the X509 ASN1_ITEM https://boringssl-review.googlesource.com/c/boringssl/+/63946 removed it, but sadly wpa_supplicant depends on it. Change-Id: Ib3aca5269d740457ba83ca529b797bfb4a089763 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64907 Reviewed-by: Bob Beck Auto-Submit: David Benjamin Commit-Queue: Bob Beck (cherry picked from commit faac623b09d6b14fbb06fe5150016de78b359eea) --- crypto/asn1/asn1_test.cc | 162 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 162 insertions(+) diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index c8cea4613c..25e19d32eb 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -2851,4 +2851,166 @@ TEST(ASN1Test, OptionalChoice) { TestSerialize(obj.get(), i2d_OPTIONAL_CHOICE, kTrue); } +struct EMBED_X509_ALGOR { + X509_ALGOR *simple; + X509_ALGOR *opt; + STACK_OF(X509_ALGOR) *seq; +}; + +struct EMBED_X509_NAME { + X509_NAME *simple; + X509_NAME *opt; + STACK_OF(X509_NAME) *seq; +}; + +struct EMBED_X509 { + X509 *simple; + X509 *opt; + STACK_OF(X509) *seq; +}; + +DECLARE_ASN1_FUNCTIONS(EMBED_X509_ALGOR) +ASN1_SEQUENCE(EMBED_X509_ALGOR) = { + ASN1_SIMPLE(EMBED_X509_ALGOR, simple, X509_ALGOR), + ASN1_EXP_OPT(EMBED_X509_ALGOR, opt, X509_ALGOR, 0), + ASN1_IMP_SEQUENCE_OF_OPT(EMBED_X509_ALGOR, seq, X509_ALGOR, 1), +} ASN1_SEQUENCE_END(EMBED_X509_ALGOR) +IMPLEMENT_ASN1_FUNCTIONS(EMBED_X509_ALGOR) + +DECLARE_ASN1_FUNCTIONS(EMBED_X509_NAME) +ASN1_SEQUENCE(EMBED_X509_NAME) = { + ASN1_SIMPLE(EMBED_X509_NAME, simple, X509_NAME), + ASN1_EXP_OPT(EMBED_X509_NAME, opt, X509_NAME, 0), + ASN1_IMP_SEQUENCE_OF_OPT(EMBED_X509_NAME, seq, X509_NAME, 1), +} ASN1_SEQUENCE_END(EMBED_X509_NAME) +IMPLEMENT_ASN1_FUNCTIONS(EMBED_X509_NAME) + +DECLARE_ASN1_FUNCTIONS(EMBED_X509) +ASN1_SEQUENCE(EMBED_X509) = { + ASN1_SIMPLE(EMBED_X509, simple, X509), + ASN1_EXP_OPT(EMBED_X509, opt, X509, 0), + ASN1_IMP_SEQUENCE_OF_OPT(EMBED_X509, seq, X509, 1), +} ASN1_SEQUENCE_END(EMBED_X509) +IMPLEMENT_ASN1_FUNCTIONS(EMBED_X509) + +template +void TestEmbedType(bssl::Span inp, + int (*i2d)(MaybeConstT *, uint8_t **), + EmbedT *(*embed_new)(), void (*embed_free)(EmbedT *), + EmbedT *(*d2i_embed)(EmbedT **, const uint8_t **, long), + int (*i2d_embed)(EmbedT *, uint8_t **), + size_t (*sk_num)(const StackT *), + T *(*sk_value)(const StackT *, size_t)) { + std::unique_ptr obj(nullptr, embed_free); + + // Test only the first field present. + bssl::ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 64)); + CBB seq; + ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE)); + ASSERT_TRUE(CBB_add_bytes(&seq, inp.data(), inp.size())); + ASSERT_TRUE(CBB_flush(cbb.get())); + const uint8_t *ptr = CBB_data(cbb.get()); + obj.reset(d2i_embed(nullptr, &ptr, CBB_len(cbb.get()))); + ASSERT_TRUE(obj); + ASSERT_TRUE(obj->simple); + // Test the field was parsed correctly by reserializing it. + TestSerialize(obj->simple, i2d, inp); + EXPECT_FALSE(obj->opt); + EXPECT_FALSE(obj->seq); + TestSerialize(obj.get(), i2d_embed, + {CBB_data(cbb.get()), CBB_len(cbb.get())}); + + // Test all fields present. + cbb.Reset(); + ASSERT_TRUE(CBB_init(cbb.get(), 64)); + ASSERT_TRUE(CBB_add_asn1(cbb.get(), &seq, CBS_ASN1_SEQUENCE)); + ASSERT_TRUE(CBB_add_bytes(&seq, inp.data(), inp.size())); + CBB child; + ASSERT_TRUE(CBB_add_asn1( + &seq, &child, CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 0)); + ASSERT_TRUE(CBB_add_bytes(&child, inp.data(), inp.size())); + ASSERT_TRUE(CBB_add_asn1( + &seq, &child, CBS_ASN1_CONTEXT_SPECIFIC | CBS_ASN1_CONSTRUCTED | 1)); + ASSERT_TRUE(CBB_add_bytes(&child, inp.data(), inp.size())); + ASSERT_TRUE(CBB_add_bytes(&child, inp.data(), inp.size())); + ASSERT_TRUE(CBB_flush(cbb.get())); + ptr = CBB_data(cbb.get()); + obj.reset(d2i_embed(nullptr, &ptr, CBB_len(cbb.get()))); + ASSERT_TRUE(obj); + ASSERT_TRUE(obj->simple); + TestSerialize(obj->simple, i2d, inp); + ASSERT_TRUE(obj->opt); + TestSerialize(obj->opt, i2d, inp); + ASSERT_EQ(sk_num(obj->seq), 2u); + TestSerialize(sk_value(obj->seq, 0), i2d, inp); + TestSerialize(sk_value(obj->seq, 1), i2d, inp); + TestSerialize(obj.get(), i2d_embed, + {CBB_data(cbb.get()), CBB_len(cbb.get())}); +} + +// Test that X.509 types defined in this library can be embedded into other +// types, as we rewrite them away from the templating system. +TEST(ASN1Test, EmbedTypes) { + static const uint8_t kTestAlg[] = {0x30, 0x09, 0x06, 0x07, 0x2a, 0x86, + 0x48, 0xce, 0x3d, 0x04, 0x01}; + TestEmbedType(kTestAlg, i2d_X509_ALGOR, EMBED_X509_ALGOR_new, + EMBED_X509_ALGOR_free, d2i_EMBED_X509_ALGOR, + i2d_EMBED_X509_ALGOR, sk_X509_ALGOR_num, sk_X509_ALGOR_value); + + static const uint8_t kTestName[] = { + 0x30, 0x45, 0x31, 0x0b, 0x30, 0x09, 0x06, 0x03, 0x55, 0x04, 0x06, 0x13, + 0x02, 0x41, 0x55, 0x31, 0x13, 0x30, 0x11, 0x06, 0x03, 0x55, 0x04, 0x08, + 0x0c, 0x0a, 0x53, 0x6f, 0x6d, 0x65, 0x2d, 0x53, 0x74, 0x61, 0x74, 0x65, + 0x31, 0x21, 0x30, 0x1f, 0x06, 0x03, 0x55, 0x04, 0x0a, 0x0c, 0x18, 0x49, + 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x65, 0x74, 0x20, 0x57, 0x69, 0x64, 0x67, + 0x69, 0x74, 0x73, 0x20, 0x50, 0x74, 0x79, 0x20, 0x4c, 0x74, 0x64}; + TestEmbedType(kTestName, i2d_X509_NAME, EMBED_X509_NAME_new, + EMBED_X509_NAME_free, d2i_EMBED_X509_NAME, i2d_EMBED_X509_NAME, + sk_X509_NAME_num, sk_X509_NAME_value); + + static const uint8_t kTestCert[] = { + 0x30, 0x82, 0x01, 0xcf, 0x30, 0x82, 0x01, 0x76, 0xa0, 0x03, 0x02, 0x01, + 0x02, 0x02, 0x09, 0x00, 0xd9, 0x4c, 0x04, 0xda, 0x49, 0x7d, 0xbf, 0xeb, + 0x30, 0x09, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x01, 0x30, + 0x45, 0x31, 0x0b, 0x30, 0x09, 0x06, 0x03, 0x55, 0x04, 0x06, 0x13, 0x02, + 0x41, 0x55, 0x31, 0x13, 0x30, 0x11, 0x06, 0x03, 0x55, 0x04, 0x08, 0x0c, + 0x0a, 0x53, 0x6f, 0x6d, 0x65, 0x2d, 0x53, 0x74, 0x61, 0x74, 0x65, 0x31, + 0x21, 0x30, 0x1f, 0x06, 0x03, 0x55, 0x04, 0x0a, 0x0c, 0x18, 0x49, 0x6e, + 0x74, 0x65, 0x72, 0x6e, 0x65, 0x74, 0x20, 0x57, 0x69, 0x64, 0x67, 0x69, + 0x74, 0x73, 0x20, 0x50, 0x74, 0x79, 0x20, 0x4c, 0x74, 0x64, 0x30, 0x1e, + 0x17, 0x0d, 0x31, 0x34, 0x30, 0x34, 0x32, 0x33, 0x32, 0x33, 0x32, 0x31, + 0x35, 0x37, 0x5a, 0x17, 0x0d, 0x31, 0x34, 0x30, 0x35, 0x32, 0x33, 0x32, + 0x33, 0x32, 0x31, 0x35, 0x37, 0x5a, 0x30, 0x45, 0x31, 0x0b, 0x30, 0x09, + 0x06, 0x03, 0x55, 0x04, 0x06, 0x13, 0x02, 0x41, 0x55, 0x31, 0x13, 0x30, + 0x11, 0x06, 0x03, 0x55, 0x04, 0x08, 0x0c, 0x0a, 0x53, 0x6f, 0x6d, 0x65, + 0x2d, 0x53, 0x74, 0x61, 0x74, 0x65, 0x31, 0x21, 0x30, 0x1f, 0x06, 0x03, + 0x55, 0x04, 0x0a, 0x0c, 0x18, 0x49, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x65, + 0x74, 0x20, 0x57, 0x69, 0x64, 0x67, 0x69, 0x74, 0x73, 0x20, 0x50, 0x74, + 0x79, 0x20, 0x4c, 0x74, 0x64, 0x30, 0x59, 0x30, 0x13, 0x06, 0x07, 0x2a, + 0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, + 0x3d, 0x03, 0x01, 0x07, 0x03, 0x42, 0x00, 0x04, 0xe6, 0x2b, 0x69, 0xe2, + 0xbf, 0x65, 0x9f, 0x97, 0xbe, 0x2f, 0x1e, 0x0d, 0x94, 0x8a, 0x4c, 0xd5, + 0x97, 0x6b, 0xb7, 0xa9, 0x1e, 0x0d, 0x46, 0xfb, 0xdd, 0xa9, 0xa9, 0x1e, + 0x9d, 0xdc, 0xba, 0x5a, 0x01, 0xe7, 0xd6, 0x97, 0xa8, 0x0a, 0x18, 0xf9, + 0xc3, 0xc4, 0xa3, 0x1e, 0x56, 0xe2, 0x7c, 0x83, 0x48, 0xdb, 0x16, 0x1a, + 0x1c, 0xf5, 0x1d, 0x7e, 0xf1, 0x94, 0x2d, 0x4b, 0xcf, 0x72, 0x22, 0xc1, + 0xa3, 0x50, 0x30, 0x4e, 0x30, 0x1d, 0x06, 0x03, 0x55, 0x1d, 0x0e, 0x04, + 0x16, 0x04, 0x14, 0xab, 0x84, 0xd2, 0xac, 0xab, 0x95, 0xf0, 0x82, 0x4e, + 0x16, 0x78, 0x07, 0x55, 0x57, 0x5f, 0xe4, 0x26, 0x8d, 0x82, 0xd1, 0x30, + 0x1f, 0x06, 0x03, 0x55, 0x1d, 0x23, 0x04, 0x18, 0x30, 0x16, 0x80, 0x14, + 0xab, 0x84, 0xd2, 0xac, 0xab, 0x95, 0xf0, 0x82, 0x4e, 0x16, 0x78, 0x07, + 0x55, 0x57, 0x5f, 0xe4, 0x26, 0x8d, 0x82, 0xd1, 0x30, 0x0c, 0x06, 0x03, + 0x55, 0x1d, 0x13, 0x04, 0x05, 0x30, 0x03, 0x01, 0x01, 0xff, 0x30, 0x09, + 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x04, 0x01, 0x03, 0x48, 0x00, + 0x30, 0x45, 0x02, 0x21, 0x00, 0xf2, 0xa0, 0x35, 0x5e, 0x51, 0x3a, 0x36, + 0xc3, 0x82, 0x79, 0x9b, 0xee, 0x27, 0x50, 0x85, 0x8e, 0x70, 0x06, 0x74, + 0x95, 0x57, 0xd2, 0x29, 0x74, 0x00, 0xf4, 0xbe, 0x15, 0x87, 0x5d, 0xc4, + 0x07, 0x02, 0x20, 0x7c, 0x1e, 0x79, 0x14, 0x6a, 0x21, 0x83, 0xf0, 0x7a, + 0x74, 0x68, 0x79, 0x5f, 0x14, 0x99, 0x9a, 0x68, 0xb4, 0xf1, 0xcb, 0x9e, + 0x15, 0x5e, 0xe6, 0x1f, 0x32, 0x52, 0x61, 0x5e, 0x75, 0xc9, 0x14}; + TestEmbedType(kTestCert, i2d_X509, EMBED_X509_new, EMBED_X509_free, + d2i_EMBED_X509, i2d_EMBED_X509, sk_X509_num, sk_X509_value); +} + #endif // !WINDOWS || !SHARED_LIBRARY From 64c88349deb228218e0f9dc25c788c9b2f0adfef Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 16 Dec 2023 02:43:26 -0500 Subject: [PATCH 16/27] Give time.h a title and move to low-level infra group Change-Id: I4cb50bda726fcc0f22fcb53dca92cf297aeea169 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64929 Commit-Queue: David Benjamin Reviewed-by: Bob Beck (cherry picked from commit 3d5a848d2fc081872123ba3d6e2b0f653281aa13) --- include/openssl/time.h | 45 ++++++++++++++++++++++++++++++++++++++++++ util/doc.config | 3 ++- 2 files changed, 47 insertions(+), 1 deletion(-) create mode 100644 include/openssl/time.h diff --git a/include/openssl/time.h b/include/openssl/time.h new file mode 100644 index 0000000000..50db22d324 --- /dev/null +++ b/include/openssl/time.h @@ -0,0 +1,45 @@ +/* Copyright (c) 2022, Google Inc. + * + * Permission to use, copy, modify, and/or distribute this software for any + * purpose with or without fee is hereby granted, provided that the above + * copyright notice and this permission notice appear in all copies. + * + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY + * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION + * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN + * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ + +#ifndef OPENSSL_HEADER_TIME_H +#define OPENSSL_HEADER_TIME_H + +#include + +#include + +#if defined(__cplusplus) +extern "C" { +#endif + + +// Time functions. + + +// OPENSSL_posix_to_tm converts a int64_t POSIX time value in |time|, which must +// be in the range of year 0000 to 9999, to a broken out time value in |tm|. It +// returns one on success and zero on error. +OPENSSL_EXPORT int OPENSSL_posix_to_tm(int64_t time, struct tm *out_tm); + +// OPENSSL_tm_to_posix converts a time value between the years 0 and 9999 in +// |tm| to a POSIX time value in |out|. One is returned on success, zero is +// returned on failure. It is a failure if |tm| contains out of range values. +OPENSSL_EXPORT int OPENSSL_tm_to_posix(const struct tm *tm, int64_t *out); + + +#if defined(__cplusplus) +} // extern C +#endif + +#endif // OPENSSL_HEADER_TIME_H diff --git a/util/doc.config b/util/doc.config index 340130e8c1..9663eb5b07 100644 --- a/util/doc.config +++ b/util/doc.config @@ -16,7 +16,8 @@ "include/openssl/pool.h", "include/openssl/rand.h", "include/openssl/service_indicator.h", - "include/openssl/stack.h" + "include/openssl/stack.h", + "include/openssl/time.h" ] },{ "Name": "Low-level crypto primitives", From cb404c57d3c0cadbb66bd69c5355a23d4b928324 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Thu, 14 Dec 2023 14:27:27 -0500 Subject: [PATCH 17/27] Support lists and code blocks in doc.go Our documentation comments already include examples of code blocks and lists, they just don't get rendered right. We also have things that were trying to be lists but aren't. Go ahead and add support for it, and fix the handful of list-like things that didn't get rendered as lists. I took inspiration from CommonMark (https://spec.commonmark.org/0.30/) to resolve questions such as whether blank lines are needed between lists, etc., but this does not support any kind of nesting and is still far from a CommonMark parser. Aligning with CommonMark leaves the door open to pulling in a real Markdown parser if we start to need too many features. I've also borrowed the "block" terminology from CommonMark. One ambiguity of note: whether lists may interrupt paragraphs (i.e. without a blank line in between) is a little thorny. If we say no, this doesn't work: Callers should heed the following warnings: 1) Don't use the function 2) Seriously, don't use this function 3) This function is a bad idea But if we say yes, this renders wrong: This function parses an X.509 certificate (see RFC 5280) into an X509 object. We have examples of both in existing comments, though we could easily add a blank line in the former or rewrap the latter. CommonMark has a discussion on this in https://spec.commonmark.org/0.30/#lists CommonMark says yes, but with a hack that only lists starting with 1 can interrupt paragraphs. Since we're unlikely to cite RFC 1, I've matched for now, but we may want to revisit this if it gets to be a pain. I could imagine this becoming a problem: This function, on success, does some stuff and returns 1. Otherwise, it returns 0. But that looks a little weird and we usually spell out "one" and "zero". I printed all the lists we detected in existing comments, and this has not happened so far. I've also required fewer spaces than CommonMark to trigger a code block. CommonMark uses four, but four spaces plus a leading "//" and a " " is quite a lot. For now I'm not stripping the spaces after the comment marker at comment extraction time and then requiring three spaces, so two spaces relative to normal text. This is mostly to match what we've currently been doing, but we can always change it and our comments later. Change-Id: Ic61a8e93491ed96aba755aec2a5f32914bdc42ae Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64930 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit a942d572073e98944200e154597442796fdb13de) --- include/openssl/bn.h | 6 +- include/openssl/curve25519.h | 4 +- include/openssl/ec.h | 10 +- include/openssl/ssl.h | 28 ++-- util/doc.css | 11 +- util/doc.go | 283 +++++++++++++++++++++++++++-------- 6 files changed, 249 insertions(+), 93 deletions(-) diff --git a/include/openssl/bn.h b/include/openssl/bn.h index 77c2505e09..9ec83c8fa6 100644 --- a/include/openssl/bn.h +++ b/include/openssl/bn.h @@ -675,11 +675,11 @@ OPENSSL_EXPORT int BN_pseudo_rand_range(BIGNUM *rnd, const BIGNUM *range); // The callback receives the address of that |BN_GENCB| structure as its last // argument and the user is free to put an arbitrary pointer in |arg|. The other // arguments are set as follows: -// event=BN_GENCB_GENERATED, n=i: after generating the i'th possible prime +// - event=BN_GENCB_GENERATED, n=i: after generating the i'th possible prime // number. -// event=BN_GENCB_PRIME_TEST, n=-1: when finished trial division primality +// - event=BN_GENCB_PRIME_TEST, n=-1: when finished trial division primality // checks. -// event=BN_GENCB_PRIME_TEST, n=i: when the i'th primality test has finished. +// - event=BN_GENCB_PRIME_TEST, n=i: when the i'th primality test has finished. // // The callback can return zero to abort the generation progress or one to // allow it to continue. diff --git a/include/openssl/curve25519.h b/include/openssl/curve25519.h index e7c88fa9c9..6c79228c11 100644 --- a/include/openssl/curve25519.h +++ b/include/openssl/curve25519.h @@ -165,10 +165,10 @@ OPENSSL_EXPORT int SPAKE2_generate_msg(SPAKE2_CTX *ctx, uint8_t *out, // |*out_key_len| to the number of bytes written. // // The resulting keying material is suitable for: -// a) Using directly in a key-confirmation step: i.e. each side could +// - Using directly in a key-confirmation step: i.e. each side could // transmit a hash of their role, a channel-binding value and the key // material to prove to the other side that they know the shared key. -// b) Using as input keying material to HKDF to generate a variety of subkeys +// - Using as input keying material to HKDF to generate a variety of subkeys // for encryption etc. // // If |max_out_key_key| is smaller than the amount of key material generated diff --git a/include/openssl/ec.h b/include/openssl/ec.h index e52d3de9fc..f1b9a6b2d0 100644 --- a/include/openssl/ec.h +++ b/include/openssl/ec.h @@ -124,11 +124,11 @@ OPENSSL_EXPORT const EC_GROUP *EC_group_secp256k1(void); // calling |EC_GROUP_free| is optional. // // The supported NIDs are (see crypto/fipsmodule/ec/ec.c): -// NID_secp224r1 (NIST P-224), -// NID_X9_62_prime256v1 (NIST P-256), -// NID_secp384r1 (NIST P-384), -// NID_secp521r1 (NIST P-521), -// NID_secp256k1 (SEC/ANSI P-256 K1) +// - |NID_secp224r1| (NIST P-224) +// - |NID_X9_62_prime256v1| (NIST P-256) +// - |NID_secp384r1| (NIST P-384) +// - |NID_secp521r1| (NIST P-521) +// - |NID_secp256k1| (SEC/ANSI P-256 K1) // // Calling this function causes all four curves to be linked into the binary. // Prefer calling |EC_group_*| to allow the static linker to drop unused curves. diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 364fbbd96e..556b21c0e4 100644 --- a/include/openssl/ssl.h +++ b/include/openssl/ssl.h @@ -1603,19 +1603,19 @@ OPENSSL_EXPORT size_t SSL_get_all_standard_cipher_names(const char **out, // // Available opcodes are: // -// The empty opcode enables and appends all matching disabled ciphers to the +// - The empty opcode enables and appends all matching disabled ciphers to the // end of the enabled list. The newly appended ciphers are ordered relative to // each other matching their order in the disabled list. // -// |-| disables all matching enabled ciphers and prepends them to the disabled +// - |-| disables all matching enabled ciphers and prepends them to the disabled // list, with relative order from the enabled list preserved. This means the // most recently disabled ciphers get highest preference relative to other // disabled ciphers if re-enabled. // -// |+| moves all matching enabled ciphers to the end of the enabled list, with +// - |+| moves all matching enabled ciphers to the end of the enabled list, with // relative order preserved. // -// |!| deletes all matching ciphers, enabled or not, from either list. Deleted +// - |!| deletes all matching ciphers, enabled or not, from either list. Deleted // ciphers will not matched by future operations. // // A selector may be a specific cipher (using either the standard or OpenSSL @@ -1625,38 +1625,38 @@ OPENSSL_EXPORT size_t SSL_get_all_standard_cipher_names(const char **out, // // Available cipher rules are: // -// |ALL| matches all ciphers. +// - |ALL| matches all ciphers. // -// |kRSA|, |kDHE|, |kECDHE|, and |kPSK| match ciphers using plain RSA, DHE, +// - |kRSA|, |kDHE|, |kECDHE|, and |kPSK| match ciphers using plain RSA, DHE, // ECDHE, and plain PSK key exchanges, respectively. Note that ECDHE_PSK is // matched by |kECDHE| and not |kPSK|. // -// |aRSA|, |aECDSA|, and |aPSK| match ciphers authenticated by RSA, ECDSA, and +// - |aRSA|, |aECDSA|, and |aPSK| match ciphers authenticated by RSA, ECDSA, and // a pre-shared key, respectively. // -// |RSA|, |DHE|, |ECDHE|, |PSK|, |ECDSA|, and |PSK| are aliases for the +// - |RSA|, |DHE|, |ECDHE|, |PSK|, |ECDSA|, and |PSK| are aliases for the // corresponding |k*| or |a*| cipher rule. |RSA| is an alias for |kRSA|, not // |aRSA|. // -// |3DES|, |AES128|, |AES256|, |AES|, |AESGCM|, |CHACHA20| match ciphers +// - |3DES|, |AES128|, |AES256|, |AES|, |AESGCM|, |CHACHA20| match ciphers // whose bulk cipher use the corresponding encryption scheme. Note that // |AES|, |AES128|, and |AES256| match both CBC and GCM ciphers. // -// |SHA1|, and its alias |SHA|, match legacy cipher suites using HMAC-SHA1. +// - |SHA1|, and its alias |SHA|, match legacy cipher suites using HMAC-SHA1. // // Although implemented, authentication-only ciphers match no rules and must be // explicitly selected by name. // // Deprecated cipher rules: // -// |kEDH|, |EDH|, |kEECDH|, and |EECDH| are legacy aliases for |kDHE|, |DHE|, +// - |kEDH|, |EDH|, |kEECDH|, and |EECDH| are legacy aliases for |kDHE|, |DHE|, // |kECDHE|, and |ECDHE|, respectively. // -// |HIGH| is an alias for |ALL|. +// - |HIGH| is an alias for |ALL|. // -// |FIPS| is an alias for |HIGH|. +// - |FIPS| is an alias for |HIGH|. // -// |SSLv3| and |TLSv1| match ciphers available in TLS 1.1 or earlier. +// - |SSLv3| and |TLSv1| match ciphers available in TLS 1.1 or earlier. // |TLSv1_2| matches ciphers new in TLS 1.2. This is confusing and should not // be used. // diff --git a/util/doc.css b/util/doc.css index a868e44445..f176f259b7 100644 --- a/util/doc.css +++ b/util/doc.css @@ -16,12 +16,13 @@ div.title { margin-bottom: 2em; } -ol { +ol.toc { list-style: none; + padding-left: 0; margin-bottom: 4em; } -li a { +ol.toc li a { color: black; } @@ -49,12 +50,16 @@ div.decl p:first-child .first-word { font-size: 1.5em; } -.section pre { +pre.code { background-color: #b2c9db; padding: 5px; border-radius: 5px; } +.comment pre { + margin-left: 2em; +} + td { padding: 2px; } diff --git a/util/doc.go b/util/doc.go index 0449a210ab..0d2e2628b4 100644 --- a/util/doc.go +++ b/util/doc.go @@ -18,7 +18,9 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" + "unicode" ) // Config describes the structure of the config JSON file. @@ -41,7 +43,7 @@ type HeaderFile struct { Name string // Preamble contains a comment for the file as a whole. Each string // is a separate paragraph. - Preamble []string + Preamble []CommentBlock Sections []HeaderSection // AllDecls maps all decls to their URL fragments. AllDecls map[string]string @@ -49,7 +51,7 @@ type HeaderFile struct { type HeaderSection struct { // Preamble contains a comment for a group of functions. - Preamble []string + Preamble []CommentBlock Decls []HeaderDecl // Anchor, if non-empty, is the URL fragment to use in anchor tags. Anchor string @@ -62,7 +64,7 @@ type HeaderDecl struct { // Comment contains a comment for a specific function. Each string is a // paragraph. Some paragraph may contain \n runes to indicate that they // are preformatted. - Comment []string + Comment []CommentBlock // Name contains the name of the function, if it could be extracted. Name string // Decl contains the preformatted C declaration itself. @@ -71,6 +73,20 @@ type HeaderDecl struct { Anchor string } +type CommentBlockType int + +const ( + CommentParagraph CommentBlockType = iota + CommentOrderedListItem + CommentBulletListItem + CommentCode +) + +type CommentBlock struct { + Type CommentBlockType + Paragraph string +} + const ( cppGuard = "#if defined(__cplusplus)" commentStart = "/* " @@ -95,7 +111,7 @@ func commentSubject(line string) string { return line[:idx] } -func extractComment(lines []string, lineNo int) (comment []string, rest []string, restLineNo int, err error) { +func extractCommentLines(lines []string, lineNo int) (comment []string, rest []string, restLineNo int, err error) { if len(lines) == 0 { return nil, lines, lineNo, nil } @@ -109,22 +125,19 @@ func extractComment(lines []string, lineNo int) (comment []string, rest []string } else if !strings.HasPrefix(rest[0], lineComment) { panic("extractComment called on non-comment") } - commentParagraph := rest[0][len(commentStart):] + comment = []string{rest[0][len(commentStart):]} rest = rest[1:] restLineNo++ for len(rest) > 0 { if isBlock { - i := strings.Index(commentParagraph, commentEnd) - if i >= 0 { - if i != len(commentParagraph)-len(commentEnd) { + last := &comment[len(comment)-1] + if i := strings.Index(*last, commentEnd); i >= 0 { + if i != len(*last)-len(commentEnd) { err = fmt.Errorf("garbage after comment end on line %d", restLineNo) return } - commentParagraph = commentParagraph[:i] - if len(commentParagraph) > 0 { - comment = append(comment, commentParagraph) - } + *last = (*last)[:i] return } } @@ -136,36 +149,136 @@ func extractComment(lines []string, lineNo int) (comment []string, rest []string return } } else if !strings.HasPrefix(line, "//") { - if len(commentParagraph) > 0 { - comment = append(comment, commentParagraph) - } return } - if len(line) == 2 || !isBlock || line[2] != '/' { - line = line[2:] + comment = append(comment, line[2:]) + rest = rest[1:] + restLineNo++ + } + + err = errors.New("hit EOF in comment") + return +} + +func removeBulletListMarker(line string) (string, bool) { + orig := line + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, "+ ") && !strings.HasPrefix(line, "- ") && !strings.HasPrefix(line, "* ") { + return orig, false + } + return line[2:], true +} + +func removeOrderedListMarker(line string) (rest string, num int, ok bool) { + orig := line + line = strings.TrimSpace(line) + if len(line) == 0 || !unicode.IsDigit(rune(line[0])) { + return orig, -1, false + } + + l := 0 + for l < len(line) && unicode.IsDigit(rune(line[l])) { + l++ + } + num, err := strconv.Atoi(line[:l]) + if err != nil { + return orig, -1, false + } + + line = line[l:] + if line, ok := strings.CutPrefix(line, ". "); ok { + return line, num, true + } + if line, ok := strings.CutPrefix(line, ") "); ok { + return line, num, true + } + + return orig, -1, false +} + +func removeCodeIndent(line string) (string, bool) { + return strings.CutPrefix(line, " ") +} + +func extractComment(lines []string, lineNo int) (comment []CommentBlock, rest []string, restLineNo int, err error) { + commentLines, rest, restLineNo, err := extractCommentLines(lines, lineNo) + if err != nil { + return + } + + // This syntax and parsing algorithm is loosely inspired by CommonMark, + // but reduced to a small subset with no nesting. Blocks being open vs. + // closed can be tracked implicitly. We're also much slopplier about how + // indentation. Additionally, rather than grouping list items into + // lists, our parser just emits a list items, which are grouped later at + // rendering time. + // + // If we later need more features, such as nested lists, this can evolve + // into a more complex implementation. + var numBlankLines int + for _, line := range commentLines { + // Defer blank lines until we know the next element. + if len(strings.TrimSpace(line)) == 0 { + numBlankLines++ + continue } - if strings.HasPrefix(line, " ") { - /* Identing the lines of a paragraph marks them as - * preformatted. */ - if len(commentParagraph) > 0 { - commentParagraph += "\n" + + blankLinesSkipped := numBlankLines + numBlankLines = 0 + + // Attempt to continue the previous block. + if len(comment) > 0 { + last := &comment[len(comment)-1] + if last.Type == CommentCode { + l, ok := removeCodeIndent(line) + if ok { + for i := 0; i < blankLinesSkipped; i++ { + last.Paragraph += "\n" + } + last.Paragraph += l + "\n" + continue + } + } else if blankLinesSkipped == 0 { + _, isBulletList := removeBulletListMarker(line) + _, num, isOrderedList := removeOrderedListMarker(line) + if isOrderedList && last.Type == CommentParagraph && num != 1 { + // A list item can only interrupt a paragraph if the number is one. + // See the discussion in https://spec.commonmark.org/0.30/#lists. + // This avoids wrapping like "(See RFC\n5280)" turning into a list. + isOrderedList = false + } + if !isBulletList && !isOrderedList { + // This is a continuation line of the previous paragraph. + last.Paragraph += " " + strings.TrimSpace(line) + continue + } } - line = line[3:] } - if len(line) > 0 { - commentParagraph = commentParagraph + line - if len(commentParagraph) > 0 && commentParagraph[0] == ' ' { - commentParagraph = commentParagraph[1:] - } + + // Make a new block. + if line, ok := removeBulletListMarker(line); ok { + comment = append(comment, CommentBlock{ + Type: CommentBulletListItem, + Paragraph: strings.TrimSpace(line), + }) + } else if line, _, ok := removeOrderedListMarker(line); ok { + comment = append(comment, CommentBlock{ + Type: CommentOrderedListItem, + Paragraph: strings.TrimSpace(line), + }) + } else if line, ok := removeCodeIndent(line); ok { + comment = append(comment, CommentBlock{ + Type: CommentCode, + Paragraph: line + "\n", + }) } else { - comment = append(comment, commentParagraph) - commentParagraph = "" + comment = append(comment, CommentBlock{ + Type: CommentParagraph, + Paragraph: strings.TrimSpace(line), + }) } - rest = rest[1:] - restLineNo++ } - err = errors.New("hit EOF in comment") return } @@ -390,7 +503,8 @@ func (config *Config) parseHeader(path string) (*HeaderFile, error) { return nil, err } if len(rest) > 0 && len(rest[0]) == 0 { - anchor := sanitizeAnchor(firstSentence(comment)) + heading := firstSentence(comment) + anchor := sanitizeAnchor(heading) if len(anchor) > 0 { if _, ok := allAnchors[anchor]; ok { return nil, fmt.Errorf("duplicate anchor: %s", anchor) @@ -399,7 +513,7 @@ func (config *Config) parseHeader(path string) (*HeaderFile, error) { } section.Preamble = comment - section.IsPrivate = len(comment) > 0 && isPrivateSection(comment[0]) + section.IsPrivate = isPrivateSection(heading) section.Anchor = anchor lines = rest[1:] lineNo = restLineNo + 1 @@ -417,7 +531,7 @@ func (config *Config) parseHeader(path string) (*HeaderFile, error) { return nil, fmt.Errorf("hit ending C++ guard while in section on line %d (possibly missing two empty lines ahead of guard?)", lineNo) } - var comment []string + var comment []CommentBlock var decl string if isComment(line) { comment, lines, lineNo, err = extractComment(lines, lineNo) @@ -444,10 +558,11 @@ func (config *Config) parseHeader(path string) (*HeaderFile, error) { // with the name of the thing that they are // commenting on. We make an exception here for // collective comments. + sentence := firstSentence(comment) if len(comment) > 0 && len(name) > 0 && - !isCollectiveComment(comment[0]) { - subject := commentSubject(comment[0]) + !isCollectiveComment(sentence) { + subject := commentSubject(sentence) ok := subject == name if l := len(subject); l > 0 && subject[l-1] == '*' { // Groups of names, notably #defines, are often @@ -486,11 +601,11 @@ func (config *Config) parseHeader(path string) (*HeaderFile, error) { return header, nil } -func firstSentence(paragraphs []string) string { - if len(paragraphs) == 0 { +func firstSentence(comment []CommentBlock) string { + if len(comment) == 0 { return "" } - s := paragraphs[0] + s := comment[0].Paragraph i := strings.Index(s, ". ") if i >= 0 { return s[:i] @@ -501,6 +616,61 @@ func firstSentence(paragraphs []string) string { return s } +func markupComment(allDecls map[string]string, comment []CommentBlock) template.HTML { + var b strings.Builder + lastType := CommentParagraph + closeList := func() { + if lastType == CommentOrderedListItem { + b.WriteString("") + } else if lastType == CommentBulletListItem { + b.WriteString("") + } + } + + for _, block := range comment { + // Group consecutive list items of the same type into a list. + if block.Type != lastType { + closeList() + if block.Type == CommentOrderedListItem { + b.WriteString("
    ") + } else if block.Type == CommentBulletListItem { + b.WriteString("
      ") + } + } + lastType = block.Type + + switch block.Type { + case CommentParagraph: + b.WriteString("

      ") + b.WriteString(string(markupParagraph(allDecls, block.Paragraph))) + b.WriteString("

      ") + case CommentOrderedListItem, CommentBulletListItem: + b.WriteString("
    • ") + b.WriteString(string(markupParagraph(allDecls, block.Paragraph))) + b.WriteString("
    • ") + case CommentCode: + b.WriteString("
      ")
      +			b.WriteString(block.Paragraph)
      +			b.WriteString("
      ") + default: + panic(block.Type) + } + } + + closeList() + return template.HTML(b.String()) +} + +func markupParagraph(allDecls map[string]string, s string) template.HTML { + // TODO(davidben): Ideally the inline transforms would be unified into + // one pass, so that the HTML output of one pass does not interfere with + // the next. + ret := markupPipeWords(allDecls, s, true /* linkDecls */) + ret = markupFirstWord(ret) + ret = markupRFC(ret) + return ret +} + // markupPipeWords converts |s| into an HTML string, safe to be included outside // a tag, while also marking up words surrounded by |. func markupPipeWords(allDecls map[string]string, s string, linkDecls bool) template.HTML { @@ -585,27 +755,14 @@ func markupRFC(html template.HTML) template.HTML { return template.HTML(b.String()) } -func newlinesToBR(html template.HTML) template.HTML { - s := string(html) - if !strings.Contains(s, "\n") { - return html - } - s = strings.Replace(s, "\n", "
      ", -1) - s = strings.Replace(s, " ", " ", -1) - return template.HTML(s) -} - func generate(outPath string, config *Config) (map[string]string, error) { allDecls := make(map[string]string) headerTmpl := template.New("headerTmpl") headerTmpl.Funcs(template.FuncMap{ "firstSentence": firstSentence, - "markupPipeWords": func(s string) template.HTML { return markupPipeWords(allDecls, s, true /* linkDecls */) }, "markupPipeWordsNoLink": func(s string) template.HTML { return markupPipeWords(allDecls, s, false /* linkDecls */) }, - "markupFirstWord": markupFirstWord, - "markupRFC": markupRFC, - "newlinesToBR": newlinesToBR, + "markupComment": func(c []CommentBlock) template.HTML { return markupComment(allDecls, c) }, }) headerTmpl, err := headerTmpl.Parse(` @@ -622,9 +779,9 @@ func generate(outPath string, config *Config) (map[string]string, error) { All headers - {{range .Preamble}}

      {{. | markupPipeWords | markupRFC}}

      {{end}} + {{if .Preamble}}
      {{.Preamble | markupComment}}
      {{end}} -
        +
          {{range .Sections}} {{if not .IsPrivate}} {{if .Anchor}}
        1. {{.Preamble | firstSentence | markupPipeWordsNoLink}}
        2. {{end}} @@ -638,18 +795,12 @@ func generate(outPath string, config *Config) (map[string]string, error) { {{range .Sections}} {{if not .IsPrivate}}
          - {{if .Preamble}} -
          - {{range .Preamble}}

          {{. | markupPipeWords | markupRFC}}

          {{end}} -
          - {{end}} + {{if .Preamble}}
          {{.Preamble | markupComment}}
          {{end}} {{range .Decls}}
          - {{range .Comment}} -

          {{. | markupPipeWords | newlinesToBR | markupFirstWord | markupRFC}}

          - {{end}} - {{if .Decl}}
          {{.Decl}}
          {{end}} + {{if .Comment}}
          {{.Comment | markupComment}}
          {{end}} + {{if .Decl}}
          {{.Decl}}
          {{end}}
          {{end}}
          From eb56a63c0da761ff6fff83ff04ddec0c190c5734 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Sat, 16 Dec 2023 10:47:35 -0500 Subject: [PATCH 18/27] Give WARNING paragraphs a splash of color I'm not sure if this is necessary. I was playing around and this didn't look terrible. Though it will probably turn x509.h into a sea of yellow when it's ready to be rendered. Change-Id: I34b26aad8a779a3fde761558d15b64c79159892a Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64931 Reviewed-by: Bob Beck Commit-Queue: David Benjamin (cherry picked from commit 89dd8d9eb4eabb4fbe20eac977f4827065bc493b) --- util/doc.css | 10 ++++++++++ util/doc.go | 6 +++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/util/doc.css b/util/doc.css index f176f259b7..05d90bec6b 100644 --- a/util/doc.css +++ b/util/doc.css @@ -56,6 +56,16 @@ pre.code { border-radius: 5px; } +p.warning { + background-color: #fef5d3; + padding: 5px; + border-radius: 5px; +} + +p.warning .first-word { + font-weight: bold; +} + .comment pre { margin-left: 2em; } diff --git a/util/doc.go b/util/doc.go index 0d2e2628b4..da4b290756 100644 --- a/util/doc.go +++ b/util/doc.go @@ -641,7 +641,11 @@ func markupComment(allDecls map[string]string, comment []CommentBlock) template. switch block.Type { case CommentParagraph: - b.WriteString("

          ") + if strings.HasPrefix(block.Paragraph, "WARNING:") { + b.WriteString("

          ") + } else { + b.WriteString("

          ") + } b.WriteString(string(markupParagraph(allDecls, block.Paragraph))) b.WriteString("

          ") case CommentOrderedListItem, CommentBulletListItem: From 0aab2946e6429987a9db1dde1005183c3bcd2e78 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Mon, 18 Dec 2023 13:33:36 -0500 Subject: [PATCH 19/27] Restore the X509_EXTENSION ASN1_ITEM too wpa_supplicant also depends on that one. Change-Id: I6abb505d076eb258e6b8d68be42e624f4aedc725 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/64947 Auto-Submit: David Benjamin Commit-Queue: Bob Beck Reviewed-by: Bob Beck (cherry picked from commit 538b2a6cf0497cf8bb61ae726a484a3d7a34e54e) --- crypto/asn1/asn1_test.cc | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/crypto/asn1/asn1_test.cc b/crypto/asn1/asn1_test.cc index 25e19d32eb..d738e59071 100644 --- a/crypto/asn1/asn1_test.cc +++ b/crypto/asn1/asn1_test.cc @@ -2857,6 +2857,12 @@ struct EMBED_X509_ALGOR { STACK_OF(X509_ALGOR) *seq; }; +struct EMBED_X509_EXTENSION { + X509_EXTENSION *simple; + X509_EXTENSION *opt; + STACK_OF(X509_EXTENSION) *seq; +}; + struct EMBED_X509_NAME { X509_NAME *simple; X509_NAME *opt; @@ -2885,6 +2891,14 @@ ASN1_SEQUENCE(EMBED_X509_NAME) = { } ASN1_SEQUENCE_END(EMBED_X509_NAME) IMPLEMENT_ASN1_FUNCTIONS(EMBED_X509_NAME) +DECLARE_ASN1_FUNCTIONS(EMBED_X509_EXTENSION) +ASN1_SEQUENCE(EMBED_X509_EXTENSION) = { + ASN1_SIMPLE(EMBED_X509_EXTENSION, simple, X509_EXTENSION), + ASN1_EXP_OPT(EMBED_X509_EXTENSION, opt, X509_EXTENSION, 0), + ASN1_IMP_SEQUENCE_OF_OPT(EMBED_X509_EXTENSION, seq, X509_EXTENSION, 1), +} ASN1_SEQUENCE_END(EMBED_X509_EXTENSION) +IMPLEMENT_ASN1_FUNCTIONS(EMBED_X509_EXTENSION) + DECLARE_ASN1_FUNCTIONS(EMBED_X509) ASN1_SEQUENCE(EMBED_X509) = { ASN1_SIMPLE(EMBED_X509, simple, X509), @@ -2969,6 +2983,14 @@ TEST(ASN1Test, EmbedTypes) { EMBED_X509_NAME_free, d2i_EMBED_X509_NAME, i2d_EMBED_X509_NAME, sk_X509_NAME_num, sk_X509_NAME_value); + static const uint8_t kTestExtension[] = {0x30, 0x0c, 0x06, 0x03, 0x55, + 0x1d, 0x13, 0x04, 0x05, 0x30, + 0x03, 0x01, 0x01, 0xf}; + TestEmbedType(kTestExtension, i2d_X509_EXTENSION, EMBED_X509_EXTENSION_new, + EMBED_X509_EXTENSION_free, d2i_EMBED_X509_EXTENSION, + i2d_EMBED_X509_EXTENSION, sk_X509_EXTENSION_num, + sk_X509_EXTENSION_value); + static const uint8_t kTestCert[] = { 0x30, 0x82, 0x01, 0xcf, 0x30, 0x82, 0x01, 0x76, 0xa0, 0x03, 0x02, 0x01, 0x02, 0x02, 0x09, 0x00, 0xd9, 0x4c, 0x04, 0xda, 0x49, 0x7d, 0xbf, 0xeb, From 6b85f2f251923ba18127cb15a1a268131b2cba74 Mon Sep 17 00:00:00 2001 From: Andrew Hopkins Date: Tue, 4 Jun 2024 09:14:57 -0700 Subject: [PATCH 20/27] Add libevent to GitHub integration CI (#1615) ### Issues: Resolves CryptoAlg-2465 ### Description of changes: Libevent already works with AWS-LC thanks to other compatibility work. This adds a test to make sure it keeps working. ### Testing: Tested the script locally but need to see what happens with GitHub actions. 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. --------- Co-authored-by: Justin W Smith <103147162+justsmth@users.noreply.github.com> --- .github/workflows/integrations.yml | 13 +++++++ .../integration/run_libevent_integration.sh | 38 +++++++++++++++++++ 2 files changed, 51 insertions(+) create mode 100755 tests/ci/integration/run_libevent_integration.sh diff --git a/.github/workflows/integrations.yml b/.github/workflows/integrations.yml index 1d6fd570d5..ab71319f95 100644 --- a/.github/workflows/integrations.yml +++ b/.github/workflows/integrations.yml @@ -148,3 +148,16 @@ jobs: - name: Run strongswan build run: | ./tests/ci/integration/run_strongswan_integration.sh + libevent: + if: github.repository_owner == 'aws' + runs-on: ubuntu-latest + steps: + - name: Install OS Dependencies + run: | + sudo apt-get update + sudo apt-get -y --no-install-recommends install \ + cmake gcc ninja-build golang + - uses: actions/checkout@v4 + - name: Run libevent build + run: | + ./tests/ci/integration/run_libevent_integration.sh diff --git a/tests/ci/integration/run_libevent_integration.sh b/tests/ci/integration/run_libevent_integration.sh new file mode 100755 index 0000000000..ccba997632 --- /dev/null +++ b/tests/ci/integration/run_libevent_integration.sh @@ -0,0 +1,38 @@ +#!/usr/bin/env bash +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 OR ISC + +set -exu + +source tests/ci/common_posix_setup.sh + +# Assumes script is executed from the root of aws-lc directory +SCRATCH_FOLDER=${SRC_ROOT}/"scratch" +AWS_LC_BUILD_FOLDER="${SCRATCH_FOLDER}/aws-lc-build" +AWS_LC_INSTALL_FOLDER="${SCRATCH_FOLDER}/aws-lc-install" +LIBEVENT_SRC="${SCRATCH_FOLDER}/libevent" +export LD_LIBRARY_PATH="${AWS_LC_INSTALL_FOLDER}/lib" + +function build_and_test_libevent() { + pushd "${LIBEVENT_SRC}" + mkdir build && pushd build + cmake -GNinja -DOPENSSL_ROOT_DIR="${AWS_LC_INSTALL_FOLDER}" ../ + ninja verify + popd && popd +} + +# Make script execution idempotent. +mkdir -p "${SCRATCH_FOLDER}" +rm -rf "${SCRATCH_FOLDER:?}"/* +pushd "${SCRATCH_FOLDER}" + +mkdir -p "${AWS_LC_BUILD_FOLDER}" "${AWS_LC_INSTALL_FOLDER}" +git clone --depth 1 https://github.com/libevent/libevent.git + +# Test with shared AWS-LC libraries +aws_lc_build "$SRC_ROOT" "$AWS_LC_BUILD_FOLDER" "$AWS_LC_INSTALL_FOLDER" -DBUILD_TESTING=OFF -DBUILD_TOOL=OFF -DCMAKE_BUILD_TYPE=RelWithDebInfo -DBUILD_SHARED_LIBS=1 +export LD_LIBRARY_PATH="${LD_LIBRARY_PATH:-}:${AWS_LC_INSTALL_FOLDER}/lib/" +build_and_test_libevent + +ldd "${LIBEVENT_SRC}/build/lib/libevent_openssl.so" | grep "${AWS_LC_INSTALL_FOLDER}/lib/libcrypto.so" || exit 1 +popd From c7759e4947ff2b5233a3908564ced9a2a6f1443b Mon Sep 17 00:00:00 2001 From: ecdeye Date: Tue, 4 Jun 2024 09:15:40 -0700 Subject: [PATCH 21/27] Add support for ocsp get id (#1609) 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. Description: Added "OCSP_onereq_get0_id," which is an OCSP function available in OpenSSL, but missing in AWS-LC --------- Co-authored-by: Justin W Smith <103147162+justsmth@users.noreply.github.com> --- crypto/ocsp/ocsp_server.c | 7 +++++++ crypto/ocsp/ocsp_test.cc | 19 +++++++++++++++++++ include/openssl/ocsp.h | 4 ++++ 3 files changed, 30 insertions(+) diff --git a/crypto/ocsp/ocsp_server.c b/crypto/ocsp/ocsp_server.c index 925ebe9f5a..31afb710da 100644 --- a/crypto/ocsp/ocsp_server.c +++ b/crypto/ocsp/ocsp_server.c @@ -34,6 +34,13 @@ int OCSP_id_get0_info(ASN1_OCTET_STRING **nameHash, ASN1_OBJECT **algor, return 1; } +OCSP_CERTID *OCSP_onereq_get0_id(OCSP_ONEREQ *one) { + if(one == NULL) { + return NULL; + } + return one->reqCert; +} + int OCSP_basic_add1_cert(OCSP_BASICRESP *resp, X509 *cert) { if (resp->certs == NULL && (resp->certs = sk_X509_new_null()) == NULL) { return 0; diff --git a/crypto/ocsp/ocsp_test.cc b/crypto/ocsp/ocsp_test.cc index cade86be56..cf591d4b65 100644 --- a/crypto/ocsp/ocsp_test.cc +++ b/crypto/ocsp/ocsp_test.cc @@ -1581,3 +1581,22 @@ TEST(OCSPTest, OCSPRequestPrint) { EXPECT_EQ(line, expected); } } + +TEST(OCSPTest, OCSPGetID) { + // Create new OCSP_CERTID + OCSP_CERTID *cert_id = OCSP_CERTID_new(); + ASSERT_TRUE(cert_id); + + bssl::UniquePtr request(OCSP_REQUEST_new()); + ASSERT_TRUE(request); + + OCSP_ONEREQ *one = OCSP_request_add0_id(request.get(), cert_id); + ASSERT_TRUE(one); + + // Call function to get OCSP_CERTID + OCSP_CERTID *returned_id = OCSP_onereq_get0_id(one); + + // Verify the returned OCSP_CERTID is same as the one set + ASSERT_EQ(returned_id, cert_id); +} + diff --git a/include/openssl/ocsp.h b/include/openssl/ocsp.h index ed82af5c18..60a05f578c 100644 --- a/include/openssl/ocsp.h +++ b/include/openssl/ocsp.h @@ -159,6 +159,10 @@ OPENSSL_EXPORT int OCSP_REQ_CTX_add1_header(OCSP_REQ_CTX *rctx, OPENSSL_EXPORT OCSP_ONEREQ *OCSP_request_add0_id(OCSP_REQUEST *req, OCSP_CERTID *cid); +// OCSP_onereq_get0_id returns the certificate identifier +// associated with an OCSP request +OPENSSL_EXPORT OCSP_CERTID *OCSP_onereq_get0_id(OCSP_ONEREQ *one); + // OCSP_request_add1_nonce adds a nonce of value |val| and length |len| to // |req|. If |val| is NULL, a random nonce is generated and used. If |len| is // zero or negative, a default length of 16 bytes will be used. From 94f5900258a27c946e9b7e3678e41b1d9fd2d717 Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Tue, 4 Jun 2024 14:49:24 -0400 Subject: [PATCH 22/27] Disable CI for gcc-14/FIPS until relocation issue is resolved (#1622) ### Issues: Addresses V1387993904 ### Description of changes: Remove CI test for gcc-14/FIPS until delocator is updated. 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. --- .github/workflows/actions-ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/actions-ci.yml b/.github/workflows/actions-ci.yml index 21ea696785..7cbbf2d665 100644 --- a/.github/workflows/actions-ci.yml +++ b/.github/workflows/actions-ci.yml @@ -255,8 +255,12 @@ jobs: cxx-compiler: g++-${{ matrix.gccversion }} options: FIPS=${{ matrix.fips }} CMAKE_BUILD_TYPE=Release - name: Build Project + # TODO: Re-enable gcc-14/FIPS build once delocator updated + if: ${{ !( matrix.gccversion == '14' && matrix.fips == '1' ) }} run: cmake --build ./build --target all - name: Run tests + # TODO: Re-enable gcc-14/FIPS build once delocator updated + if: ${{ !( matrix.gccversion == '14' && matrix.fips == '1' ) }} run: cmake --build ./build --target run_tests clang-ubuntu-2004-sanity: From e44fc2ccf802f440b25c037e4d994203790c1ccd Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Tue, 4 Jun 2024 14:55:55 -0400 Subject: [PATCH 23/27] Update for FIPS documentation (#1610) ### Issues: Resolves: CryptoAlg-2419 ### Description of changes: Updating the documentation about our FIPS module. * You can see the markdown rendered here: https://github.com/justsmth/aws-lc/blob/fips-doc-update/crypto/fipsmodule/FIPS.md 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/FIPS.md | 161 +++++++++++++++----------------------- 1 file changed, 64 insertions(+), 97 deletions(-) diff --git a/crypto/fipsmodule/FIPS.md b/crypto/fipsmodule/FIPS.md index b514b1a717..13f905fd45 100644 --- a/crypto/fipsmodule/FIPS.md +++ b/crypto/fipsmodule/FIPS.md @@ -1,175 +1,142 @@ -# FIPS 140-2 +# FIPS 140-3 -BoringSSL as a whole is not FIPS validated. However, there is a core library (called BoringCrypto) that has been FIPS validated. This document contains some notes about the design of the FIPS module and some documentation on performing FIPS-related tasks. This is not a substitute for reading the offical Security Policy. - -Please note that we cannot answer questions about FIPS, nor about using BoringSSL in a FIPS-compliant manner. Please consult with an [accredited CMVP lab](http://csrc.nist.gov/groups/STM/testing_labs/) on these subjects. +A submodule of AWS-LC, referred to here as the “FIPS module”, is periodically submitted for FIPS validation from the National Institute for Standards and Technology (NIST). This document contains notes about the design of our FIPS module and documentation on performing certain FIPS-related tasks. This document is not a substitute for reading our latest official [FIPS Security Policy](https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp4631.pdf). Please note that we cannot answer questions about FIPS, nor about using AWS-LC in a FIPS-compliant manner. Please consult with an [accredited CMVP lab](http://csrc.nist.gov/groups/STM/testing_labs/) or a compliance specialist on these topics. ## Validations -BoringCrypto has undergone the following validations: +NIST has awarded the FIPS module of AWS-LC its validation certificate as a Federal Information Processing Standards (FIPS) 140-3, level 1, cryptographic module. -1. 2017-06-15: certificate [#2964](https://csrc.nist.gov/Projects/Cryptographic-Module-Validation-Program/Certificate/2964), [security policy](./policydocs/BoringCrypto-Security-Policy-20170615.docx) (in docx format). -1. 2018-07-30: certificate [#3318](https://csrc.nist.gov/Projects/Cryptographic-Module-Validation-Program/Certificate/3318), [security policy](./policydocs/BoringCrypto-Security-Policy-20180730.docx) (in docx format). -1. 2019-08-08: certificate [#3678](https://csrc.nist.gov/Projects/Cryptographic-Module-Validation-Program/Certificate/3678), [security policy](./policydocs/BoringCrypto-Security-Policy-20190808.docx) (in docx format). -1. 2019-10-20: certificate [#3753](https://csrc.nist.gov/Projects/Cryptographic-Module-Validation-Program/Certificate/3753), [security policy](./policydocs/BoringCrypto-Android-Security-Policy-20191020.docx) (in docx format). -1. 2021-01-28: certificate [#4156](https://csrc.nist.gov/Projects/Cryptographic-Module-Validation-Program/Certificate/4156), [security policy](./policydocs/BoringCrypto-Android-Security-Policy-20210319.docx) (in docx format). -1. 2021-04-29: certificate [#4407](https://csrc.nist.gov/projects/cryptographic-module-validation-program/certificate/4407). +1. AWS-LC-FIPS v1.0: certificate [#4631](https://csrc.nist.gov/projects/cryptographic-module-validation-program/certificate/4631), [security policy](https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/security-policies/140sp4631.pdf) -## Running ACVP tests +NIST has also awarded SP 800-90B validation certificate for our CPU Jitter Entropy Source. -See `util/fipstools/acvp/ACVP.md` for details of how ACVP testing is done. - -## Breaking known-answer and continuous tests - -Each known-answer test (KAT) uses a unique, random input value. `util/fipstools/break-kat.go` contains a listing of those values and can be used to corrupt a given test in a binary. Since changes to the KAT input values will invalidate the integrity test, `BORINGSSL_FIPS_BREAK_TESTS` can be defined in `fips_break_tests.h` to disable it for the purposes of testing. - -Some FIPS tests cannot be broken by replacing a known string in the binary. For those, when `BORINGSSL_FIPS_BREAK_TESTS` is defined, the environment variable `BORINGSSL_FIPS_BREAK_TEST` can be set to one of a number of values in order to break the corresponding test: +1. 2023-09-14: entropy certificate [#E77](https://csrc.nist.gov/projects/cryptographic-module-validation-program/entropy-validations/certificate/77), [public use document](https://csrc.nist.gov/CSRC/media/projects/cryptographic-module-validation-program/documents/entropy/E77_PublicUse.pdf) -1. `RSA_PWCT` -1. `ECDSA_PWCT` +### Modules in Process -## Breaking the integrity test +The modules below have been tested by an accredited lab and have been submitted to NIST for FIPS 140-3 validation. -The utility in `util/fipstools/break-hash.go` can be used to corrupt the FIPS module inside a binary and thus trigger a failure of the integrity test. Note that the binary must not be stripped, otherwise the utility will not be able to find the FIPS module. +* AWS-LC-FIPS v2.0 (dynamic library): [Review Pending](https://csrc.nist.gov/Projects/Cryptographic-Module-Validation-Program/Modules-In-Process/Modules-In-Process-List) - [Draft security policy](https://github.com/aws/aws-lc/blob/fips-2022-11-02/crypto/fipsmodule/policydocs/DRAFT-140-3-AmazonSecurityPolicy-2.0.0-dynamic.pdf) +* AWS-LC-FIPS v2.0 (static library): [Review Pending](https://csrc.nist.gov/Projects/Cryptographic-Module-Validation-Program/Modules-In-Process/Modules-In-Process-List) - [Draft security policy](https://github.com/aws/aws-lc/blob/fips-2022-11-02/crypto/fipsmodule/policydocs/DRAFT-140-3-AmazonSecurityPolicy-2.0.0-static.pdf) ## RNG design -FIPS 140-2 requires that one of its PRNGs be used (which they call DRBGs). In BoringCrypto, we use CTR-DRBG with AES-256 exclusively and `RAND_bytes` (the primary interface for the rest of the system to get random data) takes its output from there. +FIPS 140-3 requires the use of one of its Deterministic Random Bit Generators (DRBGs, also called Pseudo-Random Number Generators, PRNGs). In AWS-LC, we use CTR-DRBG with AES-256 exclusively from which `RAND_bytes` produces its output. The DRBG state is kept in a thread-local structure and is seeded using the configured entropy source. The CTR-DRBG is periodically reseeded from the entropy source during calls to `RAND_bytes`. The rest of the library obtains random data from `RAND_bytes`. -The DRBG state is kept in a thread-local structure and is seeded using the configured entropy source. +The FIPS DRBGs allow “additional input” to be fed into a given call. We use this feature to be as robust as possible against state duplication from process forks and VM copies. A caller may supply bytes that will be XOR'd into the generated “additional input” using `RAND_bytes_with_additional_data`. -The CTR-DRBG is reseeded every 4096 calls to `RAND_bytes`. Thus the process will randomly crash about every 2¹³⁵ calls. +FIPS requires that RNG state be zeroed when the process exits. In order to implement this, all per-thread RNG states are tracked in a linked list and a [destructor function](https://github.com/aws/aws-lc/blob/eb58525ce1704c5183af9aa28f50945c11fe5a0d/crypto/fipsmodule/rand/rand.c#L251) is included which clears them. In order for this to be safe in the presence of threads, a lock is used to stop all other threads from using the RNG once this process has begun. Thus the main thread exiting may cause other threads to deadlock, and drawing on entropy in a destructor function may also deadlock. -The FIPS PRNGs allow “additional input” to be fed into a given call. We use this feature to be as robust as possible to state duplication from process forks and VM copies: on Intel CPUs with RDRAND, for every call we read 32 bytes of “additional data” from RDRAND, which means that cloned states will diverge at the next call to `RAND_bytes`. This is called “prediction resistance” by FIPS, but we do *not* claim this property in a FIPS context because we don't implement it the way they want. +## AWS-LC-FIPS v1.0 Entropy Source -There is a second interface to the RNG which allows the caller to supply bytes that will be XORed into the generated additional data (`RAND_bytes_with_additional_data`). This is used in the ECDSA code to include the message and private key in the generation of *k*, the ECDSA nonce. This allows ECDSA to be robust to entropy failures while still following the FIPS rules. +The AWS-LC-FIPS v1.0 entropy source is CPU Jitter Entropy ([version 3.1.0](https://github.com/aws/aws-lc/blob/fips-2021-10-20/third_party/jitterentropy/README.md)). For technical details on how CPU Jitter generates entropy, please refer to the [Jitter RNG homepage](https://www.chronox.de/jent/index.html). -FIPS requires that RNG state be zeroed when the process exits. In order to implement this, all per-thread RNG states are tracked in a linked list and a destructor function is included which clears them. In order for this to be safe in the presence of threads, a lock is used to stop all other threads from using the RNG once this process has begun. Thus the main thread exiting may cause other threads to deadlock, and drawing on entropy in a destructor function may also deadlock. +## AWS-LC-FIPS v2.0 Entropy Source -## Entropy sources +The AWS-LC-FIPS v2.0 module uses passive entropy by default and the specific entropy gathering mechanism depends on the operating environment installed on. CPU Jitter Entropy ([version 3.4.0](https://github.com/aws/aws-lc/blob/fips-2022-11-02/third_party/jitterentropy/README.md)) can still be enabled by using the `ENABLE_FIPS_ENTROPY_CPU_JITTER=ON` flag. -By default, entropy is sourced using a "Passive" method where the specific entropy source depends on the OE. Seeding and reseeding material for the DRBG is sourced from the specific entropy source. +## Breaking known-answer and continuous tests -In FIPS mode, when the CPU Jitter entropy source is used, we do a 3x oversampling (the CPU Jitter default setting). Note that the CPU Jitter is not the default entropy source (see subsection below). +Each known-answer test (KAT) uses a unique, random input value. `util/fipstools/break-kat.go` contains a listing of those values and can be used to corrupt a given test in a binary. Since changes to the KAT input values will invalidate the integrity test, `BORINGSSL_FIPS_BREAK_TESTS` can be defined in `fips_break_tests.h` to disable it for the purposes of testing. -### Modify entropy source - not recommended +Some FIPS tests cannot be broken by replacing a known string in the binary. For those, when `BORINGSSL_FIPS_BREAK_TESTS` is defined, the environment variable `BORINGSSL_FIPS_BREAK_TEST` can be set to one of a number of values in order to break the corresponding test: -Modifying the entropy source can invalidate your validation status. Changing the entropy is **not** recommended. +1. `RSA_PWCT` +2. `ECDSA_PWCT` -It is possible to modify the entropy method at build-time. Using `ENABLE_FIPS_ENTROPY_CPU_JITTER=ON`, the entropy source is switched to [CPU Jitter](https://github.com/smuellerDD/jitterentropy-library). +## Running ACVP tests -CPU Jitter is less performant than the default method and can incur up to a 2-digit millisecond latency when queried. You can test CPU Jitter entropy on your system using `bssl speed -filter Jitter`. +See `util/fipstools/acvp/ACVP.md` for details of how ACVP testing is done. ## Integrity Test -FIPS-140 mandates that a module calculate an HMAC of its own code in a constructor function and compare the result to a known-good value. Typical code produced by a C compiler includes large numbers of relocations: places in the machine code where the linker needs to resolve and inject the final value of a symbolic expression. These relocations mean that the bytes that make up any specific bit of code generally aren't known until the final link has completed. - -Additionally, because of shared libraries and ASLR, some relocations can only be resolved at run-time, and thus targets of those relocations vary even after the final link. +FIPS-140 mandates that a module calculate an HMAC of its own code in a constructor function and compare the result to a known-good value. Typical code produced by a C compiler includes large numbers of relocations: places in the machine code where the linker needs to resolve and inject the final value of a symbolic expression. These relocations mean that the bytes that make up any specific bit of code generally aren't known until the final link has completed. Additionally, because of shared libraries and [ASLR](https://en.wikipedia.org/wiki/Address_space_layout_randomization), some relocations can only be resolved at run-time, and thus targets of those relocations vary even after the final link. -BoringCrypto is linked (often statically) into a large number of binaries. It would be a significant cost if each of these binaries had to be post-processed in order to calculate the known-good HMAC value. We would much prefer if the value could be calculated, once, when BoringCrypto itself is compiled. +AWS-LC is linked (often statically) into a large number of binaries. It would be a significant cost if each of these binaries had to be post-processed in order to calculate the known-good HMAC value. We would much prefer if the value could be calculated, once, when AWS-LC itself is compiled. In order for the value to be calculated before the final link, there can be no relocations in the hashed code and data. -In order for the value to be calculated before the final link, there can be no relocations in the hashed code and data. This document describes how we build C and assembly code in order to produce a binary file containing all the code and data for the FIPS module without that code having any relocations. - -There are two build configurations supported: static and shared. The shared build produces `libcrypto.so`, which includes the FIPS module and is significantly more straightforward and so is described first: +We describe below how we build C and assembly code in order to produce a binary file containing the code and data for the FIPS module without that code having any relocations. There are two build configurations supported: **static** and **shared**. The shared build produces `libcrypto.so`, which includes the FIPS module and is significantly more straightforward and so is described first. ### Linux Shared build -First, all the C source files for the module are compiled as a single unit by compiling a single source file that `#include`s them all (this is `bcm.c`). This, along with some assembly sources, comprise the FIPS module. +First, all the C source files for the module are compiled as a single unit by compiling a single source file that `#include`s them all (this is `[bcm.c](https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/bcm.c)`). This, along with some assembly sources, comprise the FIPS module. -The object files resulting from compiling (or assembling) those files is linked in partial-linking mode with a linker script that causes the linker to insert symbols marking the beginning and end of the text and rodata sections. The linker script also discards other types of data sections to ensure that no unhashed data is used by the module. +The object files resulting from compiling/assembling those files are linked in partial-linking mode with a [linker script](https://github.com/aws/aws-lc/blob/main/crypto/fipsmodule/gcc_fips_shared.lds) that causes the linker to insert symbols marking the beginning and end of the text and rodata sections. The linker script also discards other types of data sections to ensure that no unhashed data is used by the module. -One source of such data are `rel.ro` sections, which contain data that includes function pointers. Since these function pointers are absolute, they are written by the dynamic linker at run-time and so we must eliminate them. The pattern that causes them is when we have a static `EVP_MD` or `EVP_CIPHER` object thus, inside the module, this pattern is changed to instead reserve space in the BSS for the object, and to add a `CRYPTO_once_t` to protect its initialisation. +One source of unhashable data is `rel.ro` sections, which contain data that includes function pointers. Since these function pointers are absolute, they are written by the dynamic linker at run-time and so must be eliminated. The pattern that causes them is when we have a static `EVP_MD` or `EVP_CIPHER` object thus, inside the module, this pattern is changed to instead reserve space in the [BSS](https://en.wikipedia.org/wiki/.bss) for the object, and to add a `CRYPTO_once_t` to protect its initialization. -Once the partially-linked result is linked again, with other parts of libcrypto, to produce `libcrypto.so`, the contents of the module are fixed, as required. The module code uses the linker-added symbols to find the its code and data at run-time and hashes them upon initialisation. The result is compared against a value stored inside `libcrypto.so`, but outside of the module. That value will, initially, be incorrect, but `inject-hash.go` can inject the correct value. +Once the partially-linked result is linked again with other parts of libcrypto, to produce `libcrypto.so`, the contents of the module are fixed, as required. The module code uses the linker-added symbols to find its code and data at run-time and hashes them upon initialization. The result is compared against a value stored inside `libcrypto.so`, but outside of the module. That value will, initially, be incorrect, but `inject-hash.go` injects the correct value. ### Windows Shared build The Shared Windows FIPS integrity test differs in two key ways: + 1. How the start and end of the module are marked 2. How the correct integrity hash is calculated -Microsoft Visual C compiler (MSVC) does not support linker scripts which add symbols to mark the start and end of the text and rodata sections on Linux. Instead, fips_shared_library_marker.c is compiled twice to generate two object files that contain start/end functions and variables. MSVC `pragma` segment definitions are used to place the markers in specific sections (e.g. `.fipstx$a`). This particular name format uses Portable Executable Grouped Sections to control what section the code is placed in and the order within the section. With the start and end markers placed at `$a` and `$z` respectively, BCM puts everything in the `$b` section. When the final crypto.dll is built all the code is in the `.fipstx` section, all data is in `.fipsda`, all constants are in `.fipsco`, all unitialized items in `.fipsbs`, and everything is in the correct order. - +Microsoft Visual C compiler (MSVC) does not support linker scripts that add symbols to mark the start and end of the text and rodata sections, as is done on Linux. Instead, `fips_shared_library_marker.c` is compiled twice to generate two object files that contain start/end functions and variables. MSVC `pragma` segment definitions are used to place the markers in specific sections (e.g. `.fipstx$a`). This particular name format uses [Portable Executable Grouped Sections](https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#grouped-sections-object-only) to control what section the code is placed in and the order within the section. With the start and end markers placed at `$a` and `$z` respectively, BCM puts everything in the `$b` section. When the final crypto.dll is built, all the code is in the `.fipstx` section, all data is in `.fipsda`, all constants are in `.fipsco`, all uninitialized items in `.fipsbs`, and everything is in the correct order. The process to generate the expected integrity fingerprint is also different from Linux: -1. Build the required object files once: `bcm.obj` from `bcm.c` and the start/end object files - 1. `bcm.obj` places the power-on self tests in the `.CRT$XCU` section which is run automatically by the Windows Common Runtime library (CRT) startup code -2. Use MSVC's `lib.exe` to combine the start/end object files with `bcm.obj` to create the static library `bcm.lib`. - 1. MSVC does not support combining multiple object files into another object file like the Apple build. + +1. Build the required object files once: `bcm.obj` from `bcm.c` and the start/end object files + 1. `bcm.obj` places the power-on self tests in the `.CRT$XCU` section which is run automatically by the Windows Common Runtime library (CRT) startup code +2. Use MSVC's `lib.exe` to combine the start/end object files with `bcm.obj` to create the static library `bcm.lib`. + 1. MSVC does not support combining multiple object files into another object file like the Apple build. 3. Build `fipsmodule` which contains the placeholder integrity hash 4. Build `precrypto.dll` with `bcm.obj` and `fipsmodule` 5. Build the small application `fips_empty_main.exe` and link it with `precrypto.dll` 6. `capture-hash.go` runs `fips_empty_main.exe` - 1. The CRT runs all functions in the `.CRT$XC*` sections in order starting with `.CRT$XCA` - 2. The BCM power-on tests are in `.CRT$XCU` and are run after all other Windows initialization is complete - 3. BCM calculates the correct integrity value which will not match the placeholder value. Before aborting the process the correct value is printed - 4. `capture-hash.go` reads the correct integrity value and writes it to `generated_fips_shared_support.c` + 1. The CRT runs all functions in the `.CRT$XC*` sections in order starting with `.CRT$XCA` + 2. The BCM power-on tests are in `.CRT$XCU` and are run after all other Windows initialization is complete + 3. BCM calculates the correct integrity value which will not match the placeholder value. Before aborting the process the correct value is printed + 4. `capture-hash.go` reads the correct integrity value and writes it to `generated_fips_shared_support.c` 7. `generated_fipsmodule` is built with `generated_fips_shared_support.c` 8. `crypto.dll` is built with the same original `bcm.lib` and `generated_fipsmodule` -### Static build - -The static build cannot depend on the shared-object link to resolve relocations and thus must take another path. +### Linux Static build -As with the shared build, all the C sources are build in a single compilation unit. The `-fPIC` flag is used to cause the compiler to use IP-relative addressing in many (but not all) cases. Also the `-S` flag is used to instruct the compiler to produce a textual assembly file rather than a binary object file. +The static build cannot depend on the shared-object linkage to resolve relocations and thus must take another path. +As with the shared build, all the C sources are built into a single compilation unit. The `-fPIC` flag is used to cause the compiler to use [IP-relative addressing](https://en.wikipedia.org/wiki/Position-independent_code) in many (but not all) cases. -The textual assembly file is then processed by a script to merge in assembly implementations of some primitives and to eliminate the remaining sources of relocations. +The unit is built with the `-S` flag to instruct the compiler to produce a textual assembly file rather than a binary object file. +The textual assembly file is then processed by a “delocator” script to merge in assembly implementations of some primitives and to eliminate the remaining sources of relocations. -##### Redirector functions +#### Redirector functions -The most obvious cause of relocations are out-calls from the module to non-cryptographic functions outside of the module. Most obviously these include `malloc`, `memcpy` and other libc functions, but also include calls to support code in BoringSSL such as functions for managing the error queue. +The most obvious cause of relocations are out-calls from the module to non-cryptographic functions outside of the module. Most obviously these include `malloc`, `memcpy` and other libc functions, but also include calls to support code in AWS-LC such as functions for managing the error queue. -Offsets to these functions cannot be known until the final link because only the linker sees the object files containing them. Thus calls to these functions are rewritten into an IP-relative jump to a redirector function. The redirector functions contain a single jump instruction to the real function and are placed outside of the module and are thus not hashed (see diagram). +Offsets to these functions cannot be known until the final link because only the linker sees the object files containing them. Thus calls to these functions are rewritten into an IP-relative jump to a *redirector* function. The redirector functions contain a single jump instruction to the real function and are placed outside of the module and are thus not hashed (see diagram). +In this diagram, the integrity check hashes from `module_start` to `module_end`. Since this does not cover the jump to `memcpy`, it's fine that the linker will poke the final offset into that instruction. ![module structure](./intcheck1.png) -In this diagram, the integrity check hashes from `module_start` to `module_end`. Since this does not cover the jump to `memcpy`, it's fine that the linker will poke the final offset into that instruction. - -##### Read-only data +#### Read-only data Normally read-only data is placed in an `.rodata` segment that doesn't get mapped into memory with execute permissions. However, the offset of the data segment from the text segment is another thing that isn't determined until the final link. In order to fix data offsets before the link, read-only data is simply placed in the module's `.text` segment. This might make building ROP chains easier for an attacker, but so it goes. Data containing function pointers remains an issue. The source-code changes described above for the shared build apply here too, but no direct references to a BSS section are possible because the offset to that section is not known at compile time. Instead, the script generates functions outside of the module that return pointers to these areas of memory—they effectively act like special-purpose malloc calls that cannot fail. -##### Read-write data +#### Read-write data Mutable data is a problem. It cannot be in the text segment because the text segment is mapped read-only. If it's in a different segment then the code cannot reference it with a known, IP-relative offset because the segment layout is only fixed during the final link. - In order to allow this we use a similar design to the redirector functions: the code references a symbol that's in the text segment, but out of the module and thus not hashed. A relocation record is emitted to instruct the linker to poke the final offset to the variable in that location. Thus the only change needed is an extra indirection when loading the value. -##### Other transforms +#### Other transforms -The script performs a number of other transformations which are worth noting but do not warrant their own discussions: +The delocator script performs a number of other transformations which are worth noting but do not warrant their own discussions: -1. It duplicates each global symbol with a local symbol that has `_local_target` appended to the name. References to the global symbols are rewritten to use these duplicates instead. Otherwise, although the generated code uses IP-relative references, relocations are emitted for global symbols in case they are overridden by a different object file during the link. -1. Various sections, notably `.rodata`, are moved to the `.text` section, inside the module, so module code may reference it without relocations. -1. For each BSS symbol, it generates a function named after that symbol but with `_bss_get` appended, which returns its address. -1. It inserts the labels that delimit the module's code and data (called `module_start` and `module_end` in the diagram above). -1. It adds a 64-byte, read-only array outside of the module to contain the known-good HMAC value. +1. It duplicates each global symbol with a local symbol that has `_local_target` appended to the name. References to the global symbols are rewritten to use these duplicates instead. Otherwise, although the generated code uses IP-relative references, relocations are emitted for global symbols in case they are overridden by a different object file during the link. +2. Various sections, notably `.rodata`, are moved to the `.text` section, inside the module, so module code may reference it without relocations. +3. For each BSS symbol, it generates a function named after that symbol but with `_bss_get` appended, which returns its address. +4. It inserts the labels that delimit the module's code and data (called `module_start` and `module_end` in the diagram above). +5. It adds a 64-byte, read-only array outside of the module to contain the known-good HMAC value. -##### Integrity testing +#### Integrity testing In order to actually implement the integrity test, a constructor function within the module calculates an HMAC from `module_start` to `module_end` using a fixed, all-zero key. It compares the result with the known-good value added (by the script) to the unhashed portion of the text segment. If they don't match, it calls `exit` in an infinite loop. - Initially the known-good value will be incorrect. Another script (`inject_hash.go`) calculates the correct value from the assembled object and injects it back into the object. -![build process](./intcheck2.png) - -### Comparison with OpenSSL's method - -(This is based on reading OpenSSL's [user guide](https://www.openssl.org/docs/fips/UserGuide-2.0.pdf) and inspecting the code of OpenSSL FIPS 2.0.12.) +### Breaking the integrity test -OpenSSL's solution to this problem is very similar to our shared build, with just a few differences: - -1. OpenSSL deals with run-time relocations by not hashing parts of the module's data. -1. OpenSSL uses `ld -r` (the partial linking mode) to merge a number of object files into their `fipscanister.o`. For BoringCrypto's static build, we merge all the C source files by building a single C file that #includes all the others, and we merge the assembly sources by appending them to the assembly output from the C compiler. -1. OpenSSL depends on the link order and inserts two object files, `fips_start.o` and `fips_end.o`, in order to establish the `module_start` and `module_end` values. BoringCrypto adds labels at the correct places in the assembly for the static build, or uses a linker script for the shared build. -1. OpenSSL calculates the hash after the final link and either injects it into the binary or recompiles with the value of the hash passed in as a #define. BoringCrypto calculates it prior to the final link and injects it into the object file. -1. OpenSSL references read-write data directly, since it can know the offsets to it. BoringCrypto indirects these loads and stores. -1. OpenSSL doesn't run the power-on test until `FIPS_module_mode_set` is called. BoringCrypto does it in a constructor function. Failure of the test is non-fatal in OpenSSL, BoringCrypto will crash. -1. Since the contents of OpenSSL's module change between compilation and use, OpenSSL generates `fipscanister.o.sha1` to check that the compiled object doesn't change before linking. Since BoringCrypto's module is fixed after compilation (in the static case), the final integrity check is unbroken through the linking process. - -Some of the similarities are worth noting: +The utility in `util/fipstools/break-hash.go` can be used to corrupt the FIPS module inside a binary and thus trigger a failure of the integrity test. Note that the binary must not be stripped, otherwise the utility will not be able to find the FIPS module. -1. OpenSSL has all out-calls from the module indirecting via the PLT, which is equivalent to the redirector functions described above. +![build process](./intcheck2.png) -![OpenSSL build process](./intcheck3.png) From 2431353ceae69a50a494e2a0e9f5b5d58ca8bec3 Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Tue, 4 Jun 2024 15:23:09 -0700 Subject: [PATCH 24/27] Fix SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR behavior (#1620) ### Description of changes: This was discovered when taking google/boringssl@5b3dc49 during the upstream merge.`ERR_clear_error` is being called more eagerly with the new change, which led us to discover that `SSLTest.BuildCertChain` was actually testing against an error code propagated onto the stack by the previous call to `SSL_CTX_build_cert_chain`. Upon further examination, we weren't propagating an error when calling `SSL_CTX_build_cert_chain` with `SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR` . The correct behavior should be to push an error onto the stack regardless. 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. --- ssl/ssl_test.cc | 2 ++ ssl/ssl_x509.cc | 13 +++++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index 8d512bf580..8487778412 100644 --- a/ssl/ssl_test.cc +++ b/ssl/ssl_test.cc @@ -5217,12 +5217,14 @@ TEST(SSLTest, BuildCertChain) { // Verification will fail because there is no valid root cert available. EXPECT_FALSE(SSL_CTX_build_cert_chain(ctx.get(), 0)); + ERR_clear_error(); // Should return 2 when |SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR| is set. EXPECT_EQ( SSL_CTX_build_cert_chain(ctx.get(), SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR), 2); EXPECT_TRUE(ExpectSingleError(ERR_LIB_SSL, SSL_R_CERTIFICATE_VERIFY_FAILED)); + ERR_clear_error(); // Should return 2, but with no error on the stack when // |SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR| and |SSL_BUILD_CHAIN_FLAG_CLEAR_ERROR| diff --git a/ssl/ssl_x509.cc b/ssl/ssl_x509.cc index 5388a475ee..112d0747ed 100644 --- a/ssl/ssl_x509.cc +++ b/ssl/ssl_x509.cc @@ -1061,12 +1061,13 @@ static int ssl_build_cert_chain(CERT *cert, X509_STORE *cert_store, int flags) { bool ignore_error = false; if (X509_verify_cert(store_ctx.get()) <= 0) { + OPENSSL_PUT_ERROR(SSL, SSL_R_CERTIFICATE_VERIFY_FAILED); + ERR_add_error_data(2, "Verify error:", + X509_verify_cert_error_string( + X509_STORE_CTX_get_error(store_ctx.get()))); + // Fail if |SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR| is not set. - if(!is_flag_set(flags, SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR)) { - OPENSSL_PUT_ERROR(SSL, SSL_R_CERTIFICATE_VERIFY_FAILED); - ERR_add_error_data(2, "Verify error:", - X509_verify_cert_error_string( - X509_STORE_CTX_get_error(store_ctx.get()))); + if (!is_flag_set(flags, SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR)) { return 0; } @@ -1098,7 +1099,7 @@ static int ssl_build_cert_chain(CERT *cert, X509_STORE *cert_store, int flags) { // Anything that has passed successfully up to here is valid. // 2 is used to indicate a verification error has happened, but was ignored // because |SSL_BUILD_CHAIN_FLAG_IGNORE_ERROR| was set. - if(ignore_error) { + if (ignore_error) { return 2; } return 1; From ff3d99ce94ddb782914b5bb79a13d7ea564da5d5 Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Tue, 4 Jun 2024 20:35:14 -0400 Subject: [PATCH 25/27] Fixes for building with `-pedantic` (#1608) --- .github/workflows/actions-ci.yml | 54 ++++++++++++++++++++++++++------ CMakeLists.txt | 5 +-- crypto/fipsmodule/hmac/hmac.c | 16 +++++----- crypto/mem.c | 2 +- 4 files changed, 57 insertions(+), 20 deletions(-) diff --git a/.github/workflows/actions-ci.yml b/.github/workflows/actions-ci.yml index 7cbbf2d665..e20c527cd9 100644 --- a/.github/workflows/actions-ci.yml +++ b/.github/workflows/actions-ci.yml @@ -263,13 +263,49 @@ jobs: if: ${{ !( matrix.gccversion == '14' && matrix.fips == '1' ) }} run: cmake --build ./build --target run_tests + gcc-13-pedantic: + if: github.repository_owner == 'aws' + needs: [ sanity-test-run ] + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v3 + - name: Setup CMake + uses: threeal/cmake-action@v1.3.0 + with: + generator: Ninja + c-compiler: gcc-13 + cxx-compiler: g++-13 + options: CMAKE_BUILD_TYPE=Release CMAKE_C_FLAGS=-pedantic CMAKE_CXX_FLAGS=-pedantic + - name: Build Crypto + run: cmake --build ./build --target crypto + - name: Build SSL + run: cmake --build ./build --target ssl + + clang-18-pedantic: + if: github.repository_owner == 'aws' + needs: [ sanity-test-run ] + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v3 + - name: Setup CMake + uses: threeal/cmake-action@v1.3.0 + with: + generator: Ninja + c-compiler: clang-18 + cxx-compiler: clang++-18 + options: CMAKE_BUILD_TYPE=Release CMAKE_C_FLAGS=-pedantic CMAKE_CXX_FLAGS=-pedantic + - name: Build Crypto + run: cmake --build ./build --target crypto + - name: Build SSL + run: cmake --build ./build --target ssl + clang-ubuntu-2004-sanity: if: github.repository_owner == 'aws' needs: [sanity-test-run] strategy: fail-fast: false matrix: - gccversion: + clangversion: - "10" - "11" - "12" @@ -286,8 +322,8 @@ jobs: uses: threeal/cmake-action@v1.3.0 with: generator: Ninja - c-compiler: clang-${{ matrix.gccversion }} - cxx-compiler: clang++-${{ matrix.gccversion }} + c-compiler: clang-${{ matrix.clangversion }} + cxx-compiler: clang++-${{ matrix.clangversion }} options: FIPS=${{ matrix.fips }} CMAKE_BUILD_TYPE=Release - name: Build Project run: cmake --build ./build --target all @@ -300,7 +336,7 @@ jobs: strategy: fail-fast: false matrix: - gccversion: + clangversion: - "13" - "14" - "15" @@ -317,8 +353,8 @@ jobs: uses: threeal/cmake-action@v1.3.0 with: generator: Ninja - c-compiler: clang-${{ matrix.gccversion }} - cxx-compiler: clang++-${{ matrix.gccversion }} + c-compiler: clang-${{ matrix.clangversion }} + cxx-compiler: clang++-${{ matrix.clangversion }} options: FIPS=${{ matrix.fips }} CMAKE_BUILD_TYPE=Release - name: Build Project run: cmake --build ./build --target all @@ -331,7 +367,7 @@ jobs: strategy: fail-fast: false matrix: - gccversion: + clangversion: - "16" - "17" - "18" @@ -348,8 +384,8 @@ jobs: uses: threeal/cmake-action@v1.3.0 with: generator: Ninja - c-compiler: clang-${{ matrix.gccversion }} - cxx-compiler: clang++-${{ matrix.gccversion }} + c-compiler: clang-${{ matrix.clangversion }} + cxx-compiler: clang++-${{ matrix.clangversion }} options: FIPS=${{ matrix.fips }} CMAKE_BUILD_TYPE=Release - name: Build Project run: cmake --build ./build --target all diff --git a/CMakeLists.txt b/CMakeLists.txt index b640b3ffcf..3b66ce6fe8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -358,8 +358,9 @@ if(GCC OR CLANG) set(C_CXX_FLAGS "${C_CXX_FLAGS} -Wall -fvisibility=hidden -fno-common") endif() set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wunused -Wcomment -Wchar-subscripts -Wuninitialized -Wshadow") - set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wwrite-strings -Wformat-security -Wunused-result") - set(C_CXX_FLAGS "${C_CXX_FLAGS} -Wvla -Wtype-limits -Wno-unused-parameter") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Wwrite-strings -Wformat-security -Wunused-result -Wno-overlength-strings") + set(CMAKE_ASM_FLAGS "${CMAKE_ASM_FLAGS} -Wno-newline-eof") + set(C_CXX_FLAGS "${C_CXX_FLAGS} -Wno-c11-extensions -Wvla -Wtype-limits -Wno-unused-parameter") endif() set(C_CXX_FLAGS "${C_CXX_FLAGS} -Werror -Wformat=2 -Wsign-compare -Wmissing-field-initializers -Wwrite-strings") diff --git a/crypto/fipsmodule/hmac/hmac.c b/crypto/fipsmodule/hmac/hmac.c index 0e576c026a..0393888e58 100644 --- a/crypto/fipsmodule/hmac/hmac.c +++ b/crypto/fipsmodule/hmac/hmac.c @@ -108,14 +108,14 @@ struct hmac_methods_st { // The maximum number of HMAC implementations #define HMAC_METHOD_MAX 8 -MD_TRAMPOLINES_EXPLICIT(MD5, MD5_CTX, MD5_CBLOCK); -MD_TRAMPOLINES_EXPLICIT(SHA1, SHA_CTX, SHA_CBLOCK); -MD_TRAMPOLINES_EXPLICIT(SHA224, SHA256_CTX, SHA256_CBLOCK); -MD_TRAMPOLINES_EXPLICIT(SHA256, SHA256_CTX, SHA256_CBLOCK); -MD_TRAMPOLINES_EXPLICIT(SHA384, SHA512_CTX, SHA512_CBLOCK); -MD_TRAMPOLINES_EXPLICIT(SHA512, SHA512_CTX, SHA512_CBLOCK); -MD_TRAMPOLINES_EXPLICIT(SHA512_224, SHA512_CTX, SHA512_CBLOCK); -MD_TRAMPOLINES_EXPLICIT(SHA512_256, SHA512_CTX, SHA512_CBLOCK); +MD_TRAMPOLINES_EXPLICIT(MD5, MD5_CTX, MD5_CBLOCK) +MD_TRAMPOLINES_EXPLICIT(SHA1, SHA_CTX, SHA_CBLOCK) +MD_TRAMPOLINES_EXPLICIT(SHA224, SHA256_CTX, SHA256_CBLOCK) +MD_TRAMPOLINES_EXPLICIT(SHA256, SHA256_CTX, SHA256_CBLOCK) +MD_TRAMPOLINES_EXPLICIT(SHA384, SHA512_CTX, SHA512_CBLOCK) +MD_TRAMPOLINES_EXPLICIT(SHA512, SHA512_CTX, SHA512_CBLOCK) +MD_TRAMPOLINES_EXPLICIT(SHA512_224, SHA512_CTX, SHA512_CBLOCK) +MD_TRAMPOLINES_EXPLICIT(SHA512_256, SHA512_CTX, SHA512_CBLOCK) struct hmac_method_array_st { HmacMethods methods[HMAC_METHOD_MAX]; diff --git a/crypto/mem.c b/crypto/mem.c index 02799f8fbc..efe42dbee3 100644 --- a/crypto/mem.c +++ b/crypto/mem.c @@ -107,7 +107,7 @@ static void __asan_unpoison_memory_region(const void *addr, size_t size) {} // implementation is statically linked with BoringSSL. So, if |sdallocx| is // provided in, say, libc.so, we still won't use it because that's dynamically // linked. This isn't an ideal result, but its helps in some cases. -WEAK_SYMBOL_FUNC(void, sdallocx, (void *ptr, size_t size, int flags)); +WEAK_SYMBOL_FUNC(void, sdallocx, (void *ptr, size_t size, int flags)) // The following four functions can be defined to override default heap // allocation and freeing. If defined, it is the responsibility of From 77355988e5c97a5bdebff1c39e0a8943e43e4828 Mon Sep 17 00:00:00 2001 From: Justin W Smith <103147162+justsmth@users.noreply.github.com> Date: Wed, 5 Jun 2024 09:08:32 -0400 Subject: [PATCH 26/27] Script for creating compilation database (#1617) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### Description of changes: * A [compilation database](https://clang.llvm.org/docs/JSONCompilationDatabase.html) is a JSON file that helps facilitate code assistance within a code/text editor. This is typically via the editor's integration with [Clangd/LSP](https://clangd.llvm.org/). It is supported by a variety of editors: [VSCode](https://code.visualstudio.com/docs/cpp/faq-cpp#_how-do-i-get-intellisense-to-work-correctly), [CLion](https://www.jetbrains.com/help/clion/compilation-database.html), [Neovim](https://neovim.io/), [Helix](https://helix-editor.com/) among others. * This script simplifies the creation of this database for AWS-LC: `compile_commands.json` * This adds the `compile_commands.json` file to `.gitignore`. ### Testing * I tested the script on both Linux and macOS. ``` ❯ ls -lh *.json -rw-rw-r-- 1 justsmth justsmth 456K Jun 3 14:04 compile_commands.json ``` 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. --- .gitignore | 1 + util/build_compilation_database.sh | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100755 util/build_compilation_database.sh diff --git a/.gitignore b/.gitignore index 941a5e2413..eae71e6471 100644 --- a/.gitignore +++ b/.gitignore @@ -41,3 +41,4 @@ symbols.txt .fleet/ .cache/ /CMakePresets.json +/compile_commands.json diff --git a/util/build_compilation_database.sh b/util/build_compilation_database.sh new file mode 100755 index 0000000000..8cd675cf5c --- /dev/null +++ b/util/build_compilation_database.sh @@ -0,0 +1,19 @@ +#!/usr/bin/env bash + +set -ex + +BASE_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}/" )/.." &> /dev/null && pwd ) + +TMP_DIR=`mktemp -d` +echo ${TMP_DIR} +AWS_LC_BUILD="${TMP_DIR}/AWS-LC-BUILD" + +MY_CMAKE_FLAGS=("-GNinja" "-DCMAKE_BUILD_TYPE=Debug" "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON") + +mkdir -p "${AWS_LC_BUILD}" + +cmake "${BASE_DIR}" -B "${AWS_LC_BUILD}" ${MY_CMAKE_FLAGS[@]} + +cmake --build "${AWS_LC_BUILD}" --target all + +cp "${AWS_LC_BUILD}"/compile_commands.json "${BASE_DIR}"/ From e587bb5ef86e8efb80e027b99ab317c7a912110e Mon Sep 17 00:00:00 2001 From: Samuel Chiang Date: Wed, 5 Jun 2024 10:15:46 -0700 Subject: [PATCH 27/27] Update ec2-test-framework to use gv2 (#1623) Codebuild has upgraded their ARM containers to use gv3. This means we have to update our ec2 testing framework to test on gv2 instead. 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. --- .../ci/cdk/cdk/codebuild/ec2_test_framework_omnibus.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/ci/cdk/cdk/codebuild/ec2_test_framework_omnibus.yaml b/tests/ci/cdk/cdk/codebuild/ec2_test_framework_omnibus.yaml index bd12fa827e..88d617ed26 100644 --- a/tests/ci/cdk/cdk/codebuild/ec2_test_framework_omnibus.yaml +++ b/tests/ci/cdk/cdk/codebuild/ec2_test_framework_omnibus.yaml @@ -6,8 +6,8 @@ version: 0.2 # Doc for batch https://docs.aws.amazon.com/codebuild/latest/userguide/batch-build-buildspec.html#build-spec.batch.build-list batch: build-list: - # Actual tests are ran on an Graviton3 ec2 instance via SSM Commands. - - identifier: graviton3_tests + # Actual tests are ran on an Graviton2 ec2 instance via SSM Commands. + - identifier: graviton2_tests buildspec: ./tests/ci/codebuild/common/run_ec2_target.yml env: type: LINUX_CONTAINER @@ -15,6 +15,6 @@ 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-7x-bm-framework_latest variables: - EC2_AMI: "ami-0a24e6e101933d294" - EC2_INSTANCE_TYPE: "c7g.2xlarge" + EC2_AMI: "ami-0c29a2c5cf69b5a9c" + EC2_INSTANCE_TYPE: "c6g.2xlarge" ECR_DOCKER_TAG: "amazonlinux-2023_clang-15x_sanitizer"