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 */

[wtc, 2013/02/05 16:45:48]

I'm going to suggest three naming changes here to make them
more consistent with the naming convention of PKCS #11,
in the hope that these will be eventually adopted by
PKCS #11.

1. CKM_NSS_SSLV3 => CKM_NSS_SSL3

This is because the existing SSLv3 PKCS #11 mechanisms
are all named CKM_SSL3_xxx:

http://mxr.mozilla.org/security/search?string=CKM_.*SSL.*&regexp=1&case=1&fin...

2. CK_NSS_MACConstantTimeParams => CK_NSS_MAC_CONSTANT_TIME_PARAMS

This is the naming convention of mechanism parameter structs
in PKCS #11.

http://mxr.mozilla.org/security/search?string=CK_.*PARAMS&regexp=1&case=1&fin...

3. ulBodyTotalLength => ulBodyTotalLen
   ulHeaderLength => ulHeaderLen

This is because the CK_ULONG length fields are all named
ulFooLen, except two:

http://mxr.mozilla.org/security/search?string=ul.*Len%3B&regexp=1&case=1&find...

vs.

http://mxr.mozilla.org/security/search?string=ul.*Length%3B&regexp=1&case=1&f...

If you agree with these coding style changes, please go
ahead and make the changes in this CL. Thanks.

[agl, 2013/02/05 21:44:18]

I have applied all your suggested naming changes.

+} CK_NSS_MACConstantTimeParams;
+#endif
+
+/* 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;

[wtc, 2013/02/05 16:45:48]

IMPORTANT: please port my isDTLS change from
https://bugzilla.mozilla.org/attachment.cgi?oldid=710008&action=interdiff&new...

It would be nice to ask juberti or ronghuawu to test
this patch with WebRTC, if the WebRTC code is already
enabled in Chromium.

[agl, 2013/02/05 21:44:18]

Done.
I've emailed them to ask whether there are tests in Chrome already.

+        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. */

[wtc, 2013/02/05 16:45:48]

I think this code is also appropriate for NSS upstream, because
sometimes we want to test with an older version of the
NSS softoken.

It seems that we should call PK11_CreateContextBySymKey
earlier in this function so that if we do need to fall
back, we can skip the code to set up |params|.

In the upstream NSS patch, you are using the new
PK11_SignWithSymKey function. Why don't you use that
function here?

[agl, 2013/02/05 21:44:18]

That's true, although it's only local memory access and so probably on the order
of a few dozen cycles. I haven't made this change here yet, although you could
upstream if you wish and we would eventually sync up.
PK11_SignWithSymKey is a new function, therefore we would have to link against
an NSS that provided it. By expanding PK11_SignWithSymKey here, we can continue
to link against older versions of NSS.

+        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 for odd compilers. */
+#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)
+{
+    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 */