This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 7.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 8f03960ca7e6e7a4b5ca2bd4c4daea45583b3e59 Author: Mark Thomas <ma...@apache.org> AuthorDate: Tue Mar 10 21:32:16 2020 +0000 Fix BZ 64210. Correct parsing of CRLF across packet boundaries, Also improve request line parsing including HTTP/0.9. --- .../coyote/http11/AbstractHttp11Processor.java | 43 +++--- .../apache/coyote/http11/AbstractInputBuffer.java | 6 +- .../coyote/http11/InternalAprInputBuffer.java | 55 ++++---- .../apache/coyote/http11/InternalInputBuffer.java | 53 ++++--- .../coyote/http11/InternalNioInputBuffer.java | 51 ++++--- .../coyote/http11/TestHttp11InputBufferCRLF.java | 156 +++++++++++++++++++++ .../coyote/http11/TestInternalInputBuffer.java | 101 +++++++++++-- webapps/docs/changelog.xml | 17 ++- 8 files changed, 371 insertions(+), 111 deletions(-) diff --git a/java/org/apache/coyote/http11/AbstractHttp11Processor.java b/java/org/apache/coyote/http11/AbstractHttp11Processor.java index 8453067..33c0b27 100644 --- a/java/org/apache/coyote/http11/AbstractHttp11Processor.java +++ b/java/org/apache/coyote/http11/AbstractHttp11Processor.java @@ -1111,6 +1111,11 @@ public abstract class AbstractHttp11Processor<S> extends AbstractProcessor<S> { } } + // Process the Protocol component of the request line + // Need to know if this is an HTTP 0.9 request before trying to + // parse headers. + prepareRequestProtocol(); + if (endpoint.isPaused()) { // 503 - Service unavailable response.setStatus(503); @@ -1121,7 +1126,8 @@ public abstract class AbstractHttp11Processor<S> extends AbstractProcessor<S> { request.getMimeHeaders().setLimit(endpoint.getMaxHeaderCount()); request.getCookies().setLimit(getMaxCookieCount()); // Currently only NIO will ever return false here - if (!getInputBuffer().parseHeaders()) { + // Don't parse headers for HTTP/0.9 + if (!http09 && !getInputBuffer().parseHeaders()) { // We've read part of the request, don't recycle it // instead associate it with the socket openSocket = true; @@ -1320,26 +1326,14 @@ public abstract class AbstractHttp11Processor<S> extends AbstractProcessor<S> { } - /** - * After reading the request headers, we have to setup the request filters. - */ - protected void prepareRequest() throws IOException { - - http11 = true; - http09 = false; - contentDelimitation = false; - expectation = false; - - prepareRequestInternal(); - - if (endpoint.isSSLEnabled()) { - request.scheme().setString("https"); - } + private void prepareRequestProtocol() { MessageBytes protocolMB = request.protocol(); if (protocolMB.equals(Constants.HTTP_11)) { + http09 = false; http11 = true; protocolMB.setString(Constants.HTTP_11); } else if (protocolMB.equals(Constants.HTTP_10)) { + http09 = false; http11 = false; keepAlive = false; protocolMB.setString(Constants.HTTP_10); @@ -1350,6 +1344,7 @@ public abstract class AbstractHttp11Processor<S> extends AbstractProcessor<S> { keepAlive = false; } else { // Unsupported protocol + http09 = false; http11 = false; // Send 505; Unsupported HTTP version response.setStatus(505); @@ -1359,6 +1354,22 @@ public abstract class AbstractHttp11Processor<S> extends AbstractProcessor<S> { " Unsupported HTTP version \""+protocolMB+"\""); } } + } + + + /** + * After reading the request headers, we have to setup the request filters. + */ + protected void prepareRequest() throws IOException { + + contentDelimitation = false; + expectation = false; + + prepareRequestInternal(); + + if (endpoint.isSSLEnabled()) { + request.scheme().setString("https"); + } MessageBytes methodMB = request.method(); if (methodMB.equals(Constants.GET)) { diff --git a/java/org/apache/coyote/http11/AbstractInputBuffer.java b/java/org/apache/coyote/http11/AbstractInputBuffer.java index 379fab5..7f9b856 100644 --- a/java/org/apache/coyote/http11/AbstractInputBuffer.java +++ b/java/org/apache/coyote/http11/AbstractInputBuffer.java @@ -117,11 +117,12 @@ public abstract class AbstractInputBuffer<S> implements InputBuffer{ protected HttpParser httpParser; + protected byte prevChr = 0; + protected byte chr = 0; // ------------------------------------------------------------- Properties - /** * Add an input filter to the filter library. */ @@ -214,12 +215,13 @@ public abstract class AbstractInputBuffer<S> implements InputBuffer{ activeFilters[i].recycle(); } + prevChr = 0; + chr = 0; lastValid = 0; pos = 0; lastActiveFilter = -1; parsingHeader = true; swallowInput = true; - } diff --git a/java/org/apache/coyote/http11/InternalAprInputBuffer.java b/java/org/apache/coyote/http11/InternalAprInputBuffer.java index 72850ba..b2e9a41 100644 --- a/java/org/apache/coyote/http11/InternalAprInputBuffer.java +++ b/java/org/apache/coyote/http11/InternalAprInputBuffer.java @@ -130,7 +130,6 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { // Skipping blank lines // - byte chr = 0; do { // Read new bytes if needed @@ -147,7 +146,7 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { request.setStartTime(System.currentTimeMillis()); } chr = buf[pos++]; - } while ((chr == Constants.CR) || (chr == Constants.LF)); + } while (chr == Constants.CR || chr == Constants.LF); pos--; @@ -223,16 +222,29 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { throw new EOFException(sm.getString("iib.eof.error")); } + if (buf[pos -1] == Constants.CR && buf[pos] != Constants.LF) { + // CR not followed by LF so not an HTTP/0.9 request and + // therefore invalid. Trigger error handling. + // Avoid unknown protocol triggering an additional error + request.protocol().setString(Constants.HTTP_11); + throw new IllegalArgumentException(sm.getString("iib.invalidRequestTarget")); + } + // Spec says single SP but it also says be tolerant of HT if (buf[pos] == Constants.SP || buf[pos] == Constants.HT) { space = true; end = pos; - } else if ((buf[pos] == Constants.CR) - || (buf[pos] == Constants.LF)) { + } else if (buf[pos] == Constants.CR) { + // HTTP/0.9 style request. CR is optional. LF is not. + } else if (buf[pos] == Constants.LF) { // HTTP/0.9 style request eol = true; space = true; - end = pos; + if (buf[pos - 1] == Constants.CR) { + end = pos - 1; + } else { + end = pos; + } } else if ((buf[pos] == Constants.QUESTION) && (questionPos == -1)) { questionPos = pos; } else if (questionPos != -1 && !httpParser.isQueryRelaxed(buf[pos])) { @@ -244,11 +256,8 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { // happen in AbstractHttp11Processor#prepareRequest() throw new IllegalArgumentException(sm.getString("iib.invalidRequestTarget")); } - pos++; - } - request.unparsedURI().setBytes(buf, start, end - start); if (questionPos >= 0) { request.queryString().setBytes(buf, questionPos + 1, @@ -259,7 +268,7 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { } // Spec says single SP but also says be tolerant of multiple and/or HT - while (space) { + while (space && !eol) { // Read new bytes if needed if (pos >= lastValid) { if (!fill()) @@ -291,10 +300,9 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { } if (buf[pos] == Constants.CR) { - end = pos; - } else if (buf[pos] == Constants.LF) { - if (end == 0) - end = pos; + // Possible end of request line. Need LF next. + } else if (buf[pos - 1] == Constants.CR && buf[pos] == Constants.LF) { + end = pos - 1; eol = true; } else if (!HttpParser.isHttpProtocol(buf[pos])) { throw new IllegalArgumentException(sm.getString("iib.invalidHttpProtocol")); @@ -343,15 +351,7 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { * HTTP header parsing is done */ @SuppressWarnings("null") // headerValue cannot be null - private boolean parseHeader() - throws IOException { - - // - // Check for blank line - // - - byte chr = 0; - byte prevChr = 0; + private boolean parseHeader() throws IOException { while (true) { @@ -367,7 +367,7 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { if (chr == Constants.CR && prevChr != Constants.CR) { // Possible start of CRLF - process the next byte. } else if (prevChr == Constants.CR && chr == Constants.LF) { - pos++; + pos++; return false; } else { if (prevChr == Constants.CR) { @@ -504,14 +504,14 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { throw new EOFException(sm.getString("iib.eof.error")); } - chr = buf[pos]; - if ((chr != Constants.SP) && (chr != Constants.HT)) { + byte peek = buf[pos]; + if (peek != Constants.SP && peek != Constants.HT) { validLine = false; } else { eol = false; // Copying one extra space in the buffer (since there must // be at least one space inserted between the lines) - buf[realPos] = chr; + buf[realPos] = peek; realPos++; } @@ -532,9 +532,6 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { lastRealByte = pos - 1; } - byte chr = 0; - byte prevChr = 0; - while (!eol) { // Read new bytes if needed diff --git a/java/org/apache/coyote/http11/InternalInputBuffer.java b/java/org/apache/coyote/http11/InternalInputBuffer.java index d1f9e48..e3f36d6 100644 --- a/java/org/apache/coyote/http11/InternalInputBuffer.java +++ b/java/org/apache/coyote/http11/InternalInputBuffer.java @@ -95,7 +95,6 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { // Skipping blank lines // - byte chr = 0; do { // Read new bytes if needed @@ -109,7 +108,7 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { request.setStartTime(System.currentTimeMillis()); } chr = buf[pos++]; - } while ((chr == Constants.CR) || (chr == Constants.LF)); + } while (chr == Constants.CR || chr == Constants.LF); pos--; @@ -177,16 +176,29 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { throw new EOFException(sm.getString("iib.eof.error")); } + if (buf[pos -1] == Constants.CR && buf[pos] != Constants.LF) { + // CR not followed by LF so not an HTTP/0.9 request and + // therefore invalid. Trigger error handling. + // Avoid unknown protocol triggering an additional error + request.protocol().setString(Constants.HTTP_11); + throw new IllegalArgumentException(sm.getString("iib.invalidRequestTarget")); + } + // Spec says single SP but it also says be tolerant of HT if (buf[pos] == Constants.SP || buf[pos] == Constants.HT) { space = true; end = pos; - } else if ((buf[pos] == Constants.CR) - || (buf[pos] == Constants.LF)) { + } else if (buf[pos] == Constants.CR) { + // HTTP/0.9 style request. CR is optional. LF is not. + } else if (buf[pos] == Constants.LF) { // HTTP/0.9 style request eol = true; space = true; - end = pos; + if (buf[pos - 1] == Constants.CR) { + end = pos - 1; + } else { + end = pos; + } } else if ((buf[pos] == Constants.QUESTION) && (questionPos == -1)) { questionPos = pos; } else if (questionPos != -1 && !httpParser.isQueryRelaxed(buf[pos])) { @@ -198,11 +210,8 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { // happen in AbstractHttp11Processor#prepareRequest() throw new IllegalArgumentException(sm.getString("iib.invalidRequestTarget")); } - pos++; - } - request.unparsedURI().setBytes(buf, start, end - start); if (questionPos >= 0) { request.queryString().setBytes(buf, questionPos + 1, @@ -213,7 +222,7 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { } // Spec says single SP but also says be tolerant of multiple SP and/or HT - while (space) { + while (space && !eol) { // Read new bytes if needed if (pos >= lastValid) { if (!fill()) @@ -243,10 +252,9 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { } if (buf[pos] == Constants.CR) { - end = pos; - } else if (buf[pos] == Constants.LF) { - if (end == 0) - end = pos; + // Possible end of request line. Need LF next. + } else if (buf[pos - 1] == Constants.CR && buf[pos] == Constants.LF) { + end = pos - 1; eol = true; } else if (!HttpParser.isHttpProtocol(buf[pos])) { throw new IllegalArgumentException(sm.getString("iib.invalidHttpProtocol")); @@ -295,15 +303,7 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { * HTTP header parsing is done */ @SuppressWarnings("null") // headerValue cannot be null - private boolean parseHeader() - throws IOException { - - // - // Check for blank line - // - - byte chr = 0; - byte prevChr = 0; + private boolean parseHeader() throws IOException { while (true) { @@ -457,14 +457,14 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { throw new EOFException(sm.getString("iib.eof.error")); } - chr = buf[pos]; - if ((chr != Constants.SP) && (chr != Constants.HT)) { + byte peek = buf[pos]; + if (peek != Constants.SP && peek != Constants.HT) { validLine = false; } else { eol = false; // Copying one extra space in the buffer (since there must // be at least one space inserted between the lines) - buf[realPos] = chr; + buf[realPos] = peek; realPos++; } @@ -503,9 +503,6 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { lastRealByte = pos - 1; } - byte chr = 0; - byte prevChr = 0; - while (!eol) { // Read new bytes if needed diff --git a/java/org/apache/coyote/http11/InternalNioInputBuffer.java b/java/org/apache/coyote/http11/InternalNioInputBuffer.java index 9cf0dd0..6b80d01 100644 --- a/java/org/apache/coyote/http11/InternalNioInputBuffer.java +++ b/java/org/apache/coyote/http11/InternalNioInputBuffer.java @@ -220,7 +220,6 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { // Skipping blank lines // if ( parsingRequestLinePhase == 0 ) { - byte chr = 0; do { // Read new bytes if needed @@ -306,15 +305,32 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { if (!fill(true,false)) //request line parsing return false; } + + if (buf[pos -1] == Constants.CR && buf[pos] != Constants.LF) { + // CR not followed by LF so not an HTTP/0.9 request and + // therefore invalid. Trigger error handling. + // Avoid unknown protocol triggering an additional error + request.protocol().setString(Constants.HTTP_11); + throw new IllegalArgumentException(sm.getString("iib.invalidRequestTarget")); + } + + // Spec says single SP but it also says be tolerant of HT if (buf[pos] == Constants.SP || buf[pos] == Constants.HT) { space = true; end = pos; - } else if ((buf[pos] == Constants.CR) - || (buf[pos] == Constants.LF)) { + } else if (buf[pos] == Constants.CR) { + // HTTP/0.9 style request. CR is optional. LF is not. + } else if (buf[pos] == Constants.LF) { // HTTP/0.9 style request parsingRequestLineEol = true; space = true; - end = pos; + // Skip the protocol processing + parsingRequestLinePhase = 6; + if (buf[pos - 1] == Constants.CR) { + end = pos - 1; + } else { + end = pos; + } } else if ((buf[pos] == Constants.QUESTION) && (parsingRequestLineQPos == -1)) { parsingRequestLineQPos = pos; } else if (parsingRequestLineQPos != -1 && !httpParser.isQueryRelaxed(buf[pos])) { @@ -336,7 +352,9 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { } else { request.requestURI().setBytes(buf, parsingRequestLineStart, end - parsingRequestLineStart); } - parsingRequestLinePhase = 5; + if (!parsingRequestLineEol) { + parsingRequestLinePhase = 5; + } } if ( parsingRequestLinePhase == 5 ) { // Spec says single SP but also be tolerant of multiple and/or HT @@ -372,10 +390,9 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { } if (buf[pos] == Constants.CR) { - end = pos; - } else if (buf[pos] == Constants.LF) { - if (end == 0) - end = pos; + // Possible end of request line. Need LF next. + } else if (buf[pos - 1] == Constants.CR && buf[pos] == Constants.LF) { + end = pos - 1; parsingRequestLineEol = true; } else if (!HttpParser.isHttpProtocol(buf[pos])) { throw new IllegalArgumentException(sm.getString("iib.invalidHttpProtocol")); @@ -510,13 +527,6 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { private HeaderParseStatus parseHeader() throws IOException { - // - // Check for blank line - // - - byte chr = 0; - byte prevChr = 0; - while (headerParsePos == HeaderParsePosition.HEADER_START) { // Read new bytes if needed @@ -683,15 +693,15 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { } } - chr = buf[pos]; + byte peek = buf[pos]; if ( headerParsePos == HeaderParsePosition.HEADER_MULTI_LINE ) { - if ( (chr != Constants.SP) && (chr != Constants.HT)) { + if (peek != Constants.SP && peek != Constants.HT) { headerParsePos = HeaderParsePosition.HEADER_START; break; } else { // Copying one extra space in the buffer (since there must // be at least one space inserted between the lines) - buf[headerData.realPos] = chr; + buf[headerData.realPos] = peek; headerData.realPos++; headerParsePos = HeaderParsePosition.HEADER_VALUE_START; } @@ -712,9 +722,6 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { headerParsePos = HeaderParsePosition.HEADER_SKIPLINE; boolean eol = false; - byte chr = 0; - byte prevChr = 0; - // Reading bytes until the end of the line while (!eol) { diff --git a/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java b/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java new file mode 100644 index 0000000..1acb03e --- /dev/null +++ b/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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 org.apache.coyote.http11; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.List; + +import org.junit.Assert; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameter; + +import org.apache.catalina.Context; +import org.apache.catalina.startup.SimpleHttpClient; +import org.apache.catalina.startup.TesterServlet; +import org.apache.catalina.startup.Tomcat; +import org.apache.catalina.startup.TomcatBaseTest; + +@RunWith(Parameterized.class) +public class TestHttp11InputBufferCRLF extends TomcatBaseTest { + + private static final String CR = "\r"; + private static final String LF = "\n"; + private static final String CRLF = CR + LF; + + @Parameterized.Parameters + public static Collection<Object[]> parameters() { + List<Object[]> parameterSets = new ArrayList<Object[]>(); + + // Requests to exercise code that allows HT in place of SP + parameterSets.add(new Object[] { Boolean.FALSE, new String[] { + "GET\thttp://localhost:8080/test\tHTTP/1.1" + CRLF + + "Host: localhost:8080" + CRLF + + "Connection: close" + CRLF + + CRLF } } ); + + // Requests to simulate package boundaries + // HTTP/0.9 request + addRequestWithSplits("GET /test" + CRLF, Boolean.TRUE, parameterSets); + + // HTTP/0.9 request (no optional CR) + addRequestWithSplits("GET /test" + LF, Boolean.TRUE, parameterSets); + + // Standard HTTP/1.1 request + addRequestWithSplits("GET http://localhost:8080/test HTTP/1.1" + CRLF + + "Host: localhost:8080" + CRLF + + "Connection: close" + CRLF + + CRLF, + Boolean.FALSE, parameterSets); + + return parameterSets; + } + + + private static void addRequestWithSplits(String request, Boolean isHttp09, List<Object[]> parameterSets) { + // Add as a single String + parameterSets.add(new Object[] { isHttp09, new String[] { request } } ); + + // Add with all CRLF split between the CR and LF + List<String> parts = new ArrayList<String>(); + int lastPos = 0; + int pos = request.indexOf("\n"); + while (pos > -1) { + parts.add(request.substring(lastPos, pos)); + lastPos = pos; + pos = request.indexOf("\n", lastPos + 1); + } + parts.add(request.substring(lastPos)); + parameterSets.add(new Object[] { isHttp09, parts.toArray(new String[0]) }); + + // Add with a split between each character + List<String> chars = new ArrayList<String>(); + for (char c : request.toCharArray()) { + chars.add(Character.toString(c)); + } + parameterSets.add(new Object[] { isHttp09, chars.toArray(new String[0]) }); + } + + @Parameter(0) + public boolean isHttp09; + + @Parameter(1) + public String[] request; + + @Test + public void testBug54947() { + + Client client = new Client(request, isHttp09); + + client.doRequest(); + Assert.assertTrue(client.isResponseBodyOK()); + } + + + private class Client extends SimpleHttpClient { + + public Client(String[] request, boolean isHttp09) { + setRequest(request); + setUseHttp09(isHttp09); + } + + private Exception doRequest() { + + Tomcat tomcat = getTomcatInstance(); + + Context root = tomcat.addContext("", TEMP_DIR); + Tomcat.addServlet(root, "TesterServlet", new TesterServlet()); + root.addServletMapping("/test", "TesterServlet"); + + try { + tomcat.start(); + setPort(tomcat.getConnector().getLocalPort()); + setRequestPause(20); + + // Open connection + connect(); + + setRequest(request); + processRequest(); // blocks until response has been read + + // Close the connection + disconnect(); + } catch (Exception e) { + return e; + } + return null; + } + + @Override + public boolean isResponseBodyOK() { + if (getResponseBody() == null) { + return false; + } + if (!getResponseBody().contains("OK")) { + return false; + } + return true; + } + } +} diff --git a/test/org/apache/coyote/http11/TestInternalInputBuffer.java b/test/org/apache/coyote/http11/TestInternalInputBuffer.java index 24005fe..4c149b0 100644 --- a/test/org/apache/coyote/http11/TestInternalInputBuffer.java +++ b/test/org/apache/coyote/http11/TestInternalInputBuffer.java @@ -38,6 +38,10 @@ import org.apache.catalina.startup.TomcatBaseTest; public class TestInternalInputBuffer extends TomcatBaseTest { + private static final String CR = "\r"; + private static final String LF = "\n"; + private static final String CRLF = CR + LF; + /** * Test case for https://bz.apache.org/bugzilla/show_bug.cgi?id=48839 */ @@ -661,7 +665,83 @@ public class TestInternalInputBuffer extends TomcatBaseTest { @Test public void testInvalidMethod() { - InvalidMethodClient client = new InvalidMethodClient(); + String[] request = new String[1]; + request[0] = + "GET" + (char) 0 + " /test HTTP/1.1" + CRLF + + "Host: localhost:8080" + CRLF + + "Connection: close" + CRLF + + CRLF; + + InvalidClient client = new InvalidClient(request); + + client.doRequest(); + Assert.assertTrue(client.getResponseLine(), client.isResponse400()); + Assert.assertTrue(client.isResponseBodyOK()); + } + + + @Test + public void testInvalidHttp09() { + + String[] request = new String[1]; + request[0] = "GET /test" + CR + " " + LF; + + InvalidClient client = new InvalidClient(request); + + client.doRequest(); + Assert.assertTrue(client.getResponseLine(), client.isResponse400()); + Assert.assertTrue(client.isResponseBodyOK()); + } + + + @Test + public void testInvalidEndOfRequestLine01() { + + String[] request = new String[1]; + request[0] = + "GET /test HTTP/1.1" + CR + + "Host: localhost:8080" + CRLF + + "Connection: close" + CRLF + + CRLF; + + InvalidClient client = new InvalidClient(request); + + client.doRequest(); + Assert.assertTrue(client.getResponseLine(), client.isResponse400()); + Assert.assertTrue(client.isResponseBodyOK()); + } + + + @Test + public void testInvalidEndOfRequestLine02() { + + String[] request = new String[1]; + request[0] = + "GET /test HTTP/1.1" + LF + + "Host: localhost:8080" + CRLF + + "Connection: close" + CRLF + + CRLF; + + InvalidClient client = new InvalidClient(request); + + client.doRequest(); + Assert.assertTrue(client.getResponseLine(), client.isResponse400()); + Assert.assertTrue(client.isResponseBodyOK()); + } + + + @Test + public void testInvalidHeader01() { + + String[] request = new String[1]; + request[0] = + "GET /test HTTP/1.1" + CRLF + + "Host: localhost:8080" + CRLF + + CR + "X-Header: xxx" + CRLF + + "Connection: close" + CRLF + + CRLF; + + InvalidClient client = new InvalidClient(request); client.doRequest(); Assert.assertTrue(client.getResponseLine(), client.isResponse400()); @@ -670,13 +750,20 @@ public class TestInternalInputBuffer extends TomcatBaseTest { /** - * Bug 48839 test client. + * Invalid request test client. */ - private class InvalidMethodClient extends SimpleHttpClient { + private class InvalidClient extends SimpleHttpClient { + + private final String[] request; + + public InvalidClient(String[] request) { + this.request = request; + } private Exception doRequest() { Tomcat tomcat = getTomcatInstance(); + tomcat.getConnector().setProperty("rejectIllegalHeader", "true"); tomcat.addContext("", TEMP_DIR); @@ -686,14 +773,6 @@ public class TestInternalInputBuffer extends TomcatBaseTest { // Open connection connect(); - - String[] request = new String[1]; - request[0] = - "GET" + (char) 0 + " /test HTTP/1.1" + CRLF + - "Host: localhost:8080" + CRLF + - "Connection: close" + CRLF + - CRLF; - setRequest(request); processRequest(); // blocks until response has been read diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index ada2d32..df12a96 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -60,6 +60,17 @@ issues do not "pop up" wrt. others). --> <section name="Tomcat 7.0.102 (violetagg)"> + <subsection name="Coyote"> + <changelog> + <fix> + <bug>64210</bug>: Correct a regression in the improvements to HTTP + header validation that caused requests to be incorrectly treated as + invalid if a <code>CRLF</code> sequence was split between TCP packets. + Improve validation of request lines, including for HTTP/0.9 requests. + (markt) + </fix> + </changelog> + </subsection> </section> <section name="Tomcat 7.0.101 (violetagg)"> <subsection name="Catalina"> @@ -146,9 +157,9 @@ used by JULI's <code>OneLineFormatter</code>. (markt) </add> <fix> - <bug>64206</bug>: Correct regression introduced in 10.0.0-M1 that meant - the HTTP specified when using the Windows Installer was ignored and 8080 - was always used. (markt) + <bug>64206</bug>: Correct a regression introduced in 7.0.100 that meant + that the HTTP port specified when using the Windows Installer was + ignored and 8080 was always used. (markt) </fix> </changelog> </subsection> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org