connectd: treat websocket handshake headers as case-insensitive#9308
connectd: treat websocket handshake headers as case-insensitive#9308coreyphillips wants to merge 1 commit into
Conversation
880909c to
4438a15
Compare
| "sec-websocket-version: 13\r\n\r\n"; | ||
|
|
||
| my_rbuf = tal_strdup(tmpctx, hdr); | ||
| my_rbuf_off = 0; |
ff22cee to
5710e47
Compare
| static bool contains_ci(const char *haystack, const char *needle) | ||
| { | ||
| size_t n = strlen(needle); | ||
|
|
||
| for (const char *p = haystack; *p; p++) { | ||
| if (strncasecmp(p, needle, n) == 0) | ||
| return true; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
You can use strcasestr as far as I can see, as pointed out in man 3 strstr :-)
It may also be more efficient as I think your example can be quadratic.
There was a problem hiding this comment.
Good call, thanks! I replaced the custom contains_ci() helper with strcasestr() and amended the commit.
| if (memstarts(buf, hdrlen, hdrname, strlen(hdrname)) | ||
| /* header name (ASCII case-insensitive, RFC 9110 #5.1) | ||
| * followed by : */ | ||
| if (hdrlen > strlen(hdrname) |
There was a problem hiding this comment.
There is also memstarts_str which retains the length limit check on the buf, but loses the strlen on the headername. This is likely more robust than searching for the next \0 in the buf, which may overflow if I read this correctly (well the reader creating buf needs to take care of null-terminating the string.
There was a problem hiding this comment.
Thanks! memstarts_str(a, al, s) expands to memstarts(a, al, s, strlen(s)), which bottoms out in a memcmp, a case-sensitive byte comparison, so from what I understand, using it here would lose the case-insensitive header-name matching this patch is fixing. I didn't find a case-insensitive counterpart in ccan (no memstarts_casestr or similar), so strncasecmp with an explicit length guard seemed like the closest safe equivalent.
On the overflow concern: strncasecmp never scans buf for a \0; it reads at most hdrnamelen bytes. strlen is only ever called on hdrname, which is a NUL-terminated literal. The hdrlen > hdrnamelen check guarantees the compared bytes and the following buf[hdrnamelen] == ':' check are within the current header line, so this doesn't rely on buf being NUL-terminated.
I did incorporate the two properties memstarts_str would have given us: the length check explicitly bounds access to buf, and strlen(hdrname) is hoisted out so it's computed once.
5710e47 to
a523a9d
Compare
HTTP header field names are case-insensitive (RFC 9110 section 5.1) and RFC 6455 says the Upgrade and Connection values are treated as ASCII case-insensitive, as the comment in websocketd.c already quotes. The handshake parser matched header names byte-exactly and the values with case-sensitive strstr, so any client that normalizes header casing got 400: notably Node.js's built-in WebSocket (undici) lowercases all request headers and can never connect to a bind-addr=ws: listener, while browsers happen to send RFC casing and work. Match header names case-insensitively in get_http_hdr and use a case-insensitive search for the Upgrade and Connection values. The Sec-WebSocket-Version value comparison stays exact and the key is base64 data, unchanged. Extends the unit test with a lowercased handshake as sent by Node. Fixes ElementsProject#9307 Changelog-Fixed: connectd: the WebSocket port now accepts handshakes from clients that send lowercase HTTP header names, such as Node.js's built-in WebSocket. Signed-off-by: Corey Phillips <corey.lyle.phillips@proton.me>
a523a9d to
3e63246
Compare
3 participants
Fixes #9307.
The websocket handshake parser matches header names byte-exactly (get_http_hdr) and the Upgrade/Connection values with case-sensitive strstr. Header field names are case-insensitive per RFC 9110 section 5.1, and RFC 6455 says the Upgrade and Connection values are "treated as an ASCII case-insensitive value" (already quoted in the comment above http_respond).
Practical impact: Node.js's built-in WebSocket (undici) lowercases all request headers, so it can never connect to a bind-addr=ws: listener and gets "400 I only speak websocket". Browsers happen to send RFC casing and work.
Changes:
Verified the failure mode against a live v26.06.1 bind-addr=ws: listener with raw replays (lowercased names rejected before, accepted with this change per the unit test vector).