Skip to content

Commit

Permalink
DH Fix
Browse files Browse the repository at this point in the history
These changes fix several fuzz testing reports. (ZD 11088 and ZD 11101)
1. In GetDhPublicKey(), the DH Pubkey is owned by the SSL session. It
   doesn't need to be in the check for weOwnDh before freeing. There
   could be a chance it leaks.
2. In GeneratePublicDh() and GeneratePrivateDh(), the size of the
   destination buffer should be stored at the location pointed to by the
   size pointer. Check that before writing into the destination buffer.
3. Ensure the size of the private and public key values are in the size
   value before generating or getting the DH keys.
  • Loading branch information
ejohnstown committed Oct 16, 2020
1 parent a595e3c commit 4364700
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 11 deletions.
18 changes: 11 additions & 7 deletions src/internal.c
Original file line number Diff line number Diff line change
Expand Up @@ -21164,11 +21164,12 @@ static int GetDhPublicKey(WOLFSSL* ssl, const byte* input, word32 size,
ssl->buffers.serverDH_G.buffer = NULL;
}

if (ssl->buffers.serverDH_Pub.buffer) {
XFREE(ssl->buffers.serverDH_Pub.buffer, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
ssl->buffers.serverDH_Pub.buffer = NULL;
}
}

if (ssl->buffers.serverDH_Pub.buffer) {
XFREE(ssl->buffers.serverDH_Pub.buffer, ssl->heap,
DYNAMIC_TYPE_PUBLIC_KEY);
ssl->buffers.serverDH_Pub.buffer = NULL;
}

/* p */
Expand Down Expand Up @@ -23219,10 +23220,9 @@ int SendClientKeyExchange(WOLFSSL* ssl)
args->output += OPAQUE16_LEN;
XMEMCPY(args->output, ssl->arrays->client_identity, esSz);
args->output += esSz;
args->length = args->encSz - esSz - OPAQUE16_LEN;
args->encSz = esSz + OPAQUE16_LEN;

args->length = 0;

ret = AllocKey(ssl, DYNAMIC_TYPE_DH,
(void**)&ssl->buffers.serverDH_Key);
if (ret != 0) {
Expand Down Expand Up @@ -25161,6 +25161,8 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
if (ssl->buffers.serverDH_Pub.buffer == NULL) {
ERROR_OUT(MEMORY_E, exit_sske);
}
ssl->buffers.serverDH_Pub.length =
ssl->buffers.serverDH_P.length + OPAQUE16_LEN;
}

if (ssl->buffers.serverDH_Priv.buffer == NULL) {
Expand All @@ -25171,6 +25173,8 @@ static int DoSessionTicket(WOLFSSL* ssl, const byte* input, word32* inOutIdx,
if (ssl->buffers.serverDH_Priv.buffer == NULL) {
ERROR_OUT(MEMORY_E, exit_sske);
}
ssl->buffers.serverDH_Priv.length =
ssl->buffers.serverDH_P.length + OPAQUE16_LEN;
}

ssl->options.dhKeySz =
Expand Down
16 changes: 14 additions & 2 deletions wolfcrypt/src/dh.c
Original file line number Diff line number Diff line change
Expand Up @@ -1209,7 +1209,11 @@ static int GeneratePrivateDh(DhKey* key, WC_RNG* rng, byte* priv,
#endif
}

ret = wc_RNG_GenerateBlock(rng, priv, sz);
if (sz > *privSz)
ret = WC_KEY_SIZE_E;

if (ret == 0)
ret = wc_RNG_GenerateBlock(rng, priv, sz);

if (ret == 0) {
priv[0] |= 0x0C;
Expand Down Expand Up @@ -1241,6 +1245,7 @@ static int GeneratePublicDh(DhKey* key, byte* priv, word32 privSz,
mp_int y[1];
#endif
#endif
word32 binSz;

#ifdef WOLFSSL_HAVE_SP_DH
#ifndef WOLFSSL_SP_NO_2048
Expand Down Expand Up @@ -1282,11 +1287,18 @@ static int GeneratePublicDh(DhKey* key, byte* priv, word32 privSz,
if (ret == 0 && mp_exptmod(&key->g, x, &key->p, y) != MP_OKAY)
ret = MP_EXPTMOD_E;

if (ret == 0) {
binSz = mp_unsigned_bin_size(y);
if (binSz > *pubSz) {
ret = WC_KEY_SIZE_E;
}
}

if (ret == 0 && mp_to_unsigned_bin(y, pub) != MP_OKAY)
ret = MP_TO_E;

if (ret == 0)
*pubSz = mp_unsigned_bin_size(y);
*pubSz = binSz;

mp_clear(y);
mp_clear(x);
Expand Down
14 changes: 12 additions & 2 deletions wolfcrypt/test/test.c
Original file line number Diff line number Diff line change
Expand Up @@ -14642,6 +14642,11 @@ static int dh_test_ffdhe(WC_RNG *rng, const DhParams* params)
ERROR_OUT(-7835, done);
#endif

pubSz = FFDHE_KEY_SIZE;
pubSz2 = FFDHE_KEY_SIZE;
privSz = FFDHE_KEY_SIZE;
privSz2 = FFDHE_KEY_SIZE;

XMEMSET(key, 0, sizeof *key);
XMEMSET(key2, 0, sizeof *key2);

Expand Down Expand Up @@ -14763,8 +14768,8 @@ static int dh_test(void)
byte *agree2 = (byte *)XMALLOC(DH_TEST_BUF_SIZE, HEAP_HINT, DYNAMIC_TYPE_TMP_BUFFER);

if ((tmp == NULL) || (priv == NULL) || (pub == NULL) ||
(priv2 == NULL) || (pub2 == NULL) || (agree == NULL) ||
(agree2 == NULL))
(priv2 == NULL) || (pub2 == NULL) || (agree == NULL) ||
(agree2 == NULL))
ERROR_OUT(-7960, done);
#else
DhKey key_buf, *key = &key_buf;
Expand Down Expand Up @@ -14810,6 +14815,11 @@ static int dh_test(void)
(void)tmp;
(void)bytes;

pubSz = DH_TEST_BUF_SIZE;
pubSz2 = DH_TEST_BUF_SIZE;
privSz = DH_TEST_BUF_SIZE;
privSz2 = DH_TEST_BUF_SIZE;

XMEMSET(&rng, 0, sizeof(rng));
/* Use API for coverage. */
ret = wc_InitDhKey(key);
Expand Down

0 comments on commit 4364700

Please sign in to comment.