andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

HTTP/2 HPACK Header Name Validation and Trailing Padding

Browse Source 
Motivation:
The HPACK code currently disallows empty header names. This is not explicitly forbidden by the HPACK RFC https://tools.ietf.org/html/rfc7541. However the HTTP/1.x RFC https://tools.ietf.org/html/rfc7230#section-3.2 and thus HTTP/2 both disallow empty header names, and so this precondition check should be moved from the HPACK code to the protocol level.
HPACK also requires that string literals which are huffman encoded must be treated as an encoding error if the string has more than 7 trailing padding bits https://tools.ietf.org/html/rfc7541#section-5.2, but this is currently not enforced.

Result:
- HPACK to allow empty header names
- HTTP/1.x and HTTP/2 header validation should not allow empty header names
- Enforce max of 7 trailing padding bits

Result:
Code is more compliant with the above mentioned RFCs
Fixes https://github.com/netty/netty/issues/5228
This commit is contained in:
Scott Mitchell 2016-05-10 09:09:37 -07:00 committed by nmittler
parent a517cce92f
commit 1cb706ac93
11 changed files with 229 additions and 70 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
View File

@ -53,6 +53,9 @@ public class DefaultHttpHeaders extends HttpHeaders {
static final NameValidator<CharSequence> HttpNameValidator = new NameValidator<CharSequence>() {
@Override
public void validateName(CharSequence name) {
if (name == null || name.length() == 0) {
throw new IllegalArgumentException("empty headers are not allowed [" + name + "]");
}
if (name instanceof AsciiString) {
try {
((AsciiString) name).forEachByte(HEADER_NAME_VALIDATOR);

11
codec-http/src/test/java/io/netty/handler/codec/http/DefaultHttpHeadersTest.java
View File

@ -17,6 +17,7 @@ package io.netty.handler.codec.http;
import io.netty.handler.codec.http.HttpHeadersTestUtils.HeaderValue;
import io.netty.util.AsciiString;
import io.netty.util.internal.StringUtil;
import org.junit.Test;
import java.util.Arrays;
@ -36,6 +37,16 @@ import static org.junit.Assert.assertTrue;
public class DefaultHttpHeadersTest {
private static final CharSequence HEADER_NAME = "testHeader";
@Test(expected = IllegalArgumentException.class)
public void nullHeaderNameNotAllowed() {
new DefaultHttpHeaders().add(null, "foo");
}
@Test(expected = IllegalArgumentException.class)
public void emtpyHeaderNameNotAllowed() {
new DefaultHttpHeaders().add(StringUtil.EMPTY_STRING, "foo");
}
@Test
public void keysShouldBeCaseInsensitive() {
DefaultHttpHeaders headers = new DefaultHttpHeaders();

4
codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java
View File

@ -37,6 +37,10 @@ public class DefaultHttp2Headers
private static final NameValidator<CharSequence> HTTP2_NAME_VALIDATOR = new NameValidator<CharSequence>() {
@Override
public void validateName(CharSequence name) {
if (name == null || name.length() == 0) {
PlatformDependent.throwException(connectionError(PROTOCOL_ERROR,
"empty headers are not allowed [%s]", name));
}
if (name instanceof AsciiString) {
final int index;
try {

36
codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java
View File

@ -32,22 +32,30 @@
package io.netty.handler.codec.http2.internal.hpack;
import io.netty.handler.codec.http2.internal.hpack.HpackUtil.IndexType;
import io.netty.util.internal.EmptyArrays;
import java.io.IOException;
import java.io.InputStream;
import static io.netty.util.internal.EmptyArrays.EMPTY_BYTES;
public final class Decoder {
private static final IOException DECOMPRESSION_EXCEPTION =
new IOException("decompression failure");
new IOException("HPACK - decompression failure");
private static final IOException ILLEGAL_INDEX_VALUE =
new IOException("illegal index value");
new IOException("HPACK - illegal index value");
private static final IOException INVALID_MAX_DYNAMIC_TABLE_SIZE =
new IOException("invalid max dynamic table size");
new IOException("HPACK - invalid max dynamic table size");
private static final IOException MAX_DYNAMIC_TABLE_SIZE_CHANGE_REQUIRED =
new IOException("max dynamic table size change required");
new IOException("HPACK - max dynamic table size change required");
private static final byte[] EMPTY = {};
static {
DECOMPRESSION_EXCEPTION.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE);
ILLEGAL_INDEX_VALUE.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE);
INVALID_MAX_DYNAMIC_TABLE_SIZE.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE);
MAX_DYNAMIC_TABLE_SIZE_CHANGE_REQUIRED.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE);
}
private final DynamicTable dynamicTable;
@ -214,17 +222,12 @@ public final class Decoder {
} else {
nameLength = index;
// Disallow empty names -- they cannot be represented in HTTP/1.x
if (nameLength == 0) {
throw DECOMPRESSION_EXCEPTION;
}
// Check name length against max header size
if (exceedsMaxHeaderSize(nameLength)) {
if (indexType == IndexType.NONE) {
// Name is unused so skip bytes
name = EMPTY;
name = EMPTY_BYTES;
skipLength = nameLength;
state = State.SKIP_LITERAL_HEADER_NAME;
break;
@ -233,7 +236,7 @@ public final class Decoder {
// Check name length against max dynamic table size
if (nameLength + HeaderField.HEADER_ENTRY_OVERHEAD > dynamicTable.capacity()) {
dynamicTable.clear();
name = EMPTY;
name = EMPTY_BYTES;
skipLength = nameLength;
state = State.SKIP_LITERAL_HEADER_NAME;
break;
@ -260,7 +263,7 @@ public final class Decoder {
if (exceedsMaxHeaderSize(nameLength)) {
if (indexType == IndexType.NONE) {
// Name is unused so skip bytes
name = EMPTY;
name = EMPTY_BYTES;
skipLength = nameLength;
state = State.SKIP_LITERAL_HEADER_NAME;
break;
@ -269,7 +272,7 @@ public final class Decoder {
// Check name length against max dynamic table size
if (nameLength + HeaderField.HEADER_ENTRY_OVERHEAD > dynamicTable.capacity()) {
dynamicTable.clear();
name = EMPTY;
name = EMPTY_BYTES;
skipLength = nameLength;
state = State.SKIP_LITERAL_HEADER_NAME;
break;
@ -327,7 +330,7 @@ public final class Decoder {
}
if (valueLength == 0) {
insertHeader(headerListener, name, EMPTY, indexType);
insertHeader(headerListener, name, EMPTY_BYTES, indexType);
state = State.READ_HEADER_REPRESENTATION;
} else {
state = State.READ_LITERAL_HEADER_VALUE;
@ -502,9 +505,6 @@ public final class Decoder {
private void addHeader(HeaderListener headerListener, byte[] name, byte[] value,
boolean sensitive) {
if (name.length == 0) {
throw new AssertionError("name is empty");
}
long newSize = headerSize + name.length + value.length;
if (newSize <= maxHeaderSize) {
headerListener.addHeader(name, value, sensitive);

8
codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/HeaderField.java
View File

@ -31,8 +31,8 @@
*/
package io.netty.handler.codec.http2.internal.hpack;
import static io.netty.handler.codec.http2.internal.hpack.HpackUtil.ISO_8859_1;
import static io.netty.handler.codec.http2.internal.hpack.HpackUtil.requireNonNull;
import static io.netty.util.CharsetUtil.ISO_8859_1;
import static io.netty.util.internal.ObjectUtil.checkNotNull;
class HeaderField implements Comparable<HeaderField> {
@ -54,8 +54,8 @@ class HeaderField implements Comparable<HeaderField> {
}
HeaderField(byte[] name, byte[] value) {
this.name = requireNonNull(name);
this.value = requireNonNull(value);
this.name = checkNotNull(name, "name");
this.value = checkNotNull(value, "value");
}
int size() {

13
codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/HpackUtil.java
View File

@ -34,9 +34,6 @@ package io.netty.handler.codec.http2.internal.hpack;
import java.nio.charset.Charset;
final class HpackUtil {
static final Charset ISO_8859_1 = Charset.forName("ISO-8859-1");
/**
* A string compare that doesn't leak timing information.
*/
@ -51,16 +48,6 @@ final class HpackUtil {
return c == 0;
}
/**
* Checks that the specified object reference is not {@code null}.
*/
static <T> T requireNonNull(T obj) {
if (obj == null) {
throw new NullPointerException();
}
return obj;
}
// Section 6.2. Literal Header Field Representation
enum IndexType {
INCREMENTAL, // Section 6.2.1. Literal Header Field with Incremental Indexing

66
codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/HuffmanDecoder.java
View File

@ -31,13 +31,20 @@
*/
package io.netty.handler.codec.http2.internal.hpack;
import io.netty.util.internal.EmptyArrays;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
final class HuffmanDecoder {
private static final IOException EOS_DECODED = new IOException("EOS Decoded");
private static final IOException INVALID_PADDING = new IOException("Invalid Padding");
private static final IOException EOS_DECODED = new IOException("HPACK - EOS Decoded");
private static final IOException INVALID_PADDING = new IOException("HPACK - Invalid Padding");
static {
EOS_DECODED.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE);
INVALID_PADDING.setStackTrace(EmptyArrays.EMPTY_STACK_TRACE);
}
private final Node root;
@ -65,44 +72,77 @@ final class HuffmanDecoder {
public byte[] decode(byte[] buf) throws IOException {
ByteArrayOutputStream baos = new ByteArrayOutputStream();
/*
* The idea here is to consume whole bytes at a time rather than individual bits. node
* represents the Huffman tree, with all bit patterns denormalized as 256 children. Each
* child represents the last 8 bits of the huffman code. The parents of each child each
* represent the successive 8 bit chunks that lead up to the last most part. 8 bit bytes
* from buf are used to traverse these tree until a terminal node is found.
*
* current is a bit buffer. The low order bits represent how much of the huffman code has
* not been used to traverse the tree. Thus, the high order bits are just garbage.
* currentBits represents how many of the low order bits of current are actually valid.
* currentBits will vary between 0 and 15.
*
* symbolBits is the number of bits of the the symbol being decoded, *including* all those
* of the parent nodes. symbolBits tells how far down the tree we are. For example, when
* decoding the invalid sequence {0xff, 0xff}, currentBits will be 0, but symbolBits will be
* 16. This is used to know if buf ended early (before consuming a whole symbol) or if
* there is too much padding.
*/
Node node = root;
int current = 0;
int bits = 0;
int currentBits = 0;
int symbolBits = 0;
for (int i = 0; i < buf.length; i++) {
int b = buf[i] & 0xFF;
current = (current << 8) | b;
bits += 8;
while (bits >= 8) {
int c = (current >>> (bits - 8)) & 0xFF;
currentBits += 8;
symbolBits += 8;
// While there are unconsumed bits in current, keep consuming symbols.
while (currentBits >= 8) {
int c = (current >>> (currentBits - 8)) & 0xFF;
node = node.children[c];
bits -= node.bits;
currentBits -= node.bits;
if (node.isTerminal()) {
if (node.symbol == HpackUtil.HUFFMAN_EOS) {
throw EOS_DECODED;
}
baos.write(node.symbol);
node = root;
// Upon consuming a whole symbol, reset the symbol bits to the number of bits
// left over in the byte.
symbolBits = currentBits;
}
}
}
while (bits > 0) {
int c = (current << (8 - bits)) & 0xFF;
/*
* We have consumed all the bytes in buf, but haven't consumed all the symbols. We may be on
* a partial symbol, so consume until there is nothing left. This will loop at most 2 times.
*/
while (currentBits > 0) {
int c = (current << (8 - currentBits)) & 0xFF;
node = node.children[c];
if (node.isTerminal() && node.bits <= bits) {
bits -= node.bits;
if (node.isTerminal() && node.bits <= currentBits) {
if (node.symbol == HpackUtil.HUFFMAN_EOS) {
throw EOS_DECODED;
}
currentBits -= node.bits;
baos.write(node.symbol);
node = root;
symbolBits = currentBits;
} else {
break;
}
}
// Section 5.2. String Literal Representation
// A padding strictly longer than 7 bits MUST be treated as a decoding error.
// Padding not corresponding to the most significant bits of the code
// for the EOS symbol (0xFF) MUST be treated as a decoding error.
int mask = (1 << bits) - 1;
if ((current & mask) != mask) {
int mask = (1 << symbolBits) - 1;
if (symbolBits > 7 || (current & mask) != mask) {
throw INVALID_PADDING;
}

6
codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/StaticTable.java
View File

@ -36,6 +36,8 @@ import java.util.HashMap;
import java.util.List;
import java.util.Map;
import static io.netty.util.CharsetUtil.ISO_8859_1;
final class StaticTable {
private static final String EMPTY = "";
@ -125,7 +127,7 @@ final class StaticTable {
* -1 if the header field name is not in the static table.
*/
static int getIndex(byte[] name) {
String nameString = new String(name, 0, name.length, HpackUtil.ISO_8859_1);
String nameString = new String(name, 0, name.length, ISO_8859_1);
Integer index = STATIC_INDEX_BY_NAME.get(nameString);
if (index == null) {
return -1;
@ -166,7 +168,7 @@ final class StaticTable {
// save the smallest index for a given name in the map.
for (int index = length; index > 0; index--) {
HeaderField entry = getEntry(index);
String name = new String(entry.name, 0, entry.name.length, HpackUtil.ISO_8859_1);
String name = new String(entry.name, 0, entry.name.length, ISO_8859_1);
ret.put(name, index);
}
return ret;

11
codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2HeadersTest.java
View File

@ -17,6 +17,7 @@
package io.netty.handler.codec.http2;
import io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName;
import io.netty.util.internal.StringUtil;
import org.junit.Test;
import java.util.Map.Entry;
@ -30,6 +31,16 @@ import static org.junit.Assert.fail;
public class DefaultHttp2HeadersTest {
@Test(expected = Http2Exception.class)
public void nullHeaderNameNotAllowed() {
new DefaultHttp2Headers().add(null, "foo");
}
@Test(expected = Http2Exception.class)
public void emtpyHeaderNameNotAllowed() {
new DefaultHttp2Headers().add(StringUtil.EMPTY_STRING, "foo");
}
@Test
public void testPseudoHeadersMustComeFirstWhenIterating() {
Http2Headers headers = newHeaders();

83
codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java
View File

@ -31,6 +31,14 @@
*/
package io.netty.handler.codec.http2.internal.hpack;
import io.netty.util.CharsetUtil;
import org.junit.Before;
import org.junit.Test;
import java.io.ByteArrayInputStream;
import java.io.IOException;
import static io.netty.util.internal.EmptyArrays.EMPTY_BYTES;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
@ -40,12 +48,6 @@ import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import org.junit.Before;
import org.junit.Test;
import java.io.ByteArrayInputStream;
import java.io.IOException;
public class DecoderTest {
private static final int MAX_HEADER_SIZE = 8192;
@ -59,12 +61,17 @@ public class DecoderTest {
}
private static byte[] getBytes(String s) {
return s.getBytes(HpackUtil.ISO_8859_1);
return s.getBytes(CharsetUtil.ISO_8859_1);
}
private void decode(String encoded) throws IOException {
byte[] b = Hex.decodeHex(encoded.toCharArray());
decoder.decode(new ByteArrayInputStream(b), mockListener);
ByteArrayInputStream in = new ByteArrayInputStream(b);
try {
decoder.decode(in, mockListener);
} finally {
in.close();
}
}
@Before
@ -73,15 +80,64 @@ public class DecoderTest {
mockListener = mock(HeaderListener.class);
}
@Test
public void testLiteralHuffmanEncodedWithEmptyNameAndValue() throws IOException {
byte[] input = {0, (byte) 0x80, 0};
ByteArrayInputStream in = new ByteArrayInputStream(input);
try {
decoder.decode(in, mockListener);
verify(mockListener, times(1)).addHeader(EMPTY_BYTES, EMPTY_BYTES, false);
} finally {
in.close();
}
}
@Test(expected = IOException.class)
public void testLiteralHuffmanEncodedWithPaddingGreaterThan7Throws() throws IOException {
byte[] input = {0, (byte) 0x81, -1};
ByteArrayInputStream in = new ByteArrayInputStream(input);
try {
decoder.decode(in, mockListener);
} finally {
in.close();
}
}
@Test(expected = IOException.class)
public void testLiteralHuffmanEncodedWithDecodingEOSThrows() throws IOException {
byte[] input = {0, (byte) 0x84, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF};
ByteArrayInputStream in = new ByteArrayInputStream(input);
try {
decoder.decode(in, mockListener);
} finally {
in.close();
}
}
@Test(expected = IOException.class)
public void testLiteralHuffmanEncodedWithPaddingNotCorrespondingToMSBThrows() throws IOException {
byte[] input = {0, (byte) 0x81, 0};
ByteArrayInputStream in = new ByteArrayInputStream(input);
try {
decoder.decode(in, mockListener);
} finally {
in.close();
}
}
@Test
public void testIncompleteIndex() throws IOException {
// Verify incomplete indices are unread
byte[] compressed = Hex.decodeHex("FFF0".toCharArray());
ByteArrayInputStream in = new ByteArrayInputStream(compressed);
try {
decoder.decode(in, mockListener);
assertEquals(1, in.available());
decoder.decode(in, mockListener);
assertEquals(1, in.available());
} finally {
in.close();
}
}
@Test(expected = IOException.class)
@ -150,9 +206,10 @@ public class DecoderTest {
decode("81");
}
@Test(expected = IOException.class)
@Test
public void testLiteralWithIncrementalIndexingWithEmptyName() throws Exception {
decode("000005" + hex("value"));
decode("400005" + hex("value"));
verify(mockListener, times(1)).addHeader(EMPTY_BYTES, getBytes("value"), false);
}
@Test
@ -228,9 +285,10 @@ public class DecoderTest {
verifyNoMoreInteractions(mockListener);
}
@Test(expected = IOException.class)
@Test
public void testLiteralWithoutIndexingWithEmptyName() throws Exception {
decode("000005" + hex("value"));
verify(mockListener, times(1)).addHeader(EMPTY_BYTES, getBytes("value"), false);
}
@Test(expected = IOException.class)
@ -272,9 +330,10 @@ public class DecoderTest {
decode("BE");
}
@Test(expected = IOException.class)
@Test
public void testLiteralNeverIndexedWithEmptyName() throws Exception {
decode("100005" + hex("value"));
verify(mockListener, times(1)).addHeader(EMPTY_BYTES, getBytes("value"), true);
}
@Test(expected = IOException.class)

50
codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/HuffmanTest.java
View File

@ -72,14 +72,56 @@ public class HuffmanTest {
Huffman.DECODER.decode(buf);
}
@Test//(expected = IOException.class) TODO(jpinner) fix me
@Test(expected = IOException.class)
public void testDecodeExtraPadding() throws IOException {
byte[] buf = new byte[2];
buf[0] = 0x0F; // '1', 'EOS'
buf[1] = (byte) 0xFF; // 'EOS'
byte[] buf = makeBuf(0x0f, 0xFF); // '1', 'EOS'
Huffman.DECODER.decode(buf);
}
@Test(expected = IOException.class)
public void testDecodeExtraPadding1byte() throws IOException {
byte[] buf = makeBuf(0xFF);
Huffman.DECODER.decode(buf);
}
@Test(expected = IOException.class)
public void testDecodeExtraPadding2byte() throws IOException {
byte[] buf = makeBuf(0x1F, 0xFF); // 'a'
Huffman.DECODER.decode(buf);
}
@Test(expected = IOException.class)
public void testDecodeExtraPadding3byte() throws IOException {
byte[] buf = makeBuf(0x1F, 0xFF, 0xFF); // 'a'
Huffman.DECODER.decode(buf);
}
@Test(expected = IOException.class)
public void testDecodeExtraPadding4byte() throws IOException {
byte[] buf = makeBuf(0x1F, 0xFF, 0xFF, 0xFF); // 'a'
Huffman.DECODER.decode(buf);
}
@Test(expected = IOException.class)
public void testDecodeExtraPadding29bit() throws IOException {
byte[] buf = makeBuf(0xFF, 0x9F, 0xFF, 0xFF, 0xFF); // '|'
Huffman.DECODER.decode(buf);
}
@Test(expected = IOException.class)
public void testDecodePartialSymbol() throws IOException {
byte[] buf = makeBuf(0x52, 0xBC, 0x30, 0xFF, 0xFF, 0xFF, 0xFF); // " pFA\x00", 31 bits of padding, a.k.a. EOS
Huffman.DECODER.decode(buf);
}
private byte[] makeBuf(int ... bytes) {
byte[] buf = new byte[bytes.length];
for (int i = 0; i < buf.length; i++) {
buf[i] = (byte) bytes[i];
}
return buf;
}
private void roundTrip(String s) throws IOException {
roundTrip(Huffman.ENCODER, Huffman.DECODER, s);
}