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

HTTP/2 HPACK Integer Encoding Bugs

Browse Source 
Motivation:
- Decoder#decodeULE128 has a bounds bug and cannot decode Integer.MAX_VALUE
- Decoder#decodeULE128 doesn't support values greater than can be represented with Java's int data type. This is a problem because there are cases that require at least unsigned 32 bits (max header table size).
- Decoder#decodeULE128 treats overflowing the data type and invalid input the same. This can be misleading when inspecting the error that is thrown.
- Encoder#encodeInteger doesn't support values greater than can be represented with Java's int data type. This is a problem because there are cases that require at least unsigned 32 bits (max header table size).

Modifications:
- Correct the above issues and add unit tests.

Result:
Fixes https://github.com/netty/netty/issues/6210.
This commit is contained in:
Scott Mitchell 2017-01-12 20:44:52 -08:00
parent 3ea807e375
commit b701da8d1c
6 changed files with 379 additions and 28 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -51,10 +51,12 @@ import static io.netty.util.AsciiString.EMPTY_STRING;
import static io.netty.util.internal.ThrowableUtil.unknownStackTrace;
public final class Decoder {
private static final Http2Exception DECODE_DECOMPRESSION_EXCEPTION = unknownStackTrace(
connectionError(COMPRESSION_ERROR, "HPACK - decompression failure"), Decoder.class, "decode(...)");
private static final Http2Exception DECODE_ULE_128_DECOMPRESSION_EXCEPTION = unknownStackTrace(
connectionError(COMPRESSION_ERROR, "HPACK - decompression failure"), Decoder.class, "decodeULE128(...)");
private static final Http2Exception DECODE_ULE_128_TO_LONG_DECOMPRESSION_EXCEPTION = unknownStackTrace(
connectionError(COMPRESSION_ERROR, "HPACK - long overflow"), Decoder.class, "decodeULE128(...)");
private static final Http2Exception DECODE_ULE_128_TO_INT_DECOMPRESSION_EXCEPTION = unknownStackTrace(
connectionError(COMPRESSION_ERROR, "HPACK - int overflow"), Decoder.class, "decodeULE128ToInt(...)");
private static final Http2Exception DECODE_ILLEGAL_INDEX_VALUE = unknownStackTrace(
connectionError(COMPRESSION_ERROR, "HPACK - illegal index value"), Decoder.class, "decode(...)");
private static final Http2Exception INDEX_HEADER_ILLEGAL_INDEX_VALUE = unknownStackTrace(
@ -184,7 +186,7 @@ public final class Decoder {
break;
case READ_MAX_DYNAMIC_TABLE_SIZE:
setDynamicTableSize(decodeULE128(in, index));
setDynamicTableSize(decodeULE128(in, (long) index));
state = READ_HEADER_REPRESENTATION;
break;
@ -346,7 +348,7 @@ public final class Decoder {
return dynamicTable.getEntry(index + 1);
}
private void setDynamicTableSize(int dynamicTableSize) throws Http2Exception {
private void setDynamicTableSize(long dynamicTableSize) throws Http2Exception {
if (dynamicTableSize > maxDynamicTableSize) {
throw INVALID_MAX_DYNAMIC_TABLE_SIZE;
}
@ -422,26 +424,53 @@ public final class Decoder {
return new IllegalArgumentException("decode only works with an entire header block! " + in);
}
// Unsigned Little Endian Base 128 Variable-Length Integer Encoding
private static int decodeULE128(ByteBuf in, int result) throws Http2Exception {
assert result <= 0x7f && result >= 0;
final int writerIndex = in.writerIndex();
for (int readerIndex = in.readerIndex(), shift = 0;
readerIndex < writerIndex; ++readerIndex, shift += 7) {
byte b = in.getByte(readerIndex);
if (shift == 28 && ((b & 0x80) != 0 || b > 6)) {
/**
* Unsigned Little Endian Base 128 Variable-Length Integer Encoding
* <p>
* Visible for testing only!
*/
static int decodeULE128(ByteBuf in, int result) throws Http2Exception {
final int readerIndex = in.readerIndex();
final long v = decodeULE128(in, (long) result);
if (v > Integer.MAX_VALUE) {
// the maximum value that can be represented by a signed 32 bit number is:
// 0x7f + 0x7f + (0x7f << 7) + (0x7f << 14) + (0x7f << 21) + (0x6 << 28)
// [0x1,0x7f] + 0x7f + (0x7f << 7) + (0x7f << 14) + (0x7f << 21) + (0x6 << 28)
// OR
// 0x0 + 0x7f + (0x7f << 7) + (0x7f << 14) + (0x7f << 21) + (0x7 << 28)
// we should reset the readerIndex if we overflowed the int type.
in.readerIndex(readerIndex);
throw DECODE_ULE_128_TO_INT_DECOMPRESSION_EXCEPTION;
}
return (int) v;
}
/**
* Unsigned Little Endian Base 128 Variable-Length Integer Encoding
* <p>
* Visible for testing only!
*/
static long decodeULE128(ByteBuf in, long result) throws Http2Exception {
assert result <= 0x7f && result >= 0;
final boolean resultStartedAtZero = result == 0;
final int writerIndex = in.writerIndex();
for (int readerIndex = in.readerIndex(), shift = 0; readerIndex < writerIndex; ++readerIndex, shift += 7) {
byte b = in.getByte(readerIndex);
if (shift == 56 && ((b & 0x80) != 0 || b == 0x7F && !resultStartedAtZero)) {
// the maximum value that can be represented by a signed 64 bit number is:
// [0x01L, 0x7fL] + 0x7fL + (0x7fL << 7) + (0x7fL << 14) + (0x7fL << 21) + (0x7fL << 28) + (0x7fL << 35)
// + (0x7fL << 42) + (0x7fL << 49) + (0x7eL << 56)
// OR
// 0x0L + 0x7fL + (0x7fL << 7) + (0x7fL << 14) + (0x7fL << 21) + (0x7fL << 28) + (0x7fL << 35) +
// (0x7fL << 42) + (0x7fL << 49) + (0x7fL << 56)
// this means any more shifts will result in overflow so we should break out and throw an error.
in.readerIndex(readerIndex + 1);
break;
throw DECODE_ULE_128_TO_LONG_DECOMPRESSION_EXCEPTION;
}
if ((b & 0x80) == 0) {
in.readerIndex(readerIndex + 1);
return result + ((b & 0x7F) << shift);
return result + ((b & 0x7FL) << shift);
}
result += (b & 0x7F) << shift;
result += (b & 0x7FL) << shift;
}
throw DECODE_ULE_128_DECOMPRESSION_EXCEPTION;

19
codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Encoder.java
View File

@ -204,7 +204,7 @@ public final class Encoder {
this.maxHeaderTableSize = maxHeaderTableSize;
ensureCapacity(0);
// Casting to integer is safe as we verified the maxHeaderTableSize is a valid unsigned int.
encodeInteger(out, 0x20, 5, (int) maxHeaderTableSize);
encodeInteger(out, 0x20, 5, maxHeaderTableSize);
}
/**
@ -227,20 +227,27 @@ public final class Encoder {
}
/**
* Encode integer according to Section 5.1.
* Encode integer according to <a href="https://tools.ietf.org/html/rfc7541#section-5.1">Section 5.1</a>.
*/
private static void encodeInteger(ByteBuf out, int mask, int n, int i) {
encodeInteger(out, mask, n, (long) i);
}
/**
* Encode integer according to <a href="https://tools.ietf.org/html/rfc7541#section-5.1">Section 5.1</a>.
*/
private static void encodeInteger(ByteBuf out, int mask, int n, long i) {
assert n >= 0 && n <= 8 : "N: " + n;
int nbits = 0xFF >>> (8 - n);
if (i < nbits) {
out.writeByte(mask | i);
out.writeByte((int) (mask | i));
} else {
out.writeByte(mask | nbits);
int length = i - nbits;
long length = i - nbits;
for (; (length & ~0x7F) != 0; length >>>= 7) {
out.writeByte((length & 0x7F) | 0x80);
out.writeByte((int) ((length & 0x7F) | 0x80));
}
out.writeByte(length);
out.writeByte((int) length);
}
}

2
codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/HuffmanEncoder.java
View File

@ -118,7 +118,7 @@ final class HuffmanEncoder {
* @param data the string literal to be Huffman encoded
* @return the number of bytes required to Huffman encode <code>data</code>
*/
public int getEncodedLength(CharSequence data) {
int getEncodedLength(CharSequence data) {
if (data instanceof AsciiString) {
AsciiString string = (AsciiString) data;
try {

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

@ -38,7 +38,7 @@ import io.netty.handler.codec.http2.Http2Headers;
import org.junit.Before;
import org.junit.Test;
import static io.netty.handler.codec.http2.Http2TestUtil.newTestDecoder;
import static io.netty.handler.codec.http2.internal.hpack.Decoder.decodeULE128;
import static io.netty.util.AsciiString.EMPTY_STRING;
import static io.netty.util.AsciiString.of;
import static java.lang.Integer.MAX_VALUE;
@ -50,10 +50,6 @@ import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
public class DecoderTest {
private static final int MAX_HEADER_LIST_SIZE = 8192;
private static final int MAX_HEADER_TABLE_SIZE = 4096;
private Decoder decoder;
private Http2Headers mockHeaders;
@ -77,6 +73,109 @@ public class DecoderTest {
mockHeaders = mock(Http2Headers.class);
}
@Test
public void testDecodeULE128IntMax() throws Http2Exception {
byte[] input = {(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0x07};
ByteBuf in = Unpooled.wrappedBuffer(input);
try {
assertEquals(Integer.MAX_VALUE, decodeULE128(in, 0));
} finally {
in.release();
}
}
@Test(expected = Http2Exception.class)
public void testDecodeULE128IntOverflow1() throws Http2Exception {
byte[] input = {(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0x07};
ByteBuf in = Unpooled.wrappedBuffer(input);
final int readerIndex = in.readerIndex();
try {
decodeULE128(in, 1);
} finally {
assertEquals(readerIndex, in.readerIndex());
in.release();
}
}
@Test(expected = Http2Exception.class)
public void testDecodeULE128IntOverflow2() throws Http2Exception {
byte[] input = {(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0x08};
ByteBuf in = Unpooled.wrappedBuffer(input);
final int readerIndex = in.readerIndex();
try {
decodeULE128(in, 0);
} finally {
assertEquals(readerIndex, in.readerIndex());
in.release();
}
}
@Test
public void testDecodeULE128LongMax() throws Http2Exception {
byte[] input = {(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF,
(byte) 0xFF, (byte) 0x7F};
ByteBuf in = Unpooled.wrappedBuffer(input);
try {
assertEquals(Long.MAX_VALUE, decodeULE128(in, 0L));
} finally {
in.release();
}
}
@Test(expected = Http2Exception.class)
public void testDecodeULE128LongOverflow1() throws Http2Exception {
byte[] input = {(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF,
(byte) 0xFF, (byte) 0xFF};
ByteBuf in = Unpooled.wrappedBuffer(input);
final int readerIndex = in.readerIndex();
try {
decodeULE128(in, 0L);
} finally {
assertEquals(readerIndex, in.readerIndex());
in.release();
}
}
@Test(expected = Http2Exception.class)
public void testDecodeULE128LongOverflow2() throws Http2Exception {
byte[] input = {(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF,
(byte) 0xFF, (byte) 0x7F};
ByteBuf in = Unpooled.wrappedBuffer(input);
final int readerIndex = in.readerIndex();
try {
decodeULE128(in, 1L);
} finally {
assertEquals(readerIndex, in.readerIndex());
in.release();
}
}
@Test
public void testSetTableSizeWithMaxUnsigned32BitValueSucceeds() throws Http2Exception {
byte[] input = {(byte) 0x3F, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0x0E};
ByteBuf in = Unpooled.wrappedBuffer(input);
try {
final long expectedHeaderSize = 4026531870L; // based on the input above
decoder.setMaxHeaderTableSize(expectedHeaderSize);
decoder.decode(0, in, mockHeaders);
assertEquals(expectedHeaderSize, decoder.getMaxHeaderTableSize());
} finally {
in.release();
}
}
@Test(expected = Http2Exception.class)
public void testSetTableSizeOverLimitFails() throws Http2Exception {
byte[] input = {(byte) 0x3F, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0x0E};
ByteBuf in = Unpooled.wrappedBuffer(input);
try {
decoder.setMaxHeaderTableSize(4026531870L - 1); // based on the input above ... 1 less than is above.
decoder.decode(0, in, mockHeaders);
} finally {
in.release();
}
}
@Test
public void testLiteralHuffmanEncodedWithEmptyNameAndValue() throws Http2Exception {
byte[] input = {0, (byte) 0x80, 0};
@ -123,7 +222,7 @@ public class DecoderTest {
}
@Test(expected = Http2Exception.class)
public void testIncompleteIndex() throws Http2Exception, Http2Exception {
public void testIncompleteIndex() throws Http2Exception {
byte[] compressed = Hex.decodeHex("FFF0".toCharArray());
ByteBuf in = Unpooled.wrappedBuffer(compressed);
try {

60
codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/EncoderTest.java Normal file
View File

@ -0,0 +1,60 @@
/*
* Copyright 2017 The Netty Project
*
* The Netty Project licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package io.netty.handler.codec.http2.internal.hpack;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.handler.codec.http2.Http2Exception;
import io.netty.handler.codec.http2.Http2Headers;
import org.junit.Before;
import org.junit.Test;
import static io.netty.handler.codec.http2.Http2CodecUtil.MAX_HEADER_TABLE_SIZE;
import static org.junit.Assert.assertEquals;
import static org.mockito.Mockito.mock;
public class EncoderTest {
private Decoder decoder;
private Encoder encoder;
private Http2Headers mockHeaders;
@Before
public void setUp() throws Http2Exception {
encoder = new Encoder();
decoder = new Decoder();
mockHeaders = mock(Http2Headers.class);
}
@Test
public void testSetMaxHeaderTableSizeToMaxValue() throws Http2Exception {
ByteBuf buf = Unpooled.buffer();
encoder.setMaxHeaderTableSize(buf, MAX_HEADER_TABLE_SIZE);
decoder.setMaxHeaderTableSize(MAX_HEADER_TABLE_SIZE);
decoder.decode(0, buf, mockHeaders);
assertEquals(MAX_HEADER_TABLE_SIZE, decoder.getMaxHeaderTableSize());
buf.release();
}
@Test(expected = Http2Exception.class)
public void testSetMaxHeaderTableSizeOverflow() throws Http2Exception {
ByteBuf buf = Unpooled.buffer();
try {
encoder.setMaxHeaderTableSize(buf, MAX_HEADER_TABLE_SIZE + 1);
} finally {
buf.release();
}
}
}

156
microbench/src/main/java/io/netty/microbench/http2/internal/hpack/DecoderULE128Benchmark.java Normal file
View File

@ -0,0 +1,156 @@
/*
* Copyright 2017 The Netty Project
*
* The Netty Project licenses this file to you under the Apache License,
* version 2.0 (the "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at:
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/
package io.netty.microbench.http2.internal.hpack;
import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.netty.handler.codec.http2.Http2Error;
import io.netty.handler.codec.http2.Http2Exception;
import io.netty.microbench.util.AbstractMicrobenchmark;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Scope;
import org.openjdk.jmh.annotations.Setup;
import org.openjdk.jmh.annotations.State;
import org.openjdk.jmh.annotations.Threads;
import org.openjdk.jmh.annotations.Warmup;
import java.util.concurrent.TimeUnit;
@Threads(1)
@State(Scope.Benchmark)
@Fork(1)
@Warmup(iterations = 5)
@Measurement(iterations = 10)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
public class DecoderULE128Benchmark extends AbstractMicrobenchmark {
private static final Http2Exception DECODE_ULE_128_TO_LONG_DECOMPRESSION_EXCEPTION =
new Http2Exception(Http2Error.COMPRESSION_ERROR);
private static final Http2Exception DECODE_ULE_128_TO_INT_DECOMPRESSION_EXCEPTION =
new Http2Exception(Http2Error.COMPRESSION_ERROR);
private static final Http2Exception DECODE_ULE_128_DECOMPRESSION_EXCEPTION =
new Http2Exception(Http2Error.COMPRESSION_ERROR);
private ByteBuf longMaxBuf;
private ByteBuf intMaxBuf;
@Setup
public void setup() {
byte[] longMax = {(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF,
(byte) 0xFF, (byte) 0x7F};
longMaxBuf = Unpooled.wrappedBuffer(longMax);
byte[] intMax = {(byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0xFF, (byte) 0x07};
intMaxBuf = Unpooled.wrappedBuffer(intMax);
}
@Benchmark
@BenchmarkMode(Mode.AverageTime)
public long decodeMaxLong() throws Http2Exception {
long v = decodeULE128(longMaxBuf, 0L);
longMaxBuf.readerIndex(0);
return v;
}
@Benchmark
@BenchmarkMode(Mode.AverageTime)
public long decodeMaxIntWithLong() throws Http2Exception {
long v = decodeULE128(intMaxBuf, 0L);
intMaxBuf.readerIndex(0);
return v;
}
@Benchmark
@BenchmarkMode(Mode.AverageTime)
public int decodeMaxInt() throws Http2Exception {
int v = decodeULE128(intMaxBuf, 0);
intMaxBuf.readerIndex(0);
return v;
}
@Benchmark
@BenchmarkMode(Mode.AverageTime)
public int decodeMaxIntUsingLong() throws Http2Exception {
int v = decodeULE128UsingLong(intMaxBuf, 0);
intMaxBuf.readerIndex(0);
return v;
}
static int decodeULE128UsingLong(ByteBuf in, int result) throws Http2Exception {
final int readerIndex = in.readerIndex();
final long v = decodeULE128(in, (long) result);
if (v > Integer.MAX_VALUE) {
in.readerIndex(readerIndex);
throw DECODE_ULE_128_TO_INT_DECOMPRESSION_EXCEPTION;
}
return (int) v;
}
static long decodeULE128(ByteBuf in, long result) throws Http2Exception {
assert result <= 0x7f && result >= 0;
final boolean resultStartedAtZero = result == 0;
final int writerIndex = in.writerIndex();
for (int readerIndex = in.readerIndex(), shift = 0; readerIndex < writerIndex; ++readerIndex, shift += 7) {
byte b = in.getByte(readerIndex);
if (shift == 56 && ((b & 0x80) != 0 || b == 0x7F && !resultStartedAtZero)) {
// the maximum value that can be represented by a signed 64 bit number is:
// [0x01L, 0x7fL] + 0x7fL + (0x7fL << 7) + (0x7fL << 14) + (0x7fL << 21) + (0x7fL << 28) + (0x7fL << 35)
// + (0x7fL << 42) + (0x7fL << 49) + (0x7eL << 56)
// OR
// 0x0L + 0x7fL + (0x7fL << 7) + (0x7fL << 14) + (0x7fL << 21) + (0x7fL << 28) + (0x7fL << 35) +
// (0x7fL << 42) + (0x7fL << 49) + (0x7fL << 56)
// this means any more shifts will result longMaxBuf overflow so we should break out and throw an error.
throw DECODE_ULE_128_TO_LONG_DECOMPRESSION_EXCEPTION;
}
if ((b & 0x80) == 0) {
in.readerIndex(readerIndex + 1);
return result + ((b & 0x7FL) << shift);
}
result += (b & 0x7FL) << shift;
}
throw DECODE_ULE_128_DECOMPRESSION_EXCEPTION;
}
static int decodeULE128(ByteBuf in, int result) throws Http2Exception {
assert result <= 0x7f && result >= 0;
final boolean resultStartedAtZero = result == 0;
final int writerIndex = in.writerIndex();
for (int readerIndex = in.readerIndex(), shift = 0; readerIndex < writerIndex; ++readerIndex, shift += 7) {
byte b = in.getByte(readerIndex);
if (shift == 28 && ((b & 0x80) != 0 || !resultStartedAtZero && b > 6 || resultStartedAtZero && b > 7)) {
// the maximum value that can be represented by a signed 32 bit number is:
// [0x1,0x7f] + 0x7f + (0x7f << 7) + (0x7f << 14) + (0x7f << 21) + (0x6 << 28)
// OR
// 0x0 + 0x7f + (0x7f << 7) + (0x7f << 14) + (0x7f << 21) + (0x7 << 28)
// this means any more shifts will result longMaxBuf overflow so we should break out and throw an error.
throw DECODE_ULE_128_TO_INT_DECOMPRESSION_EXCEPTION;
}
if ((b & 0x80) == 0) {
in.readerIndex(readerIndex + 1);
return result + ((b & 0x7F) << shift);
}
result += (b & 0x7F) << shift;
}
throw DECODE_ULE_128_DECOMPRESSION_EXCEPTION;
}
}