From e9b0f6f8896039824f78a43623cd14b67f24e2ce Mon Sep 17 00:00:00 2001 From: Kevin Kane Date: Thu, 17 Feb 2022 11:10:03 -0800 Subject: [PATCH] Use mpint representation for shared_secret when deriving keys in pure-PQ key exchange, and some other bug fixes; fixes #119 (#120) * Use mpint representation for shared_secret when deriving keys in pure-PQ key exchange, as required by spec; fixes #119 * Increase MAX_PROP to 160 MAX_PROP limits the number of kex algorithm proposals considered from the server by the client. With the liboqs options exceeding this number, an unfortunate ordering in the server's proposal can cause OpenSSH to pick the wrong kex in violation of the RFC. This change increases MAX_PROP to 160, which will allow for a longer list including all liboqs options. * ssh-keygen: When generating a key pair for an OQS hybrid signature algorithm, always use the curve specified by the key type If ssh-keygen is called with the -t parameter using the short name for one of the OQS hybrid algorithms, P-256 incorrectly always ends up being used for the ECDSA part of the key. * Set the error return code in kex_kem_generic_{enc,dec} if the call to OQS_KEM_{en,de}caps fails r is uninitialized otherwise. * Enclose one-line if bodies in braces in kex_kem_generic_{keypair,dec,enc} * Correct inconsistent indendation in newly-added blocks --- kexoqs.c | 35 +++++++++++++++++++++++++++++------ match.c | 2 +- ssh-keygen.c | 6 ++++-- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/kexoqs.c b/kexoqs.c index 648cf1bea6ee..447ed0f69a83 100644 --- a/kexoqs.c +++ b/kexoqs.c @@ -43,10 +43,12 @@ static int kex_kem_generic_keypair(OQS_KEM *kem, struct kex *kex) struct sshbuf *buf = NULL; u_char *cp = NULL; int r; - if ((buf = sshbuf_new()) == NULL) + if ((buf = sshbuf_new()) == NULL) { return SSH_ERR_ALLOC_FAIL; - if ((r = sshbuf_reserve(buf, kem->length_public_key, &cp)) != 0) \ + } + if ((r = sshbuf_reserve(buf, kem->length_public_key, &cp)) != 0) { goto out; + } kex->oqs_client_key_size = kem->length_secret_key; if ((kex->oqs_client_key = malloc(kex->oqs_client_key_size)) == NULL || OQS_KEM_keypair(kem, cp, kex->oqs_client_key) != OQS_SUCCESS) { @@ -68,7 +70,7 @@ static int kex_kem_generic_enc(OQS_KEM *kem, struct kex *kex, struct sshbuf *server_blob = NULL; struct sshbuf *buf = NULL; const u_char *client_pub; - u_char *kem_key, *ciphertext; + u_char *kem_key = NULL, *ciphertext; int r; *server_blobp = NULL; *shared_secretp = NULL; @@ -81,17 +83,24 @@ static int kex_kem_generic_enc(OQS_KEM *kem, struct kex *kex, r = SSH_ERR_ALLOC_FAIL; goto out; } - if ((r = sshbuf_reserve(buf, kem->length_shared_secret, &kem_key)) != 0) + if ((kem_key = malloc(kem->length_shared_secret)) == NULL) { + r = SSH_ERR_ALLOC_FAIL; goto out; + } /* allocate space for encrypted KEM key */ if ((server_blob = sshbuf_new()) == NULL) { r = SSH_ERR_ALLOC_FAIL; goto out; } - if ((r = sshbuf_reserve(server_blob, kem->length_ciphertext, &ciphertext)) != 0) + if ((r = sshbuf_reserve(server_blob, kem->length_ciphertext, &ciphertext)) != 0) { goto out; + } /* generate and encrypt KEM key with client key */ if (OQS_KEM_encaps(kem, ciphertext, kem_key, client_pub) != OQS_SUCCESS) { + r = SSH_ERR_LIBCRYPTO_ERROR; + goto out; + } + if ((r = sshbuf_put_string(buf, kem_key, kem->length_shared_secret)) != 0) { goto out; } *server_blobp = server_blob; @@ -101,6 +110,10 @@ static int kex_kem_generic_enc(OQS_KEM *kem, struct kex *kex, out: sshbuf_free(server_blob); sshbuf_free(buf); + if (kem_key != NULL) { + explicit_bzero(kem_key, kem->length_shared_secret); + free(kem_key); + } return r; } @@ -127,15 +140,25 @@ static int kex_kem_generic_dec(OQS_KEM *kem, r = SSH_ERR_ALLOC_FAIL; goto out; } - if ((r = sshbuf_reserve(buf, kem->length_shared_secret, &kem_key)) != 0) + if ((kem_key = malloc(kem->length_shared_secret)) == NULL) { + r = SSH_ERR_ALLOC_FAIL; goto out; + } if (OQS_KEM_decaps(kem, kem_key, ciphertext, kex->oqs_client_key) != OQS_SUCCESS) { + r = SSH_ERR_LIBCRYPTO_ERROR; + goto out; + } + if ((r = sshbuf_put_string(buf, kem_key, kem->length_shared_secret)) != 0) { goto out; } *shared_secretp = buf; buf = NULL; out: sshbuf_free(buf); + if (kem_key != NULL) { + explicit_bzero(kem_key, kem->length_shared_secret); + free(kem_key); + } return r; } diff --git a/match.c b/match.c index 55bf784a0a39..15cc158b7654 100644 --- a/match.c +++ b/match.c @@ -265,7 +265,7 @@ match_user(const char *user, const char *host, const char *ipaddr, * Returns first item from client-list that is also supported by server-list, * caller must free the returned string. */ -#define MAX_PROP 60 +#define MAX_PROP 160 #define SEP "," char * match_list(const char *client, const char *server, u_int *next) diff --git a/ssh-keygen.c b/ssh-keygen.c index e3acd925d9a5..cb8076173914 100644 --- a/ssh-keygen.c +++ b/ssh-keygen.c @@ -213,9 +213,11 @@ type_bits_valid(int type, const char *name, u_int32_t *bitsp) * by different types (unlike ECDSA which uses one key type and a 2nd * 'nid' value to identify the curve. We need this special processing * for ECDSA hybrid of levels 3+ to avoid defaulting to P256 when - * name is NULL (like when called from do_gen_all_hostkeys). + * name is NULL (like when called from do_gen_all_hostkeys) or when the + * -t parameter is provided with a shortname, which sshkey_ecdsa_nid_from_name + * doesn't check. */ - if (name == NULL && oqs_utils_is_ecdsa_hybrid(type)) { + if (oqs_utils_is_ecdsa_hybrid(type)) { switch (type) { ///// OQS_TEMPLATE_FRAGMENT_HANDLE_ECDSA_HYBRIDS_START case KEY_ECDSA_NISTP521_FALCON_1024: