Use fast HPACK comparisons when not checking sensitive headers (#9259)

Motivation:
Constant time comparison functions are used to compare HTTP/2 header
values, even if they are not sensitive.

Modification:
After checking for sensitivity, use fast comparison.

Result: Faster HPACK table reads/writes

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java

@@ -41,6 +41,7 @@ import java.util.Arrays;
 import java.util.Map;
 
 import static io.netty.handler.codec.http2.HpackUtil.equalsConstantTime;
+import static io.netty.handler.codec.http2.HpackUtil.equalsVariableTime;
 import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_HEADER_TABLE_SIZE;
 import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_HEADER_LIST_SIZE;
 import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_HEADER_TABLE_SIZE;
@@ -53,6 +54,13 @@ import static io.netty.util.internal.MathUtil.findNextPositivePowerOfTwo;
 import static java.lang.Math.max;
 import static java.lang.Math.min;
 
+/**
+ * An HPACK encoder.
+ *
+ * <p>Implementation note:  This class is security sensitive, and depends on users correctly identifying their headers
+ * as security sensitive or not.  If a header is considered not sensitive, methods names "insensitive" are used which
+ * are fast, but don't provide any security guarantees.
+ */
 final class HpackEncoder {
     static final int HUFF_CODE_THRESHOLD = 512;
     // a linked hash map of header fields
@@ -153,7 +161,7 @@ final class HpackEncoder {
 
         // If the peer will only use the static table
         if (maxHeaderTableSize == 0) {
-            int staticTableIndex = HpackStaticTable.getIndex(name, value);
+            int staticTableIndex = HpackStaticTable.getIndexInsensitive(name, value);
             if (staticTableIndex == -1) {
                 int nameIndex = HpackStaticTable.getIndex(name);
                 encodeLiteral(out, name, value, IndexType.NONE, nameIndex);
@@ -170,13 +178,13 @@ final class HpackEncoder {
             return;
         }
 
-        HeaderEntry headerField = getEntry(name, value);
+        HeaderEntry headerField = getEntryInsensitive(name, value);
         if (headerField != null) {
             int index = getIndex(headerField.index) + HpackStaticTable.length;
             // Section 6.1. Indexed Header Field Representation
             encodeInteger(out, 0x80, 7, index);
         } else {
-            int staticTableIndex = HpackStaticTable.getIndex(name, value);
+            int staticTableIndex = HpackStaticTable.getIndexInsensitive(name, value);
             if (staticTableIndex != -1) {
                 // Section 6.1. Indexed Header Field Representation
                 encodeInteger(out, 0x80, 7, staticTableIndex);
@@ -351,15 +359,16 @@ final class HpackEncoder {
      * Returns the header entry with the lowest index value for the header field. Returns null if
      * header field is not in the dynamic table.
      */
-    private HeaderEntry getEntry(CharSequence name, CharSequence value) {
+    private HeaderEntry getEntryInsensitive(CharSequence name, CharSequence value) {
         if (length() == 0 || name == null || value == null) {
             return null;
         }
         int h = AsciiString.hashCode(name);
         int i = index(h);
         for (HeaderEntry e = headerFields[i]; e != null; e = e.next) {
-            // To avoid short circuit behavior a bitwise operator is used instead of a boolean operator.
+            // Check the value before then name, as it is more likely the value will be different incase there is no
-            if (e.hash == h && (equalsConstantTime(name, e.name) & equalsConstantTime(value, e.value)) != 0) {
+            // match.
+            if (e.hash == h && equalsVariableTime(value, e.value) && equalsVariableTime(name, e.name)) {
                 return e;
             }
         }

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHeaderField.java

@@ -31,6 +31,7 @@
  */
 package io.netty.handler.codec.http2;
 
+import static io.netty.handler.codec.http2.HpackUtil.equalsVariableTime;
 import static io.netty.util.internal.ObjectUtil.checkNotNull;
 
 class HpackHeaderField {
@@ -57,23 +58,8 @@ class HpackHeaderField {
         return name.length() + value.length() + HEADER_ENTRY_OVERHEAD;
     }
 
-    @Override
+    public final boolean equalsForTest(HpackHeaderField other) {
-    public final int hashCode() {
+        return equalsVariableTime(name, other.name) && equalsVariableTime(value, other.value);
-        // TODO(nmittler): Netty's build rules require this. Probably need a better implementation.
-        return super.hashCode();
-    }
-
-    @Override
-    public final boolean equals(Object obj) {
-        if (obj == this) {
-            return true;
-        }
-        if (!(obj instanceof HpackHeaderField)) {
-            return false;
-        }
-        HpackHeaderField other = (HpackHeaderField) obj;
-        // To avoid short circuit behavior a bitwise operator is used instead of a boolean operator.
-        return (HpackUtil.equalsConstantTime(name, other.name) & HpackUtil.equalsConstantTime(value, other.value)) != 0;
     }
 
     @Override

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java

@@ -38,6 +38,7 @@ import java.util.Arrays;
 import java.util.List;
 
 import static io.netty.handler.codec.http2.HpackUtil.equalsConstantTime;
+import static io.netty.handler.codec.http2.HpackUtil.equalsVariableTime;
 
 final class HpackStaticTable {
 
@@ -145,7 +146,7 @@ final class HpackStaticTable {
      * Returns the index value for the given header field in the static table. Returns -1 if the
      * header field is not in the static table.
      */
-    static int getIndex(CharSequence name, CharSequence value) {
+    static int getIndexInsensitive(CharSequence name, CharSequence value) {
         int index = getIndex(name);
         if (index == -1) {
             return -1;
@@ -154,10 +155,7 @@ final class HpackStaticTable {
         // Note this assumes all entries for a given header field are sequential.
         while (index <= length) {
             HpackHeaderField entry = getEntry(index);
-            if (equalsConstantTime(name, entry.name) == 0) {
+            if (equalsVariableTime(name, entry.name) && equalsVariableTime(value, entry.value)) {
-                break;
-            }
-            if (equalsConstantTime(value, entry.value) != 0) {
                 return index;
             }
             index++;

codec-http2/src/main/java/io/netty/handler/codec/http2/HpackUtil.java

@@ -65,6 +65,16 @@ final class HpackUtil {
         return ConstantTimeUtils.equalsConstantTime(s1, s2);
     }
 
+    /**
+     * Compare two {@link CharSequence}s.
+     * @param s1 the first value.
+     * @param s2 the second value.
+     * @return {@code false} if not equal. {@code true} if equal.
+     */
+    static boolean equalsVariableTime(CharSequence s1, CharSequence s2) {
+        return AsciiString.contentEquals(s1, s2);
+    }
+
     // Section 6.2. Literal Header Field Representation
     enum IndexType {
         INCREMENTAL, // Section 6.2.1. Literal Header Field with Incremental Indexing

codec-http2/src/test/java/io/netty/handler/codec/http2/HpackTestCase.java

@@ -102,7 +102,7 @@ final class HpackTestCase {
 
             List<HpackHeaderField> expectedDynamicTable = headerBlock.getDynamicTable();
 
-            if (!expectedDynamicTable.equals(actualDynamicTable)) {
+            if (!headersEqual(expectedDynamicTable, actualDynamicTable)) {
                 throw new AssertionError(
                         "\nEXPECTED DYNAMIC TABLE:\n" + expectedDynamicTable +
                                 "\nACTUAL DYNAMIC TABLE:\n" + actualDynamicTable);
@@ -128,7 +128,7 @@ final class HpackTestCase {
                 expectedHeaders.add(new HpackHeaderField(h.name, h.value));
             }
 
-            if (!expectedHeaders.equals(actualHeaders)) {
+            if (!headersEqual(expectedHeaders, actualHeaders)) {
                 throw new AssertionError(
                         "\nEXPECTED:\n" + expectedHeaders +
                                 "\nACTUAL:\n" + actualHeaders);
@@ -141,7 +141,7 @@ final class HpackTestCase {
 
             List<HpackHeaderField> expectedDynamicTable = headerBlock.getDynamicTable();
 
-            if (!expectedDynamicTable.equals(actualDynamicTable)) {
+            if (!headersEqual(expectedDynamicTable, actualDynamicTable)) {
                 throw new AssertionError(
                         "\nEXPECTED DYNAMIC TABLE:\n" + expectedDynamicTable +
                                 "\nACTUAL DYNAMIC TABLE:\n" + actualDynamicTable);
@@ -229,6 +229,18 @@ final class HpackTestCase {
         return ret.toString();
     }
 
+    private static boolean headersEqual(List<HpackHeaderField> expected, List<HpackHeaderField> actual) {
+        if (expected.size() != actual.size()) {
+            return false;
+        }
+        for (int i = 0; i < expected.size(); i++) {
+            if (!expected.get(i).equalsForTest(actual.get(i))) {
+                return false;
+            }
+        }
+        return true;
+    }
+
     static class HeaderBlock {
         private int maxHeaderTableSize = -1;
         private byte[] encodedBytes;