Repository navigation
Memory leaks in oqs ecdh path #150
Description
I see two memory leaks present in the current code. To setup reproduction:
- clone oqs openssh repo
./oqs-scripts/clone_liboqs.sh./oqs-scripts/build_liboqs.sh./oqs-scripts/build_openssh.sh(requires openssl 3.x in the usual place)oqs-test/run_tests.sh(populates/regressrepo with useful things)
Modify sshd_config to listen on 22222. Start server:
$(pwd)/sshd -ddd -h $(pwd)/regress/host.ecdsa-sha2-nistp256 -f sshd_config
Leak 1
kex_ecdh_dec_key_group allocates new memory and saves a reference in *shared_secretp. The function is called twice:
- https://github.com/open-quantum-safe/openssh/blob/OQS-v8/kexoqsecdh.c#L254, and
- https://github.com/open-quantum-safe/openssh/blob/OQS-v8/kexoqsecdh.c#L329
sshbuf_putb does not free ecdh_shared_secret, even if fully consumed. Hence, memory leaks at function exit.
Trace with valgrind
valgrind --trace-children=yes --leak-check=full ./ssh -i regress/host.ecdsa-sha2-nistp256 localhost -p 22222 -oKexAlgorithms=ecdh-nistp256-kyber-512r3-sha256-d00@openquantumsafe.org
[...]
==639219== 328 (72 direct, 256 indirect) bytes in 1 blocks are definitely lost in loss record 885 of 983
==639219== at 0x4848464: calloc (vg_replace_malloc.c:1328)
==639219== by 0x142B50: sshbuf_new (sshbuf.c:73)
==639219== by 0x1B6968: kex_ecdh_dec_key_group.constprop.0 (kexoqsecdh.c:57)
==639219== by 0x1B6D53: kex_kem_generic_with_ec_dec (kexoqsecdh.c:329)
==639219== by 0x1B82FC: kex_kem_kyber_512_ecdh_nistp256_dec (kexoqsecdh.c:638)
==639219== by 0x1A1EDE: input_kex_gen_reply (kexgen.c:440)
==639219== by 0x1815A5: ssh_dispatch_run (dispatch.c:113)
==639219== by 0x181708: ssh_dispatch_run_fatal (dispatch.c:133)
==639219== by 0x136CA2: ssh_kex2 (sshconnect2.c:346)
==639219== by 0x131F7D: ssh_login (sshconnect.c:1565)
==639219== by 0x116C89: main (ssh.c:1680)
Leak 2
EC_KEY_new_by_curve_name returns a pointer to newly allocated memory, referenced by ecdh_client_key. On the error path, ownership of the memory referenced by ecdh_client_key is not transferred to the caller kex and therefore leaks when exiting the function.
Impose deliberate error to trace error path
diff --git a/kexoqsecdh.c b/kexoqsecdh.c
index 36eabce5..d3447052 100644
--- a/kexoqsecdh.c
+++ b/kexoqsecdh.c
@@ -138,7 +138,7 @@ static int kex_kem_generic_with_ec_keypair(OQS_KEM *kem, struct kex *kex)
r = SSH_ERR_ALLOC_FAIL;
goto out;
}
- if (EC_KEY_generate_key(ecdh_client_key) != 1) {
+ if (1 || EC_KEY_generate_key(ecdh_client_key) != 1) {
r = SSH_ERR_LIBCRYPTO_ERROR;
goto out;
}
Trace with valgrind
valgrind --trace-children=yes --leak-check=full ./ssh -i regress/host.ecdsa-sha2-nistp256 localhost -p 22222 -oKexAlgorithms=ecdh-nistp256-kyber-512r3-sha256-d00@openquantumsafe.org
[...]
==639585== 1,316 (112 direct, 1,204 indirect) bytes in 1 blocks are definitely lost in loss record 856 of 893
==639585== at 0x484386F: malloc (vg_replace_malloc.c:381)
==639585== by 0x4A1116D: CRYPTO_zalloc (in /usr/lib64/libcrypto.so.3.0.8)
==639585== by 0x499DD1D: ??? (in /usr/lib64/libcrypto.so.3.0.8)
==639585== by 0x499F27C: EC_KEY_new_by_curve_name_ex (in /usr/lib64/libcrypto.so.3.0.8)
==639585== by 0x1B6866: kex_kem_generic_with_ec_keypair (kexoqsecdh.c:137)
==639585== by 0x1B809C: kex_kem_kyber_512_ecdh_nistp256_keypair (kexoqsecdh.c:611)
==639585== by 0x1A24E7: kex_gen_client (kexgen.c:218)
==639585== by 0x19E2FB: kex_input_kexinit (kex.c:698)
==639585== by 0x1815A5: ssh_dispatch_run (dispatch.c:113)
==639585== by 0x181708: ssh_dispatch_run_fatal (dispatch.c:133)
==639585== by 0x136CA2: ssh_kex2 (sshconnect2.c:346)
==639585== by 0x131F7D: ssh_login (sshconnect.c:1565)
Activity