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

Ensure service indicator is incremented only once, update RSA and ED25519 to ensure the state is locked #2112

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andrewhop
Copy link
Contributor

@andrewhop andrewhop commented Jan 13, 2025

Issues:

Resolves P186477736

Description of changes:

Currently the service indicator checks that before != after and multiple approved APIs might call each other. If a lock is missed a lower approved algorithm will increment the count which incorrectly marks the higher level API as approved. This is happening in three spots:

  1. Approved API's self tests run and increment the service indicator on first use
  2. In Ed25519 sign/verify the calls to SHA were incrementing the indicator
  3. In the Ec25519 and RSA keygen the PCT sign/verify was incrementing the count

This change updates the service indicator to enforce before + 1 == after with a debug assert.

Call-outs

This doesn't change the external behavior of the service indicator, what algorithms are approved, or what APIs are approved. The service indicator tests are unchanged. This change just ensures what we expect to be modifying the indicator is in the thing doing the update.

Testing:

The existing service indicator tests cover all approved APIs, and the new requirement that before + 1 = after ensures only one thing per call increments the count.

I took out a lock and verified it failed as expected:

[ RUN      ] ServiceIndicatorTest.ED25519KeyGen
Assertion failed: (before + 1 == after), function TestBody, file service_indicator_test.cc, line 5184.
OPENSSL_armcap=0x3D crypto/crypto_test [shard 8/10]
crypto/crypto_test failed to complete: signal: abort trap

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.

@andrewhop andrewhop requested a review from a team as a code owner January 13, 2025 19:07
@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2025

Codecov Report

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

Project coverage is 78.97%. Comparing base (ea58b3f) to head (354fc6f).

Files with missing lines Patch % Lines
crypto/curve25519_extra/curve25519_extra.c 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2112   +/-   ##
=======================================
  Coverage   78.97%   78.97%           
=======================================
  Files         611      611           
  Lines      105500   105504    +4     
  Branches    14938    14937    -1     
=======================================
+ Hits        83316    83322    +6     
+ Misses      21531    21529    -2     
  Partials      653      653           

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

smittals2
smittals2 previously approved these changes Jan 14, 2025
@skmcgrail
Copy link
Member

You'll want to rebase since Ed25519ph got merged in, but I think we are aligned / should be fixed now in the functions you touched.

@andrewhop andrewhop force-pushed the service_inidicator_test branch from 626dbf3 to 354fc6f Compare January 28, 2025 23:24
int res = ED25519ph_sign_digest_no_self_test(out_sig, digest, private_key,
context, context_len);
FIPS_service_indicator_unlock_state();
Copy link
Member

Choose a reason for hiding this comment

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

Here I thought I got it right in all of the places :)

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.

6 participants