Skip to content

Commit

Permalink
Fix failing h2spec tests 8.1.2.1 related to pseudo-headers validation
Browse files Browse the repository at this point in the history 
Motivation:

According to the spec:
All pseudo-header fields MUST appear in the header block before regular
header fields. Any request or response that contains a pseudo-header
field that appears in a header block after
a regular header field MUST be treated as malformed (Section 8.1.2.6).

Pseudo-header fields are only valid in the context in which they are defined.
Pseudo-header fields defined for requests MUST NOT appear in responses;
pseudo-header fields defined for responses MUST NOT appear in requests.
Pseudo-header fields MUST NOT appear in trailers.
Endpoints MUST treat a request or response that contains undefined or
invalid pseudo-header fields as malformed (Section 8.1.2.6).

Clients MUST NOT accept a malformed response. Note that these requirements
are intended to protect against several types of common attacks against HTTP;
they are deliberately strict because being permissive can expose
implementations to these vulnerabilities.

Modifications:

- Introduce validation in HPackDecoder

Result:

- Requests with unknown pseudo-field headers are rejected
- Requests with containing response specific pseudo-headers are rejected
- Requests where pseudo-header appear after regular header are rejected
- h2spec 8.1.2.1 pass
  • Loading branch information
@Scottmitch
Julien Hoarau authored and Scottmitch committed Jan 30, 2018
1 parent c795e88 commit 3e6b54b
Show file tree
Hide file tree
Showing 12 changed files with 361 additions and 51 deletions.
13 changes: 8 additions & 5 deletions codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2Headers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,19 @@
import io.netty.util.internal.PlatformDependent;
import io.netty.util.internal.UnstableApi;

import static io.netty.handler.codec.http2.Http2Error.*;
import static io.netty.handler.codec.http2.Http2Exception.*;
import static io.netty.util.AsciiString.*;
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.Http2Headers.PseudoHeaderName.hasPseudoHeaderFormat;
import static io.netty.util.AsciiString.CASE_INSENSITIVE_HASHER;
import static io.netty.util.AsciiString.CASE_SENSITIVE_HASHER;
import static io.netty.util.AsciiString.isUpperCase;

@UnstableApi
public class DefaultHttp2Headers
extends DefaultHeaders<CharSequence, CharSequence, Http2Headers> implements Http2Headers {
private static final ByteProcessor HTTP2_NAME_VALIDATOR_PROCESSOR = new ByteProcessor() {
@Override
public boolean process(byte value) throws Exception {
public boolean process(byte value) {
return !isUpperCase(value);
}
};
Expand Down Expand Up @@ -207,7 +210,7 @@ protected Http2HeaderEntry(int hash, CharSequence key, CharSequence value,
this.next = next;

// Make sure the pseudo headers fields are first in iteration order
if (key.length() != 0 && key.charAt(0) == ':') {
if (hasPseudoHeaderFormat(key)) {
after = firstNonPseudo;
before = firstNonPseudo.before();
} else {
Expand Down
2 changes: 1 addition & 1 deletion codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ public Configuration configuration() {
public Http2Headers decodeHeaders(int streamId, ByteBuf headerBlock) throws Http2Exception {
try {
final Http2Headers headers = newHeaders();
hpackDecoder.decode(streamId, headerBlock, headers);
hpackDecoder.decode(streamId, headerBlock, headers, validateHeaders);
headerArraySizeAccumulator = HEADERS_COUNT_WEIGHT_NEW * headers.size() +
HEADERS_COUNT_WEIGHT_HISTORICAL * headerArraySizeAccumulator;
return headers;
Expand Down
65 changes: 57 additions & 8 deletions codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
import static io.netty.handler.codec.http2.Http2Error.INTERNAL_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.Http2Headers.PseudoHeaderName.getPseudoHeader;
import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.hasPseudoHeaderFormat;
import static io.netty.util.AsciiString.EMPTY_STRING;
import static io.netty.util.internal.ObjectUtil.checkPositive;
import static io.netty.util.internal.ThrowableUtil.unknownStackTrace;
Expand Down Expand Up @@ -119,14 +121,15 @@ final class HpackDecoder {
* <p>
* This method assumes the entire header block is contained in {@code in}.
*/
public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2Exception {
public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean validateHeaders) throws Http2Exception {
int index = 0;
long headersLength = 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 @@ -146,7 +149,10 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2E
state = READ_INDEXED_HEADER;
break;
default:
headersLength = indexHeader(index, headers, headersLength);
HpackHeaderField indexedHeader = getIndexedHeader(index);
headerType = validate(indexedHeader.name, headerType, validateHeaders);
headersLength = addHeader(headers, indexedHeader.name, indexedHeader.value,
headersLength);
}
} else if ((b & 0x40) == 0x40) {
// Literal Header Field with Incremental Indexing
Expand All @@ -162,6 +168,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2E
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 @@ -188,6 +195,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2E
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 @@ -200,13 +208,16 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2E
break;

case READ_INDEXED_HEADER:
headersLength = indexHeader(decodeULE128(in, index), headers, headersLength);
HpackHeaderField indexedHeader = getIndexedHeader(decodeULE128(in, index));
headerType = validate(indexedHeader.name, headerType, validateHeaders);
headersLength = addHeader(headers, indexedHeader.name, indexedHeader.value, headersLength);
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 @@ -243,6 +254,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2E
}

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

state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX;
break;
Expand All @@ -256,6 +268,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2E
state = READ_LITERAL_HEADER_VALUE_LENGTH;
break;
case 0:
headerType = validate(name, headerType, validateHeaders);
headersLength = insertHeader(headers, name, EMPTY_STRING, indexType, headersLength);
state = READ_HEADER_REPRESENTATION;
break;
Expand Down Expand Up @@ -288,6 +301,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers) throws Http2E
}

CharSequence value = readStringLiteral(in, valueLength, huffmanEncoded);
headerType = validate(name, headerType, validateHeaders);
headersLength = insertHeader(headers, name, value, indexType, headersLength);
state = READ_HEADER_REPRESENTATION;
break;
Expand Down Expand Up @@ -386,6 +400,34 @@ 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;
}

if (hasPseudoHeaderFormat(name)) {
if (previousHeaderType == HeaderType.REGULAR_HEADER) {
throw connectionError(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);
}

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.");
}

return currentHeaderType;
}

return HeaderType.REGULAR_HEADER;
}

private CharSequence readName(int index) throws Http2Exception {
if (index <= HpackStaticTable.length) {
HpackHeaderField hpackHeaderField = HpackStaticTable.getEntry(index);
Expand All @@ -398,14 +440,12 @@ private CharSequence readName(int index) throws Http2Exception {
throw READ_NAME_ILLEGAL_INDEX_VALUE;
}

private long indexHeader(int index, Http2Headers headers, long headersLength) throws Http2Exception {
private HpackHeaderField getIndexedHeader(int index) throws Http2Exception {
if (index <= HpackStaticTable.length) {
HpackHeaderField hpackHeaderField = HpackStaticTable.getEntry(index);
return addHeader(headers, hpackHeaderField.name, hpackHeaderField.value, headersLength);
return HpackStaticTable.getEntry(index);
}
if (index - HpackStaticTable.length <= hpackDynamicTable.length()) {
HpackHeaderField hpackHeaderField = hpackDynamicTable.getEntry(index - HpackStaticTable.length);
return addHeader(headers, hpackHeaderField.name, hpackHeaderField.value, headersLength);
return hpackDynamicTable.getEntry(index - HpackStaticTable.length);
}
throw INDEX_HEADER_ILLEGAL_INDEX_VALUE;
}
Expand Down Expand Up @@ -504,4 +544,13 @@ static long decodeULE128(ByteBuf in, long result) throws Http2Exception {

throw DECODE_ULE_128_DECOMPRESSION_EXCEPTION;
}

/**
* HTTP/2 header types.
*/
private enum HeaderType {
REGULAR_HEADER,
REQUEST_PSEUDO_HEADER,
RESPONSE_PSEUDO_HEADER
}
}
54 changes: 46 additions & 8 deletions codec-http2/src/main/java/io/netty/handler/codec/http2/Http2Headers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,51 +35,89 @@ enum PseudoHeaderName {
/**
* {@code :method}.
*/
METHOD(":method"),
METHOD(":method", true),

/**
* {@code :scheme}.
*/
SCHEME(":scheme"),
SCHEME(":scheme", true),

/**
* {@code :authority}.
*/
AUTHORITY(":authority"),
AUTHORITY(":authority", true),

/**
* {@code :path}.
*/
PATH(":path"),
PATH(":path", true),

/**
* {@code :status}.
*/
STATUS(":status");
STATUS(":status", false);

private static final char PSEUDO_HEADER_PREFIX = ':';
private static final byte PSEUDO_HEADER_PREFIX_BYTE = (byte) PSEUDO_HEADER_PREFIX;

private final AsciiString value;
private static final CharSequenceMap<AsciiString> PSEUDO_HEADERS = new CharSequenceMap<AsciiString>();
private final boolean requestOnly;
private static final CharSequenceMap<PseudoHeaderName> PSEUDO_HEADERS = new CharSequenceMap<PseudoHeaderName>();

static {
for (PseudoHeaderName pseudoHeader : PseudoHeaderName.values()) {
PSEUDO_HEADERS.add(pseudoHeader.value(), AsciiString.EMPTY_STRING);
PSEUDO_HEADERS.add(pseudoHeader.value(), pseudoHeader);
}
}

PseudoHeaderName(String value) {
PseudoHeaderName(String value, boolean requestOnly) {
this.value = AsciiString.cached(value);
this.requestOnly = requestOnly;
}

public AsciiString value() {
// Return a slice so that the buffer gets its own reader index.
return value;
}

/**
* Indicates whether the specified header follows the pseudo-header format (begins with ':' character)
*
* @return {@code true} if the header follow the pseudo-header format
*/
public static boolean hasPseudoHeaderFormat(CharSequence headerName) {
if (headerName instanceof AsciiString) {
final AsciiString asciiHeaderName = (AsciiString) headerName;
return asciiHeaderName.length() > 0 && asciiHeaderName.byteAt(0) == PSEUDO_HEADER_PREFIX_BYTE;
} else {
return headerName.length() > 0 && headerName.charAt(0) == PSEUDO_HEADER_PREFIX;
}
}

/**
* Indicates whether the given header name is a valid HTTP/2 pseudo header.
*/
public static boolean isPseudoHeader(CharSequence header) {
return PSEUDO_HEADERS.contains(header);
}

/**
* Returns the {@link PseudoHeaderName} corresponding to the specified header name.
*
* @return corresponding {@link PseudoHeaderName} if any, {@code null} otherwise.
*/
public static PseudoHeaderName getPseudoHeader(CharSequence header) {
return PSEUDO_HEADERS.get(header);
}

/**
* Indicates whether the pseudo-header is to be used in a request context.
*
* @return {@code true} if the pseudo-header is to be used in a request context
*/
public boolean isRequestOnly() {
return requestOnly;
}
}

/**
Expand Down
Loading

0 comments on commit 3e6b54b

Please sign in to comment.