UNDERTOW-1165 Fix possble HTTP request smuggling vulnerability

This makes the parser much stricter in terms of sanitising its inputs
undertow-io · Mar 2, 2018 · 3436b03 · 3436b03
1 parent 26ab444
commit 3436b03
Show file tree

Hide file tree

Showing 8 changed files with 225 additions and 65 deletions.
diff --git a/core/src/main/java/io/undertow/client/http/HttpResponseParser.java b/core/src/main/java/io/undertow/client/http/HttpResponseParser.java
@@ -19,6 +19,7 @@
 package io.undertow.client.http;
 
 import io.undertow.annotationprocessor.HttpResponseParserConfig;
+import io.undertow.util.BadRequestException;
 import io.undertow.util.Headers;
 import io.undertow.util.HttpString;
 import io.undertow.util.Methods;
@@ -117,11 +118,11 @@ abstract class HttpResponseParser {
         }
     }
 
-    abstract void handleHttpVersion(ByteBuffer buffer, ResponseParseState currentState, HttpResponseBuilder builder);
+    abstract void handleHttpVersion(ByteBuffer buffer, ResponseParseState currentState, HttpResponseBuilder builder) throws BadRequestException;
 
-    abstract void handleHeader(ByteBuffer buffer, ResponseParseState currentState, HttpResponseBuilder builder);
+    abstract void handleHeader(ByteBuffer buffer, ResponseParseState currentState, HttpResponseBuilder builder) throws BadRequestException;
 
-    public void handle(final ByteBuffer buffer, final ResponseParseState currentState, final HttpResponseBuilder builder) {
+    public void handle(final ByteBuffer buffer, final ResponseParseState currentState, final HttpResponseBuilder builder) throws BadRequestException {
 
         if (currentState.state == ResponseParseState.VERSION) {
             handleHttpVersion(buffer, currentState, builder);

diff --git a/core/src/main/java/io/undertow/server/protocol/http/HttpRequestParser.java b/core/src/main/java/io/undertow/server/protocol/http/HttpRequestParser.java
@@ -239,7 +239,11 @@ public void handle(ByteBuffer buffer, final ParseState currentState, final HttpS
                 builder.setRequestMethod(Methods.GET);
                 currentState.state = ParseState.PATH;
             } else {
-                handleHttpVerb(buffer, currentState, builder);
+                try {
+                    handleHttpVerb(buffer, currentState, builder);
+                } catch (IllegalArgumentException e) {
+                    throw new BadRequestException(e);
+                }
             }
             handlePath(buffer, currentState, builder);
             boolean failed = false;
@@ -341,11 +345,11 @@ private void handleStateful(ByteBuffer buffer, ParseState currentState, HttpServ
     }
 
 
-    abstract void handleHttpVerb(ByteBuffer buffer, final ParseState currentState, final HttpServerExchange builder);
+    abstract void handleHttpVerb(ByteBuffer buffer, final ParseState currentState, final HttpServerExchange builder) throws BadRequestException;
 
-    abstract void handleHttpVersion(ByteBuffer buffer, final ParseState currentState, final HttpServerExchange builder);
+    abstract void handleHttpVersion(ByteBuffer buffer, final ParseState currentState, final HttpServerExchange builder) throws BadRequestException;
 
-    abstract void handleHeader(ByteBuffer buffer, final ParseState currentState, final HttpServerExchange builder);
+    abstract void handleHeader(ByteBuffer buffer, final ParseState currentState, final HttpServerExchange builder) throws BadRequestException;
 
     /**
      * The parse states for parsing the path.
@@ -515,6 +519,9 @@ final void handleQueryParameters(ByteBuffer buffer, ParseState state, HttpServer
 
         while (buffer.hasRemaining()) {
             char next = (char) (buffer.get() & 0xFF);
+            if(!allowUnescapedCharactersInUrl && !ALLOWED_TARGET_CHARACTER[next]) {
+                throw new BadRequestException(UndertowMessages.MESSAGES.invalidCharacterInRequestTarget(next));
+            }
             if (next == ' ' || next == '\t') {
                 final String queryString = stringBuilder.toString();
                 exchange.setQueryString(queryString);
@@ -595,6 +602,9 @@ final void handlePathParameters(ByteBuffer buffer, ParseState state, HttpServerE
 
         while (buffer.hasRemaining()) {
             char next = (char) (buffer.get() & 0xFF);
+            if(!allowUnescapedCharactersInUrl && !ALLOWED_TARGET_CHARACTER[next]) {
+                throw new BadRequestException(UndertowMessages.MESSAGES.invalidCharacterInRequestTarget(next));
+            }
             if (next == ' ' || next == '\t' || next == '?') {
                 if (nextQueryParam == null) {
                     if (queryParamPos != stringBuilder.length()) {

diff --git a/core/src/main/java/io/undertow/util/BadRequestException.java b/core/src/main/java/io/undertow/util/BadRequestException.java
@@ -25,6 +25,9 @@
  */
 public class BadRequestException extends Exception {
 
+    public BadRequestException() {
+    }
+
     public BadRequestException(String message) {
         super(message);
     }

diff --git a/core/src/test/java/io/undertow/client/http/ResponseParserResumeTestCase.java b/core/src/test/java/io/undertow/client/http/ResponseParserResumeTestCase.java
@@ -19,6 +19,7 @@
 package io.undertow.client.http;
 
 import io.undertow.testutils.category.UnitTest;
+import io.undertow.util.BadRequestException;
 import io.undertow.util.HttpString;
 import io.undertow.util.Protocols;
 import io.undertow.util.StatusCodes;
@@ -51,7 +52,7 @@ public void testMethodSplit() {
     }
 
     @Test
-    public void testOneCharacterAtATime() {
+    public void testOneCharacterAtATime() throws BadRequestException {
         byte[] in = DATA.getBytes();
         final ResponseParseState context = new ResponseParseState();
         HttpResponseBuilder result = new HttpResponseBuilder();
@@ -64,7 +65,7 @@ public void testOneCharacterAtATime() {
         runAssertions(result, context);
     }
 
-    private void testResume(final int split, byte[] in) {
+    private void testResume(final int split, byte[] in) throws BadRequestException {
         final ResponseParseState context = new ResponseParseState();
         HttpResponseBuilder result = new HttpResponseBuilder();
         ByteBuffer buffer = ByteBuffer.wrap(in);

diff --git a/core/src/test/java/io/undertow/server/protocol/http/SimpleParserTestCase.java b/core/src/test/java/io/undertow/server/protocol/http/SimpleParserTestCase.java
@@ -39,8 +39,6 @@
  * <p/>
  * This tests parsing the same basic request, over and over, with minor differences.
  * <p/>
- * Not all these actually conform to the HTTP/1.1 specification, however we are supposed to be
- * liberal in what we accept.
  *
  * @author Stuart Douglas
  */
@@ -133,13 +131,89 @@ public void testFullUrlRootPath() throws BadRequestException {
         Assert.assertEquals("/", result.getRequestPath());
         Assert.assertEquals("http://myurl.com", result.getRequestURI());
     }
+
+    @Test(expected = BadRequestException.class)
+    public void testLineEndingInsteadOfSpacesAfterVerb() throws BadRequestException {
+        byte[] in = "GET\r/somepath HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testLineEndingInsteadOfSpacesAfterPath() throws BadRequestException {
+        byte[] in = "GET /somepath\rHTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testLineEndingInsteadOfSpacesAfterVerb2() throws BadRequestException {
+        byte[] in = "GET\n/somepath HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testLineEndingInsteadOfSpacesAfterVerb3() throws BadRequestException {
+        byte[] in = "FOO\n/somepath HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+    @Test(expected = BadRequestException.class)
+    public void testLineEndingInsteadOfSpacesAfterPath2() throws BadRequestException {
+        byte[] in = "GET /somepath\nHTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
     @Test
     public void testSimpleRequest() throws BadRequestException {
         byte[] in = "GET /somepath HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
         runTest(in);
     }
 
+    @Test(expected = BadRequestException.class)
+    public void testTabInsteadOfSpaceAfterVerb() throws BadRequestException {
+        byte[] in = "GET\t/somepath HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testTabInsteadOfSpaceAfterVerb2() throws BadRequestException {
+        byte[] in = "FOO\t/somepath HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testTabInsteadOfSpaceAfterPath() throws BadRequestException {
+        byte[] in = "GET\t/somepath HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
 
+    @Test(expected = BadRequestException.class)
+    public void testInvalidCharacterInPath() throws BadRequestException {
+        byte[] in = "GET /some>path HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testInvalidCharacterInQueryString1() throws BadRequestException {
+        byte[] in = "GET /somepath?foo>f=bar HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testInvalidCharacterInQueryString2() throws BadRequestException {
+        byte[] in = "GET /somepath?foo=ba>r HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testInvalidCharacterInPathParam1() throws BadRequestException {
+        byte[] in = "GET /somepath;foo>f=bar HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
+
+    @Test(expected = BadRequestException.class)
+    public void testInvalidCharacterInPathParam2() throws BadRequestException {
+        byte[] in = "GET /somepath;foo=ba>r HTTP/1.1\r\nHost:   www.somehost.net\r\nOtherHeader: some\r\n    value\r\n\r\n".getBytes();
+        runTest(in);
+    }
 
     @Test
     public void testSimpleRequestWithHeaderCaching() throws BadRequestException {
@@ -197,7 +271,7 @@ public void testNoHeaders() throws BadRequestException {
 
     @Test
     public void testQueryParams() throws BadRequestException {
-        byte[] in = "GET\thttp://www.somehost.net/somepath?a=b&b=c&d&e&f=\tHTTP/1.1\nHost: \t www.somehost.net\nOtherHeader:\tsome\n \t  value\n\r\n".getBytes();
+        byte[] in = "GET http://www.somehost.net/somepath?a=b&b=c&d&e&f= HTTP/1.1\nHost: \t www.somehost.net\nOtherHeader:\tsome\n \t  value\n\r\n".getBytes();
 
         final ParseState context = new ParseState(10);
         HttpServerExchange result = new HttpServerExchange(null);