Skip to content

Commit

Permalink
Add tests for HTTP request smuggling in headers (#1126)
Browse files Browse the repository at this point in the history 
Motivation:
HTTP uses a non-binary framing layer that is subject to request
smuggling. Our decoding relies upon the terminal newline characters to
frame requests and we should have tests to exercise behavior in cases
where a request is injected into a header value.

Modifications:
- Add HttpObjectDecoder tests which attempt to smuggle
requests/responses in headers

Result:
More tests verifying smuggling behavior.
  • Loading branch information
@Scottmitch
Scottmitch committed Aug 18, 2020
1 parent 22dd975 commit f795959
Show file tree
Hide file tree
Showing 4 changed files with 154 additions and 2 deletions.
2 changes: 1 addition & 1 deletion servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/HttpObjectDecoder.java
View file
Expand Up @@ -735,7 +735,7 @@ private static int findCRLF(final ByteBuf buffer, int startIndex, final int maxL
final int lfIndex = findLF(buffer, startIndex, toIndex);
if (lfIndex == -1) {
if (toIndex - startIndex == maxLineSize) {
throw new IllegalStateException("Could not find CRLF within " + maxLineSize + " bytes");
throw new IllegalArgumentException("Could not find CRLF within " + maxLineSize + " bytes");
}
return -2;
} else if (lfIndex == buffer.readerIndex()) {
Expand Down
74 changes: 73 additions & 1 deletion servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpObjectDecoderTest.java
View file
Expand Up @@ -445,7 +445,7 @@ public void chunkedNoTrailersNoChunkCRLF() {
writeContent(chunkLength);
// we omit writing the "\r\n" after chunk-data intentionally
DecoderException e = assertThrows(DecoderException.class, this::writeLastChunk);
assertThat(e.getCause(), is(instanceOf(IllegalStateException.class)));
assertThat(e.getCause(), is(instanceOf(IllegalArgumentException.class)));
assertThat(e.getCause().getMessage(), startsWith("Could not find CRLF"));
assertThat(channel().inboundMessages(), is(not(empty())));
}
Expand Down Expand Up @@ -502,4 +502,76 @@ public void unexpectedTrailersAfterContentLength() {
assertThat(e.getCause(), is(instanceOf(IllegalArgumentException.class)));
assertThat(channel().inboundMessages(), is(not(empty())));
}

@Test
public void smuggleBeforeZeroContentLengthHeader() {
smuggleZeroContentLength(true);
}

@Test
public void smuggleAfterZeroContentLengthHeader() {
smuggleZeroContentLength(false);
}

private void smuggleZeroContentLength(boolean smuggleBeforeContentLength) {
DecoderException e = assertThrows(DecoderException.class, () -> writeMsg(startLine() + "\r\n" +
"Host: servicetalk.io" + "\r\n" +
// Smuggled requests injected into a header will terminate the current request due to valid \r\n\r\n
// framing terminating the request with no content-length or transfer-encoding, or with known zero
// content-length [1].
// [1] https://tools.ietf.org/html/rfc7230#section-3.3.3
(smuggleBeforeContentLength ?
"Smuggled: " + startLine() + "\r\n\r\n" + "Content-Length: 0" + "\r\n" :
"Content-Length: 0" + "\r\n" + "Smuggled: " + startLine() + "\r\n\r\n") +
"Connection: keep-alive" + "\r\n\r\n"));
assertThat(e.getCause(), is(instanceOf(IllegalArgumentException.class)));

HttpMetaData metaData = assertStartLine();
assertSingleHeaderValue(metaData.headers(), HOST, "servicetalk.io");
assertSingleHeaderValue(metaData.headers(), "Smuggled", startLine());
assertEmptyTrailers(channel());
}

@Test
public void smuggleAfterTransferEncodingHeader() {
smuggleTransferEncoding(false);
}

protected void smuggleTransferEncoding(boolean smuggleBeforeTransferEncoding) {
DecoderException e = assertThrows(DecoderException.class, () -> writeMsg(startLineForContent() + "\r\n" +
"Host: servicetalk.io" + "\r\n" +
// Smuggled requests injected into a header will terminate the current request due to valid \r\n\r\n
// framing terminating the request with no content-length or transfer-encoding, or with known zero
// content-length [1].
// [1] https://tools.ietf.org/html/rfc7230#section-3.3.3
(smuggleBeforeTransferEncoding ?
"Smuggled: " + startLine() + "\r\n\r\n" + TRANSFER_ENCODING + ":" + CHUNKED + "\r\n" :
TRANSFER_ENCODING + ":" + CHUNKED + "\r\n" + "Smuggled: " + startLine() + "\r\n\r\n") +
"Connection: keep-alive" + "\r\n\r\n"));
assertThat(e.getCause(), is(instanceOf(IllegalArgumentException.class)));

HttpMetaData metaData = assertStartLineForContent();
assertSingleHeaderValue(metaData.headers(), HOST, "servicetalk.io");
assertSingleHeaderValue(metaData.headers(), "Smuggled", startLine());
}

@Test
public void smuggleNameBeforeNonZeroContentLengthHeader() {
smuggleNameZeroContentLengthHeader(true);
}

@Test
public void smuggleNameAfterNonZeroContentLengthHeader() {
smuggleNameZeroContentLengthHeader(false);
}

private void smuggleNameZeroContentLengthHeader(boolean smuggleBeforeContentLength) {
DecoderException e = assertThrows(DecoderException.class, () -> writeMsg(startLineForContent() + "\r\n" +
"Host: servicetalk.io" + "\r\n" +
(smuggleBeforeContentLength ?
startLine() + "\r\n\r\n" + "Content-Length: 0" + "\r\n" :
"Content-Length: 0" + "\r\n" + startLine() + "\r\n\r\n") +
"Connection: keep-alive" + "\r\n\r\n"));
assertThat(e.getCause(), is(instanceOf(IllegalArgumentException.class)));
}
}
26 changes: 26 additions & 0 deletions servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpRequestDecoderTest.java
View file
Expand Up @@ -31,6 +31,7 @@

import static io.servicetalk.buffer.netty.BufferAllocators.DEFAULT_ALLOCATOR;
import static io.servicetalk.buffer.netty.BufferUtils.getByteBufAllocator;
import static io.servicetalk.http.api.HttpHeaderNames.HOST;
import static io.servicetalk.http.api.HttpProtocolVersion.HTTP_1_1;
import static io.servicetalk.http.api.HttpRequestMethod.GET;
import static io.servicetalk.http.api.HttpRequestMethod.POST;
Expand Down Expand Up @@ -283,4 +284,29 @@ private HttpRequestMetaData assertRequestLine(HttpRequestMethod expectedMethod,
assertThat(request.version(), equalTo(expectedVersion));
return request;
}

@Test
public void smuggleBeforeNonZeroContentLengthHeader() {
int contentLength = 128;
DecoderException e = assertThrows(DecoderException.class, () -> writeMsg(startLineForContent() + "\r\n" +
"Host: servicetalk.io" + "\r\n" +
// Smuggled requests injected into a header will terminate the current request due to valid \r\n\r\n
// framing terminating the request with no content-length or transfer-encoding, or with known zero
// content-length [1].
// [1] https://tools.ietf.org/html/rfc7230#section-3.3.3
"Smuggled: " + startLine() + "\r\n\r\n" +
"Content-Length: " + contentLength + "\r\n" +
"Connection: keep-alive" + "\r\n\r\n"));
assertThat(e.getCause(), is(instanceOf(IllegalArgumentException.class)));

HttpMetaData metaData = assertStartLineForContent();
assertSingleHeaderValue(metaData.headers(), HOST, "servicetalk.io");
assertSingleHeaderValue(metaData.headers(), "Smuggled", startLine());
assertEmptyTrailers(channel());
}

@Test
public void smuggleBeforeTransferEncodingHeader() {
smuggleTransferEncoding(true);
}
}
54 changes: 54 additions & 0 deletions servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/HttpResponseDecoderTest.java
View file
Expand Up @@ -30,6 +30,7 @@
*/
package io.servicetalk.http.netty;

import io.servicetalk.buffer.api.Buffer;
import io.servicetalk.http.api.DefaultHttpHeadersFactory;
import io.servicetalk.http.api.HttpMetaData;
import io.servicetalk.http.api.HttpProtocolVersion;
Expand All @@ -47,10 +48,14 @@

import static io.servicetalk.buffer.netty.BufferAllocators.DEFAULT_ALLOCATOR;
import static io.servicetalk.buffer.netty.BufferUtils.getByteBufAllocator;
import static io.servicetalk.http.api.HttpHeaderNames.HOST;
import static io.servicetalk.http.api.HttpHeaderNames.TRANSFER_ENCODING;
import static io.servicetalk.http.api.HttpHeaderValues.CHUNKED;
import static io.servicetalk.http.api.HttpProtocolVersion.HTTP_1_1;
import static io.servicetalk.http.api.HttpResponseStatus.NO_CONTENT;
import static io.servicetalk.http.api.HttpResponseStatus.OK;
import static java.lang.Integer.toHexString;
import static java.nio.charset.StandardCharsets.US_ASCII;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
Expand Down Expand Up @@ -320,4 +325,53 @@ private HttpResponseMetaData assertResponseLine(HttpProtocolVersion expectedVers
assertThat(response.status().reasonPhrase(), equalTo(expectedStatus.reasonPhrase()));
return response;
}

@Test
public void smuggleBeforeNonZeroContentLengthHeader() {
int contentLength = 128;
writeMsg(startLineForContent() + "\r\n" +
"Host: servicetalk.io" + "\r\n" +
// If a Transfer-Encoding header field is present in a response and
// the chunked transfer coding is not the final encoding, the
// message body length is determined by reading the connection until
// it is closed by the server [1].
// [1] https://tools.ietf.org/html/rfc7230#section-3.3.3
"Smuggled: " + startLine() + "\r\n\r\n" +
"Content-Length: " + contentLength + "\r\n" +
"Connection: keep-alive" + "\r\n\r\n");

HttpMetaData metaData = assertStartLineForContent();
assertSingleHeaderValue(metaData.headers(), HOST, "servicetalk.io");
assertSingleHeaderValue(metaData.headers(), "Smuggled", startLine());
Buffer buffer = channel().readInbound();
assertThat(buffer.toString(US_ASCII), is("Content-Length: " + contentLength +
"\r\nConnection: keep-alive\r\n\r\n"));
channel().close();
assertEmptyTrailers(channel());
assertFalse(channel().finishAndReleaseAll());
}

@Test
public void smuggleBeforeTransferEncodingHeader() {
writeMsg(startLineForContent() + "\r\n" +
"Host: servicetalk.io" + "\r\n" +
// Otherwise, this is a response message without a declared message
// body length, so the message body length is determined by the
// number of octets received prior to the server closing the
// connection [1].
// [1] https://tools.ietf.org/html/rfc7230#section-3.3.3
"Smuggled: " + startLine() + "\r\n\r\n" +
TRANSFER_ENCODING + ": " + CHUNKED + "\r\n" +
"Connection: keep-alive" + "\r\n\r\n");

HttpMetaData metaData = assertStartLineForContent();
assertSingleHeaderValue(metaData.headers(), HOST, "servicetalk.io");
assertSingleHeaderValue(metaData.headers(), "Smuggled", startLine());
Buffer buffer = channel().readInbound();
assertThat(buffer.toString(US_ASCII), is(TRANSFER_ENCODING + ": " + CHUNKED +
"\r\nConnection: keep-alive\r\n\r\n"));
channel().close();
assertEmptyTrailers(channel());
assertFalse(channel().finishAndReleaseAll());
}
}

0 comments on commit f795959

Please sign in to comment.