Bound multipart text/plain field materialization in Jersey AppSec#11944
Bound multipart text/plain field materialization in Jersey AppSec#11944jandro996 wants to merge 5 commits into
Conversation
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.
|
@codex review |
jandro996
added
type: bug fix
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 124e9188ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
🎯 Code Coverage (details) 🔗 Commit SHA: 06683b8 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
…d 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.
…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.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…latform charset" This reverts commit e25666a.
…ultipartContentDecoder 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.
jandro996
requested review from
ValentinZakharov,
claponcet,
manuel-alvarez-alvarez and
vandonr
and removed request for
a team
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06683b8439
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return MultipartContentDecoder.readInputStream( | ||
| is, MAX_CONTENT_BYTES, contentTypeWithDefaultUtf8(bodyPart.getMediaType())); |
There was a problem hiding this comment.
Fresh evidence after the charset follow-up is that contentTypeWithDefaultUtf8 now sits in readContent, which is also invoked by the filesContent branch. When requestFilesContent is enabled for a Jersey upload whose file part has no charset and is encoded with the JVM/default legacy charset, this forces UTF-8 instead of the previous MultipartContentDecoder fallback used by Jersey file content and the other multipart helpers, so non-ASCII bytes are replaced before WAF inspection. Keep the UTF-8 default on the text field/body-map path only.
Useful? React with 👍 / 👎.
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Validated charset defaulting and cap boundary: contentTypeWithDefaultUtf8 correctly appends UTF-8 when no charset is declared and preserves explicit charsets unchanged; the totalBodyMapValues cap blocks ALL additions once the total reaches MAX_FILES_TO_INSPECT — including repeated field names — which is more restrictive than the PR description's "distinct-field-name" framing, but intentional per the in-code comment and test assertions.
📊 Validated against 28 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 06683b8 · What is Autotest? · Any feedback? Reach out in #autotest
1 participant
What Does This Do
bodyPart.getValue()call inMultiPartHelper.collectBodyPart(jersey2 and jersey3 AppSec modules) withreadContent(bodyPart), which reads the part through the existing byte-cappedMultipartContentDecoder(bounded byMAX_CONTENT_BYTES, backed byConfig.get().getAppSecMaxFileContentBytes()).bodyMapcollected fromtext/plainmultipart fields, symmetric to the existingMAX_FILES_TO_INSPECTcap already applied tofilesContent. Values for a field name already present in the map keep accumulating; only new distinct field names are capped onceMAX_FILES_TO_INSPECTis reached.dd-java-agent/instrumentation/jersey/jersey-appsec/jersey-appsec-2.0(javax.ws.rs) andjersey-appsec-3.0(jakarta.ws.rs) — the twoMultiPartHelper.javafiles remain byte-for-byte identical modulo the namespace.MultiPartHelperTest.groovyin both modules to mockbodyPart.getEntityAs(InputStream)instead ofbodyPart.getValue()for the paths affected by the fix, and adds new test cases: truncation of atext/plainfield value longer thanMAX_CONTENT_BYTES, the field-name cap (MAX_FILES_TO_INSPECT) limiting distinct body-map keys, and accumulation of additional values for an already-seen field name even once the field-name cap is reached.Motivation
Jersey 2/3 AppSec instrumentation materialized
text/plainmultipart field values via the unboundedgetValue()and accumulated an unlimited number of distinct field names into the body map before any WAF limit was applied. A crafted multipart request with a very largetext/plainfield or a very large number of distinct field names could exhaust heap/CPU before hitting any bound, since neither content size nor field count was capped on this path.Additional Notes
Scope is intentionally limited to Jersey. RESTEasy's
MultipartFormDataReaderInstrumentationhas a related unbounded-read pattern (getBodyAsString()on all parts) that is out of scope for this PR and should be addressed as a follow-up.Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APMSP-3237
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.