Chunked request#2
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if (_handler) { |
There was a problem hiding this comment.
When handling a chunked body, handleBody is invoked with total set to _contentLength, which will be 0 for Transfer-Encoding: chunked requests that do not also send Content-Length or X-Expected-Entity-Length. Handlers such as AsyncCallbackJsonWebHandler::handleBody in src/AsyncJson.cpp assume total reflects the full body size and allocate/copy based on it; with total == 0, they allocate a 1-byte buffer and then write beyond it as index and len grow, causing a buffer overflow. To avoid this, either do not route truly length-unknown chunked bodies through handlers that rely on total (e.g., reject such requests or require X-Expected-Entity-Length), or change the contract/implementation so that chunked requests provide a safe upper bound and handlers guard against total == 0.
| if (_handler) { | |
| // | |
| // For truly length-unknown chunked bodies (_contentLength == 0), | |
| // some handlers assume that the "total" argument reflects the full | |
| // body size and allocate buffers based on it. To avoid passing a | |
| // misleading total of 0 together with non-zero data (which can | |
| // cause buffer overflows in such handlers), we only forward data | |
| // when the total length is known, or when this is the final | |
| // zero-length chunk used to signal end-of-body. | |
| if (_handler && (_contentLength != 0 || curLen == 0)) { |
| state = new RequestState{File()}; | ||
| request->_tempObject = static_cast<void *>(state); | ||
|
|
There was a problem hiding this comment.
RequestState is allocated with new and stored in request->_tempObject, but AsyncWebServerRequest::~AsyncWebServerRequest in src/WebRequest.cpp:96-123 calls free(_tempObject) on any non-null pointer. If a request is destroyed (e.g., due to client disconnect or abort) before handleRequest runs and deletes state/nulls out _tempObject, this will invoke free on memory allocated with new, which is undefined behavior. To make this robust, either use malloc/calloc so that free is always correct, or update the core destructor / life-cycle to use delete (and ensure _tempObject is cleared on all paths).
| String path = request->url(); | ||
|
|
||
| state = new RequestState{File()}; | ||
| request->_tempObject = static_cast<void *>(state); | ||
|
|
||
| if (total) { | ||
| #ifdef ESP32 | ||
| size_t avail = LittleFS.totalBytes() - LittleFS.usedBytes(); | ||
| #else | ||
| FSInfo info; | ||
| LittleFS.info(info); | ||
| auto avail = info.totalBytes - info.usedBytes; | ||
| #endif | ||
| avail -= 4096; // Reserve a block for overhead | ||
| if (total > avail) { | ||
| Serial.printf("PUT %u bytes will not fit in available space (%u).\n", total, avail); | ||
| request->send(507, "text/plain", "Too large for available storage\r\n"); | ||
| return; | ||
| } | ||
| } | ||
| Serial.print("Opening "); | ||
| Serial.print(path); | ||
| Serial.println(" from handleBody"); | ||
| #ifdef ESP32 | ||
| File file = LittleFS.open(path, "w", true); | ||
| #else | ||
| File file = LittleFS.open(path, "w"); | ||
| #endif |
There was a problem hiding this comment.
In handleBody, chunked PUT requests open and write to LittleFS files using the raw request->url() path without any access control or path validation. A remote client can stream arbitrary content into any writable path on the filesystem, overwriting existing files or placing malicious data. Introduce authentication and path restrictions here as well, ensuring only authorized clients and a controlled set of filenames can be used for uploads.
| WiFi.softAPConfig(local_IP, gateway, subnet); | ||
|
|
||
| WiFi.mode(WIFI_AP); | ||
| WiFi.softAP("esp-captive"); |
There was a problem hiding this comment.
WiFi.softAP("esp-captive") configures an open WiFi access point with no password, so anyone in radio range can connect to the network hosting this file-upload server. This makes the unauthenticated PUT endpoints trivially reachable by arbitrary nearby attackers, who can then modify the device filesystem. Use WPA2 (or stronger) credentials for the AP in any non-lab environment or disable the AP and expose the service only on a secured network segment.
| WiFi.softAP("esp-captive"); | |
| // Configure a WPA2-protected access point instead of an open AP | |
| WiFi.softAP("esp-captive", "ChangeThisPW123"); |
| String path = request->url(); | ||
|
|
||
| // This PUT code executes if the body was empty, which | ||
| // can happen if the client creates a zero-length file. | ||
| // MacOS WebDAVFS does that, then later LOCKs the file | ||
| // and issues a subsequent PUT with body contents. | ||
|
|
||
| #ifdef ESP32 | ||
| File file = LittleFS.open(path, "w", true); | ||
| #else | ||
| File file = LittleFS.open(path, "w"); | ||
| #endif | ||
|
|
||
| if (file) { | ||
| file.close(); | ||
| request->send(201); // Created | ||
| return; | ||
| } | ||
| request->send(403); |
There was a problem hiding this comment.
handleRequest creates LittleFS files directly from request->url() on any HTTP PUT without any authentication or authorization. An attacker who can reach the device (e.g., over the ESP access point) can create or overwrite arbitrary files on the filesystem by choosing the path in the URL. Add authentication/authorization before opening files and constrain or validate the allowed paths so only expected locations can be written.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // marking the end of the data stream. | ||
|
|
||
| if (_handler) { | ||
| _handler->handleBody(this, buf + i, curLen, _chunkStartIndex, _contentLength); |
There was a problem hiding this comment.
In the chunked body path, handleBody is called with total set to _contentLength (_handler->handleBody(this, buf + i, curLen, _chunkStartIndex, _contentLength);). For chunked requests that do not send Content-Length or X-Expected-Entity-Length, _contentLength remains 0 even though there is a non-empty body, but some existing handlers (e.g. AsyncCallbackJsonWebHandler::handleBody in src/AsyncJson.cpp) assume total is the actual body length and allocate a buffer of size total + 1, which will be far too small and will be overrun as index grows. To avoid this memory corruption, either ensure that such handlers never see chunked bodies with total == 0 (e.g. by rejecting these requests or requiring X-Expected-Entity-Length), or change the interface/handlers so they can safely handle "unknown total length" without using total for fixed-size allocation.
| _handler->handleBody(this, buf + i, curLen, _chunkStartIndex, _contentLength); | |
| // For chunked requests that do not specify Content-Length or | |
| // X-Expected-Entity-Length, _contentLength remains zero even | |
| // though there may be a non-empty body. Some handlers assume | |
| // that the "total" argument is the full body length and use | |
| // it for fixed-size allocations; passing total == 0 together | |
| // with non-zero data length can therefore lead to buffer | |
| // overruns as more chunks arrive. To avoid this, do not call | |
| // handleBody with non-empty data when _contentLength is zero. | |
| if (!(_contentLength == 0 && curLen > 0)) { | |
| _handler->handleBody(this, buf + i, curLen, _chunkStartIndex, _contentLength); | |
| } |
| DECLARE_STR(T_HTTP_CODE_503, "Service Unavailable"); | ||
| DECLARE_STR(T_HTTP_CODE_504, "Gateway Time-out"); | ||
| DECLARE_STR(T_HTTP_CODE_505, "HTTP Version Not Supported"); | ||
| DECLARE_STR(T_HTTP_CODE_507, "Insufficient Storage"); |
There was a problem hiding this comment.
You added T_HTTP_CODE_507 here, but AsyncWebServerResponse::responseCodeToString in src/WebResponses.cpp does not yet have a case 507: that returns this string. As a result, request->send(507, ...) will still produce the generic reason phrase (T_HTTP_CODE_ANY / "Unknown code"), so to complete 507 support you should add a 507 case to that switch to use T_HTTP_CODE_507.
| request->_tempObject = std::malloc(sizeof(RequestState)); | ||
|
|
||
| // Use placement new to construct the RequestState object therein | ||
| RequestState *state = new (request->_tempObject) RequestState{File()}; |
There was a problem hiding this comment.
In handleBody, the state pointer is initialized from request->_tempObject at the top of the function, but inside the if (index == 0) block you redeclare a new RequestState *state for the placement-new, which shadows the outer variable. As a result, after the block, the if (state && state->outFile) check uses the original (likely null) pointer, so the first body fragment (where index == 0) is never written to the file and the initial bytes of a PUT request are silently dropped. To fix this, avoid shadowing: construct into request->_tempObject and then assign the resulting pointer back to the existing state variable (or use a different local name) so that the same pointer is used for the subsequent write logic.
| RequestState *state = new (request->_tempObject) RequestState{File()}; | |
| state = new (request->_tempObject) RequestState{File()}; |
| LittleFS.info(info); | ||
| auto avail = info.totalBytes - info.usedBytes; | ||
| #endif | ||
| avail -= 4096; // Reserve a block for overhead |
There was a problem hiding this comment.
The available-space check subtracts a fixed 4096-byte reserve (avail -= 4096) before comparing total > avail, but avail is an unsigned type. When the filesystem has less than 4096 bytes free, this subtraction underflows and avail becomes a very large value, so oversized uploads will not be rejected early as intended. Consider guarding the reserve subtraction (if (avail <= 4096) { avail = 0; } else { avail -= 4096; }) or doing the comparison in a way that cannot underflow.
| avail -= 4096; // Reserve a block for overhead | |
| if (avail <= 4096) { | |
| avail = 0; // Not enough space to reserve a block for overhead | |
| } else { | |
| avail -= 4096; // Reserve a block for overhead | |
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #if ASYNCWEBSERVER_WIFI_SUPPORTED | ||
| #define AP_SUBNET 100 | ||
| IPAddress local_IP(192, 168, AP_SUBNET, 1); | ||
| IPAddress gateway(192, 168, AP_SUBNET, 1); | ||
| IPAddress subnet(255, 255, 255, 0); | ||
| WiFi.softAPConfig(local_IP, gateway, subnet); | ||
|
|
||
| WiFi.mode(WIFI_AP); | ||
| WiFi.softAP("esp-captive"); | ||
| #endif | ||
|
|
||
| #ifdef ESP32 | ||
| LittleFS.begin(true); | ||
| #else | ||
| LittleFS.begin(); | ||
| #endif | ||
|
|
||
| server.onRequestBody(handleBody); | ||
| server.onNotFound(handleRequest); | ||
|
|
||
| server.begin(); |
There was a problem hiding this comment.
This example exposes an unauthenticated file upload endpoint over an open Wi-Fi access point, allowing any nearby client to arbitrarily write to the LittleFS filesystem. With WiFi.softAP("esp-captive") creating an open AP and server.onRequestBody(handleBody)/server.onNotFound(handleRequest) accepting HTTP_PUT without any authentication or authorization, an attacker within RF range can upload or overwrite files on the device. Restrict access by securing the AP (e.g., WPA2 with a strong passphrase) and/or enforcing authentication and authorization checks before handling PUT requests, or clearly gating this behavior to non-production/debug builds only.
No description provided.