Conversation
drain_cursor_pages accumulated pages from an untrusted representative until the node returned an empty page or a None cursor - both server-controlled. With no cursor-progress check and no ceiling, a malicious node that always returns a non-empty page plus Some(next_key) (e.g. by replaying one page or echoing a constant cursor) could drive unbounded memory growth or a non-terminating loop in the client. Reached from the public chain_all/history_all/global_history_all. Require the cursor to strictly advance (tighten CURSOR to Copy + PartialEq) and add a MAX_DRAINED_ITEMS safety ceiling; both return the new ClientError::PaginationLimitExceeded instead of looping/allocating without bound. Co-authored-by: Ty Schenk <schenkty@users.noreply.github.com>
larseidsvoll
left a comment
larseidsvoll
left a comment
There was a problem hiding this comment.
Unbounded drain closed: cursor must advance + hard item ceiling + dedicated error. Nits: ceiling unit test; fix Some(_) comment. Approve for the security fix. Draft + Ty lock — no merge.
larseidsvoll
left a comment
larseidsvoll
left a comment
There was a problem hiding this comment.
Verdict: APPROVE (re-stamp after #45)
Tip moved for main merge. Diff unchanged: cursor must advance + MAX_DRAINED_ITEMS + PaginationLimitExceeded.
Security Audit green. Lint green; Tests still running.
Review only — do not bot-merge. Humans merge. No @ humans.
|
No security issues found. Tip |
|
Thanks — nits noted from the APPROVE:
Will land those on this branch shortly. |
Co-authored-by: Ty Schenk <schenkty@users.noreply.github.com>
|
3 participants
Summary
drain_cursor_pages(used by the publicchain_all,history_all, andglobal_history_all) accumulates pages from a remote representative and only terminates when the node returns an empty page or aNonecursor — both entirely server-controlled. It appends server-controlled items each iteration with no cursor-progress check and no aggregate ceiling (the generic bound was onlyCURSOR: Copy).A malicious, compromised, or MITM representative can therefore return a non-empty page plus
Some(next_key)forever — e.g. by replaying one page under a constant cursor — driving unbounded memory growth or a non-terminating call in the client (DoS). Forchain_allthe appended blocks are only decoded (not quorum-verified), so replay is essentially free.Reproduced with a standalone model of the loop: a mock
fetchreturning one item and a constant cursor appended 1,000,000 items without self-terminating.Fix
CURSOR: Copy + PartialEq); a repeated cursor now returns an error instead of looping.MAX_DRAINED_ITEMSsafety ceiling as a backstop against a node that fabricates advancing cursors forever.ClientError::PaginationLimitExceeded(codePAGINATION_LIMIT_EXCEEDED).Both cursor types in use (
BlockHash,VoteBlockHash) already deriveCopy + PartialEq, so no caller signature changes.Note:
MAX_DRAINED_ITEMSis a conservative constant (1,000,000). If a legitimate account/global history can exceed it, consider making it configurable (viaClientConfig) or having callers page explicitly through the existing*_page_cursorAPIs.Tests
cargo test -p keetanetwork-client, including:drain_cursor_pages_rejects_a_non_advancing_cursor— a constant-cursor server yieldsPaginationLimitExceeded.drain_cursor_pages_rejects_when_advancing_pages_exceed_the_item_ceiling— a server that keeps advancing cursors still tripsMAX_DRAINED_ITEMS.The
Some(_)arm comment now describes a non-advancing cursor (not end of sequence).Severity: medium (client memory exhaustion / non-terminating call).
Draft — audit fix; do not merge without maintainer review of the ceiling/config choice.