Fix two DoS vectors in sc-hop rate limiter#11988
Conversation
In order to support promotion to Bulletin in HOP, we need both node-side and runtime-side changes. This PR adds the node-side changes to sc-hop and ultimately to polkadot-omni-node, since that's what will be used to run the Bulletin chain. The runtime-side is tested in [polkadot-bulletin-chain](paritytech/polkadot-bulletin-chain#348) before the real deploy to fellowship runtimes. ## How promotion works When entries are about to expire, the collator will submit low-priority transactions to store them in Bulletin. These transactions are not propagated, they'll stay in the collators transaction pool until they expire or it's his turn to build a block and he can fit them in. Given that they are low-priority, all other external transactions take precedence. That's why promotion is a best-effort system. It only tries to fill in blocks if they have some space available. A background HopMaintenanceTask runs on a configurable interval (default 60s). Each cycle it: 1. Queries the pool for entries within hop_buffer_blocks (default ~2h) of their expiry that haven't been promoted yet (get_promotable) 2. For each, calls HopPromoter::promote(data) which uses the HopPromotionApi runtime API to construct a general transaction extrinsic and submits it to the local tx pool 3. Marks successfully promoted entries 4. Cleans up expired entries Promoted entries don't get immediately deleted, they wait for their actual expiry. This is so that the recipient can quickly get them from HOP if he comes online before the expiry time. If he comes online after the expiry time, then it will try to get it from HOP, fail and then go try to retrieve it from Bulletin via IPFS. ## Runtime API detection try_build_promoter() probes for HopPromotionApi support at startup. If the runtime doesn't implement it, the node logs a warning and runs in cleanup-only mode. The pool still works for submit/claim, just without automatic on-chain promotion. This keeps the node functional regardless of whether the runtime has pallet-hop-promotion. We don't want the omni node to crash if the runtime is not Bulletin, clearly. Ideally we want all of this to be enabled only when the node is run with --enable-hop. ## Companion: bulletin-chain runtime The companion to this PR is on bulletin-chain. It has pallet-hop-promotion which implements the runtime API. This API gives the node the necessary promote() extrinsic to submit. To know more, go to that PR. We'll have to deploy that pallet to Polkadot to make use of HOP promotion.
Users can only publish to HOP if they have an active Bulletin subscription. In order to communicate this to the node, I added a runtime API that returns the active authorization for an AccountId. This runtime API could just return a bool, but returning more information is useful to give users a way of knowing how much they have left.
Suggested by @skunert : "If we introduce faster block times, why should HOP be affected?" This changes the retention period to use wall time (std::SystemTime). --------- Co-authored-by: Branislav Kontur <bkontur@gmail.com>
Commit bbf63e2 ("Moved retention to wall time") renamed the `HopParams` field but missed the two omni-node call sites (`spec.rs`, `aura.rs`) and the integration example in the sc-hop README, breaking the `polkadot-omni-node-lib` build.
Removed comments about runtime API requirements and stub implementations.
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
| data_dir: PathBuf, | ||
| rate_limit_cfg: RateLimitConfig, | ||
| ) -> Result<Self, HopError> { | ||
| if rate_limit_cfg.enabled && rate_limit_cfg.bandwidth_burst < MAX_DATA_SIZE { |
There was a problem hiding this comment.
This check only covers the per-sender bandwidth_burst but the new global bucket has the same problem and no check.
--hop-global-bandwidth-burst-mib can be set as low as 1 MiB, but MAX_DATA_SIZE is 8 MiB. With a global burst smaller than the data size, a large submission will never fit in the global bucket, so it gets rejected every time. The error tells the caller to retry later, but retrying can never succeed.
if rate_limit_cfg.enabled && rate_limit_cfg.global_bandwidth_burst < MAX_DATA_SIZE {
return Err(HopError::InvalidConfig(...));
}
There was a problem hiding this comment.
Related detail that affects both this check and the global one suggested above: the limiter isn't charged data.len(), it's charged entry_accounted_size(data_len, recipients.len()), which adds 40 bytes per recipient, up to 10,240 extra bytes at 256 recipients.
So bandwidth_burst = MAX_DATA_SIZE passes this check, but a max-size blob with a full recipient list is charged MAX_DATA_SIZE + 10,240 and never fits in the bucket. It gets rejected every time, with a retry hint that never helps. That's exactly the config test_rate_limit_rejects_burst_overflow now uses.
So the minimum for both checks should be the real worst case:
entry_accounted_size(MAX_DATA_SIZE, MAX_RECIPIENTS as usize)
5 participants
Fixes two DoS vectors in the HOP rate limiter identified by automated security triage.
No global bandwidth cap (Medium, CWE-799): The per-account token buckets had no cross-account aggregate limit. N coordinated accounts could submit at N× the per-account rate — with defaults, 40 accounts fill the 10 GiB pool in under two minutes. A global Mutex is added to RateLimiter and checked after the per-sender phase; tokens are refunded if the global bucket rejects. Two new CLI flags: --hop-global-bandwidth-per-min-mib (default 1024) and --hop-global-bandwidth-burst-mib (default 2048).
No bounded sender map (Low, CWE-770): RateLimiter::users grew without bound between hourly eviction passes — ~200 bytes/entry × many authorized accounts. RateLimitConfig gains max_tracked_senders (default 100 000, ~20 MiB ceiling); when the cap is hit get_or_create evicts the least-recently-used entry rather than rejecting the newcomer, preventing map pre-filling attacks. New CLI flag: --hop-max-rate-limit-senders.
Seven new unit tests cover both fixes.