esp_http_server: close WebSocket sessions on parse failures (IDFGH-17706)

components/esp_http_server/include/esp_http_server.h

@@ -1777,6 +1777,7 @@ typedef void (*transfer_complete_cb)(esp_err_t err, int socket, void *arg);
  * @note    Calling httpd_ws_recv_frame() with max_len as 0 will give actual frame size in pkt->len.
  *          The user can dynamically allocate space for pkt->payload as per this length and call httpd_ws_recv_frame() again to get the actual data.
  *          Please refer to the corresponding example for usage.
+ *          Socket or WebSocket framing errors mark the session for closure.
  *
  * @param[in]   req         Current request
  * @param[out]  pkt         WebSocket packet
@@ -1796,6 +1797,7 @@ esp_err_t httpd_ws_recv_frame(httpd_req_t *req, httpd_ws_frame_t *pkt, size_t ma
  * @note    Calling httpd_ws_recv_frame_part() with max_len as 0 will give actual frame size in pkt->len.
  *          The user can dynamically allocate space for pkt->payload or user defined chunk size and call httpd_ws_recv_frame_part() again to get the actual data.
  *          In contrast to httpd_ws_recv_frame, this method is able to read frame payload partially. The amount of data that is yet to be received is stored in pkt->left_len
+ *          Socket or WebSocket framing errors mark the session for closure.
  *
  * @param[in]   req         Current request
  * @param[out]  pkt         WebSocket packet

components/esp_http_server/src/httpd_ws.c

@@ -266,6 +266,17 @@ static esp_err_t httpd_ws_unmask_payload(uint8_t *payload, size_t len, const uin
     return ESP_OK;
 }
 
+static esp_err_t httpd_ws_fail_and_mark_close(httpd_req_t *req, esp_err_t err)
+{
+    struct httpd_req_aux *aux = req->aux;
+
+    if (aux != NULL && aux->sd != NULL) {
+        aux->sd->ws_close = true;
+    }
+
+    return err;
+}
+
 static esp_err_t httpd_ws_recv_frame_internal(httpd_req_t *req, httpd_ws_frame_t *frame, size_t max_len, bool partial)
 {
     esp_err_t ret = httpd_ws_check_req(req);
@@ -294,7 +305,7 @@ static esp_err_t httpd_ws_recv_frame_internal(httpd_req_t *req, httpd_ws_frame_t
         int recv_ret = httpd_recv_with_opt(req, (char *)&second_byte, sizeof(second_byte), HTTPD_RECV_OPT_BLOCKING);
         if (recv_ret != (int)sizeof(second_byte)) {
             ESP_LOGW(TAG, LOG_FMT("Failed to receive the second byte"));
-            return ESP_FAIL;
+            return httpd_ws_fail_and_mark_close(req, ESP_FAIL);
         }
 
         /* Parse the second byte */
@@ -312,7 +323,7 @@ static esp_err_t httpd_ws_recv_frame_internal(httpd_req_t *req, httpd_ws_frame_t
             recv_ret = httpd_recv_with_opt(req, (char *)length_bytes, sizeof(length_bytes), HTTPD_RECV_OPT_BLOCKING);
             if (recv_ret != (int)sizeof(length_bytes)) {
                 ESP_LOGW(TAG, LOG_FMT("Failed to receive 2 bytes length"));
-                return ESP_FAIL;
+                return httpd_ws_fail_and_mark_close(req, ESP_FAIL);
             }
 
             frame->len = ((uint32_t)(length_bytes[0] << 8U) | (length_bytes[1]));
@@ -322,7 +333,7 @@ static esp_err_t httpd_ws_recv_frame_internal(httpd_req_t *req, httpd_ws_frame_t
             recv_ret = httpd_recv_with_opt(req, (char *)length_bytes, sizeof(length_bytes), HTTPD_RECV_OPT_BLOCKING);
             if (recv_ret != (int)sizeof(length_bytes)) {
                 ESP_LOGW(TAG, LOG_FMT("Failed to receive 8 bytes length"));
-                return ESP_FAIL;
+                return httpd_ws_fail_and_mark_close(req, ESP_FAIL);
             }
 
             frame->len = (((uint64_t)length_bytes[0] << 56U) |
@@ -341,13 +352,13 @@ static esp_err_t httpd_ws_recv_frame_internal(httpd_req_t *req, httpd_ws_frame_t
             recv_ret = httpd_recv_with_opt(req, (char *)aux->mask_key, sizeof(aux->mask_key), HTTPD_RECV_OPT_BLOCKING);
             if (recv_ret != (int)sizeof(aux->mask_key)) {
                 ESP_LOGW(TAG, LOG_FMT("Failed to receive mask key"));
-                return ESP_FAIL;
+                return httpd_ws_fail_and_mark_close(req, ESP_FAIL);
             }
         } else {
             /* If the WS frame from client to server is not masked, it should be rejected.
              * Please refer to RFC6455 Section 5.2 for more details. */
             ESP_LOGW(TAG, LOG_FMT("WS frame is not properly masked."));
-            return ESP_ERR_INVALID_STATE;
+            return httpd_ws_fail_and_mark_close(req, ESP_ERR_INVALID_STATE);
         }
     }
     /* If max_len is 0, regard it OK for userspace to get frame len */
@@ -382,7 +393,7 @@ static esp_err_t httpd_ws_recv_frame_internal(httpd_req_t *req, httpd_ws_frame_t
         int read_len = httpd_recv_with_opt(req, (char *)frame->payload + offset, left_len, HTTPD_RECV_OPT_NONE);
         if (read_len <= 0) {
             ESP_LOGW(TAG, LOG_FMT("Failed to receive payload"));
-            return ESP_FAIL;
+            return httpd_ws_fail_and_mark_close(req, ESP_FAIL);
         }
         offset += read_len;
         left_len -= read_len;

components/esp_http_server/test_apps/main/test_http_server.c

@@ -6,6 +6,7 @@
 
 #include <stdlib.h>
 #include <stdbool.h>
+#include <string.h>
 #include <esp_system.h>
 #include <esp_http_server.h>
 #include <esp_heap_caps.h>
@@ -55,6 +56,8 @@ static httpd_uri_t handler_limit_ws_uri(char *path, const char *subprotocol)
 }
 
 static int ws_recv_fail_handler_calls;
+static size_t ws_unmasked_frame_offset;
+static esp_err_t ws_recv_frame_result;
 
 static int ws_recv_fail_override(httpd_handle_t hd, int sockfd, char *buf, size_t buf_len, int flags)
 {
@@ -66,12 +69,41 @@ static int ws_recv_fail_override(httpd_handle_t hd, int sockfd, char *buf, size_
     return HTTPD_SOCK_ERR_FAIL;
 }
 
+static int ws_unmasked_frame_recv_override(httpd_handle_t hd, int sockfd, char *buf, size_t buf_len, int flags)
+{
+    static const uint8_t unmasked_text_frame[] = {
+        0x81, /* FIN, text frame */
+        0x00, /* zero-length payload, no mask bit */
+    };
+
+    (void)hd;
+    (void)sockfd;
+    (void)flags;
+
+    if (ws_unmasked_frame_offset >= sizeof(unmasked_text_frame)) {
+        return HTTPD_SOCK_ERR_FAIL;
+    }
+
+    size_t read_len = MIN(buf_len, sizeof(unmasked_text_frame) - ws_unmasked_frame_offset);
+    memcpy(buf, unmasked_text_frame + ws_unmasked_frame_offset, read_len);
+    ws_unmasked_frame_offset += read_len;
+    return read_len;
+}
+
 static esp_err_t ws_counting_handler(httpd_req_t *req)
 {
     (void)req;
     ws_recv_fail_handler_calls++;
     return ESP_OK;
 }
+
+static esp_err_t ws_unmasked_frame_handler(httpd_req_t *req)
+{
+    httpd_ws_frame_t frame = {0};
+
+    ws_recv_frame_result = httpd_ws_recv_frame(req, &frame, 0);
+    return ESP_OK;
+}
 #endif /* CONFIG_HTTPD_WS_SUPPORT */
 
 static inline unsigned num_digits(unsigned x)
@@ -455,6 +487,36 @@ TEST_CASE("WS recv failure marks close without dispatching handler", "[HTTP SERV
 
     free(hd.hd_req_aux.resp_hdrs);
 }
+
+TEST_CASE("WS frame parse failure marks session for close", "[HTTP SERVER][websocket]")
+{
+    httpd_config_t config = HTTPD_DEFAULT_CONFIG();
+    struct httpd_data hd = {0};
+    struct sock_db session = {0};
+
+    hd.config = config;
+    hd.hd_req_aux.resp_hdrs = calloc(config.max_resp_headers, sizeof(*hd.hd_req_aux.resp_hdrs));
+    TEST_ASSERT_NOT_NULL(hd.hd_req_aux.resp_hdrs);
+
+    session.fd = 123;
+    session.handle = (httpd_handle_t) &hd;
+    session.recv_fn = ws_unmasked_frame_recv_override;
+    session.ws_handshake_done = true;
+    session.ws_handler = ws_unmasked_frame_handler;
+    session.ws_control_frames = false;
+    session.ws_close = false;
+
+    ws_unmasked_frame_offset = 0;
+    ws_recv_frame_result = ESP_OK;
+
+    esp_err_t ret = httpd_req_new(&hd, &session);
+
+    TEST_ASSERT_EQUAL(ESP_OK, ret);
+    TEST_ASSERT_EQUAL(ESP_ERR_INVALID_STATE, ws_recv_frame_result);
+    TEST_ASSERT_TRUE(session.ws_close);
+
+    free(hd.hd_req_aux.resp_hdrs);
+}
 #endif /* CONFIG_HTTPD_WS_SUPPORT */
 
 void app_main(void)