From 22d034ab2fc74e01570b92a2557104aba546ab52 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 27 Sep 2024 18:50:23 +0000 Subject: [PATCH 1/8] Add PKCS7-internal BIO_f_md --- crypto/CMakeLists.txt | 2 + crypto/pkcs7/bio/bio_deprecated_test.cc | 226 ++++++++++++++++++++++++ crypto/pkcs7/bio/md.c | 177 +++++++++++++++++++ crypto/pkcs7/internal.h | 9 + 4 files changed, 414 insertions(+) create mode 100644 crypto/pkcs7/bio/bio_deprecated_test.cc create mode 100644 crypto/pkcs7/bio/md.c diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt index b2314d263c..e9fa435123 100644 --- a/crypto/CMakeLists.txt +++ b/crypto/CMakeLists.txt @@ -460,6 +460,7 @@ add_library( pem/pem_pkey.c pem/pem_x509.c pem/pem_xaux.c + pkcs7/bio/md.c pkcs7/pkcs7.c pkcs7/pkcs7_asn1.c pkcs7/pkcs7_x509.c @@ -814,6 +815,7 @@ if(BUILD_TESTING) obj/obj_test.cc ocsp/ocsp_test.cc pem/pem_test.cc + pkcs7/bio/bio_deprecated_test.cc pkcs7/pkcs7_test.cc pkcs8/pkcs8_test.cc pkcs8/pkcs12_test.cc diff --git a/crypto/pkcs7/bio/bio_deprecated_test.cc b/crypto/pkcs7/bio/bio_deprecated_test.cc new file mode 100644 index 0000000000..9a46268c47 --- /dev/null +++ b/crypto/pkcs7/bio/bio_deprecated_test.cc @@ -0,0 +1,226 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 OR ISC + +#include + +#include +#include +#include +#include + +#include "../../test/test_util.h" +#include "../internal.h" + +struct MessageDigestParams { + const char name[40]; + const EVP_MD *(*md)(void); +}; + +static const struct MessageDigestParams MessageDigests[] = { + {"MD5", EVP_md5}, + {"SHA1", EVP_sha1}, + {"SHA224", EVP_sha224}, + {"SHA256", EVP_sha256}, + {"SHA284", EVP_sha384}, + {"SHA512", EVP_sha512}, + {"SHA512_224", EVP_sha512_224}, + {"SHA512_256", EVP_sha512_256}, + {"SHA3_224", EVP_sha3_224}, + {"SHA3_256", EVP_sha3_256}, + {"SHA3_384", EVP_sha3_384}, + {"SHA3_512", EVP_sha3_512}, +}; + +class BIODeprecatedTest : public testing::TestWithParam {}; + +INSTANTIATE_TEST_SUITE_P( + PKCS7Test, BIODeprecatedTest, testing::ValuesIn(MessageDigests), + [](const testing::TestParamInfo ¶ms) + -> std::string { return params.param.name; }); + +TEST_P(BIODeprecatedTest, MessageDigestBasic) { + uint8_t message[1024 * 8]; + uint8_t buf[16 * 1024]; + std::vector message_vec; + std::vector buf_vec; + bssl::UniquePtr bio; + bssl::UniquePtr bio_md; + bssl::UniquePtr bio_mem; + bssl::UniquePtr ctx; + + OPENSSL_memset(message, 'A', sizeof(message)); + OPENSSL_memset(buf, '\0', sizeof(buf)); + + const EVP_MD *md = GetParam().md(); + ASSERT_TRUE(md); + + // Simple initialization and error cases + bio_md.reset(BIO_new(BIO_f_md())); + ASSERT_TRUE(bio_md); + EXPECT_FALSE(BIO_reset(bio_md.get())); + EXPECT_TRUE(BIO_set_md(bio_md.get(), (EVP_MD *)md)); + EVP_MD_CTX *ctx_tmp; // |bio_md| owns the context, we just take a ref here + EXPECT_TRUE(BIO_get_md_ctx(bio_md.get(), &ctx_tmp)); + EXPECT_EQ(EVP_MD_type(md), EVP_MD_CTX_type(ctx_tmp)); + EXPECT_EQ(md, EVP_MD_CTX_md(ctx_tmp)); // for static *EVP_MD_CTX, ptrs equal + EXPECT_FALSE(BIO_ctrl(bio_md.get(), BIO_C_GET_MD, 0, nullptr)); + EXPECT_FALSE(BIO_ctrl(bio_md.get(), BIO_C_SET_MD_CTX, 0, nullptr)); + EXPECT_FALSE(BIO_ctrl(bio_md.get(), BIO_C_DO_STATE_MACHINE, 0, nullptr)); + EXPECT_FALSE(BIO_ctrl(bio_md.get(), BIO_CTRL_DUP, 0, nullptr)); + EXPECT_FALSE(BIO_ctrl(bio_md.get(), BIO_CTRL_GET_CALLBACK, 0, nullptr)); + EXPECT_FALSE(BIO_ctrl(bio_md.get(), BIO_CTRL_SET_CALLBACK, 0, nullptr)); + EXPECT_FALSE(BIO_read(bio_md.get(), buf, 0)); + EXPECT_FALSE(BIO_write(bio_md.get(), buf, 0)); + EXPECT_EQ(0UL, BIO_number_read(bio_md.get())); + EXPECT_EQ(0UL, BIO_number_written(bio_md.get())); + EXPECT_FALSE(BIO_gets(bio_md.get(), (char *)buf, EVP_MD_size(md) - 1)); + + // Write-through digest BIO + bio_md.reset(BIO_new(BIO_f_md())); + ASSERT_TRUE(bio_md); + EXPECT_TRUE(BIO_set_md(bio_md.get(), (void *)md)); + bio_mem.reset(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio_mem); + bio.reset(BIO_push(bio_md.get(), bio_mem.get())); + ASSERT_TRUE(bio); + EXPECT_TRUE(BIO_write(bio.get(), message, sizeof(message))); + unsigned digest_len = BIO_gets(bio_md.get(), (char *)buf, sizeof(buf)); + buf_vec.clear(); + buf_vec.insert(buf_vec.begin(), buf, buf + digest_len); + OPENSSL_memset(buf, '\0', sizeof(buf)); + message_vec.clear(); + int rsize; + while ((rsize = BIO_read(bio_mem.get(), buf, sizeof(buf))) > 0) { + message_vec.insert(message_vec.end(), buf, buf + rsize); + } + ctx.reset(EVP_MD_CTX_new()); + ASSERT_TRUE(EVP_DigestInit_ex(ctx.get(), md, NULL)); + ASSERT_TRUE( + EVP_DigestUpdate(ctx.get(), message_vec.data(), message_vec.size())); + ASSERT_TRUE(EVP_DigestFinal_ex(ctx.get(), buf, &digest_len)); + EXPECT_EQ(Bytes(buf_vec.data(), buf_vec.size()), Bytes(buf, digest_len)); + bio_md.release(); // |bio| took ownership + bio_mem.release(); // |bio| took ownership + + // Read-through digest BIO + bio_md.reset(BIO_new(BIO_f_md())); + ASSERT_TRUE(bio_md); + EXPECT_TRUE(BIO_set_md(bio_md.get(), (void *)md)); + bio_mem.reset(BIO_new_mem_buf(message, sizeof(message))); + ASSERT_TRUE(bio_mem); + bio.reset(BIO_push(bio_md.get(), bio_mem.get())); + ASSERT_TRUE(bio); + message_vec.clear(); + OPENSSL_memset(buf, '\0', sizeof(buf)); + while ((rsize = BIO_read(bio.get(), buf, sizeof(buf))) > 0) { + message_vec.insert(message_vec.begin(), buf, buf + rsize); + } + EXPECT_EQ(Bytes(message_vec.data(), message_vec.size()), + Bytes(message, sizeof(message))); + digest_len = BIO_gets(bio_md.get(), (char *)buf, sizeof(buf)); + buf_vec.clear(); + buf_vec.insert(buf_vec.begin(), buf, buf + digest_len); + ctx.reset(EVP_MD_CTX_new()); + ASSERT_TRUE(EVP_DigestInit_ex(ctx.get(), md, NULL)); + ASSERT_TRUE( + EVP_DigestUpdate(ctx.get(), message_vec.data(), message_vec.size())); + ASSERT_TRUE(EVP_DigestFinal_ex(ctx.get(), buf, &digest_len)); + EXPECT_EQ(Bytes(buf, digest_len), Bytes(buf_vec.data(), buf_vec.size())); + EXPECT_EQ(Bytes(buf_vec.data(), buf_vec.size()), Bytes(buf, digest_len)); + // Resetting |bio_md| should reset digest state, elicit different digest + // output + EXPECT_TRUE(BIO_reset(bio.get())); + digest_len = BIO_gets(bio_md.get(), (char *)buf, sizeof(buf)); + EXPECT_NE(Bytes(buf_vec.data(), buf_vec.size()), Bytes(buf, digest_len)); + bio_md.release(); // |bio| took ownership + bio_mem.release(); // |bio| took ownership +} + +TEST_P(BIODeprecatedTest, MessageDigestRandomized) { + uint8_t message_buf[8 * 1024]; + uint8_t digest_buf[EVP_MAX_MD_SIZE]; + std::vector message; + std::vector expected_digest; + bssl::UniquePtr bio; + bssl::UniquePtr bio_md; + bssl::UniquePtr bio_mem; + bssl::UniquePtr ctx; + + const EVP_MD *md = GetParam().md(); + ASSERT_TRUE(md); + + const size_t block_size = EVP_MD_block_size(md); + std::vector> io_patterns = { + {}, + {0}, + {1}, + {8, 8, 8, 8}, + {block_size - 1, 1, block_size + 1, block_size, block_size - 1}, + {4, 1, 5, 3, 2, 0, 1, sizeof(message_buf), 133, 4555, 22, 4, 7964, 1234}, + }; + + for (auto io_pattern : io_patterns) { + message.clear(); + expected_digest.clear(); + ctx.reset(EVP_MD_CTX_new()); + EVP_DigestInit_ex(ctx.get(), md, NULL); + // Construct overall message and its expected expected_digest + for (auto io_size : io_pattern) { + ASSERT_LE(io_size, sizeof(message_buf)); + RAND_bytes(message_buf, io_size); + message.insert(message.end(), &message_buf[0], &message_buf[io_size]); + } + EVP_DigestUpdate(ctx.get(), message.data(), message.size()); + unsigned digest_size; + EVP_DigestFinal_ex(ctx.get(), digest_buf, &digest_size); + ASSERT_EQ(EVP_MD_CTX_size(ctx.get()), digest_size); + expected_digest.insert(expected_digest.begin(), &digest_buf[0], + &digest_buf[digest_size]); + OPENSSL_cleanse(digest_buf, sizeof(digest_buf)); + + // Write-through digest BIO, check against expectation + bio_md.reset(BIO_new(BIO_f_md())); + ASSERT_TRUE(bio_md); + EXPECT_TRUE(BIO_set_md(bio_md.get(), (void *)md)); + bio_mem.reset(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio_mem); + bio.reset(BIO_push(bio_md.get(), bio_mem.get())); + ASSERT_TRUE(bio); + int pos = 0; + for (auto io_size : io_pattern) { + int wsize = BIO_write(bio.get(), (char *)(message.data() + pos), io_size); + EXPECT_EQ((int)io_size, wsize); + pos += io_size; + } + digest_size = + BIO_gets(bio_md.get(), (char *)digest_buf, sizeof(digest_buf)); + ASSERT_EQ(EVP_MD_CTX_size(ctx.get()), digest_size); + EXPECT_EQ(Bytes(expected_digest.data(), expected_digest.size()), + Bytes(digest_buf, digest_size)); + OPENSSL_cleanse(digest_buf, sizeof(digest_buf)); + bio_md.release(); // |bio| took ownership + bio_mem.release(); // |bio| took ownership + + // Read-through digest BIO, check against expectation + bio_md.reset(BIO_new(BIO_f_md())); + ASSERT_TRUE(bio_md); + EXPECT_TRUE(BIO_set_md(bio_md.get(), (void *)md)); + bio_mem.reset(BIO_new_mem_buf(message.data(), message.size())); + ASSERT_TRUE(bio_mem); + bio.reset(BIO_push(bio_md.get(), bio_mem.get())); + ASSERT_TRUE(bio); + for (auto io_size : io_pattern) { + int rsize = BIO_read(bio.get(), message_buf, io_size); + EXPECT_EQ((int)io_size, rsize); + } + EXPECT_TRUE(BIO_eof(bio.get())); + digest_size = + BIO_gets(bio_md.get(), (char *)digest_buf, sizeof(digest_buf)); + ASSERT_EQ(EVP_MD_CTX_size(ctx.get()), digest_size); + EXPECT_EQ(Bytes(expected_digest.data(), expected_digest.size()), + Bytes(digest_buf, digest_size)); + OPENSSL_cleanse(digest_buf, sizeof(digest_buf)); + bio_md.release(); // |bio| took ownership + bio_mem.release(); // |bio| took ownership + } +} diff --git a/crypto/pkcs7/bio/md.c b/crypto/pkcs7/bio/md.c new file mode 100644 index 0000000000..5a21b8cdf7 --- /dev/null +++ b/crypto/pkcs7/bio/md.c @@ -0,0 +1,177 @@ +// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 OR ISC + +#include +#include +#include +#include +#include "../../internal.h" +#include "../crypto/bio/internal.h" +#include "../internal.h" + +// BIO_put and BIO_get both add to the digest, BIO_gets returns the digest +static int md_write(BIO *h, char const *buf, int num); +static int md_read(BIO *h, char *buf, int size); +static int md_gets(BIO *h, char *str, int size); +static long md_ctrl(BIO *h, int cmd, long arg1, void *arg2); +static int md_new(BIO *h); +static int md_free(BIO *data); + + +static const BIO_METHOD methods_md = { + BIO_TYPE_MD, "message digest", md_write, md_read, /*puts*/ NULL, + md_gets, md_ctrl, md_new, md_free, /*callback_ctrl*/ NULL, +}; +const BIO_METHOD *BIO_f_md(void) { return &methods_md; } + +static int md_new(BIO *b) { + GUARD_PTR(b); + EVP_MD_CTX *ctx; + + ctx = EVP_MD_CTX_new(); + if (ctx == NULL) { + return 0; + } + + BIO_set_data(b, ctx); + + return 1; +} + +static int md_free(BIO *b) { + GUARD_PTR(b); + EVP_MD_CTX_free(BIO_get_data(b)); + BIO_set_data(b, NULL); + BIO_set_init(b, 0); + + return 1; +} + +static int md_read(BIO *b, char *out, int outl) { + GUARD_PTR(b); + GUARD_PTR(out); + int ret = 0; + EVP_MD_CTX *ctx; + BIO *next; + + ctx = BIO_get_data(b); + next = BIO_next(b); + + if ((ctx == NULL) || (next == NULL) || outl <= 0) { + return 0; + } + + ret = BIO_read(next, out, outl); + if (BIO_get_init(b)) { + if (ret > 0) { + if (EVP_DigestUpdate(ctx, (unsigned char *)out, ret) <= 0) { + return -1; + } + } + } + BIO_clear_retry_flags(b); + BIO_copy_next_retry(b); + return ret; +} + +static int md_write(BIO *b, const char *in, int inl) { + GUARD_PTR(b); + GUARD_PTR(in); + int ret = 0; + EVP_MD_CTX *ctx; + BIO *next; + + if (inl <= 0) { + return 0; + } + + ctx = BIO_get_data(b); + next = BIO_next(b); + if ((ctx != NULL) && (next != NULL)) { + ret = BIO_write(next, in, inl); + } + + if (BIO_get_init(b)) { + if (ret > 0) { + if (!EVP_DigestUpdate(ctx, (const unsigned char *)in, ret)) { + BIO_clear_retry_flags(b); + return 0; + } + } + } + if (next != NULL) { + BIO_clear_retry_flags(b); + BIO_copy_next_retry(b); + } + return ret; +} + +static long md_ctrl(BIO *b, int cmd, long num, void *ptr) { + GUARD_PTR(b); + EVP_MD_CTX *ctx, **pctx; + EVP_MD *md; + long ret = 1; + BIO *next; + + ctx = BIO_get_data(b); + next = BIO_next(b); + + switch (cmd) { + case BIO_CTRL_RESET: + if (BIO_get_init(b)) { + ret = EVP_DigestInit_ex(ctx, EVP_MD_CTX_md(ctx), NULL); + } else { + ret = 0; + } + if (ret > 0) { + ret = BIO_ctrl(next, cmd, num, ptr); + } + break; + case BIO_C_GET_MD_CTX: + pctx = ptr; + *pctx = ctx; + break; + case BIO_C_SET_MD: + md = ptr; + ret = EVP_DigestInit_ex(ctx, md, NULL); + if (ret > 0) { + BIO_set_init(b, 1); + } + break; + // OpenSSL implements these, but because we don't need them and cipher BIO + // is internal, we can fail loudly if they're called. If this case is hit, + // it likely means you're making a change that will require implementing + // these. + case BIO_C_GET_MD: + case BIO_C_SET_MD_CTX: + case BIO_C_DO_STATE_MACHINE: + case BIO_CTRL_DUP: + case BIO_CTRL_GET_CALLBACK: + case BIO_CTRL_SET_CALLBACK: + OPENSSL_PUT_ERROR(PKCS7, ERR_R_BIO_LIB); + return 0; + default: + ret = BIO_ctrl(next, cmd, num, ptr); + break; + } + return ret; +} + +static int md_gets(BIO *b, char *buf, int size) { + GUARD_PTR(b); + GUARD_PTR(buf); + EVP_MD_CTX *ctx; + unsigned int ret; + + ctx = BIO_get_data(b); + + if (((size_t)size) < EVP_MD_CTX_size(ctx)) { + return 0; + } + + if (EVP_DigestFinal_ex(ctx, (unsigned char *)buf, &ret) <= 0) { + return -1; + } + + return ret; +} diff --git a/crypto/pkcs7/internal.h b/crypto/pkcs7/internal.h index 4cdda60d00..be80dea060 100644 --- a/crypto/pkcs7/internal.h +++ b/crypto/pkcs7/internal.h @@ -199,6 +199,15 @@ int pkcs7_add_signed_data(CBB *out, int (*signer_infos_cb)(CBB *out, const void *arg), const void *arg); +// BIO_f_md is used internally by the pkcs7 module. It is not recommended +// for external use. +OPENSSL_EXPORT const BIO_METHOD *BIO_f_md(void); + +// BIO_get_md_ctx writes a reference of |b|'s EVP_MD_CTX* to |*mdcp| +#define BIO_get_md_ctx(b, mdcp) BIO_ctrl(b, BIO_C_GET_MD_CTX, 0, mdcp) + +// BIO_set_md set's |b|'s EVP_MD* to |md| +#define BIO_set_md(b, md) BIO_ctrl(b, BIO_C_SET_MD, 0, md) #if defined(__cplusplus) } // extern C From 54aad500aa9439067e28c34b8b7cbb64e5d9a6ab Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Fri, 4 Oct 2024 15:10:34 +0000 Subject: [PATCH 2/8] Fix comment --- crypto/pkcs7/bio/md.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crypto/pkcs7/bio/md.c b/crypto/pkcs7/bio/md.c index 5a21b8cdf7..54fc1d3af9 100644 --- a/crypto/pkcs7/bio/md.c +++ b/crypto/pkcs7/bio/md.c @@ -9,7 +9,8 @@ #include "../crypto/bio/internal.h" #include "../internal.h" -// BIO_put and BIO_get both add to the digest, BIO_gets returns the digest +// |md_write| and |md_read| both contribute to the digest, |md_gets| returns +// digest contents static int md_write(BIO *h, char const *buf, int num); static int md_read(BIO *h, char *buf, int size); static int md_gets(BIO *h, char *str, int size); From b0825750d52ea71027be399dcba961d91f442eae Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Wed, 23 Oct 2024 20:43:54 +0000 Subject: [PATCH 3/8] Rename md bio test --- crypto/CMakeLists.txt | 2 +- .../pkcs7/bio/{bio_deprecated_test.cc => bio_md_test.cc} | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) rename crypto/pkcs7/bio/{bio_deprecated_test.cc => bio_md_test.cc} (96%) diff --git a/crypto/CMakeLists.txt b/crypto/CMakeLists.txt index e9fa435123..991b51638b 100644 --- a/crypto/CMakeLists.txt +++ b/crypto/CMakeLists.txt @@ -815,7 +815,7 @@ if(BUILD_TESTING) obj/obj_test.cc ocsp/ocsp_test.cc pem/pem_test.cc - pkcs7/bio/bio_deprecated_test.cc + pkcs7/bio/bio_md_test.cc pkcs7/pkcs7_test.cc pkcs8/pkcs8_test.cc pkcs8/pkcs12_test.cc diff --git a/crypto/pkcs7/bio/bio_deprecated_test.cc b/crypto/pkcs7/bio/bio_md_test.cc similarity index 96% rename from crypto/pkcs7/bio/bio_deprecated_test.cc rename to crypto/pkcs7/bio/bio_md_test.cc index 9a46268c47..a7ceac8456 100644 --- a/crypto/pkcs7/bio/bio_deprecated_test.cc +++ b/crypto/pkcs7/bio/bio_md_test.cc @@ -31,14 +31,14 @@ static const struct MessageDigestParams MessageDigests[] = { {"SHA3_512", EVP_sha3_512}, }; -class BIODeprecatedTest : public testing::TestWithParam {}; +class BIOMessageDigestTest : public testing::TestWithParam {}; INSTANTIATE_TEST_SUITE_P( - PKCS7Test, BIODeprecatedTest, testing::ValuesIn(MessageDigests), + PKCS7Test, BIOMessageDigestTest, testing::ValuesIn(MessageDigests), [](const testing::TestParamInfo ¶ms) -> std::string { return params.param.name; }); -TEST_P(BIODeprecatedTest, MessageDigestBasic) { +TEST_P(BIOMessageDigestTest, MessageDigestBasic) { uint8_t message[1024 * 8]; uint8_t buf[16 * 1024]; std::vector message_vec; @@ -136,7 +136,7 @@ TEST_P(BIODeprecatedTest, MessageDigestBasic) { bio_mem.release(); // |bio| took ownership } -TEST_P(BIODeprecatedTest, MessageDigestRandomized) { +TEST_P(BIOMessageDigestTest, MessageDigestRandomized) { uint8_t message_buf[8 * 1024]; uint8_t digest_buf[EVP_MAX_MD_SIZE]; std::vector message; From 0d8a33dc0eeac380355ede2521b6ca581a5ac71a Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Thu, 24 Oct 2024 15:11:03 +0000 Subject: [PATCH 4/8] Address PR feedback --- crypto/pkcs7/bio/bio_md_test.cc | 38 +++++++++++++++++++++++++++------ crypto/pkcs7/bio/md.c | 38 +++++++++++++++------------------ crypto/pkcs7/internal.h | 3 ++- 3 files changed, 50 insertions(+), 29 deletions(-) diff --git a/crypto/pkcs7/bio/bio_md_test.cc b/crypto/pkcs7/bio/bio_md_test.cc index a7ceac8456..575974ff0f 100644 --- a/crypto/pkcs7/bio/bio_md_test.cc +++ b/crypto/pkcs7/bio/bio_md_test.cc @@ -3,9 +3,10 @@ #include +#include "stdlib.h" + #include #include -#include #include #include "../../test/test_util.h" @@ -31,14 +32,15 @@ static const struct MessageDigestParams MessageDigests[] = { {"SHA3_512", EVP_sha3_512}, }; -class BIOMessageDigestTest : public testing::TestWithParam {}; +class BIOMessageDigestTest + : public testing::TestWithParam {}; INSTANTIATE_TEST_SUITE_P( PKCS7Test, BIOMessageDigestTest, testing::ValuesIn(MessageDigests), [](const testing::TestParamInfo ¶ms) -> std::string { return params.param.name; }); -TEST_P(BIOMessageDigestTest, MessageDigestBasic) { +TEST_P(BIOMessageDigestTest, Basic) { uint8_t message[1024 * 8]; uint8_t buf[16 * 1024]; std::vector message_vec; @@ -75,6 +77,22 @@ TEST_P(BIOMessageDigestTest, MessageDigestBasic) { EXPECT_EQ(0UL, BIO_number_written(bio_md.get())); EXPECT_FALSE(BIO_gets(bio_md.get(), (char *)buf, EVP_MD_size(md) - 1)); + // Pre-initialization IO should fail, but |BIO_get_md_ctx| should do init + bio_md.reset(BIO_new(BIO_f_md())); + ASSERT_TRUE(bio_md); + bio_mem.reset(BIO_new(BIO_s_mem())); + ASSERT_TRUE(bio_mem); + bio.reset(BIO_push(bio_md.get(), bio_mem.get())); + ASSERT_TRUE(bio); + EXPECT_GT(1, BIO_write(bio.get(), message, sizeof(message))); + EXPECT_GT(1, BIO_read(bio.get(), message, sizeof(message))); + EXPECT_TRUE(BIO_get_md_ctx(bio_md.get(), &ctx_tmp)); + ASSERT_TRUE(EVP_DigestInit_ex(ctx_tmp, md, NULL)); + EXPECT_TRUE(BIO_write(bio.get(), message, sizeof(message))); + EXPECT_TRUE(BIO_read(bio.get(), message, sizeof(message))); + bio_md.release(); // |bio| took ownership + bio_mem.release(); // |bio| took ownership + // Write-through digest BIO bio_md.reset(BIO_new(BIO_f_md())); ASSERT_TRUE(bio_md); @@ -136,8 +154,9 @@ TEST_P(BIOMessageDigestTest, MessageDigestBasic) { bio_mem.release(); // |bio| took ownership } -TEST_P(BIOMessageDigestTest, MessageDigestRandomized) { - uint8_t message_buf[8 * 1024]; +TEST_P(BIOMessageDigestTest, Randomized) { + const size_t max_len = 8 * 1024; + uint8_t message_buf[max_len]; uint8_t digest_buf[EVP_MAX_MD_SIZE]; std::vector message; std::vector expected_digest; @@ -150,14 +169,18 @@ TEST_P(BIOMessageDigestTest, MessageDigestRandomized) { ASSERT_TRUE(md); const size_t block_size = EVP_MD_block_size(md); + srand(42); std::vector> io_patterns = { {}, {0}, {1}, {8, 8, 8, 8}, {block_size - 1, 1, block_size + 1, block_size, block_size - 1}, - {4, 1, 5, 3, 2, 0, 1, sizeof(message_buf), 133, 4555, 22, 4, 7964, 1234}, + {4, 1, 5, 3, 2, 0, 1, max_len, 133, 4555, 22, 4, 7964, 1234}, }; + std::vector v(1000); + std::generate(v.begin(), v.end(), [max_len] { return rand() % max_len; }); + io_patterns.push_back(v); for (auto io_pattern : io_patterns) { message.clear(); @@ -167,7 +190,8 @@ TEST_P(BIOMessageDigestTest, MessageDigestRandomized) { // Construct overall message and its expected expected_digest for (auto io_size : io_pattern) { ASSERT_LE(io_size, sizeof(message_buf)); - RAND_bytes(message_buf, io_size); + char c = rand() % 256; + OPENSSL_memset(message_buf, c, io_size); message.insert(message.end(), &message_buf[0], &message_buf[io_size]); } EVP_DigestUpdate(ctx.get(), message.data(), message.size()); diff --git a/crypto/pkcs7/bio/md.c b/crypto/pkcs7/bio/md.c index 54fc1d3af9..985e6bf996 100644 --- a/crypto/pkcs7/bio/md.c +++ b/crypto/pkcs7/bio/md.c @@ -63,13 +63,12 @@ static int md_read(BIO *b, char *out, int outl) { } ret = BIO_read(next, out, outl); - if (BIO_get_init(b)) { - if (ret > 0) { - if (EVP_DigestUpdate(ctx, (unsigned char *)out, ret) <= 0) { - return -1; - } + if (ret > 0) { + if (EVP_DigestUpdate(ctx, (unsigned char *)out, ret) <= 0) { + return -1; } } + BIO_clear_retry_flags(b); BIO_copy_next_retry(b); return ret; @@ -82,28 +81,23 @@ static int md_write(BIO *b, const char *in, int inl) { EVP_MD_CTX *ctx; BIO *next; - if (inl <= 0) { - return 0; - } - ctx = BIO_get_data(b); next = BIO_next(b); - if ((ctx != NULL) && (next != NULL)) { - ret = BIO_write(next, in, inl); + + if ((ctx == NULL) || (next == NULL) || inl <= 0) { + return 0; } - if (BIO_get_init(b)) { - if (ret > 0) { - if (!EVP_DigestUpdate(ctx, (const unsigned char *)in, ret)) { - BIO_clear_retry_flags(b); - return 0; - } + ret = BIO_write(next, in, inl); + if (ret > 0) { + if (!EVP_DigestUpdate(ctx, (const unsigned char *)in, ret)) { + BIO_clear_retry_flags(b); + return 0; } } - if (next != NULL) { - BIO_clear_retry_flags(b); - BIO_copy_next_retry(b); - } + + BIO_clear_retry_flags(b); + BIO_copy_next_retry(b); return ret; } @@ -131,8 +125,10 @@ static long md_ctrl(BIO *b, int cmd, long num, void *ptr) { case BIO_C_GET_MD_CTX: pctx = ptr; *pctx = ctx; + BIO_set_init(b, 1); break; case BIO_C_SET_MD: + GUARD_PTR(ptr); md = ptr; ret = EVP_DigestInit_ex(ctx, md, NULL); if (ret > 0) { diff --git a/crypto/pkcs7/internal.h b/crypto/pkcs7/internal.h index be80dea060..8fb8c1781a 100644 --- a/crypto/pkcs7/internal.h +++ b/crypto/pkcs7/internal.h @@ -200,7 +200,8 @@ int pkcs7_add_signed_data(CBB *out, const void *arg); // BIO_f_md is used internally by the pkcs7 module. It is not recommended -// for external use. +// for external use. The BIO must be initialized with |BIO_set_md| or +// |BIO_get_md_ctx| before it can be used. OPENSSL_EXPORT const BIO_METHOD *BIO_f_md(void); // BIO_get_md_ctx writes a reference of |b|'s EVP_MD_CTX* to |*mdcp| From 4c01dd8c72cbbc5b25de73a11419d727f8eeedbc Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Thu, 24 Oct 2024 17:29:57 +0000 Subject: [PATCH 5/8] Fix strict compiler error --- crypto/pkcs7/bio/bio_md_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/pkcs7/bio/bio_md_test.cc b/crypto/pkcs7/bio/bio_md_test.cc index 575974ff0f..12b45be0db 100644 --- a/crypto/pkcs7/bio/bio_md_test.cc +++ b/crypto/pkcs7/bio/bio_md_test.cc @@ -179,7 +179,7 @@ TEST_P(BIOMessageDigestTest, Randomized) { {4, 1, 5, 3, 2, 0, 1, max_len, 133, 4555, 22, 4, 7964, 1234}, }; std::vector v(1000); - std::generate(v.begin(), v.end(), [max_len] { return rand() % max_len; }); + std::generate(v.begin(), v.end(), [] { return rand() % max_len; }); io_patterns.push_back(v); for (auto io_pattern : io_patterns) { From 9910dc55d0f5f9f51957b8b88670c809d9ed87e1 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Mon, 28 Oct 2024 19:44:49 +0000 Subject: [PATCH 6/8] PR feedback --- crypto/pkcs7/bio/md.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/crypto/pkcs7/bio/md.c b/crypto/pkcs7/bio/md.c index 985e6bf996..d233939ab3 100644 --- a/crypto/pkcs7/bio/md.c +++ b/crypto/pkcs7/bio/md.c @@ -9,21 +9,6 @@ #include "../crypto/bio/internal.h" #include "../internal.h" -// |md_write| and |md_read| both contribute to the digest, |md_gets| returns -// digest contents -static int md_write(BIO *h, char const *buf, int num); -static int md_read(BIO *h, char *buf, int size); -static int md_gets(BIO *h, char *str, int size); -static long md_ctrl(BIO *h, int cmd, long arg1, void *arg2); -static int md_new(BIO *h); -static int md_free(BIO *data); - - -static const BIO_METHOD methods_md = { - BIO_TYPE_MD, "message digest", md_write, md_read, /*puts*/ NULL, - md_gets, md_ctrl, md_new, md_free, /*callback_ctrl*/ NULL, -}; -const BIO_METHOD *BIO_f_md(void) { return &methods_md; } static int md_new(BIO *b) { GUARD_PTR(b); @@ -65,7 +50,7 @@ static int md_read(BIO *b, char *out, int outl) { ret = BIO_read(next, out, outl); if (ret > 0) { if (EVP_DigestUpdate(ctx, (unsigned char *)out, ret) <= 0) { - return -1; + ret = -1; } } @@ -90,9 +75,8 @@ static int md_write(BIO *b, const char *in, int inl) { ret = BIO_write(next, in, inl); if (ret > 0) { - if (!EVP_DigestUpdate(ctx, (const unsigned char *)in, ret)) { - BIO_clear_retry_flags(b); - return 0; + if (EVP_DigestUpdate(ctx, (const unsigned char *)in, ret) <= 0) { + ret = -1; } } @@ -172,3 +156,18 @@ static int md_gets(BIO *b, char *buf, int size) { return ret; } + +static const BIO_METHOD methods_md = { + BIO_TYPE_MD, // type + "message digest", // name + md_write, // bwrite + md_read, // bread + NULL, // bputs + md_gets, // bgets + md_ctrl, // ctrl + md_new, // create + md_free, // destroy + NULL, // callback_ctrl +}; + +const BIO_METHOD *BIO_f_md(void) { return &methods_md; } From e89a4c783d85839beba9ba0b7a04c93af1b5acfd Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Tue, 29 Oct 2024 13:45:42 +0000 Subject: [PATCH 7/8] Fix test lambda capture to appease MSVC --- crypto/pkcs7/bio/bio_md_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crypto/pkcs7/bio/bio_md_test.cc b/crypto/pkcs7/bio/bio_md_test.cc index 12b45be0db..575974ff0f 100644 --- a/crypto/pkcs7/bio/bio_md_test.cc +++ b/crypto/pkcs7/bio/bio_md_test.cc @@ -179,7 +179,7 @@ TEST_P(BIOMessageDigestTest, Randomized) { {4, 1, 5, 3, 2, 0, 1, max_len, 133, 4555, 22, 4, 7964, 1234}, }; std::vector v(1000); - std::generate(v.begin(), v.end(), [] { return rand() % max_len; }); + std::generate(v.begin(), v.end(), [max_len] { return rand() % max_len; }); io_patterns.push_back(v); for (auto io_pattern : io_patterns) { From b834fd966b0215b57a63db6ff173fa53fca13e12 Mon Sep 17 00:00:00 2001 From: WillChilds-Klein Date: Tue, 29 Oct 2024 14:27:44 +0000 Subject: [PATCH 8/8] Use macro instead of const var --- crypto/pkcs7/bio/bio_md_test.cc | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/crypto/pkcs7/bio/bio_md_test.cc b/crypto/pkcs7/bio/bio_md_test.cc index 575974ff0f..e7572c9935 100644 --- a/crypto/pkcs7/bio/bio_md_test.cc +++ b/crypto/pkcs7/bio/bio_md_test.cc @@ -12,6 +12,8 @@ #include "../../test/test_util.h" #include "../internal.h" +#define MESSAGE_LEN 8 * 1024 + struct MessageDigestParams { const char name[40]; const EVP_MD *(*md)(void); @@ -41,8 +43,8 @@ INSTANTIATE_TEST_SUITE_P( -> std::string { return params.param.name; }); TEST_P(BIOMessageDigestTest, Basic) { - uint8_t message[1024 * 8]; - uint8_t buf[16 * 1024]; + uint8_t message[MESSAGE_LEN]; + uint8_t buf[2 * MESSAGE_LEN]; std::vector message_vec; std::vector buf_vec; bssl::UniquePtr bio; @@ -155,8 +157,7 @@ TEST_P(BIOMessageDigestTest, Basic) { } TEST_P(BIOMessageDigestTest, Randomized) { - const size_t max_len = 8 * 1024; - uint8_t message_buf[max_len]; + uint8_t message_buf[MESSAGE_LEN]; uint8_t digest_buf[EVP_MAX_MD_SIZE]; std::vector message; std::vector expected_digest; @@ -176,10 +177,10 @@ TEST_P(BIOMessageDigestTest, Randomized) { {1}, {8, 8, 8, 8}, {block_size - 1, 1, block_size + 1, block_size, block_size - 1}, - {4, 1, 5, 3, 2, 0, 1, max_len, 133, 4555, 22, 4, 7964, 1234}, + {4, 1, 5, 3, 2, 0, 1, MESSAGE_LEN, 133, 4555, 22, 4, 7964, 1234}, }; std::vector v(1000); - std::generate(v.begin(), v.end(), [max_len] { return rand() % max_len; }); + std::generate(v.begin(), v.end(), [] { return rand() % MESSAGE_LEN; }); io_patterns.push_back(v); for (auto io_pattern : io_patterns) {