net: implement CBC processing in constant-time in libssl.

net/third_party/nss/ssl/ssl3con.c

 /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
 /*
  * SSL3 Protocol
  *
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 /* $Id: ssl3con.c,v 1.192 2012/09/28 05:10:25 wtc%google.com Exp $ */
 
 /* TODO(ekr): Implement HelloVerifyRequest on server side. OK for now. */
@@ skipping 1828 matching lines @@
     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
     0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c, 0x5c,
     0x5c, 0x5c, 0x5c, 0x5c
 };
 
 /* Called from: ssl3_SendRecord()
-**              ssl3_HandleRecord()
 ** Caller must already hold the SpecReadLock. (wish we could assert that!)
 */
 static SECStatus
 ssl3_ComputeRecordMAC(
     ssl3CipherSpec *   spec,
     PRBool             useServerMacKey,
     PRBool             isDTLS,
     SSL3ContentType    type,
     SSL3ProtocolVersion version,
     SSL3SequenceNumber seq_num,
@@ skipping 161 matching lines @@
 
     PRINT_BUF(95, (NULL, "frag hash2: result", outbuf, *outLength));
 
     if (rv != SECSuccess) {
         rv = SECFailure;
         ssl_MapLowLevelError(SSL_ERROR_MAC_COMPUTATION_FAILURE);
     }
     return rv;
 }
 
+/* This is a bodge to allow this code to be compiled against older NSS headers
+ * that don't contains the CBC constant-time changes. */
+#ifndef CKM_NSS_HMAC_CONSTANT_TIME
+#define CKM_NSS_HMAC_CONSTANT_TIME (CKM_NSS + 19)
+#define CKM_NSS_SSLV3_MAC_CONSTANT_TIME (CKM_NSS + 20)
+
+typedef struct CK_NSS_MACConstantTimeParams {
+    CK_MECHANISM_TYPE hashAlg;  /* in */
+    CK_ULONG ulBodyTotalLength; /* in */
+    CK_BYTE * pHeader;          /* in */
+    CK_ULONG ulHeaderLength;    /* in */
+} CK_NSS_MACConstantTimeParams;
+#endif

[wtc, 2013/02/04 20:03:25]

I reviewed this block of code and confirmed it is correct.

+
+/* Called from: ssl3_HandleRecord()
+ * Caller must already hold the SpecReadLock. (wish we could assert that!)
+ *
+ * On entry:
+ *   originalLen >= inputLen >= MAC size
+*/
+static SECStatus
+ssl3_ComputeRecordMACConstantTime(
+    ssl3CipherSpec *   spec,
+    PRBool             useServerMacKey,
+    PRBool             isDTLS,
+    SSL3ContentType    type,
+    SSL3ProtocolVersion version,
+    SSL3SequenceNumber seq_num,
+    const SSL3Opaque * input,
+    int                inputLen,
+    int                originalLen,
+    unsigned char *    outbuf,
+    unsigned int *     outLen)
+{
+    CK_MECHANISM_TYPE            macType;
+    CK_NSS_MACConstantTimeParams params;
+    PK11Context *                mac_context;
+    SECItem                      param;
+    SECStatus                    rv;
+    unsigned char                header[13];
+    PK11SymKey *                 key;
+    int                          recordLength;
+
+    PORT_Assert(inputLen >= spec->mac_size);
+    PORT_Assert(originalLen >= inputLen);
+
+    if (spec->bypassCiphers) {
+        /* This function doesn't support PKCS#11 bypass. We fallback on the
+         * non-constant time version. */
+        goto fallback;
+    }
+
+    if (spec->mac_def->mac == mac_null) {
+        *outLen = 0;
+        return SECSuccess;
+    }
+
+    header[0] = (unsigned char)(seq_num.high >> 24);
+    header[1] = (unsigned char)(seq_num.high >> 16);
+    header[2] = (unsigned char)(seq_num.high >>  8);
+    header[3] = (unsigned char)(seq_num.high >>  0);
+    header[4] = (unsigned char)(seq_num.low  >> 24);
+    header[5] = (unsigned char)(seq_num.low  >> 16);
+    header[6] = (unsigned char)(seq_num.low  >>  8);
+    header[7] = (unsigned char)(seq_num.low  >>  0);
+    header[8] = type;
+
+    macType = CKM_NSS_HMAC_CONSTANT_TIME;
+    recordLength = inputLen - spec->mac_size;
+    if (spec->version <= SSL_LIBRARY_VERSION_3_0) {
+        macType = CKM_NSS_SSLV3_MAC_CONSTANT_TIME;
+        header[9] = recordLength >> 8;
+        header[10] = recordLength;
+        params.ulHeaderLength = 11;
+    } else {
+        header[9] = version >> 8;
+        header[10] = version;
+        header[11] = recordLength >> 8;
+        header[12] = recordLength;
+        params.ulHeaderLength = 13;
+    }
+
+    params.hashAlg = spec->mac_def->mmech;
+    params.ulBodyTotalLength = originalLen;
+    params.pHeader = header;
+
+    param.data = (unsigned char*) &params;
+    param.len = sizeof(params);
+    param.type = 0;
+
+    key = spec->server.write_mac_key;
+    if (!useServerMacKey) {
+        key = spec->client.write_mac_key;
+    }
+    mac_context = PK11_CreateContextBySymKey(macType, CKA_SIGN, key, &param);
+    if (mac_context == NULL) {
+        /* Older versions of NSS may not support constant-time MAC. */
+        goto fallback;
+    }
+
+    rv  = PK11_DigestBegin(mac_context);
+    rv |= PK11_DigestOp(mac_context, input, inputLen);
+    rv |= PK11_DigestFinal(mac_context, outbuf, outLen, spec->mac_size);
+    PK11_DestroyContext(mac_context, PR_TRUE);
+
+    PORT_Assert(rv != SECSuccess || *outLen == (unsigned)spec->mac_size);
+
+    if (rv != SECSuccess) {
+        rv = SECFailure;
+        ssl_MapLowLevelError(SSL_ERROR_MAC_COMPUTATION_FAILURE);
+    }
+    return rv;
+
+fallback:
+    /* ssl3_ComputeRecordMAC expects the MAC to have been removed from the
+     * length already. */
+    inputLen -= spec->mac_size;
+    return ssl3_ComputeRecordMAC(spec, useServerMacKey, isDTLS, type,
+                                 version, seq_num, input, inputLen,
+                                 outbuf, outLen);
+}
+
 static PRBool
 ssl3_ClientAuthTokenPresent(sslSessionID *sid) {
     PK11SlotInfo *slot = NULL;
     PRBool isPresent = PR_TRUE;
 
     /* we only care if we are doing client auth */
     /* If NSS_PLATFORM_CLIENT_AUTH is defined and a platformClientKey is being
      * used, u.ssl3.clAuthValid will be false and this function will always
      * return PR_TRUE. */
     if (!sid || !sid->u.ssl3.clAuthValid) {
@@ skipping 7997 matching lines @@
                 break;
             }
         }
     }   /* end loop */
 
     origBuf->len = 0;   /* So ssl3_GatherAppDataRecord will keep looping. */
     buf->buf = NULL;    /* not a leak. */
     return SECSuccess;
 }
 
+/* These macros return the given value with the MSB copied to all the other
+ * bits. They use the fact that arithmetic shift shifts-in the sign bit.
+ * However, this is not ensured by the C standard so you may need to replace
+ * them with something else on odd CPUs. */

[wtc, 2013/02/04 20:03:25]

I researched this signed right-shift issue when working on
your P-256 curve. I found that the behavior is up to the
compiler (CPU support merely makes it easier to implement
arithmetic shift) and in practice all compilers today do an
arithmetic shift.

So in this comment "CPUs" should be replaced by "compilers".

I am sure there is a lot of code that simply assumes an
arithmetic shift.
This also implies a new compiler will have to do an
arithmetic shift in order to compile existing code correctly.

[agl, 2013/02/04 20:55:00]

Done.

+#define DUPLICATE_MSB_TO_ALL(x) ( (unsigned)( (int)(x) >> (sizeof(int)*8-1) ) )
+#define DUPLICATE_MSB_TO_ALL_8(x) ((unsigned char)(DUPLICATE_MSB_TO_ALL(x)))
+
+/* SECStatusToMask returns, in constant time, a mask value of all ones if rv ==
+ * SECSuccess.  Otherwise it returns zero. */
+static unsigned SECStatusToMask(SECStatus rv) {

[wtc, 2013/02/04 20:03:25]

In NSS style, the opening curly brace for a function should be
on its own line. This is best done in the NSS upstream.

[agl, 2013/02/04 20:55:00]

Done. Have uploaded new upstream patches and updated here.

+    unsigned int good;
+    /* rv ^ SECSuccess is zero iff rv == SECSuccess. Subtracting one results in
+     * the MSB being set to one iff it was zero before. */
+    good = rv ^ SECSuccess;
+    good--;
+    return DUPLICATE_MSB_TO_ALL(good);
+}
+
+/* ssl_ConstantTimeGE returns 0xff if a>=b and 0x00 otherwise. */
+static unsigned char ssl_ConstantTimeGE(unsigned a, unsigned b) {
+    a -= b;
+    return DUPLICATE_MSB_TO_ALL(~a);
+}
+
+/* ssl_ConstantTimeEQ8 returns 0xff if a==b and 0x00 otherwise. */
+static unsigned char ssl_ConstantTimeEQ8(unsigned char a, unsigned char b) {
+    unsigned c = a ^ b;
+    c--;
+    return DUPLICATE_MSB_TO_ALL_8(c);
+}
+
+static SECStatus ssl_RemoveSSLv3CBCPadding(sslBuffer *plaintext,
+                                           unsigned blockSize,
+                                           unsigned macSize) {
+    unsigned int paddingLength, good, t;
+    const unsigned int overhead = 1 /* padding length byte */ + macSize;
+
+    /* These lengths are all public so we can test them in non-constant
+     * time. */
+    if (overhead > plaintext->len) {
+        return SECFailure;
+    }
+
+    paddingLength = plaintext->buf[plaintext->len-1];
+    /* SSLv3 padding bytes are random and cannot be checked. */
+    t = plaintext->len;
+    t -= paddingLength+overhead;
+    /* If len >= padding_length+overhead then the MSB of t is zero. */
+    good = DUPLICATE_MSB_TO_ALL(~t);
+    /* SSLv3 requires that the padding is minimal. */
+    t = blockSize - (paddingLength+1);
+    good &= DUPLICATE_MSB_TO_ALL(~t);
+    plaintext->len -= good & (paddingLength+1);
+    return (good & SECSuccess) | (~good & SECFailure);
+}
+
+
+static SECStatus ssl_RemoveTLSCBCPadding(sslBuffer *plaintext,
+                                         unsigned macSize) {
+    unsigned int paddingLength, good, t, toCheck, i;
+    const unsigned int overhead = 1 /* padding length byte */ + macSize;
+
+    /* These lengths are all public so we can test them in non-constant
+     * time. */
+    if (overhead > plaintext->len) {
+        return SECFailure;
+    }
+
+    paddingLength = plaintext->buf[plaintext->len-1];
+    t = plaintext->len;
+    t -= paddingLength+overhead;
+    /* If len >= paddingLength+overhead then the MSB of t is zero. */
+    good = DUPLICATE_MSB_TO_ALL(~t);
+
+    /* The padding consists of a length byte at the end of the record and then
+     * that many bytes of padding, all with the same value as the length byte.
+     * Thus, with the length byte included, there are paddingLength+1 bytes of
+     * padding.
+     *
+     * We can't check just |paddingLength+1| bytes because that leaks
+     * decrypted information. Therefore we always have to check the maximum
+     * amount of padding possible. (Again, the length of the record is
+     * public information so we can use it.) */
+    toCheck = 255; /* maximum amount of padding. */
+    if (toCheck > plaintext->len-1) {
+        toCheck = plaintext->len-1;
+    }
+
+    for (i = 0; i < toCheck; i++) {
+        unsigned int t = paddingLength - i;
+        /* If i <= paddingLength then the MSB of t is zero and mask is
+         * 0xff.  Otherwise, mask is 0. */
+        unsigned char mask = DUPLICATE_MSB_TO_ALL(~t);
+        unsigned char b = plaintext->buf[plaintext->len-1-i];
+        /* The final |paddingLength+1| bytes should all have the value
+         * |paddingLength|. Therefore the XOR should be zero. */
+        good &= ~(mask&(paddingLength ^ b));
+    }
+
+    /* If any of the final |paddingLength+1| bytes had the wrong value,
+     * one or more of the lower eight bits of |good| will be cleared. We
+     * AND the bottom 8 bits together and duplicate the result to all the
+     * bits. */
+    good &= good >> 4;
+    good &= good >> 2;
+    good &= good >> 1;
+    good <<= sizeof(good)*8-1;
+    good = DUPLICATE_MSB_TO_ALL(good);
+
+    plaintext->len -= good & (paddingLength+1);
+    return (good & SECSuccess) | (~good & SECFailure);
+}
+
+/* On entry:
+ *   originalLength >= macSize
+ *   macSize <= MAX_MAC_LENGTH
+ *   plaintext->len >= macSize
+ */
+static void ssl_CBCExtractMAC(sslBuffer *plaintext,
+                              unsigned int originalLength,
+                              SSL3Opaque* out,
+                              unsigned int macSize) {
+    unsigned char rotatedMac[MAX_MAC_LENGTH];
+    /* macEnd is the index of |plaintext->buf| just after the end of the MAC. */
+    unsigned macEnd = plaintext->len;
+    unsigned macStart = macEnd - macSize;
+    /* scanStart contains the number of bytes that we can ignore because
+     * the MAC's position can only vary by 255 bytes. */
+    unsigned scanStart = 0;
+    unsigned i, j, divSpoiler;
+    unsigned char rotateOffset;
+
+    if (originalLength > macSize + 255 + 1)
+        scanStart = originalLength - (macSize + 255 + 1);
+
+    /* divSpoiler contains a multiple of macSize that is used to cause the
+     * modulo operation to be constant time. Without this, the time varies
+     * based on the amount of padding when running on Intel chips at least.
+     *
+     * The aim of right-shifting macSize is so that the compiler doesn't
+     * figure out that it can remove divSpoiler as that would require it
+     * to prove that macSize is always even, which I hope is beyond it. */
+    divSpoiler = macSize >> 1;
+    divSpoiler <<= (sizeof(divSpoiler)-1)*8;
+    rotateOffset = (divSpoiler + macStart - scanStart) % macSize;
+
+    memset(rotatedMac, 0, macSize);
+    for (i = scanStart; i < originalLength;) {
+        for (j = 0; j < macSize && i < originalLength; i++, j++) {
+            unsigned char macStarted = ssl_ConstantTimeGE(i, macStart);
+            unsigned char macEnded = ssl_ConstantTimeGE(i, macEnd);
+            unsigned char b = 0;
+            b = plaintext->buf[i];
+            rotatedMac[j] |= b & macStarted & ~macEnded;
+        }
+    }
+
+    /* Now rotate the MAC. If we knew that the MAC fit into a CPU cache line we
+     * could line-align |rotatedMac| and rotate in place. */
+    memset(out, 0, macSize);
+    for (i = 0; i < macSize; i++) {
+        unsigned char offset = (divSpoiler + macSize - rotateOffset + i) % macSize;
+        for (j = 0; j < macSize; j++) {
+            out[j] |= rotatedMac[i] & ssl_ConstantTimeEQ8(j, offset);
+        }
+    }
+}
+
 /* if cText is non-null, then decipher, check MAC, and decompress the
  * SSL record from cText->buf (typically gs->inbuf)
  * into databuf (typically gs->buf), and any previous contents of databuf
  * is lost.  Then handle databuf according to its SSL record type,
  * unless it's an application record.
  *
  * If cText is NULL, then the ciphertext has previously been deciphered and
  * checked, and is already sitting in databuf.  It is processed as an SSL
  * Handshake message.
  *
  * DOES NOT process the decrypted/decompressed application data.
  * On return, databuf contains the decrypted/decompressed record.
  *
  * Called from ssl3_GatherCompleteHandshake
  *             ssl3_RestartHandshakeAfterCertReq
  *
  * Caller must hold the RecvBufLock.
  *
  * This function aquires and releases the SSL3Handshake Lock, holding the
  * lock around any calls to functions that handle records other than
  * Application Data records.
  */
 SECStatus
 ssl3_HandleRecord(sslSocket *ss, SSL3Ciphertext *cText, sslBuffer *databuf)
 {
     const ssl3BulkCipherDef *cipher_def;
     ssl3CipherSpec *     crSpec;
     SECStatus            rv;
     unsigned int         hashBytes = MAX_MAC_LENGTH + 1;
-    unsigned int         padding_length;
     PRBool               isTLS;
-    PRBool               padIsBad = PR_FALSE;
     SSL3ContentType      rType;
     SSL3Opaque           hash[MAX_MAC_LENGTH];
+    SSL3Opaque           givenHashBuf[MAX_MAC_LENGTH];
+    SSL3Opaque          *givenHash;
     sslBuffer           *plaintext;
     sslBuffer            temp_buf;
     PRUint64             dtls_seq_num;
     unsigned int         ivLen = 0;
+    unsigned int         originalLen = 0;
+    unsigned int         good;
+    unsigned int         minLength;
 
     PORT_Assert( ss->opt.noLocks || ssl_HaveRecvBufLock(ss) );
 
     if (!ss->ssl3.initialized) {
         ssl_GetSSL3HandshakeLock(ss);
         rv = ssl3_InitState(ss);
         ssl_ReleaseSSL3HandshakeLock(ss);
         if (rv != SECSuccess) {
             return rv;          /* ssl3_InitState has set the error code. */
         }
@@ skipping 47 matching lines @@
         if (dtls_RecordGetRecvd(&crSpec->recvdRecords, dtls_seq_num) != 0) {
             ssl_ReleaseSpecReadLock(ss);
             SSL_DBG(("%d: SSL3[%d]: HandleRecord, rejecting "
                      "potentially replayed packet", SSL_GETPID(), ss->fd));
             /* Silently drop the packet */
             databuf->len = 0; /* Needed to ensure data not left around */
             return SECSuccess;
         }
     }
 
+    good = (unsigned)-1;
+    minLength = crSpec->mac_size;
+    if (cipher_def->type == type_block) {
+        /* CBC records have a padding length byte at the end. */
+        minLength++;
+        if (crSpec->version >= SSL_LIBRARY_VERSION_TLS_1_1) {
+            /* With >= TLS 1.1, CBC records have an explicit IV. */
+            minLength += cipher_def->iv_size;
+        }
+    }
+
+    /* We can perform this test in variable time because the record's total
+     * length and the ciphersuite are both public knowledge. */
+    if (cText->buf->len < minLength) {
+        SSL_DBG(("%d: SSL3[%d]: HandleRecord, record too small.",
+                 SSL_GETPID(), ss->fd));
+        /* must not hold spec lock when calling SSL3_SendAlert. */
+        ssl_ReleaseSpecReadLock(ss);
+        SSL3_SendAlert(ss, alert_fatal, bad_record_mac);
+        /* always log mac error, in case attacker can read server logs. */
+        PORT_SetError(SSL_ERROR_BAD_MAC_READ);
+        return SECFailure;
+    }
+
     if (cipher_def->type == type_block &&
         crSpec->version >= SSL_LIBRARY_VERSION_TLS_1_1) {
         /* Consume the per-record explicit IV. RFC 4346 Section 6.2.3.2 states
          * "The receiver decrypts the entire GenericBlockCipher structure and
          * then discards the first cipher block corresponding to the IV
          * component." Instead, we decrypt the first cipher block and then
          * discard it before decrypting the rest.
          */
         SSL3Opaque iv[MAX_IV_LENGTH];
         int decoded;
 
         ivLen = cipher_def->iv_size;
         if (ivLen < 8 || ivLen > sizeof(iv)) {
             ssl_ReleaseSpecReadLock(ss);
             PORT_SetError(SEC_ERROR_LIBRARY_FAILURE);
             return SECFailure;
         }
-        if (ivLen > cText->buf->len) {
-            SSL_DBG(("%d: SSL3[%d]: HandleRecord, IV length check failed",
-                     SSL_GETPID(), ss->fd));
-            /* must not hold spec lock when calling SSL3_SendAlert. */
-            ssl_ReleaseSpecReadLock(ss);
-            SSL3_SendAlert(ss, alert_fatal, bad_record_mac);
-            /* always log mac error, in case attacker can read server logs. */
-            PORT_SetError(SSL_ERROR_BAD_MAC_READ);
-            return SECFailure;
-        }
 
         PRINT_BUF(80, (ss, "IV (ciphertext):", cText->buf->buf, ivLen));
 
         /* The decryption result is garbage, but since we just throw away
          * the block it doesn't matter.  The decryption of the next block
          * depends only on the ciphertext of the IV block.
          */
         rv = crSpec->decode(crSpec->decodeContext, iv, &decoded,
                             sizeof(iv), cText->buf->buf, ivLen);
 
-        if (rv != SECSuccess) {
-            /* All decryption failures must be treated like a bad record
-             * MAC; see RFC 5246 (TLS 1.2). 
-             */
-            padIsBad = PR_TRUE;
-        }
+        good &= SECStatusToMask(rv);
     }
 
     /* If we will be decompressing the buffer we need to decrypt somewhere
      * other than into databuf */
     if (crSpec->decompressor) {
         temp_buf.buf = NULL;
         temp_buf.space = 0;
         plaintext = &temp_buf;
     } else {
         plaintext = databuf;
@@ skipping 21 matching lines @@
         ssl_ReleaseSpecReadLock(ss);
         SSL3_SendAlert(ss, alert_fatal, record_overflow);
         PORT_SetError(SSL_ERROR_RX_RECORD_TOO_LONG);
         return SECFailure;
     }
 
     /* decrypt from cText buf to plaintext. */
     rv = crSpec->decode(
         crSpec->decodeContext, plaintext->buf, (int *)&plaintext->len,
         plaintext->space, cText->buf->buf + ivLen, cText->buf->len - ivLen);
+    good &= SECStatusToMask(rv);
 
     PRINT_BUF(80, (ss, "cleartext:", plaintext->buf, plaintext->len));
-    if (rv != SECSuccess) {
-        /* All decryption failures must be treated like a bad record
-         * MAC; see RFC 5246 (TLS 1.2). 
-         */
-        padIsBad = PR_TRUE;
-    }
+
+    originalLen = plaintext->len;
 
     /* If it's a block cipher, check and strip the padding. */
-    if (cipher_def->type == type_block && !padIsBad) {
-        PRUint8 * pPaddingLen = plaintext->buf + plaintext->len - 1;
-        padding_length = *pPaddingLen;
-        /* TLS permits padding to exceed the block size, up to 255 bytes. */
-        if (padding_length + 1 + crSpec->mac_size > plaintext->len)
-            padIsBad = PR_TRUE;
-        else {
-            plaintext->len -= padding_length + 1;
-            /* In TLS all padding bytes must be equal to the padding length. */
-            if (isTLS) {
-                PRUint8 *p;
-                for (p = pPaddingLen - padding_length; p < pPaddingLen; ++p) {
-                    padIsBad |= *p ^ padding_length;
-                }
-            }
-        }
+    if (cipher_def->type == type_block) {
+        const unsigned int blockSize = cipher_def->iv_size;
+        const unsigned int macSize = crSpec->mac_size;
+
+        if (crSpec->version <= SSL_LIBRARY_VERSION_3_0) {
+            good &= SECStatusToMask(ssl_RemoveSSLv3CBCPadding(
+                        plaintext, blockSize, macSize));
+        } else {
+            good &= SECStatusToMask(ssl_RemoveTLSCBCPadding(
+                        plaintext, macSize));
+        }
     }
 
-    /* Remove the MAC. */
-    if (plaintext->len >= crSpec->mac_size)
-        plaintext->len -= crSpec->mac_size;
-    else
-        padIsBad = PR_TRUE;     /* really macIsBad */
-
     /* compute the MAC */
     rType = cText->type;
-    rv = ssl3_ComputeRecordMAC( crSpec, (PRBool)(!ss->sec.isServer),
-        IS_DTLS(ss), rType, cText->version,
-        IS_DTLS(ss) ? cText->seq_num : crSpec->read_seq_num,
-        plaintext->buf, plaintext->len, hash, &hashBytes);
-    if (rv != SECSuccess) {
-        padIsBad = PR_TRUE;     /* really macIsBad */
+    if (cipher_def->type == type_block) {
+        rv = ssl3_ComputeRecordMACConstantTime(
+            crSpec, (PRBool)(!ss->sec.isServer),
+            IS_DTLS(ss), rType, cText->version,
+            IS_DTLS(ss) ? cText->seq_num : crSpec->read_seq_num,
+            plaintext->buf, plaintext->len, originalLen,
+            hash, &hashBytes);
+
+        ssl_CBCExtractMAC(plaintext, originalLen, givenHashBuf,
+                          crSpec->mac_size);
+        givenHash = givenHashBuf;
+
+        /* plaintext->len will always have enough space to remove the MAC
+         * because in ssl_Remove{SSLv3|TLS}CBCPadding we only adjust
+         * plaintext->len if the result has enough space for the MAC and we
+         * tested the unadjusted size against minLength, above. */
+        plaintext->len -= crSpec->mac_size;
+    } else {
+        /* This is safe because we checked the minLength above. */
+        plaintext->len -= crSpec->mac_size;
+
+        rv = ssl3_ComputeRecordMAC(
+            crSpec, (PRBool)(!ss->sec.isServer),
+            IS_DTLS(ss), rType, cText->version,
+            IS_DTLS(ss) ? cText->seq_num : crSpec->read_seq_num,
+            plaintext->buf, plaintext->len,
+            hash, &hashBytes);
+
+        /* We can read the MAC directly from the record because its location is
+         * public when a stream cipher is used. */
+        givenHash = plaintext->buf + plaintext->len;
     }
 
-    /* Check the MAC */
-    if (hashBytes != (unsigned)crSpec->mac_size || padIsBad || 
-        NSS_SecureMemcmp(plaintext->buf + plaintext->len, hash,
-                         crSpec->mac_size) != 0) {
+    good &= SECStatusToMask(rv);
+
+    if (hashBytes != (unsigned)crSpec->mac_size ||
+        NSS_SecureMemcmp(givenHash, hash, crSpec->mac_size) != 0) {
+        /* We're allowed to leak whether or not the MAC check was correct */
+        good = 0;
+    }
+
+    if (good == 0) {
         /* must not hold spec lock when calling SSL3_SendAlert. */
         ssl_ReleaseSpecReadLock(ss);
 
         SSL_DBG(("%d: SSL3[%d]: mac check failed", SSL_GETPID(), ss->fd));
 
         if (!IS_DTLS(ss)) {
             SSL3_SendAlert(ss, alert_fatal, bad_record_mac);
             /* always log mac error, in case attacker can read server logs. */
             PORT_SetError(SSL_ERROR_BAD_MAC_READ);
             return SECFailure;
@@ skipping 639 matching lines @@
             PORT_Free(ss->ssl3.hs.recvdFragments.buf);
         }
     }
 
     ss->ssl3.initialized = PR_FALSE;
 
     SECITEM_FreeItem(&ss->ssl3.nextProto, PR_FALSE);
 }
 
 /* End of ssl3con.c */