From 8a0795f4c6902811a3254e4677739955ba640674 Mon Sep 17 00:00:00 2001 From: Mark Thomas Date: Wed, 8 Nov 2023 09:25:17 -0600 Subject: [PATCH] Ensure IOException on request read always triggers error handling --- .../connector/BadRequestException.java | 68 +++++++++++++ .../catalina/connector/InputBuffer.java | 29 +++++- .../catalina/core/StandardHostValve.java | 18 +++- java/org/apache/coyote/Request.java | 36 +++++++ .../http11/filters/ChunkedInputFilter.java | 14 ++- .../filters/TestChunkedInputFilter.java | 98 ++++++++++++++++--- webapps/docs/changelog.xml | 5 + 7 files changed, 247 insertions(+), 21 deletions(-) create mode 100644 java/org/apache/catalina/connector/BadRequestException.java diff --git a/java/org/apache/catalina/connector/BadRequestException.java b/java/org/apache/catalina/connector/BadRequestException.java new file mode 100644 index 000000000000..7918f04d8ff4 --- /dev/null +++ b/java/org/apache/catalina/connector/BadRequestException.java @@ -0,0 +1,68 @@ +/* + * 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.catalina.connector; + +import java.io.IOException; + +/** + * Extend IOException to identify it as being caused by a bad request from a remote client. + */ +public class BadRequestException extends IOException { + + private static final long serialVersionUID = 1L; + + + // ------------------------------------------------------------ Constructors + + /** + * Construct a new BadRequestException with no other information. + */ + public BadRequestException() { + super(); + } + + + /** + * Construct a new BadRequestException for the specified message. + * + * @param message Message describing this exception + */ + public BadRequestException(String message) { + super(message); + } + + + /** + * Construct a new BadRequestException for the specified throwable. + * + * @param throwable Throwable that caused this exception + */ + public BadRequestException(Throwable throwable) { + super(throwable); + } + + + /** + * Construct a new BadRequestException for the specified message and throwable. + * + * @param message Message describing this exception + * @param throwable Throwable that caused this exception + */ + public BadRequestException(String message, Throwable throwable) { + super(message, throwable); + } +} \ No newline at end of file diff --git a/java/org/apache/catalina/connector/InputBuffer.java b/java/org/apache/catalina/connector/InputBuffer.java index 18c3e1efc5b9..887b08951208 100644 --- a/java/org/apache/catalina/connector/InputBuffer.java +++ b/java/org/apache/catalina/connector/InputBuffer.java @@ -23,6 +23,9 @@ import java.security.PrivilegedExceptionAction; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; + +import javax.servlet.RequestDispatcher; import org.apache.catalina.security.SecurityUtil; import org.apache.coyote.ActionCode; @@ -287,10 +290,28 @@ public int realReadBytes(byte cbuf[], int off, int len) state = BYTE_STATE; } - int result = coyoteRequest.doRead(bb); - - return result; - + try { + return coyoteRequest.doRead(bb); + } catch (BadRequestException bre) { + // Set flag used by asynchronous processing to detect errors on non-container threads + coyoteRequest.setErrorException(bre); + // In synchronous processing, this exception may be swallowed by the application so set error flags here. + coyoteRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, bre); + coyoteRequest.getResponse().setStatus(400); + coyoteRequest.getResponse().setError(); + // Make the exception visible to the application + throw bre; + } catch (IOException ioe) { + // Set flag used by asynchronous processing to detect errors on non-container threads + coyoteRequest.setErrorException(ioe); + // In synchronous processing, this exception may be swallowed by the application so set error flags here. + coyoteRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, ioe); + coyoteRequest.getResponse().setStatus(400); + coyoteRequest.getResponse().setError(); + // Any other IOException on a read is almost always due to the remote client aborting the request. + // Make the exception visible to the application + throw new ClientAbortException(ioe); + } } diff --git a/java/org/apache/catalina/core/StandardHostValve.java b/java/org/apache/catalina/core/StandardHostValve.java index 290b4d41a87f..c35b73705fe4 100644 --- a/java/org/apache/catalina/core/StandardHostValve.java +++ b/java/org/apache/catalina/core/StandardHostValve.java @@ -425,11 +425,19 @@ protected void throwable(Request request, Response response, } } } else { - // A custom error-page has not been defined for the exception - // that was thrown during request processing. Check if an - // error-page for error code 500 was specified and if so, - // send that page back as the response. - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + + /* + * A custom error-page has not been defined for the exception that was thrown during request processing. + * Set the status to 500 if an error status has not already been set and check for custom error-page for + * the status. + */ + if (response.getStatus() < HttpServletResponse.SC_BAD_REQUEST) { + response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } else if (throwable instanceof org.apache.catalina.connector.BadRequestException) { + response.setStatus(HttpServletResponse.SC_BAD_REQUEST); + } + + // The response is an error response.setError(); diff --git a/java/org/apache/coyote/Request.java b/java/org/apache/coyote/Request.java index a71507d9eb26..2ec7f46e0a0e 100644 --- a/java/org/apache/coyote/Request.java +++ b/java/org/apache/coyote/Request.java @@ -140,6 +140,13 @@ public Request() { private int available = 0; private RequestInfo reqProcessorMX=new RequestInfo(this); + + + /** + * Holds request body reading error exception. + */ + private Exception errorException = null; + // ------------------------------------------------------------- Properties @@ -446,6 +453,33 @@ public int doRead(ByteChunk chunk) return n; } + // -------------------- Error tracking -------------------- + + /** + * Set the error Exception that occurred during the writing of the response processing. + * + * @param ex The exception that occurred + */ + public void setErrorException(Exception ex) { + errorException = ex; + } + + + /** + * Get the Exception that occurred during the writing of the response. + * + * @return The exception that occurred + */ + public Exception getErrorException() { + return errorException; + } + + + public boolean isExceptionPresent() { + return errorException != null; + } + + // -------------------- debug -------------------- @@ -528,6 +562,8 @@ public void recycle() { authType.recycle(); attributes.clear(); + errorException = null; + startTime = -1; } diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java index 8401510a1eef..90a0cf171a29 100644 --- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java +++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java @@ -22,6 +22,7 @@ import java.util.Locale; import java.util.Set; +import org.apache.catalina.connector.BadRequestException; import org.apache.coyote.InputBuffer; import org.apache.coyote.Request; import org.apache.coyote.http11.Constants; @@ -509,7 +510,9 @@ private boolean parseHeader() throws IOException { colon = true; } else if (!HttpParser.isToken(chr)) { // Non-token characters are illegal in header names - throw new IOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName")); + throwBadRequestException(sm.getString("chunkedInputFilter.invalidTrailerHeaderName")); + } else if (trailingHeaders.getEnd() >= trailingHeaders.getLimit()) { + throwBadRequestException(sm.getString("chunkedInputFilter.maxTrailer")); } else { trailingHeaders.append(chr); } @@ -573,6 +576,8 @@ private boolean parseHeader() throws IOException { eol = true; } else if (HttpParser.isControl(chr) && chr != Constants.HT) { throw new IOException(sm.getString("chunkedInputFilter.invalidTrailerHeaderValue")); + } else if (trailingHeaders.getEnd() >= trailingHeaders.getLimit()) { + throw new IOException(sm.getString("chunkedInputFilter.maxTrailer")); } else if (chr == Constants.SP || chr == Constants.HT) { trailingHeaders.append(chr); } else { @@ -598,6 +603,8 @@ private boolean parseHeader() throws IOException { chr = buf[pos]; if ((chr != Constants.SP) && (chr != Constants.HT)) { validLine = false; + } else if (trailingHeaders.getEnd() >= trailingHeaders.getLimit()) { + throwBadRequestException(sm.getString("chunkedInputFilter.maxTrailer")); } else { eol = false; // Copying one extra space in the buffer (since there must @@ -627,6 +634,11 @@ private void throwIOException(String msg) throws IOException { throw new IOException(msg); } + private void throwBadRequestException(String msg) throws IOException { + error = true; + throw new BadRequestException(msg); + } + private void throwEOFException(String msg) throws IOException { error = true; diff --git a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java index 507b5d60d2a6..a453dc8bc65a 100644 --- a/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java +++ b/test/org/apache/coyote/http11/filters/TestChunkedInputFilter.java @@ -14,7 +14,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package org.apache.coyote.http11.filters; import java.io.IOException; @@ -101,7 +100,7 @@ private void doTestChunkingCRLF(boolean chunkHeaderUsesCRLF, Context ctx = tomcat.addContext("", null); // Configure allowed trailer headers - tomcat.getConnector().setProperty("allowedTrailerHeaders", "X-Trailer1,X-Trailer2"); + Assert.assertTrue(tomcat.getConnector().setProperty("allowedTrailerHeaders", "x-trailer1,x-trailer2")); EchoHeaderServlet servlet = new EchoHeaderServlet(expectPass); Tomcat.addServlet(ctx, "servlet", servlet); @@ -170,7 +169,7 @@ public void testTrailingHeadersSizeLimit() throws Exception { ctx.addServletMapping("/", "servlet"); // Limit the size of the trailing header - tomcat.getConnector().setProperty("maxTrailerSize", "10"); + Assert.assertTrue(tomcat.getConnector().setProperty("maxTrailerSize", "10")); tomcat.start(); String[] request = new String[]{ @@ -223,8 +222,8 @@ private void doTestExtensionSizeLimit(int len, boolean ok) throws Exception { // Setup Tomcat instance Tomcat tomcat = getTomcatInstance(); - tomcat.getConnector().setProperty( - "maxExtensionSize", Integer.toString(EXT_SIZE_LIMIT)); + Assert.assertTrue(tomcat.getConnector().setProperty( + "maxExtensionSize", Integer.toString(EXT_SIZE_LIMIT))); // No file system docBase required Context ctx = tomcat.addContext("", null); @@ -237,7 +236,7 @@ private void doTestExtensionSizeLimit(int len, boolean ok) throws Exception { String extName = ";foo="; StringBuilder extValue = new StringBuilder(len); for (int i = 0; i < (len - extName.length()); i++) { - extValue.append("x"); + extValue.append('x'); } String[] request = new String[]{ @@ -429,6 +428,83 @@ private void doTestChunkSize(boolean expectPass, } } + + @Test + public void testTrailerHeaderNameNotTokenThrowException() throws Exception { + doTestTrailerHeaderNameNotToken(false); + } + + @Test + public void testTrailerHeaderNameNotTokenSwallowException() throws Exception { + doTestTrailerHeaderNameNotToken(true); + } + + private void doTestTrailerHeaderNameNotToken(boolean swallowException) throws Exception { + + // Setup Tomcat instance + Tomcat tomcat = getTomcatInstance(); + + // No file system docBase required + Context ctx = tomcat.addContext("", null); + + Tomcat.addServlet(ctx, "servlet", new SwallowBodyServlet(swallowException)); + ctx.addServletMapping("/", "servlet"); + + tomcat.start(); + + String[] request = new String[]{ + "POST / HTTP/1.1" + SimpleHttpClient.CRLF + + "Host: localhost" + SimpleHttpClient.CRLF + + "Transfer-encoding: chunked" + SimpleHttpClient.CRLF + + "Content-Type: application/x-www-form-urlencoded" + SimpleHttpClient.CRLF + + "Connection: close" + SimpleHttpClient.CRLF + + SimpleHttpClient.CRLF + + "3" + SimpleHttpClient.CRLF + + "a=0" + SimpleHttpClient.CRLF + + "4" + SimpleHttpClient.CRLF + + "&b=1" + SimpleHttpClient.CRLF + + "0" + SimpleHttpClient.CRLF + + "x@trailer: Test" + SimpleHttpClient.CRLF + + SimpleHttpClient.CRLF }; + + TrailerClient client = new TrailerClient(tomcat.getConnector().getLocalPort()); + client.setRequest(request); + + client.connect(); + client.processRequest(); + // Expected to fail because of invalid trailer header name + Assert.assertTrue(client.getResponseLine(), client.isResponse400()); + } + + private static class SwallowBodyServlet extends HttpServlet { + private static final long serialVersionUID = 1L; + + private final boolean swallowException; + + SwallowBodyServlet(boolean swallowException) { + this.swallowException = swallowException; + } + + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) + throws ServletException, IOException { + resp.setContentType("text/plain"); + PrintWriter pw = resp.getWriter(); + + // Read the body + InputStream is = req.getInputStream(); + try { + while (is.read() > -1) { + } + pw.write("OK"); + } catch (IOException ioe) { + if (!swallowException) { + throw ioe; + } + } + } + } + private static class EchoHeaderServlet extends HttpServlet { private static final long serialVersionUID = 1L; @@ -436,7 +512,7 @@ private static class EchoHeaderServlet extends HttpServlet { private final boolean expectPass; - public EchoHeaderServlet(boolean expectPass) { + EchoHeaderServlet(boolean expectPass) { this.expectPass = expectPass; } @@ -466,7 +542,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throw ioe; } - pw.write(Integer.valueOf(count).toString()); + pw.write(Integer.toString(count)); // Headers should be visible now dumpHeader("x-trailer1", req, pw); @@ -495,7 +571,7 @@ private static class BodyReadServlet extends HttpServlet { private final boolean expectPass; private final int readLimit; - public BodyReadServlet(boolean expectPass, int readLimit) { + BodyReadServlet(boolean expectPass, int readLimit) { this.expectPass = expectPass; this.readLimit = readLimit; } @@ -522,7 +598,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throw ioe; } - pw.write(Integer.valueOf(countRead).toString()); + pw.write(Integer.toString(countRead)); } public boolean getExceptionDuringRead() { @@ -536,7 +612,7 @@ public int getCountRead() { private static class TrailerClient extends SimpleHttpClient { - public TrailerClient(int port) { + TrailerClient(int port) { setPort(port); } diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index ceaa6f7857fa..b98271b8bdd9 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -161,6 +161,11 @@ Examples. Fix CVE-2022-34305, a low severity XSS vulnerability in the Form authentication example. (markt) + + Ensure that an IOException during the reading of the + request triggers always error handling, regardless of whether the + application swallows the exception. (markt) +