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

@@ -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;
 }
 

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)

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: