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

Reject HTTP/2 header values with invalid characters #12760

Merged
merged 15 commits into from Sep 21, 2022
Merged

Reject HTTP/2 header values with invalid characters #12760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7804234
Validate HTTP/2 header values
chrisvest Aug 31, 2022
ba309af
Specialize HPACK header validation for AsciiString and CharSequence
chrisvest Sep 2, 2022
7a64b63
Address some comments from #12755
chrisvest Sep 2, 2022
1e50b06
Fix HTTP/2 header value validation bug caused by signed bytes
chrisvest Sep 2, 2022
05fccc2
Include the offending byte and index when HTTP/2 header value validat…
chrisvest Sep 3, 2022
6167cb2
Make header value validation a separate toggle
chrisvest Sep 6, 2022
0cd6d05
Fix backwards compatibility
chrisvest Sep 6, 2022
392e6e0
Address some small review comments
chrisvest Sep 7, 2022
cdf3258
Specialize validation in HpackDecoder since it only produce AsciiStri…
chrisvest Sep 7, 2022
fcd4f4f
Address some more comments
chrisvest Sep 8, 2022
9cc54d6
Make illegal http header char tests parameterized
chrisvest Sep 12, 2022
23620a6
Consolidate HTTP header value and name validation
chrisvest Sep 14, 2022
bd406d3
Small javadoc update
chrisvest Sep 16, 2022
73d119f
Address PR review comments
chrisvest Sep 20, 2022
e45ea6a
Address nit
chrisvest Sep 21, 2022
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
10 changes: 6 additions & 4 deletions codec-http/src/main/java/io/netty/handler/codec/http/CombinedHttpHeaders.java
View file
Open in desktop
Expand Up @@ -38,7 +38,8 @@
*/
public class CombinedHttpHeaders extends DefaultHttpHeaders {
public CombinedHttpHeaders(boolean validate) {
super(new CombinedHttpHeadersImpl(CASE_INSENSITIVE_HASHER, valueConverter(validate), nameValidator(validate)));
super(new CombinedHttpHeadersImpl(CASE_INSENSITIVE_HASHER, valueConverter(), nameValidator(validate),
valueValidator(validate)));
}

@Override
Expand Down Expand Up @@ -87,9 +88,10 @@ public CharSequence escape(CharSequence name, CharSequence value) {
}

CombinedHttpHeadersImpl(HashingStrategy<CharSequence> nameHashingStrategy,
ValueConverter<CharSequence> valueConverter,
DefaultHeaders.NameValidator<CharSequence> nameValidator) {
super(nameHashingStrategy, valueConverter, nameValidator);
ValueConverter<CharSequence> valueConverter,
NameValidator<CharSequence> nameValidator,
ValueValidator<CharSequence> valueValidator) {
super(nameHashingStrategy, valueConverter, nameValidator, 16, valueValidator);
}

@Override
Expand Down
177 changes: 27 additions & 150 deletions codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpHeaders.java
View file
Open in desktop
Expand Up @@ -22,9 +22,6 @@
import io.netty.handler.codec.DefaultHeadersImpl;
import io.netty.handler.codec.HeadersUtils;
import io.netty.handler.codec.ValueConverter;
import io.netty.util.AsciiString;
import io.netty.util.ByteProcessor;
import io.netty.util.internal.PlatformDependent;

import java.util.ArrayList;
import java.util.Calendar;
Expand All @@ -43,31 +40,17 @@
* Default implementation of {@link HttpHeaders}.
*/
public class DefaultHttpHeaders extends HttpHeaders {
private static final int HIGHEST_INVALID_VALUE_CHAR_MASK = ~15;
private static final ByteProcessor HEADER_NAME_VALIDATOR = new ByteProcessor() {
@Override
public boolean process(byte value) throws Exception {
validateHeaderNameElement(value);
return true;
}
};
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 + "]");
throw new IllegalArgumentException("empty headers are not allowed [" + name + ']');
}
if (name instanceof AsciiString) {
try {
((AsciiString) name).forEachByte(HEADER_NAME_VALIDATOR);
} catch (Exception e) {
PlatformDependent.throwException(e);
}
} else {
// Go through each character in the name
for (int index = 0; index < name.length(); ++index) {
validateHeaderNameElement(name.charAt(index));
}
int index = HttpHeaderValidationUtil.validateToken(name);
if (index != -1) {
throw new IllegalArgumentException("a header name can only contain \"token\" characters, " +
"but found invalid character 0x" + Integer.toHexString(name.charAt(index)) +
" at index " + index + " of header '" + name + "'.");
}
}
};
Expand All @@ -79,7 +62,7 @@ public DefaultHttpHeaders() {
}

/**
* <b>Warning!</b> Setting <code>validate</code> to <code>false</code> will mean that Netty won't
* <b>Warning!</b> Setting {@code validate} to {@code false} will mean that Netty won't
* validate & protect against user-supplied header values that are malicious.
* This can leave your server implementation vulnerable to
* <a href="https://cwe.mitre.org/data/definitions/113.html">
Expand All @@ -88,16 +71,19 @@ public DefaultHttpHeaders() {
* When disabling this validation, it is the responsibility of the caller to ensure that the values supplied
* do not contain a non-url-escaped carriage return (CR) and/or line feed (LF) characters.
*
* @param validate Should Netty validate Header values to ensure they aren't malicious.
* @param validate Should Netty validate header values to ensure they aren't malicious.
*/
public DefaultHttpHeaders(boolean validate) {
this(validate, nameValidator(validate));
}

protected DefaultHttpHeaders(boolean validate, NameValidator<CharSequence> nameValidator) {
this(new DefaultHeadersImpl<CharSequence, CharSequence>(CASE_INSENSITIVE_HASHER,
valueConverter(validate),
nameValidator));
this(new DefaultHeadersImpl<CharSequence, CharSequence>(
CASE_INSENSITIVE_HASHER,
HeaderValueConverter.INSTANCE,
nameValidator,
16,
valueValidator(validate)));
}

protected DefaultHttpHeaders(DefaultHeaders<CharSequence, CharSequence, ?> headers) {
Expand Down Expand Up @@ -365,65 +351,14 @@ public HttpHeaders copy() {
return new DefaultHttpHeaders(headers.copy());
}

private static void validateHeaderNameElement(byte value) {
switch (value) {
case 0x1c:
case 0x1d:
case 0x1e:
case 0x1f:
case 0x00:
case '\t':
case '\n':
case 0x0b:
case '\f':
case '\r':
case ' ':
case ',':
case ':':
case ';':
case '=':
throw new IllegalArgumentException(
"a header name cannot contain the following prohibited characters: =,;: \\t\\r\\n\\v\\f: " +
value);
default:
// Check to see if the character is not an ASCII character, or invalid
if (value < 0) {
throw new IllegalArgumentException("a header name cannot contain non-ASCII character: " + value);
}
}
}

private static void validateHeaderNameElement(char value) {
switch (value) {
case 0x1c:
case 0x1d:
case 0x1e:
case 0x1f:
case 0x00:
case '\t':
case '\n':
case 0x0b:
case '\f':
case '\r':
case ' ':
case ',':
case ':':
case ';':
case '=':
throw new IllegalArgumentException(
"a header name cannot contain the following prohibited characters: =,;: \\t\\r\\n\\v\\f: " +
value);
default:
// Check to see if the character is not an ASCII character, or invalid
if (value > 127) {
throw new IllegalArgumentException("a header name cannot contain non-ASCII character: " +
value);
}
}
static ValueConverter<CharSequence> valueConverter() {
return HeaderValueConverter.INSTANCE;
}

static ValueConverter<CharSequence> valueConverter(boolean validate) {
return validate ? HeaderValueConverterAndValidator.INSTANCE : HeaderValueConverter.INSTANCE;
@SuppressWarnings("unchecked")
static DefaultHeaders.ValueValidator<CharSequence> valueValidator(boolean validate) {
return validate ? HeaderValueValidator.INSTANCE :
(DefaultHeaders.ValueValidator<CharSequence>) DefaultHeaders.ValueValidator.NO_VALIDATION;
}

@SuppressWarnings("unchecked")
Expand All @@ -449,74 +384,16 @@ public CharSequence convertObject(Object value) {
}
}

private static final class HeaderValueConverterAndValidator extends HeaderValueConverter {
static final HeaderValueConverterAndValidator INSTANCE = new HeaderValueConverterAndValidator();
private static final class HeaderValueValidator implements DefaultHeaders.ValueValidator<CharSequence> {
static final HeaderValueValidator INSTANCE = new HeaderValueValidator();

@Override
public CharSequence convertObject(Object value) {
CharSequence seq = super.convertObject(value);
int state = 0;
// Start looping through each of the character
for (int index = 0; index < seq.length(); index++) {
state = validateValueChar(seq, state, seq.charAt(index));
}

if (state != 0) {
throw new IllegalArgumentException("a header value must not end with '\\r' or '\\n'");
}
return seq;
}

private static int validateValueChar(CharSequence seq, int state, char character) {
/*
* State:
* 0: Previous character was neither CR nor LF
* 1: The previous character was CR
* 2: The previous character was LF
*/
if ((character & HIGHEST_INVALID_VALUE_CHAR_MASK) == 0) {
// Check the absolutely prohibited characters.
switch (character) {
case 0x0: // NULL
throw new IllegalArgumentException("a header value contains a prohibited character '\0'");
case 0x0b: // Vertical tab
throw new IllegalArgumentException("a header value contains a prohibited character '\\v'");
case '\f':
throw new IllegalArgumentException("a header value contains a prohibited character '\\f'");
default:
break;
}
}

// Check the CRLF (HT | SP) pattern
switch (state) {
case 0:
switch (character) {
case '\r':
return 1;
case '\n':
return 2;
default:
break;
}
break;
case 1:
if (character == '\n') {
return 2;
}
throw new IllegalArgumentException("only '\\n' is allowed after '\\r'");
case 2:
switch (character) {
case '\t':
case ' ':
return 0;
default:
throw new IllegalArgumentException("only ' ' and '\\t' are allowed after '\\n'");
}
default:
break;
public void validate(CharSequence value) {
int index = HttpHeaderValidationUtil.validateValidHeaderValue(value);
Copy link

Choose a reason for hiding this comment

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

should the same validation rules be applied for both HTTP/1 and HTTP/2? This change introduces backward compatibility issues

Copy link
Member

Choose a reason for hiding this comment

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

@chrisvest this looks like omission. value validation should be off by default for all protocols with opt-in flag. I see that for DefaultHttp2Headers but not for `DefaultHttpHeaders

if (index != -1) {
throw new IllegalArgumentException("a header value contains prohibited character 0x" +
Integer.toHexString(value.charAt(index)) + " at index " + index + '.');
}
return state;
}
}
}