Skip to content

fix(ws): guard _onData against a torn 2-byte frame header (OOB read + size_t underflow) - #481

Open
macdylan wants to merge 1 commit into
ESP32Async:mainfrom
macdylan:fix/ws-torn-header-guard
Open

macdylan wants to merge 1 commit into
ESP32Async:mainfrom
macdylan:fix/ws-torn-header-guard

Conversation

@macdylan

Copy link
Copy Markdown

Summary

When a single TCP delivery ends on exactly one byte of the next frame's 2-byte header, the STATE_FRAME_START branch of AsyncWebSocketClient::_onData():

  1. reads fdata[1] out of bounds (only 1 byte is valid), and
  2. computes data += 2; plen -= 2; on plen == 1, underflowing size_t to SIZE_MAX,

after which the parser walks wild memory until the device panics.

The v3.12 websocket refactor (#462) added careful torn-frame handling for the mask bytes (STATE_FRAME_MASK) and the payload, but the base 2-byte header consume is still only guarded by while (plen > 0). This PR adds the missing guard.

Reproduction

Naturally occurs under retransmission/load (a segment happens to split at a frame boundary). Deterministic trigger with a raw-socket client that writes one header byte, flushes, waits, then writes the rest:

const f = frame('t');            // any small masked text frame
sock.write(f.subarray(0, 1));    // ONE header byte in its own delivery
await sleep(60);
sock.write(f.subarray(1));       // remainder arrives in the next delivery

Evidence

Version Result of the injection
v3.6.0 Load access fault panic at AsyncWebSocket.cpp:538, 3/3 runs under load
current main (v3.12.1) Guru Meditation Error: Core 0 panic'ed (Store access fault) -> reboot; occasionally the accept path stalls for seconds afterwards

Tested on ESP32-C3 (arduino-esp32 3.2.1 / IDF 5.4, AsyncTCP 3.5.0); plain WS echo scenario, no TLS.

The fix

if (_pstate == STATE_FRAME_START) {
  if (plen < 2) break;  // torn base header: the delivery ends inside the 2 header bytes

  const uint8_t *fdata = data;

Dropping the un-parseable tail makes the connection desynchronize and get closed by the client - the same trade-off the adjacent plen >= 2 / plen >= 8 / plen >= 4 extended-length and mask guards already make.

A larger alternative would accumulate the base header across deliveries with a new STATE_FRAME_HEADER state, mirroring the STATE_FRAME_MASK machinery - happy to rework in that direction if you prefer it. The minimal guard is offered first because it is trivially reviewable and the desync outcome is already how the other partial-header paths behave.

Testing

  • With the guard: 8/8 injection rounds clean, device stays alive, the desynced connection is closed and reaped normally.
  • Without the guard: panics as above, reproducibly.

When a single TCP delivery ends on exactly one byte of the next frame's
header, the STATE_FRAME_START branch reads fdata[1] out of bounds and
computes plen -= 2 on plen == 1, underflowing size_t to SIZE_MAX; the
parser then walks wild memory until the device panics.

The v3.12 websocket refactor (ESP32Async#462) added careful torn-frame handling
for the mask bytes and the payload, but the base 2-byte header consume
is still only guarded by 'while (plen > 0)'.

Drop the un-parseable tail instead of crashing: the connection
desynchronizes and is closed by the client, the same trade-off the
adjacent extended-length guards already make.
@willmmiles

Copy link
Copy Markdown

I believe #473 fixes this properly, with correct handling of frames split between multiple packets, instead of dropping the connection.

Can you please test with #473 and see if it resolves this issue in your test case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants