From 43be90e287996e1be6f92f5a426475df25c16e10 Mon Sep 17 00:00:00 2001 From: Simon Tatham Date: Thu, 10 Sep 2015 08:10:52 +0100 Subject: [PATCH] Split ssh2_cipher's keylen field into two. The revamp of key generation in commit e460f3083 made the assumption that you could decide how many bytes of key material to generate by converting cipher->keylen from bits to bytes. This is a good assumption for all ciphers except DES/3DES: since the SSH DES key setup ignores one bit in every byte of key material it's given, you need more bytes than its keylen field would have you believe. So currently the DES ciphers aren't being keyed correctly. The original keylen field is used for deciding how big a DH group to request, and on that basis I think it still makes sense to keep it reflecting the true entropy of a cipher key. So it turns out we need two _separate_ key length fields per cipher - one for the real entropy, and one for the much more obvious purpose of knowing how much data to ask for from ssh2_mkkey. A compensatory advantage, though, is that we can now measure the latter directly in bytes rather than bits, so we no longer have to faff about with dividing by 8 and rounding up. --- ssh.c | 12 ++++++------ ssh.h | 14 +++++++++++++- sshaes.c | 14 +++++++------- ssharcf.c | 4 ++-- sshblowf.c | 4 ++-- sshccp.c | 2 +- sshdes.c | 8 ++++---- 7 files changed, 35 insertions(+), 23 deletions(-) diff --git a/ssh.c b/ssh.c index f56e10a9..4979268f 100644 --- a/ssh.c +++ b/ssh.c @@ -6728,8 +6728,8 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, { int csbits, scbits; - csbits = s->cscipher_tobe->keylen; - scbits = s->sccipher_tobe->keylen; + csbits = s->cscipher_tobe->real_keybits; + scbits = s->sccipher_tobe->real_keybits; s->nbits = (csbits > scbits ? csbits : scbits); } /* The keys only have hlen-bit entropy, since they're based on @@ -7183,9 +7183,9 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, unsigned char *key; key = ssh2_mkkey(ssh, s->K, s->exchange_hash, 'C', - (ssh->cscipher->keylen+7) / 8); + ssh->cscipher->padded_keybytes); ssh->cscipher->setkey(ssh->cs_cipher_ctx, key); - smemclr(key, (ssh->cscipher->keylen+7) / 8); + smemclr(key, ssh->cscipher->padded_keybytes); sfree(key); key = ssh2_mkkey(ssh, s->K, s->exchange_hash, 'A', @@ -7256,9 +7256,9 @@ static void do_ssh2_transport(Ssh ssh, const void *vin, int inlen, unsigned char *key; key = ssh2_mkkey(ssh, s->K, s->exchange_hash, 'D', - (ssh->sccipher->keylen + 7) / 8); + ssh->sccipher->padded_keybytes); ssh->sccipher->setkey(ssh->sc_cipher_ctx, key); - smemclr(key, (ssh->sccipher->keylen + 7) / 8); + smemclr(key, ssh->sccipher->padded_keybytes); sfree(key); key = ssh2_mkkey(ssh, s->K, s->exchange_hash, 'B', diff --git a/ssh.h b/ssh.h index ef488e83..e8f1adad 100644 --- a/ssh.h +++ b/ssh.h @@ -319,7 +319,19 @@ struct ssh2_cipher { void (*decrypt_length) (void *, unsigned char *blk, int len, unsigned long seq); const char *name; int blksize; - int keylen; + /* real_keybits is the number of bits of entropy genuinely used by + * the cipher scheme; it's used for deciding how big a + * Diffie-Hellman group is needed to exchange a key for the + * cipher. */ + int real_keybits; + /* padded_keybytes is the number of bytes of key data expected as + * input to the setkey function; it's used for deciding how much + * data needs to be generated from the post-kex generation of key + * material. In a sensible cipher which uses all its key bytes for + * real work, this will just be real_keybits/8, but in DES-type + * ciphers which ignore one bit in each byte, it'll be slightly + * different. */ + int padded_keybytes; unsigned int flags; #define SSH_CIPHER_IS_CBC 1 #define SSH_CIPHER_SEPARATE_LENGTH 2 diff --git a/sshaes.c b/sshaes.c index 0465d879..904cbdb2 100644 --- a/sshaes.c +++ b/sshaes.c @@ -1173,7 +1173,7 @@ static const struct ssh2_cipher ssh_aes128_ctr = { aes_make_context, aes_free_context, aes_iv, aes128_key, aes_ssh2_sdctr, aes_ssh2_sdctr, NULL, NULL, "aes128-ctr", - 16, 128, 0, "AES-128 SDCTR", + 16, 128, 16, 0, "AES-128 SDCTR", NULL }; @@ -1181,7 +1181,7 @@ static const struct ssh2_cipher ssh_aes192_ctr = { aes_make_context, aes_free_context, aes_iv, aes192_key, aes_ssh2_sdctr, aes_ssh2_sdctr, NULL, NULL, "aes192-ctr", - 16, 192, 0, "AES-192 SDCTR", + 16, 192, 24, 0, "AES-192 SDCTR", NULL }; @@ -1189,7 +1189,7 @@ static const struct ssh2_cipher ssh_aes256_ctr = { aes_make_context, aes_free_context, aes_iv, aes256_key, aes_ssh2_sdctr, aes_ssh2_sdctr, NULL, NULL, "aes256-ctr", - 16, 256, 0, "AES-256 SDCTR", + 16, 256, 32, 0, "AES-256 SDCTR", NULL }; @@ -1197,7 +1197,7 @@ static const struct ssh2_cipher ssh_aes128 = { aes_make_context, aes_free_context, aes_iv, aes128_key, aes_ssh2_encrypt_blk, aes_ssh2_decrypt_blk, NULL, NULL, "aes128-cbc", - 16, 128, SSH_CIPHER_IS_CBC, "AES-128 CBC", + 16, 128, 16, SSH_CIPHER_IS_CBC, "AES-128 CBC", NULL }; @@ -1205,7 +1205,7 @@ static const struct ssh2_cipher ssh_aes192 = { aes_make_context, aes_free_context, aes_iv, aes192_key, aes_ssh2_encrypt_blk, aes_ssh2_decrypt_blk, NULL, NULL, "aes192-cbc", - 16, 192, SSH_CIPHER_IS_CBC, "AES-192 CBC", + 16, 192, 24, SSH_CIPHER_IS_CBC, "AES-192 CBC", NULL }; @@ -1213,7 +1213,7 @@ static const struct ssh2_cipher ssh_aes256 = { aes_make_context, aes_free_context, aes_iv, aes256_key, aes_ssh2_encrypt_blk, aes_ssh2_decrypt_blk, NULL, NULL, "aes256-cbc", - 16, 256, SSH_CIPHER_IS_CBC, "AES-256 CBC", + 16, 256, 32, SSH_CIPHER_IS_CBC, "AES-256 CBC", NULL }; @@ -1221,7 +1221,7 @@ static const struct ssh2_cipher ssh_rijndael_lysator = { aes_make_context, aes_free_context, aes_iv, aes256_key, aes_ssh2_encrypt_blk, aes_ssh2_decrypt_blk, NULL, NULL, "rijndael-cbc@lysator.liu.se", - 16, 256, SSH_CIPHER_IS_CBC, "AES-256 CBC", + 16, 256, 32, SSH_CIPHER_IS_CBC, "AES-256 CBC", NULL }; diff --git a/ssharcf.c b/ssharcf.c index a9d5f7da..d16ce225 100644 --- a/ssharcf.c +++ b/ssharcf.c @@ -102,7 +102,7 @@ const struct ssh2_cipher ssh_arcfour128_ssh2 = { arcfour_make_context, arcfour_free_context, arcfour_iv, arcfour128_key, arcfour_block, arcfour_block, NULL, NULL, "arcfour128", - 1, 128, 0, "Arcfour-128", + 1, 128, 16, 0, "Arcfour-128", NULL }; @@ -110,7 +110,7 @@ const struct ssh2_cipher ssh_arcfour256_ssh2 = { arcfour_make_context, arcfour_free_context, arcfour_iv, arcfour256_key, arcfour_block, arcfour_block, NULL, NULL, "arcfour256", - 1, 256, 0, "Arcfour-256", + 1, 256, 32, 0, "Arcfour-256", NULL }; diff --git a/sshblowf.c b/sshblowf.c index 8a561b37..353116a6 100644 --- a/sshblowf.c +++ b/sshblowf.c @@ -643,7 +643,7 @@ static const struct ssh2_cipher ssh_blowfish_ssh2 = { blowfish_make_context, blowfish_free_context, blowfish_iv, blowfish_key, blowfish_ssh2_encrypt_blk, blowfish_ssh2_decrypt_blk, NULL, NULL, "blowfish-cbc", - 8, 128, SSH_CIPHER_IS_CBC, "Blowfish-128 CBC", + 8, 128, 16, SSH_CIPHER_IS_CBC, "Blowfish-128 CBC", NULL }; @@ -651,7 +651,7 @@ static const struct ssh2_cipher ssh_blowfish_ssh2_ctr = { blowfish_make_context, blowfish_free_context, blowfish_iv, blowfish256_key, blowfish_ssh2_sdctr, blowfish_ssh2_sdctr, NULL, NULL, "blowfish-ctr", - 8, 256, 0, "Blowfish-256 SDCTR", + 8, 256, 32, 0, "Blowfish-256 SDCTR", NULL }; diff --git a/sshccp.c b/sshccp.c index 5400d36c..0b3dc573 100644 --- a/sshccp.c +++ b/sshccp.c @@ -1331,7 +1331,7 @@ static const struct ssh2_cipher ssh2_chacha20_poly1305 = { ccp_decrypt_length, "chacha20-poly1305@openssh.com", - 1, 512, SSH_CIPHER_SEPARATE_LENGTH, "ChaCha20", + 1, 512, 64, SSH_CIPHER_SEPARATE_LENGTH, "ChaCha20", &ssh2_poly1305 }; diff --git a/sshdes.c b/sshdes.c index 9276fe06..13487fcd 100644 --- a/sshdes.c +++ b/sshdes.c @@ -949,7 +949,7 @@ static const struct ssh2_cipher ssh_3des_ssh2 = { des3_make_context, des3_free_context, des3_iv, des3_key, des3_ssh2_encrypt_blk, des3_ssh2_decrypt_blk, NULL, NULL, "3des-cbc", - 8, 168, SSH_CIPHER_IS_CBC, "triple-DES CBC", + 8, 168, 24, SSH_CIPHER_IS_CBC, "triple-DES CBC", NULL }; @@ -957,7 +957,7 @@ static const struct ssh2_cipher ssh_3des_ssh2_ctr = { des3_make_context, des3_free_context, des3_iv, des3_key, des3_ssh2_sdctr, des3_ssh2_sdctr, NULL, NULL, "3des-ctr", - 8, 168, 0, "triple-DES SDCTR", + 8, 168, 24, 0, "triple-DES SDCTR", NULL }; @@ -973,7 +973,7 @@ static const struct ssh2_cipher ssh_des_ssh2 = { des_make_context, des3_free_context, des3_iv, des_key, des_ssh2_encrypt_blk, des_ssh2_decrypt_blk, NULL, NULL, "des-cbc", - 8, 56, SSH_CIPHER_IS_CBC, "single-DES CBC", + 8, 56, 8, SSH_CIPHER_IS_CBC, "single-DES CBC", NULL }; @@ -981,7 +981,7 @@ static const struct ssh2_cipher ssh_des_sshcom_ssh2 = { des_make_context, des3_free_context, des3_iv, des_key, des_ssh2_encrypt_blk, des_ssh2_decrypt_blk, NULL, NULL, "des-cbc@ssh.com", - 8, 56, SSH_CIPHER_IS_CBC, "single-DES CBC", + 8, 56, 8, SSH_CIPHER_IS_CBC, "single-DES CBC", NULL };