From 33fe1fd7e374f20ac6a5d04e081b7423f471b07c Mon Sep 17 00:00:00 2001 From: Bongjae Chang Date: Sat, 14 Sep 2024 21:33:03 +0900 Subject: [PATCH] Issue #2212 Enhances validation of HTTP header names --- .../grizzly/http/HttpCodecFilter.java | 57 ++++++++++++++++--- .../grizzly/http/HttpRequestParseTest.java | 55 +++++++++++++++++- 2 files changed, 100 insertions(+), 12 deletions(-) diff --git a/modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java b/modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java index 49bdd6be29..bc5061c4af 100644 --- a/modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java +++ b/modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2010, 2024 Oracle and/or its affiliates. All rights reserved. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0, which is available at @@ -37,6 +37,7 @@ import org.glassfish.grizzly.http.util.ByteChunk; import org.glassfish.grizzly.http.util.CacheableDataChunk; import org.glassfish.grizzly.http.util.Constants; +import org.glassfish.grizzly.http.util.CookieHeaderParser; import org.glassfish.grizzly.http.util.DataChunk; import org.glassfish.grizzly.http.util.Header; import org.glassfish.grizzly.http.util.MimeHeaders; @@ -707,8 +708,13 @@ protected boolean parseHeaderFromBytes(final HttpHeader httpHeader, final MimeHe parsingState.subState++; } case 1: { // parse header name - if (!parseHeaderName(httpHeader, mimeHeaders, parsingState, input, end)) { + final int result = parseHeaderName(httpHeader, mimeHeaders, parsingState, input, end); + if (result == -1) { return false; + } else if (result == -2) { // EOL. ignore field-lines + parsingState.subState = 0; + parsingState.start = -1; + return true; } parsingState.subState++; @@ -754,7 +760,7 @@ protected boolean parseHeaderFromBytes(final HttpHeader httpHeader, final MimeHe } } - protected boolean parseHeaderName(final HttpHeader httpHeader, final MimeHeaders mimeHeaders, final HeaderParsingState parsingState, final byte[] input, + protected int parseHeaderName(final HttpHeader httpHeader, final MimeHeaders mimeHeaders, final HeaderParsingState parsingState, final byte[] input, final int end) { final int arrayOffs = parsingState.arrayOffset; @@ -770,19 +776,33 @@ protected boolean parseHeaderName(final HttpHeader httpHeader, final MimeHeaders parsingState.offset = offset + 1 - arrayOffs; finalizeKnownHeaderNames(httpHeader, parsingState, input, start, offset); - return true; + return 0; } else if (b >= Constants.A && b <= Constants.Z) { if (!preserveHeaderCase) { b -= Constants.LC_OFFSET; } input[offset] = b; + } else if (b == Constants.CR) { + parsingState.offset = offset - arrayOffs; + final int eol = checkEOL(parsingState, input, end); + if (eol == 0) { // EOL + // the offset is already increased in the check + return -2; + } else if (eol == -2) { // not enough data + // by keeping the offset unchanged, we will recheck the EOL at the next opportunity. + break; + } } + if (!CookieHeaderParser.isToken(b)) { + throw new IllegalStateException( + "An invalid character 0x" + Integer.toHexString(b) + " was found in the header name"); + } offset++; } parsingState.offset = offset - arrayOffs; - return false; + return -1; } protected static int parseHeaderValue(final HttpHeader httpHeader, final HeaderParsingState parsingState, final byte[] input, final int end) { @@ -963,8 +983,13 @@ protected boolean parseHeaderFromBuffer(final HttpHeader httpHeader, final MimeH parsingState.subState++; } case 1: { // parse header name - if (!parseHeaderName(httpHeader, mimeHeaders, parsingState, input)) { + final int result = parseHeaderName(httpHeader, mimeHeaders, parsingState, input); + if (result == -1) { return false; + } else if (result == -2) { // EOL. ignore field-lines + parsingState.subState = 0; + parsingState.start = -1; + return true; } parsingState.subState++; @@ -1010,7 +1035,7 @@ protected boolean parseHeaderFromBuffer(final HttpHeader httpHeader, final MimeH } } - protected boolean parseHeaderName(final HttpHeader httpHeader, final MimeHeaders mimeHeaders, final HeaderParsingState parsingState, final Buffer input) { + protected int parseHeaderName(final HttpHeader httpHeader, final MimeHeaders mimeHeaders, final HeaderParsingState parsingState, final Buffer input) { final int limit = Math.min(input.limit(), parsingState.packetLimit); final int start = parsingState.start; int offset = parsingState.offset; @@ -1023,19 +1048,33 @@ protected boolean parseHeaderName(final HttpHeader httpHeader, final MimeHeaders parsingState.offset = offset + 1; finalizeKnownHeaderNames(httpHeader, parsingState, input, start, offset); - return true; + return 0; } else if (b >= Constants.A && b <= Constants.Z) { if (!preserveHeaderCase) { b -= Constants.LC_OFFSET; } input.put(offset, b); + } else if (b == Constants.CR) { + parsingState.offset = offset; + final int eol = checkEOL(parsingState, input); + if (eol == 0) { // EOL + // the offset is already increased in the check + return -2; + } else if (eol == -2) { // not enough data + // by keeping the offset unchanged, we will recheck the EOL at the next opportunity. + break; + } } + if (!CookieHeaderParser.isToken(b)) { + throw new IllegalStateException( + "An invalid character 0x" + Integer.toHexString(b) + " was found in the header name"); + } offset++; } parsingState.offset = offset; - return false; + return -1; } protected static int parseHeaderValue(final HttpHeader httpHeader, final HeaderParsingState parsingState, final Buffer input) { diff --git a/modules/http/src/test/java/org/glassfish/grizzly/http/HttpRequestParseTest.java b/modules/http/src/test/java/org/glassfish/grizzly/http/HttpRequestParseTest.java index 79c10e622a..6d81a74e97 100644 --- a/modules/http/src/test/java/org/glassfish/grizzly/http/HttpRequestParseTest.java +++ b/modules/http/src/test/java/org/glassfish/grizzly/http/HttpRequestParseTest.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010, 2020 Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2010, 2024 Oracle and/or its affiliates. All rights reserved. * * This program and the accompanying materials are made available under the * terms of the Eclipse Public License v. 2.0, which is available at @@ -81,6 +81,42 @@ public void testSimpleHeadersPreserveCase() throws Exception { doHttpRequestTest("POST", "/index.html", "HTTP/1.1", headers, "\r\n", true); } + public void testDisallowedHeaders() { + final StringBuilder sb = new StringBuilder("GET / HTTP/1.1\r\n"); + sb.append("Host: localhost\r\n"); + sb.append(new char[]{0x00, 0x01, 0x02, '\t', '\n', '\r', ' ', '\"', '(', ')', '/', ';', '<', '=', '>', '?', '@', + '[', 0x5c, ']', '{', '}'}).append(": some-value\r\n"); + sb.append("\r\n"); + try { + doTestDecoder(sb.toString(), 128); + fail("Bad HTTP headers exception had to be thrown"); + } catch (IllegalStateException e) { + // expected + } + try { + doTestDecoder("GET /index.html HTTP/1.1\nHost: localhost\nContent -Length: 1234\n\n", 128); + fail("Bad HTTP headers exception had to be thrown"); + } catch (IllegalStateException e) { + // expected + } + try { + doTestDecoder("GET /index.html HTTP/1.1\nHost: localhost\nContent-\rLength: 1234\n\n", 128); + fail("Bad HTTP headers exception had to be thrown"); + } catch (IllegalStateException e) { + // expected + } + } + + public void testIgnoredHeaders() throws Exception { + final Map> headers = new HashMap<>(); + headers.put("Host", new Pair<>("localhost", "localhost")); + headers.put("Ignore\r\nContent-length", new Pair<>("2345", "2345")); + final Map> expectedHeaders = new HashMap<>(); + expectedHeaders.put("Host", new Pair<>("localhost", "localhost")); + expectedHeaders.put("Content-length", new Pair<>("2345", "2345")); + doHttpRequestTest("POST", "/index.html", "HTTP/1.1", headers, expectedHeaders, "\r\n"); + } + public void testMultiLineHeaders() throws Exception { Map> headers = new HashMap<>(); headers.put("Host", new Pair<>("localhost", "localhost")); @@ -204,16 +240,29 @@ private void doHttpRequestTest(String method, String requestURI, String protocol new Pair<>(protocol, protocol), headers, eol, false); } + private void doHttpRequestTest(String method, String requestURI, String protocol, + Map> headers, + Map> expectedHeaders, String eol) throws Exception { + doHttpRequestTest(new Pair<>(method, method), new Pair<>(requestURI, requestURI), + new Pair<>(protocol, protocol), headers, expectedHeaders, eol, false); + } + private void doHttpRequestTest(String method, String requestURI, String protocol, Map> headers, String eol, boolean preserveCase) throws Exception { doHttpRequestTest(new Pair<>(method, method), new Pair<>(requestURI, requestURI), new Pair<>(protocol, protocol), headers, eol, preserveCase); } - @SuppressWarnings("unchecked") private void doHttpRequestTest(Pair method, Pair requestURI, Pair protocol, Map> headers, String eol, boolean preserveHeaderCase) throws Exception { + doHttpRequestTest(method, requestURI, protocol, headers, headers, eol, preserveHeaderCase); + } + @SuppressWarnings("unchecked") + private void doHttpRequestTest(Pair method, Pair requestURI, + Pair protocol, Map> headers, + Map> expectedHeaders, String eol, + boolean preserveHeaderCase) throws Exception { final FutureImpl parseResult = SafeFutureImpl.create(); Connection connection = null; @@ -222,7 +271,7 @@ private void doHttpRequestTest(Pair method, Pair serverFilter.setPreserveHeaderCase(preserveHeaderCase); FilterChainBuilder filterChainBuilder = FilterChainBuilder.stateless().add(new TransportFilter()).add(new ChunkingFilter(2)).add(serverFilter) - .add(new HTTPRequestCheckFilter(parseResult, method, requestURI, protocol, headers, preserveHeaderCase)); + .add(new HTTPRequestCheckFilter(parseResult, method, requestURI, protocol, expectedHeaders == null ? headers : expectedHeaders, preserveHeaderCase)); TCPNIOTransport transport = TCPNIOTransportBuilder.newInstance().build(); transport.setProcessor(filterChainBuilder.build());