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.

Sign up for GitHub

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

Issue #2212 Enhances validation of HTTP header names #2213

Closed
wants to merge 8 commits into from
Closed

Issue #2212 Enhances validation of HTTP header names #2213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ed2eba1
Issue #2212 Enhances validation of HTTP header names
carryel Sep 14, 2024
2b6f103
FISH-9690: adding properties to enable Header Name and Header Value v…
breakponchito Nov 11, 2024
d581202
FISH-9690: adding unit test for header value validation
breakponchito Nov 12, 2024
59216d7
FISH-9690: adding missed import
breakponchito Nov 12, 2024
be0e4a1
Merge pull request #1 from breakponchito/AddingProperties
carryel Nov 18, 2024
6bb7eab
Issue #2212 Enhances validation of HTTP header names
carryel Nov 18, 2024
9b09e76
Issue #2212 Enhances validation of HTTP header names
carryel Nov 19, 2024
d258c8b
Issue #2212 Enhances validation of HTTP header names
carryel Dec 14, 2024
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
14 changes: 7 additions & 7 deletions modules/comet/src/test/java/org/glassfish/grizzly/comet/BasicCometTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2009, 2020 Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2009, 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
Expand Down Expand Up @@ -105,7 +105,7 @@ public void testClientCloseConnection() throws Exception {
s.setSoLinger(false, 0);
s.setSoTimeout(500);
OutputStream os = s.getOutputStream();
String a = "GET " + alias + " HTTP/1.1\n" + "Host: localhost:" + PORT + "\n\n";
String a = "GET " + alias + " HTTP/1.1\n" + "Host: localhost:" + PORT + "\r\n\n";
System.out.println(" " + a);
os.write(a.getBytes());
os.flush();
Expand Down Expand Up @@ -167,10 +167,10 @@ public void service(Request request, Response response) throws Exception {
Socket s = new Socket("localhost", PORT);
s.setSoTimeout(10 * 1000);
OutputStream os = s.getOutputStream();
String cometRequest = "GET " + alias + " HTTP/1.1\nHost: localhost:" + PORT + "\n\n";
String staticRequest = "GET /static HTTP/1.1\nHost: localhost:" + PORT + "\n\n";
String cometRequest = "GET " + alias + " HTTP/1.1\nHost: localhost:" + PORT + "\r\n\n";
String staticRequest = "GET /static HTTP/1.1\nHost: localhost:" + PORT + "\r\n\n";

String lastCometRequest = "GET " + alias + " HTTP/1.1\n" + "Host: localhost:" + PORT + "\nConnection: close\n\n";
String lastCometRequest = "GET " + alias + " HTTP/1.1\n" + "Host: localhost:" + PORT + "\r\nConnection: close\r\n\n";

String pipelinedRequest1 = cometRequest + staticRequest + cometRequest;
String pipelinedRequest2 = cometRequest + staticRequest + lastCometRequest;
Expand Down Expand Up @@ -256,8 +256,8 @@ public void service(Request request, Response response) throws Exception {
Socket s = new Socket("localhost", PORT);
s.setSoTimeout(10 * 1000);
OutputStream os = s.getOutputStream();
String cometRequest = "GET " + alias + " HTTP/1.1\nHost: localhost:" + PORT + "\n\n";
String staticRequest = "GET /static HTTP/1.1\nHost: localhost:" + PORT + "\n\n";
String cometRequest = "GET " + alias + " HTTP/1.1\nHost: localhost:" + PORT + "\r\n\n";
String staticRequest = "GET /static HTTP/1.1\nHost: localhost:" + PORT + "\r\n\n";

try {
os.write(cometRequest.getBytes());
Expand Down
168 changes: 128 additions & 40 deletions modules/http/src/main/java/org/glassfish/grizzly/http/HttpCodecFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -108,6 +109,14 @@ public abstract class HttpCodecFilter extends HttpBaseFilter implements Monitori
* @see #setRemoveHandledContentEncodingHeaders
*/
private boolean removeHandledContentEncodingHeaders = false;

public static final String STRICT_HEADER_NAME_VALIDATION_RFC_9110 = "org.glassfish.grizzly.http.STRICT_HEADER_NAME_VALIDATION_RFC_9110";

public static final String STRICT_HEADER_VALUE_VALIDATION_RFC_9110 = "org.glassfish.grizzly.http.STRICT_HEADER_VALUE_VALIDATION_RFC_9110";

private final boolean isStrictHeaderNameValidationSet = Boolean.parseBoolean(System.getProperty(STRICT_HEADER_NAME_VALIDATION_RFC_9110));

private final boolean isStrictHeaderValueValidationSet = Boolean.parseBoolean(System.getProperty(STRICT_HEADER_VALUE_VALIDATION_RFC_9110));

/**
* File cache probes
Expand Down Expand Up @@ -707,8 +716,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++;
Expand Down Expand Up @@ -754,7 +768,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;

Expand All @@ -770,22 +784,36 @@ 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 (isStrictHeaderNameValidationSet && 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 (isStrictHeaderNameValidationSet && !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) {
protected int parseHeaderValue(final HttpHeader httpHeader, final HeaderParsingState parsingState, final byte[] input, final int end) {

final int arrayOffs = parsingState.arrayOffset;
final int limit = Math.min(end, arrayOffs + parsingState.packetLimit);
Expand All @@ -797,24 +825,41 @@ protected static int parseHeaderValue(final HttpHeader httpHeader, final HeaderP
while (offset < limit) {
final byte b = input[offset];
if (b == Constants.CR) {
} else if (b == Constants.LF) {
// Check if it's not multi line header
if (offset + 1 < limit) {
final byte b2 = input[offset + 1];
if (b2 == Constants.SP || b2 == Constants.HT) {
input[arrayOffs + parsingState.checkpoint++] = b2;
parsingState.offset = offset + 2 - arrayOffs;
return -2;
} else {
parsingState.offset = offset + 1 - arrayOffs;
finalizeKnownHeaderValues(httpHeader, parsingState, input, arrayOffs + parsingState.start, arrayOffs + parsingState.checkpoint2);
parsingState.headerValueStorage.setBytes(input, arrayOffs + parsingState.start, arrayOffs + parsingState.checkpoint2);
if (isStrictHeaderValueValidationSet) {
parsingState.offset = offset - arrayOffs;
final int eol = checkEOL(parsingState, input, end);
if (eol == 0) { // EOL
// the offset is already increased in the check
finalizeKnownHeaderValues(httpHeader, parsingState, input, arrayOffs + parsingState.start,
arrayOffs + parsingState.checkpoint2);
parsingState.headerValueStorage.setBytes(input, arrayOffs + parsingState.start,
arrayOffs + parsingState.checkpoint2);
return 0;
} else if (eol == -2) { // not enough data
// by keeping the offset unchanged, we will recheck the EOL at the next opportunity.
return -1;
}
}

parsingState.offset = offset - arrayOffs;
return -1;
} else if (b == Constants.LF) {
if (!isStrictHeaderValueValidationSet) {
// Check if it's not multi line header
if (offset + 1 < limit) {
final byte b2 = input[offset + 1];
if (b2 == Constants.SP || b2 == Constants.HT) {
input[arrayOffs + parsingState.checkpoint++] = b2;
parsingState.offset = offset + 2 - arrayOffs;
return -2;
} else {
parsingState.offset = offset + 1 - arrayOffs;
finalizeKnownHeaderValues(httpHeader, parsingState, input, arrayOffs + parsingState.start, arrayOffs + parsingState.checkpoint2);
parsingState.headerValueStorage.setBytes(input, arrayOffs + parsingState.start, arrayOffs + parsingState.checkpoint2);
return 0;
}
}
// not enough data
parsingState.offset = offset - arrayOffs;
return -1;
}
} else if (b == Constants.SP) {
if (hasShift) {
input[arrayOffs + parsingState.checkpoint++] = b;
Expand All @@ -830,6 +875,10 @@ protected static int parseHeaderValue(final HttpHeader httpHeader, final HeaderP
parsingState.checkpoint2 = parsingState.checkpoint;
}

if (isStrictHeaderValueValidationSet && !CookieHeaderParser.isText(b)) {
throw new IllegalStateException(
"An invalid character 0x" + Integer.toHexString(b) + " was found in the header value");
}
offset++;
}
parsingState.offset = offset - arrayOffs;
Expand Down Expand Up @@ -963,8 +1012,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++;
Expand Down Expand Up @@ -1010,7 +1064,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;
Expand All @@ -1023,22 +1077,36 @@ 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 (isStrictHeaderNameValidationSet && 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 (isStrictHeaderNameValidationSet && !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) {
protected int parseHeaderValue(final HttpHeader httpHeader, final HeaderParsingState parsingState, final Buffer input) {

final int limit = Math.min(input.limit(), parsingState.packetLimit);

Expand All @@ -1049,24 +1117,40 @@ protected static int parseHeaderValue(final HttpHeader httpHeader, final HeaderP
while (offset < limit) {
final byte b = input.get(offset);
if (b == Constants.CR) {
} else if (b == Constants.LF) {
// Check if it's not multi line header
if (offset + 1 < limit) {
final byte b2 = input.get(offset + 1);
if (b2 == Constants.SP || b2 == Constants.HT) {
input.put(parsingState.checkpoint++, b2);
parsingState.offset = offset + 2;
return -2;
} else {
parsingState.offset = offset + 1;
finalizeKnownHeaderValues(httpHeader, parsingState, input, parsingState.start, parsingState.checkpoint2);
if (isStrictHeaderValueValidationSet) {
parsingState.offset = offset;
final int eol = checkEOL(parsingState, input);
if (eol == 0) { // EOL
// the offset is already increased in the check
finalizeKnownHeaderValues(httpHeader, parsingState, input, parsingState.start,
parsingState.checkpoint2);
parsingState.headerValueStorage.setBuffer(input, parsingState.start, parsingState.checkpoint2);
return 0;
} else if (eol == -2) { // not enough data
// by keeping the offset unchanged, we will recheck the EOL at the next opportunity.
return -1;
}
}

parsingState.offset = offset;
return -1;
} else if (b == Constants.LF) {
if (!isStrictHeaderValueValidationSet) {
// Check if it's not multi line header
if (offset + 1 < limit) {
final byte b2 = input.get(offset + 1);
if (b2 == Constants.SP || b2 == Constants.HT) {
input.put(parsingState.checkpoint++, b2);
parsingState.offset = offset + 2;
return -2;
} else {
parsingState.offset = offset + 1;
finalizeKnownHeaderValues(httpHeader, parsingState, input, parsingState.start, parsingState.checkpoint2);
parsingState.headerValueStorage.setBuffer(input, parsingState.start, parsingState.checkpoint2);
return 0;
}
}
// not enough data
parsingState.offset = offset;
return -1;
}
} else if (b == Constants.SP) {
if (hasShift) {
input.put(parsingState.checkpoint++, b);
Expand All @@ -1082,6 +1166,10 @@ protected static int parseHeaderValue(final HttpHeader httpHeader, final HeaderP
parsingState.checkpoint2 = parsingState.checkpoint;
}

if (isStrictHeaderValueValidationSet && !CookieHeaderParser.isText(b)) {
throw new IllegalStateException(
"An invalid character 0x" + Integer.toHexString(b) + " was found in the header value");
}
offset++;
}
parsingState.offset = offset;
Expand Down
19 changes: 18 additions & 1 deletion modules/http/src/main/java/org/glassfish/grizzly/http/util/CookieHeaderParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2022, 2022 Contributors to the Eclipse Foundation
* Copyright (c) 2022, 2024 Contributors to the Eclipse Foundation
* Copyright 2004, 2022 The Apache Software Foundation
*
* Licensed under the Apache License, Version 2.0 (the "License");
Expand Down Expand Up @@ -240,6 +240,10 @@ private static ByteBuffer readToken(ByteBuffer byteBuffer) {
}

public static boolean isToken(int c) {
if (c < 0 || c >= ARRAY_SIZE) {
// out of bounds
return false;
}
// Fast for correct values, slower for incorrect ones
try {
return IS_TOKEN[c];
Expand All @@ -248,6 +252,19 @@ public static boolean isToken(int c) {
}
}

public static boolean isText(int c) {
if (c < 0 || c >= 256) {
// out of bounds
return false;
}
// Fast for correct values, slower for incorrect ones
try {
return isText[c];
} catch (ArrayIndexOutOfBoundsException ex) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to check if you are in the 0 - isText.length range than "exception driven code"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I agree that checking the range is better than throwing an exception. The reason for doing this is to maintain similarity with other existing code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great opportunity to stop this madness.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

But for the comment:

grizzly/modules/http/src/main/java/org/glassfish/grizzly/http/util/CookieHeaderParser.java

Lines 33 to 34 in 4316ca1

* <p>Implementation note:<br>
* This class has been carefully tuned. </p>

need to dig at origin

grizzly/modules/http/src/main/java/org/glassfish/grizzly/http/util/CookieHeaderParser.java

Line 36 in 4316ca1

* @author The Tomcat team

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason for doing this is to maintain similarity with other existing code.

👍
+ to investigate the change in both places later (perhaps never)

return false;
}
}


/**
* Custom implementation that skips many of the safety checks in
Expand Down
Loading
Loading