upstream commit

Improve crypto ordering for Encrypt-then-MAC (EtM) mode
MAC algorithms.

Previously we were computing the MAC, decrypting the packet and then
checking the MAC. This gave rise to the possibility of creating a
side-channel oracle in the decryption step, though no such oracle has
been identified.

This adds a mac_check() function that computes and checks the MAC in
one pass, and uses it to advance MAC checking for EtM algorithms to
before payload decryption.

Reported by Jean Paul Degabriele, Kenny Paterson, Torben Hansen and
Martin Albrecht. feedback and ok markus@

Upstream-ID: 1999bb67cab47dda5b10b80d8155fe83d4a1867b

diff --git a/mac.c b/mac.c
index f63fbff09edc3f4ba39946101276c78567545ec4..6b12cd1970b111c3a38debcd6e93906496ac548e 100644 (file)
--- a/mac.c
+++ b/mac.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: mac.c,v 1.32 2015/01/15 18:32:54 naddy Exp $ */
+/* $OpenBSD: mac.c,v 1.33 2016/07/08 03:44:42 djm Exp $ */
 /*
  * Copyright (c) 2001 Markus Friedl.  All rights reserved.
  *
@@ -167,7 +167,8 @@ mac_init(struct sshmac *mac)
 }
 
 int
-mac_compute(struct sshmac *mac, u_int32_t seqno, const u_char *data, int datalen,
+mac_compute(struct sshmac *mac, u_int32_t seqno,
+    const u_char *data, int datalen,
     u_char *digest, size_t dlen)
 {
        static union {
@@ -211,6 +212,24 @@ mac_compute(struct sshmac *mac, u_int32_t seqno, const u_char *data, int datalen
        return 0;
 }
 
+int
+mac_check(struct sshmac *mac, u_int32_t seqno,
+    const u_char *data, size_t dlen,
+    const u_char *theirmac, size_t mlen)
+{
+       u_char ourmac[SSH_DIGEST_MAX_LENGTH];
+       int r;
+
+       if (mac->mac_len > mlen)
+               return SSH_ERR_INVALID_ARGUMENT;
+       if ((r = mac_compute(mac, seqno, data, dlen,
+           ourmac, sizeof(ourmac))) != 0)
+               return r;
+       if (timingsafe_bcmp(ourmac, theirmac, mac->mac_len) != 0)
+               return SSH_ERR_MAC_INVALID;
+       return 0;
+}
+
 void
 mac_clear(struct sshmac *mac)
 {

diff --git a/mac.h b/mac.h
index e5f6b84d9ed6db0fcf088bfdb8535dc0b74d9e11..0b119d7a1c3b0e524b356a9014b7f02a9a0422ea 100644 (file)
--- a/mac.h
+++ b/mac.h
@@ -1,4 +1,4 @@
-/* $OpenBSD: mac.h,v 1.9 2015/01/13 19:31:40 markus Exp $ */
+/* $OpenBSD: mac.h,v 1.10 2016/07/08 03:44:42 djm Exp $ */
 /*
  * Copyright (c) 2001 Markus Friedl.  All rights reserved.
  *
@@ -46,6 +46,8 @@ int    mac_setup(struct sshmac *, char *);
 int     mac_init(struct sshmac *);
 int     mac_compute(struct sshmac *, u_int32_t, const u_char *, int,
     u_char *, size_t);
+int     mac_check(struct sshmac *, u_int32_t, const u_char *, size_t,
+    const u_char *, size_t);
 void    mac_clear(struct sshmac *);
 
 #endif /* SSHMAC_H */

diff --git a/packet.c b/packet.c
index 48111bb151d4c4ee58db6aa110312be4cb9131e5..9839c94ddc550c40da9ac64d4a171f120e7e0e76 100644 (file)
--- a/packet.c
+++ b/packet.c
@@ -1,4 +1,4 @@
-/* $OpenBSD: packet.c,v 1.230 2016/03/07 19:02:43 djm Exp $ */
+/* $OpenBSD: packet.c,v 1.231 2016/07/08 03:44:42 djm Exp $ */
 /*
  * Author: Tatu Ylonen <ylo@cs.hut.fi>
  * Copyright (c) 1995 Tatu Ylonen <ylo@cs.hut.fi>, Espoo, Finland
@@ -1690,7 +1690,7 @@ ssh_packet_read_poll2(struct ssh *ssh, u_char *typep, u_int32_t *seqnr_p)
 {
        struct session_state *state = ssh->state;
        u_int padlen, need;
-       u_char *cp, macbuf[SSH_DIGEST_MAX_LENGTH];
+       u_char *cp;
        u_int maclen, aadlen = 0, authlen = 0, block_size;
        struct sshenc *enc   = NULL;
        struct sshmac *mac   = NULL;
@@ -1790,17 +1790,21 @@ ssh_packet_read_poll2(struct ssh *ssh, u_char *typep, u_int32_t *seqnr_p)
         * 'maclen' bytes of message authentication code.
         */
        if (sshbuf_len(state->input) < aadlen + need + authlen + maclen)
-               return 0;
+               return 0; /* packet is incomplete */
 #ifdef PACKET_DEBUG
        fprintf(stderr, "read_poll enc/full: ");
        sshbuf_dump(state->input, stderr);
 #endif
-       /* EtM: compute mac over encrypted input */
+       /* EtM: check mac over encrypted input */
        if (mac && mac->enabled && mac->etm) {
-               if ((r = mac_compute(mac, state->p_read.seqnr,
+               if ((r = mac_check(mac, state->p_read.seqnr,
                    sshbuf_ptr(state->input), aadlen + need,
-                   macbuf, sizeof(macbuf))) != 0)
+                   sshbuf_ptr(state->input) + aadlen + need + authlen,
+                   maclen)) != 0) {
+                       if (r == SSH_ERR_MAC_INVALID)
+                               logit("Corrupted MAC on input.");
                        goto out;
+               }
        }
        if ((r = sshbuf_reserve(state->incoming_packet, aadlen + need,
            &cp)) != 0)
@@ -1810,26 +1814,21 @@ ssh_packet_read_poll2(struct ssh *ssh, u_char *typep, u_int32_t *seqnr_p)
                goto out;
        if ((r = sshbuf_consume(state->input, aadlen + need + authlen)) != 0)
                goto out;
-       /*
-        * compute MAC over seqnr and packet,
-        * increment sequence number for incoming packet
-        */
        if (mac && mac->enabled) {
-               if (!mac->etm)
-                       if ((r = mac_compute(mac, state->p_read.seqnr,
-                           sshbuf_ptr(state->incoming_packet),
-                           sshbuf_len(state->incoming_packet),
-                           macbuf, sizeof(macbuf))) != 0)
+               /* Not EtM: check MAC over cleartext */
+               if (!mac->etm && (r = mac_check(mac, state->p_read.seqnr,
+                   sshbuf_ptr(state->incoming_packet),
+                   sshbuf_len(state->incoming_packet),
+                   sshbuf_ptr(state->input), maclen)) != 0) {
+                       if (r != SSH_ERR_MAC_INVALID)
                                goto out;
-               if (timingsafe_bcmp(macbuf, sshbuf_ptr(state->input),
-                   mac->mac_len) != 0) {
                        logit("Corrupted MAC on input.");
                        if (need > PACKET_MAX_SIZE)
                                return SSH_ERR_INTERNAL_ERROR;
                        return ssh_packet_start_discard(ssh, enc, mac,
                            state->packlen, PACKET_MAX_SIZE - need);
                }
-
+               /* Remove MAC from input buffer */
                DBG(debug("MAC #%d ok", state->p_read.seqnr));
                if ((r = sshbuf_consume(state->input, mac->mac_len)) != 0)
                        goto out;