PST-005: Rate limits longer than five minutes collapse to five minutes on the default database cache #734

New issue

Closed

opened 2026-10-10 10:55:45 +00:00 by tiagoagueda · 0 comments

tiagoagueda commented

2026-10-10 10:55:45 +00:00

Owner

Found in the security audit of 2026-10-10 (SECURITY-AUDIT-2026-10-10.md), run against main at c30851b from 8 to 10 October 2026, as PST-005. Severity: Medium.

The text below is the audit's entry, unchanged apart from links to the other issues it names. The finding was produced by automated review and checked by a second automated pass; re-check it before acting on it.

Severity: Medium
Category: rate-limiting / weak control
Location: src/postulo/core/throttle.py (112-136)
Found by: completeness review: chains
Related: PST-006 (#735), PST-026 (#755), PST-027 (#756), PST-029 (#758)

Who can do it, and what they need. Any signed-in member (or, for the /logs and /metrics wrong-token bar, any stranger) on a stock install. The stock install uses the database cache (POSTULO_CACHE_URL unset), whose incr() is Django's generic get-then-set, not an atomic increment. The attacker only needs to pause between bursts.

How it works.

consume() creates the counter with cache.add(key, 1, timeout=rate.seconds), so a '30/h' counter gets a 3600 s expiry.
Every later request goes through cache.incr(key). DatabaseCache has no native incr, so BaseCache.incr runs self.set(key, new_value) with the DEFAULT timeout, 300 s. The expiry is now 300 s after the last use, not the end of the window.
The attacker spends the allowance, waits just over 5 minutes, and the counter has expired. They get a full allowance again, about 12 times the configured hourly rate.
Affected limits include: number-collision 20/h (the #142 telephone-number existence oracle, becoming about 240/h), capture 30/h (outbound fetches), fetch 300/h, api 600/h per token, outbox 60/h, recovery-link 10/h per administrator, connection-test 10/h, confirmation 5/h, text 5/h per account and 3/h per number, and the REFUSED 10/h wrong-token bar on /logs and /metrics. That bar lifts about 5 minutes after the last counted guess, not after an hour.
Verified: with a real DatabaseCache, the stored expiry after the first consume was 3599 s away and after the second only 299 s away.

Impact. Every Postulo-owned limit longer than 5 minutes is really 'N per 5 minutes of idleness'. This weakens the anti-enumeration bound on telephone numbers, the outbound-fetch bound, the recovery-link mint limit, the per-number SMS bound (a stranger's phone can be texted about 12 times more often than documented) and the wrong-token lockout. The tests cannot see it: throttle._now() moves only the module's clock, and LocMem and Redis caches increment natively, so only the default database cache is affected.

Evidence.

throttle.py:125 `if cache.add(key, 1, timeout=rate.seconds):` / :129 `used = cache.incr(key)`. django base.py:284 `self.set(key, new_value, version=version)` (no timeout argument). Scratch test output: `seconds left after first: [3599]`, `seconds left after second: [299]`.

Verification (verdict: confirmed; severity after verification: Medium).

The code path is unambiguous and the scratch test reproduces it. throttle.consume() creates the counter with cache.add(key, 1, timeout=rate.seconds) (throttle.py:125). Every later request in the window goes through cache.incr(key) (throttle.py:129). The stock install uses DatabaseCache, because POSTULO_CACHE_URL is unset and base.py:189-191 defaults to dbcache://postulo_cache. Neither docker/compose*.yml nor .env.example sets a cache. DatabaseCache does not override incr, so it inherits BaseCache.incr (Django 6.1.1). That does get, then self.set(key, new_value, version=version) with no timeout, so the row's expiry is reset to the 300 s default. The counter therefore lives only 5 minutes after its last use, not until the end of the window. Because the key is a fixed-window key and expires on its own, the next consume() in the same hour takes the cache.add branch and starts a fresh allowance. I looked for a mitigation and found none: there is no TIMEOUT override in CACHES, nothing touches the key, and the 'barred' and 'spent' readers only call cache.get. A client that keeps hammering while refused keeps extending the TTL and stays blocked, so the attacker has to pause, but pausing is trivial for a script. Redis, Memcached and LocMem increment natively and keep the TTL. FileBasedCache would share the bug. One correction to the report: the test settings (settings/test.py:61-65) also use DatabaseCache, so the suite does not miss the bug because of the cache backend. It misses it because the tests only move throttle._now() and never move the cache's own clock. The effect is a roughly 12x weakening of every limit longer than 5 minutes, not removal of the limit. The anti-enumeration bound on phone numbers is weakened (20/h becomes 240/h), as are the outbound-fetch bounds, per-number and per-account SMS limits, the recovery-link and connection-test limits, and the 10/h wrong-token bar on /logs and /metrics. The /metrics bar impact is negligible if the shared token has high entropy. Severity stays at medium, not high, because the limits still bind a burst, the SSRF guards still apply to every fetch, and the attacker must be signed in for everything except the /logs and /metrics token bar. The project's own docstring (throttle.py:20-24) discusses the non-atomic incr but not the expiry reset.

Corrections from verification. File and lines are correct: src/postulo/core/throttle.py:112-136, add at 125, incr at 129, ValueError fallback at 131. The test-suite explanation should read that the tests move only throttle._now() and not the cache's clock, since the test settings also use DatabaseCache. The caveat that the effect is about 12x, not removal, should be recorded. A suggested fix is to avoid BaseCache.incr on non-native caches by doing get then set with timeout equal to the remaining seconds of the window, or to follow the incr with cache.touch(key, remaining).

Verification evidence.

Code: throttle.py:125 `if cache.add(key, 1, timeout=rate.seconds):` and :129 `used = cache.incr(key)`. Django 6.1.1 BaseCache.incr: `value = self.get(key, self._missing_key, version=version) ... new_value = value + delta; self.set(key, new_value, version=version)`; `DatabaseCache.incr.__qualname__` resolves to the inherited method. base.py:189-191 `CACHES = {"default": env.cache_url("POSTULO_CACHE_URL", default=f"dbcache://{CACHE_TABLE}")}`.

Scratch test ([scratch test]), 8 passed:
seconds left after first consume : [3599]
seconds left after second consume: [299]
db cache: 3/h spent, 301 s pause, another 3 allowed inside the same hour
control (LocMem): still refused after 301 s, as documented
/metrics wrong-token bar (10/h) lifted after 301 s on the db cache

In the behavioural tests the cache clock and throttle._now were moved together with a 301 s offset inside one hour window. On the DB cache, the 4th consume after the pause succeeded and 3 more were allowed. On the LocMem control, it still raised TooOften.

Suggested remediation. Do not rely on cache.incr() to preserve the TTL. Store {count, window_end} in the value and set it with timeout = remaining window time. Alternatively use cache.touch() after incr(), or use a dedicated counter table or an atomic backend. Add a test that runs against the real DatabaseCache and asserts the expiry after the second consume.

Found in the security audit of 2026-10-10 (`SECURITY-AUDIT-2026-10-10.md`), run against `main` at `c30851b` from 8 to 10 October 2026, as **PST-005**. Severity: **Medium**. The text below is the audit's entry, unchanged apart from links to the other issues it names. The finding was produced by automated review and checked by a second automated pass; re-check it before acting on it. --- - **Severity:** Medium - **Category:** rate-limiting / weak control - **Location:** `src/postulo/core/throttle.py` (112-136) - **Found by:** completeness review: chains - **Related:** PST-006 (#735), PST-026 (#755), PST-027 (#756), PST-029 (#758) **Who can do it, and what they need.** Any signed-in member (or, for the /logs and /metrics wrong-token bar, any stranger) on a stock install. The stock install uses the database cache (POSTULO_CACHE_URL unset), whose incr() is Django's generic get-then-set, not an atomic increment. The attacker only needs to pause between bursts. **How it works.** 1. consume() creates the counter with cache.add(key, 1, timeout=rate.seconds), so a '30/h' counter gets a 3600 s expiry. 2. Every later request goes through cache.incr(key). DatabaseCache has no native incr, so BaseCache.incr runs self.set(key, new_value) with the DEFAULT timeout, 300 s. The expiry is now 300 s after the last use, not the end of the window. 3. The attacker spends the allowance, waits just over 5 minutes, and the counter has expired. They get a full allowance again, about 12 times the configured hourly rate. 4. Affected limits include: number-collision 20/h (the #142 telephone-number existence oracle, becoming about 240/h), capture 30/h (outbound fetches), fetch 300/h, api 600/h per token, outbox 60/h, recovery-link 10/h per administrator, connection-test 10/h, confirmation 5/h, text 5/h per account and 3/h per number, and the REFUSED 10/h wrong-token bar on /logs and /metrics. That bar lifts about 5 minutes after the last counted guess, not after an hour. Verified: with a real DatabaseCache, the stored expiry after the first consume was 3599 s away and after the second only 299 s away. **Impact.** Every Postulo-owned limit longer than 5 minutes is really 'N per 5 minutes of idleness'. This weakens the anti-enumeration bound on telephone numbers, the outbound-fetch bound, the recovery-link mint limit, the per-number SMS bound (a stranger's phone can be texted about 12 times more often than documented) and the wrong-token lockout. The tests cannot see it: throttle._now() moves only the module's clock, and LocMem and Redis caches increment natively, so only the default database cache is affected. **Evidence.** ```text throttle.py:125 `if cache.add(key, 1, timeout=rate.seconds):` / :129 `used = cache.incr(key)`. django base.py:284 `self.set(key, new_value, version=version)` (no timeout argument). Scratch test output: `seconds left after first: [3599]`, `seconds left after second: [299]`. ``` **Verification** (verdict: confirmed; severity after verification: Medium). The code path is unambiguous and the scratch test reproduces it. throttle.consume() creates the counter with cache.add(key, 1, timeout=rate.seconds) (throttle.py:125). Every later request in the window goes through cache.incr(key) (throttle.py:129). The stock install uses DatabaseCache, because POSTULO_CACHE_URL is unset and base.py:189-191 defaults to dbcache://postulo_cache. Neither docker/compose*.yml nor .env.example sets a cache. DatabaseCache does not override incr, so it inherits BaseCache.incr (Django 6.1.1). That does get, then self.set(key, new_value, version=version) with no timeout, so the row's expiry is reset to the 300 s default. The counter therefore lives only 5 minutes after its last use, not until the end of the window. Because the key is a fixed-window key and expires on its own, the next consume() in the same hour takes the cache.add branch and starts a fresh allowance. I looked for a mitigation and found none: there is no TIMEOUT override in CACHES, nothing touches the key, and the 'barred' and 'spent' readers only call cache.get. A client that keeps hammering while refused keeps extending the TTL and stays blocked, so the attacker has to pause, but pausing is trivial for a script. Redis, Memcached and LocMem increment natively and keep the TTL. FileBasedCache would share the bug. One correction to the report: the test settings (settings/test.py:61-65) also use DatabaseCache, so the suite does not miss the bug because of the cache backend. It misses it because the tests only move throttle._now() and never move the cache's own clock. The effect is a roughly 12x weakening of every limit longer than 5 minutes, not removal of the limit. The anti-enumeration bound on phone numbers is weakened (20/h becomes 240/h), as are the outbound-fetch bounds, per-number and per-account SMS limits, the recovery-link and connection-test limits, and the 10/h wrong-token bar on /logs and /metrics. The /metrics bar impact is negligible if the shared token has high entropy. Severity stays at medium, not high, because the limits still bind a burst, the SSRF guards still apply to every fetch, and the attacker must be signed in for everything except the /logs and /metrics token bar. The project's own docstring (throttle.py:20-24) discusses the non-atomic incr but not the expiry reset. **Corrections from verification.** File and lines are correct: src/postulo/core/throttle.py:112-136, add at 125, incr at 129, ValueError fallback at 131. The test-suite explanation should read that the tests move only throttle._now() and not the cache's clock, since the test settings also use DatabaseCache. The caveat that the effect is about 12x, not removal, should be recorded. A suggested fix is to avoid BaseCache.incr on non-native caches by doing get then set with timeout equal to the remaining seconds of the window, or to follow the incr with cache.touch(key, remaining). **Verification evidence.** ```text Code: throttle.py:125 `if cache.add(key, 1, timeout=rate.seconds):` and :129 `used = cache.incr(key)`. Django 6.1.1 BaseCache.incr: `value = self.get(key, self._missing_key, version=version) ... new_value = value + delta; self.set(key, new_value, version=version)`; `DatabaseCache.incr.__qualname__` resolves to the inherited method. base.py:189-191 `CACHES = {"default": env.cache_url("POSTULO_CACHE_URL", default=f"dbcache://{CACHE_TABLE}")}`. Scratch test ([scratch test]), 8 passed: seconds left after first consume : [3599] seconds left after second consume: [299] db cache: 3/h spent, 301 s pause, another 3 allowed inside the same hour control (LocMem): still refused after 301 s, as documented /metrics wrong-token bar (10/h) lifted after 301 s on the db cache In the behavioural tests the cache clock and throttle._now were moved together with a 301 s offset inside one hour window. On the DB cache, the 4th consume after the pause succeeded and 3 more were allowed. On the LocMem control, it still raised TooOften. ``` **Suggested remediation.** Do not rely on cache.incr() to preserve the TTL. Store {count, window_end} in the value and set it with timeout = remaining window time. Alternatively use cache.touch() after incr(), or use a dedicated counter table or an atomic backend. Add a test that runs against the real DatabaseCache and asserts the expiry after the second consume.

tiagoagueda added the

bug

security

labels

2026-10-10 10:55:45 +00:00

tiagoagueda added this to the 0.5.0 milestone

2026-10-10 10:57:19 +00:00

tiagoagueda referenced this issue

2026-10-10 10:57:20 +00:00

PST-006: The candidate-file review keeps answering whether a telephone number is held by another account after the allowance is spent #735

tiagoagueda referenced this issue

2026-10-10 10:57:34 +00:00