Skip to content

Validate peer DH public value before key agreement #1077

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: master
Choose a base branch
from
Open

Validate peer DH public value before key agreement #1077

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
31 changes: 23 additions & 8 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5675,17 +5675,27 @@ static int KeyAgreeDh_client(WOLFSSH* ssh, byte hashId,
WLOG(WS_LOG_DEBUG, "Entering KeyAgreeDh_client()");
WOLFSSH_UNUSED(hashId);

PRIVATE_KEY_UNLOCK();
ret = wc_DhAgree(&ssh->handshake->privKey.dh,
ssh->k, &ssh->kSz,
ssh->handshake->x, ssh->handshake->xSz,
f, fSz);
PRIVATE_KEY_LOCK();
/* Reject a peer public value outside the safe range [2, p-2] before key
* agreement. Keeps wolfSSH independent of whether the linked wolfSSL
* validates the peer key inside wc_DhAgree. */
ret = wc_DhCheckPubKey(&ssh->handshake->privKey.dh, f, fSz);
if (ret != 0) {
WLOG(WS_LOG_ERROR,
"Generate DH shared secret failed, %d", ret);
WLOG(WS_LOG_ERROR, "Peer DH public value rejected, %d", ret);
ret = WS_CRYPTO_FAILED;
}
if (ret == 0) {
PRIVATE_KEY_UNLOCK();
ret = wc_DhAgree(&ssh->handshake->privKey.dh,
ssh->k, &ssh->kSz,
ssh->handshake->x, ssh->handshake->xSz,
f, fSz);
PRIVATE_KEY_LOCK();
if (ret != 0) {
WLOG(WS_LOG_ERROR,
"Generate DH shared secret failed, %d", ret);
ret = WS_CRYPTO_FAILED;
}
}
ForceZero(ssh->handshake->x, ssh->handshake->xSz);
wc_FreeDhKey(&ssh->handshake->privKey.dh);

Expand Down Expand Up @@ -12459,6 +12469,11 @@ static int KeyAgreeDh_server(WOLFSSH* ssh, byte hashId, byte* f, word32* fSz)
if (ret == 0)
ret = wc_DhGenerateKeyPair(privKey, ssh->rng,
y_ptr, &ySz, f, fSz);
/* Reject a peer public value outside the safe range [2, p-2] before
* key agreement, independent of the linked wolfSSL's own check. */
if (ret == 0)
ret = wc_DhCheckPubKey(privKey, ssh->handshake->e,
ssh->handshake->eSz);
if (ret == 0) {
PRIVATE_KEY_UNLOCK();
ret = wc_DhAgree(privKey, ssh->k, &ssh->kSz, y_ptr, ySz,
Expand Down
141 changes: 138 additions & 3 deletions tests/unit.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3674,8 +3674,8 @@ static int test_KeyAgreeDh_client_zeroesEphemeralPrivKey(void)
WMEMSET(hs->x, 0xA5, markedSz);
hs->xSz = markedSz;

/* Pass a garbage f to force wc_DhAgree to fail (the DH context has no
* prime group set). The ForceZero on x runs regardless of the result. */
/* No prime group is set, so wc_DhCheckPubKey fails before wc_DhAgree is
* reached. The ForceZero on x is unconditional and runs regardless. */
WMEMSET(bogusF, 0xCC, sizeof(bogusF));
(void)wolfSSH_TestKeyAgreeDh_client(ssh, WC_HASH_TYPE_SHA256,
bogusF, (word32)sizeof(bogusF));
Expand Down Expand Up @@ -3802,7 +3802,12 @@ static void DrainCaptured(void)
* either 0x00 or 0xCC AND that contains at least one 0x00. The DH private
* key emitted by wc_DhGenerateKeyPair is overwhelmingly unlikely to be
* entirely composed of 0x00 / 0xCC bytes, so this catches the mutation
* while staying deterministic regardless of underlying malloc state. */
* while staying deterministic regardless of underlying malloc state.
*
* The peer value ssh->handshake->e is left zero, so the new
* wc_DhCheckPubKey now fails between wc_DhGenerateKeyPair and wc_DhAgree;
* the ForceZero on y_ptr is unconditional and still runs, so the buffer
* assertion above is unaffected. */
static int test_KeyAgreeDh_server_zeroesEphemeralPrivKey(void)
{
WOLFSSH_CTX* ctx = NULL;
Expand Down Expand Up @@ -3887,6 +3892,124 @@ static int test_KeyAgreeDh_server_zeroesEphemeralPrivKey(void)
return result;
}
#endif /* WOLFSSH_SMALL_STACK && WOLFSSH_TEST_CAPTURING_ALLOCATOR */

/* Verify KeyAgreeDh_server rejects an out-of-range peer public value.
* The server hook loads the SSH prime group from kexId itself, so feeding
* ssh->handshake->e a value of 0 or 1 (outside [2, p-2]) must make
* wc_DhCheckPubKey fail and the call return non-WS_SUCCESS instead of
* deriving a known shared secret. On a wolfSSL whose wc_DhAgree already
* validates the peer key this is an equivalent guard; its value is
* defense-in-depth for builds whose wc_DhAgree does not validate. */
static int test_KeyAgreeDh_server_rejectsOutOfRangePeer(void)
{
WOLFSSH_CTX* ctx = NULL;
WOLFSSH* ssh = NULL;
HandshakeInfo* hs = NULL;
byte f[MAX_KEX_KEY_SZ];
word32 fSz;
word32 c;
int result = 0;
static const byte badVals[2] = { 0x00, 0x01 };

ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL);
if (ctx == NULL)
return -740;
ssh = wolfSSH_new(ctx);
if (ssh == NULL) {
wolfSSH_CTX_free(ctx);
return -741;
}
hs = ssh->handshake;
if (hs == NULL) {
result = -742;
goto cleanup;
}
#ifndef WOLFSSH_NO_DH_GROUP14_SHA256
hs->kexId = ID_DH_GROUP14_SHA256;
#elif !defined(WOLFSSH_NO_DH_GROUP14_SHA1)
hs->kexId = ID_DH_GROUP14_SHA1;
#else
hs->kexId = ID_DH_GROUP1_SHA1;
#endif

for (c = 0; c < (word32)sizeof(badVals); c++) {
int ret;
hs->e[0] = badVals[c];
hs->eSz = 1;
fSz = (word32)sizeof(f);
ret = wolfSSH_TestKeyAgreeDh_server(ssh, WC_HASH_TYPE_SHA256, f, &fSz);
if (ret == WS_SUCCESS) {
result = -743;
break;
}
}

cleanup:
wolfSSH_free(ssh);
wolfSSH_CTX_free(ctx);
return result;
}

#ifdef HAVE_FFDHE_2048
/* Verify KeyAgreeDh_client rejects an out-of-range peer public value.
* A real prime group is loaded into ssh->handshake->privKey.dh so that an
* f of 0 or 1 (outside [2, p-2]) is caught by wc_DhCheckPubKey before key
* agreement, yielding WS_CRYPTO_FAILED rather than a known shared secret.
* Equivalent-mutant caveat as in the server case above: it is a regression
* guard for builds whose wc_DhAgree does not validate the peer key. */
static int test_KeyAgreeDh_client_rejectsOutOfRangePeer(void)
{
WOLFSSH_CTX* ctx = NULL;
WOLFSSH* ssh = NULL;
HandshakeInfo* hs = NULL;
byte badF[1];
word32 c;
int result = 0;
static const byte badVals[2] = { 0x00, 0x01 };

ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
if (ctx == NULL)
return -750;
ssh = wolfSSH_new(ctx);
if (ssh == NULL) {
wolfSSH_CTX_free(ctx);
return -751;
}
hs = ssh->handshake;
if (hs == NULL) {
result = -752;
goto cleanup;
}

for (c = 0; c < (word32)sizeof(badVals); c++) {
int ret;
/* The hook frees privKey.dh on return, so re-init each iteration. */
if (wc_InitDhKey(&hs->privKey.dh) != 0) {
result = -753;
break;
}
if (wc_DhSetNamedKey(&hs->privKey.dh, WC_FFDHE_2048) != 0) {
wc_FreeDhKey(&hs->privKey.dh);
result = -754;
break;
}
/* x is unused on the reject path but ForceZero still runs over it. */
hs->xSz = 0;
badF[0] = badVals[c];
ret = wolfSSH_TestKeyAgreeDh_client(ssh, WC_HASH_TYPE_SHA256,
badF, (word32)sizeof(badF));
if (ret != WS_CRYPTO_FAILED) {
result = -755;
break;
}
}

cleanup:
wolfSSH_free(ssh);
wolfSSH_CTX_free(ctx);
return result;
}
#endif /* HAVE_FFDHE_2048 */
#endif /* !WOLFSSH_NO_DH */

#if defined(WOLFSSH_SCP) && !defined(WOLFSSH_SCP_USER_CALLBACKS) && \
Expand Down Expand Up @@ -5741,6 +5864,18 @@ int wolfSSH_UnitTest(int argc, char** argv)
(unitResult == 0 ? "SUCCESS" : "FAILED"));
testResult = testResult || unitResult;
#endif

unitResult = test_KeyAgreeDh_server_rejectsOutOfRangePeer();
printf("KeyAgreeDh_server_rejectsOutOfRangePeer: %s\n",
(unitResult == 0 ? "SUCCESS" : "FAILED"));
testResult = testResult || unitResult;

#ifdef HAVE_FFDHE_2048
unitResult = test_KeyAgreeDh_client_rejectsOutOfRangePeer();
printf("KeyAgreeDh_client_rejectsOutOfRangePeer: %s\n",
(unitResult == 0 ? "SUCCESS" : "FAILED"));
testResult = testResult || unitResult;
#endif
#endif /* !WOLFSSH_NO_DH */

#endif
Expand Down
Loading