fix(WebRequest): CWE-190/DoS fix and boundary-parsing refactor by mathieucarbou · Pull Request #447 · ESP32Async/ESPAsyncWebServer

mathieucarbou · 2026-06-09T11:10:29Z

Tighten multipart boundary parsing in _parseReqHeader():

Replace String::charAt() inner loop with a raw C-string pointer and pre-computed length for faster, allocation-free scanning.
Add an early-exit upper bound on the scan loop so that positions where 'boundary=' cannot possibly fit are skipped entirely.
Replace the three-pass quoted-string extraction (find close-quote, substring, then unescape) with a single-pass approach that writes directly into _boundary using a std::string_view window into the original header value — no intermediate heap allocations.
Enforce the RFC 2046 §5.1 70-character limit on boundary length in both token and quoted-string paths; abort with PARSE_REQ_FAIL on violation to prevent CWE-190 integer-overflow / DoS.
Replace strncmp length guard with a position-based early break that is clearer and avoids the redundant cast.

Copilot

Pull request overview

This PR hardens and refactors multipart boundary= parsing in AsyncWebServerRequest::_parseReqHeader() to reduce allocation overhead during header scanning and to enforce RFC 2046’s 70-character boundary limit as a mitigation for CWE-190/DoS scenarios.

Changes:

Refactors the boundary= parameter scan to use raw C-string access and earlier exit conditions.
Reworks boundary value extraction to a single-pass unescape/write path (token or quoted-string) and enforces the 70-char limit early.
Introduces std::string_view-based slicing to avoid intermediate String::substring() allocations during parsing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mathieucarbou · 2026-06-09T11:20:10Z

@willmmiles : fyi we cannot use string_view if we want to keep supporting arduino 2 and libretiny.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

mathieucarbou · 2026-06-19T05:59:35Z

@me-no-dev @willmmiles : can you guys review / approve this pr do that we can merge it ? It is sitting there since a while now. Thanks !

willmmiles · 2026-06-19T10:38:39Z

+              // If there is not enough room left for "boundary=" plus at least
+              // one value byte, no later ';' can match either — stop scanning.
+              if (j + (int)T_BOUNDARY_LEN + 1 > llen) {
+                break;
+              }


We shouldn't have to check this here - the test at the top of the loop should be enough. The mistake is that lscan needs to deduct the semicolon too.

The code is valid and this check is necessary.
lscan already accounts for semicolon.

I have doubled checked this one with GLM 5.2 to be sure and have a more in depth explanation of why your proposal would fail.

the loop would stop one iteration early and miss valid boundaries like ;boundary=X at the very end of the string.

You're right - this function does require a duplicate check as it is currently arranged. I'm sorry for the confusion - I really couldn't put my finger on what it was that was bugging me with the logic here, and I didn't do a very good job of articulating that unease.

I did a more thorough re-review with Claude and it was very helpful for clarifying my intuition. The deeper issue IMO is that the function tries to mix token analysis in a byte loop. A clearer formulation is to make the outer loop a loop over parameters instead, with "skip logic" over the quoted values inside.

The cleaner version is here: willmmiles@65e128e

It's substantially shorter and clearer IMO. (It's also dropped the toLowerCase call in favor of strncasecmp which should be much faster and avoids the extra allocation.)

Thanks! I will have a look!

@willmmiles I cherry picked your commit and verified it. I added a commit on top of it to fix an issue in your commit that re-introduced the CVE, and updated the MultiPart.ino example. The issue was an non-handled quoted empty boundary parameter. All the test cases in MultiPart.ino are working fine now.

mathieucarbou · 2026-06-20T10:42:32Z

@willmmiles : I have addressed your comments. Could you please have a look ? Thanks!

willmmiles' parameter-loop refactor (65e128e) removed the final _boundary length safety net that existed after both extraction branches, relying on per-branch checks. However, the per-branch checks do not cover the empty quoted-string case: boundary="" closes immediately with _boundary still empty and falls through to _isMultipart = true without rejection. Downstream in EXPECT_BOUNDARY, an empty _boundary would cause --\r\n to be accepted as a valid boundary delimiter, reintroducing the CWE-190 / DoS condition this PR is meant to fix. Restore the final safety check (length == 0 || length > 70) with a comment explaining the empty-quoted-string gap that motivates it. Ref: #447 (comment)

Tighten multipart boundary parsing in _parseReqHeader(): - Replace String::charAt() inner loop with a raw C-string pointer and pre-computed length for faster, allocation-free scanning. - Add an early-exit upper bound on the scan loop so that positions where 'boundary=' cannot possibly fit are skipped entirely. - Replace the three-pass quoted-string extraction (find close-quote, substring, then unescape) with a single-pass approach that writes directly into _boundary using a raw pointer+length pair — no intermediate heap allocations and no C++17 dependency. - Enforce the RFC 2046 §5.1 70-character limit on boundary length in both token and quoted-string paths; abort with PARSE_REQ_FAIL on violation to prevent CWE-190 integer-overflow / DoS. - Replace strncmp length guard with a position-based early break that is clearer and avoids the redundant cast. - Fix ESP8266 build: String(const char*, size_t) is unavailable; use a 71-byte stack buffer + String(const char*) instead (#ifdef ESP8266). - Fix LibreTiny/C++14 build: remove std::string_view (C++17 only); replaced with plain const char* + size_t throughout. Ref: #445

Use a token-based loop instead of a byte loop as the primary parser for multipart options. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

willmmiles' parameter-loop refactor (65e128e) removed the final _boundary length safety net that existed after both extraction branches, relying on per-branch checks. However, the per-branch checks do not cover the empty quoted-string case: boundary="" closes immediately with _boundary still empty and falls through to _isMultipart = true without rejection. Downstream in EXPECT_BOUNDARY, an empty _boundary would cause --\r\n to be accepted as a valid boundary delimiter, reintroducing the CWE-190 / DoS condition this PR is meant to fix. Restore the final safety check (length == 0 || length > 70) with a comment explaining the empty-quoted-string gap that motivates it. Ref: #447 (comment)

mathieucarbou requested review from Copilot, me-no-dev and willmmiles June 9, 2026 11:10

mathieucarbou self-assigned this Jun 9, 2026

Copilot started reviewing on behalf of mathieucarbou June 9, 2026 11:10 View session

mathieucarbou force-pushed the CWE-190 branch from 5b4a35f to 8a85ec9 Compare June 9, 2026 11:11

mathieucarbou mentioned this pull request Jun 9, 2026

Fix CWE-190 - DoS could happen with a specially crafted boundary parameter in a multipart request #445

Merged

mathieucarbou added the Status: Pending Merge label Jun 9, 2026

Copilot AI reviewed Jun 9, 2026

View reviewed changes

Comment thread src/WebRequest.cpp Outdated

Comment thread src/WebRequest.cpp Outdated

mathieucarbou force-pushed the CWE-190 branch 2 times, most recently from 521087c to 9ed7c0d Compare June 9, 2026 11:26

mathieucarbou requested a review from Copilot June 9, 2026 11:26

Copilot started reviewing on behalf of mathieucarbou June 9, 2026 11:27 View session

Copilot AI reviewed Jun 9, 2026

View reviewed changes

Comment thread src/WebRequest.cpp

mathieucarbou requested a review from Copilot June 9, 2026 12:08

Copilot started reviewing on behalf of mathieucarbou June 9, 2026 12:08 View session

Copilot AI reviewed Jun 9, 2026

View reviewed changes

mathieucarbou force-pushed the CWE-190 branch 2 times, most recently from 7bbca28 to d62f601 Compare June 9, 2026 12:47

mathieucarbou force-pushed the CWE-190 branch from d62f601 to 86be0c3 Compare June 19, 2026 05:58

willmmiles reviewed Jun 19, 2026

View reviewed changes

mathieucarbou force-pushed the CWE-190 branch from 86be0c3 to c865b37 Compare June 20, 2026 10:41

mathieucarbou requested a review from willmmiles June 20, 2026 10:41

mathieucarbou and others added 3 commits June 24, 2026 09:34

Tighten up boundary parsing

55b1a44

Use a token-based loop instead of a byte loop as the primary parser for multipart options. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

mathieucarbou force-pushed the CWE-190 branch from 824328b to 4a172a0 Compare June 24, 2026 07:34

Conversation

mathieucarbou commented Jun 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

mathieucarbou commented Jun 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

mathieucarbou commented Jun 19, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willmmiles Jun 19, 2026

Choose a reason for hiding this comment

Uh oh!

mathieucarbou Jun 20, 2026

Choose a reason for hiding this comment

Uh oh!

willmmiles Jun 23, 2026

Choose a reason for hiding this comment

Uh oh!

mathieucarbou Jun 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathieucarbou Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mathieucarbou commented Jun 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mathieucarbou Jun 23, 2026 •

edited

Loading

mathieucarbou Jun 24, 2026 •

edited

Loading