From 9916cc1e8d26f1caef36fc1ee525819e6ca62f42 Mon Sep 17 00:00:00 2001 From: ben Date: Wed, 26 Nov 2008 12:49:25 +0000 Subject: [PATCH] Mitigation for VU#958563: When using a CBC-mode server-to-client cipher under SSH-2, don't risk looking at the length field of an incoming packet until we've successfully MAC'ed the packet. This requires a change to the MAC mechanics so that we can calculate MACs incrementally, and output a MAC for the packet so far while still being able to add more data to the packet later. git-svn-id: svn://svn.tartarus.org/sgt/putty@8334 cda61777-01e9-0310-a592-d414129be87e --- ssh.c | 221 +++++++++++++++++++++++++++++++++++++++++++-------------------- ssh.h | 6 ++ sshmd5.c | 43 ++++++++++--- sshsha.c | 70 ++++++++++++++++---- 4 files changed, 252 insertions(+), 88 deletions(-) diff --git a/ssh.c b/ssh.c index 53093de0..6a882b79 100644 --- a/ssh.c +++ b/ssh.c @@ -492,6 +492,16 @@ static void do_ssh2_authconn(Ssh ssh, unsigned char *in, int inlen, * * - OUR_V2_BIGWIN is the window size we advertise for the only * channel in a simple connection. It must be <= INT_MAX. + * + * - OUR_V2_MAXPKT is the official "maximum packet size" we send + * to the remote side. This actually has nothing to do with the + * size of the _packet_, but is instead a limit on the amount + * of data we're willing to receive in a single SSH2 channel + * data message. + * + * - OUR_V2_PACKETLIMIT is actually the maximum size of SSH + * _packet_ we're prepared to cope with. It must be a multiple + * of the cipher block size, and must be at least 35000. */ #define SSH1_BUFFER_LIMIT 32768 @@ -499,6 +509,7 @@ static void do_ssh2_authconn(Ssh ssh, unsigned char *in, int inlen, #define OUR_V2_WINSIZE 16384 #define OUR_V2_BIGWIN 0x7fffffff #define OUR_V2_MAXPKT 0x4000UL +#define OUR_V2_PACKETLIMIT 0x9000UL /* Maximum length of passwords/passphrases (arbitrary) */ #define SSH_MAX_PASSWORD_LEN 100 @@ -1309,90 +1320,162 @@ static struct Packet *ssh2_rdpkt(Ssh ssh, unsigned char **data, int *datalen) st->cipherblk = 8; if (st->cipherblk < 8) st->cipherblk = 8; + st->maclen = ssh->scmac ? ssh->scmac->len : 0; - st->pktin->data = snewn(st->cipherblk + APIEXTRA, unsigned char); + if (ssh->sccipher && (ssh->sccipher->flags & SSH_CIPHER_IS_CBC) && + ssh->scmac) { + /* + * When dealing with a CBC-mode cipher, we want to avoid the + * possibility of an attacker's tweaking the ciphertext stream + * so as to cause us to feed the same block to the block + * cipher more than once and thus leak information + * (VU#958563). The way we do this is not to take any + * decisions on the basis of anything we've decrypted until + * we've verified it with a MAC. That includes the packet + * length, so we just read data and check the MAC repeatedly, + * and when the MAC passes, see if the length we've got is + * plausible. + */ - /* - * Acquire and decrypt the first block of the packet. This will - * contain the length and padding details. - */ - for (st->i = st->len = 0; st->i < st->cipherblk; st->i++) { - while ((*datalen) == 0) - crReturn(NULL); - st->pktin->data[st->i] = *(*data)++; - (*datalen)--; - } + /* May as well allocate the whole lot now. */ + st->pktin->data = snewn(OUR_V2_PACKETLIMIT + st->maclen + APIEXTRA, + unsigned char); - if (ssh->sccipher) - ssh->sccipher->decrypt(ssh->sc_cipher_ctx, - st->pktin->data, st->cipherblk); + /* Read an amount corresponding to the MAC. */ + for (st->i = 0; st->i < st->maclen; st->i++) { + while ((*datalen) == 0) + crReturn(NULL); + st->pktin->data[st->i] = *(*data)++; + (*datalen)--; + } - /* - * Now get the length and padding figures. - */ - st->len = GET_32BIT(st->pktin->data); - st->pad = st->pktin->data[4]; + st->packetlen = 0; + { + unsigned char seq[4]; + ssh->scmac->start(ssh->sc_mac_ctx); + PUT_32BIT(seq, st->incoming_sequence); + ssh->scmac->bytes(ssh->sc_mac_ctx, seq, 4); + } - /* - * _Completely_ silly lengths should be stomped on before they - * do us any more damage. - */ - if (st->len < 0 || st->len > 35000 || st->pad < 4 || - st->len - st->pad < 1 || (st->len + 4) % st->cipherblk != 0) { - bombout(("Incoming packet was garbled on decryption")); - ssh_free_packet(st->pktin); - crStop(NULL); - } + for (;;) { /* Once around this loop per cipher block. */ + /* Read another cipher-block's worth, and tack it onto the end. */ + for (st->i = 0; st->i < st->cipherblk; st->i++) { + while ((*datalen) == 0) + crReturn(NULL); + st->pktin->data[st->packetlen+st->maclen+st->i] = *(*data)++; + (*datalen)--; + } + /* Decrypt one more block (a little further back in the stream). */ + ssh->sccipher->decrypt(ssh->sc_cipher_ctx, + st->pktin->data + st->packetlen, + st->cipherblk); + /* Feed that block to the MAC. */ + ssh->scmac->bytes(ssh->sc_mac_ctx, + st->pktin->data + st->packetlen, st->cipherblk); + st->packetlen += st->cipherblk; + /* See if that gives us a valid packet. */ + if (ssh->scmac->verresult(ssh->sc_mac_ctx, + st->pktin->data + st->packetlen) && + (st->len = GET_32BIT(st->pktin->data)) + 4 == st->packetlen) + break; + if (st->packetlen >= OUR_V2_PACKETLIMIT) { + bombout(("No valid incoming packet found")); + ssh_free_packet(st->pktin); + crStop(NULL); + } + } + st->pktin->maxlen = st->packetlen + st->maclen; + st->pktin->data = sresize(st->pktin->data, + st->pktin->maxlen + APIEXTRA, + unsigned char); + } else { + st->pktin->data = snewn(st->cipherblk + APIEXTRA, unsigned char); - /* - * This enables us to deduce the payload length. - */ - st->payload = st->len - st->pad - 1; + /* + * Acquire and decrypt the first block of the packet. This will + * contain the length and padding details. + */ + for (st->i = st->len = 0; st->i < st->cipherblk; st->i++) { + while ((*datalen) == 0) + crReturn(NULL); + st->pktin->data[st->i] = *(*data)++; + (*datalen)--; + } - st->pktin->length = st->payload + 5; + if (ssh->sccipher) + ssh->sccipher->decrypt(ssh->sc_cipher_ctx, + st->pktin->data, st->cipherblk); - /* - * So now we can work out the total packet length. - */ - st->packetlen = st->len + 4; - st->maclen = ssh->scmac ? ssh->scmac->len : 0; + /* + * Now get the length figure. + */ + st->len = GET_32BIT(st->pktin->data); - /* - * Allocate memory for the rest of the packet. - */ - st->pktin->maxlen = st->packetlen + st->maclen; - st->pktin->data = sresize(st->pktin->data, - st->pktin->maxlen + APIEXTRA, - unsigned char); + /* + * _Completely_ silly lengths should be stomped on before they + * do us any more damage. + */ + if (st->len < 0 || st->len > OUR_V2_PACKETLIMIT || + (st->len + 4) % st->cipherblk != 0) { + bombout(("Incoming packet was garbled on decryption")); + ssh_free_packet(st->pktin); + crStop(NULL); + } - /* - * Read and decrypt the remainder of the packet. - */ - for (st->i = st->cipherblk; st->i < st->packetlen + st->maclen; - st->i++) { - while ((*datalen) == 0) - crReturn(NULL); - st->pktin->data[st->i] = *(*data)++; - (*datalen)--; - } - /* Decrypt everything _except_ the MAC. */ - if (ssh->sccipher) - ssh->sccipher->decrypt(ssh->sc_cipher_ctx, - st->pktin->data + st->cipherblk, - st->packetlen - st->cipherblk); + /* + * So now we can work out the total packet length. + */ + st->packetlen = st->len + 4; - st->pktin->encrypted_len = st->packetlen; + /* + * Allocate memory for the rest of the packet. + */ + st->pktin->maxlen = st->packetlen + st->maclen; + st->pktin->data = sresize(st->pktin->data, + st->pktin->maxlen + APIEXTRA, + unsigned char); - /* - * Check the MAC. - */ - if (ssh->scmac - && !ssh->scmac->verify(ssh->sc_mac_ctx, st->pktin->data, st->len + 4, - st->incoming_sequence)) { - bombout(("Incorrect MAC received on packet")); + /* + * Read and decrypt the remainder of the packet. + */ + for (st->i = st->cipherblk; st->i < st->packetlen + st->maclen; + st->i++) { + while ((*datalen) == 0) + crReturn(NULL); + st->pktin->data[st->i] = *(*data)++; + (*datalen)--; + } + /* Decrypt everything _except_ the MAC. */ + if (ssh->sccipher) + ssh->sccipher->decrypt(ssh->sc_cipher_ctx, + st->pktin->data + st->cipherblk, + st->packetlen - st->cipherblk); + + /* + * Check the MAC. + */ + if (ssh->scmac + && !ssh->scmac->verify(ssh->sc_mac_ctx, st->pktin->data, + st->len + 4, st->incoming_sequence)) { + bombout(("Incorrect MAC received on packet")); + ssh_free_packet(st->pktin); + crStop(NULL); + } + } + /* Get and sanity-check the amount of random padding. */ + st->pad = st->pktin->data[4]; + if (st->pad < 4 || st->len - st->pad < 1) { + bombout(("Invalid padding length on received packet")); ssh_free_packet(st->pktin); crStop(NULL); } + /* + * This enables us to deduce the payload length. + */ + st->payload = st->len - st->pad - 1; + + st->pktin->length = st->payload + 5; + st->pktin->encrypted_len = st->packetlen; st->pktin->sequence = st->incoming_sequence++; diff --git a/ssh.h b/ssh.h index 7e928ff1..2e9194fd 100644 --- a/ssh.h +++ b/ssh.h @@ -190,8 +190,14 @@ struct ssh_mac { void *(*make_context)(void); void (*free_context)(void *); void (*setkey) (void *, unsigned char *key); + /* whole-packet operations */ void (*generate) (void *, unsigned char *blk, int len, unsigned long seq); int (*verify) (void *, unsigned char *blk, int len, unsigned long seq); + /* partial-packet operations */ + void (*start) (void *); + void (*bytes) (void *, unsigned char const *, int); + void (*genresult) (void *, unsigned char *); + int (*verresult) (void *, unsigned char const *); char *name; int len; char *text_name; diff --git a/sshmd5.c b/sshmd5.c index 215b81fd..7112d406 100644 --- a/sshmd5.c +++ b/sshmd5.c @@ -222,7 +222,7 @@ void MD5Simple(void const *p, unsigned len, unsigned char output[16]) void *hmacmd5_make_context(void) { - return snewn(2, struct MD5Context); + return snewn(3, struct MD5Context); } void hmacmd5_free_context(void *handle) @@ -257,24 +257,50 @@ static void hmacmd5_key_16(void *handle, unsigned char *key) hmacmd5_key(handle, key, 16); } -static void hmacmd5_do_hmac_internal(void *handle, - unsigned char const *blk, int len, - unsigned char const *blk2, int len2, - unsigned char *hmac) +static void hmacmd5_start(void *handle) +{ + struct MD5Context *keys = (struct MD5Context *)handle; + + keys[2] = keys[0]; /* structure copy */ +} + +static void hmacmd5_bytes(void *handle, unsigned char const *blk, int len) +{ + struct MD5Context *keys = (struct MD5Context *)handle; + MD5Update(&keys[2], blk, len); +} + +static void hmacmd5_genresult(void *handle, unsigned char *hmac) { struct MD5Context *keys = (struct MD5Context *)handle; struct MD5Context s; unsigned char intermediate[16]; - s = keys[0]; /* structure copy */ - MD5Update(&s, blk, len); - if (blk2) MD5Update(&s, blk2, len2); + s = keys[2]; /* structure copy */ MD5Final(intermediate, &s); s = keys[1]; /* structure copy */ MD5Update(&s, intermediate, 16); MD5Final(hmac, &s); } +static int hmacmd5_verresult(void *handle, unsigned char const *hmac) +{ + unsigned char correct[16]; + hmacmd5_genresult(handle, correct); + return !memcmp(correct, hmac, 16); +} + +static void hmacmd5_do_hmac_internal(void *handle, + unsigned char const *blk, int len, + unsigned char const *blk2, int len2, + unsigned char *hmac) +{ + hmacmd5_start(handle); + hmacmd5_bytes(handle, blk, len); + if (blk2) hmacmd5_bytes(handle, blk2, len2); + hmacmd5_genresult(handle, hmac); +} + void hmacmd5_do_hmac(void *handle, unsigned char const *blk, int len, unsigned char *hmac) { @@ -311,6 +337,7 @@ static int hmacmd5_verify(void *handle, unsigned char *blk, int len, const struct ssh_mac ssh_hmac_md5 = { hmacmd5_make_context, hmacmd5_free_context, hmacmd5_key_16, hmacmd5_generate, hmacmd5_verify, + hmacmd5_start, hmacmd5_bytes, hmacmd5_genresult, hmacmd5_verresult, "hmac-md5", 16, "HMAC-MD5" diff --git a/sshsha.c b/sshsha.c index 1db5c26d..cbe9d78b 100644 --- a/sshsha.c +++ b/sshsha.c @@ -227,7 +227,7 @@ const struct ssh_hash ssh_sha1 = { static void *sha1_make_context(void) { - return snewn(2, SHA_State); + return snewn(3, SHA_State); } static void sha1_free_context(void *handle) @@ -266,33 +266,61 @@ static void sha1_key_buggy(void *handle, unsigned char *key) sha1_key_internal(handle, key, 16); } -static void sha1_do_hmac(void *handle, unsigned char *blk, int len, - unsigned long seq, unsigned char *hmac) +static void hmacsha1_start(void *handle) +{ + SHA_State *keys = (SHA_State *)handle; + + keys[2] = keys[0]; /* structure copy */ +} + +static void hmacsha1_bytes(void *handle, unsigned char const *blk, int len) +{ + SHA_State *keys = (SHA_State *)handle; + SHA_Bytes(&keys[2], (void *)blk, len); +} + +static void hmacsha1_genresult(void *handle, unsigned char *hmac) { SHA_State *keys = (SHA_State *)handle; SHA_State s; unsigned char intermediate[20]; - intermediate[0] = (unsigned char) ((seq >> 24) & 0xFF); - intermediate[1] = (unsigned char) ((seq >> 16) & 0xFF); - intermediate[2] = (unsigned char) ((seq >> 8) & 0xFF); - intermediate[3] = (unsigned char) ((seq) & 0xFF); - - s = keys[0]; /* structure copy */ - SHA_Bytes(&s, intermediate, 4); - SHA_Bytes(&s, blk, len); + s = keys[2]; /* structure copy */ SHA_Final(&s, intermediate); s = keys[1]; /* structure copy */ SHA_Bytes(&s, intermediate, 20); SHA_Final(&s, hmac); } +static void sha1_do_hmac(void *handle, unsigned char *blk, int len, + unsigned long seq, unsigned char *hmac) +{ + unsigned char seqbuf[4]; + + seqbuf[0] = (unsigned char) ((seq >> 24) & 0xFF); + seqbuf[1] = (unsigned char) ((seq >> 16) & 0xFF); + seqbuf[2] = (unsigned char) ((seq >> 8) & 0xFF); + seqbuf[3] = (unsigned char) ((seq) & 0xFF); + + hmacsha1_start(handle); + hmacsha1_bytes(handle, seqbuf, 4); + hmacsha1_bytes(handle, blk, len); + hmacsha1_genresult(handle, hmac); +} + static void sha1_generate(void *handle, unsigned char *blk, int len, unsigned long seq) { sha1_do_hmac(handle, blk, len, seq, blk + len); } +static int hmacsha1_verresult(void *handle, unsigned char const *hmac) +{ + unsigned char correct[20]; + hmacsha1_genresult(handle, correct); + return !memcmp(correct, hmac, 20); +} + static int sha1_verify(void *handle, unsigned char *blk, int len, unsigned long seq) { @@ -301,6 +329,13 @@ static int sha1_verify(void *handle, unsigned char *blk, int len, return !memcmp(correct, blk + len, 20); } +static void hmacsha1_96_genresult(void *handle, unsigned char *hmac) +{ + unsigned char full[20]; + hmacsha1_genresult(handle, full); + memcpy(hmac, full, 12); +} + static void sha1_96_generate(void *handle, unsigned char *blk, int len, unsigned long seq) { @@ -309,6 +344,13 @@ static void sha1_96_generate(void *handle, unsigned char *blk, int len, memcpy(blk + len, full, 12); } +static int hmacsha1_96_verresult(void *handle, unsigned char const *hmac) +{ + unsigned char correct[20]; + hmacsha1_genresult(handle, correct); + return !memcmp(correct, hmac, 12); +} + static int sha1_96_verify(void *handle, unsigned char *blk, int len, unsigned long seq) { @@ -333,6 +375,7 @@ void hmac_sha1_simple(void *key, int keylen, void *data, int datalen, const struct ssh_mac ssh_hmac_sha1 = { sha1_make_context, sha1_free_context, sha1_key, sha1_generate, sha1_verify, + hmacsha1_start, hmacsha1_bytes, hmacsha1_genresult, hmacsha1_verresult, "hmac-sha1", 20, "HMAC-SHA1" @@ -341,6 +384,8 @@ const struct ssh_mac ssh_hmac_sha1 = { const struct ssh_mac ssh_hmac_sha1_96 = { sha1_make_context, sha1_free_context, sha1_key, sha1_96_generate, sha1_96_verify, + hmacsha1_start, hmacsha1_bytes, + hmacsha1_96_genresult, hmacsha1_96_verresult, "hmac-sha1-96", 12, "HMAC-SHA1-96" @@ -349,6 +394,7 @@ const struct ssh_mac ssh_hmac_sha1_96 = { const struct ssh_mac ssh_hmac_sha1_buggy = { sha1_make_context, sha1_free_context, sha1_key_buggy, sha1_generate, sha1_verify, + hmacsha1_start, hmacsha1_bytes, hmacsha1_genresult, hmacsha1_verresult, "hmac-sha1", 20, "bug-compatible HMAC-SHA1" @@ -357,6 +403,8 @@ const struct ssh_mac ssh_hmac_sha1_buggy = { const struct ssh_mac ssh_hmac_sha1_96_buggy = { sha1_make_context, sha1_free_context, sha1_key_buggy, sha1_96_generate, sha1_96_verify, + hmacsha1_start, hmacsha1_bytes, + hmacsha1_96_genresult, hmacsha1_96_verresult, "hmac-sha1-96", 12, "bug-compatible HMAC-SHA1-96" -- 2.11.0