Skip to content

Commit

Permalink
JBEAP-9909 Prevent HTTP smuggling attacks by making sure messages do …
Browse files Browse the repository at this point in the history
…not contain invalid headers.

Also verify that there is at most one Content-Length and Transfer-Encoding header
  • Loading branch information
stuartwdouglas committed Jun 8, 2017
1 parent 5bb280c commit 5b008b7
Show file tree
Hide file tree
Showing 14 changed files with 302 additions and 15 deletions.
12 changes: 12 additions & 0 deletions core/src/main/java/io/undertow/UndertowMessages.java
Original file line number Diff line number Diff line change
Expand Up @@ -513,4 +513,16 @@ public interface UndertowMessages {

@Message(id = 161, value = "HTTP/2 header block is too large")
String headerBlockTooLarge();

@Message(id = 162, value = "Same-site attribute %s is invalid. It must be Strict or Lax")
IllegalArgumentException invalidSameSiteMode(String mode);

@Message(id = 163, value = "Invalid token %s")
IllegalArgumentException invalidToken(byte c);

@Message(id = 164, value = "Request contained invalid headers")
IllegalArgumentException invalidHeaders();

@Message(id = 165, value = "Invalid character %s in request-target")
String invalidCharacterInRequestTarget(char next);
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import io.undertow.UndertowLogger;

import io.undertow.UndertowMessages;
import io.undertow.server.Connectors;
import io.undertow.util.HeaderMap;
import io.undertow.util.Headers;
import io.undertow.util.HttpString;
Expand Down Expand Up @@ -177,6 +178,9 @@ public void emitHeader(HttpString name, String value, boolean neverIndex) throws
if(c>= 'A' && c <= 'Z') {
invalid = true;
UndertowLogger.REQUEST_LOGGER.debugf("Malformed request, header %s contains uppercase characters", name);
} else if(c != ':' && !Connectors.isValidTokenCharacter(c)) {
invalid = true;
UndertowLogger.REQUEST_LOGGER.debugf("Malformed request, header %s contains invalid token character", name);
}
}

Expand Down
76 changes: 76 additions & 0 deletions core/src/main/java/io/undertow/server/Connectors.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,14 @@
package io.undertow.server;

import io.undertow.UndertowLogger;
import io.undertow.UndertowMessages;
import io.undertow.UndertowOptions;
import io.undertow.server.handlers.Cookie;
import io.undertow.util.DateUtils;
import io.undertow.util.HeaderMap;
import io.undertow.util.HeaderValues;
import io.undertow.util.Headers;
import io.undertow.util.HttpString;
import io.undertow.util.ParameterLimitException;
import io.undertow.util.StatusCodes;
import io.undertow.util.URLUtils;
Expand All @@ -46,7 +50,40 @@
*/
public class Connectors {

private static final boolean[] ALLOWED_TOKEN_CHARACTERS = new boolean[256];

static {
for(int i = 0; i < ALLOWED_TOKEN_CHARACTERS.length; ++i) {
if((i >='0' && i <= '9') ||
(i >='a' && i <= 'z') ||
(i >='A' && i <= 'Z')) {
ALLOWED_TOKEN_CHARACTERS[i] = true;
} else {
switch (i) {
case '!':
case '#':
case '$':
case '%':
case '&':
case '\'':
case '*':
case '+':
case '-':
case '.':
case '^':
case '_':
case '`':
case '|':
case '~': {
ALLOWED_TOKEN_CHARACTERS[i] = true;
break;
}
default:
ALLOWED_TOKEN_CHARACTERS[i] = false;
}
}
}
}
/**
* Flattens the exchange cookie map into the response header map. This should be called by a
* connector just before the response is started.
Expand Down Expand Up @@ -363,4 +400,43 @@ public static void updateResponseBytesSent(HttpServerExchange exchange, long byt
public static ConduitStreamSinkChannel getConduitSinkChannel(HttpServerExchange exchange) {
return exchange.getConnection().getSinkChannel();
}

/**
* Verifies that the contents of the HttpString are a valid token according to rfc7230.
* @param header The header to verify
*/
public static void verifyToken(HttpString header) {
int length = header.length();
for(int i = 0; i < length; ++i) {
byte c = header.byteAt(i);
if(!ALLOWED_TOKEN_CHARACTERS[c]) {
throw UndertowMessages.MESSAGES.invalidToken(c);
}
}
}

/**
* Returns true if the token character is valid according to rfc7230
*/
public static boolean isValidTokenCharacter(byte c) {
return ALLOWED_TOKEN_CHARACTERS[c];
}


/**
* Verifies that the provided request headers are valid according to rfc7230. In particular:
* - At most one content-length or transfer encoding
*/
public static boolean areRequestHeadersValid(HeaderMap headers) {
HeaderValues te = headers.get(Headers.TRANSFER_ENCODING);
HeaderValues cl = headers.get(Headers.CONTENT_LENGTH);
if(te != null && cl != null) {
return false;
} else if(te != null && te.size() > 1) {
return false;
} else if(cl != null && cl.size() > 1) {
return false;
}
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,10 @@ public void handleEvent(AjpServerResponseConduit channel) {
if(connectorStatistics != null) {
connectorStatistics.setup(httpServerExchange);
}
if(!Connectors.areRequestHeadersValid(httpServerExchange.getRequestHeaders())) {
oldState.badRequest = true;
UndertowLogger.REQUEST_IO_LOGGER.debugf("Invalid AJP request from %s, request contained invalid headers", connection.getPeerAddress());
}

if(oldState.badRequest) {
httpServerExchange.setStatusCode(StatusCodes.BAD_REQUEST);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import io.undertow.UndertowLogger;
import io.undertow.UndertowMessages;
import io.undertow.security.impl.ExternalAuthenticationMechanism;
import io.undertow.server.Connectors;
import io.undertow.server.HttpServerExchange;
import io.undertow.util.Headers;
import io.undertow.util.HttpString;
Expand Down Expand Up @@ -345,6 +346,7 @@ public void parse(final ByteBuffer buf, final AjpRequestParseState state, final
state.currentHeader = result.header;
} else {
state.currentHeader = HttpString.tryFromString(result.value);
Connectors.verifyToken(state.currentHeader);
}
}
StringHolder result = parseString(buf, state, StringType.OTHER);
Expand Down Expand Up @@ -423,7 +425,9 @@ public void parse(final ByteBuffer buf, final AjpRequestParseState state, final
} else if (state.currentAttribute.equals(AUTH_TYPE)) {
exchange.putAttachment(ExternalAuthenticationMechanism.EXTERNAL_AUTHENTICATION_TYPE, result);
} else if (state.currentAttribute.equals(STORED_METHOD)) {
exchange.setRequestMethod(new HttpString(result));
HttpString requestMethod = new HttpString(result);
Connectors.verifyToken(requestMethod);
exchange.setRequestMethod(requestMethod);
} else if (state.currentAttribute.equals(AJP_REMOTE_PORT)) {
state.remotePort = Integer.parseInt(result);
} else if (state.currentAttribute.equals(SSL_SESSION)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,10 @@ public void handleEventWithNoRunningRequest(final ConduitStreamSourceChannel cha
return;
}
}
if(!Connectors.areRequestHeadersValid(httpServerExchange.getRequestHeaders())) {
sendBadRequestAndClose(connection.getChannel(), UndertowMessages.MESSAGES.invalidHeaders());
return;
}
Connectors.executeRootHandler(connection.getRootHandler(), httpServerExchange);
} catch (Exception e) {
sendBadRequestAndClose(connection.getChannel(), e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,37 @@ public abstract class HttpRequestParser {
private final String charset;
private final int maxCachedHeaderSize;

private static final boolean[] ALLOWED_TARGET_CHARACTER = new boolean[256];

static {
try {
HTTP = "HTTP/1.".getBytes("ASCII");
HTTP_LENGTH = HTTP.length;
} catch (UnsupportedEncodingException e) {
throw new RuntimeException(e);
}
for(int i = 0; i < 256; ++i) {
if(i < 32 || i > 126) {
ALLOWED_TARGET_CHARACTER[i] = false;
} else {
switch ((char)i) {
case '\"':
case '#':
case '<':
case '>':
case '\\':
case '^':
case '`':
case '{':
case '|':
case '}':
ALLOWED_TARGET_CHARACTER[i] = false;
break;
default:
ALLOWED_TARGET_CHARACTER[i] = true;
}
}
}
}

public HttpRequestParser(OptionMap options) {
Expand Down Expand Up @@ -348,6 +372,9 @@ final void handlePath(ByteBuffer buffer, ParseState state, HttpServerExchange ex

while (buffer.hasRemaining()) {
char next = (char) (buffer.get() & 0xFF);
if(!ALLOWED_TARGET_CHARACTER[next]) {
throw new BadRequestException(UndertowMessages.MESSAGES.invalidCharacterInRequestTarget(next));
}
if (next == ' ' || next == '\t') {
if (stringBuilder.length() != 0) {
final String path = stringBuilder.toString();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,11 @@ private void handleRequests(Http2Channel channel, Http2StreamSourceChannel frame
exchange.setProtocol(Protocols.HTTP_2_0);
exchange.setRequestMethod(Methods.fromString(exchange.getRequestHeaders().getFirst(METHOD)));
exchange.getRequestHeaders().put(Headers.HOST, exchange.getRequestHeaders().getFirst(AUTHORITY));
if(!Connectors.areRequestHeadersValid(exchange.getRequestHeaders())) {
UndertowLogger.REQUEST_IO_LOGGER.debugf("Invalid headers in HTTP/2 request, closing connection. Remote peer %s", connection.getPeerAddress());
channel.sendGoAway(Http2Channel.ERROR_PROTOCOL_ERROR);
return;
}

final String path = exchange.getRequestHeaders().getFirst(PATH);
if(path == null || path.isEmpty()) {
Expand Down
6 changes: 5 additions & 1 deletion core/src/main/java/io/undertow/util/Methods.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import java.util.HashMap;
import java.util.Map;

import io.undertow.server.Connectors;

/**
* NOTE: If you add a new method here you must also add it to {@link io.undertow.server.protocol.http.HttpRequestParser}
*
Expand Down Expand Up @@ -138,7 +140,9 @@ private static void putString(Map<String, HttpString> methods, HttpString option
public static HttpString fromString(String method) {
HttpString res = METHODS.get(method);
if(res == null) {
return new HttpString(method);
HttpString httpString = new HttpString(method);
Connectors.verifyToken(httpString);
return httpString;
}
return res;
}
Expand Down
134 changes: 134 additions & 0 deletions core/src/test/java/io/undertow/server/InvalidHtpRequestTestCase.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
/*
* JBoss, Home of Professional Open Source.
* Copyright 2014 Red Hat, Inc., and individual contributors
* as indicated by the @author tags.
*
* Licensed 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.undertow.server;

import java.io.IOException;
import java.net.URI;
import java.net.URISyntaxException;

import org.apache.http.HttpResponse;
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpRequestBase;
import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
import org.junit.runner.RunWith;
import io.undertow.server.handlers.ResponseCodeHandler;
import io.undertow.testutils.DefaultServer;
import io.undertow.testutils.HttpOneOnly;
import io.undertow.testutils.ProxyIgnore;
import io.undertow.testutils.TestHttpClient;
import io.undertow.util.Headers;
import io.undertow.util.StatusCodes;

/**
* @author Stuart Douglas
*/
@RunWith(DefaultServer.class)
@ProxyIgnore
@HttpOneOnly
public class InvalidHtpRequestTestCase {

@BeforeClass
public static void setup() {
DefaultServer.setRootHandler(ResponseCodeHandler.HANDLE_200);
}

@Test
public void testInvalidCharacterInMethod() throws IOException {
final TestHttpClient client = new TestHttpClient();
try {
HttpRequestBase method = new HttpRequestBase() {

@Override
public String getMethod() {
return "GET;POST";
}

@Override
public URI getURI() {
try {
return new URI(DefaultServer.getDefaultServerURL());
} catch (URISyntaxException e) {
throw new RuntimeException(e);
}
}
};
HttpResponse result = client.execute(method);
Assert.assertEquals(StatusCodes.BAD_REQUEST, result.getStatusLine().getStatusCode());
} finally {
client.getConnectionManager().shutdown();
}
}


@Test
public void testInvalidCharacterInHeader() throws IOException {
final TestHttpClient client = new TestHttpClient();
try {
HttpRequestBase method = new HttpGet(DefaultServer.getDefaultServerURL());
method.addHeader("fake;header", "value");
HttpResponse result = client.execute(method);
Assert.assertEquals(StatusCodes.BAD_REQUEST, result.getStatusLine().getStatusCode());
} finally {
client.getConnectionManager().shutdown();
}
}

@Test
public void testMultipleContentLengths() throws IOException {
final TestHttpClient client = new TestHttpClient();
try {
HttpRequestBase method = new HttpGet(DefaultServer.getDefaultServerURL());
method.addHeader(Headers.CONTENT_LENGTH_STRING, "0");
method.addHeader(Headers.CONTENT_LENGTH_STRING, "10");
HttpResponse result = client.execute(method);
Assert.assertEquals(StatusCodes.BAD_REQUEST, result.getStatusLine().getStatusCode());
} finally {
client.getConnectionManager().shutdown();
}
}
@Test
public void testContentLengthAndTransferEncoding() throws IOException {
final TestHttpClient client = new TestHttpClient();
try {
HttpRequestBase method = new HttpGet(DefaultServer.getDefaultServerURL());
method.addHeader(Headers.CONTENT_LENGTH_STRING, "0");
method.addHeader(Headers.TRANSFER_ENCODING_STRING, "chunked");
HttpResponse result = client.execute(method);
Assert.assertEquals(StatusCodes.BAD_REQUEST, result.getStatusLine().getStatusCode());
} finally {
client.getConnectionManager().shutdown();
}
}

@Test
public void testMultipleTransferEncoding() throws IOException {
final TestHttpClient client = new TestHttpClient();
try {
HttpRequestBase method = new HttpGet(DefaultServer.getDefaultServerURL());
method.addHeader(Headers.TRANSFER_ENCODING_STRING, "chunked");
method.addHeader(Headers.TRANSFER_ENCODING_STRING, "gzip, chunked");
HttpResponse result = client.execute(method);
Assert.assertEquals(StatusCodes.BAD_REQUEST, result.getStatusLine().getStatusCode());
} finally {
client.getConnectionManager().shutdown();
}
}
}
Loading

0 comments on commit 5b008b7

Please # to comment.