[Fix] fuzzy: check admission before parsing UDP commands

Every datagram used to be parsed, and decrypted when encrypted, before
any admission check ran: the blocklist and the rate limit were only
consulted from rspamd_fuzzy_process_command, well after a session
allocation, an ECDH, a MAC verification and the Lua pre handlers.

- Check the blocklist in the UDP read loop, before allocating a
  session. It only needs the source address, so a blocked peer now
  costs one radix lookup. Nothing is sent back, since building a reply
  would require the parse being avoided; accept_tcp_socket already
  drops blocked peers without replying, so the two transports now
  behave alike.
- Log a failed key lookup or MAC check at debug level rather than
  error. Both are reachable by anyone who can send us a datagram,
  before any rate limit applies, so an error level line per packet
  turned a spoofed source flood into a log volume attack.
- Extend the per source rate limit from FUZZY_CHECK to PING and STAT,
  which are answered unauthenticated and were previously unmetered. A
  rate limited PING or STAT is dropped rather than answered with 403:
  the reply is the entire cost of those commands, so replying anyway
  would leave egress unchanged. This stays inert unless ratelimit_rate
  and ratelimit_burst are set, and the existing whitelist and local
  address exemptions still apply.
- Skip the ratelimit bucket allocation when rate or burst are unset.
  The NaN short circuit lives in the callee, which is only reached
  once a bucket exists, so every new masked source still allocated an
  LRU entry that could never limit anything.

Report blocked_requests, decrypt_errors and ratelimited_requests in
fuzzystat so the drops that are now silent stay observable.

Also document why errors_ips must remain telemetry: it is incremented
when parsing failed, which takes no key and no handshake, so on UDP the
recorded address is forgeable and banning on it would let anyone
silence an arbitrary third party.

diff --git a/src/fuzzy_storage.c b/src/fuzzy_storage.c
index a7343417813a3a11e4428b625291bd5bbd1381a6..9424cc695aa348ae089e5cd54777f6aced8c6687 100644 (file)
--- a/src/fuzzy_storage.c
+++ b/src/fuzzy_storage.c
@@ -1376,12 +1376,21 @@ rspamd_fuzzy_process_command(struct fuzzy_session *session)
                return;
        }
 
-       int block_code = rspamd_fuzzy_check_client(session->ctx, session->addr);
-       if (block_code > 0) {
-               result.v1.value = block_code;
-               result.v1.prob = 0.0f;
-               rspamd_fuzzy_make_reply(cmd, &result, NULL, session, send_flags);
-               return;
+       /*
+        * UDP sessions were already screened before parsing, so re-checking here
+        * would just repeat the radix lookups on every accepted datagram. TCP
+        * command sessions still need it: the connection is only screened at
+        * accept time, and a dynamic block can land while it is open.
+        */
+       if (!session->client_checked) {
+               int block_code = rspamd_fuzzy_check_client(session->ctx, session->addr);
+               if (block_code > 0) {
+                       session->ctx->stat.blocked_requests++;
+                       result.v1.value = block_code;
+                       result.v1.prob = 0.0f;
+                       rspamd_fuzzy_make_reply(cmd, &result, NULL, session, send_flags);
+                       return;
+               }
        }
 
        if (session->key && session->addr) {
@@ -1431,20 +1440,33 @@ rspamd_fuzzy_process_command(struct fuzzy_session *session)
                session->ip_stat = ip_stat;
        }
 
-       if (cmd->cmd == FUZZY_CHECK) {
-               bool is_rate_allowed = true;
+       /*
+        * The per-source bucket covers every command that produces a reply
+        * without needing write authorisation: CHECK, PING and STAT. PING and
+        * STAT are answered unauthenticated, so leaving them unmetered let a
+        * source spend our parse and reply budget for free. Writes and deletes
+        * are gated by rspamd_fuzzy_check_write instead.
+        *
+        * This is inert unless ratelimit_rate and ratelimit_burst are configured,
+        * and rspamd_fuzzy_check_ratelimit already exempts ratelimit_whitelist
+        * and local addresses, which is where monitoring probes belong.
+        */
+       bool is_rate_allowed = true;
 
-               if (session->ctx->ratelimit_buckets) {
-                       if (session->ctx->ratelimit_log_only) {
-                               (void) rspamd_fuzzy_check_ratelimit(session->ctx, session->addr,
-                                                                                                       session->worker, session->timestamp); /* Check but ignore */
-                       }
-                       else {
-                               is_rate_allowed = rspamd_fuzzy_check_ratelimit(session->ctx, session->addr,
-                                                                                                                          session->worker, session->timestamp);
-                       }
+       if (session->ctx->ratelimit_buckets &&
+               (cmd->cmd == FUZZY_CHECK || cmd->cmd == FUZZY_PING ||
+                cmd->cmd == FUZZY_STAT)) {
+               if (session->ctx->ratelimit_log_only) {
+                       (void) rspamd_fuzzy_check_ratelimit(session->ctx, session->addr,
+                                                                                               session->worker, session->timestamp); /* Check but ignore */
                }
+               else {
+                       is_rate_allowed = rspamd_fuzzy_check_ratelimit(session->ctx, session->addr,
+                                                                                                                  session->worker, session->timestamp);
+               }
+       }
 
+       if (cmd->cmd == FUZZY_CHECK) {
                if (session->key && session->key->rl_bucket) {
                        /* Check per-key bucket */
 
@@ -1560,17 +1582,33 @@ rspamd_fuzzy_process_command(struct fuzzy_session *session)
                        rspamd_fuzzy_make_reply(cmd, &result, NULL, session, send_flags);
                }
        }
-       else if (cmd->cmd == FUZZY_STAT) {
-               /* Store approximation (if needed) */
-               result.v1.prob = session->ctx->stat.fuzzy_hashes;
-               /* Store high qword in value and low qword in flag */
-               result.v1.value = (int32_t) ((uint64_t) session->ctx->stat.fuzzy_hashes >> 32);
-               result.v1.flag = (uint32_t) (session->ctx->stat.fuzzy_hashes & G_MAXUINT32);
-               rspamd_fuzzy_make_reply(cmd, &result, NULL, session, send_flags);
-       }
-       else if (cmd->cmd == FUZZY_PING) {
-               result.v1.prob = 1.0f;
-               result.v1.value = cmd->value;
+       else if (cmd->cmd == FUZZY_STAT || cmd->cmd == FUZZY_PING) {
+               /*
+                * Unlike CHECK, a rate limited PING or STAT is dropped rather than
+                * answered with 403: the reply *is* the entire cost of these
+                * commands, so replying anyway would leave egress unchanged and
+                * defeat the limit. This matches how a blocklisted source is handled.
+                */
+               if (!is_rate_allowed) {
+                       session->ctx->stat.ratelimited_requests++;
+                       msg_debug("dropping ratelimited %s from %s",
+                                         cmd->cmd == FUZZY_PING ? "ping" : "stat",
+                                         rspamd_inet_address_to_string(session->addr));
+                       return;
+               }
+
+               if (cmd->cmd == FUZZY_STAT) {
+                       /* Store approximation (if needed) */
+                       result.v1.prob = session->ctx->stat.fuzzy_hashes;
+                       /* Store high qword in value and low qword in flag */
+                       result.v1.value = (int32_t) ((uint64_t) session->ctx->stat.fuzzy_hashes >> 32);
+                       result.v1.flag = (uint32_t) (session->ctx->stat.fuzzy_hashes & G_MAXUINT32);
+               }
+               else {
+                       result.v1.prob = 1.0f;
+                       result.v1.value = cmd->value;
+               }
+
                rspamd_fuzzy_make_reply(cmd, &result, NULL, session, send_flags);
        }
        else {
@@ -1745,8 +1783,15 @@ rspamd_fuzzy_decrypt_command(struct fuzzy_session *s, unsigned char *buf, gsize
        rk = rspamd_pubkey_from_bin(hdr.pubkey, sizeof(hdr.pubkey), RSPAMD_KEYPAIR_KEX);
 
        if (rk == NULL) {
-               msg_err("bad key; ip=%s",
-                               rspamd_inet_address_to_string(s->addr));
+               /*
+                * Debug level on purpose: this is reachable by any host that can
+                * send us a datagram, before any rate limit applies, so logging it
+                * at error level turns a spoofed-source flood into one log line per
+                * packet. The decrypt_errors counter carries the signal instead.
+                */
+               s->ctx->stat.decrypt_errors++;
+               msg_debug("bad key; ip=%s",
+                                 rspamd_inet_address_to_string(s->addr));
                return FALSE;
        }
 
@@ -1757,8 +1802,10 @@ rspamd_fuzzy_decrypt_command(struct fuzzy_session *s, unsigned char *buf, gsize
        if (!rspamd_cryptobox_decrypt_nm_inplace(buf, buflen, hdr.nonce,
                                                                                         rspamd_pubkey_get_nm(rk, key->key),
                                                                                         hdr.mac)) {
-               msg_err("decryption failed; ip=%s",
-                               rspamd_inet_address_to_string(s->addr));
+               /* Debug level for the same reason as the bad key case above */
+               s->ctx->stat.decrypt_errors++;
+               msg_debug("decryption failed; ip=%s",
+                                 rspamd_inet_address_to_string(s->addr));
                rspamd_pubkey_unref(rk);
 
                return FALSE;
@@ -2185,6 +2232,27 @@ accept_fuzzy_socket(EV_P_ ev_io *w, int revents)
                                        client_addr = NULL;
                                }
 
+                               /*
+                                * Drop blocklisted sources before doing any work on the
+                                * datagram. This only needs the source address, so a blocked
+                                * peer costs one radix lookup instead of a session
+                                * allocation, command parsing, an ECDH plus MAC verification
+                                * for encrypted commands, and the Lua pre-handlers.
+                                *
+                                * Nothing is sent back: building a reply would require
+                                * parsing the command first, which is exactly the work being
+                                * avoided. The TCP path already drops blocked peers without
+                                * a reply (see accept_tcp_socket), so this makes the two
+                                * transports behave alike.
+                                */
+                               if (client_addr && rspamd_fuzzy_check_client(ctx, client_addr) > 0) {
+                                       ctx->stat.blocked_requests++;
+                                       msg_debug("dropping fuzzy command from blocked address %s",
+                                                         rspamd_inet_address_to_string(client_addr));
+                                       rspamd_inet_address_free(client_addr);
+                                       continue;
+                               }
+
                                session = g_malloc0(sizeof(*session));
                                REF_INIT_RETAIN(session, fuzzy_session_destroy);
                                session->worker = worker;
@@ -2192,6 +2260,7 @@ accept_fuzzy_socket(EV_P_ ev_io *w, int revents)
                                session->ctx = ctx;
                                session->timestamp = ev_now(ctx->event_loop);
                                session->addr = client_addr;
+                               session->client_checked = true;
                                worker->nconns++;
 
                                /* Each message can have its length in case of recvmmsg */
@@ -2209,6 +2278,18 @@ accept_fuzzy_socket(EV_P_ ev_io *w, int revents)
                                        session->ctx->stat.invalid_requests++;
                                        msg_debug("invalid fuzzy command of size %z received", r);
 
+                                       /*
+                                        * errors_ips is telemetry only: it is reported by
+                                        * fuzzystat and deliberately never feeds a blocking
+                                        * decision. It is incremented exactly when parsing
+                                        * failed, which takes no key and no handshake, so on UDP
+                                        * the source address here is trivially forgeable. Banning
+                                        * on it would let anyone silence an arbitrary third party
+                                        * by sending a handful of malformed datagrams carrying
+                                        * that victim's address. Operators who know their network
+                                        * is spoof resistant can implement their own policy with
+                                        * worker:block_fuzzy_client().
+                                        */
                                        if (session->addr) {
                                                nerrors = rspamd_lru_hash_lookup(session->ctx->errors_ips,
                                                                                                                 session->addr, -1);

diff --git a/src/libserver/fuzzy_storage_internal.h b/src/libserver/fuzzy_storage_internal.h
index 3353d2183808bb2fbe882459b406679aa09b9e67..161c438cb9c39a801c17f1fd77680935d86f46e1 100644 (file)
--- a/src/libserver/fuzzy_storage_internal.h
+++ b/src/libserver/fuzzy_storage_internal.h
@@ -56,6 +56,17 @@ struct fuzzy_global_stat {
        uint64_t fuzzy_hashes_found[RSPAMD_FUZZY_EPOCH_MAX];
        uint64_t invalid_requests;
        uint64_t delayed_hashes;
+       /*
+        * Requests dropped before parsing because the source is blocklisted,
+        * and requests whose encrypted header failed key lookup or MAC check.
+        * Both are logged at debug level only (a spoofed-source flood would
+        * otherwise turn into one error line per datagram), so these counters
+        * are the sole operational signal that either is happening.
+        */
+       uint64_t blocked_requests;
+       uint64_t decrypt_errors;
+       /* PING/STAT dropped by the per-source rate limit rather than answered */
+       uint64_t ratelimited_requests;
 };
 
 struct fuzzy_key_stat {
@@ -241,6 +252,15 @@ struct fuzzy_session {
        struct rspamd_fuzzy_cmd_extension *extensions;
        unsigned char nm[rspamd_cryptobox_MAX_NMBYTES];
 
+       /*
+        * Set when the source address was already run through
+        * rspamd_fuzzy_check_client before the session was created (the UDP
+        * path checks it up front, prior to parsing). Lets the per-command
+        * check be skipped for that path without losing it for TCP, where a
+        * dynamic block can land after the connection was accepted.
+        */
+       bool client_checked;
+
        /* If this is a TCP session, this pointer will be set */
        struct fuzzy_tcp_session *tcp_session;
 };

diff --git a/src/libserver/fuzzy_storage_ratelimit.c b/src/libserver/fuzzy_storage_ratelimit.c
index 511f0b08564806706dec8b076dcc9bcdbde86ca8..b55b467076e4c29cd0d537211564f00d01ecc323 100644 (file)
--- a/src/libserver/fuzzy_storage_ratelimit.c
+++ b/src/libserver/fuzzy_storage_ratelimit.c
@@ -118,6 +118,17 @@ rspamd_fuzzy_check_ratelimit(struct rspamd_fuzzy_storage_ctx *ctx,
                return TRUE;
        }
 
+       /*
+        * Rate limiting is disabled unless both rate and burst are configured.
+        * rspamd_fuzzy_check_ratelimit_bucket short circuits on NaN, but it is
+        * only reached when a bucket already exists, so without this guard every
+        * new masked source would still allocate and insert an LRU entry that can
+        * never limit anything.
+        */
+       if (isnan(ctx->leaky_bucket_burst) || isnan(ctx->leaky_bucket_rate)) {
+               return TRUE;
+       }
+
        if (ctx->ratelimit_whitelist != NULL) {
                if (rspamd_match_radix_map_addr(ctx->ratelimit_whitelist,
                                                                                addr) != NULL) {

diff --git a/src/libserver/fuzzy_storage_stat.c b/src/libserver/fuzzy_storage_stat.c
index e169844a236fe7036a37cd28e84a6cd6b161aa15..0cf06e1ec1d56b5a3507b654b5f8649f3d715193 100644 (file)
--- a/src/libserver/fuzzy_storage_stat.c
+++ b/src/libserver/fuzzy_storage_stat.c
@@ -181,6 +181,21 @@ rspamd_fuzzy_stat_to_ucl(struct rspamd_fuzzy_storage_ctx *ctx, gboolean ip_stat)
                                                  "delayed_hashes",
                                                  0,
                                                  false);
+       ucl_object_insert_key(obj,
+                                                 ucl_object_fromint(ctx->stat.blocked_requests),
+                                                 "blocked_requests",
+                                                 0,
+                                                 false);
+       ucl_object_insert_key(obj,
+                                                 ucl_object_fromint(ctx->stat.decrypt_errors),
+                                                 "decrypt_errors",
+                                                 0,
+                                                 false);
+       ucl_object_insert_key(obj,
+                                                 ucl_object_fromint(ctx->stat.ratelimited_requests),
+                                                 "ratelimited_requests",
+                                                 0,
+                                                 false);
 
        if (ctx->errors_ips && ip_stat) {
                gpointer k, v;