Bug 1293334 - fix spoiler, r=wladd,rbarnes; backport to 3.21, r=franziskus NSS_3_21_BRANCH
authorFranziskus Kiefer <franziskuskiefer@gmail.com>
Fri, 23 Sep 2016 11:19:24 +0200
branchNSS_3_21_BRANCH
changeset 12596 6883f1fc912908d3767a147e8df803a3b11bde28
parent 12595 48ee7d5ef3da45f0bbb2e5814323531b3ca0c635
child 12597 6ca6047461825fea7538d4de357cd42f3801e2f0
push id1580
push userkaie@kuix.de
push dateFri, 23 Sep 2016 09:20:19 +0000
reviewerswladd, rbarnes, backport, franziskus
bugs1293334
Bug 1293334 - fix spoiler, r=wladd,rbarnes; backport to 3.21, r=franziskus
coreconf/Werror.mk
lib/ssl/ssl3con.c
--- a/coreconf/Werror.mk
+++ b/coreconf/Werror.mk
@@ -55,16 +55,16 @@ ifndef WARNING_CFLAGS
           $(warning Unable to find gcc 4.8 or greater, disabling -Werror)
           NSS_ENABLE_WERROR = 0
         endif
       endif
     endif
   endif #ndef NSS_ENABLE_WERROR
 
   ifeq ($(NSS_ENABLE_WERROR),1)
-    WARNING_CFLAGS += -Werror
+    WARNING_CFLAGS += -Werror -Wno-error=misleading-indentation
   else
     # Old versions of gcc (< 4.8) don't support #pragma diagnostic in functions.
     # Use this to disable use of that #pragma and the warnings it suppresses.
     WARNING_CFLAGS += -DNSS_NO_GCC48
   endif
   export WARNING_CFLAGS
 endif # ndef WARNING_CFLAGS
--- a/lib/ssl/ssl3con.c
+++ b/lib/ssl/ssl3con.c
@@ -11818,16 +11818,23 @@ ssl_ConstantTimeGE(unsigned int a, unsig
 static unsigned char
 ssl_ConstantTimeEQ8(unsigned char a, unsigned char b)
 {
     unsigned int c = a ^ b;
     c--;
     return DUPLICATE_MSB_TO_ALL_8(c);
 }
 
+/* ssl_constantTimeSelect return a if mask is 0xFF and b if mask is 0x00 */
+static unsigned char
+ssl_constantTimeSelect(unsigned char mask, unsigned char a, unsigned char b)
+{
+    return (mask & a) | (~mask & b);
+}
+
 static SECStatus
 ssl_RemoveSSLv3CBCPadding(sslBuffer *plaintext,
 			  unsigned int blockSize,
 			  unsigned int macSize)
 {
     unsigned int paddingLength, good, t;
     const unsigned int overhead = 1 /* padding length byte */ + macSize;
 
@@ -11921,53 +11928,89 @@ ssl_CBCExtractMAC(sslBuffer *plaintext,
     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 i, j;
     unsigned char rotateOffset;
 
-    if (originalLength > macSize + 255 + 1)
+    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;
+    }
+
+    /* We want to compute
+     * rotateOffset = (macStart - scanStart) % macSize
+     * But the time to compute this varies based on the amount of padding. Thus
+     * we explicitely handle all mac sizes with (hopefully) constant time modulo
+     * using Barrett reduction:
+     *  q := (rotateOffset * m) >> k
+     *  rotateOffset -= q * n
+     *  if (n <= rotateOffset) rotateOffset -= n
+     */
+    rotateOffset = macStart - scanStart;
+    /* rotateOffset < 255 + 1 + 48 = 304 */
+    if (macSize == 16) {
+        rotateOffset &= 15;
+    } else if (macSize == 20) {
+        /*
+         * Correctness: rotateOffset * ( 1/20 - 25/2^9 ) < 1
+         *              with rotateOffset <= 853
+         */
+        unsigned q = (rotateOffset * 25) >> 9; /* m = 25, k = 9 */
+        rotateOffset -= q * 20;
+        rotateOffset -= ssl_constantTimeSelect(ssl_ConstantTimeGE(rotateOffset, 20),
+                                               20, 0);
+    } else if (macSize == 32) {
+        rotateOffset &= 31;
+    } else if (macSize == 48) {
+        /*
+         * Correctness: rotateOffset * ( 1/48 - 10/2^9 ) < 1
+         *              with rotateOffset < 768
+         */
+        unsigned q = (rotateOffset * 10) >> 9; /* m = 25, k = 9 */
+        rotateOffset -= q * 48;
+        rotateOffset -= ssl_constantTimeSelect(ssl_ConstantTimeGE(rotateOffset, 48),
+                                               48, 0);
+    } else {
+        /*
+         * SHA384 (macSize == 48) is the largest we support. We should never
+         * get here.
+         */
+        PORT_Assert(0);
+        rotateOffset = rotateOffset % 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);
+    rotateOffset = macSize - rotateOffset;
+    rotateOffset = ssl_constantTimeSelect(ssl_ConstantTimeGE(rotateOffset, macSize),
+                                          0, rotateOffset);
     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);
-	}
+            out[j] |= rotatedMac[i] & ssl_ConstantTimeEQ8(j, rotateOffset);
+        }
+        rotateOffset++;
+        rotateOffset = ssl_constantTimeSelect(ssl_ConstantTimeGE(rotateOffset, macSize),
+                                              0, rotateOffset);
     }
 }
 
 /* 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.