Reject invalid Sec-WebSocket-Key values#118
Conversation
get_handshake_accept() always hashed a fixed WS_KEYMS_LEN (60) bytes of the key+magic-string buffer, regardless of the actual length of the client-supplied Sec-WebSocket-Key. This only produced the correct SHA-1 input for keys that happen to be exactly 24 characters (the common/base64-of-16-bytes case); for any other key length the hash covered stale/zero bytes beyond the real key+magic-string content, so the server computed and returned a Sec-WebSocket-Accept value that does not match RFC 6455 4.1, even though the connection was otherwise accepted. Fix by hashing strlen(str) instead of the fixed WS_KEYMS_LEN. Since str is calloc-zeroed and built via strncpy(str, wsKey, WS_KEY_LEN) followed by strcat(str, MAGIC_STRING), strlen(str) always equals the true (possibly truncated) key length plus the magic-string length, and is bounded by the buffer's allocated size (WS_KEY_LEN + WS_MS_LEN). Verified with a standalone harness against src/handshake.c: the standard 24-char RFC6455 example key still yields the well-known correct Accept value, and a 7-char test key now yields the independently-computed correct value (previously wrong).
|
Hi @94xhn, Unfortunately, per RFC 6455 (section 4.2.1), the
A canonical base64 encoding of 16 bytes is always 24 characters long. Therefore, accepting a key with a different encoded length would not be compliant with the specs. What wsServer could do instead, is to reject a connection attempt if provided an invalid key. |
94xhn
changed the title
|
Thanks for the correction. I reworked the change to reject keys that are not the required 24-character padded Base64 form or do not decode to exactly 16 bytes. The previous variable-length SHA-1 change has been removed, so valid keys use WS_KEYMS_LEN again. I verified the RFC example still produces the expected accept value, while short, overlong, invalid-alphabet, bad-padding, embedded-padding, and 18-byte keys are rejected. The full CMake/Ninja build also passes. GitHub would not let me reopen the PR after updating the branch, so please reopen it if this direction matches what you had in mind. |
|
GitHub returned state cannot be changed because the branch was force-pushed or recreated, so the corrected change is now available in #119. Please review the updated implementation there. |
2 participants
Description
RFC 6455 requires
Sec-WebSocket-Keyto be a Base64 value that decodes toexactly 16 bytes. Before this fix, malformed keys could still produce a
101 Switching Protocolsresponse.This revision rejects a key unless it has the required 24-character padded
form and the existing Base64 decoder produces exactly 16 bytes. The previous
variable-length SHA-1 change is removed, so the accepted key and GUID are
hashed using the existing
WS_KEYMS_LENlength.Verification
s3pPLMBiTxaQ9kYGzzhZRbK+xOo=.and 18-byte keys are rejected by the complete handshake-response path.
-Werror -pedantic.Checklist