Skip to content

Commit 670dc2a

Browse files
Fix CWE-190: integer overflow in multipart boundary parser (DoS)
_boundaryPosition was declared as uint8_t. A remote attacker could send a Content-Type header with a boundary string of exactly 256 bytes, causing the increment in _parseMultipartPostByte() to overflow back to 0. The parser would then loop indefinitely in the BOUNDARY_OR_DATA state, consuming 100% CPU and triggering the FreeRTOS watchdog on ESP32/ESP8266. Fixes: - ESPAsyncWebServer.h: change _boundaryPosition from uint8_t to size_t, eliminating the overflow entirely. - WebRequest.cpp (_parseMultipartPostByte): change all rewind loop variables from uint8_t to size_t (BOUNDARY_OR_DATA, DASH3_OR_RETURN2, EXPECT_FEED2). - WebRequest.cpp (_parseMultipartPostByte / EXPECT_BOUNDARY): add a comment explaining the intentional unsigned underflow when _parsedLength < 2. - WebRequest.cpp (_parseReqHeader): validate the boundary value extracted from the Content-Type header before any body parsing begins: * Media type comparison changed from case-sensitive startsWith() to a case-insensitive check on a pre-lowercased copy of the value, so that e.g. 'Multipart/form-data' is correctly recognised (RFC 2045 §5.1). * Require the 'boundary=' token to be a proper RFC 2045 parameter (immediately preceded by ';'), preventing false matches on tokens whose name merely ends in 'boundary=' (e.g. 'x-boundary='). * Parse the boundary value as either a token or a quoted-string per RFC 2046 §5.1. Quoted-string form correctly handles backslash-escaped quotes ("), pairs of backslashes (\), and rejects unterminated quoted strings. The closing-quote detector counts consecutive backslashes before the quote so that an even count (backslashes cancelling each other) correctly identifies the real closing quote. * Reject boundaries that are empty or longer than 70 characters (RFC 2046 §5.1 hard limit); drop the connection immediately with PARSE_REQ_FAIL + abort() instead of continuing. * Trim _contentType defensively after extraction. - AsyncWebHeader.cpp (parse): fix OWS handling per RFC 7230 §3.2.3. The previous code only skipped a single space after the colon; the correct rule is OWS = *( SP / HTAB ). This affected every parsed header: Content-Type, Upgrade, Expect, Transfer-Encoding, Host, etc. The fix is applied at the source so all header branches benefit. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
1 parent a87fcdb commit 670dc2a

4 files changed

Lines changed: 141 additions & 12 deletions

File tree

src/AsyncWebHeader.cpp

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,9 @@ const AsyncWebHeader AsyncWebHeader::parse(const char *data) {
2323
return AsyncWebHeader(); // Header name cannot be empty
2424
}
2525
const char *startOfValue = colon + 1; // Skip the colon
26-
// skip one optional whitespace after the colon
27-
if (*startOfValue == ' ') {
26+
// RFC 7230 §3.2.3: OWS (optional whitespace) = *( SP / HTAB )
27+
// Skip all leading spaces and tabs, not just the first one.
28+
while (*startOfValue == ' ' || *startOfValue == '\t') {
2829
startOfValue++;
2930
}
3031
String name;

src/ESPAsyncWebServer.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -465,7 +465,9 @@ class AsyncWebServerRequest {
465465
std::unordered_map<const char *, String, std::hash<const char *>, std::equal_to<const char *>> _attributes;
466466

467467
uint8_t _multiParseState;
468-
uint8_t _boundaryPosition;
468+
// CWE-190 fix: was uint8_t, which would overflow to 0 for boundaries >= 256
469+
// bytes and cause the BOUNDARY_OR_DATA parser loop to never terminate (DoS).
470+
size_t _boundaryPosition;
469471
size_t _itemStartIndex;
470472
size_t _itemSize;
471473
String _itemName;

src/WebRequest.cpp

Lines changed: 133 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -473,9 +473,129 @@ bool AsyncWebServerRequest::_parseReqHeader() {
473473
_host = value;
474474
} else if (name.equalsIgnoreCase(T_Content_Type)) {
475475
_contentType = value.substring(0, value.indexOf(';'));
476-
if (value.startsWith(T_MULTIPART_)) {
477-
_boundary = value.substring(value.indexOf('=') + 1);
478-
_boundary.replace(String('"'), String());
476+
// Trim _contentType defensively; AsyncWebHeader::parse now strips all
477+
// leading OWS per RFC 7230, but trim() guards against any future change.
478+
_contentType.trim();
479+
// Media types are case-insensitive (RFC 2045 §5.1), so lowercase the
480+
// entire header value once and reuse it for all subsequent searches.
481+
String lowcase(value);
482+
lowcase.toLowerCase();
483+
if (lowcase.startsWith(T_MULTIPART_)) {
484+
// Case-insensitive, quote-aware search for the boundary parameter.
485+
// We scan forward tracking quoted-string state so that a 'boundary='
486+
// substring inside a quoted parameter value (e.g. foo="x; boundary=y")
487+
// is never mistaken for the real parameter delimiter. A ';' inside a
488+
// quoted-string is not a parameter separator per RFC 2045 §5.1.
489+
// We also require 'boundary=' to be immediately preceded by ';' (with
490+
// optional OWS) so that e.g. 'x-boundary=' is not matched.
491+
int bpos = -1;
492+
bool inQuotes = false;
493+
for (int i = 0; i < (int)lowcase.length(); i++) {
494+
char c = lowcase.charAt(i);
495+
if (inQuotes) {
496+
if (c == '\\' && i + 1 < (int)lowcase.length()) {
497+
i++; // skip quoted-pair — the escaped character cannot be '"'
498+
} else if (c == '"') {
499+
inQuotes = false;
500+
}
501+
} else {
502+
if (c == '"') {
503+
inQuotes = true;
504+
} else if (c == ';') {
505+
// Skip OWS after the ';' and check for 'boundary='
506+
int j = i + 1;
507+
while (j < (int)lowcase.length() && (lowcase.charAt(j) == ' ' || lowcase.charAt(j) == '\t')) {
508+
j++;
509+
}
510+
if (lowcase.substring(j).startsWith(T_BOUNDARY)) {
511+
bpos = j;
512+
break;
513+
}
514+
}
515+
}
516+
}
517+
518+
if (bpos < 0) {
519+
// No valid boundary parameter found — cannot parse multipart body.
520+
async_ws_log_d("Missing multipart boundary parameter, aborting");
521+
_parseState = PARSE_REQ_FAIL;
522+
abort();
523+
return true;
524+
}
525+
526+
// Extract the boundary value that follows "boundary=" and strip leading/
527+
// trailing whitespace. The value may be either a token or a
528+
// quoted-string (RFC 2045 §5.1 / RFC 2046 §5.1).
529+
_boundary = value.substring(bpos + (int)T_BOUNDARY_LEN);
530+
_boundary.trim();
531+
532+
if (_boundary.startsWith("\"")) {
533+
// Quoted-string form: scan forward from position 1 for the closing
534+
// double-quote. A quote is escaped only when preceded by an ODD
535+
// number of consecutive backslashes (e.g. \" is escaped, \\" is not
536+
// because the two backslashes escape each other, leaving the quote
537+
// unescaped). Checking only the immediately preceding character
538+
// would mishandle the \\" case.
539+
int endQuote = 1;
540+
while (true) {
541+
endQuote = _boundary.indexOf('"', endQuote);
542+
if (endQuote < 0) {
543+
break; // string ran out — unterminated quote
544+
}
545+
// Count consecutive backslashes immediately before this quote.
546+
int backslashes = 0;
547+
while (endQuote - 1 - backslashes >= 0 && _boundary.charAt(endQuote - 1 - backslashes) == '\\') {
548+
backslashes++;
549+
}
550+
if (backslashes % 2 == 0) {
551+
break; // even number of backslashes → quote is real closing quote
552+
}
553+
endQuote++; // odd number → quote is escaped, keep scanning
554+
}
555+
if (endQuote < 0) {
556+
// Opening quote was never closed — malformed header.
557+
async_ws_log_d("Invalid multipart boundary (unterminated quote), aborting");
558+
_parseState = PARSE_REQ_FAIL;
559+
abort();
560+
return true;
561+
}
562+
// Strip the surrounding quotes; content between them is the boundary.
563+
_boundary = _boundary.substring(1, endQuote);
564+
565+
// Unescape quoted-pair sequences so the boundary matches the actual bytes on the wire.
566+
String unescaped;
567+
unescaped.reserve(_boundary.length());
568+
for (size_t i = 0; i < _boundary.length(); ++i) {
569+
char c = _boundary.charAt(i);
570+
if (c == '\\' && (i + 1) < _boundary.length()) {
571+
c = _boundary.charAt(++i);
572+
}
573+
unescaped += c;
574+
}
575+
_boundary = unescaped;
576+
} else {
577+
// Token form: the value ends at the next ';' (start of the next
578+
// parameter) or at the end of the header field.
579+
int semi = _boundary.indexOf(';');
580+
if (semi >= 0) {
581+
_boundary = _boundary.substring(0, semi);
582+
}
583+
_boundary.trim();
584+
}
585+
586+
// CWE-190 / DoS fix: RFC 2046 §5.1 limits boundary strings to 70
587+
// characters. _boundaryPosition was formerly uint8_t, so a boundary
588+
// of exactly 256 bytes would overflow the counter to 0, preventing the
589+
// BOUNDARY_OR_DATA loop from ever reaching the end of the boundary and
590+
// causing unbounded CPU consumption (watchdog reset on ESP32/ESP8266).
591+
// Reject boundaries outside the valid range before any parsing begins.
592+
if (_boundary.length() == 0 || _boundary.length() > 70) {
593+
async_ws_log_d("Invalid multipart boundary length (%u), aborting", _boundary.length());
594+
_parseState = PARSE_REQ_FAIL;
595+
abort();
596+
return true;
597+
}
598+
479599
_isMultipart = true;
480600
}
481601
} else if (name.equalsIgnoreCase(T_Content_Length) || name.equalsIgnoreCase(T_X_Expected_Entity_Length)) {
@@ -628,6 +748,11 @@ void AsyncWebServerRequest::_parseMultipartPostByte(uint8_t data, bool last) {
628748
_multiParseState = EXPECT_FEED1;
629749
}
630750
} else if (_multiParseState == EXPECT_BOUNDARY) {
751+
// Note: when _parsedLength < 2, the subtractions below (_parsedLength - 2
752+
// and _parsedLength - 3) wrap around to very large size_t values. This is
753+
// intentional: the wrapped values are always greater than _boundary.length()
754+
// (which is at most 70), so those comparisons evaluate to false and the
755+
// first two bytes (the '--' prefix) are consumed without error.
631756
if (_parsedLength < 2 && data != '-') {
632757
_multiParseState = PARSE_ERROR;
633758
return;
@@ -743,8 +868,9 @@ void AsyncWebServerRequest::_parseMultipartPostByte(uint8_t data, bool last) {
743868
itemWriteByte('\n');
744869
itemWriteByte('-');
745870
itemWriteByte('-');
746-
uint8_t i;
747-
for (i = 0; i < _boundaryPosition; i++) {
871+
// CWE-190 fix: loop variable was uint8_t, matching the old type of
872+
// _boundaryPosition. Changed to size_t for consistency.
873+
for (size_t i = 0; i < _boundaryPosition; i++) {
748874
itemWriteByte(_boundary.c_str()[i]);
749875
}
750876
_parseMultipartPostByte(data, last);
@@ -784,8 +910,7 @@ void AsyncWebServerRequest::_parseMultipartPostByte(uint8_t data, bool last) {
784910
itemWriteByte('\n');
785911
itemWriteByte('-');
786912
itemWriteByte('-');
787-
uint8_t i;
788-
for (i = 0; i < _boundary.length(); i++) {
913+
for (size_t i = 0; i < _boundary.length(); i++) {
789914
itemWriteByte(_boundary.c_str()[i]);
790915
}
791916
_parseMultipartPostByte(data, last);
@@ -800,8 +925,7 @@ void AsyncWebServerRequest::_parseMultipartPostByte(uint8_t data, bool last) {
800925
itemWriteByte('\n');
801926
itemWriteByte('-');
802927
itemWriteByte('-');
803-
uint8_t i;
804-
for (i = 0; i < _boundary.length(); i++) {
928+
for (size_t i = 0; i < _boundary.length(); i++) {
805929
itemWriteByte(_boundary.c_str()[i]);
806930
}
807931
itemWriteByte('\r');

src/literals.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,8 @@ static constexpr const char T_username[] = "username";
108108
static constexpr const char T_WS[] = "websocket";
109109
static constexpr const char T_WWW_AUTH[] = "WWW-Authenticate";
110110
static constexpr const char T_X_Expected_Entity_Length[] = "X-Expected-Entity-Length";
111+
static constexpr const char T_BOUNDARY[] = "boundary=";
112+
static constexpr size_t T_BOUNDARY_LEN = sizeof(T_BOUNDARY) - 1;
111113

112114
// HTTP Methods
113115
static constexpr const char T_GET[] = "GET";

0 commit comments

Comments
 (0)