Skip to content

Commit f6fe554

Browse files
thcedthced
committed
fix: Close integer validation bypass and query parser differential
Two validation bypasses surfaced in a security review of the request pipeline: - checkInteger narrowed numbers with longValue() before bound-checking, so a fractional Double (4.9) satisfied `type: integer` and a value above 2^64 wrapped past a declared maximum. It now rejects non-integral numbers and compares the full magnitude, keeping an allocation-free long path for the common case and falling back to BigDecimal/BigInteger otherwise. - Query parameters were validated from the already-decoded URI.getQuery() but delivered to handlers from getRawQuery(), so an encoded separator (%26) could smuggle a value past a schema constraint. Both paths now share QueryParams.parse over the raw query, so the value validated is the value the handler receives. Adds exploit tests asserting both are rejected.
1 parent 23117db commit f6fe554

6 files changed

Lines changed: 199 additions & 41 deletions

File tree

‎src/main/java/com/retailsvc/http/Request.java‎

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
package com.retailsvc.http;
22

3+
import com.retailsvc.http.internal.QueryParams;
34
import com.retailsvc.http.spec.HttpMethod;
4-
import java.net.URLDecoder;
5-
import java.nio.charset.StandardCharsets;
65
import java.util.ArrayList;
76
import java.util.Collections;
8-
import java.util.LinkedHashMap;
97
import java.util.List;
108
import java.util.Map;
119
import java.util.Objects;
@@ -265,7 +263,7 @@ public String rawQuery() {
265263
*/
266264
public Map<String, String> queryParams() {
267265
if (queryParamCache == null) {
268-
queryParamCache = parseQuery(rawQuery);
266+
queryParamCache = QueryParams.parse(rawQuery);
269267
}
270268
return queryParamCache;
271269
}
@@ -345,23 +343,4 @@ public void afterResponse(Runnable runnable) {
345343
public List<Runnable> afterHooks() {
346344
return Collections.unmodifiableList(afterHooks);
347345
}
348-
349-
private static Map<String, String> parseQuery(String query) {
350-
if (query == null || query.isBlank()) {
351-
return Map.of();
352-
}
353-
Map<String, String> out = new LinkedHashMap<>();
354-
for (String pair : query.split("&")) {
355-
if (pair.isEmpty()) {
356-
continue;
357-
}
358-
int eq = pair.indexOf('=');
359-
String rawKey = eq < 0 ? pair : pair.substring(0, eq);
360-
String rawValue = eq < 0 ? "" : pair.substring(eq + 1);
361-
out.putIfAbsent(
362-
URLDecoder.decode(rawKey, StandardCharsets.UTF_8),
363-
URLDecoder.decode(rawValue, StandardCharsets.UTF_8));
364-
}
365-
return out;
366-
}
367346
}

‎src/main/java/com/retailsvc/http/internal/QueryParams.java‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
package com.retailsvc.http.internal;
2+
3+
import java.net.URLDecoder;
4+
import java.nio.charset.StandardCharsets;
5+
import java.util.LinkedHashMap;
6+
import java.util.Map;
7+
8+
/**
9+
* Parses a raw (percent-encoded) URI query string into decoded parameters. Splitting on {@code &}
10+
* and {@code =} happens <em>before</em> per-component decoding, so a percent-encoded separator
11+
* ({@code %26}, {@code %3D}) stays inside a value instead of becoming a delimiter. Request
12+
* validation and handler consumption both parse through here, so the value validated is exactly the
13+
* value the handler receives.
14+
*/
15+
public final class QueryParams {
16+
17+
private QueryParams() {}
18+
19+
/**
20+
* Parses {@code rawQuery} (the undecoded query component, e.g. {@code URI.getRawQuery()}) into a
21+
* name→value map. Values are URL-decoded with UTF-8. For repeated names the first occurrence
22+
* wins.
23+
*/
24+
public static Map<String, String> parse(String rawQuery) {
25+
if (rawQuery == null || rawQuery.isBlank()) {
26+
return Map.of();
27+
}
28+
Map<String, String> out = new LinkedHashMap<>();
29+
for (String pair : rawQuery.split("&")) {
30+
if (pair.isEmpty()) {
31+
continue;
32+
}
33+
int eq = pair.indexOf('=');
34+
String rawKey = eq < 0 ? pair : pair.substring(0, eq);
35+
String rawValue = eq < 0 ? "" : pair.substring(eq + 1);
36+
out.putIfAbsent(
37+
URLDecoder.decode(rawKey, StandardCharsets.UTF_8),
38+
URLDecoder.decode(rawValue, StandardCharsets.UTF_8));
39+
}
40+
return out;
41+
}
42+
}

‎src/main/java/com/retailsvc/http/internal/RequestPreparationFilter.java‎

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@
2020
import com.sun.net.httpserver.Headers;
2121
import com.sun.net.httpserver.HttpExchange;
2222
import java.io.IOException;
23-
import java.util.HashMap;
2423
import java.util.LinkedHashMap;
2524
import java.util.List;
2625
import java.util.Locale;
@@ -189,7 +188,7 @@ private void validateParameters(
189188
for (Parameter p : op.parameters()) {
190189
String pointer = p.pointer();
191190
if (p.in() == Parameter.Location.QUERY && query == null) {
192-
query = parseQuery(exchange.getRequestURI().getQuery());
191+
query = QueryParams.parse(exchange.getRequestURI().getRawQuery());
193192
}
194193
String value =
195194
switch (p.in()) {
@@ -253,19 +252,4 @@ private ParsedBody validateAndParseBody(HttpExchange exchange, Operation op, byt
253252
validator.validate(parsed, mt.schema(), "");
254253
return new ParsedBody(parsed, mapper);
255254
}
256-
257-
private static Map<String, String> parseQuery(String query) {
258-
if (query == null || query.isBlank()) {
259-
return Map.of();
260-
}
261-
Map<String, String> out = new HashMap<>();
262-
for (String pair : query.split("&")) {
263-
int eq = pair.indexOf('=');
264-
if (eq <= 0) {
265-
continue;
266-
}
267-
out.putIfAbsent(pair.substring(0, eq), pair.substring(eq + 1));
268-
}
269-
return out;
270-
}
271255
}

‎src/main/java/com/retailsvc/http/validate/DefaultValidator.java‎

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.retailsvc.http.spec.schema.StringSchema;
2222
import com.retailsvc.http.spec.schema.TypeName;
2323
import java.math.BigDecimal;
24+
import java.math.BigInteger;
2425
import java.net.URI;
2526
import java.net.URISyntaxException;
2627
import java.time.LocalDate;
@@ -365,13 +366,40 @@ private static boolean isHextet(String hextet) {
365366
return true;
366367
}
367368

369+
/** Largest magnitude an IEEE-754 double represents as an exact integer (2^53). */
370+
private static final double MAX_EXACT_INTEGRAL_DOUBLE = 9007199254740992.0;
371+
368372
private static Optional<ValidationError> checkInteger(
369373
Object value, IntegerSchema s, String pointer) {
370374
if (!(value instanceof Number num)) {
371375
return err(pointer, "type", "expected integer", value);
372376
}
373-
long n = num.longValue();
377+
// Fast path: integral wrappers bound-check as a long with no allocation. This is the common
378+
// case (e.g. Jackson int/long, Gson integral tokens parsed as Long).
379+
if (num instanceof Long
380+
|| num instanceof Integer
381+
|| num instanceof Short
382+
|| num instanceof Byte) {
383+
return checkIntegerBounds(num.longValue(), s, pointer);
384+
}
385+
// Integral doubles small enough to be represented exactly also take the long path.
386+
double d = num.doubleValue();
387+
if (Double.isFinite(d) && d == Math.rint(d) && Math.abs(d) <= MAX_EXACT_INTEGRAL_DOUBLE) {
388+
return checkIntegerBounds((long) d, s, pointer);
389+
}
390+
// Non-integral (e.g. 4.9), non-finite, or beyond a double's exact-integer range: verify the
391+
// value is a whole number and compare the full magnitude so nothing wraps past a bound.
392+
BigInteger n;
393+
try {
394+
n = new BigDecimal(num.toString()).toBigIntegerExact();
395+
} catch (ArithmeticException | NumberFormatException e) {
396+
return err(pointer, "type", "expected integer", value);
397+
}
398+
return checkIntegerBounds(n, s, pointer);
399+
}
374400

401+
private static Optional<ValidationError> checkIntegerBounds(
402+
long n, IntegerSchema s, String pointer) {
375403
if (s.minimum() != null && n < s.minimum()) {
376404
return err(pointer, "minimum", "integer below minimum " + s.minimum(), n);
377405
}
@@ -394,6 +422,45 @@ private static Optional<ValidationError> checkInteger(
394422
return OK;
395423
}
396424

425+
private static Optional<ValidationError> checkIntegerBounds(
426+
BigInteger n, IntegerSchema s, String pointer) {
427+
long l;
428+
try {
429+
l = n.longValueExact();
430+
} catch (ArithmeticException outsideLong) {
431+
// Magnitude exceeds long range: it is below any Long minimum (if negative) or above any Long
432+
// maximum (if positive), and cannot satisfy an int32/int64 format.
433+
if (n.signum() > 0) {
434+
if (s.maximum() != null) {
435+
return err(pointer, "maximum", "integer above maximum " + s.maximum(), n);
436+
}
437+
if (s.exclusiveMaximum() != null) {
438+
return err(
439+
pointer, "exclusiveMaximum", "integer not less than " + s.exclusiveMaximum(), n);
440+
}
441+
} else {
442+
if (s.minimum() != null) {
443+
return err(pointer, "minimum", "integer below minimum " + s.minimum(), n);
444+
}
445+
if (s.exclusiveMinimum() != null) {
446+
return err(
447+
pointer, "exclusiveMinimum", "integer not greater than " + s.exclusiveMinimum(), n);
448+
}
449+
}
450+
if (s.multipleOf() != null && n.remainder(BigInteger.valueOf(s.multipleOf())).signum() != 0) {
451+
return err(pointer, "multipleOf", "not a multiple of " + s.multipleOf(), n);
452+
}
453+
if (s.format() != null) {
454+
IntegerFormatCheck check = INTEGER_FORMAT_CHECKS.get(s.format());
455+
if (check != null) {
456+
return err(pointer, FORMAT_KEYWORD, check.message(), n);
457+
}
458+
}
459+
return OK;
460+
}
461+
return checkIntegerBounds(l, s, pointer);
462+
}
463+
397464
private static Optional<ValidationError> checkNumber(
398465
Object value, NumberSchema s, String pointer) {
399466
if (!(value instanceof Number num)) {

‎src/test/java/com/retailsvc/http/QueryParamSmugglingTest.java‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
package com.retailsvc.http;
2+
3+
import static java.net.http.HttpRequest.BodyPublishers.noBody;
4+
import static org.assertj.core.api.Assertions.assertThat;
5+
import static org.assertj.core.api.Assertions.fail;
6+
7+
import com.retailsvc.http.spec.Spec;
8+
import java.io.InputStream;
9+
import java.net.http.HttpResponse;
10+
import java.net.http.HttpResponse.BodyHandlers;
11+
import java.nio.charset.StandardCharsets;
12+
import java.util.List;
13+
import java.util.Map;
14+
import org.junit.jupiter.api.Test;
15+
16+
class QueryParamSmugglingTest extends ServerBaseTest {
17+
18+
@Test
19+
void encodedAmpersandCannotSmuggleAValuePastQueryValidation() throws Exception {
20+
// Constrain q1 so any value containing '&' or '=' fails validation.
21+
constrainQ1ToLowercaseLetters();
22+
server = newServer(Map.of());
23+
24+
// %26 must stay inside q1: the handler would decode q1 to "abc&x=y", so validation must see the
25+
// same value and reject it against ^[a-z]+$ (400). The exploit was that validation parsed the
26+
// already-decoded query and saw q1="abc" (passing) while the handler received "abc&x=y".
27+
var request = newRequest(server, "/params/query?q1=abc%26x=y&q2=ok", "GET", noBody());
28+
HttpResponse<String> response = httpClient().send(request, BodyHandlers.ofString());
29+
30+
assertThat(response.statusCode()).isEqualTo(400);
31+
}
32+
33+
@SuppressWarnings("unchecked")
34+
private void constrainQ1ToLowercaseLetters() {
35+
try (InputStream in = QueryParamSmugglingTest.class.getResourceAsStream("/openapi.json")) {
36+
Map<String, Object> raw =
37+
(Map<String, Object>)
38+
gson.fromJson(new String(in.readAllBytes(), StandardCharsets.UTF_8), Map.class);
39+
var paths = (Map<String, Object>) raw.get("paths");
40+
var op = (Map<String, Object>) ((Map<String, Object>) paths.get("/params/query")).get("get");
41+
for (Object p : (List<Object>) op.get("parameters")) {
42+
var param = (Map<String, Object>) p;
43+
if ("q1".equals(param.get("name"))) {
44+
((Map<String, Object>) param.get("schema")).put("pattern", "^[a-z]+$");
45+
}
46+
}
47+
spec = Spec.from(raw);
48+
} catch (Exception e) {
49+
fail(e);
50+
}
51+
}
52+
}

‎src/test/java/com/retailsvc/http/validate/StringIntegerNumberTest.java‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import com.retailsvc.http.spec.schema.NumberSchema;
99
import com.retailsvc.http.spec.schema.StringSchema;
1010
import com.retailsvc.http.spec.schema.TypeName;
11+
import java.math.BigInteger;
1112
import java.util.List;
1213
import java.util.Map;
1314
import java.util.Set;
@@ -225,6 +226,39 @@ void integerMultipleOf() {
225226
.isEqualTo("multipleOf");
226227
}
227228

229+
@Test
230+
void integerRejectsFractionalDouble() {
231+
// Exploit: a Double with a fractional part must not satisfy `type: integer`. Master narrowed
232+
// via
233+
// longValue() before the bound check, so 4.9 slipped through minimum/maximum as 4.
234+
IntegerSchema s =
235+
new IntegerSchema(Set.of(TypeName.INTEGER), 1L, 5L, null, null, null, null, Map.of());
236+
assertThatThrownBy(() -> v.validate(4.9, s, "/v"))
237+
.isInstanceOf(ValidationException.class)
238+
.extracting(t -> ((ValidationException) t).error().keyword())
239+
.isEqualTo("type");
240+
}
241+
242+
@Test
243+
void integerAcceptsIntegralDouble() {
244+
IntegerSchema s =
245+
new IntegerSchema(Set.of(TypeName.INTEGER), 1L, 5L, null, null, null, null, Map.of());
246+
assertThatCode(() -> v.validate(4.0, s, "/v")).doesNotThrowAnyException();
247+
}
248+
249+
@Test
250+
void integerRejectsValueOverflowingLongPastMaximum() {
251+
// Exploit: 2^64 + 50 narrows to 50 via longValue(), sneaking past maximum:1000. The full
252+
// magnitude must be compared so the bound cannot be wrapped.
253+
IntegerSchema s =
254+
new IntegerSchema(Set.of(TypeName.INTEGER), 0L, 1000L, null, null, null, null, Map.of());
255+
BigInteger huge = BigInteger.TWO.pow(64).add(BigInteger.valueOf(50));
256+
assertThatThrownBy(() -> v.validate(huge, s, "/v"))
257+
.isInstanceOf(ValidationException.class)
258+
.extracting(t -> ((ValidationException) t).error().keyword())
259+
.isEqualTo("maximum");
260+
}
261+
228262
@Test
229263
void numberAcceptsDoublesAndIntegers() {
230264
NumberSchema s =

0 commit comments

Comments
 (0)