Skip to content

fix wp_ecx_get_security_bits to return the correct security bits for x448 and x25519 #417

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

Open
gasbytes wants to merge 1 commit into
base: master
Choose a base branch
from
+85 −2
Open

fix wp_ecx_get_security_bits to return the correct security bits for x448 and x25519 #417

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
4 changes: 2 additions & 2 deletions src/wp_ecx_kmgmt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,10 +511,10 @@ static int wp_ecx_get_security_bits(wp_Ecx* ecx)
{
int bits = 0;

if (ecx->data->bits >= 456) {
if (ecx->data->bits >= 448) {

@ColtonWilley ColtonWilley Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 [Low] Magic-number thresholds in wp_ecx_get_security_bits are undocumented

The thresholds >= 448 and >= 255 are derived from the data->bits values assigned to each curve (x25519=255, x448=448, ed25519=256, ed448=456), but that relationship is non-obvious to a reader. The function infers a security level from a key-size threshold even though data->keyType (WP_KEY_TYPE_X25519, etc.) is available and would express intent more directly. The pre-existing code already used magic numbers, so this matches existing style, but a short comment mapping the thresholds to the curve families would help future maintainers and prevent regressions like the off-by-a-few-bits values this PR is fixing.

Fix: Add a brief comment explaining how the 255/448 thresholds map to the X/Ed curve families, or switch on data->keyType for clarity. Non-blocking.

bits = 224;
}
else if (ecx->data->bits >= 256) {
else if (ecx->data->bits >= 255) {
bits = 128;
}

Expand Down
76 changes: 76 additions & 0 deletions test/test_ecx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -897,3 +897,79 @@ int test_ecx_dup(void *data)

#endif /* defined(WP_HAVE_ED25519) || defined(WP_HAVE_ECD444) */

#if defined(WP_HAVE_X25519) || defined(WP_HAVE_X448)

/*
* Check that the correct security bits are provided for x25519 and x448
*/
int test_ecx_x_security_bits(void *data)
{
int err = 0;
(void)data;

EVP_PKEY *pkey_ossl = NULL;
EVP_PKEY *pkey_wolf = NULL;
EVP_PKEY_CTX *ctx_ossl = NULL;
EVP_PKEY_CTX *ctx_wolf = NULL;

struct {
const char *name;
} types[] = {
#ifdef WP_HAVE_X25519
{ "X25519" },
#endif
#ifdef WP_HAVE_X448
{ "X448" },
#endif
};

for (unsigned i = 0; i < ARRAY_SIZE(types) && err == 0; i++) {

@ColtonWilley ColtonWilley Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔴 [High] New test uses ARRAY_SIZE outside its defining feature guard — compile break in curve-only builds

The new test test_ecx_x_security_bits is wrapped in #if defined(WP_HAVE_X25519) || defined(WP_HAVE_X448) (line 900) and calls ARRAY_SIZE(types) at line 926. However, ARRAY_SIZE is #defined only inside the #if defined(WP_HAVE_ED25519) || defined(WP_HAVE_ECD448) block at lines 29-33, which closes at line 898 — before the new test begins. Since a #define is only processed when its enclosing #if is true, ARRAY_SIZE is available only when WP_HAVE_ED25519 is defined (WP_HAVE_ECD448 is a typo that is never defined; the macro is WP_HAVE_ED448). In include/wolfprovider/settings.h (lines 174-184), WP_HAVE_X25519/WP_HAVE_X448 (from HAVE_CURVE25519/HAVE_CURVE448) are gated independently of WP_HAVE_ED25519/WP_HAVE_ED448. So a build with curve25519/curve448 enabled but ed25519/ed448 disabled compiles this test with ARRAY_SIZE undefined, producing a compile error ('ARRAY_SIZE' undeclared). ARRAY_SIZE is not provided by any included wolfSSL header. Default builds that enable Ed25519 compile fine, which is why this is easy to miss.

Fix: Hoist the ARRAY_SIZE definition above the #if defined(WP_HAVE_ED25519) ... guard (or add a local #ifndef ARRAY_SIZE inside the new block) so the X25519/X448-only build configuration compiles. Verify with a curve-only build (e.g. wolfSSL --enable-curve25519 --disable-ed25519 --disable-ed448).

if (err == 0) {
ctx_ossl = EVP_PKEY_CTX_new_from_name(osslLibCtx, types[i].name,
NULL);
err = ctx_ossl == NULL;
}
if (err == 0) {
ctx_wolf = EVP_PKEY_CTX_new_from_name(wpLibCtx, types[i].name,
NULL);
err = ctx_wolf == NULL;
}
if (err == 0) {
err = EVP_PKEY_keygen_init(ctx_ossl) != 1;
}
if (err == 0) {
err = EVP_PKEY_keygen_init(ctx_wolf) != 1;
}
if (err == 0) {
pkey_ossl = NULL;
err = EVP_PKEY_generate(ctx_ossl, &pkey_ossl) != 1;
}
if (err == 0) {
pkey_wolf = NULL;
err = EVP_PKEY_generate(ctx_wolf, &pkey_wolf) != 1;
}
if (err == 0) {

@ColtonWilley ColtonWilley Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟠 [Medium] Security-bits test only checks parity with OpenSSL, not the expected absolute values

The test validates correctness only by comparing wolfProvider's EVP_PKEY_get_security_bits against OpenSSL's (sec_ossl != sec_wolf). The PR's stated intent is that X25519 returns 128 and X448 returns 224. The differential check transitively verifies this only because OpenSSL happens to return those values; it would not catch a case where both sides are wrong, and it pins the test to OpenSSL's behavior rather than the documented expectation. This matches the differential-testing convention used elsewhere in this file, so it is acceptable, but asserting the absolute expected value per curve would make the test directly exercise the threshold logic that was changed in wp_ecx_get_security_bits.

Fix: Optionally assert the absolute expected security bits (128 for X25519, 224 for X448) in addition to the OpenSSL parity check, so the test directly pins the threshold logic this PR changed.

int sec_ossl = EVP_PKEY_get_security_bits(pkey_ossl);
int sec_wolf = EVP_PKEY_get_security_bits(pkey_wolf);
if (sec_ossl != sec_wolf) {
PRINT_MSG("EVP_PKEY_get_security_bits failed for %s:"
" expected %d, got %d", types[i].name, sec_ossl, sec_wolf);
err = 1;
}
}

EVP_PKEY_free(pkey_ossl);
EVP_PKEY_free(pkey_wolf);
EVP_PKEY_CTX_free(ctx_ossl);
EVP_PKEY_CTX_free(ctx_wolf);
pkey_ossl = NULL;
pkey_wolf = NULL;
ctx_ossl = NULL;
ctx_wolf = NULL;
}

return err;
}

#endif /* defined(WP_HAVE_X25519) || defined(WP_HAVE_X448) */

3 changes: 3 additions & 0 deletions test/unit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,9 @@ TEST_CASE test_case[] = {
#endif
TEST_DECL(test_ecx_dup, NULL),
#endif
#if defined(WP_HAVE_X25519) || defined(WP_HAVE_X448)
TEST_DECL(test_ecx_x_security_bits, NULL),
#endif

TEST_DECL(test_pkcs7_x509_sign_verify, NULL),
TEST_DECL(test_x509_cert, NULL),
Expand Down
4 changes: 4 additions & 0 deletions test/unit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,10 @@ int test_ecx_x25519_raw_priv_roundtrip(void *data);
int test_ecx_dup(void *data);
#endif

#if defined(WP_HAVE_X25519) || defined(WP_HAVE_X448)
int test_ecx_x_security_bits(void *data);
#endif

int test_pkcs7_x509_sign_verify(void *data);
int test_x509_cert(void *data);

Expand Down
Loading