Skip to content

Un-nest Ed25519 cryptocb cases from HAVE_CURVE25519 #493

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
yosuke-wolfssl wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Un-nest Ed25519 cryptocb cases from HAVE_CURVE25519 #493

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: 4 additions & 0 deletions .github/workflows/build-and-test-refactor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,10 @@ jobs:
- name: Build and test refactor DMA ASAN LMS verify-only XMSS full
run: cd test-refactor/posix && make clean && make -j DMA=1 ASAN=1 LMS_VERIFY_ONLY=1 WOLFSSL_DIR=../../wolfssl && make run

# Build and test with Ed25519 enabled but Curve25519 disabled
- name: Build and test refactor DMA ASAN NOCURVE25519
run: cd test-refactor/posix && make clean && make -j DMA=1 ASAN=1 NOCURVE25519=1 WOLFSSL_DIR=../../wolfssl && make run

# Build and test ASAN build, with wolfCrypt tests enabled.
- name: Build and test refactor ASAN TESTWOLFCRYPT
run: cd test-refactor/posix && make clean && make -j ASAN=1 TESTWOLFCRYPT=1 WOLFSSL_DIR=../../wolfssl && make run
Expand Down
2 changes: 1 addition & 1 deletion src/wh_client_cryptocb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,7 @@ int wh_Client_CryptoCbStd(int devId, wc_CryptoInfo* info, void* inCtx)
*out_len = len;
}
} break;
#endif /* HAVE_CURVE25519 */

#ifdef HAVE_ED25519
case WC_PK_TYPE_ED25519_KEYGEN: {
Expand Down Expand Up @@ -488,7 +489,6 @@ int wh_Client_CryptoCbStd(int devId, wc_CryptoInfo* info, void* inCtx)
}
} break;
#endif /* HAVE_ED25519 */
#endif /* HAVE_CURVE25519 */

#if defined(WOLFSSL_HAVE_MLKEM)
case WC_PK_TYPE_PQC_KEM_KEYGEN:
Expand Down
89 changes: 89 additions & 0 deletions test-refactor/client-server/wh_test_crypto_ed25519.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,94 @@ static int _whTest_CryptoEd25519ServerKey(whClientContext* ctx)
return ret;
}

/* Signs and verifies through the wolfCrypt API on a key whose private material
* lives only in the server, so the crypto callback must handle the operation.
* A software fallback has no private scalar and returns BAD_FUNC_ARG. */
static int _whTest_CryptoEd25519CryptoCbHsmKey(whClientContext* ctx)
{
int devId = WH_CLIENT_DEVID(ctx);
int ret = 0;
WC_RNG rng[1];
ed25519_key key[1] = {0};
ed25519_key pubKey[1] = {0};
whKeyId signKeyId = WH_KEYID_ERASED;
whKeyId verifyKeyId = WH_KEYID_ERASED;
byte msg[] = "Ed25519 cryptocb dispatch message";
byte sig[ED25519_SIG_SIZE];
word32 sigSz = sizeof(sig);
int verified = 0;
uint8_t label[] = "Ed25519 CryptoCb Key";

ret = wc_InitRng_ex(rng, NULL, devId);
if (ret != 0) {
WH_ERROR_PRINT("Failed to wc_InitRng_ex %d\n", ret);
return ret;
}

ret = wc_ed25519_init_ex(key, NULL, devId);
if (ret != 0) {
WH_ERROR_PRINT("Failed to initialize Ed25519 key: %d\n", ret);
(void)wc_FreeRng(rng);
return ret;
}

ret = wc_ed25519_init_ex(pubKey, NULL, devId);
if (ret != 0) {
WH_ERROR_PRINT("Failed to initialize Ed25519 public key: %d\n", ret);
wc_ed25519_free(key);
(void)wc_FreeRng(rng);
return ret;
}

ret = wc_ed25519_make_key(rng, ED25519_KEY_SIZE, key);

@Frauschi Frauschi Jul 29, 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] WC_PK_TYPE_ED25519_KEYGEN dispatch remains uncovered by the new regression test
💡 SUGGEST

The PR un-nests three dispatch cases — WC_PK_TYPE_ED25519_KEYGEN, _SIGN and _VERIFY — but the new test only pins two of them. The sign case is covered because the key is reduced to a bare keyId (software fallback hits if (!key->privKeySet) return BAD_FUNC_ARG; in wolfSSL ed25519.c:559), and verify likewise (pubKeySet == 0 after re-init). The keygen case is not: wc_ed25519_make_key(rng, ED25519_KEY_SIZE, key) at line 403 runs against a freshly initialised key with local material available, so when WC_PK_TYPE_ED25519_KEYGEN is compiled out the callback returns CRYPTOCB_UNAVAILABLE, wolfCrypt generates the key in software, and every subsequent step of the test still succeeds. This is exactly the failure mode described in the PR body — silent software fallback with no diagnostic — surviving for one of the three cases the fix touches.

Recommendation: Add a keygen assertion that can only pass through the callback — e.g. generate into a key whose devCtx already carries a keyId via wh_Client_Ed25519MakeCacheKey/wh_Client_Ed25519MakeExportKey semantics, or assert the resulting key is a server handle rather than one holding local private material — so a re-nested WC_PK_TYPE_ED25519_KEYGEN guard is caught too.

if (ret != 0) {
WH_ERROR_PRINT("Failed to generate Ed25519 key: %d\n", ret);
}
else {
ret = whTest_Ed25519ImportToServer(ctx, devId, key, pubKey, label,
sizeof(label), &signKeyId,
&verifyKeyId);
}

if (ret == 0) {

@Frauschi Frauschi Jul 29, 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] New regression test's dispatch path is ambient rather than pinned, and the only CI row that runs it is DMA=1
💡 SUGGEST test

The defect fixed in this PR lives in wh_Client_CryptoCbStd (src/wh_client_cryptocb.c:426-491). wh_Client_CryptoCb routes to wh_Client_CryptoCbDma first whenever ctx->dma.preferDma is set (src/wh_client_cryptocb.c:103-118), so which callback the new test exercises depends on ambient client state. The new test never pins that state — unlike sibling suites which explicitly call wh_Client_SetDmaMode() or loop WH_TEST_DMA_MODE_CNT (e.g. wh_test_crypto_aes.c:1112, wh_test_crypto_sha.c:2451). By code inspection it happens to work today: test-refactor/posix/wh_test_posix_client.c passes no dmaConfig, and wh_Client_Init only sets preferDma when config->dmaConfig != NULL (src/wh_client.c:150-157), so preferDma == 0 and the Std callback is hit even in the DMA=1 CI row. (I could not build/run to confirm — the review sandbox has no access to the wolfSSL checkout outside the worktree.) That is fragile in two ways: if the harness default ever flips to preferDma=1, or if an earlier suite leaves DMA mode on after an early return, the guard regression stops being detected with no visible signal. The Ed25519 group in wh_Client_CryptoCbDma (src/wh_client_cryptocb.c:1539-1596) is also never reached through wc_ed25519_sign_msg() by this test.

Suggestion:

Suggested change
if (ret == 0) {
/* Pin the std cryptocb dispatch: this test exists to prove the
* WC_PK_TYPE_ED25519_* cases in wh_Client_CryptoCbStd are compiled in. */
(void)wh_Client_SetDmaMode(ctx, 0);
if (ret == 0) {
sigSz = sizeof(sig);
ret = wc_ed25519_sign_msg(msg, (word32)sizeof(msg), sig, &sigSz, key);

sigSz = sizeof(sig);
ret = wc_ed25519_sign_msg(msg, (word32)sizeof(msg), sig, &sigSz, key);
if (ret != 0) {
WH_ERROR_PRINT("wc_ed25519_sign_msg on server key failed: %d\n",
ret);
}
}

if (ret == 0) {
ret = wc_ed25519_verify_msg(sig, sigSz, msg, (word32)sizeof(msg),
&verified, pubKey);
if (ret != 0) {
WH_ERROR_PRINT("wc_ed25519_verify_msg on server key failed: %d\n",
ret);
}
else if (verified != 1) {
WH_ERROR_PRINT("Server key Ed25519 signature did not verify\n");
ret = -1;
}
}

if (!WH_KEYID_ISERASED(signKeyId)) {
(void)wh_Client_KeyEvict(ctx, signKeyId);
}
if (!WH_KEYID_ISERASED(verifyKeyId)) {
(void)wh_Client_KeyEvict(ctx, verifyKeyId);
}

if (ret == 0) {
WH_TEST_PRINT("Ed25519 CRYPTOCB DEVID=0x%X SUCCESS\n", devId);
}

wc_ed25519_free(pubKey);
wc_ed25519_free(key);
(void)wc_FreeRng(rng);
return ret;
}

#ifdef WOLFHSM_CFG_DMA
static int _whTest_CryptoEd25519Dma(whClientContext* ctx)
{
Expand Down Expand Up @@ -731,6 +819,7 @@ int whTest_Crypto_Ed25519(whClientContext* ctx)
{
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoEd25519Inline(ctx));
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoEd25519ServerKey(ctx));
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoEd25519CryptoCbHsmKey(ctx));
#ifdef WOLFHSM_CFG_DMA
WH_TEST_RETURN_ON_FAIL(_whTest_CryptoEd25519Dma(ctx));
#endif
Expand Down
5 changes: 5 additions & 0 deletions test-refactor/posix/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,11 @@ ifeq ($(DMA),1)
DEF += -DWOLFHSM_CFG_DMA
endif

# Build without Curve25519, leaving Ed25519 enabled
ifeq ($(NOCURVE25519),1)
DEF += -DWOLFHSM_CFG_TEST_NO_CURVE25519
endif

# Build LMS/XMSS in verify-only mode (omits private-key, sign, and keygen
# paths). May be combined to exercise the mixed (one verify-only) case.
ifeq ($(LMS_VERIFY_ONLY),1)
Expand Down
5 changes: 5 additions & 0 deletions test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,11 @@ ifeq ($(NOCRYPTO),1)
DEF += -DWOLFHSM_CFG_NO_CRYPTO
endif

# Build without Curve25519, leaving Ed25519 enabled
ifeq ($(NOCURVE25519),1)
DEF += -DWOLFHSM_CFG_TEST_NO_CURVE25519
endif

# Enable scan-build
ifeq ($(SCAN),1)
SCAN_LOG = scan_test.log
Expand Down
4 changes: 4 additions & 0 deletions test/config/user_settings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,11 @@
#define ECC_SHAMIR

/** Curve25519 Options */
/* Curve25519 and Ed25519 are independent options. Allow a build that omits
* Curve25519 so the Ed25519-only configuration stays exercised. */
#ifndef WOLFHSM_CFG_TEST_NO_CURVE25519
#define HAVE_CURVE25519
#endif

/** DH and DHE Options */
#define NO_DH
Expand Down
Loading