← Community
bugfixed

Per-command daily rate limit: check and consume are separate lock-free steps, so concurrent executes cross the cap (CWE-362)

marcofgvmarcofgv#218d ago · 39 views
affected: station-v1.3.1fixed in: station-v1.4.0

studio_server.py enforces a per-command daily cap — _rate_limits_config ships stripe.create_refund at 5/day, twilio.send_sms and pagerduty.trigger_incident at 25/day, and so on. It's implemented as two separate functions:

  • _rate_limit_check(cmd_id) reads today's counter and returns current < per_day. It's a read only — it takes no reservation.
  • _rate_limit_consume(cmd_id) is a read-modify-write: jload → day[cmd_id] += 1 → jsave.

On the execute path (routes/commands.py::execute_command) the check runs near the top and the consume runs later, after the approval is spent. Neither is under a lock. The consume's own docstring calls it an "Atomic-ish increment" — which is itself an acknowledgement that it is not atomic.

Because the check is a plain read with no reservation, two executes that arrive while the counter sits at per_day - 1 both read the same value, both see current < per_day, and both proceed. At the cap the check does block — it returns blocked_by_policy and doesn't consume the approval — so the limit is enforced as a hard ceiling, not an advisory one; the only gap is that the check isn't serialized against the consume. Under overlap the ceiling admits one more than it should, and the lost update on the consume can also leave the recorded count lower than the number that actually fired.

Reproduction, driving the two real functions directly:

  • Serial control — with per_day = 5, a serial run fires exactly 5 and then blocks. The cap holds cleanly when nothing overlaps, so this isn't an off-by-one in the counter itself.
  • Concurrent — fire several executes against a counter at the boundary; more than the cap pass the check. The over-admission is specific to the overlap.

The realistic trigger is modest: a 2-way overlap at the boundary — two /api/commands/execute for the same rate-limited command, each carrying its own valid approval, landing together on the threaded server. This is not a large-multiplier remote bypass; it's a boundary race that lets the daily ceiling be crossed by one or more. Impact is confined to whatever the cap protects: an extra refund, an extra page or SMS beyond the configured daily number.

For context, this release already serialized the two sibling instances of the same pattern — the single-use approval consume in routes/commands.py is now wrapped in a consume lock, and the replay check in team_mesh.verify_envelope is now under a seen-lock. The per-command rate counter is the same check-then-write shape and wasn't included. A lock spanning check + consume, or an atomic counter (or a reservation on the way in), would bring it in line with the paths that were already closed.

5 pts

1 reply

Fixed in station-v1.4.0. The check and the increment are now one atomic reserve under a lock (_rate_limit_reserve); the old _rate_limit_check + later _rate_limit_consume were a lock-free check-then-act, so two concurrent executes read the same stale count and both crossed the cap (CWE-362). A slot reserved but then refused before execution (team gate pending/blocked, approval already consumed) is handed back with _rate_limit_release, so only real execution attempts count. Proven with a two-thread differential test.

Thanks for the report — credited.

Sign in to reply.