Mitigation for VU#958563: When using a CBC-mode server-to-client cipher
authorben <ben@cda61777-01e9-0310-a592-d414129be87e>
Wed, 26 Nov 2008 12:49:25 +0000 (12:49 +0000)
committerben <ben@cda61777-01e9-0310-a592-d414129be87e>
Wed, 26 Nov 2008 12:49:25 +0000 (12:49 +0000)
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
ssh.h
sshmd5.c
sshsha.c

diff --git a/ssh.c b/ssh.c
index 53093de..6a882b7 100644 (file)
--- 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 7e928ff..2e9194f 100644 (file)
--- 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;
index 215b81f..7112d40 100644 (file)
--- 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"
index 1db5c26..cbe9d78 100644 (file)
--- 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"