From c180211c6b804628d308743a51a0270873cf7c6f Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Mon, 1 Oct 2018 12:56:32 -0400 Subject: Clean up code that generates session keys This patch simply reworks the code to make it more understandable and reduce if() branches. It also avoids reallocs, and instead uses a support buffer to hold intermediate results of the hmac function so that no buffer overrides happen when the requested size is not an exact mutiple of the digest_len. Signed-off-by: Simo Sorce Reviewed-by: Anderson Toshiyuki Sasaki Reviewed-by: Andreas Schneider --- src/kex.c | 257 +++++++++++++++++++++++++++----------------------------------- 1 file changed, 110 insertions(+), 147 deletions(-) diff --git a/src/kex.c b/src/kex.c index 03d66e70..1aaa4154 100644 --- a/src/kex.c +++ b/src/kex.c @@ -1346,17 +1346,15 @@ int ssh_hashbufin_add_cookie(ssh_session session, unsigned char *cookie) return 0; } -static int generate_one_key(ssh_string k, - struct ssh_crypto_struct *crypto, - unsigned char **output, - char letter, +static int generate_one_key(ssh_string k, struct ssh_crypto_struct *crypto, + unsigned char *output, char letter, size_t requested_size) { ssh_mac_ctx ctx; - unsigned char *tmp; size_t size = crypto->digest_len; - ctx = ssh_mac_ctx_init(crypto->mac_type); + unsigned char digest[size]; + ctx = ssh_mac_ctx_init(crypto->mac_type); if (ctx == NULL) { return -1; } @@ -1365,25 +1363,27 @@ static int generate_one_key(ssh_string k, ssh_mac_update(ctx, crypto->secret_hash, crypto->digest_len); ssh_mac_update(ctx, &letter, 1); ssh_mac_update(ctx, crypto->session_id, crypto->digest_len); - ssh_mac_final(*output, ctx); + ssh_mac_final(digest, ctx); - while(requested_size > size) { - tmp = realloc(*output, size + crypto->digest_len); - if (tmp == NULL) { - return -1; - } - *output = tmp; + if (requested_size < size) { + size = requested_size; + } + memcpy(output, digest, size); + while (requested_size > size) { ctx = ssh_mac_ctx_init(crypto->mac_type); if (ctx == NULL) { return -1; } ssh_mac_update(ctx, k, ssh_string_len(k) + 4); - ssh_mac_update(ctx, - crypto->secret_hash, - crypto->digest_len); - ssh_mac_update(ctx, tmp, size); - ssh_mac_final(tmp + size, ctx); + ssh_mac_update(ctx, crypto->secret_hash, crypto->digest_len); + ssh_mac_update(ctx, output, size); + ssh_mac_final(digest, ctx); + if (requested_size < size + crypto->digest_len) { + memcpy(output+size, digest, requested_size - size); + } else { + memcpy(output+size, digest, crypto->digest_len); + } size += crypto->digest_len; } @@ -1394,6 +1394,17 @@ int ssh_generate_session_keys(ssh_session session) { ssh_string k_string = NULL; struct ssh_crypto_struct *crypto = session->next_crypto; + unsigned char *IV_cli_to_srv = NULL; + unsigned char *IV_srv_to_cli = NULL; + unsigned char *enckey_cli_to_srv = NULL; + unsigned char *enckey_srv_to_cli = NULL; + unsigned char *intkey_cli_to_srv = NULL; + unsigned char *intkey_srv_to_cli = NULL; + size_t IV_len = 0; + size_t enckey_cli_to_srv_len = 0; + size_t enckey_srv_to_cli_len = 0; + size_t intkey_cli_to_srv_len = 0; + size_t intkey_srv_to_cli_len = 0; int rc = -1; k_string = ssh_make_bignum_string(crypto->k); @@ -1402,153 +1413,105 @@ int ssh_generate_session_keys(ssh_session session) goto error; } - crypto->encryptIV = malloc(crypto->digest_len); - crypto->decryptIV = malloc(crypto->digest_len); - crypto->encryptkey = malloc(crypto->digest_len); - crypto->decryptkey = malloc(crypto->digest_len); - crypto->encryptMAC = malloc(crypto->digest_len); - crypto->decryptMAC = malloc(crypto->digest_len); - if (crypto->encryptIV == NULL || - crypto->decryptIV == NULL || - crypto->encryptkey == NULL || crypto->decryptkey == NULL || - crypto->encryptMAC == NULL || crypto->decryptMAC == NULL){ + IV_len = crypto->digest_len; + if (session->client) { + enckey_cli_to_srv_len = crypto->out_cipher->keysize / 8; + enckey_srv_to_cli_len = crypto->in_cipher->keysize / 8; + intkey_cli_to_srv_len = hmac_digest_len(crypto->out_hmac); + intkey_srv_to_cli_len = hmac_digest_len(crypto->in_hmac); + } else { + enckey_cli_to_srv_len = crypto->in_cipher->keysize / 8; + enckey_srv_to_cli_len = crypto->out_cipher->keysize / 8; + intkey_cli_to_srv_len = hmac_digest_len(crypto->in_hmac); + intkey_srv_to_cli_len = hmac_digest_len(crypto->out_hmac); + } + + IV_cli_to_srv = malloc(IV_len); + IV_srv_to_cli = malloc(IV_len); + enckey_cli_to_srv = malloc(enckey_cli_to_srv_len); + enckey_srv_to_cli = malloc(enckey_srv_to_cli_len); + intkey_cli_to_srv = malloc(intkey_cli_to_srv_len); + intkey_srv_to_cli = malloc(intkey_srv_to_cli_len); + if (IV_cli_to_srv == NULL || IV_srv_to_cli == NULL || + enckey_cli_to_srv == NULL || enckey_srv_to_cli == NULL || + intkey_cli_to_srv == NULL || intkey_srv_to_cli == NULL) { ssh_set_error_oom(session); goto error; } /* IV */ - if (session->client) { - rc = generate_one_key(k_string, - crypto, - &crypto->encryptIV, - 'A', - crypto->digest_len); - if (rc < 0) { - goto error; - } - rc = generate_one_key(k_string, - crypto, - &crypto->decryptIV, - 'B', - crypto->digest_len); - if (rc < 0) { - goto error; - } - } else { - rc = generate_one_key(k_string, - crypto, - &crypto->decryptIV, - 'A', - crypto->digest_len); - if (rc < 0) { - goto error; - } - rc = generate_one_key(k_string, - crypto, - &crypto->encryptIV, - 'B', - crypto->digest_len); - if (rc < 0) { - goto error; - } + rc = generate_one_key(k_string, crypto, IV_cli_to_srv, 'A', IV_len); + if (rc < 0) { + goto error; } - if (session->client) { - rc = generate_one_key(k_string, - crypto, - &crypto->encryptkey, - 'C', - crypto->out_cipher->keysize / 8); - if (rc < 0) { - goto error; - } - rc = generate_one_key(k_string, - crypto, - &crypto->decryptkey, - 'D', - crypto->in_cipher->keysize / 8); - if (rc < 0) { - goto error; - } - } else { - rc = generate_one_key(k_string, - crypto, - &crypto->decryptkey, - 'C', - crypto->in_cipher->keysize / 8); - if (rc < 0) { - goto error; - } - rc = generate_one_key(k_string, - crypto, - &crypto->encryptkey, - 'D', - crypto->out_cipher->keysize / 8); - if (rc < 0) { - goto error; - } + rc = generate_one_key(k_string, crypto, IV_srv_to_cli, 'B', IV_len); + if (rc < 0) { + goto error; + } + /* Encryption Key */ + rc = generate_one_key(k_string, crypto, enckey_cli_to_srv, 'C', + enckey_cli_to_srv_len); + if (rc < 0) { + goto error; + } + rc = generate_one_key(k_string, crypto, enckey_srv_to_cli, 'D', + enckey_srv_to_cli_len); + if (rc < 0) { + goto error; + } + /* Integrity Key */ + rc = generate_one_key(k_string, crypto, intkey_cli_to_srv, 'E', + intkey_cli_to_srv_len); + if (rc < 0) { + goto error; + } + rc = generate_one_key(k_string, crypto, intkey_srv_to_cli, 'F', + intkey_srv_to_cli_len); + if (rc < 0) { + goto error; } - if(session->client) { - rc = generate_one_key(k_string, - crypto, - &crypto->encryptMAC, - 'E', - hmac_digest_len(crypto->out_hmac)); - if (rc < 0) { - goto error; - } - rc = generate_one_key(k_string, - crypto, - &crypto->decryptMAC, - 'F', - hmac_digest_len(crypto->in_hmac)); - if (rc < 0) { - goto error; - } + if (session->client) { + crypto->encryptIV = IV_cli_to_srv; + crypto->decryptIV = IV_srv_to_cli; + crypto->encryptkey = enckey_cli_to_srv; + crypto->decryptkey = enckey_srv_to_cli; + crypto->encryptMAC = intkey_cli_to_srv; + crypto->decryptMAC = intkey_srv_to_cli; } else { - rc = generate_one_key(k_string, - crypto, - &crypto->decryptMAC, - 'E', - hmac_digest_len(crypto->in_hmac)); - if (rc < 0) { - goto error; - } - rc = generate_one_key(k_string, - crypto, - &crypto->encryptMAC, - 'F', - hmac_digest_len(crypto->out_hmac)); - if (rc < 0) { - goto error; - } + crypto->encryptIV = IV_srv_to_cli; + crypto->decryptIV = IV_cli_to_srv; + crypto->encryptkey = enckey_srv_to_cli; + crypto->decryptkey = enckey_cli_to_srv; + crypto->encryptMAC = intkey_srv_to_cli; + crypto->decryptMAC = intkey_cli_to_srv; } #ifdef DEBUG_CRYPTO - ssh_print_hexa("Encrypt IV", - crypto->encryptIV, - crypto->digest_len); - ssh_print_hexa("Decrypt IV", - crypto->decryptIV, - crypto->digest_len); - ssh_print_hexa("Encryption key", - crypto->encryptkey, - crypto->out_cipher->keysize / 8); - ssh_print_hexa("Decryption key", - crypto->decryptkey, - crypto->in_cipher->keysize / 8); - ssh_print_hexa("Encryption MAC", - crypto->encryptMAC, - hmac_digest_len(crypto->out_hmac)); - ssh_print_hexa("Decryption MAC", - crypto->decryptMAC, - hmac_digest_len(crypto->in_hmac)); + ssh_print_hexa("Client to Server IV", IV_cli_to_srv, IV_len); + ssh_print_hexa("Server to Client IV", IV_srv_to_cli, IV_len); + ssh_print_hexa("Client to Server Encryption Key", enckey_cli_to_srv, + enckey_cli_to_srv_len); + ssh_print_hexa("Server to Client Encryption Key", enckey_srv_to_cli, + enckey_srv_to_cli_len); + ssh_print_hexa("Client to Server Integrity Key", intkey_cli_to_srv, + intkey_cli_to_srv_len); + ssh_print_hexa("Server to Client Integrity Key", intkey_srv_to_cli, + intkey_srv_to_cli_len); #endif rc = 0; error: ssh_string_burn(k_string); ssh_string_free(k_string); + if (rc != 0) { + free(IV_cli_to_srv); + free(IV_srv_to_cli); + free(enckey_cli_to_srv); + free(enckey_srv_to_cli); + free(intkey_cli_to_srv); + free(intkey_srv_to_cli); + } return rc; } -- cgit v1.2.3