Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HpackDecoder treats invalid pseudo-headers as stream level errors #8046

Merged
merged 1 commit into from Jun 26, 2018
Merged

HpackDecoder treats invalid pseudo-headers as stream level errors #8046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
83 changes: 46 additions & 37 deletions codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
View file
Open in desktop
Expand Up @@ -44,6 +44,7 @@
import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR;
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
import static io.netty.handler.codec.http2.Http2Exception.connectionError;
import static io.netty.handler.codec.http2.Http2Exception.streamError;
import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.getPseudoHeader;
import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.hasPseudoHeaderFormat;
import static io.netty.util.AsciiString.EMPTY_STRING;
Expand Down Expand Up @@ -119,24 +120,21 @@ final class HpackDecoder {
* This method assumes the entire header block is contained in {@code in}.
*/
public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean validateHeaders) throws Http2Exception {
Http2HeadersSink sink = new Http2HeadersSink(headers, maxHeaderListSize);
decode(in, sink, validateHeaders);
Http2HeadersSink sink = new Http2HeadersSink(streamId, headers, maxHeaderListSize, validateHeaders);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it wasn't introduced by this PR but it would be nice to not have to introduce object allocation for each decode. Seems like this can be handled as local state in the now private decode method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind if we defer this for a later commit? I think the private decode method is really heavy at this time and don't really want to add more state to it, at least as part of another behavior change.

fwiw, I suspect this is a really good candidate for stack allocation and even if it's not, it's super short lived garbage that only happens once on a complete HEADERS block.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am +1 to defer this for a followup as its not introduced by this change

decode(in, sink);

// we have read all of our headers. See if we have exceeded our maxHeaderListSize. We must
// delay throwing until this point to prevent dynamic table corruption
if (sink.exceededMaxLength()) {
headerListSizeExceeded(streamId, maxHeaderListSize, true);
}
// Now that we've read all of our headers we can perform the validation steps. We must
// delay throwing until this point to prevent dynamic table corruption.
sink.finish();
}

private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2Exception {
private void decode(ByteBuf in, Sink sink) throws Http2Exception {
int index = 0;
int nameLength = 0;
int valueLength = 0;
byte state = READ_HEADER_REPRESENTATION;
boolean huffmanEncoded = false;
CharSequence name = null;
HeaderType headerType = null;
IndexType indexType = IndexType.NONE;
while (in.isReadable()) {
switch (state) {
Expand All @@ -157,7 +155,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
break;
default:
HpackHeaderField indexedHeader = getIndexedHeader(index);
headerType = validate(indexedHeader.name, headerType, validateHeaders);
sink.appendToHeaderList(indexedHeader.name, indexedHeader.value);
}
} else if ((b & 0x40) == 0x40) {
Expand All @@ -174,7 +171,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
default:
// Index was stored as the prefix
name = readName(index);
headerType = validate(name, headerType, validateHeaders);
nameLength = name.length();
state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
}
Expand All @@ -201,7 +197,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
default:
// Index was stored as the prefix
name = readName(index);
headerType = validate(name, headerType, validateHeaders);
nameLength = name.length();
state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
}
Expand All @@ -215,15 +210,13 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2

case READ_INDEXED_HEADER:
HpackHeaderField indexedHeader = getIndexedHeader(decodeULE128(in, index));
headerType = validate(indexedHeader.name, headerType, validateHeaders);
sink.appendToHeaderList(indexedHeader.name, indexedHeader.value);
state = READ_HEADER_REPRESENTATION;
break;

case READ_INDEXED_HEADER_NAME:
// Header Name matches an entry in the Header Table
name = readName(decodeULE128(in, index));
headerType = validate(name, headerType, validateHeaders);
nameLength = name.length();
state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
break;
Expand Down Expand Up @@ -254,7 +247,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
}

name = readStringLiteral(in, nameLength, huffmanEncoded);
headerType = validate(name, headerType, validateHeaders);

state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
break;
Expand All @@ -268,7 +260,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
state = READ_LITERAL_HEADER_VALUE_LENGTH;
break;
case 0:
headerType = validate(name, headerType, validateHeaders);
insertHeader(sink, name, EMPTY_STRING, indexType);
state = READ_HEADER_REPRESENTATION;
break;
Expand All @@ -293,7 +284,6 @@ private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2
}

CharSequence value = readStringLiteral(in, valueLength, huffmanEncoded);
headerType = validate(name, headerType, validateHeaders);
insertHeader(sink, name, value, indexType);
state = READ_HEADER_REPRESENTATION;
break;
Expand Down Expand Up @@ -327,7 +317,7 @@ public void setMaxHeaderTableSize(long maxHeaderTableSize) throws Http2Exception
}

/**
* @deprecated use {@link #setmaxHeaderListSize(long)}; {@code maxHeaderListSizeGoAway} is
* @deprecated use {@link #setMaxHeaderListSize(long)}; {@code maxHeaderListSizeGoAway} is
* ignored
*/
@Deprecated
Expand Down Expand Up @@ -385,26 +375,23 @@ private void setDynamicTableSize(long dynamicTableSize) throws Http2Exception {
hpackDynamicTable.setCapacity(dynamicTableSize);
}

private HeaderType validate(CharSequence name, HeaderType previousHeaderType,
final boolean validateHeaders) throws Http2Exception {
if (!validateHeaders) {
return null;
}

private static HeaderType validate(int streamId, CharSequence name,
HeaderType previousHeaderType) throws Http2Exception {
if (hasPseudoHeaderFormat(name)) {
if (previousHeaderType == HeaderType.REGULAR_HEADER) {
throw connectionError(PROTOCOL_ERROR, "Pseudo-header field '%s' found after regular header.", name);
throw streamError(streamId, PROTOCOL_ERROR,
"Pseudo-header field '%s' found after regular header.", name);
}

final Http2Headers.PseudoHeaderName pseudoHeader = getPseudoHeader(name);
if (pseudoHeader == null) {
throw connectionError(PROTOCOL_ERROR, "Invalid HTTP/2 pseudo-header '%s' encountered.", name);
throw streamError(streamId, PROTOCOL_ERROR, "Invalid HTTP/2 pseudo-header '%s' encountered.", name);
}

final HeaderType currentHeaderType = pseudoHeader.isRequestOnly() ?
HeaderType.REQUEST_PSEUDO_HEADER : HeaderType.RESPONSE_PSEUDO_HEADER;
if (previousHeaderType != null && currentHeaderType != previousHeaderType) {
throw connectionError(PROTOCOL_ERROR, "Mix of request and response pseudo-headers.");
throw streamError(streamId, PROTOCOL_ERROR, "Mix of request and response pseudo-headers.");
}

return currentHeaderType;
Expand Down Expand Up @@ -435,8 +422,7 @@ private HpackHeaderField getIndexedHeader(int index) throws Http2Exception {
throw INDEX_HEADER_ILLEGAL_INDEX_VALUE;
}

private void insertHeader(Sink sink, CharSequence name, CharSequence value,
IndexType indexType) throws Http2Exception {
private void insertHeader(Sink sink, CharSequence name, CharSequence value, IndexType indexType) {
sink.appendToHeaderList(name, value);

switch (indexType) {
Expand Down Expand Up @@ -529,32 +515,55 @@ private enum HeaderType {

private interface Sink {
void appendToHeaderList(CharSequence name, CharSequence value);
void finish() throws Http2Exception;
}

private static final class Http2HeadersSink implements Sink {
private final Http2Headers headers;
private final long maxHeaderListSize;
private final int streamId;
private final boolean validate;
private long headersLength;
private boolean exceededMaxLength;
private HeaderType previousType;
private Http2Exception validationException;

public Http2HeadersSink(Http2Headers headers, long maxHeaderListSize) {
public Http2HeadersSink(int streamId, Http2Headers headers, long maxHeaderListSize, boolean validate) {
this.headers = headers;
this.maxHeaderListSize = maxHeaderListSize;
this.streamId = streamId;
this.validate = validate;
}

@Override
public void finish() throws Http2Exception {
if (exceededMaxLength) {
headerListSizeExceeded(streamId, maxHeaderListSize, true);
} else if (validationException != null) {
throw validationException;
}
}

@Override
public void appendToHeaderList(CharSequence name, CharSequence value) {
headersLength += HpackHeaderField.sizeOf(name, value);
if (headersLength > maxHeaderListSize) {
exceededMaxLength = true;
exceededMaxLength |= headersLength > maxHeaderListSize;

if (exceededMaxLength || validationException != null) {
// We don't store the header since we've already failed validation requirements.
return;
}
if (!exceededMaxLength) {
headers.add(name, value);

if (validate) {
try {
previousType = HpackDecoder.validate(streamId, name, previousType);
} catch (Http2Exception ex) {
validationException = ex;
return;
}
}
}

public boolean exceededMaxLength() {
return exceededMaxLength;
headers.add(name, value);
}
}
}
45 changes: 41 additions & 4 deletions codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
View file
Open in desktop
Expand Up @@ -544,7 +544,7 @@ public void unknownPseudoHeader() throws Exception {

Http2Headers decoded = new DefaultHttp2Headers();

expectedException.expect(Http2Exception.class);
expectedException.expect(Http2Exception.StreamException.class);
hpackDecoder.decode(1, in, decoded, true);
} finally {
in.release();
Expand Down Expand Up @@ -588,7 +588,7 @@ public void requestPseudoHeaderInResponse() throws Exception {

Http2Headers decoded = new DefaultHttp2Headers();

expectedException.expect(Http2Exception.class);
expectedException.expect(Http2Exception.StreamException.class);
hpackDecoder.decode(1, in, decoded, true);
} finally {
in.release();
Expand All @@ -608,7 +608,7 @@ public void responsePseudoHeaderInRequest() throws Exception {

Http2Headers decoded = new DefaultHttp2Headers();

expectedException.expect(Http2Exception.class);
expectedException.expect(Http2Exception.StreamException.class);
hpackDecoder.decode(1, in, decoded, true);
} finally {
in.release();
Expand All @@ -628,10 +628,47 @@ public void pseudoHeaderAfterRegularHeader() throws Exception {

Http2Headers decoded = new DefaultHttp2Headers();

expectedException.expect(Http2Exception.class);
expectedException.expect(Http2Exception.StreamException.class);
hpackDecoder.decode(1, in, decoded, true);
} finally {
in.release();
}
}

@Test
public void failedValidationDoesntCorruptHpack() throws Exception {
ByteBuf in1 = Unpooled.buffer(200);
ByteBuf in2 = Unpooled.buffer(200);
try {
HpackEncoder hpackEncoder = new HpackEncoder(true);

Http2Headers toEncode = new DefaultHttp2Headers();
toEncode.add(":method", "GET");
toEncode.add(":status", "200");
toEncode.add("foo", "bar");
hpackEncoder.encodeHeaders(1, in1, toEncode, NEVER_SENSITIVE);

Http2Headers decoded = new DefaultHttp2Headers();

try {
hpackDecoder.decode(1, in1, decoded, true);
fail("Should have thrown a StreamException");
} catch (Http2Exception.StreamException expected) {
assertEquals(1, expected.streamId());
}

// Do it again, this time without validation, to make sure the HPACK state is still sane.
decoded.clear();
hpackEncoder.encodeHeaders(1, in2, toEncode, NEVER_SENSITIVE);
hpackDecoder.decode(1, in2, decoded, false);

assertEquals(3, decoded.size());
assertEquals("GET", decoded.method().toString());
assertEquals("200", decoded.status().toString());
assertEquals("bar", decoded.get("foo").toString());
} finally {
in1.release();
in2.release();
}
}
}