This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 744333edb6de7fb71ff4fb4159d7674712325f55
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Dec 6 23:15:08 2021 +0000
Fix BZ 65726 - handle HTTP/1.1 upgrade with request body
https://bz.apache.org/bugzilla/show_bug.cgi?id=65726
---
java/org/apache/coyote/http11/Http11Processor.java | 116 +++++++++++++++------
.../upgrade/UpgradeApplicationBufferHandler.java | 45 ++++++++
java/org/apache/coyote/http2/Stream.java | 2 +-
test/org/apache/coyote/http2/Http2TestBase.java | 27 +++++
.../coyote/http2/TestHttp2UpgradeHandler.java | 80 ++++++++++++++
webapps/docs/changelog.xml | 9 ++
webapps/docs/config/http.xml | 24 +++--
webapps/docs/security-howto.xml | 8 +-
8 files changed, 262 insertions(+), 49 deletions(-)
diff --git a/java/org/apache/coyote/http11/Http11Processor.java
b/java/org/apache/coyote/http11/Http11Processor.java
index 4f27473..623631a 100644
--- a/java/org/apache/coyote/http11/Http11Processor.java
+++ b/java/org/apache/coyote/http11/Http11Processor.java
@@ -47,6 +47,7 @@ import
org.apache.coyote.http11.filters.SavedRequestInputFilter;
import org.apache.coyote.http11.filters.VoidInputFilter;
import org.apache.coyote.http11.filters.VoidOutputFilter;
import org.apache.coyote.http11.upgrade.InternalHttpUpgradeHandler;
+import org.apache.coyote.http11.upgrade.UpgradeApplicationBufferHandler;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.ExceptionUtils;
@@ -58,6 +59,7 @@ import org.apache.tomcat.util.http.parser.HttpParser;
import org.apache.tomcat.util.http.parser.TokenList;
import org.apache.tomcat.util.log.UserDataHelper;
import org.apache.tomcat.util.net.AbstractEndpoint.Handler.SocketState;
+import org.apache.tomcat.util.net.ApplicationBufferHandler;
import org.apache.tomcat.util.net.SSLSupport;
import org.apache.tomcat.util.net.SendfileDataBase;
import org.apache.tomcat.util.net.SendfileKeepAliveState;
@@ -336,18 +338,33 @@ public class Http11Processor extends AbstractProcessor {
UpgradeProtocol upgradeProtocol =
protocol.getUpgradeProtocol(requestedProtocol);
if (upgradeProtocol != null) {
if (upgradeProtocol.accept(request)) {
-
response.setStatus(HttpServletResponse.SC_SWITCHING_PROTOCOLS);
- response.setHeader("Connection", "Upgrade");
- response.setHeader("Upgrade", requestedProtocol);
- action(ActionCode.CLOSE, null);
- getAdapter().log(request, response, 0);
-
- InternalHttpUpgradeHandler upgradeHandler =
- upgradeProtocol.getInternalUpgradeHandler(
- socketWrapper, getAdapter(),
cloneRequest(request));
- UpgradeToken upgradeToken = new
UpgradeToken(upgradeHandler, null, null, requestedProtocol);
- action(ActionCode.UPGRADE, upgradeToken);
- return SocketState.UPGRADING;
+ // Create clone of request for upgraded protocol
+ Request upgradeRequest = null;
+ try {
+ upgradeRequest = cloneRequest(request);
+ } catch (ByteChunk.BufferOverflowException ioe) {
+
response.setStatus(HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ } catch (IOException ioe) {
+
response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+ setErrorState(ErrorState.CLOSE_CLEAN, ioe);
+ }
+
+ if (upgradeRequest != null) {
+ // Complete the HTTP/1.1 upgrade process
+
response.setStatus(HttpServletResponse.SC_SWITCHING_PROTOCOLS);
+ response.setHeader("Connection", "Upgrade");
+ response.setHeader("Upgrade", requestedProtocol);
+ action(ActionCode.CLOSE, null);
+ getAdapter().log(request, response, 0);
+
+ // Continue processing using new protocol
+ InternalHttpUpgradeHandler upgradeHandler =
+
upgradeProtocol.getInternalUpgradeHandler(socketWrapper, getAdapter(),
upgradeRequest);
+ UpgradeToken upgradeToken = new
UpgradeToken(upgradeHandler, null, null, requestedProtocol);
+ action(ActionCode.UPGRADE, upgradeToken);
+ return SocketState.UPGRADING;
+ }
}
}
}
@@ -491,16 +508,39 @@ public class Http11Processor extends AbstractProcessor {
// Transfer the minimal information required for the copy of the
Request
// that is passed to the HTTP upgrade process
-
dest.decodedURI().duplicate(source.decodedURI());
dest.method().duplicate(source.method());
dest.getMimeHeaders().duplicate(source.getMimeHeaders());
dest.requestURI().duplicate(source.requestURI());
dest.queryString().duplicate(source.queryString());
- return dest;
+ // Preparation for reading the request body
+ MimeHeaders headers = source.getMimeHeaders();
+ prepareExpectation(headers);
+ prepareInputFilters(headers);
+ ack(ContinueResponseTiming.ALWAYS);
+
+ // Need to read and buffer the request body, if any. RFC 7230 requires
+ // that the request is fully read before the upgrade takes place.
+ ByteChunk body = new ByteChunk();
+ int maxSavePostSize = protocol.getMaxSavePostSize();
+ if (maxSavePostSize != 0) {
+ body.setLimit(maxSavePostSize);
+ ApplicationBufferHandler buffer = new
UpgradeApplicationBufferHandler();
+
+ while (source.getInputBuffer().doRead(buffer) >= 0) {
+ body.append(buffer.getByteBuffer());
+ }
+ }
+
+ // Make the buffered request body available to the upgraded protocol.
+ SavedRequestInputFilter srif = new SavedRequestInputFilter(body);
+ dest.setInputBuffer(srif);
+ return dest;
}
+
+
private boolean handleIncompleteRequestLineRead() {
// Haven't finished reading the request so keep the socket
// open
@@ -594,8 +634,6 @@ public class Http11Processor extends AbstractProcessor {
*/
private void prepareRequest() throws IOException {
- contentDelimitation = false;
-
if (protocol.isSSLEnabled()) {
request.scheme().setString("https");
}
@@ -615,16 +653,7 @@ public class Http11Processor extends AbstractProcessor {
}
if (http11) {
- MessageBytes expectMB = headers.getValue("expect");
- if (expectMB != null && !expectMB.isNull()) {
- if
(expectMB.toString().trim().equalsIgnoreCase("100-continue")) {
- inputBuffer.setSwallowInput(false);
- request.setExpectation(true);
- } else {
-
response.setStatus(HttpServletResponse.SC_EXPECTATION_FAILED);
- setErrorState(ErrorState.CLOSE_CLEAN, null);
- }
- }
+ prepareExpectation(headers);
}
// Check user-agent header
@@ -759,6 +788,34 @@ public class Http11Processor extends AbstractProcessor {
}
// Input filter setup
+ prepareInputFilters(headers);
+
+ // Validate host name and extract port if present
+ parseHost(hostValueMB);
+
+ if (!getErrorState().isIoAllowed()) {
+ getAdapter().log(request, response, 0);
+ }
+ }
+
+
+ private void prepareExpectation(MimeHeaders headers) {
+ MessageBytes expectMB = headers.getValue("expect");
+ if (expectMB != null && !expectMB.isNull()) {
+ if (expectMB.toString().trim().equalsIgnoreCase("100-continue")) {
+ inputBuffer.setSwallowInput(false);
+ request.setExpectation(true);
+ } else {
+ response.setStatus(HttpServletResponse.SC_EXPECTATION_FAILED);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ }
+ }
+ }
+
+ private void prepareInputFilters(MimeHeaders headers) throws IOException {
+
+ contentDelimitation = false;
+
InputFilter[] inputFilters = inputBuffer.getFilters();
// Parse transfer-encoding header
@@ -804,9 +861,6 @@ public class Http11Processor extends AbstractProcessor {
}
}
- // Validate host name and extract port if present
- parseHost(hostValueMB);
-
if (!contentDelimitation) {
// If there's no content length
// (broken HTTP/1.0 or HTTP/1.1), assume
@@ -814,10 +868,6 @@ public class Http11Processor extends AbstractProcessor {
inputBuffer.addActiveFilter(inputFilters[Constants.VOID_FILTER]);
contentDelimitation = true;
}
-
- if (!getErrorState().isIoAllowed()) {
- getAdapter().log(request, response, 0);
- }
}
diff --git
a/java/org/apache/coyote/http11/upgrade/UpgradeApplicationBufferHandler.java
b/java/org/apache/coyote/http11/upgrade/UpgradeApplicationBufferHandler.java
new file mode 100644
index 0000000..b551aab
--- /dev/null
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeApplicationBufferHandler.java
@@ -0,0 +1,45 @@
+/*
+ * 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.upgrade;
+
+import java.nio.ByteBuffer;
+
+import org.apache.tomcat.util.net.ApplicationBufferHandler;
+
+/**
+ * Trivial implementation of {@link ApplicationBufferHandler} to support saving
+ * of HTTP request bodies during an HTTP/1.1 upgrade.
+ */
+public class UpgradeApplicationBufferHandler implements
ApplicationBufferHandler {
+
+ private ByteBuffer byteBuffer;
+
+ @Override
+ public void setByteBuffer(ByteBuffer byteBuffer) {
+ this.byteBuffer = byteBuffer;
+ }
+
+ @Override
+ public ByteBuffer getByteBuffer() {
+ return byteBuffer;
+ }
+
+ @Override
+ public void expand(int size) {
+ // NO-OP
+ }
+}
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index e3d6fa8..ac3dd72 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -121,7 +121,7 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
coyoteResponse.setError();
}
}
- // TODO Assuming the body has been read at this point is not valid
+ // Request body, if any, has been read and buffered
state.receivedEndOfStream();
}
this.coyoteRequest.setSendfile(handler.hasAsyncIO() &&
handler.getProtocol().getUseSendfile());
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java
b/test/org/apache/coyote/http2/Http2TestBase.java
index f677719..6e97e5a 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -21,6 +21,7 @@ import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
+import java.io.PrintWriter;
import java.net.Socket;
import java.net.SocketException;
import java.nio.ByteBuffer;
@@ -1384,6 +1385,32 @@ public abstract class Http2TestBase extends
TomcatBaseTest {
}
+ static class ReadRequestBodyServlet extends HttpServlet {
+
+ private static final long serialVersionUID = 1L;
+
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+ throws ServletException, IOException {
+ // Request bodies are unusal with GET but not illegal
+
+ long total = 0;
+ long read = 0;
+ byte[] buffer = new byte[1024];
+ try (InputStream is = req.getInputStream()) {
+ while ((read = is.read(buffer)) > 0) {
+ total += read;
+ }
+ }
+
+ resp.setContentType("text/plain");
+ resp.setCharacterEncoding("UTF-8");
+ PrintWriter pw = resp.getWriter();
+ pw.print("Total bytes read from request body [" + total + "]");
+ }
+ }
+
+
static class SettingValue {
private final int setting;
private final long value;
diff --git a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
index 383b486..ebdb14a 100644
--- a/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
+++ b/test/org/apache/coyote/http2/TestHttp2UpgradeHandler.java
@@ -17,6 +17,7 @@
package org.apache.coyote.http2;
import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
import org.junit.Assert;
import org.junit.Test;
@@ -69,4 +70,83 @@ public class TestHttp2UpgradeHandler extends Http2TestBase {
"3-EndOfStream\n", output.getTrace());
}
+
+ @Test
+ public void testUpgradeWithRequestBody() throws Exception {
+ doTestUpgradeWithRequestBody(false);
+ }
+
+
+ @Test
+ public void testUpgradeWithRequestBodyTooBig() throws Exception {
+ doTestUpgradeWithRequestBody(true);
+ }
+
+
+ private void doTestUpgradeWithRequestBody(boolean tooBig) throws Exception
{
+ enableHttp2();
+
+ Tomcat tomcat = getTomcatInstance();
+
+ Context ctxt = tomcat.addContext("", null);
+ Tomcat.addServlet(ctxt, "ReadRequestBodyServlet", new
ReadRequestBodyServlet());
+ ctxt.addServletMappingDecoded("/", "ReadRequestBodyServlet");
+
+ if (tooBig) {
+ // Reduce maxSavePostSize rather than use a larger request body
+ tomcat.getConnector().setProperty("maxSavePostSize", "10");
+ }
+ tomcat.start();
+
+ openClientConnection();
+
+ byte[] upgradeRequest = ("GET / HTTP/1.1\r\n" +
+ "Host: localhost:" + getPort() + "\r\n" +
+ "Content-Length: 18\r\n" +
+ "Connection: Upgrade,HTTP2-Settings\r\n" +
+ "Upgrade: h2c\r\n" +
+ EMPTY_HTTP2_SETTINGS_HEADER +
+ "\r\n" +
+ "Small request body").getBytes(StandardCharsets.ISO_8859_1);
+ os.write(upgradeRequest);
+ os.flush();
+
+ if (tooBig) {
+ String[] responseHeaders = readHttpResponseHeaders();
+ Assert.assertNotNull(responseHeaders);
+ Assert.assertNotEquals(0, responseHeaders.length);
+ Assert.assertEquals("HTTP/1.1 413 ", responseHeaders[0]);
+ } else {
+ Assert.assertTrue("Failed to read HTTP Upgrade response",
readHttpUpgradeResponse());
+
+ sendClientPreface();
+
+ // - 101 response acts as acknowledgement of the HTTP2-Settings
header
+ // Need to read 5 frames
+ // - settings (server settings - must be first)
+ // - settings ack (for the settings frame in the client preface)
+ // - ping
+ // - headers (for response)
+ // - data (for response body)
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+ parser.readFrame(true);
+
+ Assert.assertEquals("0-Settings-[3]-[200]\n" +
+ "0-Settings-End\n" +
+ "0-Settings-Ack\n" +
+ "0-Ping-[0,0,0,0,0,0,0,1]\n" +
+ "1-HeadersStart\n" +
+ "1-Header-[:status]-[200]\n" +
+ "1-Header-[content-type]-[text/plain;charset=UTF-8]\n" +
+ "1-Header-[content-length]-[39]\n" +
+ "1-Header-[date]-[" + DEFAULT_DATE + "]\n" +
+ "1-HeadersEnd\n" +
+ "1-Body-39\n" +
+ "1-EndOfStream\n"
+ , output.getTrace());
+ }
+ }
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 3c5631a..192470c 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -114,6 +114,15 @@
</fix>
</changelog>
</subsection>
+ <subsection name="Coyote">
+ <changelog>
+ <fix>
+ <bug>65726</bug>: Implement support for HTTP/1.1 upgrade when the
+ request includes a body. The maximum permitted size of the body is
+ controlled by <code>maxSavePostSize</code>. (markt)
+ </fix>
+ </changelog>
+ </subsection>
<subsection name="Jasper">
<changelog>
<fix>
diff --git a/webapps/docs/config/http.xml b/webapps/docs/config/http.xml
index f6e8d08..6b2fa5f 100644
--- a/webapps/docs/config/http.xml
+++ b/webapps/docs/config/http.xml
@@ -185,18 +185,20 @@
</attribute>
<attribute name="maxSavePostSize" required="false">
- <p>The maximum size in bytes of the POST which will be saved/buffered by
- the container during FORM or CLIENT-CERT authentication. For both types
- of authentication, the POST will be saved/buffered before the user is
- authenticated. For CLIENT-CERT authentication, the POST is buffered for
- the duration of the SSL handshake and the buffer emptied when the request
- is processed. For FORM authentication the POST is saved whilst the user
- is re-directed to the login form and is retained until the user
- successfully authenticates or the session associated with the
- authentication request expires. The limit can be disabled by setting this
+ <p>The maximum size in bytes of the request body which will be
+ saved/buffered by the container during FORM or CLIENT-CERT authentication
+ or during HTTP/1.1 upgarde. For both types of authentication, the request
+ body will be saved/buffered before the user is authenticated. For
+ CLIENT-CERT authentication, the request body is buffered for the duration
+ of the SSL handshake and the buffer emptied when the request is
processed.
+ For FORM authentication the POST is saved whilst the user is re-directed
+ to the login form and is retained until the user successfully
+ authenticates or the session associated with the authentication request
+ expires. For HTTP/1.1 upgrade, the request body is buffered for the
+ duration of the upgrade process. The limit can be disabled by setting
this
attribute to -1. Setting the attribute to zero will disable the saving of
- POST data during authentication. If not specified, this attribute is set
- to 4096 (4 kilobytes).</p>
+ the requets body data during authentication and HTTP/1.1 upgrade. If not
+ specified, this attribute is set to 4096 (4 kilobytes).</p>
</attribute>
<attribute name="parseBodyMethods" required="false">
diff --git a/webapps/docs/security-howto.xml b/webapps/docs/security-howto.xml
index 197cbe9..bd60acb 100644
--- a/webapps/docs/security-howto.xml
+++ b/webapps/docs/security-howto.xml
@@ -294,10 +294,10 @@
default to reduce exposure to a DOS attack.</p>
<p>The <strong>maxSavePostSize</strong> attribute controls the saving of
- POST requests during FORM and CLIENT-CERT authentication. The parameters
- are cached for the duration of the authentication (which may be many
- minutes) so this is limited to 4KB by default to reduce exposure to a DOS
- attack.</p>
+ the request body during FORM and CLIENT-CERT authentication and HTTP/1.1
+ upgrade. For FORM authentication, the request body is cached for the
+ duration of the authentication (which may be many minutes) so this is
+ limited to 4KB by default to reduce exposure to a DOS attack.</p>
<p>The <strong>maxParameterCount</strong> attribute controls the
maximum number of parameter and value pairs (GET plus POST) that can
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
