diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java index a1b25179c1..5dab8f836e 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackEncoder.java +++ b/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. + * + *

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. - if (e.hash == h && (equalsConstantTime(name, e.name) & equalsConstantTime(value, e.value)) != 0) { + // Check the value before then name, as it is more likely the value will be different incase there is no + // match. + if (e.hash == h && equalsVariableTime(value, e.value) && equalsVariableTime(name, e.name)) { return e; } } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHeaderField.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHeaderField.java index c1997ba444..dfdcd22b84 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackHeaderField.java +++ b/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 java.util.Objects.requireNonNull; class HpackHeaderField { @@ -57,23 +58,8 @@ class HpackHeaderField { return name.length() + value.length() + HEADER_ENTRY_OVERHEAD; } - @Override - public final int hashCode() { - // 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; + public final boolean equalsForTest(HpackHeaderField other) { + return equalsVariableTime(name, other.name) && equalsVariableTime(value, other.value); } @Override diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java index 068ba0bfd9..47e884cebc 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackStaticTable.java +++ b/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) { - break; - } - if (equalsConstantTime(value, entry.value) != 0) { + if (equalsVariableTime(name, entry.name) && equalsVariableTime(value, entry.value)) { return index; } index++; diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackUtil.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackUtil.java index 62c24aa280..d0f0da0c28 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackUtil.java +++ b/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 diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackTestCase.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackTestCase.java index 44f96c9d3a..bcbe716999 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackTestCase.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackTestCase.java @@ -102,7 +102,7 @@ final class HpackTestCase { List 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 expectedDynamicTable = headerBlock.getDynamicTable(); - if (!expectedDynamicTable.equals(actualDynamicTable)) { + if (!headersEqual(expectedDynamicTable, actualDynamicTable)) { throw new AssertionError( "\nEXPECTED DYNAMIC TABLE:\n" + expectedDynamicTable + "\nACTUAL DYNAMIC TABLE:\n" + actualDynamicTable); @@ -224,6 +224,18 @@ final class HpackTestCase { return ret.toString(); } + private static boolean headersEqual(List expected, List 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;