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.

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

Conversation

geedo0
Copy link
Contributor

@geedo0 geedo0 commented Oct 9, 2024

Issues:

PQCrypto-103

Description of changes:

EVP_PKEY_keygen_deterministic allows callers to query the required
size for the seed parameter. This change avoids the unnecessary
allocation of memory for the out_pkey parameter to ensure that this
integer getter has no side effects.

Call-outs:

Point out areas that need special attention or support during the review process. Discuss architecture or design changes.

Testing:

How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?

Updated the unit tests.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@geedo0 geedo0 requested a review from a team as a code owner October 9, 2024 15:46
@codecov-commenter
Copy link

codecov-commenter commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.51%. Comparing base (9ff8458) to head (e0f4597).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crypto/fipsmodule/evp/evp_ctx.c 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1911      +/-   ##
==========================================
- Coverage   78.51%   78.51%   -0.01%     
==========================================
  Files         585      585              
  Lines       99681    99691      +10     
  Branches    14271    14275       +4     
==========================================
+ Hits        78265    78272       +7     
- Misses      20782    20783       +1     
- Partials      634      636       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

`EVP_PKEY_keygen_deterministic` allows callers to query the required
size for the `seed` parameter. This change avoids the unnecessary
allocation of memory for the `out_pkey` parameter to ensure that this
integer getter has no side effects.
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?

@geedo0 geedo0 enabled auto-merge (squash) October 15, 2024 15:07
@geedo0 geedo0 merged commit 0ea1cb1 into aws:main Oct 15, 2024
112 of 115 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants