Fix unbounded multipart form-field buffering in Jetty 8 PartHelper#11876
Conversation
… advice PartHelper.readPartContent() copied a multipart Part's InputStream into an unbounded ByteArrayOutputStream before any WAF size check applied, allowing a single oversized text field to exhaust heap. Cap the read at Config.get().getAppSecMaxFileContentBytes(), reusing the existing file-content byte cap since there is no dedicated form-field knob.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
jandro996
added
type: bug fix
extractFormFields() bounded the bytes read per field but not the number of fields processed, leaving total allocation unbounded as O(fields x byteCap). Cap the field count via Config.get().getAppSecMaxFileContentCount(), mirroring the MAX_FILES_TO_INSPECT pattern used by MultipartHelper#extractContents() (PR #11706) for file content.
|
@codex review |
1 similar comment
|
@codex review |
![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: 37fc40deb4
ℹ️ 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: 54eabd9 | 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. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 217473510677503781 took longer than expected. The current limit for the base branch 'master' is 120 minutes. Possible reasons:
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
What Does This Do
PartHelper.readPartContent()(jetty-appsec-8.1.3) so it no longer buffers a multipart form-field'sInputStreaminto an unboundedByteArrayOutputStream.Config.get().getAppSecMaxFileContentBytes()(appsec.max.file-content.bytes, default 4096 bytes). There is no dedicated "max form-field bytes" config, so this intentionally reuses the existing file-content knob — documented with a code comment at the call site.PartHelper.extractFormFields()viaConfig.get().getAppSecMaxFileContentCount(), mirroring theMAX_FILES_TO_INSPECTpattern used byMultipartHelper#extractContents()(PR Add server.request.body.files_content AppSec address support for Jetty #11706) for file content. Bounding only the per-field byte read left the total allocation unbounded asO(fields × byteCap)— an attacker could still exhaust memory with many small fields instead of one large one.charsetFromContentType()and its explicit UTF-8 fallback untouched; the fix does not switch toMultipartContentDecoder(used for file content elsewhere), since that decoder falls back toCharset.defaultCharset(), which would make WAF pattern-matching non-deterministic across hosts.extractFormFieldsTruncatesFieldExceedingMaxContentBytesandextractFormFieldsCapsAtMaxFileContentCounttoPartHelperTest.java, asserting the byte truncation and the field-count cap land exactly at their respective configured limits.Motivation
Jetty 8 (Servlet 3.0) has no
getParameterMap()-equivalent for multipart requests, soPartHelpermust manually read each form-fieldPart's stream to extract its text value. Before this fix, that read had no upper bound — either per field or across all fields — so a remote attacker could exhaust heap / cause severe GC pressure with a single oversized text field, or with a large number of small fields. Found via automated security scan (APMSP-3580).Additional Notes
MultipartContentDecoder/MAX_CONTENT_BYTES/MAX_FILES_TO_INSPECTto this same class for an unrelatedfiles_contentfeature, and explicitly leavesreadPartContent()/extractFormFields()untouched. Expect a small, easily-resolvable merge conflict on imports/constants depending on merge order — this fix does not depend on Add server.request.body.files_content AppSec address support for Jetty #11706 landing first, since bothConfig.get().getAppSecMaxFileContentBytes()andConfig.get().getAppSecMaxFileContentCount()already exist independently ininternal-api.readPartContent()andextractFormFields().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-3580
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.