Fix #384: Remove the buffer overflow intended by design allowing users to add a null terminator after the buffer end

Author: mathieucarbou

examples/WebSocket/WebSocket.ino

@@ -76,9 +76,11 @@ void setup() {
   // Run in terminal 2: websocat ws://192.168.4.1/ws => should stream data
   // Run in terminal 3: websocat ws://192.168.4.1/ws => should fail:
   //
-  // To send a message to the WebSocket server:
+  // To send a message to the WebSocket server (\n at the end):
+  // > echo "Hello!" | websocat ws://192.168.4.1/ws
   //
-  // echo "Hello!" | websocat ws://192.168.4.1/ws
+  // Generates 2001 characters (\n at the end) to cause a fragmentation (over TCP MSS):
+  // > openssl rand -hex 1000 | websocat ws://192.168.4.1/ws
   //
   ws.onEvent([](AsyncWebSocket *server, AsyncWebSocketClient *client, AwsEventType type, void *arg, uint8_t *data, size_t len) {
     (void)len;
@@ -108,7 +110,6 @@ void setup() {
       // complete frame
       if (info->final && info->index == 0 && info->len == len) {
         if (info->opcode == WS_TEXT) {
-          data[len] = 0;
           Serial.printf("ws text: %s\n", (char *)data);
           client->ping();
         }
@@ -130,7 +131,6 @@ void setup() {
         );
 
         if (info->message_opcode == WS_TEXT) {
-          data[len] = 0;
           Serial.printf("%s\n", (char *)data);
         } else {
           for (size_t i = 0; i < len; i++) {

idf_component_examples/websocket/main/main.cpp

@@ -55,7 +55,6 @@ void setup() {
       String msg = "";
       if (info->final && info->index == 0 && info->len == len) {
         if (info->opcode == WS_TEXT) {
-          data[len] = 0;
           Serial.printf("ws text: %s\n", (char *)data);
         }
       }

src/AsyncWebSocket.cpp

@@ -586,7 +586,6 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
     }
 
     const size_t datalen = std::min((size_t)(_pinfo.len - _pinfo.index), plen);
-    const auto datalast = datalen ? data[datalen] : 0;
 
     if (_pinfo.masked) {
       for (size_t i = 0; i < datalen; i++) {
@@ -606,7 +605,31 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
       }
 
       if (datalen > 0) {
-        _server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, data, datalen);
+        // ------------------------------------------------------------
+        // Issue 384: https://github.com/ESP32Async/ESPAsyncWebServer/issues/384
+        // Discussion: https://github.com/ESP32Async/ESPAsyncWebServer/pull/383#discussion_r2760425739
+        // The initial design of the library was doing a backup of the byte following the data buffer because the client code
+        // was allowed and documented to do something like data[len] = 0; to facilitate null-terminated string handling.
+        // This was a bit hacky but it was working and it was documented, although completely incorrect because it was modifying a byte outside of the data buffer.
+        // So to fix this behavior and to avoid breaking existing client code that may be relying on this behavior, we now have to copy the data to a temporary buffer that has an extra byte for the null terminator.
+        // ------------------------------------------------------------
+        uint8_t *copy = (uint8_t *)malloc(datalen + 1);
+
+        if (copy == NULL) {
+          async_ws_log_e("Failed to allocate");
+          _status = WS_DISCONNECTED;
+          if (_client) {
+            _client->abort();
+          }
+          return;
+        }
+
+        memcpy(copy, data, datalen);
+        copy[datalen] = 0;
+
+        _server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen);
+
+        free(copy);
       }
 
       // track index for next fragment
@@ -652,12 +675,37 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
 
       } else if (_pinfo.opcode < WS_DISCONNECT) {  // continuation or text/binary frame
         async_ws_log_v("WS[%" PRIu32 "]: processing data frame num=%" PRIu32 "", _clientId, _pinfo.num);
-        _server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, data, datalen);
+
+        // ------------------------------------------------------------
+        // Issue 384: https://github.com/ESP32Async/ESPAsyncWebServer/issues/384
+        // Discussion: https://github.com/ESP32Async/ESPAsyncWebServer/pull/383#discussion_r2760425739
+        // The initial design of the library was doing a backup of the byte following the data buffer because the client code
+        // was allowed and documented to do something like data[len] = 0; to facilitate null-terminated string handling.
+        // This was a bit hacky but it was working and it was documented, although completely incorrect because it was modifying a byte outside of the data buffer.
+        // So to fix this behavior and to avoid breaking existing client code that may be relying on this behavior, we now have to copy the data to a temporary buffer that has an extra byte for the null terminator.
+        // ------------------------------------------------------------
+        uint8_t *copy = (uint8_t *)malloc(datalen + 1);
+
+        if (copy == NULL) {
+          async_ws_log_e("Failed to allocate");
+          _status = WS_DISCONNECTED;
+          if (_client) {
+            _client->abort();
+          }
+          return;
+        }
+
+        memcpy(copy, data, datalen);
+        copy[datalen] = 0;
+
+        _server->_handleEvent(this, WS_EVT_DATA, (void *)&_pinfo, copy, datalen);
         if (_pinfo.final) {
           _pinfo.num = 0;
         } else {
           _pinfo.num += 1;
         }
+
+        free(copy);
       }
 
     } else {
@@ -674,11 +722,6 @@ void AsyncWebSocketClient::_onData(void *pbuf, size_t plen) {
       break;
     }
 
-    // restore byte as _handleEvent may have added a null terminator i.e., data[len] = 0;
-    if (datalen) {
-      data[datalen] = datalast;
-    }
-
     data += datalen;
     plen -= datalen;
   }

src/AsyncWebSocket.h

@@ -554,9 +554,6 @@ class AsyncWebSocketMessageHandler {
       }
     } else if (type == WS_EVT_DATA) {
       AwsFrameInfo *info = (AwsFrameInfo *)arg;
-      if (info->opcode == WS_TEXT) {
-        data[len] = 0;
-      }
       if (info->final && info->index == 0 && info->len == len) {
         if (_onMessage) {
           _onMessage(server, client, data, len);