Skip to content

Commit 27bfd56

Browse files
chrisvestyawkat
chrisvest
and
yawkat
authored
Fix HTTP startline validation (#16022)
Motivation: The code assumed that oversized bit shifting would result in zeroing out values due to overflow. However, the Java Language Specification instead says that shifts effectively only consider the lower six bits of the shift amount, resulting in modular-arithmetic shifts. The consequence is that, for instance, shifing by the capital letter 'M' produces the same bit mask as carriage-return '\r', which is an illegal character in an HTTP start line. This incorrectly rejected valid URIs. Modification: Make the shifting conditional and only use it on character values less than or equal to 64 (the Long bit size). Also add tests to check that valid URLs are accepted. Result: Fixes #16020 --------- Co-authored-by: Jonas Konrad <me@yawk.at>
1 parent 40ab418 commit 27bfd56

2 files changed

Lines changed: 116 additions & 7 deletions

File tree

‎codec-http/src/main/java/io/netty/handler/codec/http/HttpUtil.java‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -109,27 +109,31 @@ public static boolean isEncodingSafeStartLineToken(CharSequence token) {
109109
int modulo = lenBytes % 4;
110110
int lenInts = modulo == 0 ? lenBytes : lenBytes - modulo;
111111
for (; i < lenInts; i += 4) {
112-
long chars = 1L << token.charAt(i) |
113-
1L << token.charAt(i + 1) |
114-
1L << token.charAt(i + 2) |
115-
1L << token.charAt(i + 3);
112+
long chars = charMask(token, i) |
113+
charMask(token, i + 1) |
114+
charMask(token, i + 2) |
115+
charMask(token, i + 3);
116116
if ((chars & ILLEGAL_REQUEST_LINE_TOKEN_OCTET_MASK) != 0) {
117117
return false;
118118
}
119119
}
120120
for (; i < lenBytes; i++) {
121-
long ch = 1L << token.charAt(i);
121+
long ch = charMask(token, i);
122122
if ((ch & ILLEGAL_REQUEST_LINE_TOKEN_OCTET_MASK) != 0) {
123123
return false;
124124
}
125125
}
126126
return true;
127127
}
128128

129+
private static long charMask(CharSequence token, int i) {
130+
char c = token.charAt(i);
131+
return c < 64 ? 1L << c : 0;
132+
}
133+
129134
/**
130135
* Returns {@code true} if and only if the connection can remain open and
131-
* thus 'kept alive'. This methods respects the value of the.
132-
*
136+
* thus 'kept alive'. This method respects the value of the
133137
* {@code "Connection"} header first and then the return value of
134138
* {@link HttpVersion#isKeepAliveDefault()}.
135139
*/

‎codec-http/src/test/java/io/netty/handler/codec/http/DefaultHttpRequestTest.java‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,8 +18,12 @@
1818
import io.netty.util.AsciiString;
1919
import org.junit.jupiter.api.Test;
2020
import org.junit.jupiter.params.ParameterizedTest;
21+
import org.junit.jupiter.params.provider.MethodSource;
2122
import org.junit.jupiter.params.provider.ValueSource;
2223

24+
import java.util.SplittableRandom;
25+
import java.util.stream.Stream;
26+
2327
import static io.netty.handler.codec.http.HttpHeadersTestUtils.of;
2428
import static org.junit.jupiter.api.Assertions.assertNull;
2529
import static org.junit.jupiter.api.Assertions.assertThrows;
@@ -48,6 +52,83 @@ void constructorMustRejectIllegalUrisByDefault(String uri) {
4852
new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri));
4953
}
5054

55+
public static Stream<String> validUris() {
56+
String pdigit = "123456789";
57+
String digit = '0' + pdigit;
58+
String digitcolon = digit + ':';
59+
String alpha = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
60+
String alphanum = alpha + digit;
61+
String alphanumdot = alphanum + '.';
62+
String unreserved = alphanumdot + "-_~";
63+
String subdelims = "$&%=!+,;'()";
64+
String userinfochars = unreserved + subdelims + ':';
65+
String pathchars = unreserved + '/';
66+
String querychars = pathchars + subdelims + '?';
67+
return new SplittableRandom().longs(1000)
68+
.mapToObj(seed -> {
69+
SplittableRandom rng = new SplittableRandom(seed);
70+
String start;
71+
String path;
72+
String query;
73+
String fragment;
74+
if (rng.nextBoolean()) {
75+
String scheme = rng.nextBoolean() ? "http://" : "HTTP://";
76+
String userinfo = rng.nextBoolean() ? "" : pick(rng, userinfochars, 1, 8) + '@';
77+
String host;
78+
String port;
79+
switch (rng.nextInt(3)) {
80+
case 0:
81+
host = pick(rng, alphanum, 1, 1) + pick(rng, alphanumdot, 1, 5);
82+
break;
83+
case 1:
84+
host = pick(rng, pdigit, 1, 1) + pick(rng, digit, 0, 2) + '.' +
85+
pick(rng, pdigit, 1, 1) + pick(rng, digit, 0, 2) + '.' +
86+
pick(rng, pdigit, 1, 1) + pick(rng, digit, 0, 2) + '.' +
87+
pick(rng, pdigit, 1, 1) + pick(rng, digit, 0, 2);
88+
break;
89+
default:
90+
host = '[' + pick(rng, digitcolon, 1, 8) + ']';
91+
break;
92+
}
93+
if (rng.nextBoolean()) {
94+
port = ':' + pick(rng, pdigit, 1, 1) + pick(rng, digit, 0, 4);
95+
} else {
96+
port = "";
97+
}
98+
start = scheme + userinfo + host + port;
99+
} else {
100+
start = "";
101+
}
102+
path = '/' + pick(rng, pathchars, 0, 8);
103+
if (rng.nextBoolean()) {
104+
query = '?' + pick(rng, querychars, 0, 8);
105+
} else {
106+
query = "";
107+
}
108+
if (rng.nextBoolean()) {
109+
fragment = '#' + pick(rng, querychars, 0, 8);
110+
} else {
111+
fragment = "";
112+
}
113+
return start + path + query + fragment;
114+
});
115+
}
116+
117+
private static String pick(SplittableRandom rng, String cs, int lowerBound, int upperBound) {
118+
int length = rng.nextInt(lowerBound, upperBound + 1);
119+
StringBuilder sb = new StringBuilder(length);
120+
for (int i = 0; i < length; i++) {
121+
sb.append(cs.charAt(rng.nextInt(cs.length())));
122+
}
123+
return sb.toString();
124+
}
125+
126+
@ParameterizedTest
127+
@MethodSource("validUris")
128+
void constructorMustAcceptValidUris(String uri) {
129+
new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri);
130+
}
131+
51132
@ParameterizedTest
52133
@ValueSource(strings = {
53134
"GET ",
@@ -100,6 +181,30 @@ public AsciiString asciiName() {
100181
});
101182
}
102183

184+
@ParameterizedTest
185+
@ValueSource(strings = {
186+
"GET",
187+
"POST",
188+
"PUT",
189+
"HEAD",
190+
"DELETE",
191+
"OPTIONS",
192+
"CONNECT",
193+
"TRACE",
194+
"PATCH",
195+
"QUERY"
196+
})
197+
void constructorMustAcceptAllHttpMethods(String method) {
198+
new DefaultHttpRequest(HttpVersion.HTTP_1_0, new HttpMethod("GET") {
199+
@Override
200+
public AsciiString asciiName() {
201+
return new AsciiString(method);
202+
}
203+
}, "/");
204+
205+
new DefaultHttpRequest(HttpVersion.HTTP_1_0, new HttpMethod(method), "/");
206+
}
207+
103208
@Test
104209
public void testHeaderRemoval() {
105210
HttpMessage m = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/");

0 commit comments

Comments
 (0)