Skip to content

Commit 8a85ec9

Browse files
committed
fix(WebRequest): CWE-190/DoS fix and boundary-parsing refactor
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. Ref: #445
1 parent 3469e82 commit 8a85ec9

1 file changed

Lines changed: 64 additions & 55 deletions

File tree

src/WebRequest.cpp

Lines changed: 64 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
#include <algorithm>
1010
#include <cstring>
1111
#include <memory>
12+
#include <string_view>
1213
#include <utility>
1314

1415
#include "./literals.h"
@@ -490,10 +491,17 @@ bool AsyncWebServerRequest::_parseReqHeader() {
490491
// optional OWS) so that e.g. 'x-boundary=' is not matched.
491492
int bpos = -1;
492493
bool inQuotes = false;
493-
for (int i = 0; i < (int)lowcase.length(); i++) {
494-
char c = lowcase.charAt(i);
494+
const char *lc = lowcase.c_str();
495+
const int llen = (int)lowcase.length();
496+
// Stop early: a ';' followed by OWS and 'boundary=' needs at least
497+
// T_BOUNDARY_LEN+1 chars after it. Without OWS the minimum is
498+
// i+1+T_BOUNDARY_LEN+1 <= llen, i.e. i < llen-(T_BOUNDARY_LEN+1).
499+
// The inner OWS-aware check below still guards the OWS case.
500+
const int lscan = llen - (int)(T_BOUNDARY_LEN + 1);
501+
for (int i = 0; i < lscan; i++) {
502+
char c = lc[i];
495503
if (inQuotes) {
496-
if (c == '\\' && i + 1 < (int)lowcase.length()) {
504+
if (c == '\\' && i + 1 < llen) {
497505
i++; // skip quoted-pair — the escaped character cannot terminate the quoted-string
498506
} else if (c == '"') {
499507
inQuotes = false;
@@ -504,13 +512,17 @@ bool AsyncWebServerRequest::_parseReqHeader() {
504512
} else if (c == ';') {
505513
// Skip OWS after the ';' and check for 'boundary='
506514
int j = i + 1;
507-
while (j < (int)lowcase.length() && (lowcase.charAt(j) == ' ' || lowcase.charAt(j) == '\t')) {
515+
while (j < llen && (lc[j] == ' ' || lc[j] == '\t')) {
508516
j++;
509517
}
510-
// Use strncmp on the raw C string to avoid a heap allocation
511-
// from String::substring() — this code runs on attacker-controlled
512-
// input in a scan loop, so zero-allocation is preferred.
513-
if ((size_t)j + T_BOUNDARY_LEN <= lowcase.length() && std::strncmp(lowcase.c_str() + j, T_BOUNDARY, T_BOUNDARY_LEN) == 0) {
518+
// If there is not enough room left for "boundary=" plus at least
519+
// one value byte, no later ';' can match either — stop scanning.
520+
if (j + (int)T_BOUNDARY_LEN + 1 > llen) {
521+
break;
522+
}
523+
// strncmp stops at the null terminator, so no separate length
524+
// guard is needed: a short suffix causes a non-matching result.
525+
if (std::strncmp(lc + j, T_BOUNDARY, T_BOUNDARY_LEN) == 0) {
514526
bpos = j;
515527
break;
516528
}
@@ -526,64 +538,61 @@ bool AsyncWebServerRequest::_parseReqHeader() {
526538
return true;
527539
}
528540

529-
// Extract the boundary value that follows "boundary=" and strip leading/
530-
// trailing whitespace. The value may be either a token or a
531-
// quoted-string (RFC 2045 §5.1 / RFC 2046 §5.1).
532-
_boundary = value.substring(bpos + (int)T_BOUNDARY_LEN);
533-
_boundary.trim();
534-
535-
if (_boundary.startsWith("\"")) {
536-
// Quoted-string form: scan forward from position 1 for the closing
537-
// double-quote. A quote is escaped only when preceded by an ODD
538-
// number of consecutive backslashes (e.g. \" is escaped, \\" is not
539-
// because the two backslashes escape each other, leaving the quote
540-
// unescaped). Checking only the immediately preceding character
541-
// would mishandle the \\" case.
542-
int endQuote = 1;
543-
while (true) {
544-
endQuote = _boundary.indexOf('"', endQuote);
545-
if (endQuote < 0) {
546-
break; // string ran out — unterminated quote
547-
}
548-
// Count consecutive backslashes immediately before this quote.
549-
int backslashes = 0;
550-
while (endQuote - 1 - backslashes >= 0 && _boundary.charAt(endQuote - 1 - backslashes) == '\\') {
551-
backslashes++;
541+
// Use a std::string_view into the original (mixed-case) header value
542+
// to locate and extract the boundary without any intermediate heap
543+
// allocation. The value may be either a token or a quoted-string
544+
// (RFC 2045 §5.1 / RFC 2046 §5.1).
545+
std::string_view bv(value.c_str() + bpos + T_BOUNDARY_LEN, value.length() - bpos - T_BOUNDARY_LEN);
546+
547+
if (!bv.empty() && bv[0] == '"') {
548+
// Quoted-string form: scan once from the opening '"', unescaping
549+
// quoted-pairs on the fly and writing into _boundary directly.
550+
// This single pass replaces the previous three-pass approach
551+
// (find close-quote, substring, then separate unescape loop).
552+
bv.remove_prefix(1); // skip opening '"'
553+
_boundary.reserve(bv.size());
554+
bool closed = false;
555+
for (size_t i = 0; i < bv.size(); ++i) {
556+
char c = bv[i];
557+
if (c == '\\' && i + 1 < bv.size()) {
558+
_boundary += bv[++i]; // quoted-pair: keep only the escaped char
559+
} else if (c == '"') {
560+
closed = true;
561+
break;
562+
} else {
563+
_boundary += c;
552564
}
553-
if (backslashes % 2 == 0) {
554-
break; // even number of backslashes → quote is real closing quote
565+
if (_boundary.length() > 70) {
566+
async_ws_log_d("Invalid multipart boundary length (%u), aborting", _boundary.length());
567+
_parseState = PARSE_REQ_FAIL;
568+
abort();
569+
return true;
555570
}
556-
endQuote++; // odd number → quote is escaped, keep scanning
557571
}
558-
if (endQuote < 0) {
572+
if (!closed) {
559573
// Opening quote was never closed — malformed header.
560574
async_ws_log_d("Invalid multipart boundary (unterminated quote), aborting");
561575
_parseState = PARSE_REQ_FAIL;
562576
abort();
563577
return true;
564578
}
565-
// Strip the surrounding quotes; content between them is the boundary.
566-
_boundary = _boundary.substring(1, endQuote);
567-
568-
// Unescape quoted-pair sequences so the boundary matches the actual bytes on the wire.
569-
String unescaped;
570-
unescaped.reserve(_boundary.length());
571-
for (size_t i = 0; i < _boundary.length(); ++i) {
572-
char c = _boundary.charAt(i);
573-
if (c == '\\' && (i + 1) < _boundary.length()) {
574-
c = _boundary.charAt(++i);
575-
}
576-
unescaped += c;
577-
}
578-
_boundary = unescaped;
579579
} else {
580-
// Token form: the value ends at the next ';' (start of the next
581-
// parameter) or at the end of the header field.
582-
int semi = _boundary.indexOf(';');
583-
if (semi >= 0) {
584-
_boundary = _boundary.substring(0, semi);
580+
// Token form: value ends at the next ';' or end of string.
581+
// Trim trailing OWS using the view — no copy until the final assign.
582+
auto semi = bv.find(';');
583+
if (semi != std::string_view::npos) {
584+
bv = bv.substr(0, semi);
585+
}
586+
while (!bv.empty() && (bv.back() == ' ' || bv.back() == '\t')) {
587+
bv.remove_suffix(1);
588+
}
589+
if (bv.empty() || bv.size() > 70) {
590+
async_ws_log_d("Invalid multipart boundary length (%u), aborting", (unsigned)bv.size());
591+
_parseState = PARSE_REQ_FAIL;
592+
abort();
593+
return true;
585594
}
586-
_boundary.trim();
595+
_boundary = String(bv.data(), (unsigned int)bv.size());
587596
}
588597

589598
// CWE-190 / DoS fix: RFC 2046 §5.1 limits boundary strings to 70

0 commit comments

Comments
 (0)