WebSocket: bound every script-supplied value that an error message quotes

Closed
RO
1mo ago
+152-7

Problem

  • new WebSocket(url, protocols) aborts the process instead of throwing when an argument is large: panic(main thread): abort() called, exit 134, peak RSS 4.68 GB. Reproduced on main at 4ff91937 with new WebSocket("ws://localhost/", ["\u00ff".repeat(357913942)]).
  • The constructor quotes the value it rejects back in the error message. Nothing bounded the quote, so the message grew past String::MaxLength (2^31-1), and both makeString() and StringBuilder call CRASH() on overflow (StringBuilder.cpp:46). Five sites echoed a script-supplied value: the invalid subprotocol and the duplicate subprotocol through encodeProtocolString() (WebSocket.cpp:144), the invalid proxy URL and the unsupported proxy protocol (WebSocket.cpp:253, :259), and the invalid binaryType (WebSocket.cpp:1230).
  • joinStrings() (WebSocket.cpp:175) builds the Sec-WebSocket-Protocol header value. Two 2^30-character protocols sum past String::MaxLength and abort the same way.

Fix

  • Add quoteForErrorMessage(). It keeps 1024 characters and appends an ellipsis. 1024 is the bound that URL::stringCenterEllipsizedToLength() already applies to the URLs in this file. encodeProtocolString() bounds its output, because the escape expands one code unit to six characters.
  • joinStrings() measures the result first and returns a null String when it does not fit in one string. connect() then throws RangeError: Out of memory, the code Base64Helpers.cpp:24 already uses for a value that cannot be represented. The header value is sent on the wire, so it cannot be truncated. The builder also records overflow now, so an allocation failure throws instead of aborting.
  • The limit is the lower of String::MaxLength and Bun__stringSyntheticAllocationLimit, so the test reaches it through setSyntheticAllocationLimitForTesting instead of allocating 2 GB.
  • Verified: test/js/web/websocket/websocket-subprotocol-strict.test.ts (6 new blocks, stock bun fails all 6). Also all of test/js/web/websocket/ and test/js/first_party/ws/ (384 pass, 3 pre-existing failures that need wss://ws.postman-echo.com).

Background

  • StringBuilder crashes on overflow by default (m_shouldCrashOnOverflow { true }). OverflowPolicy::RecordOverflow makes it set a flag that hasOverflowed() reports instead. makeString() has no such mode. It calls CRASH() when tryMakeString() returns a null String.
  • String::MaxLength is INT32_MAX. A string of that length is 2 GB as Latin-1. One character more is the abort.
  • Bun__stringSyntheticAllocationLimit is the process-wide character cap that Bun's string constructors check (src/jsc/bindings/helpers.h). bun:internal-for-testing can lower it, which is how a test reaches a limit that needs gigabytes in production.
  • The WebSocket spec does not define the text of these errors. It defines only the exception type. Node does not echo the subprotocol at all.
Notes

Boundary, measured on main at 4ff91937 with the released binary:

inputresult
"\u00ff".repeat(357913936)SyntaxError, message length 2147483647 (exactly String::MaxLength)
"\u00ff".repeat(357913942)abort, exit 134, peak RSS 4.68 GB
two protocols of 2^30 charactersabort, exit 134, peak RSS 3.25 GB

With this change, both reproductions throw on the ASAN debug build: a SyntaxError with a 1060-character message, and RangeError: Out of memory. The truncation boundary is exact: 1024 escaped characters produce no ellipsis, 1025 produce one.

Sibling sites, checked and deliberately excluded:

  • makeString(url.user(), ':', url.password()) plus base64EncodeToString() at WebSocket.cpp:269 and :662. Base64 grows the input by 4/3, so roughly 1.6 GB of URL userinfo overflows makeString() inside base64EncodeToString(). WebKit has base64EncodeToStringReturnNullIfOverflow() for this. The fix belongs in a separate change: these build the Authorization header rather than an error message, the error path has to move above makePendingActivity() to avoid a pending activity that never settles, and no test can reach it, since the synthetic allocation limit does not apply to makeString().
  • HTTPHeaderMap::get() at src/jsc/bindings/webcore/HTTPHeaderMap.cpp:224 joins the Set-Cookie values with the same shape. It returns String and has no exception channel, so the guard has to go at the FetchHeaders boundary that can throw. That is a separate change.
  • WTF::URLParser::URLParser aborts in makeString(' ', inputString) (URLParser.cpp:1572) for any input of exactly String::MaxLength characters. That code is inside #if ASSERT_ENABLED, so it affects debug and ASAN builds only, it is in vendor/WebKit, and it fires for every WTF::URL built from a maximum-length string, not only for WebSocket.

Overlap with an open PR: #41698 also edits WebSocket.cpp:253 (Invalid proxy URL). It wraps the value in urlForErrorMessage(proxyUrl) to mask a password. This change calls url.stringCenterEllipsizedToLength() on the same line. Whichever lands second should use the URL overload, urlForErrorMessage(url), which masks and then ellipsizes.

The duplicate subprotocol message read WebSocket protocols contain duplicates: with no space and no opening quote, while the closing quote was there. Upstream WebCore has duplicates: '. Restored, rather than pinned in a new test.

Self-reviewed: 4 concerns raised, 3 addressed (the sibling quote sites in the same path, the synthetic allocation limit test hook for the join, and the duplicate message text). The fourth asked to fold in HTTPHeaderMap.cpp:224. Excluded above, with the reason.

[human-review] gate passed · iteration 0 · 2 files touched

fails on main (without fix) 
ASAN without fix: 6 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/web/websocket/websocket-subprotocol-strict.test.ts
bun test v1.4.3 (5f554969b)

test/js/web/websocket/websocket-subprotocol-strict.test.ts:
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols [328.85ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols with spaces [87.84ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols (3 protocols) [39.06ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject duplicate Sec-WebSocket-Protocol headers (same value) [35.93ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject duplicate Sec-WebSocket-Protocol headers (different values) [35.86ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject three Sec-WebSocket-Protocol headers [36.29ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject empty Sec-WebSocket-Protocol header [31.94ms]
(pass) WebSocket strict R
... (truncated)

release without fix: 6 FAILED
bun test v1.4.3-canary.1 (5f554969b)

test/js/web/websocket/websocket-subprotocol-strict.test.ts:
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols [5.42ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols with spaces [1.79ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols (3 protocols) [0.77ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject duplicate Sec-WebSocket-Protocol headers (same value) [0.76ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject duplicate Sec-WebSocket-Protocol headers (different values) [0.74ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject three Sec-WebSocket-Protocol headers [0.68ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject empty Sec-WebSocket-Protocol header [0.62ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject Sec-WebSocket-Protocol with only comma [0.58ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject Sec-WebSocket-Protocol with
... (truncated)
passes on PR (with fix) 
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/web/websocket/websocket-subprotocol-strict.test.ts
bun test v1.4.3 (5f554969b)

test/js/web/websocket/websocket-subprotocol-strict.test.ts:
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols [315.79ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols with spaces [84.68ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject multiple comma-separated protocols (3 protocols) [36.58ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject duplicate Sec-WebSocket-Protocol headers (same value) [35.83ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject duplicate Sec-WebSocket-Protocol headers (different values) [33.97ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject three Sec-WebSocket-Protocol headers [34.16ms]
(pass) WebSocket strict RFC 6455 subprotocol handling > should reject empty Sec-WebSocket-Protocol header [30.22ms]
(pass) WebSocket strict R
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     95a21fe9d9
  features     baseline

23 deps, 131 codegen, 1172 objects in 606ms

ninja: Entering directory `/workspace/bun/build/release'
[1/1244] gen ErrorCode+*.h
[2/1244] install /workspace/bun
bun install v1.4.3-canary.1 (5f554969b)

Checked 22 installs across 61 packages (no changes) [14.00ms]
[3/1244] fetch tinycc
[tinycc] up to date
[4/1243] gen bindgenv2
[5/1243] install /workspace/bun/packages/bun-error
bun install v1.4.3-canary.1 (5f554969b)

Checked 1 install across 2 packages (no changes) [13.00ms]
[6/1243] fetch libjpeg-turbo
[libjpeg-turbo] up to date
[7/1216] gen bake.{client,server,error}.js
-> bake.client.js, bake.server.js, bake.error.js
[8/1216] gen .bind.ts → GeneratedBindings.cpp
[9/1216] install /workspace/bun/src/node-fallbacks
bun install v1.4.3-canary.1 (5f554969b)

Checked 111 installs across 104 packages (no changes) [12.00ms]
[10/1216] gen ProcessBindingConstants.lut.h
Generating /workspace/bun/build/release/codegen/ProcessBindingConstants.lut.h from /works
... (truncated)
diff hotspot 
src/jsc/bindings/webcore/WebSocket.cpp             |  52 ++++++++--
 .../websocket/websocket-subprotocol-strict.test.ts | 107 ++++++++++++++++++++-
 2 files changed, 152 insertions(+), 7 deletions(-)

gate history · 1 passed · 0 rejected · iteration 0

evidence per changed file 
file                                                      reads  edits  tests
src/jsc/bindings/webcore/WebSocket.cpp                        3      9     12
…t/js/web/websocket/websocket-subprotocol-strict.test.ts      2      5     12