From d3210269930af1c9b42887b2bd53cb0b8de76b44 Mon Sep 17 00:00:00 2001 From: Vsevolod Stakhov Date: Fri, 31 Jul 2026 09:41:46 +0100 Subject: [PATCH] [Fix] Close snapshot truncation race and query corruption Two review findings. rspamd_shmem_segment_map() mapped the selected window and copied out of it. The backing object belongs to the client, so an ftruncate landing between the fstat and the memcpy raised SIGBUS inside the copy and took the worker down; the mapping was only dropped afterwards, so the earlier regression test, which resized the object once the call had already returned, never exercised that window at all. Read the window straight into pool storage instead. A read cannot fault: a concurrent truncation merely returns fewer bytes, and the payload length now reflects what was actually read rather than what was asked for. Short reads and EINTR are handled in the loop. POSIX shared memory descriptors do not accept positional reads on every platform - macOS reports ESPIPE and some BSDs ENODEV - so the mapping path is kept as a fallback for exactly those descriptors. On Linux, where shared memory objects live on tmpfs and HAVE_SANE_SHMEM is always set, the read path is the one taken and the race is closed. proxy_strip_query_args() rebuilt the url from a prefix of u.field_data[UF_QUERY].off bytes. That offset addresses the first byte after the '?', so the prefix already carried the delimiter and the appended one produced '/checkv2??From=...'. Since the proxy turns query arguments into request headers at the upstream, the first surviving argument was then parsed as '?From' and its value silently lost. Stop the prefix one byte short. The previous test stripped every argument present, so nothing survived and the corruption stayed invisible. Both regressions are now covered. A thread toggles the size of a backing object while the snapshot is taken repeatedly, which faults on the old code and passes on the new one, and a proxy request now carries an ordinary query argument alongside a privileged one and asserts the ordinary one reaches the upstream intact. --- src/libserver/task.c | 103 ++++++++- src/rspamd_proxy.c | 14 +- .../functional/cases/572_file_shm_proxy.robot | 23 ++ test/functional/lua/file_shm_probe.lua | 45 ++-- test/rspamd_cxx_unit_task_input.hxx | 211 ++++++++++++++++++ 5 files changed, 371 insertions(+), 25 deletions(-) diff --git a/src/libserver/task.c b/src/libserver/task.c index dfd077d80b..7013aa792d 100644 --- a/src/libserver/task.c +++ b/src/libserver/task.c @@ -503,6 +503,62 @@ rspamd_task_read_snapshot(int fd, char *buf, gsize len, gsize *read_len) return TRUE; } +enum rspamd_snapshot_result { + RSPAMD_SNAPSHOT_OK = 0, + RSPAMD_SNAPSHOT_ERROR, + /* This descriptor does not support positional reads at all */ + RSPAMD_SNAPSHOT_UNSUPPORTED, +}; + +/* + * Reads `len` bytes at `offset` into `buf`, coping with short reads and EINTR. + * + * Unlike a copy out of a mapping, a read can never fault: if the object is + * truncated while we are reading it we merely get fewer bytes than we asked + * for, which the caller reflects in the payload length. + */ +static enum rspamd_snapshot_result +rspamd_task_pread_snapshot(int fd, char *buf, gsize len, off_t offset, + gsize *read_len) +{ + gsize total = 0; + + while (total < len) { + ssize_t r = pread(fd, buf + total, len - total, + offset + (off_t) total); + + if (r > 0) { + total += (gsize) r; + } + else if (r == 0) { + /* Truncated under us, whatever we have got is all there is */ + break; + } + else if (errno == EINTR) { + continue; + } + else if (total == 0 && (errno == ESPIPE || errno == ENODEV || + errno == EINVAL || errno == ENOTSUP || + errno == EOPNOTSUPP)) { + /* + * POSIX shared memory descriptors cannot be read on every platform + * (macOS returns ESPIPE, some BSDs ENODEV), so the caller has to + * fall back to mapping the window there. + */ + return RSPAMD_SNAPSHOT_UNSUPPORTED; + } + else { + *read_len = total; + + return RSPAMD_SNAPSHOT_ERROR; + } + } + + *read_len = total; + + return RSPAMD_SNAPSHOT_OK; +} + struct rspamd_shmem_segment * rspamd_shmem_segment_map(rspamd_mempool_t *pool, const rspamd_ftok_t *name_tok, @@ -514,11 +570,12 @@ rspamd_shmem_segment_map(rspamd_mempool_t *pool, char namebuf[PATH_MAX]; const char *name = namebuf; gulong offset = 0, length = 0; - gsize page_size, aligned_offset, delta, map_len; + gsize page_size, aligned_offset, delta, map_len, nread = 0; struct stat st; int fd; gpointer map; char *data; + enum rspamd_snapshot_result res; struct rspamd_shmem_segment *seg; #ifdef HAVE_SANE_SHMEM const char *ft = "shm"; @@ -654,9 +711,40 @@ rspamd_shmem_segment_map(rspamd_mempool_t *pool, } /* - * Map merely the window that is really needed: the offset is rounded down - * to a page boundary and the length is extended by the very same delta, so - * that a small payload inside a huge object never maps that whole object. + * The backing object belongs to the client and it can be truncated or + * rewritten at any moment, so the parser must never be handed a live + * mapping. Read the window straight into pool storage instead of copying it + * out of one: a concurrent ftruncate between the fstat above and the copy + * would raise SIGBUS on a mapping and take the whole worker down, whereas a + * read merely returns fewer bytes. + */ + data = rspamd_mempool_alloc(pool, length); + res = rspamd_task_pread_snapshot(fd, data, length, (off_t) offset, &nread); + + if (res == RSPAMD_SNAPSHOT_ERROR) { + g_set_error(err, rspamd_task_quark(), RSPAMD_PROTOCOL_ERROR, + "cannot read %s segment (%s): %s", ft, name, + strerror(errno)); + close(fd); + + return NULL; + } + + if (res == RSPAMD_SNAPSHOT_OK) { + close(fd); + /* The object may have shrunk under us, so trust what we really got */ + seg->data_len = nread; + seg->data = data; + + return seg; + } + + /* + * This descriptor cannot be read positionally, which happens for POSIX + * shared memory on some platforms, so fall back to mapping. Map merely the + * window that is really needed: the offset is rounded down to a page + * boundary and the length is extended by the very same delta, so that a + * small payload inside a huge object never maps that whole object. */ page_size = (gsize) sysconf(_SC_PAGESIZE); @@ -679,13 +767,6 @@ rspamd_shmem_segment_map(rspamd_mempool_t *pool, return NULL; } - /* - * The backing object belongs to the client and it can be truncated or - * rewritten at any moment, so a live mapping handed over to the parser - * could fault later on. Snapshot the window into the pool and drop both the - * mapping and the descriptor right away. - */ - data = rspamd_mempool_alloc(pool, length); memcpy(data, (const char *) map + delta, length); munmap(map, map_len); close(fd); diff --git a/src/rspamd_proxy.c b/src/rspamd_proxy.c index 5217f757d0..dea0bcb5c6 100644 --- a/src/rspamd_proxy.c +++ b/src/rspamd_proxy.c @@ -1960,8 +1960,20 @@ proxy_strip_query_args(struct rspamd_http_message *msg, return; } + if (u.field_data[UF_QUERY].off == 0) { + /* A query always follows a '?', so this cannot happen; be defensive */ + g_hash_table_unref(query_args); + + return; + } + + /* + * UF_QUERY.off addresses the first byte *after* the '?', so the prefix has + * to stop one byte short of it: copying up to `off` would keep the original + * delimiter and the one appended below would produce a second one. + */ new_url = rspamd_fstring_new_init(RSPAMD_FSTRING_DATA(msg->url), - u.field_data[UF_QUERY].off); + u.field_data[UF_QUERY].off - 1); new_url = rspamd_fstring_append(new_url, "?", 1); g_hash_table_iter_init(&it, query_args); diff --git a/test/functional/cases/572_file_shm_proxy.robot b/test/functional/cases/572_file_shm_proxy.robot index f38ee2f5e9..db5879b10e 100644 --- a/test/functional/cases/572_file_shm_proxy.robot +++ b/test/functional/cases/572_file_shm_proxy.robot @@ -87,6 +87,29 @@ Client Shm query arguments never reach the upstream Expect Symbol SIMPLE_TEST Expect Symbol With Exact Options FILE_SHM_PROBE none +Ordinary query arguments survive the stripping of a privileged one + [Documentation] Stripping Shm means rebuilding the URL around it, and every + ... surviving argument becomes a request header at the upstream. + ... An argument that shared the URL with a stripped one must + ... therefore arrive under its own name: a rebuild that kept the + ... original '?' as well as the appended one renames it to + ... '?From', which silently drops the envelope sender instead. + ${data} = Get Binary File ${MESSAGE} + # From is deliberately *not* sent as a header: the query argument is the only + # source of it, so a mangled name cannot be masked by a surviving header + ${headers} = Create Dictionary Queue-Id=${TEST NAME} + ... Rcpt=qsurvive-rcpt@example.net + @{result} = HTTP Status And Reason POST ${RSPAMD_LOCAL_ADDR} ${RSPAMD_PORT_PROXY} + ... /checkv2?From=qsurvive@example.net&Shm=${SHM_NAME} ${data} ${headers} + Should Be Equal As Integers ${result}[0] 200 + ${json} = Evaluate __import__('json').loads($result[2]) + Set Test Variable ${SCAN_RESULT} ${json} + Expect Symbol SIMPLE_TEST + Expect Symbol With Exact Options REQUEST_ARG_PROBE From=qsurvive@example.net + # And the privileged one really was stripped, so this is not a case of the + # whole query having been forwarded untouched + Expect Symbol With Exact Options FILE_SHM_PROBE none + Client Shm headers cannot override a permissive proxy's own triplet [Documentation] The other proxy worker does forward through shared memory, ... so a triplet really is generated on the upstream leg. It diff --git a/test/functional/lua/file_shm_probe.lua b/test/functional/lua/file_shm_probe.lua index 08db6bea24..c3132a2e75 100644 --- a/test/functional/lua/file_shm_probe.lua +++ b/test/functional/lua/file_shm_probe.lua @@ -7,24 +7,43 @@ -- value survive?" can be observed directly. local privileged_headers = {'File', 'Path', 'Shm', 'Shm-Offset', 'Shm-Length'} +-- Ordinary query arguments that share a URL with a stripped privileged one. +-- Removing a privileged argument means rebuilding the URL around it, and the +-- '?' delimiter is easy to duplicate while doing so, which renames the first +-- surviving argument to '?From'. Both spellings are therefore reported, so +-- that a mangled rebuild is named rather than merely missed. +local ordinary_headers = {'From', '?From'} + +local function report_headers(task, names) + local seen = {} + + for _, hname in ipairs(names) do + local hvalue = task:get_request_header(hname) + if hvalue then + seen[#seen + 1] = hname .. '=' .. tostring(hvalue) + end + end + + if #seen == 0 then + return 'none' + end + + return table.concat(seen, ';') +end + rspamd_config:register_symbol({ name = 'FILE_SHM_PROBE', score = 0.0, callback = function(task) - local seen = {} - - for _, hname in ipairs(privileged_headers) do - local hvalue = task:get_request_header(hname) - if hvalue then - seen[#seen + 1] = hname .. '=' .. tostring(hvalue) - end - end - - if #seen == 0 then - return true, 'none' - end + return true, report_headers(task, privileged_headers) + end +}) - return true, table.concat(seen, ';') +rspamd_config:register_symbol({ + name = 'REQUEST_ARG_PROBE', + score = 0.0, + callback = function(task) + return true, report_headers(task, ordinary_headers) end }) diff --git a/test/rspamd_cxx_unit_task_input.hxx b/test/rspamd_cxx_unit_task_input.hxx index 33e506b6aa..22f2359579 100644 --- a/test/rspamd_cxx_unit_task_input.hxx +++ b/test/rspamd_cxx_unit_task_input.hxx @@ -31,6 +31,10 @@ * - the payload handed to the caller is a private snapshot, therefore the * client can resize the backing object afterwards without the parser * ever seeing memory that can fault; + * - and, the sharp edge of that one, the snapshot is never *copied out of a + * mapping*: a client that truncates the object in the window between the + * validating fstat and the copy would otherwise raise SIGBUS inside the + * worker; * - the name is sanitised before any syscall touches it and an overlong * name is refused rather than silently truncated; * - `Filename` is *not* a privileged control, whereas `File`, `Path`, @@ -59,10 +63,13 @@ #include #include +#include +#include #include #include #include #include +#include #include namespace rspamd_task_input_test { @@ -201,6 +208,20 @@ public: return true; } + /* + * A second, writable descriptor for the very same object. A concurrent + * resizer needs one of its own, so that it never touches this fixture's + * bookkeeping from another thread. + */ + int open_writable() const + { +#ifdef HAVE_SANE_SHMEM + return shm_open(obj_name.c_str(), O_RDWR, 0); +#else + return open(obj_name.c_str(), O_RDWR); +#endif + } + /* Rewrites the whole object with the canonical pattern */ bool fill_pattern() { @@ -260,6 +281,74 @@ private: bool ok = false; }; +/* + * Resizes a backing object from a second thread for as long as it is alive, + * which is how a client that owns the object behaves while the worker is + * reading it. + * + * The thread is bounded twice over -- by the stop flag and by a wall clock + * deadline -- and it is always joined from the destructor, so a failing + * assertion in the test body can neither leave it running nor hang the suite. + */ +class background_truncator { +public: + background_truncator(const backing_object &obj, gsize big, gsize small) + { + fd = obj.open_writable(); + + if (fd == -1) { + return; + } + + thr = std::thread([this, big, small]() { + auto deadline = std::chrono::steady_clock::now() + + std::chrono::seconds(10); + + while (!stop.load(std::memory_order_relaxed) && + std::chrono::steady_clock::now() < deadline) { + if (ftruncate(fd, (off_t) small) == -1 || + ftruncate(fd, (off_t) big) == -1) { + break; + } + + toggles.fetch_add(1, std::memory_order_relaxed); + } + }); + } + + background_truncator(const background_truncator &) = delete; + background_truncator &operator=(const background_truncator &) = delete; + + ~background_truncator() + { + stop.store(true, std::memory_order_relaxed); + + if (thr.joinable()) { + thr.join(); + } + + if (fd != -1) { + close(fd); + } + } + + bool valid() const + { + return fd != -1; + } + + unsigned long toggle_count() const + { + return toggles.load(std::memory_order_relaxed); + } + +private: + std::thread thr; + std::atomic stop{false}; + std::atomic toggles{0}; + int fd = -1; +}; + /* * Owns the pool that the snapshots are allocated from plus the last GError, * so that neither leaks when an assertion fails. @@ -883,6 +972,128 @@ TEST_SUITE("task privileged input") CHECK(segment_payload(seg) == expected); } + TEST_CASE("a concurrently truncated object is survived rather than faulted on") + { + /* + * The test above resizes the object only *after* the call has returned, + * so it never touches the interval that actually hurts: the one between + * the fstat that validates the request and the copy that fulfils it. + * A client owns the object and may truncate it exactly there, and a + * copy out of a mapping then reads pages that no longer exist, which is + * SIGBUS and a dead worker rather than a failed request. Reading the + * window instead cannot fault; it merely returns fewer bytes. + * + * Which of the two permitted outcomes a given iteration gets is up to + * the scheduler, so only the outcomes themselves are asserted: either a + * snapshot no longer than the window that was asked for, or a refusal + * that says why. Completing the loop at all is the regression test. + */ + constexpr gsize big_size = 2 * 1024 * 1024; + constexpr gsize small_size = 4096; + constexpr gsize win_len = 512 * 1024; + /* At the very end, so that shrinking really does remove its pages */ + constexpr gsize win_off = big_size - win_len; + constexpr int iterations = 600; + + backing_object obj("truncate_race", big_size); + REQUIRE(obj.valid()); + + /* + * Zero filled on purpose: ftruncate only ever zero fills, so whatever + * the two threads do, every byte that is really read back is a zero. + * A data_len that reported the requested length rather than the number + * of bytes that were actually read would therefore hand out + * uninitialised pool memory, and that is visible right here. + */ + REQUIRE(obj.fill_with('\0')); + + background_truncator truncator(obj, big_size, small_size); + REQUIRE(truncator.valid()); + + const std::string zeros(win_len, '\0'); + const auto off_str = std::to_string(win_off); + const auto len_str = std::to_string(win_len); + + int completed = 0, snapshots = 0, refusals = 0; + int short_reads = 0, partial_reads = 0; + int oversized = 0, unexplained = 0, retained = 0, dirty = 0; + int null_data = 0; + + for (int i = 0; i < iterations; i++) { + /* One pool per iteration, so that the snapshots cannot pile up */ + segment_mapper mapper; + auto *seg = mapper.map(obj.name(), off_str.c_str(), len_str.c_str()); + + completed++; + + if (seg == nullptr) { + /* Losing the race is fine as long as the refusal says why */ + refusals++; + + if (mapper.error() == nullptr) { + unexplained++; + } + + continue; + } + + snapshots++; + + if (seg->data == nullptr) { + /* Even an empty snapshot is copied from by the callers */ + null_data++; + continue; + } + + if (seg->data_len > win_len) { + oversized++; + continue; + } + + if (seg->data_len < win_len) { + short_reads++; + + if (seg->data_len > 0) { + /* The object shrank in the middle of the read itself */ + partial_reads++; + } + } + + if (seg->map != nullptr || seg->map_len != 0 || seg->fd != -1) { + retained++; + } + + /* + * Reads every byte that was reported as present, so that a + * data_len covering bytes that were never read shows up as the + * fill pattern of a fresh allocation, and one running past the + * allocation altogether is caught by the sanitiser + */ + if (memcmp(seg->data, zeros.data(), seg->data_len) != 0) { + dirty++; + } + } + + INFO("snapshots: " << snapshots << ", refusals: " << refusals + << ", short reads: " << short_reads + << " (" << partial_reads << " partial)" + << ", toggles: " << truncator.toggle_count()); + + /* Getting this far at all is the point: the old reader took SIGBUS */ + CHECK(completed == iterations); + CHECK(snapshots + refusals == iterations); + /* Otherwise nothing was ever raced and the loop proves nothing */ + CHECK(snapshots > 0); + CHECK(null_data == 0); + /* Never more than was asked for, whatever the object did meanwhile */ + CHECK(oversized == 0); + CHECK(unexplained == 0); + /* The read path keeps neither a mapping nor a descriptor */ + CHECK(retained == 0); + /* data_len bounds real content, not the tail of a fresh allocation */ + CHECK(dirty == 0); + } + TEST_CASE("rspamd_task_allow_file_shm_input") { CHECK(rspamd_task_allow_file_shm_input(nullptr) == FALSE); -- 2.47.3