diff --git a/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartFormDataReaderInstrumentation.java b/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartFormDataReaderInstrumentation.java index 4a435fb0c93..6e5d5542cbc 100644 --- a/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartFormDataReaderInstrumentation.java +++ b/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartFormDataReaderInstrumentation.java @@ -16,14 +16,10 @@ import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.gateway.RequestContextSlot; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import java.io.IOException; -import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.function.BiFunction; import net.bytebuddy.asm.Advice; -import org.jboss.resteasy.plugins.providers.multipart.InputPart; import org.jboss.resteasy.plugins.providers.multipart.MultipartFormDataInput; @AutoService(InstrumenterModule.class) @@ -67,8 +63,7 @@ public static class ReadFromAdvice { static void after( @Advice.Return final MultipartFormDataInput ret, @ActiveRequestContext RequestContext reqCtx, - @Advice.Thrown(readOnly = false) Throwable t) - throws IOException { + @Advice.Thrown(readOnly = false) Throwable t) { if (ret == null || t != null) { return; } @@ -85,14 +80,7 @@ static void after( } if (callback != null) { - Map> m = new HashMap<>(); - for (Map.Entry> e : ret.getFormDataMap().entrySet()) { - List strings = new ArrayList<>(); - m.put(e.getKey(), strings); - for (InputPart inputPart : e.getValue()) { - strings.add(inputPart.getBodyAsString()); - } - } + Map> m = MultipartHelper.collectBodyMap(ret); Flow flow = callback.apply(reqCtx, m); BlockingException be = diff --git a/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartHelper.java b/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartHelper.java index b9cd5195918..3dfd369973c 100644 --- a/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartHelper.java +++ b/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/main/java/datadog/trace/instrumentation/resteasy/MultipartHelper.java @@ -6,18 +6,22 @@ import datadog.trace.api.gateway.Flow; import datadog.trace.api.gateway.RequestContext; import datadog.trace.api.http.MultipartContentDecoder; -import java.io.IOException; import java.io.InputStream; import java.lang.reflect.Method; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Map.Entry; import org.jboss.resteasy.plugins.providers.multipart.InputPart; import org.jboss.resteasy.plugins.providers.multipart.MultipartFormDataInput; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public final class MultipartHelper { + private static final Logger log = LoggerFactory.getLogger(MultipartHelper.class); + public static final int MAX_CONTENT_BYTES = Config.get().getAppSecMaxFileContentBytes(); public static final int MAX_FILES_TO_INSPECT = Config.get().getAppSecMaxFileContentCount(); @@ -35,6 +39,142 @@ private MultipartHelper() {} GET_HEADERS = m; } + /** + * Builds the {@code server.request.body} map out of the multipart parts. + * + *

Only text/plain parts with no {@code filename} attribute are collected, matching the intent + * of the Jersey reference: file parts (including a file declared with a {@code text/plain} + * content-type) are reported separately via {@link #collectFilenames} / {@link + * #collectFilesContent} and must not also consume the body-map budget, or a request could pad out + * the cap with disposable text-file parts and push a real form field out of the map. The number + * of collected values is capped by {@link #MAX_FILES_TO_INSPECT}. The cap counts the total + * accumulated values across all field names, not the distinct keys: {@code getFormDataMap()} + * already groups parts by field name, so a per-key cap would be trivially bypassed by repeating + * the same field name on every part. + */ + public static Map> collectBodyMap(MultipartFormDataInput ret) { + Map> bodyMap = new HashMap<>(); + int total = 0; + for (Map.Entry> e : ret.getFormDataMap().entrySet()) { + for (InputPart inputPart : e.getValue()) { + Map> headers = headersOf(inputPart); + String contentType = contentTypeOf(headers); + if (!isTextPlain(contentType) || hasFilename(headers)) { + continue; + } + if (total >= MAX_FILES_TO_INSPECT) { + return bodyMap; + } + bodyMap + .computeIfAbsent(e.getKey(), k -> new ArrayList<>()) + .add(readContent(inputPart, contentTypeWithDefaultUtf8(contentType))); + total++; + } + } + return bodyMap; + } + + // No Content-Type header means RESTEasy defaults the part to text/plain (see InputPart's own + // javadoc), so a null contentType is treated as text/plain here as well. + private static boolean isTextPlain(String contentType) { + if (contentType == null) { + return true; + } + int semi = contentType.indexOf(';'); + String mediaType = (semi >= 0 ? contentType.substring(0, semi) : contentType).trim(); + return mediaType.equalsIgnoreCase("text/plain"); + } + + // Used for the body-map/text-field path only: matches Jersey's own getValue(), which decodes + // undeclared-charset text parts as UTF-8 instead of falling back to the JVM platform charset + // (MultipartContentDecoder's default for the filesContent path, kept as-is for parity with the + // other multipart integrations). + private static String contentTypeWithDefaultUtf8(String contentType) { + return MultipartContentDecoder.extractCharset(contentType) == null + ? (contentType == null ? "charset=UTF-8" : contentType + "; charset=UTF-8") + : contentType; + } + + // Used by collectBodyMap only: collectFilenames/collectFilesContent do their own reflective + // getHeaders() call and are intentionally left untouched (out of scope, already correct). + private static Map> headersOf(InputPart inputPart) { + if (GET_HEADERS == null) { + return null; + } + try { + @SuppressWarnings("unchecked") + Map> headers = (Map>) GET_HEADERS.invoke(inputPart); + return headers; + } catch (Exception e) { + // Reflective getHeaders() call failed (unexpected InputPart implementation): fall back to + // resolving no headers rather than aborting the whole request's body-map collection. + log.debug("Failed to read multipart part headers via reflection", e); + return null; + } + } + + private static String contentTypeOf(Map> headers) { + if (headers == null) { + return null; + } + List ctHeaders = getHeaderCaseInsensitive(headers, "Content-Type"); + return (ctHeaders != null && !ctHeaders.isEmpty()) ? ctHeaders.get(0) : null; + } + + // A part with a filename attribute (present, even if empty) is a file upload, not a form field, + // regardless of its declared content-type: a file can be declared text/plain and would otherwise + // pass isTextPlain() and consume the body-map budget meant for genuine form fields. Uses + // hasFilenameParam() rather than rawFilenameFromContentDisposition(), which deliberately ignores + // the RFC 5987 "filename*" form: a part carrying only "filename*" must still be excluded here. + // collectFilesContent() uses the same hasFilenameParam() gate, so such a part is still inspected + // there even though this file never decodes its filename* value. + private static boolean hasFilename(Map> headers) { + if (headers == null) { + return false; + } + List cdHeaders = getHeaderCaseInsensitive(headers, "Content-Disposition"); + if (cdHeaders == null || cdHeaders.isEmpty()) { + return false; + } + return hasFilenameParam(cdHeaders.get(0)); + } + + // Presence-only counterpart of rawFilenameFromContentDisposition(): recognizes both the plain + // "filename" parameter and the RFC 5987 extended "filename*" form (e.g. filename*=UTF-8''a.txt), + // since either form marks the part as a file upload. Unlike rawFilenameFromContentDisposition(), + // this never needs to decode the value, so the RFC 5987 charset/percent-encoding is irrelevant + // here. Shares the same quote-aware semicolon scanning; see rawFilenameFromContentDisposition() + // for the rationale. + private static boolean hasFilenameParam(String cd) { + if (cd == null) return false; + int i = 0; + int len = cd.length(); + while (i < len) { + while (i < len && cd.charAt(i) != ';') { + if (cd.charAt(i) == '"') { + i++; + while (i < len && cd.charAt(i) != '"') { + if (cd.charAt(i) == '\\') i++; + i++; + } + } + i++; + } + if (i >= len) break; + i++; + while (i < len && (cd.charAt(i) == ' ' || cd.charAt(i) == '\t')) i++; + if (cd.regionMatches(true, i, "filename", 0, 8)) { + int j = i + 8; + if (j < len && cd.charAt(j) == '*') j++; + while (j < len && (cd.charAt(j) == ' ' || cd.charAt(j) == '\t')) j++; + if (j < len && cd.charAt(j) == '=') { + return true; + } + } + } + return false; + } + public static List collectFilenames(MultipartFormDataInput ret) { List filenames = new ArrayList<>(); if (GET_HEADERS == null) { @@ -89,9 +229,9 @@ public static List collectFilesContent(MultipartFormDataInput ret) { if (cdHeaders == null || cdHeaders.isEmpty()) { continue; } - // rawFilenameFromContentDisposition returns null if filename attr absent, - // otherwise returns the value (possibly empty) — both cases warrant content inspection - if (rawFilenameFromContentDisposition(cdHeaders.get(0)) == null) { + // hasFilenameParam recognizes both filename and filename* (see its javadoc): a part with + // neither is a plain form field, already covered by collectBodyMap, and skipped here. + if (!hasFilenameParam(cdHeaders.get(0))) { continue; } List ctHeaders = getHeaderCaseInsensitive(headers, "Content-Type"); @@ -121,7 +261,10 @@ static String readContent(InputPart inputPart, String contentType) { try (InputStream is = inputPart.getBody(InputStream.class, null)) { if (is == null) return ""; return MultipartContentDecoder.readInputStream(is, MAX_CONTENT_BYTES, contentType); - } catch (IOException ignored) { + } catch (Exception e) { + // getBody()/readInputStream() can throw unchecked exceptions too (e.g. a MessageBodyReader + // lookup failure); one bad part must not abort the whole request's body/content collection. + log.debug("Failed to read multipart part content, returning empty string", e); return ""; } } diff --git a/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/test/groovy/MultipartHelperTest.groovy b/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/test/groovy/MultipartHelperTest.groovy index ab08b74d151..c56ac989b0c 100644 --- a/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/test/groovy/MultipartHelperTest.groovy +++ b/dd-java-agent/instrumentation/resteasy/resteasy-appsec-3.0/src/test/groovy/MultipartHelperTest.groovy @@ -141,7 +141,7 @@ class MultipartHelperTest extends Specification { MultipartHelper.filenameFromContentDisposition("form-data; filename*=UTF-8''evil.php") == null } - // collectFilesContent + // header fixtures private static MultivaluedMapImpl headers(String cd) { def h = new MultivaluedMapImpl() @@ -149,6 +149,275 @@ class MultipartHelperTest extends Specification { h } + private static MultivaluedMapImpl headers(String cd, String contentType) { + def h = headers(cd) + h.add('Content-Type', contentType) + h + } + + // collectBodyMap + + def "collectBodyMap collects text parts grouped by field name"() { + given: + def p1 = Mock(InputPart) + p1.getHeaders() >> headers('form-data; name="field"') + p1.getBody(_, _) >> new ByteArrayInputStream('value'.bytes) + def p2 = Mock(InputPart) + p2.getHeaders() >> headers('form-data; name="tag"') + p2.getBody(_, _) >> new ByteArrayInputStream('a'.bytes) + def p3 = Mock(InputPart) + p3.getHeaders() >> headers('form-data; name="tag"') + p3.getBody(_, _) >> new ByteArrayInputStream('b'.bytes) + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['field': [p1], 'tag': [p2, p3]] + + when: + def result = MultipartHelper.collectBodyMap(ret) + + then: + result == ['field': ['value'], 'tag': ['a', 'b']] + } + + def "collectBodyMap returns an empty map when there are no parts"() { + given: + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> [:] + + expect: + MultipartHelper.collectBodyMap(ret).isEmpty() + } + + def "collectBodyMap decodes using the part declared charset"() { + given: + def part = Mock(InputPart) + part.getHeaders() >> headers('form-data; name="field"', 'text/plain; charset=ISO-8859-1') + part.getBody(_, _) >> new ByteArrayInputStream('café'.getBytes('ISO-8859-1')) + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['field': [part]] + + expect: + MultipartHelper.collectBodyMap(ret) == ['field': ['café']] + } + + def "collectBodyMap truncates a value longer than MAX_CONTENT_BYTES"() { + given: + def longValue = 'a' * (MultipartHelper.MAX_CONTENT_BYTES + 100) + def part = Mock(InputPart) + part.getHeaders() >> headers('form-data; name="field"') + part.getBody(_, _) >> new ByteArrayInputStream(longValue.bytes) + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['field': [part]] + + when: + def result = MultipartHelper.collectBodyMap(ret) + + then: + result['field'][0] == 'a' * MultipartHelper.MAX_CONTENT_BYTES + } + + def "collectBodyMap caps total values across distinct field names"() { + given: + def entries = [:] + (1..MultipartHelper.MAX_FILES_TO_INSPECT + 3).each { i -> + def p = Mock(InputPart) + p.getHeaders() >> headers("form-data; name=\"field${i}\"") + p.getBody(_, _) >> new ByteArrayInputStream("value${i}".bytes) + entries["field${i}".toString()] = [p] + } + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> entries + + when: + def result = MultipartHelper.collectBodyMap(ret) + + then: + result.values().sum { it.size() } == MultipartHelper.MAX_FILES_TO_INSPECT + } + + def "collectBodyMap caps total values even when every part reuses the same field name"() { + given: "all parts share a single field name, so the map only ever has one key" + def parts = (1..MultipartHelper.MAX_FILES_TO_INSPECT + 5).collect { i -> + def p = Mock(InputPart) + p.getHeaders() >> headers('form-data; name="same"') + p.getBody(_, _) >> new ByteArrayInputStream("value${i}".bytes) + p + } + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['same': parts] + + when: + def result = MultipartHelper.collectBodyMap(ret) + + then: "the cap counts total values, not distinct keys" + result.size() == 1 + result['same'].size() == MultipartHelper.MAX_FILES_TO_INSPECT + } + + def "collectBodyMap keeps accumulating values for an already present field below the cap"() { + given: + def parts = (1..3).collect { i -> + def p = Mock(InputPart) + p.getHeaders() >> headers('form-data; name="tag"') + p.getBody(_, _) >> new ByteArrayInputStream("v${i}".bytes) + p + } + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['tag': parts] + + expect: + MultipartHelper.collectBodyMap(ret) == ['tag': ['v1', 'v2', 'v3']] + } + + def "collectBodyMap maps a part to an empty string when reading it fails"() { + given: + def failing = Mock(InputPart) + failing.getHeaders() >> headers('form-data; name="bad"') + failing.getBody(_, _) >> { throw new IOException('boom') } + def ok = Mock(InputPart) + ok.getHeaders() >> headers('form-data; name="good"') + ok.getBody(_, _) >> new ByteArrayInputStream('fine'.bytes) + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['bad': [failing], 'good': [ok]] + + when: + def result = MultipartHelper.collectBodyMap(ret) + + then: + result == ['bad': [''], 'good': ['fine']] + noExceptionThrown() + } + + def "collectBodyMap maps a part to an empty string when reading it throws an unchecked exception"() { + given: + def failing = Mock(InputPart) + failing.getHeaders() >> headers('form-data; name="bad"') + failing.getBody(_, _) >> { throw new RuntimeException('no MessageBodyReader') } + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['bad': [failing]] + + when: + def result = MultipartHelper.collectBodyMap(ret) + + then: + result == ['bad': ['']] + noExceptionThrown() + } + + def "collectBodyMap treats a part whose getHeaders() throws as having no content-type, without aborting"() { + given: + def broken = Mock(InputPart) + broken.getHeaders() >> { throw new IllegalStateException('boom') } + broken.getBody(_, _) >> new ByteArrayInputStream('value'.bytes) + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['field': [broken]] + + when: + def result = MultipartHelper.collectBodyMap(ret) + + then: + result == ['field': ['value']] + noExceptionThrown() + } + + def "collectBodyMap treats a part whose getHeaders() returns null as having no content-type"() { + given: + def part = Mock(InputPart) + part.getHeaders() >> null + part.getBody(_, _) >> new ByteArrayInputStream('value'.bytes) + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['field': [part]] + + expect: + MultipartHelper.collectBodyMap(ret) == ['field': ['value']] + } + + def "collectBodyMap decodes an undeclared-charset value as UTF-8, not the JVM platform default"() { + given: + def part = Mock(InputPart) + part.getHeaders() >> headers('form-data; name="field"') + part.getBody(_, _) >> new ByteArrayInputStream('café'.getBytes('UTF-8')) + def ret = Mock(MultipartFormDataInput) + ret.getFormDataMap() >> ['field': [part]] + + expect: + MultipartHelper.collectBodyMap(ret) == ['field': ['café']] + } + + def "collectBodyMap excludes a non-text/plain part and does not consume the cap for it"() { + given: "a file part and a text part sharing the same request" + def file = Mock(InputPart) + file.getHeaders() >> headers('form-data; name="upload"; filename="a.bin"', 'application/octet-stream') + def text = Mock(InputPart) + text.getHeaders() >> headers('form-data; name="q"') + text.getBody(_, _) >> new ByteArrayInputStream('