From 58eaa29acd3f43b335b7d87da12228e321cc296d Mon Sep 17 00:00:00 2001 From: Kazuho Oku Date: Wed, 4 Sep 2024 17:01:07 +0900 Subject: [PATCH] upon failure to generate X25519 secret, do not return address of freed memory --- include/picotls.h | 2 ++ lib/cifra/x25519.c | 1 + lib/hpke.c | 9 +++++++-- lib/openssl.c | 12 ++++++++---- lib/picotls.c | 9 +++++++-- 5 files changed, 25 insertions(+), 8 deletions(-) diff --git a/include/picotls.h b/include/picotls.h index dfaf9d468..3226442e2 100644 --- a/include/picotls.h +++ b/include/picotls.h @@ -348,6 +348,7 @@ typedef struct st_ptls_key_exchange_context_t { * When `secret` is non-NULL, this callback derives the shared secret using the private key of the context and the peer key being * given, and sets the value in `secret`. The memory pointed to by `secret->base` must be freed by the caller by calling `free`. * When `release` is set, the callee frees resources allocated to the context and set *keyex to NULL. + * Upon failure (i.e., when an PTLS error code is returned), `*pubkey` and `*secret` either remain unchanged or are zero-cleared. */ int (*on_exchange)(struct st_ptls_key_exchange_context_t **keyex, int release, ptls_iovec_t *secret, ptls_iovec_t peerkey); } ptls_key_exchange_context_t; @@ -369,6 +370,7 @@ typedef const struct st_ptls_key_exchange_algorithm_t { * Implements synchronous key exchange. Called when ServerHello is generated. * Given a public key provided by the peer (`peerkey`), this callback generates an ephemeral private and public key, and returns * the public key (`pubkey`) and a secret (`secret`) derived from the peerkey and private key. + * Upon failure (i.e., when an PTLS error code is returned), `*pubkey` and `*secret` either remain unchanged or are zero-cleared. */ int (*exchange)(const struct st_ptls_key_exchange_algorithm_t *algo, ptls_iovec_t *pubkey, ptls_iovec_t *secret, ptls_iovec_t peerkey); diff --git a/lib/cifra/x25519.c b/lib/cifra/x25519.c index 3697d4ddf..a677d761e 100644 --- a/lib/cifra/x25519.c +++ b/lib/cifra/x25519.c @@ -49,6 +49,7 @@ static int x25519_derive_secret(ptls_iovec_t *secret, const uint8_t *clientpriv, static const uint8_t zeros[X25519_KEY_SIZE] = {0}; if (ptls_mem_equal(secret->base, zeros, sizeof(zeros))) { free(secret->base); + secret->base = NULL; return PTLS_ERROR_INCOMPATIBLE_KEY; } diff --git a/lib/hpke.c b/lib/hpke.c index d243a5393..b81f19ce2 100644 --- a/lib/hpke.c +++ b/lib/hpke.c @@ -129,8 +129,11 @@ static int dh_encap(ptls_hpke_kem_t *kem, void *secret, ptls_iovec_t *pk_s, ptls *pk_s = ptls_iovec_init(NULL, 0); - if ((ret = kem->keyex->exchange(kem->keyex, pk_s, &dh, pk_r)) != 0) + if ((ret = kem->keyex->exchange(kem->keyex, pk_s, &dh, pk_r)) != 0) { + assert(pk_s->base == NULL); + assert(dh.base == NULL); goto Exit; + } if ((ret = dh_derive(kem, secret, *pk_s, pk_r, dh)) != 0) goto Exit; @@ -152,8 +155,10 @@ static int dh_decap(ptls_hpke_kem_t *kem, void *secret, ptls_key_exchange_contex ptls_iovec_t dh = {NULL}; int ret; - if ((ret = keyex->on_exchange(&keyex, 0, &dh, pk_s)) != 0) + if ((ret = keyex->on_exchange(&keyex, 0, &dh, pk_s)) != 0) { + assert(dh.base == NULL); goto Exit; + } if ((ret = dh_derive(kem, secret, pk_s, pk_r, dh)) != 0) goto Exit; diff --git a/lib/openssl.c b/lib/openssl.c index 65b2feadc..2833c32b6 100644 --- a/lib/openssl.c +++ b/lib/openssl.c @@ -516,7 +516,7 @@ static int evp_keyex_on_exchange(ptls_key_exchange_context_t **_ctx, int release goto Exit; } - secret->base = NULL; + *secret = ptls_iovec_init(NULL, 0); if (peerkey.len != ctx->super.pubkey.len) { ret = PTLS_ALERT_DECRYPT_ERROR; @@ -598,8 +598,10 @@ static int evp_keyex_on_exchange(ptls_key_exchange_context_t **_ctx, int release EVP_PKEY_CTX_free(evpctx); if (evppeer != NULL) EVP_PKEY_free(evppeer); - if (ret != 0) + if (ret != 0) { free(secret->base); + *secret = ptls_iovec_init(NULL, 0); + } if (release) { evp_keyex_free(ctx); *_ctx = NULL; @@ -679,7 +681,7 @@ static int evp_keyex_exchange(ptls_key_exchange_algorithm_t *algo, ptls_iovec_t ptls_key_exchange_context_t *ctx = NULL; int ret; - outpubkey->base = NULL; + *outpubkey = ptls_iovec_init(NULL, 0); if ((ret = evp_keyex_create(algo, &ctx)) != 0) goto Exit; @@ -695,8 +697,10 @@ static int evp_keyex_exchange(ptls_key_exchange_algorithm_t *algo, ptls_iovec_t Exit: if (ctx != NULL) evp_keyex_on_exchange(&ctx, 1, NULL, ptls_iovec_init(NULL, 0)); - if (ret != 0) + if (ret != 0) { free(outpubkey->base); + *outpubkey = ptls_iovec_init(NULL, 0); + } return ret; } diff --git a/lib/picotls.c b/lib/picotls.c index 6c86295f4..cc9cc91c3 100644 --- a/lib/picotls.c +++ b/lib/picotls.c @@ -2848,8 +2848,10 @@ static int client_handle_hello(ptls_t *tls, ptls_message_emitter_t *emitter, ptl ptls__key_schedule_update_hash(tls->key_schedule, message.base, message.len, 0); if (sh.peerkey.base != NULL) { - if ((ret = tls->client.key_share_ctx->on_exchange(&tls->client.key_share_ctx, 1, &ecdh_secret, sh.peerkey)) != 0) + if ((ret = tls->client.key_share_ctx->on_exchange(&tls->client.key_share_ctx, 1, &ecdh_secret, sh.peerkey)) != 0) { + assert(ecdh_secret.base == NULL); goto Exit; + } } if ((ret = key_schedule_extract(tls->key_schedule, ecdh_secret)) != 0) @@ -4677,8 +4679,11 @@ static int server_handle_hello(ptls_t *tls, ptls_message_emitter_t *emitter, ptl ret = ch->key_shares.base != NULL ? PTLS_ALERT_HANDSHAKE_FAILURE : PTLS_ALERT_MISSING_EXTENSION; goto Exit; } - if ((ret = key_share.algorithm->exchange(key_share.algorithm, &pubkey, &ecdh_secret, key_share.peer_key)) != 0) + if ((ret = key_share.algorithm->exchange(key_share.algorithm, &pubkey, &ecdh_secret, key_share.peer_key)) != 0) { + assert(pubkey.base == NULL); + assert(ecdh_secret.base == NULL); goto Exit; + } tls->key_share = key_share.algorithm; }