Skip to content

Commit 7bbca28

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 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
1 parent 3469e82 commit 7bbca28

1 file changed

Lines changed: 93 additions & 54 deletions

File tree

src/WebRequest.cpp

Lines changed: 93 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -490,10 +490,17 @@ bool AsyncWebServerRequest::_parseReqHeader() {
490490
// optional OWS) so that e.g. 'x-boundary=' is not matched.
491491
int bpos = -1;
492492
bool inQuotes = false;
493-
for (int i = 0; i < (int)lowcase.length(); i++) {
494-
char c = lowcase.charAt(i);
493+
const char *lc = lowcase.c_str();
494+
const int llen = (int)lowcase.length();
495+
// Stop early: a ';' followed by OWS and 'boundary=' needs at least
496+
// T_BOUNDARY_LEN+1 chars after it. Without OWS the minimum is
497+
// i+1+T_BOUNDARY_LEN+1 <= llen, i.e. i < llen-(T_BOUNDARY_LEN+1).
498+
// The inner OWS-aware check below still guards the OWS case.
499+
const int lscan = llen - (int)(T_BOUNDARY_LEN + 1);
500+
for (int i = 0; i < lscan; i++) {
501+
char c = lc[i];
495502
if (inQuotes) {
496-
if (c == '\\' && i + 1 < (int)lowcase.length()) {
503+
if (c == '\\' && i + 1 < llen) {
497504
i++; // skip quoted-pair — the escaped character cannot terminate the quoted-string
498505
} else if (c == '"') {
499506
inQuotes = false;
@@ -504,13 +511,17 @@ bool AsyncWebServerRequest::_parseReqHeader() {
504511
} else if (c == ';') {
505512
// Skip OWS after the ';' and check for 'boundary='
506513
int j = i + 1;
507-
while (j < (int)lowcase.length() && (lowcase.charAt(j) == ' ' || lowcase.charAt(j) == '\t')) {
514+
while (j < llen && (lc[j] == ' ' || lc[j] == '\t')) {
508515
j++;
509516
}
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) {
517+
// If there is not enough room left for "boundary=" plus at least
518+
// one value byte, no later ';' can match either — stop scanning.
519+
if (j + (int)T_BOUNDARY_LEN + 1 > llen) {
520+
break;
521+
}
522+
// strncmp stops at the null terminator, so no separate length
523+
// guard is needed: a short suffix causes a non-matching result.
524+
if (std::strncmp(lc + j, T_BOUNDARY, T_BOUNDARY_LEN) == 0) {
514525
bpos = j;
515526
break;
516527
}
@@ -526,64 +537,92 @@ bool AsyncWebServerRequest::_parseReqHeader() {
526537
return true;
527538
}
528539

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++;
540+
// Use a raw pointer + length pair into the original (mixed-case) header
541+
// value to extract the boundary without any intermediate heap allocation.
542+
// Avoids std::string_view which requires C++17 (unavailable on LibreTiny).
543+
// The value may be either a token or a quoted-string (RFC 2046 §5.1).
544+
const char *bvdata = value.c_str() + bpos + T_BOUNDARY_LEN;
545+
size_t bvlen = value.length() - bpos - T_BOUNDARY_LEN;
546+
547+
// Strip leading OWS (space/tab) after 'boundary=' to preserve prior
548+
// trim() behavior and handle non-RFC but common whitespace variants.
549+
while (bvlen > 0 && (bvdata[0] == ' ' || bvdata[0] == '\t')) {
550+
bvdata++;
551+
bvlen--;
552+
}
553+
554+
// Clear any previous value — duplicate Content-Type headers must not
555+
// concatenate into the boundary string.
556+
_boundary = String();
557+
558+
if (bvlen > 0 && bvdata[0] == '"') {
559+
// Quoted-string form: scan once from the opening '"', unescaping
560+
// quoted-pairs on the fly and writing into _boundary directly.
561+
bvdata++; // skip opening '"'
562+
bvlen--;
563+
// Reserve at most 70 chars — the RFC 2046 §5.1 maximum — rather than
564+
// the full (attacker-controlled) remaining suffix length.
565+
if (!_boundary.reserve(70)) {
566+
async_ws_log_e("Failed to allocate");
567+
_parseState = PARSE_REQ_FAIL;
568+
abort();
569+
return true;
570+
}
571+
bool closed = false;
572+
for (size_t i = 0; i < bvlen; ++i) {
573+
char c = bvdata[i];
574+
if (c == '\\' && i + 1 < bvlen) {
575+
_boundary += bvdata[++i]; // quoted-pair: keep only the escaped char
576+
} else if (c == '"') {
577+
closed = true;
578+
break;
579+
} else {
580+
_boundary += c;
552581
}
553-
if (backslashes % 2 == 0) {
554-
break; // even number of backslashes → quote is real closing quote
582+
if (_boundary.length() > 70) {
583+
async_ws_log_d("Invalid multipart boundary length (%u), aborting", _boundary.length());
584+
_parseState = PARSE_REQ_FAIL;
585+
abort();
586+
return true;
555587
}
556-
endQuote++; // odd number → quote is escaped, keep scanning
557588
}
558-
if (endQuote < 0) {
589+
if (!closed) {
559590
// Opening quote was never closed — malformed header.
560591
async_ws_log_d("Invalid multipart boundary (unterminated quote), aborting");
561592
_parseState = PARSE_REQ_FAIL;
562593
abort();
563594
return true;
564595
}
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);
596+
} else {
597+
// Token form: value ends at the next ';' or end of string.
598+
// Trim trailing OWS — no copy until the final assign.
599+
for (size_t k = 0; k < bvlen; ++k) {
600+
if (bvdata[k] == ';') {
601+
bvlen = k;
602+
break;
575603
}
576-
unescaped += c;
577604
}
578-
_boundary = unescaped;
579-
} 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);
605+
while (bvlen > 0 && (bvdata[bvlen - 1] == ' ' || bvdata[bvlen - 1] == '\t')) {
606+
bvlen--;
585607
}
586-
_boundary.trim();
608+
if (bvlen == 0 || bvlen > 70) {
609+
async_ws_log_d("Invalid multipart boundary length (%u), aborting", (unsigned)bvlen);
610+
_parseState = PARSE_REQ_FAIL;
611+
abort();
612+
return true;
613+
}
614+
#ifdef ESP8266
615+
{
616+
// ESP8266 Arduino String lacks String(const char*, size_t).
617+
// bvlen <= 70 is guaranteed above, so a stack buffer is safe.
618+
char buf[71];
619+
memcpy(buf, bvdata, bvlen);
620+
buf[bvlen] = '\0';
621+
_boundary = String(buf);
622+
}
623+
#else
624+
_boundary = String(bvdata, (unsigned int)bvlen);
625+
#endif
587626
}
588627

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

0 commit comments

Comments
 (0)