diff --git a/.github/workflows/actions-ci.yml b/.github/workflows/actions-ci.yml index 21ea696785..e20c527cd9 100644 --- a/.github/workflows/actions-ci.yml +++ b/.github/workflows/actions-ci.yml @@ -255,17 +255,57 @@ 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 + 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" @@ -282,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 @@ -296,7 +336,7 @@ jobs: strategy: fail-fast: false matrix: - gccversion: + clangversion: - "13" - "14" - "15" @@ -313,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 @@ -327,7 +367,7 @@ jobs: strategy: fail-fast: false matrix: - gccversion: + clangversion: - "16" - "17" - "18" @@ -344,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/.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 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/.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/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/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/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/asn1_test.cc b/crypto/asn1/asn1_test.cc index 1e0f453cc1..d738e59071 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) @@ -2787,4 +2851,188 @@ 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_EXTENSION { + X509_EXTENSION *simple; + X509_EXTENSION *opt; + STACK_OF(X509_EXTENSION) *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_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), + 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 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, + 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 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/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/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) 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..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]; @@ -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. 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/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 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/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 7830d0872a..9c7f2117e7 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 */; @@ -347,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 @@ -567,6 +566,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/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_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..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)); + } } } @@ -3092,8 +3125,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 { @@ -4342,46 +4375,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 +4442,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 +4451,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 +4475,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/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/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; } 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 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. 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/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/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/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/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. diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h index 779bbad810..e5c006456f 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. // @@ -2915,16 +2915,31 @@ 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. +// 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 @@ -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/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) 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/include/openssl/x509.h b/include/openssl/x509.h index c47d624869..bbd6f1311b 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. @@ -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. // @@ -3041,6 +3057,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 @@ -3456,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); @@ -3534,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) \ @@ -3785,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 @@ -4033,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); @@ -4068,11 +4213,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 +4232,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 +4252,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); @@ -4117,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); @@ -4371,8 +4534,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 +4722,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 +4774,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 diff --git a/ssl/ssl_test.cc b/ssl/ssl_test.cc index de8d31ac0b..9f98a35f0d 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; 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" 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 { 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_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 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, 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}"/ diff --git a/util/doc.config b/util/doc.config index 83c5a563b7..63c8bdca06 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", @@ -59,6 +60,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" ] },{ diff --git a/util/doc.css b/util/doc.css index a868e44445..05d90bec6b 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,26 @@ div.decl p:first-child .first-word { font-size: 1.5em; } -.section pre { +pre.code { background-color: #b2c9db; padding: 5px; 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; +} + td { padding: 2px; } diff --git a/util/doc.go b/util/doc.go index a76a4d0912..da4b290756 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,65 @@ 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: + if strings.HasPrefix(block.Paragraph, "WARNING:") { + b.WriteString("

      ") + } else { + 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 +759,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 +783,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 +799,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}} -
          {{.Decl}}
          + {{if .Comment}}
          {{.Comment | markupComment}}
          {{end}} + {{if .Decl}}
          {{.Decl}}
          {{end}}
          {{end}}