Skip to content

Commit 84de0a8

Browse files
jandro996devflow.devflow-routing-intake
jandro996
and
devflow.devflow-routing-intake
authored
Bound multipart text/plain field materialization in Jersey AppSec (#11944)
Bound multipart text/plain field materialization in Jersey AppSec Jersey 2/3 AppSec instrumentation read text/plain multipart field values via the unbounded getValue() and accumulated an unlimited number of distinct field names, allowing a crafted multipart request to exhaust heap/CPU before WAF limits apply. Reuse the existing byte-capped MultipartContentDecoder and the configured file-content limits (MAX_CONTENT_BYTES, MAX_FILES_TO_INSPECT) to bound both the size of each field value and the number of distinct field names collected. fix: bound multipart body map by total value count and avoid unguarded getName() - Cap now bounds total accumulated values in the body map, not just distinct field names, closing a bypass where repeating a field name skipped the limit. - Derive the body map key from FormDataContentDisposition.getName() (a safe field accessor) instead of FormDataBodyPart.getName(), which re-parses the disposition header and can throw on malformed input. Addresses Codex review findings 1 and 3 on PR #11944. fix: default multipart content decoding to UTF-8 instead of platform charset Charset.defaultCharset() depends on the JVM/platform locale and can differ from the UTF-8 default Jersey's own getValue() used, corrupting text captured by AppSec on JVMs whose platform charset isn't UTF-8. Addresses Codex review finding 2 on PR #11944. Revert "fix: default multipart content decoding to UTF-8 instead of platform charset" This reverts commit e25666a. fix: scope UTF-8 default charset to Jersey, revert change to shared MultipartContentDecoder MultipartContentDecoder is shared by Tomcat, Netty, Vert.x, RESTEasy and commons-fileupload. Its Charset.defaultCharset() fallback was a deliberate, reviewed decision (Manuel, PR #11198) - changing it in e25666a silently altered production behavior for all those integrations, not just Jersey. Revert the shared decoder change and instead default to UTF-8 only in Jersey's MultiPartHelper, matching Jersey's own getValue() default, by appending charset=UTF-8 to the contentType before calling the shared decoder. fix: limit UTF-8 charset default to the body-map text-field path readContent() served both the bodyMap (text field) and filesContent call sites, so the Jersey-local UTF-8 default leaked into file content too. Split it into readContent() (UTF-8 default, text fields) and readFileContent() (unmodified content type, parity with MultipartContentDecoder's fallback used by file content in every other multipart helper). Merge branch 'master' into APMSP-3237 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
1 parent 6821684 commit 84de0a8

4 files changed

Lines changed: 358 additions & 34 deletions

File tree

‎dd-java-agent/instrumentation/jersey/jersey-appsec/jersey-appsec-2.0/src/main/java/datadog/trace/instrumentation/jersey2/MultiPartHelper.java‎

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,6 @@ public static void collectBodyPart(
2828
Map<String, List<String>> bodyMap,
2929
List<String> filenames,
3030
List<String> filesContent) {
31-
if (bodyMap != null
32-
&& MediaTypes.typeEqual(MediaType.TEXT_PLAIN_TYPE, bodyPart.getMediaType())) {
33-
// BodyPartEntity allows re-reading the part without consuming the stream
34-
bodyMap.computeIfAbsent(bodyPart.getName(), k -> new ArrayList<>()).add(bodyPart.getValue());
35-
}
3631
FormDataContentDisposition cd;
3732
try {
3833
cd = bodyPart.getFormDataContentDisposition();
@@ -41,6 +36,16 @@ public static void collectBodyPart(
4136
// so a single bad part does not abort processing of the remaining parts.
4237
cd = null;
4338
}
39+
if (bodyMap != null
40+
&& cd != null
41+
&& MediaTypes.typeEqual(MediaType.TEXT_PLAIN_TYPE, bodyPart.getMediaType())
42+
&& totalBodyMapValues(bodyMap) < MAX_FILES_TO_INSPECT) {
43+
// readContent() reads the part through a byte-capped decoder (MAX_CONTENT_BYTES) instead of
44+
// the unbounded getValue(); the cap counts total accumulated values across all field names,
45+
// not distinct keys, so repeating the same field name cannot bypass the limit. cd.getName()
46+
// is a safe field accessor, unlike bodyPart.getName() which re-parses the disposition header.
47+
bodyMap.computeIfAbsent(cd.getName(), k -> new ArrayList<>()).add(readContent(bodyPart));
48+
}
4449
// rawFilename == null → no filename attribute → form field → skip filenames and content
4550
// rawFilename == "" → filename attribute present but empty → content YES, filenames NO
4651
// rawFilename != "" → file with name → both
@@ -49,25 +54,52 @@ public static void collectBodyPart(
4954
filenames.add(rawFilename);
5055
}
5156
if (filesContent != null && rawFilename != null && filesContent.size() < MAX_FILES_TO_INSPECT) {
52-
filesContent.add(readContent(bodyPart));
57+
filesContent.add(readFileContent(bodyPart));
58+
}
59+
}
60+
61+
private static int totalBodyMapValues(Map<String, List<String>> bodyMap) {
62+
int total = 0;
63+
for (List<String> values : bodyMap.values()) {
64+
total += values.size();
5365
}
66+
return total;
5467
}
5568

69+
// Used for the body-map/text-field path: Jersey's own getValue() decodes undeclared-charset
70+
// text parts as UTF-8, so this matches that default instead of MultipartContentDecoder's
71+
// platform-dependent fallback.
5672
public static String readContent(FormDataBodyPart bodyPart) {
73+
return read(bodyPart, contentTypeWithDefaultUtf8(bodyPart.getMediaType()));
74+
}
75+
76+
// Used for the filesContent path: keeps parity with the other multipart helpers, which all
77+
// rely on MultipartContentDecoder's own charset fallback for undeclared-charset file content.
78+
public static String readFileContent(FormDataBodyPart bodyPart) {
79+
MediaType mediaType = bodyPart.getMediaType();
80+
return read(bodyPart, mediaType != null ? mediaType.toString() : null);
81+
}
82+
83+
private static String read(FormDataBodyPart bodyPart, String contentType) {
5784
try {
5885
// getEntityAs(InputStream.class) is backed by BodyPartEntity which supports re-reading:
5986
// each call creates a fresh stream from the buffered MIME part data.
6087
try (InputStream is = bodyPart.getEntityAs(InputStream.class)) {
6188
if (is == null) return "";
62-
String contentType =
63-
bodyPart.getMediaType() != null ? bodyPart.getMediaType().toString() : null;
6489
return MultipartContentDecoder.readInputStream(is, MAX_CONTENT_BYTES, contentType);
6590
}
6691
} catch (IOException ignored) {
6792
return "";
6893
}
6994
}
7095

96+
private static String contentTypeWithDefaultUtf8(MediaType mediaType) {
97+
String contentType = mediaType != null ? mediaType.toString() : null;
98+
return MultipartContentDecoder.extractCharset(contentType) == null
99+
? (contentType == null ? "charset=UTF-8" : contentType + "; charset=UTF-8")
100+
: contentType;
101+
}
102+
71103
public static BlockingException tryBlock(RequestContext ctx, Flow<Void> flow, String message) {
72104
Flow.Action action = flow.getAction();
73105
if (action instanceof Flow.Action.RequestBlockingAction) {

‎dd-java-agent/instrumentation/jersey/jersey-appsec/jersey-appsec-2.0/src/test/groovy/MultiPartHelperTest.groovy‎

Lines changed: 139 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,12 @@ class MultiPartHelperTest extends Specification {
1616

1717
def "text/plain part is added to body map"() {
1818
given:
19+
def cd = Mock(FormDataContentDisposition)
20+
cd.getName() >> 'field'
1921
def bodyPart = Mock(FormDataBodyPart)
2022
bodyPart.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
21-
bodyPart.getName() >> 'field'
22-
bodyPart.getValue() >> 'value'
23-
bodyPart.getFormDataContentDisposition() >> null
23+
bodyPart.getEntityAs(InputStream) >> new ByteArrayInputStream('value'.bytes)
24+
bodyPart.getFormDataContentDisposition() >> cd
2425
def map = [:]
2526

2627
when:
@@ -56,11 +57,12 @@ class MultiPartHelperTest extends Specification {
5657

5758
def "multiple values for same field are accumulated"() {
5859
given:
60+
def cd = Mock(FormDataContentDisposition)
61+
cd.getName() >> 'tag'
5962
def bodyPart = Mock(FormDataBodyPart)
6063
bodyPart.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
61-
bodyPart.getName() >> 'tag'
62-
bodyPart.getValue() >>> ['a', 'b']
63-
bodyPart.getFormDataContentDisposition() >> null
64+
bodyPart.getEntityAs(InputStream) >>> [new ByteArrayInputStream('a'.bytes), new ByteArrayInputStream('b'.bytes)]
65+
bodyPart.getFormDataContentDisposition() >> cd
6466
def map = [:]
6567

6668
when:
@@ -71,6 +73,134 @@ class MultiPartHelperTest extends Specification {
7173
map == [tag: ['a', 'b']]
7274
}
7375

76+
def "text/plain field value longer than MAX_CONTENT_BYTES is truncated"() {
77+
given:
78+
def longValue = 'a' * (MultiPartHelper.MAX_CONTENT_BYTES + 100)
79+
def cd = Mock(FormDataContentDisposition)
80+
cd.getName() >> 'field'
81+
def bodyPart = Mock(FormDataBodyPart)
82+
bodyPart.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
83+
bodyPart.getEntityAs(InputStream) >> new ByteArrayInputStream(longValue.bytes)
84+
bodyPart.getFormDataContentDisposition() >> cd
85+
def map = [:]
86+
87+
when:
88+
MultiPartHelper.collectBodyPart(bodyPart, map, null, null)
89+
90+
then:
91+
map['field'][0] == 'a' * MultiPartHelper.MAX_CONTENT_BYTES
92+
}
93+
94+
def "text/plain part with no declared charset decodes non-ASCII content as UTF-8"() {
95+
given:
96+
def cd = Mock(FormDataContentDisposition)
97+
cd.getName() >> 'field'
98+
def bodyPart = Mock(FormDataBodyPart)
99+
bodyPart.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
100+
bodyPart.getEntityAs(InputStream) >> new ByteArrayInputStream('héllo wörld'.getBytes('UTF-8'))
101+
bodyPart.getFormDataContentDisposition() >> cd
102+
def map = [:]
103+
104+
when:
105+
MultiPartHelper.collectBodyPart(bodyPart, map, null, null)
106+
107+
then:
108+
map == [field: ['héllo wörld']]
109+
}
110+
111+
def "text/plain part with explicit non-UTF-8 charset decodes using that charset"() {
112+
given:
113+
def cd = Mock(FormDataContentDisposition)
114+
cd.getName() >> 'field'
115+
def mediaType = new MediaType('text', 'plain', [charset: 'ISO-8859-1'])
116+
def bodyPart = Mock(FormDataBodyPart)
117+
bodyPart.getMediaType() >> mediaType
118+
bodyPart.getEntityAs(InputStream) >> new ByteArrayInputStream('café'.getBytes('ISO-8859-1'))
119+
bodyPart.getFormDataContentDisposition() >> cd
120+
def map = [:]
121+
122+
when:
123+
MultiPartHelper.collectBodyPart(bodyPart, map, null, null)
124+
125+
then:
126+
map == [field: ['café']]
127+
}
128+
129+
def "MAX_FILES_TO_INSPECT limits number of distinct body map field names"() {
130+
given:
131+
def parts = (1..MultiPartHelper.MAX_FILES_TO_INSPECT + 2).collect { i ->
132+
def cd = Mock(FormDataContentDisposition)
133+
cd.getName() >> "field${i}"
134+
def bp = Mock(FormDataBodyPart)
135+
bp.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
136+
bp.getEntityAs(InputStream) >> new ByteArrayInputStream("value${i}".bytes)
137+
bp.getFormDataContentDisposition() >> cd
138+
bp
139+
}
140+
def map = [:]
141+
142+
when:
143+
parts.each { MultiPartHelper.collectBodyPart(it, map, null, null) }
144+
145+
then:
146+
map.size() == MultiPartHelper.MAX_FILES_TO_INSPECT
147+
}
148+
149+
def "MAX_FILES_TO_INSPECT limits total accumulated values, even for a repeated field name"() {
150+
given: "the map filled up to the cap with distinct field names"
151+
def existing = (1..MultiPartHelper.MAX_FILES_TO_INSPECT).collect { i ->
152+
def cd = Mock(FormDataContentDisposition)
153+
cd.getName() >> "field${i}"
154+
def bp = Mock(FormDataBodyPart)
155+
bp.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
156+
bp.getEntityAs(InputStream) >> { new ByteArrayInputStream("value${i}".bytes) }
157+
bp.getFormDataContentDisposition() >> cd
158+
bp
159+
}
160+
def map = [:]
161+
existing.each { MultiPartHelper.collectBodyPart(it, map, null, null) }
162+
163+
and: "a body part reusing an already-collected field name"
164+
def repeatCd = Mock(FormDataContentDisposition)
165+
repeatCd.getName() >> 'field1'
166+
def repeat = Mock(FormDataBodyPart)
167+
repeat.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
168+
repeat.getEntityAs(InputStream) >> new ByteArrayInputStream('extra'.bytes)
169+
repeat.getFormDataContentDisposition() >> repeatCd
170+
171+
and: "a body part with a brand-new field name"
172+
def freshCd = Mock(FormDataContentDisposition)
173+
freshCd.getName() >> 'brandNew'
174+
def fresh = Mock(FormDataBodyPart)
175+
fresh.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
176+
fresh.getEntityAs(InputStream) >> new ByteArrayInputStream('nope'.bytes)
177+
fresh.getFormDataContentDisposition() >> freshCd
178+
179+
when:
180+
MultiPartHelper.collectBodyPart(repeat, map, null, null)
181+
MultiPartHelper.collectBodyPart(fresh, map, null, null)
182+
183+
then: "the total value cap rejects both the repeated field's extra value and the brand-new field"
184+
map['field1'] == ['value1']
185+
map.size() == MultiPartHelper.MAX_FILES_TO_INSPECT
186+
!map.containsKey('brandNew')
187+
}
188+
189+
def "malformed Content-Disposition on a text/plain part skips body map gracefully"() {
190+
given:
191+
def bodyPart = Mock(FormDataBodyPart)
192+
bodyPart.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
193+
bodyPart.getFormDataContentDisposition() >> { throw new IllegalArgumentException("bad CD") }
194+
def map = [:]
195+
196+
when:
197+
MultiPartHelper.collectBodyPart(bodyPart, map, null, null)
198+
199+
then:
200+
map.isEmpty()
201+
noExceptionThrown()
202+
}
203+
74204
// collectBodyPart — filenames
75205

76206
def "filename is added to list when present"() {
@@ -96,7 +226,7 @@ class MultiPartHelperTest extends Specification {
96226
def bodyPart = Mock(FormDataBodyPart)
97227
bodyPart.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
98228
bodyPart.getName() >> 'f'
99-
bodyPart.getValue() >> 'v'
229+
bodyPart.getEntityAs(InputStream) >> new ByteArrayInputStream('v'.bytes)
100230
bodyPart.getFormDataContentDisposition() >> cd
101231

102232
expect:
@@ -107,10 +237,10 @@ class MultiPartHelperTest extends Specification {
107237
given:
108238
def cd = Mock(FormDataContentDisposition)
109239
cd.getFileName() >> 'upload.txt'
240+
cd.getName() >> 'file'
110241
def bodyPart = Mock(FormDataBodyPart)
111242
bodyPart.getMediaType() >> MediaType.TEXT_PLAIN_TYPE
112-
bodyPart.getName() >> 'file'
113-
bodyPart.getValue() >> 'content'
243+
bodyPart.getEntityAs(InputStream) >> new ByteArrayInputStream('content'.bytes)
114244
bodyPart.getFormDataContentDisposition() >> cd
115245
def map = [:]
116246
def filenames = []

‎dd-java-agent/instrumentation/jersey/jersey-appsec/jersey-appsec-3.0/src/main/java/datadog/trace/instrumentation/jersey3/MultiPartHelper.java‎

Lines changed: 40 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,11 +28,6 @@ public static void collectBodyPart(
2828
Map<String, List<String>> bodyMap,
2929
List<String> filenames,
3030
List<String> filesContent) {
31-
if (bodyMap != null
32-
&& MediaTypes.typeEqual(MediaType.TEXT_PLAIN_TYPE, bodyPart.getMediaType())) {
33-
// BodyPartEntity allows re-reading the part without consuming the stream
34-
bodyMap.computeIfAbsent(bodyPart.getName(), k -> new ArrayList<>()).add(bodyPart.getValue());
35-
}
3631
FormDataContentDisposition cd;
3732
try {
3833
cd = bodyPart.getFormDataContentDisposition();
@@ -41,6 +36,16 @@ public static void collectBodyPart(
4136
// so a single bad part does not abort processing of the remaining parts.
4237
cd = null;
4338
}
39+
if (bodyMap != null
40+
&& cd != null
41+
&& MediaTypes.typeEqual(MediaType.TEXT_PLAIN_TYPE, bodyPart.getMediaType())
42+
&& totalBodyMapValues(bodyMap) < MAX_FILES_TO_INSPECT) {
43+
// readContent() reads the part through a byte-capped decoder (MAX_CONTENT_BYTES) instead of
44+
// the unbounded getValue(); the cap counts total accumulated values across all field names,
45+
// not distinct keys, so repeating the same field name cannot bypass the limit. cd.getName()
46+
// is a safe field accessor, unlike bodyPart.getName() which re-parses the disposition header.
47+
bodyMap.computeIfAbsent(cd.getName(), k -> new ArrayList<>()).add(readContent(bodyPart));
48+
}
4449
// rawFilename == null → no filename attribute → form field → skip filenames and content
4550
// rawFilename == "" → filename attribute present but empty → content YES, filenames NO
4651
// rawFilename != "" → file with name → both
@@ -49,25 +54,52 @@ public static void collectBodyPart(
4954
filenames.add(rawFilename);
5055
}
5156
if (filesContent != null && rawFilename != null && filesContent.size() < MAX_FILES_TO_INSPECT) {
52-
filesContent.add(readContent(bodyPart));
57+
filesContent.add(readFileContent(bodyPart));
58+
}
59+
}
60+
61+
private static int totalBodyMapValues(Map<String, List<String>> bodyMap) {
62+
int total = 0;
63+
for (List<String> values : bodyMap.values()) {
64+
total += values.size();
5365
}
66+
return total;
5467
}
5568

69+
// Used for the body-map/text-field path: Jersey's own getValue() decodes undeclared-charset
70+
// text parts as UTF-8, so this matches that default instead of MultipartContentDecoder's
71+
// platform-dependent fallback.
5672
public static String readContent(FormDataBodyPart bodyPart) {
73+
return read(bodyPart, contentTypeWithDefaultUtf8(bodyPart.getMediaType()));
74+
}
75+
76+
// Used for the filesContent path: keeps parity with the other multipart helpers, which all
77+
// rely on MultipartContentDecoder's own charset fallback for undeclared-charset file content.
78+
public static String readFileContent(FormDataBodyPart bodyPart) {
79+
MediaType mediaType = bodyPart.getMediaType();
80+
return read(bodyPart, mediaType != null ? mediaType.toString() : null);
81+
}
82+
83+
private static String read(FormDataBodyPart bodyPart, String contentType) {
5784
try {
5885
// getEntityAs(InputStream.class) is backed by BodyPartEntity which supports re-reading:
5986
// each call creates a fresh stream from the buffered MIME part data.
6087
try (InputStream is = bodyPart.getEntityAs(InputStream.class)) {
6188
if (is == null) return "";
62-
String contentType =
63-
bodyPart.getMediaType() != null ? bodyPart.getMediaType().toString() : null;
6489
return MultipartContentDecoder.readInputStream(is, MAX_CONTENT_BYTES, contentType);
6590
}
6691
} catch (IOException ignored) {
6792
return "";
6893
}
6994
}
7095

96+
private static String contentTypeWithDefaultUtf8(MediaType mediaType) {
97+
String contentType = mediaType != null ? mediaType.toString() : null;
98+
return MultipartContentDecoder.extractCharset(contentType) == null
99+
? (contentType == null ? "charset=UTF-8" : contentType + "; charset=UTF-8")
100+
: contentType;
101+
}
102+
71103
public static BlockingException tryBlock(RequestContext ctx, Flow<Void> flow, String message) {
72104
Flow.Action action = flow.getAction();
73105
if (action instanceof Flow.Action.RequestBlockingAction) {

0 commit comments

Comments
 (0)