Skip to content

Commit 06ab442

Browse files
stelfragstelfrag
authored
aclk: report the HTTP status when the WebSocket upgrade fails (#24537)
* aclk: report the HTTP status when the WebSocket upgrade fails A proxy or ingress that does not upgrade the connection answers with its own status (404, 502, ...) and no Sec-WebSocket-Accept header. The accept header was checked first, so the log said "Missing sec-websocket-accept header" and the status was never shown. Decide the parsed reply in one place: status first (with the reason and the Server header), then the accept headers. Peer strings in the log are bounded and control bytes replaced. This also drops nonce_matched, which ws_client_reset() never cleared: a reconnect accepted a 101 reply with no accept header. * aclk: accept empty and unpadded header values in the upgrade reply An empty Server header made the parser fail before the status check, so the status was lost again. Split on ':' and trim optional whitespace (RFC 9112 5). * aclk: log only printable ASCII from the upgrade reply Bytes 0x80-0x9f (C1 controls, raw or UTF-8 encoded) passed through to the log. Mask everything outside 0x20-0x7e. * aclk: build handshake test replies without printf formats Test replies and expected messages were passed to snprintf as the format. Substitute a {NONCE} token instead. * aclk: check the bound before reading in ws_client_log_safe() * aclk: report the status of HTTP/1.0 replies to the upgrade A proxy may answer with an HTTP/1.0 error page, which was rejected before its status was read. Parse HTTP/1.0 status lines too and require HTTP/1.1 only for a 101.
1 parent 7fc1f97 commit 06ab442

3 files changed

Lines changed: 279 additions & 32 deletions

File tree

‎src/aclk/mqtt_websockets/ws_client-unittest.c‎

Lines changed: 175 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#include "endian_compat.h"
77

88
int ws_client_process_rx_ws(ws_client *client);
9+
int ws_client_parse_handshake_resp(ws_client *client);
910

1011
#define WS_TEST(condition, msg) do { \
1112
if (!(condition)) { \
@@ -415,6 +416,179 @@ static int ws_client_unittest_ping_payload_cleanup(void)
415416
return errors;
416417
}
417418

419+
#define WS_UT_NONCE "{NONCE}"
420+
421+
// Copies `tmpl` to `dst`, replacing every WS_UT_NONCE with `nonce`. Returns the length, or -1 if it does not fit.
422+
static int ws_client_unittest_fill(char *dst, size_t dst_size, const char *tmpl, const char *nonce)
423+
{
424+
size_t len = 0, token_len = strlen(WS_UT_NONCE), nonce_len = strlen(nonce);
425+
while (*tmpl) {
426+
const char *add = tmpl;
427+
size_t add_len = 1;
428+
if (!strncmp(tmpl, WS_UT_NONCE, token_len)) {
429+
add = nonce;
430+
add_len = nonce_len;
431+
tmpl += token_len;
432+
}
433+
else
434+
tmpl++;
435+
436+
if (len + add_len >= dst_size)
437+
return -1;
438+
memcpy(dst + len, add, add_len);
439+
len += add_len;
440+
}
441+
dst[len] = '\0';
442+
return (int)len;
443+
}
444+
445+
// Feeds one canned upgrade reply to a client that has sent its upgrade request.
446+
// `reply_tmpl` may hold WS_UT_NONCE, replaced by the accept value the client expects.
447+
static int ws_client_unittest_handshake_reply(ws_client *client, const char *reply_tmpl)
448+
{
449+
char reply[1024];
450+
int len = ws_client_unittest_fill(reply, sizeof(reply), reply_tmpl, client->hs.nonce_reply);
451+
if (len < 0 || rbuf_push(client->buf_read, reply, len) != (size_t)len)
452+
return WS_CLIENT_INTERNAL_ERROR;
453+
454+
int ret;
455+
do {
456+
ret = ws_client_parse_handshake_resp(client);
457+
} while (ret == 0);
458+
459+
return ret;
460+
}
461+
462+
// `expected_err` is the reason ws_client_check_handshake_reply() gives, NULL when the reply is accepted;
463+
// like `reply_tmpl`, it may hold WS_UT_NONCE for the expected accept value.
464+
static int ws_client_unittest_handshake_case(const char *name, const char *reply_tmpl, const char *expected_err)
465+
{
466+
int errors = 0;
467+
char *host = (char *)"localhost";
468+
ws_client *client = ws_client_new(4096, &host);
469+
470+
WS_TEST(client != NULL, name);
471+
if (!client)
472+
return errors;
473+
474+
WS_TEST(ws_client_start_handshake(client) == 0, name);
475+
476+
fprintf(stderr, "ws_client handshake case: %s\n", name);
477+
int ret = ws_client_unittest_handshake_reply(client, reply_tmpl);
478+
WS_TEST(ret == (expected_err ? WS_CLIENT_PROTOCOL_ERROR : WS_CLIENT_PARSING_DONE), name);
479+
WS_TEST(client->state == (expected_err ? WS_HANDSHAKE : WS_ESTABLISHED), name);
480+
481+
char err[512] = "", expected[512] = "";
482+
int rc = ws_client_check_handshake_reply(client, err, sizeof(err));
483+
if (expected_err) {
484+
WS_TEST(ws_client_unittest_fill(expected, sizeof(expected), expected_err, client->hs.nonce_reply) >= 0, name);
485+
WS_TEST(rc == -1, name);
486+
WS_TEST(!strcmp(err, expected), name);
487+
if (strcmp(err, expected))
488+
fprintf(stderr, " got: %s\n expected: %s\n", err, expected);
489+
}
490+
else
491+
WS_TEST(rc == 0, name);
492+
493+
ws_client_destroy(client);
494+
return errors;
495+
}
496+
497+
#define WS_UT_101 "HTTP/1.1 101 Switching Protocols\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n"
498+
#define WS_UT_404 "HTTP/1.1 404 Not Found\r\nServer: nginx/1.25.3\r\nContent-Length: 0\r\n"
499+
#define WS_UT_404_ERR "HTTP return code not 101. Received 404 with msg \"Not Found\" (server: \"nginx/1.25.3\")."
500+
#define WS_UT_WRONG "AAAAAAAAAAAAAAAAAAAAAAAAAAA="
501+
#define WS_UT_WRONG_ACCEPT "Sec-WebSocket-Accept: " WS_UT_WRONG "\r\n"
502+
#define WS_UT_WRONG_ERR "Received NONCE \"" WS_UT_WRONG "\" does not match expected nonce of \"" WS_UT_NONCE "\""
503+
#define WS_UT_MISSING_ERR "Missing sec-websocket-accept header"
504+
505+
static int ws_client_unittest_handshake(void)
506+
{
507+
int errors = 0;
508+
509+
// a non-101 reply fails whatever it says about the accept header, and the reason names the status
510+
errors += ws_client_unittest_handshake_case("404 without accept", WS_UT_404 "\r\n", WS_UT_404_ERR);
511+
errors += ws_client_unittest_handshake_case("404 with wrong accept", WS_UT_404 WS_UT_WRONG_ACCEPT "\r\n", WS_UT_404_ERR);
512+
errors += ws_client_unittest_handshake_case("404 with matching accept", WS_UT_404 "Sec-WebSocket-Accept: " WS_UT_NONCE "\r\n\r\n", WS_UT_404_ERR);
513+
errors += ws_client_unittest_handshake_case("502 with empty reason, no server", "HTTP/1.1 502 \r\n\r\n",
514+
"HTTP return code not 101. Received 502 with msg \"\".");
515+
errors += ws_client_unittest_handshake_case("403 with control bytes", "HTTP/1.1 403 Forbid\x01" "den\r\nServer: evil\x1b[31mred\r\n\r\n",
516+
"HTTP return code not 101. Received 403 with msg \"Forbid?den\" (server: \"evil?[31mred\").");
517+
errors += ws_client_unittest_handshake_case("403 with C1 and non-ASCII bytes", "HTTP/1.1 403 Forbid\x9b" "den\r\nServer: caf\xc3\xa9 \xc2\x9b" "2J\r\n\r\n",
518+
"HTTP return code not 101. Received 403 with msg \"Forbid?den\" (server: \"caf?? ??2J\").");
519+
// header values may be empty and have optional whitespace around them
520+
errors += ws_client_unittest_handshake_case("404 with empty server value", "HTTP/1.1 404 Not Found\r\nServer: \r\n\r\n",
521+
"HTTP return code not 101. Received 404 with msg \"Not Found\" (server: \"\").");
522+
errors += ws_client_unittest_handshake_case("404 with bare server field", "HTTP/1.1 404 Not Found\r\nServer:\r\n\r\n",
523+
"HTTP return code not 101. Received 404 with msg \"Not Found\" (server: \"\").");
524+
errors += ws_client_unittest_handshake_case("404 with unpadded and padded fields", "HTTP/1.1 404 Not Found\r\nX-Id:1\r\nServer: \t envoy \t\r\n\r\n",
525+
"HTTP return code not 101. Received 404 with msg \"Not Found\" (server: \"envoy\").");
526+
{
527+
// peer strings are cut to 127 bytes in the log
528+
char reply[700], expected[400], longval[301];
529+
memset(longval, 'x', 300);
530+
longval[300] = '\0';
531+
snprintf(reply, sizeof(reply), "HTTP/1.1 400 %s\r\nServer: %s\r\n\r\n", longval, longval);
532+
snprintf(expected, sizeof(expected), "HTTP return code not 101. Received 400 with msg \"%.127s\" (server: \"%.127s\").", longval, longval);
533+
errors += ws_client_unittest_handshake_case("400 with long reason and server", reply, expected);
534+
}
535+
536+
// a 101 reply needs exactly the expected accept value, in every accept header it carries
537+
errors += ws_client_unittest_handshake_case("101 without accept", WS_UT_101 "\r\n", WS_UT_MISSING_ERR);
538+
errors += ws_client_unittest_handshake_case("101 with wrong accept", WS_UT_101 WS_UT_WRONG_ACCEPT "\r\n", WS_UT_WRONG_ERR);
539+
errors += ws_client_unittest_handshake_case("101 with control bytes in wrong accept", WS_UT_101 "Sec-WebSocket-Accept: \x07" "bell\r\n\r\n",
540+
"Received NONCE \"?bell\" does not match expected nonce of \"" WS_UT_NONCE "\"");
541+
errors += ws_client_unittest_handshake_case("101 with matching then wrong accept", WS_UT_101 "Sec-WebSocket-Accept: " WS_UT_NONCE "\r\n" WS_UT_WRONG_ACCEPT "\r\n", WS_UT_WRONG_ERR);
542+
errors += ws_client_unittest_handshake_case("101 with wrong then matching accept", WS_UT_101 WS_UT_WRONG_ACCEPT "Sec-WebSocket-Accept: " WS_UT_NONCE "\r\n\r\n", WS_UT_WRONG_ERR);
543+
errors += ws_client_unittest_handshake_case("101 with matching accept", WS_UT_101 "sec-websocket-accept: " WS_UT_NONCE "\r\n\r\n", NULL);
544+
errors += ws_client_unittest_handshake_case("101 with unpadded matching accept", WS_UT_101 "Sec-WebSocket-Accept:" WS_UT_NONCE "\r\n\r\n", NULL);
545+
errors += ws_client_unittest_handshake_case("101 with padded matching accept", WS_UT_101 "Sec-WebSocket-Accept: \t " WS_UT_NONCE " \t\r\n\r\n", NULL);
546+
errors += ws_client_unittest_handshake_case("101 with empty accept", WS_UT_101 "Sec-WebSocket-Accept: \r\n\r\n",
547+
"Received NONCE \"\" does not match expected nonce of \"" WS_UT_NONCE "\"");
548+
// a proxy may answer with an HTTP/1.0 error page; its status is still reported, but 1.0 cannot upgrade
549+
errors += ws_client_unittest_handshake_case("HTTP/1.0 503 from a proxy", "HTTP/1.0 503 Service Unavailable\r\nServer: haproxy\r\n\r\n",
550+
"HTTP return code not 101. Received 503 with msg \"Service Unavailable\" (server: \"haproxy\").");
551+
errors += ws_client_unittest_handshake_case("HTTP/1.0 101 with matching accept", "HTTP/1.0 101 Switching Protocols\r\nSec-WebSocket-Accept: " WS_UT_NONCE "\r\n\r\n",
552+
"Received HTTP/1.0 101 reply; a WebSocket upgrade requires HTTP/1.1");
553+
554+
// a reply that is not HTTP/1.x fails while the status line is parsed
555+
{
556+
char *junk_host = (char *)"localhost";
557+
ws_client *junk = ws_client_new(4096, &junk_host);
558+
WS_TEST(junk != NULL, "junk status line: client");
559+
if (junk) {
560+
WS_TEST(ws_client_start_handshake(junk) == 0, "junk status line: handshake starts");
561+
fprintf(stderr, "ws_client handshake case: junk status line\n");
562+
WS_TEST(ws_client_unittest_handshake_reply(junk, "SSH-2.0-OpenSSH_9.6\r\n\r\n") == WS_CLIENT_PROTOCOL_ERROR,
563+
"junk status line: rejected");
564+
WS_TEST(junk->state == WS_HANDSHAKE, "junk status line: not established");
565+
ws_client_destroy(junk);
566+
}
567+
}
568+
569+
// a reconnect must not inherit the previous connection's accepted handshake
570+
char *host = (char *)"localhost";
571+
ws_client *client = ws_client_new(4096, &host);
572+
WS_TEST(client != NULL, "reconnect: client");
573+
if (!client)
574+
return errors;
575+
576+
WS_TEST(ws_client_start_handshake(client) == 0, "reconnect: first handshake starts");
577+
fprintf(stderr, "ws_client handshake case: reconnect, first 101 with matching accept\n");
578+
WS_TEST(ws_client_unittest_handshake_reply(client, WS_UT_101 "Sec-WebSocket-Accept: " WS_UT_NONCE "\r\n\r\n") == WS_CLIENT_PARSING_DONE,
579+
"reconnect: first handshake accepted");
580+
581+
ws_client_reset(client);
582+
WS_TEST(ws_client_start_handshake(client) == 0, "reconnect: second handshake starts");
583+
fprintf(stderr, "ws_client handshake case: reconnect, second 101 without accept\n");
584+
WS_TEST(ws_client_unittest_handshake_reply(client, WS_UT_101 "\r\n") == WS_CLIENT_PROTOCOL_ERROR,
585+
"reconnect: 101 without accept after reset is rejected");
586+
WS_TEST(client->state == WS_HANDSHAKE, "reconnect: not established");
587+
588+
ws_client_destroy(client);
589+
return errors;
590+
}
591+
418592
int ws_client_unittest(void)
419593
{
420594
int errors = 0;
@@ -428,6 +602,7 @@ int ws_client_unittest(void)
428602
errors += ws_client_unittest_cap_hit();
429603
errors += ws_client_unittest_reset_frees_partial_close_reason();
430604
errors += ws_client_unittest_ping_payload_cleanup();
605+
errors += ws_client_unittest_handshake();
431606

432607
if (errors)
433608
fprintf(stderr, "ws_client unittest: %d ERROR(S)\n", errors);

0 commit comments

Comments
 (0)