Skip to content

Commit 310e07e

Browse files
committed
resteasy: exclude filename-bearing parts from the body-map cap
A part declaring Content-Type: text/plain but also carrying a filename attribute was still treated as a form field by collectBodyMap(), so an attacker could pad the body-map cap with disposable text/plain file parts and push a genuine field out of server.request.body. Exclude any part with a filename attribute (present, even empty) regardless of its declared media type, matching the file/field distinction collectFilesContent() already uses. Addresses a P1 finding from the Codex review on PR #12085.
1 parent 6b93c56 commit 310e07e

2 files changed

Lines changed: 60 additions & 17 deletions

File tree

dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartHelper.java

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -42,20 +42,24 @@ private MultipartHelper() {}
4242
/**
4343
* Builds the {@code server.request.body} map out of the multipart parts.
4444
*
45-
* <p>Only text/plain parts are collected, matching the Jersey reference: file parts are reported
46-
* separately via {@link #collectFilenames} / {@link #collectFilesContent} and must not also
47-
* consume the body-map budget. The number of collected values is capped by {@link
48-
* #MAX_FILES_TO_INSPECT}. The cap counts the total accumulated values across all field names, not
49-
* the distinct keys: {@code getFormDataMap()} already groups parts by field name, so a per-key
50-
* cap would be trivially bypassed by repeating the same field name on every part.
45+
* <p>Only text/plain parts with no {@code filename} attribute are collected, matching the intent
46+
* of the Jersey reference: file parts (including a file declared with a {@code text/plain}
47+
* content-type) are reported separately via {@link #collectFilenames} / {@link
48+
* #collectFilesContent} and must not also consume the body-map budget, or a request could pad out
49+
* the cap with disposable text-file parts and push a real form field out of the map. The number
50+
* of collected values is capped by {@link #MAX_FILES_TO_INSPECT}. The cap counts the total
51+
* accumulated values across all field names, not the distinct keys: {@code getFormDataMap()}
52+
* already groups parts by field name, so a per-key cap would be trivially bypassed by repeating
53+
* the same field name on every part.
5154
*/
5255
public static Map<String, List<String>> collectBodyMap(MultipartFormDataInput ret) {
5356
Map<String, List<String>> bodyMap = new HashMap<>();
5457
int total = 0;
5558
for (Map.Entry<String, List<InputPart>> e : ret.getFormDataMap().entrySet()) {
5659
for (InputPart inputPart : e.getValue()) {
57-
String contentType = contentTypeOf(inputPart);
58-
if (!isTextPlain(contentType)) {
60+
Map<String, List<String>> headers = headersOf(inputPart);
61+
String contentType = contentTypeOf(headers);
62+
if (!isTextPlain(contentType) || hasFilename(headers)) {
5963
continue;
6064
}
6165
if (total >= MAX_FILES_TO_INSPECT) {
@@ -91,26 +95,47 @@ private static String contentTypeWithDefaultUtf8(String contentType) {
9195
: contentType;
9296
}
9397

94-
private static String contentTypeOf(InputPart inputPart) {
98+
// Used by collectBodyMap only: collectFilenames/collectFilesContent do their own reflective
99+
// getHeaders() call and are intentionally left untouched (out of scope, already correct).
100+
private static Map<String, List<String>> headersOf(InputPart inputPart) {
95101
if (GET_HEADERS == null) {
96102
return null;
97103
}
98104
try {
99105
@SuppressWarnings("unchecked")
100106
Map<String, List<String>> headers = (Map<String, List<String>>) GET_HEADERS.invoke(inputPart);
101-
if (headers == null) {
102-
return null;
103-
}
104-
List<String> ctHeaders = getHeaderCaseInsensitive(headers, "Content-Type");
105-
return (ctHeaders != null && !ctHeaders.isEmpty()) ? ctHeaders.get(0) : null;
107+
return headers;
106108
} catch (Exception e) {
107109
// Reflective getHeaders() call failed (unexpected InputPart implementation): fall back to
108-
// resolving no content-type rather than aborting the whole request's body-map collection.
110+
// resolving no headers rather than aborting the whole request's body-map collection.
109111
log.debug("Failed to read multipart part headers via reflection", e);
110112
return null;
111113
}
112114
}
113115

116+
private static String contentTypeOf(Map<String, List<String>> headers) {
117+
if (headers == null) {
118+
return null;
119+
}
120+
List<String> ctHeaders = getHeaderCaseInsensitive(headers, "Content-Type");
121+
return (ctHeaders != null && !ctHeaders.isEmpty()) ? ctHeaders.get(0) : null;
122+
}
123+
124+
// A part with a filename attribute (present, even if empty) is a file upload, not a form field,
125+
// regardless of its declared content-type: a file can be declared text/plain and would otherwise
126+
// pass isTextPlain() and consume the body-map budget meant for genuine form fields. Matches the
127+
// same rawFilename-presence check collectFilesContent() already uses to decide "is this a file".
128+
private static boolean hasFilename(Map<String, List<String>> headers) {
129+
if (headers == null) {
130+
return false;
131+
}
132+
List<String> cdHeaders = getHeaderCaseInsensitive(headers, "Content-Disposition");
133+
if (cdHeaders == null || cdHeaders.isEmpty()) {
134+
return false;
135+
}
136+
return rawFilenameFromContentDisposition(cdHeaders.get(0)) != null;
137+
}
138+
114139
public static List<String> collectFilenames(MultipartFormDataInput ret) {
115140
List<String> filenames = new ArrayList<>();
116141
if (GET_HEADERS == null) {

dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/test/groovy/MultipartHelperTest.groovy

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -361,12 +361,30 @@ class MultipartHelperTest extends Specification {
361361
0 * file.getBody(_, _)
362362
}
363363
364+
def "collectBodyMap excludes a text/plain part that also declares a filename"() {
365+
given: "a file uploaded with an explicit text/plain content-type, plus a real text field"
366+
def textFile = Mock(InputPart)
367+
textFile.getHeaders() >> headers('form-data; name="upload"; filename="notes.txt"', 'text/plain')
368+
def text = Mock(InputPart)
369+
text.getHeaders() >> headers('form-data; name="q"')
370+
text.getBody(_, _) >> new ByteArrayInputStream('<script>'.bytes)
371+
def ret = Mock(MultipartFormDataInput)
372+
ret.getFormDataMap() >> ['upload': [textFile], 'q': [text]]
373+
374+
when:
375+
def result = MultipartHelper.collectBodyMap(ret)
376+
377+
then: "the text/plain file part never reaches getBody() and does not consume the body-map cap"
378+
result == ['q': ['<script>']]
379+
0 * textFile.getBody(_, _)
380+
}
381+
364382
def "collectBodyMap does not let dummy file parts exhaust the cap before a real text field"() {
365-
given: "MAX_FILES_TO_INSPECT dummy file parts followed by one real text field"
383+
given: "MAX_FILES_TO_INSPECT dummy text/plain file parts followed by one real text field"
366384
def entries = [:]
367385
(1..MultipartHelper.MAX_FILES_TO_INSPECT).each { i ->
368386
def p = Mock(InputPart)
369-
p.getHeaders() >> headers("form-data; name=\"file${i}\"; filename=\"f${i}.bin\"", 'application/octet-stream')
387+
p.getHeaders() >> headers("form-data; name=\"file${i}\"; filename=\"f${i}.txt\"", 'text/plain')
370388
entries["file${i}".toString()] = [p]
371389
}
372390
def q = Mock(InputPart)

0 commit comments

Comments
 (0)