Author: markt
Date: Tue Oct 18 20:18:15 2016
New Revision: 1765502
URL: http://svn.apache.org/viewvc?rev=1765502&view=rev
Log:
Add addition checks around the handling of HTTP/2 pseudo headers.
Modified:
tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java
tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java
tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java
tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties
tomcat/trunk/java/org/apache/coyote/http2/Stream.java
tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java
tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java
tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/HeaderSink.java Tue Oct 18
20:18:15 2016
@@ -29,4 +29,9 @@ class HeaderSink implements HeaderEmitte
public void emitHeader(String name, String value) {
// NO-OP
}
+
+ @Override
+ public void validateHeaders() throws StreamException {
+ // NO-OP
+ }
}
Modified: tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/HpackDecoder.java Tue Oct 18
20:18:15 2016
@@ -324,10 +324,29 @@ public class HpackDecoder {
/**
- * Interface that can be used to immediately validate headers (ex:
uppercase detection).
+ * Interface implemented by the intended recipient of the headers.
*/
interface HeaderEmitter {
+ /**
+ * Pass a single header to the recipient.
+ *
+ * @param name Header name
+ * @param value Header value
+ */
void emitHeader(String name, String value);
+
+ /**
+ * Are the headers pass to the recipient so far valid? The decoder
needs
+ * to process all the headers to maintain state even if there is a
+ * problem. In addition, it is easy for the the intended recipient to
+ * track if the complete set of headers is valid since to do that state
+ * needs to be maintained between the parsing of the initial headers
and
+ * the parsing of any trailer headers. The recipient is the best place
+ * to maintain that state.
+ *
+ * @throws StreamException If the headers received to date are not
valid
+ */
+ void validateHeaders() throws StreamException;
}
Modified: tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/Http2Parser.java Tue Oct 18
20:18:15 2016
@@ -397,6 +397,8 @@ class Http2Parser {
headerReadBuffer.compact();
payloadSize -= toRead;
}
+
+ hpackDecoder.getHeaderEmitter().validateHeaders();
}
Modified: tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/LocalStrings.properties Tue Oct
18 20:18:15 2016
@@ -68,6 +68,8 @@ pingManager.roundTripTime=Connection [{0
stream.closed=Connection [{0}], Stream [{1}], Unable to write to stream once
it has been closed
stream.header.debug=Connection [{0}], Stream [{1}], HTTP header [{2}], Value
[{3}]
+stream.header.unexpectedPseudoHeader=Connection [{0}], Stream [{1}], Pseduo
header [{2}] received after a regular header
+stream.header.unknownPseudoHeader=Connection [{0}], Stream [{1}], Unknown
pseduo header [{2}] received
stream.notWritable=Connection [{0}], Stream [{1}], This stream is not writable
stream.reprioritisation.debug=Connection [{0}], Stream [{1}], Exclusive [{2}],
Parent [{3}], Weight [{4}]
stream.reset.debug=Connection [{0}], Stream [{1}], Reset due to [{2}]
Modified: tomcat/trunk/java/org/apache/coyote/http2/Stream.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http2/Stream.java?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http2/Stream.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http2/Stream.java Tue Oct 18 20:18:15
2016
@@ -40,6 +40,11 @@ class Stream extends AbstractStream impl
private static final Log log = LogFactory.getLog(Stream.class);
private static final StringManager sm =
StringManager.getManager(Stream.class);
+ private static final int HEADER_STATE_START = 0;
+ private static final int HEADER_STATE_PSEUDO = 1;
+ private static final int HEADER_STATE_REGULAR = 2;
+ private static final int HEADER_STATE_TRAILER = 3;
+
private static final Response ACK_RESPONSE = new Response();
static {
@@ -50,6 +55,9 @@ class Stream extends AbstractStream impl
private final Http2UpgradeHandler handler;
private final StreamStateMachine state;
+ // State machine would be too much overhead
+ private int headerState = HEADER_STATE_START;
+ private String headerStateErrorMsg = null;
// TODO: null these when finished to reduce memory used by closed stream
private final Request coyoteRequest;
private StringBuilder cookieHeader = null;
@@ -196,6 +204,25 @@ class Stream extends AbstractStream impl
name, value));
}
+ if (headerStateErrorMsg != null) {
+ // Don't bother processing the header since the stream is going to
+ // be reset anyway
+ return;
+ }
+
+ boolean pseudoHeader = name.charAt(0) == ':';
+
+ if (pseudoHeader && headerState != HEADER_STATE_PSEUDO) {
+ headerStateErrorMsg =
sm.getString("stream.header.unexpectedPseudoHeader",
+ getConnectionId(), getIdentifier(), name);
+ // No need for further processing. The stream will be reset.
+ return;
+ }
+
+ if (headerState == HEADER_STATE_PSEUDO && !pseudoHeader) {
+ headerState = HEADER_STATE_REGULAR;
+ }
+
switch(name) {
case ":method": {
coyoteRequest.method().setString(value);
@@ -244,6 +271,10 @@ class Stream extends AbstractStream impl
if ("expect".equals(name) && "100-continue".equals(value)) {
coyoteRequest.setExpectation(true);
}
+ if (pseudoHeader) {
+ headerStateErrorMsg =
sm.getString("stream.header.unknownPseudoHeader",
+ getConnectionId(), getIdentifier(), name);
+ }
// Assume other HTTP header
coyoteRequest.getMimeHeaders().addValue(name).setString(value);
}
@@ -251,6 +282,17 @@ class Stream extends AbstractStream impl
}
+ @Override
+ public void validateHeaders() throws StreamException {
+ if (headerStateErrorMsg == null) {
+ return;
+ }
+
+ throw new StreamException(headerStateErrorMsg,
Http2Error.PROTOCOL_ERROR,
+ getIdentifier().intValue());
+ }
+
+
final void receivedEndOfHeaders() {
// Cookie headers need to be concatenated into a single header
// See RFC 7540 8.1.2.5
@@ -308,6 +350,13 @@ class Stream extends AbstractStream impl
final void receivedStartOfHeaders() {
+ if (headerState == HEADER_STATE_START) {
+ headerState = HEADER_STATE_PSEUDO;
+ } else if (headerState == HEADER_STATE_PSEUDO || headerState ==
HEADER_STATE_REGULAR) {
+ headerState = HEADER_STATE_TRAILER;
+ }
+ // Parser will catch attempt to send a headers frame after the stream
+ // has closed.
state.receivedStartOfHeaders();
}
Modified: tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java (original)
+++ tomcat/trunk/test/org/apache/coyote/http2/Http2TestBase.java Tue Oct 18
20:18:15 2016
@@ -24,6 +24,8 @@ import java.io.OutputStream;
import java.net.Socket;
import java.nio.ByteBuffer;
import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.List;
import java.util.Locale;
import javax.net.SocketFactory;
@@ -170,14 +172,25 @@ public abstract class Http2TestBase exte
protected void buildGetRequest(byte[] frameHeader, ByteBuffer
headersPayload, byte[] padding,
int streamId, String url) {
+ List<Header> headers = new ArrayList<>(3);
+ headers.add(new Header(":method", "GET"));
+ headers.add(new Header(":path", url));
+ headers.add(new Header(":authority", "localhost:" + getPort()));
+
+ buildGetRequest(frameHeader, headersPayload, padding, headers,
streamId);
+ }
+
+
+ protected void buildGetRequest(byte[] frameHeader, ByteBuffer
headersPayload, byte[] padding,
+ List<Header> headers, int streamId) {
if (padding != null) {
headersPayload.put((byte) (0xFF & padding.length));
}
- MimeHeaders headers = new MimeHeaders();
- headers.addValue(":method").setString("GET");
- headers.addValue(":path").setString(url);
- headers.addValue(":authority").setString("localhost:" + getPort());
- hpackEncoder.encode(headers, headersPayload);
+ MimeHeaders mimeHeaders = new MimeHeaders();
+ for (Header header : headers) {
+
mimeHeaders.addValue(header.getName()).setString(header.getValue());
+ }
+ hpackEncoder.encode(mimeHeaders, headersPayload);
if (padding != null) {
headersPayload.put(padding);
}
@@ -197,10 +210,21 @@ public abstract class Http2TestBase exte
protected void buildSimpleGetRequestPart1(byte[] frameHeader, ByteBuffer
headersPayload,
int streamId) {
- MimeHeaders headers = new MimeHeaders();
- headers.addValue(":method").setString("GET");
- headers.addValue(":path").setString("/simple");
- hpackEncoder.encode(headers, headersPayload);
+ List<Header> headers = new ArrayList<>(3);
+ headers.add(new Header(":method", "GET"));
+ headers.add(new Header(":path", "/simple"));
+
+ buildSimpleGetRequestPart1(frameHeader, headersPayload, headers,
streamId);
+ }
+
+
+ protected void buildSimpleGetRequestPart1(byte[] frameHeader, ByteBuffer
headersPayload,
+ List<Header> headers, int streamId) {
+ MimeHeaders mimeHeaders = new MimeHeaders();
+ for (Header header : headers) {
+
mimeHeaders.addValue(header.getName()).setString(header.getValue());
+ }
+ hpackEncoder.encode(mimeHeaders, headersPayload);
headersPayload.flip();
@@ -215,9 +239,20 @@ public abstract class Http2TestBase exte
protected void buildSimpleGetRequestPart2(byte[] frameHeader, ByteBuffer
headersPayload,
int streamId) {
- MimeHeaders headers = new MimeHeaders();
- headers.addValue(":authority").setString("localhost:" + getPort());
- hpackEncoder.encode(headers, headersPayload);
+ List<Header> headers = new ArrayList<>(3);
+ headers.add(new Header(":authority", "localhost:" + getPort()));
+
+ buildSimpleGetRequestPart2(frameHeader, headersPayload, headers,
streamId);
+ }
+
+
+ protected void buildSimpleGetRequestPart2(byte[] frameHeader, ByteBuffer
headersPayload,
+ List<Header> headers, int streamId) {
+ MimeHeaders mimeHeaders = new MimeHeaders();
+ for (Header header : headers) {
+
mimeHeaders.addValue(header.getName()).setString(header.getValue());
+ }
+ hpackEncoder.encode(mimeHeaders, headersPayload);
headersPayload.flip();
@@ -856,6 +891,12 @@ public abstract class Http2TestBase exte
@Override
+ public void validateHeaders() {
+ // NO-OP: Accept anything the server sends for the unit tests
+ }
+
+
+ @Override
public void headersEnd(int streamId) {
trace.append(streamId + "-HeadersEnd\n");
}
@@ -1070,4 +1111,23 @@ public abstract class Http2TestBase exte
return value;
}
}
+
+
+ static class Header {
+ private final String name;
+ private final String value;
+
+ public Header(String name, String value) {
+ this.name = name;
+ this.value = value;
+ }
+
+ public String getName() {
+ return name;
+ }
+
+ public String getValue() {
+ return value;
+ }
+ }
}
Modified: tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java (original)
+++ tomcat/trunk/test/org/apache/coyote/http2/TestHpack.java Tue Oct 18
20:18:15 2016
@@ -79,6 +79,10 @@ public class TestHpack {
public void emitHeader(String name, String value) {
headers.setValue(name).setString(value);
}
+ @Override
+ public void validateHeaders() throws StreamException {
+ // NO-OP
+ }
}
// TODO: Write more complete tests
Modified: tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_8_1.java?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_8_1.java
(original)
+++ tomcat/trunk/test/org/apache/coyote/http2/TestHttp2Section_8_1.java Tue Oct
18 20:18:15 2016
@@ -17,6 +17,8 @@
package org.apache.coyote.http2;
import java.nio.ByteBuffer;
+import java.util.ArrayList;
+import java.util.List;
import org.junit.Assert;
import org.junit.Test;
@@ -71,6 +73,80 @@ public class TestHttp2Section_8_1 extend
"3-Body-256\n" +
"3-EndOfStream\n",
output.getTrace());
+ }
+
+
+ @Test
+ public void testUndefinedPseudoHeader() throws Exception {
+ List<Header> headers = new ArrayList<>(3);
+ headers.add(new Header(":method", "GET"));
+ headers.add(new Header(":path", "/simple"));
+ headers.add(new Header(":authority", "localhost:" + getPort()));
+ headers.add(new Header(":foo", "bar"));
+
+ doInvalidPseudoHeaderTest(headers);
+ }
+
+
+ @Test
+ public void testInvalidPseudoHeader() throws Exception {
+ List<Header> headers = new ArrayList<>(3);
+ headers.add(new Header(":method", "GET"));
+ headers.add(new Header(":path", "/simple"));
+ headers.add(new Header(":authority", "localhost:" + getPort()));
+ headers.add(new Header(":status", "200"));
+
+ doInvalidPseudoHeaderTest(headers);
+ }
+
+
+ @Test
+ public void testPseudoHeaderOrder() throws Exception {
+ // Need to do this in two frames because HPACK encoder automatically
+ // re-orders fields
+
+ http2Connect();
+
+ List<Header> headers = new ArrayList<>(3);
+ headers.add(new Header(":method", "GET"));
+ headers.add(new Header(":path", "/simple"));
+ headers.add(new Header("x-test", "test"));
+
+ byte[] headersFrameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ buildSimpleGetRequestPart1(headersFrameHeader, headersPayload, headers
, 3);
+
+ writeFrame(headersFrameHeader, headersPayload);
+
+ headers.clear();
+ headers.add(new Header(":authority", "localhost:" + getPort()));
+ headersPayload.clear();
+
+ buildSimpleGetRequestPart2(headersFrameHeader, headersPayload, headers
, 3);
+
+ writeFrame(headersFrameHeader, headersPayload);
+
+
+ parser.readFrame(true);
+
+ Assert.assertEquals("3-RST-[1]\n", output.getTrace());
+ }
+
+
+ private void doInvalidPseudoHeaderTest(List<Header> headers) throws
Exception {
+ http2Connect();
+
+ byte[] headersFrameHeader = new byte[9];
+ ByteBuffer headersPayload = ByteBuffer.allocate(128);
+
+ buildGetRequest(headersFrameHeader, headersPayload, null, headers , 3);
+
+ // Write the headers
+ writeFrame(headersFrameHeader, headersPayload);
+
+ parser.readFrame(true);
+ Assert.assertEquals("3-RST-[1]\n", output.getTrace());
}
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL:
http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1765502&r1=1765501&r2=1765502&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Tue Oct 18 20:18:15 2016
@@ -111,6 +111,10 @@
Correct the HTTP header parser so that DEL is not treated as a valid
token character. (markt)
</fix>
+ <add>
+ Add addition checks around the handling of HTTP/2 pseudo headers.
+ (markt)
+ </add>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
