Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refuse adding invalid HTTP 2.0 headers #277

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions java/org/apache/coyote/LocalStrings.properties
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ request.nullReadListener=The listener passed to setReadListener() may not be nul
request.readListenerSet=The non-blocking read listener has already been set

response.encoding.invalid=The encoding [{0}] is not recognised by the JRE
response.ignoringInvalidHeader=Ignoring invalid header [{0}: {1}]
response.noTrailers.notSupported=A trailer fields supplier may not be set for this response. Either the underlying protocol does not support trailer fields or the protocol requires that the supplier is set before the response is committed
response.notAsync=It is only valid to switch to non-blocking IO within async processing or HTTP upgrade processing
response.notNonBlocking=It is invalid to call isReady() when the response has not been put into non-blocking mode
Expand Down
1 change: 1 addition & 0 deletions java/org/apache/coyote/LocalStrings_fr.properties
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ request.nullReadListener=L'écouteur passé à setReadListener() ne peut pas êt
request.readListenerSet=L'écouteur des lectures non bloquantes a déjà été défini

response.encoding.invalid=L''encodage [{0}] n''est pas reconnu par le JRE
response.ignoringInvalidHeader=En-tête invalide ignoré [{0}: {1}]
response.noTrailers.notSupported=Un fournisseur d'en-tête de fin ne peut pas être mis sur cette réponse, soit le protocole ne supporte pas ces en-têtes, soit le protocole requiert que le fournisseur soit fourni avant le début de l'envoi de la réponse
response.notAsync=Il n'est possible de passer en mode d'entrée-sorties non bloquantes que lors de traitements asynchrones ou après mise à niveau depuis HTTP
response.notNonBlocking=Il n'est pas permis d'appeler isReady() quand la réponse n'a pas été mise en mode non-bloquant
Expand Down
37 changes: 37 additions & 0 deletions java/org/apache/coyote/Response.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

import jakarta.servlet.WriteListener;

import org.apache.coyote.http2.Http2OutputBuffer;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.buf.B2CConverter;
Expand Down Expand Up @@ -61,6 +62,11 @@ public final class Response {
*/
private static final Locale DEFAULT_LOCALE = Locale.getDefault();

/**
* Helper to log the invalid HTTP/2.0 header error only once per instance
*/
private static AtomicBoolean invalidHeaderWarningEmitted = new AtomicBoolean(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

per instance of what ? Because you use static here, so it is on class loader level.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martin-g It's a langage abuse. Per instance of running tomcat, sorry. Anyhow, @rmaucher suggestion to just throw the exception is simpler, so the AtomicBoolean is deprecated.



// ----------------------------------------------------- Instance Variables

Expand Down Expand Up @@ -435,6 +441,37 @@ private boolean checkSpecialHeader( String name, String value) {
return false;
}
}
if (outputBuffer instanceof Http2OutputBuffer && name.equalsIgnoreCase("Connection") ) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (outputBuffer instanceof Http2OutputBuffer && "Connection".equalsIgnoreCase(name) ) {


/*
Connection headers are invalid in HTTP/2.0, and some clients (like Safari or curl)
are very touchy about it. Most probably, an application component has added the
typical HTTP/1.x "Connection: keep-alive" header, but despide the component's
good intention, the header is faulty in HTTP/2.0 and *should* always be filtered.

The current implementation emits a warning in the logs only once per instance.
We could want to add an HTTP/2.0 option to rather always send/log an exception.

@see https://tools.ietf.org/html/rfc7540#section-8.1.2.2
*/

if (log.isWarnEnabled())
{
/* log a warning only once per instance *with the stacktrace* */
if (!invalidHeaderWarningEmitted.getAndSet(true))
{
try
{
throw new IllegalArgumentException(sm.getString("response.ignoringInvalidHeader", "Connection", value));
}
catch (IllegalArgumentException iae)
{
log.warn(iae);
}
}
}
return true;
}
return false;
}

Expand Down
88 changes: 88 additions & 0 deletions test/org/apache/coyote/http2/TestInvalidHeader.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*
* 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.http2;

import java.io.IOException;
import java.io.OutputStream;
import java.nio.ByteBuffer;
import java.util.ArrayList;
import java.util.List;

import jakarta.servlet.ServletException;
import jakarta.servlet.http.HttpServlet;
import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;

import org.junit.Assert;
import org.junit.Test;

import org.apache.catalina.Context;
import org.apache.catalina.startup.Tomcat;

public class TestInvalidHeader extends Http2TestBase {

/*
* @see org.apache.coyote.Response#checkSpecialHeaders()
*/
protected static class FaultyServlet extends SimpleServlet
{

private static final long serialVersionUID = 1L;

public static final int CONTENT_LENGTH = 8192;

@Override
protected void doGet(HttpServletRequest req, HttpServletResponse resp)
throws ServletException, IOException
{
// Add faulty header
resp.addHeader("Connection", "keep-alive");
super.doGet(req, resp);
}
}


@Test
public void testInvalidHeader() throws Exception {

enableHttp2();

Tomcat tomcat = getTomcatInstance();

Context ctxt = tomcat.addContext("", null);
Tomcat.addServlet(ctxt, "simple", new FaultyServlet());
ctxt.addServletMappingDecoded("/simple", "simple");

tomcat.start();

openClientConnection();
doHttpUpgrade();
sendClientPreface();
validateHttp2InitialResponse();

byte[] frameHeader = new byte[9];
ByteBuffer headersPayload = ByteBuffer.allocate(128);
buildGetRequest(frameHeader, headersPayload, null, 3, "/simple");

writeFrame(frameHeader, headersPayload);

readSimpleGetResponse();

Assert.assertEquals(getSimpleResponseTrace(3), output.getTrace());
}

}