From 334fc5b3e8b04279cb4b738df906e92fc419c28a Mon Sep 17 00:00:00 2001 From: TapTap Date: Sun, 13 Sep 2026 12:40:03 +0200 Subject: [PATCH] fix(protocol): harden STATUS_ERROR_DETAIL receive path Address review/security findings in the 2.21.0 error-detail feature: - Keepalive drain no longer erases the terminal detail: capture/clear is skipped for STATUS_KEEPALIVE so the reason the peer just sent survives the owed keepalive replies. - Replace the capture path with a dedicated protocol_receive_error_detail: the declared length is validated against MAX_ERROR_DETAIL_BYTES before any allocation, over-cap bodies are drained through a fixed scratch buffer (so the stream never desyncs), in-cap bodies read straight into the thread-local detail buffer, and session->max_alloc is never raised. Lengths beyond MAX_STRING_SIZE are treated as a fatal framing error. - The detail body now honors the caller's deadline (timed/keepalive paths) and polls the abort callback between drain chunks. - Escape peer-controlled detail text with output_escape before logging it in client_send.c and config.c. - Clear io_error_detail in io_set_fds so a new connection on the same thread cannot inherit a stale reason. - Add unit tests for the keepalive-survival, over-cap drain, absurd-length fatal framing, and deadline-clamped body read cases. --- src/client/client_send.c | 11 ++- src/shared/config.c | 11 ++- src/shared/protocol.c | 150 ++++++++++++++++++++++++++---------- src/shared/protocol.h | 7 +- tests/test_protocol_error.c | 144 ++++++++++++++++++++++++++++++++++ 5 files changed, 274 insertions(+), 49 deletions(-) diff --git a/src/client/client_send.c b/src/client/client_send.c index 42524ef..2494359 100644 --- a/src/client/client_send.c +++ b/src/client/client_send.c @@ -53,10 +53,15 @@ client-side context; a bare STATUS_ERROR still logs the context alone. */ static void log_server_rejection(const char* context) { const char* detail = protocol_last_error(); - if (detail && detail[0] != '\0') - log_message(LOG_LEVEL_ERROR, "%s: %s", context, detail); - else + if (detail && detail[0] != '\0') { + /* The detail is peer-controlled: escape it so terminal/log-format + * metacharacters cannot be injected into the client's output. */ + char* escaped = output_escape(detail, log_get_8_bit_output()); + log_message(LOG_LEVEL_ERROR, "%s: %s", context, escaped ? escaped : ""); + free(escaped); + } else { log_message(LOG_LEVEL_ERROR, "%s", context); + } } /* Forward declaration for progress-reporting thread used in multithreaded send. */ diff --git a/src/shared/config.c b/src/shared/config.c index fbe7adb..3c824b3 100644 --- a/src/shared/config.c +++ b/src/shared/config.c @@ -1239,10 +1239,15 @@ bool config_send(int file_descriptor, const Config* config) { } if (status != STATUS_OK) { const char* detail = protocol_last_error(); - if (detail && detail[0] != '\0') - log_message(LOG_LEVEL_ERROR, "Error transmitting config: %s", detail); - else + if (detail && detail[0] != '\0') { + /* The detail is peer-controlled: escape it before logging. */ + char* escaped = output_escape(detail, log_get_8_bit_output()); + log_message(LOG_LEVEL_ERROR, "Error transmitting config: %s", + escaped ? escaped : ""); + free(escaped); + } else { log_message(LOG_LEVEL_ERROR, "Error transmitting config"); + } return false; } return true; diff --git a/src/shared/protocol.c b/src/shared/protocol.c index 2bd2c44..b2f80e5 100644 --- a/src/shared/protocol.c +++ b/src/shared/protocol.c @@ -63,6 +63,10 @@ void io_set_fds(int read_fd, int write_fd) { bound_session = NULL; io_read_fd = read_fd; io_write_fd = write_fd; + /* A descriptor switch starts a new connection on this thread: a stale + rejection detail captured from the previous transport must not leak into + the new one. */ + io_error_detail[0] = '\0'; /* A descriptor switch starts a new transport; never reuse a TLS object belonging to a previous connection or test pipe. */ io_ssl = NULL; @@ -338,27 +342,25 @@ bool protocol_receive_n_data(ProtocolSession* session, void* data, size_t data_s return protocol_receive_n_data_timed(session, data, data_size, timeout_sec); } -bool protocol_receive_n_data_timed(ProtocolSession* session, void* data, size_t data_size, - int timeout_sec) { +/* Read exactly `data_size` bytes from `session` before `deadline` elapses + * (CLOCK_MONOTONIC). Shared by the ordinary timed primitive and the error-detail + * body reader so the latter can clamp itself to whatever deadline its caller + * already established instead of always applying the session's 60 s window. */ +static bool protocol_receive_n_data_until(ProtocolSession* session, void* data, size_t data_size, + const struct timespec* deadline) { log_debug_message(LOG_DEBUG_IO, " Receiving n Data: %zu", data_size); - if (!session) + if (!session || !deadline) return false; int fd = session->read_fd; - if (timeout_sec <= 0) - timeout_sec = RECEIVE_TIMEOUT_SEC; - - struct timespec deadline; - clock_gettime(CLOCK_MONOTONIC, &deadline); - deadline.tv_sec += timeout_sec; size_t total_bytes_received = 0; short wait_events = POLLIN; while (total_bytes_received < data_size) { if (!session->ssl || SSL_pending(session->ssl) == 0) { struct pollfd pfd = {.fd = fd, .events = wait_events}; - int poll_result = poll(&pfd, 1, deadline_remaining_ms(&deadline)); + int poll_result = poll(&pfd, 1, deadline_remaining_ms(deadline)); if (poll_result == 0) { - log_message(LOG_LEVEL_ERROR, "Receive timeout after %ds", timeout_sec); + log_message(LOG_LEVEL_ERROR, "Receive timeout"); return false; } if (poll_result < 0) { @@ -400,6 +402,18 @@ bool protocol_receive_n_data_timed(ProtocolSession* session, void* data, size_t return true; } +bool protocol_receive_n_data_timed(ProtocolSession* session, void* data, size_t data_size, + int timeout_sec) { + if (!session) + return false; + if (timeout_sec <= 0) + timeout_sec = RECEIVE_TIMEOUT_SEC; + struct timespec deadline; + clock_gettime(CLOCK_MONOTONIC, &deadline); + deadline.tv_sec += timeout_sec; + return protocol_receive_n_data_until(session, data, data_size, &deadline); +} + static const char* status_to_string(Status status) { switch (status) { case STATUS_OK: @@ -606,40 +620,83 @@ bool protocol_send_status(ProtocolSession* session, Status status) { return true; } +/* Read the bounded, length-prefixed body of a STATUS_ERROR_DETAIL frame within + * `deadline` (CLOCK_MONOTONIC), polling `abort_check` (may be NULL) between + * drain chunks. The declared length is validated BEFORE any allocation: + * + * - `size > MAX_STRING_SIZE`: an absurd framing error. Reading/draining that + * many bytes could never finish, so it is fatal (the caller tears the + * connection down) rather than drained. + * - `MAX_ERROR_DETAIL_BYTES < size <= MAX_STRING_SIZE`: drain exactly `size` + * bytes through a small fixed scratch buffer so the stream stays in sync, + * leaving the captured detail empty. No allocation happens. + * - `size <= MAX_ERROR_DETAIL_BYTES`: read straight into the thread-local + * `io_error_detail` buffer (size+1 capacity, already reserved), so the + * session's --max-alloc / MAX_CONNECTION_MEMORY budgets are never touched. + * + * Returns false on a fatal framing problem or any I/O failure; the terminal + * detail is then empty. The body is consumed on every non-fatal path even when + * the caller ignores protocol_last_error(), so the stream never desyncs. */ +static bool protocol_receive_error_detail_until(ProtocolSession* session, + const struct timespec* deadline, + ProtocolWaitAbort abort_check) { + io_error_detail[0] = '\0'; + size_t size = 0; + if (!protocol_receive_n_data_until(session, &size, sizeof(size), deadline)) + return false; + if (size > MAX_STRING_SIZE) { + log_message(LOG_LEVEL_ERROR, "Error detail length %zu exceeds maximum %llu", size, + (unsigned long long)MAX_STRING_SIZE); + return false; + } + if (size > MAX_ERROR_DETAIL_BYTES) { + char scratch[256]; + size_t remaining = size; + while (remaining > 0) { + if (abort_check && abort_check()) + return false; + size_t chunk = remaining < sizeof(scratch) ? remaining : sizeof(scratch); + if (!protocol_receive_n_data_until(session, scratch, chunk, deadline)) + return false; + remaining -= chunk; + } + return true; + } + if (!protocol_receive_n_data_until(session, io_error_detail, size, deadline)) + return false; + io_error_detail[size] = '\0'; + return true; +} + /* Consume the optional detail body of a STATUS_ERROR_DETAIL frame and map the * status back to STATUS_ERROR for existing callers. Invoked for EVERY status - * read (bare STATUS_OK/STATUS_ERROR too) so a stale detail from an earlier - * exchange is never reported for a later one. The body is always read, even - * when the caller ignores protocol_last_error(), so the stream never - * desynchronizes. */ -static void protocol_capture_error_detail(ProtocolSession* session, Status* status) { + * read so a stale detail from an earlier exchange is never reported for a later + * one -- except for STATUS_KEEPALIVE, which carries no body and whose drain + * (protocol_receive_status_keepalive) must NOT erase the terminal detail that + * arrived just before it. Returns false on a fatal framing error. */ +static bool protocol_capture_error_detail(ProtocolSession* session, Status* status, + const struct timespec* deadline, + ProtocolWaitAbort abort_check) { + if (*status == STATUS_KEEPALIVE) + return true; io_error_detail[0] = '\0'; if (*status != STATUS_ERROR_DETAIL) - return; + return true; *status = STATUS_ERROR; - /* The body MUST be drained even when the session's allocation ceiling is - * smaller than the message (e.g. a tiny --max-alloc), otherwise the string - * body would be left on the stream and desynchronize the next exchange. - * Lift the ceiling for this one bounded string read and restore it. */ - unsigned long long saved_max_alloc = session->max_alloc; - if (saved_max_alloc < MAX_STRING_SIZE + 1) - session->max_alloc = MAX_STRING_SIZE + 1; - char* detail = protocol_receive_str(session); - session->max_alloc = saved_max_alloc; - if (!detail) - return; - size_t len = strlen(detail); - if (len > MAX_ERROR_DETAIL_BYTES) - len = MAX_ERROR_DETAIL_BYTES; - memcpy(io_error_detail, detail, len); - io_error_detail[len] = '\0'; - free(detail); + return protocol_receive_error_detail_until(session, deadline, abort_check); } bool protocol_receive_status(ProtocolSession* session, Status* status) { - if (!protocol_receive_n_data(session, status, sizeof(Status))) + if (!session || !status) + return false; + int timeout_sec = session->io_timeout_sec > 0 ? session->io_timeout_sec : RECEIVE_TIMEOUT_SEC; + struct timespec deadline; + clock_gettime(CLOCK_MONOTONIC, &deadline); + deadline.tv_sec += timeout_sec; + if (!protocol_receive_n_data_until(session, status, sizeof(Status), &deadline)) + return false; + if (!protocol_capture_error_detail(session, status, &deadline, NULL)) return false; - protocol_capture_error_detail(session, status); log_debug_message(LOG_DEBUG_PROTO, "Received Status: %s", status_to_string(*status)); return true; } @@ -647,11 +704,20 @@ bool protocol_receive_status(ProtocolSession* session, Status* status) { /* protocol_receive_status with an explicit per-message deadline (seconds). Used where a single reply may legitimately take far longer than the default 60 s receive window - e.g. the sender waiting for the early-delete ACK after - the receiver committed a large (up to MAX_SERVER_DELETE_COUNT) deletion. */ + the receiver committed a large (up to MAX_SERVER_DELETE_COUNT) deletion. The + error-detail body shares the same deadline as the status header. */ bool protocol_receive_status_timed(ProtocolSession* session, Status* status, int timeout_sec) { - if (!protocol_receive_n_data_timed(session, status, sizeof(Status), timeout_sec)) + if (!session || !status) + return false; + if (timeout_sec <= 0) + timeout_sec = RECEIVE_TIMEOUT_SEC; + struct timespec deadline; + clock_gettime(CLOCK_MONOTONIC, &deadline); + deadline.tv_sec += timeout_sec; + if (!protocol_receive_n_data_until(session, status, sizeof(Status), &deadline)) + return false; + if (!protocol_capture_error_detail(session, status, &deadline, NULL)) return false; - protocol_capture_error_detail(session, status); log_debug_message(LOG_DEBUG_PROTO, "Received Status: %s", status_to_string(*status)); return true; } @@ -763,7 +829,8 @@ bool protocol_receive_status_keepalive(ProtocolSession* session, Status* status, Status received; if (!protocol_read_status_until(session, &received, &deadline)) return false; - protocol_capture_error_detail(session, &received); + if (!protocol_capture_error_detail(session, &received, &deadline, abort_check)) + return false; if (received == STATUS_KEEPALIVE) { /* The receiver's answer to one of our keepalives. */ replies_seen++; @@ -790,7 +857,8 @@ bool protocol_receive_status_keepalive(ProtocolSession* session, Status* status, keepalives_sent - replies_seen); break; } - protocol_capture_error_detail(session, &drained); + if (!protocol_capture_error_detail(session, &drained, &drain_deadline, abort_check)) + return false; if (drained != STATUS_KEEPALIVE) { log_message(LOG_LEVEL_ERROR, "Unexpected status while draining keepalive replies"); return false; diff --git a/src/shared/protocol.h b/src/shared/protocol.h index e79f84c..7d274e4 100644 --- a/src/shared/protocol.h +++ b/src/shared/protocol.h @@ -212,8 +212,11 @@ bool receive_status(int file_descriptor, Status* status); bool send_error_detail(int file_descriptor, const char* message); /* Human-readable reason captured from the most recent STATUS_ERROR_DETAIL * received on this thread, or "" when the last status was a bare STATUS_ERROR - * (or no detail was seen). Thread-local, and valid until the next status read - * on the same thread. */ + * (or no detail was seen). Thread-local, and valid until the next non-keepalive + * status read on the same thread; a later STATUS_KEEPALIVE does NOT clear it. + * The detail body is bounded by MAX_ERROR_DETAIL_BYTES: an over-cap declared + * length is drained and yields "" (so the stream never desyncs), while an + * absurd length is a fatal framing error that fails the status read. */ const char* protocol_last_error(void); /* Clear the thread-local last-error buffer. */ void protocol_clear_last_error(void); diff --git a/tests/test_protocol_error.c b/tests/test_protocol_error.c index 9839725..14df0bd 100644 --- a/tests/test_protocol_error.c +++ b/tests/test_protocol_error.c @@ -4,6 +4,7 @@ #include "test_utils.h" #include #include +#include #include /* A detail frame maps back to STATUS_ERROR for the caller and its body is @@ -85,6 +86,8 @@ static void test_error_detail_drains_despite_tiny_max_alloc(void) { EXPECT_TRUE(protocol_receive_status(&receiver, &status)); EXPECT_EQ_INT((int)status, (int)STATUS_ERROR); EXPECT_EQ_STR(protocol_last_error(), "reason"); + /* The bounded detail reader must never touch the session allocation ceiling. */ + EXPECT_EQ_INT((int)receiver.max_alloc, 4); EXPECT_TRUE(protocol_send_status(&sender, STATUS_NEXT)); EXPECT_TRUE(protocol_receive_status(&receiver, &status)); @@ -94,6 +97,143 @@ static void test_error_detail_drains_despite_tiny_max_alloc(void) { close(p[1]); } +/* An over-cap (but not absurd) declared length is drained through a fixed + * scratch buffer so the stream stays in sync, and yields an empty detail. The + * session's tiny --max-alloc must remain untouched. */ +static void test_error_detail_over_cap_is_drained(void) { + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + ProtocolSession receiver; + protocol_session_init(&receiver, p[0], p[1]); + protocol_session_set_max_alloc(&receiver, 8); + ProtocolSession sender; + protocol_session_init(&sender, -1, p[1]); + + size_t size = MAX_ERROR_DETAIL_BYTES + 128; + EXPECT_TRUE(protocol_send_status(&sender, STATUS_ERROR_DETAIL)); + EXPECT_TRUE(protocol_send_n_data(&sender, &size, sizeof(size))); + char chunk[512]; + memset(chunk, 'z', sizeof(chunk)); + size_t written = 0; + while (written < size) { + size_t n = size - written < sizeof(chunk) ? size - written : sizeof(chunk); + EXPECT_TRUE(protocol_send_n_data(&sender, chunk, n)); + written += n; + } + EXPECT_TRUE(protocol_send_status(&sender, STATUS_NEXT)); + + Status status = STATUS_OK; + EXPECT_TRUE(protocol_receive_status(&receiver, &status)); + EXPECT_EQ_INT((int)status, (int)STATUS_ERROR); + EXPECT_EQ_STR(protocol_last_error(), ""); + EXPECT_EQ_INT((int)receiver.max_alloc, 8); + + /* Stream is still framed: the following status is read intact. */ + EXPECT_TRUE(protocol_receive_status(&receiver, &status)); + EXPECT_EQ_INT((int)status, (int)STATUS_NEXT); + + close(p[0]); + close(p[1]); +} + +/* A declared length beyond even the absolute string bound can never be drained + * sensibly, so it is a fatal framing error and the status read fails. */ +static void test_error_detail_absurd_length_is_fatal(void) { + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + ProtocolSession receiver; + protocol_session_init(&receiver, p[0], p[1]); + ProtocolSession sender; + protocol_session_init(&sender, -1, p[1]); + + size_t size = (size_t)MAX_STRING_SIZE + 1; + EXPECT_TRUE(protocol_send_status(&sender, STATUS_ERROR_DETAIL)); + EXPECT_TRUE(protocol_send_n_data(&sender, &size, sizeof(size))); + + Status status = STATUS_OK; + EXPECT_FALSE(protocol_receive_status(&receiver, &status)); + + close(p[0]); + close(p[1]); +} + +/* The detail body must share the caller's deadline: with the session window at + * the 60 s default, a withheld body under a 1 s receive_status_timed deadline + * must fail in about a second, not fall back to the session timeout. */ +static void test_error_detail_body_honors_deadline(void) { + int p[2]; + EXPECT_EQ_INT(pipe(p), 0); + io_set_fds(p[0], p[1]); + io_set_bwlimit(0); + + /* Only the status header, body withheld. */ + EXPECT_TRUE(send_status(0, STATUS_ERROR_DETAIL)); + + struct timespec start, end; + clock_gettime(CLOCK_MONOTONIC, &start); + Status status = STATUS_OK; + EXPECT_FALSE(receive_status_timed(0, &status, 1)); + clock_gettime(CLOCK_MONOTONIC, &end); + long long elapsed_ms = + (end.tv_sec - start.tv_sec) * 1000LL + (end.tv_nsec - start.tv_nsec) / 1000000LL; + EXPECT_TRUE(elapsed_ms < 10000); + + close(p[0]); + close(p[1]); +} + +typedef struct { + int peer_read_fd; + int peer_write_fd; + bool replied; +} DetailKeepalivePeerArg; + +static int detail_keepalive_peer(void* arg) { + DetailKeepalivePeerArg* peer = arg; + ProtocolSession session; + protocol_session_init(&session, peer->peer_read_fd, peer->peer_write_fd); + Status status = STATUS_ERROR; + if (protocol_receive_status(&session, &status) && status == STATUS_KEEPALIVE) { + /* The busy receiver answers the real status (with its detail) first, then the + keepalive reply it owes -- which the client then drains. */ + peer->replied = protocol_send_status(&session, STATUS_ERROR_DETAIL) && + protocol_send_str(&session, "boom") && + protocol_send_status(&session, STATUS_KEEPALIVE); + } + return thrd_success; +} + +/* Draining the keepalive replies the peer still owes must not erase the terminal + * detail that arrived just before them. */ +static void test_error_detail_survives_keepalive_drain(void) { + int to_client[2]; + int to_peer[2]; + EXPECT_EQ_INT(pipe(to_client), 0); + EXPECT_EQ_INT(pipe(to_peer), 0); + + ProtocolSession session; + protocol_session_init(&session, to_client[0], to_peer[1]); + + DetailKeepalivePeerArg peer = {.peer_read_fd = to_peer[0], .peer_write_fd = to_client[1]}; + thrd_t thread; + EXPECT_EQ_INT(thrd_create(&thread, detail_keepalive_peer, &peer), thrd_success); + + Status received = STATUS_OK; + EXPECT_TRUE(protocol_receive_status_keepalive(&session, &received, 10, 1, NULL)); + EXPECT_EQ_INT((int)received, (int)STATUS_ERROR); + EXPECT_EQ_STR(protocol_last_error(), "boom"); + + int result = 0; + EXPECT_EQ_INT(thrd_join(thread, &result), thrd_success); + EXPECT_EQ_INT(result, thrd_success); + EXPECT_TRUE(peer.replied); + + close(to_client[0]); + close(to_client[1]); + close(to_peer[0]); + close(to_peer[1]); +} + typedef struct { ProtocolSession* receiver; } DetailWorkerArg; @@ -149,5 +289,9 @@ void test_protocol_error(void) { test_error_detail_over_long_is_bounded(); test_bare_error_clears_last_error(); test_error_detail_drains_despite_tiny_max_alloc(); + test_error_detail_over_cap_is_drained(); + test_error_detail_absurd_length_is_fatal(); + test_error_detail_body_honors_deadline(); + test_error_detail_survives_keepalive_drain(); test_last_error_is_thread_local(); }