Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid allocating EVP_PKEY on size checks #1911

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Avoid allocating EVP_PKEY on size checks #1911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions crypto/evp_extra/evp_extra_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2748,23 +2748,25 @@ TEST_P(PerKEMTest, KeygenSeedTest) {
EVP_PKEY *raw = nullptr;
ASSERT_TRUE(EVP_PKEY_keygen_init(ctx.get()));

// ---- 2. Test ctx->pkey == NULL ----
ASSERT_FALSE(EVP_PKEY_keygen_deterministic(ctx.get(), &raw, nullptr,
nullptr));
// ---- 2. Test passing in a context without the KEM parameters set. ----
size_t keygen_seed_len = GetParam().keygen_seed_len;
std::vector<uint8_t> keygen_seed(keygen_seed_len);
ASSERT_FALSE(EVP_PKEY_keygen_deterministic(ctx.get(), &raw,
keygen_seed.data(),
&keygen_seed_len));
EXPECT_EQ(EVP_R_NO_PARAMETERS_SET, ERR_GET_REASON(ERR_peek_last_error()));

// Setup the context with specific KEM parameters.
ASSERT_TRUE(EVP_PKEY_CTX_kem_set_params(ctx.get(), GetParam().nid));

// ---- 3. Test getting the lengths only ----
size_t keygen_seed_len;
ASSERT_TRUE(EVP_PKEY_keygen_deterministic(ctx.get(), &raw, nullptr,
keygen_seed_len = 0;
ASSERT_TRUE(EVP_PKEY_keygen_deterministic(ctx.get(), nullptr, nullptr,
&keygen_seed_len));
EXPECT_EQ(keygen_seed_len, GetParam().keygen_seed_len);


// ---- 4. Test failure mode on a seed len NULL ----
std::vector<uint8_t> keygen_seed(keygen_seed_len);
// ---- 4. Test failure mode on a seed len NULL ----
keygen_seed.resize(keygen_seed_len);
EXPECT_FALSE(EVP_PKEY_keygen_deterministic(ctx.get(), &raw, keygen_seed.data(),
nullptr));
EXPECT_EQ(ERR_R_PASSED_NULL_PARAMETER, ERR_GET_REASON(ERR_peek_last_error()));
Expand All @@ -2784,6 +2786,11 @@ TEST_P(PerKEMTest, KeygenSeedTest) {
EXPECT_FALSE(EVP_PKEY_keygen_deterministic(ctx.get(), &raw, big_keygen_seed.data(),
&keygen_seed_len));
EXPECT_EQ(EVP_R_INVALID_PARAMETERS, ERR_GET_REASON(ERR_peek_last_error()));

// ---- 7. Test failure mode on a non-NULL out_pkey and NULL seed ----
EXPECT_FALSE(EVP_PKEY_keygen_deterministic(ctx.get(), &raw, nullptr,
&keygen_seed_len));
EXPECT_EQ(EVP_R_INVALID_PARAMETERS, ERR_GET_REASON(ERR_peek_last_error()));
}

TEST_P(PerKEMTest, EncapsSeedTest) {
Expand Down
12 changes: 11 additions & 1 deletion crypto/fipsmodule/evp/evp_ctx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,7 +468,17 @@ int EVP_PKEY_keygen_deterministic(EVP_PKEY_CTX *ctx,
goto end;
}

if (!out_pkey) {
if ((out_pkey == NULL) != (seed == NULL)) {
OPENSSL_PUT_ERROR(EVP, EVP_R_INVALID_PARAMETERS);
goto end;
}

// Caller is performing a size check.
if (out_pkey == NULL && seed == NULL) {
if (!ctx->pmeth->keygen_deterministic(ctx, NULL, NULL, seed_len)) {
goto end;
}
ret = 1;
goto end;
}

Expand Down
6 changes: 3 additions & 3 deletions include/openssl/experimental/kem_deterministic_api.h
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A call out to the long-term plan for the include/openssl/experimental/ directory. We originally created this location to home experimental APIs for ML-KEM, as we were unsure on the exact requirements around FIPS 204 on allowing access to de-randomized modes. As FIPS 204 is now finalized we can reason about exact language.

FIPS 204 Sec 3.3 Page 16 reads:

Controlled access to internal functions. The key-encapsulation mechanism ML-KEM makes use
of internal, “derandomized” functions ML-KEM.KeyGen_internal, ML-KEM.Encaps_internal, and
ML-KEM.Decaps_internal, specified in Section 6. The interfaces for these functions should not
be made available to applications other than for testing purposes. In particular, the sampling of
random values required for key generation (as specified in ML-KEM.KeyGen) and encapsulation
(as specified in ML-KEM.Encaps) shall be performed by the cryptographic module.

i.e., that internal/de-randomized APIs such as these should not be made available to applications other than for testing purposes. While I am not prescribing any particular change in this PR, I wanted to make the call out that we (1) may like to add documentation from FIPS204 regarding the use of these APIs, (2) we may like to change the long-term location of these APIs now that FIPS204 is finalized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should mix these documentation concerns into this PR. However, there's certainly a discussion to be had on the many facets of this thread. A comment thread in this ancillary PR may not be the proper venue, but some questions that come to mind for me...

  • Should we limit adoption of these experimental APIs so that we can prevent complications in any inevitable code motions?
  • What exactly is meant by NIST's clause on testing? How will we choose to interpret that? In the strongSwan case, they are using it for their testing. Is that okay? Or does NIST mean for this to specifically refer to CAVP testing within the module itself?
  • Is there any new thinking around where these APIs will land today now that we have FIPS 204?

Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ extern "C" {
// performs deterministic keygen based on the value specified in |seed| of
// length |seed_len| bytes.
//
// If |seed| is set to NULL it is assumed that the caller is doing a size check:
// the function will write the size of the required seed in |seed_len| and return
// successfully.
// If |out_pkey| and |seed| are set to NULL it is assumed that the caller is
// doing a size check and the function will write the size of the required seed
// in |seed_len| and return successfully.
//
// EVP_PKEY_keygen_deterministic performs a deterministic key generation
// operation using the values from |ctx|, and the given |seed|. If |*out_pkey|
Expand Down
2 changes: 1 addition & 1 deletion util/fipstools/acvp/modulewrapper/modulewrapper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2918,7 +2918,7 @@ static bool ML_KEM_KEYGEN(const Span<const uint8_t> args[],
bssl::UniquePtr<EVP_PKEY_CTX> ctx(EVP_PKEY_CTX_new_id(EVP_PKEY_KEM, nullptr));
if (!EVP_PKEY_CTX_kem_set_params(ctx.get(), nid) ||
!EVP_PKEY_keygen_init(ctx.get()) ||
!EVP_PKEY_keygen_deterministic(ctx.get(), &raw, NULL, &seed_len) ||
!EVP_PKEY_keygen_deterministic(ctx.get(), NULL, NULL, &seed_len) ||
seed_len != seed.size() ||
!EVP_PKEY_keygen_deterministic(ctx.get(), &raw, seed.data(), &seed_len)) {
return false;
Expand Down
Loading