Skip to content

Commit

Permalink
Ensure service indicator is incremented only once, update RSA and ED2…
Browse files Browse the repository at this point in the history
…5519 to ensure the state is locked (#2112)

### 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.
  • Loading branch information
andrewhop authored Feb 6, 2025
1 parent 4e40ef6 commit 2c6ff65
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 1 deletion.
2 changes: 1 addition & 1 deletion crypto/curve25519_extra/curve25519_extra.c
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ int ED25519ph_sign_digest(uint8_t out_sig[ED25519_SIGNATURE_LEN],
const uint8_t *context, size_t context_len) {
FIPS_service_indicator_lock_state();
boringssl_ensure_hasheddsa_self_test();
FIPS_service_indicator_unlock_state();
int res = ED25519ph_sign_digest_no_self_test(out_sig, digest, private_key,
context, context_len);
FIPS_service_indicator_unlock_state();
if (res) {
FIPS_service_indicator_update_state();
}
Expand Down
4 changes: 4 additions & 0 deletions crypto/fipsmodule/curve25519/curve25519.c
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,9 @@ static void ed25519_keypair_pct(uint8_t public_key[ED25519_PUBLIC_KEY_LEN],

void ED25519_keypair(uint8_t out_public_key[ED25519_PUBLIC_KEY_LEN],
uint8_t out_private_key[ED25519_PRIVATE_KEY_LEN]) {
// We have to avoid the self tests and digest function in ed25519_keypair_pct
// from updating the service indicator.
FIPS_service_indicator_lock_state();
boringssl_ensure_eddsa_self_test();
SET_DIT_AUTO_RESET;

Expand All @@ -141,6 +144,7 @@ void ED25519_keypair(uint8_t out_public_key[ED25519_PUBLIC_KEY_LEN],

ed25519_keypair_pct(out_public_key, out_private_key);

FIPS_service_indicator_unlock_state();
FIPS_service_indicator_update_state();
}

Expand Down
2 changes: 2 additions & 0 deletions crypto/fipsmodule/rsa/rsa_impl.c
Original file line number Diff line number Diff line change
Expand Up @@ -1278,9 +1278,11 @@ int RSA_generate_key_fips(RSA *rsa, int bits, BN_GENCB *cb) {
}

BIGNUM *e = BN_new();
FIPS_service_indicator_lock_state();
int ret = e != NULL &&
BN_set_word(e, RSA_F4) &&
RSA_generate_key_ex_maybe_fips(rsa, bits, e, cb, /*check_fips=*/1);
FIPS_service_indicator_unlock_state();
BN_free(e);
if(ret) {
// Approved key size check step is already done at start of function.
Expand Down
6 changes: 6 additions & 0 deletions include/openssl/service_indicator.h
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,19 @@ enum FIPSStatus {
// |AWSLC_NOT_APPROVED| accordingly to the approved state of the service ran.
// It is highly recommended that users of the service indicator use this macro
// when interacting with the service indicator.
//
// This macro tests before != after to handle potential uint64_t rollover in
// long-running applications that use the release build of AWS-LC. Debug builds
// use an assert before + 1 == after to ensure in testing the service indicator
// is operating as expected.
#define CALL_SERVICE_AND_CHECK_APPROVED(approved, func) \
do { \
(approved) = AWSLC_NOT_APPROVED; \
int before = FIPS_service_indicator_before_call(); \
func; \
int after = FIPS_service_indicator_after_call(); \
if (before != after) { \
assert(before + 1 == after); \
(approved) = AWSLC_APPROVED; \
} \
} \
Expand Down

0 comments on commit 2c6ff65

Please sign in to comment.