Skip to content
Browse files

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
  • Loading branch information...
1 parent c677cad commit 9916cc1e8d26f1caef36fc1ee525819e6ca62f42 ben committed Nov 26, 2008
Showing with 252 additions and 88 deletions.
  1. +152 −69 ssh.c
  2. +6 −0 ssh.h
  3. +35 −8 sshmd5.c
  4. +59 −11 sshsha.c
View
221 ssh.c
@@ -492,13 +492,24 @@ 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
#define SSH_MAX_BACKLOG 32768
#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++;
View
6 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;
View
43 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"
View
70 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"

0 comments on commit 9916cc1

Please sign in to comment.
Something went wrong with that request. Please try again.