This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 11.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/11.0.x by this push:
new dc640a42af Respond to invalid HTTP/2 headers appropriately
dc640a42af is described below
commit dc640a42af8190b0ab68483fba988e09f24d07c5
Author: Mark Thomas <[email protected]>
AuthorDate: Tue Mar 10 14:04:20 2026 +0000
Respond to invalid HTTP/2 headers appropriately
Use as Stream reset or a 400 response rather than a connection reset
---
java/org/apache/coyote/Request.java | 4 +
java/org/apache/coyote/http2/Stream.java | 105 ++++++++++++++-------
java/org/apache/coyote/http2/StreamProcessor.java | 11 ++-
.../apache/coyote/http2/TestHttp2Section_8_1.java | 52 +++++++++-
webapps/docs/changelog.xml | 5 +
5 files changed, 142 insertions(+), 35 deletions(-)
diff --git a/java/org/apache/coyote/Request.java
b/java/org/apache/coyote/Request.java
index e717578434..163dd2fb83 100644
--- a/java/org/apache/coyote/Request.java
+++ b/java/org/apache/coyote/Request.java
@@ -70,6 +70,10 @@ public final class Request {
*/
private static final AtomicLong requestIdGenerator = new AtomicLong(0);
+ // public static final int NOTE_ADAPTER = 1; // Defined in CoyoteAdapter
+ public static final int NOTE_BAD_REQUEST = 2;
+
+
// ----------------------------------------------------------- Constructors
public Request() {
diff --git a/java/org/apache/coyote/http2/Stream.java
b/java/org/apache/coyote/http2/Stream.java
index 1edaadc2dd..831d182f26 100644
--- a/java/org/apache/coyote/http2/Stream.java
+++ b/java/org/apache/coyote/http2/Stream.java
@@ -323,17 +323,28 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
// Header names must be lowercase
if (!name.toLowerCase(Locale.US).equals(name)) {
- throw new HpackException(sm.getString("stream.header.case",
getConnectionId(), getIdAsString(), name));
+ headerException =
+ new StreamException(sm.getString("stream.headercase",
getConnectionId(), getIdAsString(), name),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be reset.
+ return;
}
if (HTTP_CONNECTION_SPECIFIC_HEADERS.contains(name)) {
- throw new HpackException(
- sm.getString("stream.header.connection",
getConnectionId(), getIdAsString(), name));
+ headerException = new StreamException(
+ sm.getString("stream.header.connection",
getConnectionId(), getIdAsString(), name),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be reset.
+ return;
}
if ("te".equals(name)) {
if (!"trailers".equals(value)) {
- throw new HpackException(sm.getString("stream.header.te",
getConnectionId(), getIdAsString(), value));
+ headerException =
+ new StreamException(sm.getString("stream.header.te",
getConnectionId(), getIdAsString(), name),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be reset.
+ return;
}
}
@@ -344,7 +355,11 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
if (name.isEmpty()) {
- throw new HpackException(sm.getString("stream.header.empty",
getConnectionId(), getIdAsString()));
+ headerException =
+ new StreamException(sm.getString("stream.header.empty",
getConnectionId(), getIdAsString(), name),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be reset.
+ return;
}
boolean pseudoHeader = name.charAt(0) == ':';
@@ -369,8 +384,11 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
configureVoidOutputFilter();
}
} else {
- throw new HpackException(
- sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), ":method"));
+ headerException = new StreamException(
+ sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), ":method"),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
}
break;
}
@@ -378,18 +396,28 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
if (coyoteRequest.scheme().isNull()) {
coyoteRequest.scheme().setString(value);
} else {
- throw new HpackException(
- sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), ":scheme"));
+ headerException = new StreamException(
+ sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), ":scheme"),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
}
break;
}
case ":path": {
if (!coyoteRequest.requestURI().isNull()) {
- throw new HpackException(
- sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), ":path"));
+ headerException = new StreamException(
+ sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), ":path"),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
}
if (value.isEmpty()) {
- throw new
HpackException(sm.getString("stream.header.noPath", getConnectionId(),
getIdAsString()));
+ headerException = new StreamException(
+ sm.getString("stream.header.noPath",
getConnectionId(), getIdAsString()),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
}
int queryStart = value.indexOf('?');
String uri;
@@ -410,10 +438,13 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
}
case ":authority": {
if (coyoteRequest.serverName().isNull()) {
- parseAuthority(value, false);
+ parseAuthority(value);
} else {
- throw new HpackException(
- sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), ":authority"));
+ headerException = new StreamException(
+ sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), ":authority"),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
}
break;
}
@@ -432,15 +463,18 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
if (coyoteRequest.serverName().isNull()) {
// No :authority header. This is first host header. Use it.
hostHeaderSeen = true;
- parseAuthority(value, true);
+ parseAuthority(value);
} else if (!hostHeaderSeen) {
// First host header - must be consistent with :authority
hostHeaderSeen = true;
compareAuthority(value);
} else {
// Multiple hosts headers - illegal
- throw new HpackException(
- sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), "host"));
+ headerException = new StreamException(
+ sm.getString("stream.header.duplicate",
getConnectionId(), getIdAsString(), "host"),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be
reset.
+ return;
}
break;
}
@@ -490,7 +524,7 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
streamOutputBuffer.closed = true;
}
- private void parseAuthority(String value, boolean host) throws
HpackException {
+ private void parseAuthority(String value) {
int i;
try {
i = Host.parse(value);
@@ -501,33 +535,40 @@ class Stream extends AbstractNonZeroStream implements
HeaderEmitter {
coyoteRequest.serverName().setString(value);
}
} catch (IllegalArgumentException iae) {
- // Host value invalid
- throw new HpackException(sm.getString("stream.header.invalid",
getConnectionId(), getIdAsString(),
- host ? "host" : ":authority", value));
+ // Bad :authority / host header -> 400 response
+ coyoteRequest.setNote(Request.NOTE_BAD_REQUEST, Boolean.TRUE);
}
// Match host name with SNI if required
- if
(!handler.getProtocol().getHttp11Protocol().checkSni(handler.getSniHostName(),
coyoteRequest.serverName().getString())) {
- throw new HpackException(sm.getString("stream.host.sni",
getConnectionId(), getIdAsString(), value,
- handler.getSniHostName()));
+ if
(!handler.getProtocol().getHttp11Protocol().checkSni(handler.getSniHostName(),
+ coyoteRequest.serverName().getString())) {
+ headerException = new StreamException(
+ sm.getString("stream.host.sni", getConnectionId(),
getIdAsString(), value),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be reset.
+ return;
}
}
- private void compareAuthority(String value) throws HpackException {
+ private void compareAuthority(String value) {
int i;
try {
i = Host.parse(value);
- if (i == -1 &&
(!value.equals(coyoteRequest.serverName().getString()) ||
coyoteRequest.getServerPort() != -1) ||
+ if (i == -1 &&
+ (!value.equals(coyoteRequest.serverName().getString()) ||
coyoteRequest.getServerPort() != -1) ||
i > -1 && ((!value.substring(0,
i).equals(coyoteRequest.serverName().getString()) ||
Integer.parseInt(value.substring(i + 1)) !=
coyoteRequest.getServerPort()))) {
// Host value inconsistent
- throw new
HpackException(sm.getString("stream.host.inconsistent", getConnectionId(),
getIdAsString(), value,
- coyoteRequest.serverName().getString(),
Integer.toString(coyoteRequest.getServerPort())));
+ headerException = new StreamException(
+ sm.getString("stream.host.inconsistent",
getConnectionId(), getIdAsString(), value,
+ coyoteRequest.serverName().getString(),
Integer.toString(coyoteRequest.getServerPort())),
+ Http2Error.PROTOCOL_ERROR, getIdAsInt());
+ // No need for further processing. The stream will be reset.
+ return;
}
} catch (IllegalArgumentException iae) {
- // Host value invalid
- throw new HpackException(
- sm.getString("stream.header.invalid", getConnectionId(),
getIdAsString(), "host", value));
+ // Bad :authority / host header -> 400 response
+ coyoteRequest.setNote(Request.NOTE_BAD_REQUEST, Boolean.TRUE);
}
}
diff --git a/java/org/apache/coyote/http2/StreamProcessor.java
b/java/org/apache/coyote/http2/StreamProcessor.java
index 942ed2014c..0b9b311031 100644
--- a/java/org/apache/coyote/http2/StreamProcessor.java
+++ b/java/org/apache/coyote/http2/StreamProcessor.java
@@ -502,7 +502,14 @@ class StreamProcessor extends AbstractProcessor implements
NonPipeliningProcesso
* The checks performed below are based on the checks in Http11InputBuffer.
*/
private boolean validateRequest() {
- HttpParser httpParser =
handler.getProtocol().getHttp11Protocol().getHttpParser();
+ // Check for issues during header processing. Include:
+ // - invalid (incorrectly formatted) :authority header
+ // - invalid (incorrectly formatted) host header
+ if (request.getNote(Request.NOTE_BAD_REQUEST) != null) {
+ // Notes not reset when request is recycled
+ request.setNote(Request.NOTE_BAD_REQUEST, null);
+ return false;
+ }
// Method name must be a token
if (!HttpParser.isToken(request.getMethod())) {
@@ -515,6 +522,8 @@ class StreamProcessor extends AbstractProcessor implements
NonPipeliningProcesso
return false;
}
+ HttpParser httpParser =
handler.getProtocol().getHttp11Protocol().getHttpParser();
+
// Invalid character in request target
// (other checks such as valid %nn happen later)
ByteChunk bc = request.requestURI().getByteChunk();
diff --git a/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
b/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
index 54f2e50431..fce4bccccc 100644
--- a/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
+++ b/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
@@ -280,6 +280,54 @@ public class TestHttp2Section_8_1 extends Http2TestBase {
}
+ @Test
+ public void testHostHeaderInvalid() throws Exception {
+ http2Connect();
+
+ List<Header> headers = new ArrayList<>(4);
+ headers.add(new Header(":method", Method.GET));
+ headers.add(new Header(":scheme", "http"));
+ headers.add(new Header(":path", "/simple"));
+ headers.add(new Header("host", "local!host:" + getPort()));
+
+ byte[] headersFrameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ buildGetRequest(headersFrameHeader, headersPayload, null, headers, 3);
+
+ writeFrame(headersFrameHeader, headersPayload);
+
+ parser.readFrame();
+
+ String trace = output.getTrace();
+ Assert.assertTrue(trace, trace.contains("3-Header-[:status]-[400]"));
+ }
+
+
+ @Test
+ public void testAuthorityHeaderInvalid() throws Exception {
+ http2Connect();
+
+ List<Header> headers = new ArrayList<>(4);
+ headers.add(new Header(":method", Method.GET));
+ headers.add(new Header(":scheme", "http"));
+ headers.add(new Header(":path", "/simple"));
+ headers.add(new Header(":authority", "local!host:" + getPort()));
+
+ byte[] headersFrameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ buildGetRequest(headersFrameHeader, headersPayload, null, headers, 3);
+
+ writeFrame(headersFrameHeader, headersPayload);
+
+ parser.readFrame();
+
+ String trace = output.getTrace();
+ Assert.assertTrue(trace, trace.contains("3-Header-[:status]-[400]"));
+ }
+
+
@Test
public void testHostHeaderDuplicate() throws Exception {
http2Connect();
@@ -301,7 +349,7 @@ public class TestHttp2Section_8_1 extends Http2TestBase {
parser.readFrame();
String trace = output.getTrace();
- Assert.assertTrue(trace, trace.contains("0-Goaway-[1]-[9]"));
+ Assert.assertTrue(trace, trace.contains("3-RST-[1]"));
}
@@ -413,7 +461,7 @@ public class TestHttp2Section_8_1 extends Http2TestBase {
parser.readFrame();
String trace = output.getTrace();
- Assert.assertTrue(trace, trace.contains("0-Goaway-[1]-[9]"));
+ Assert.assertTrue(trace, trace.contains("3-RST-[1]"));
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 57d87c3929..950f676054 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -190,6 +190,11 @@
Replace the external OpenSSL based OCSP responder used during unit
tests
with a Bouncy Castle based, in-process Java OCSP responder. (markt)
</scode>
+ <fix>
+ Relax HTTP/2 header validation and respond to invalid requests with a
+ stream reset or a 400 response as appropriate rather then with a
+ connection reset. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
