Skip to content

fix: ignore recovery symbols for unrequested sliver indices - #3559

Open
halfprice wants to merge 1 commit into
mainfrom
fix/recovery-unrequested-sliver-index
Open

fix: ignore recovery symbols for unrequested sliver indices#3559
halfprice wants to merge 1 commit into
mainfrom
fix/recovery-unrequested-sliver-index

Conversation

@halfprice

Copy link
Copy Markdown
Collaborator

Summary

Fixes a targeted read-availability defect in the SDK sliver recovery fallback (recover_slivers in crates/walrus-sdk/src/node_client.rs).

The recovery fallback seeds its progress map (symbols_by_sliver) with exactly the sliver indices the client asked to recover, but process_decoding_symbols_result then merged every index a node returned via entry().or_insert_with(), without validating those indices against the request. The upstream layers (retrieve_decoding_symbols, list_decoding_symbols) return the node's response map verbatim, so the keys are fully attacker-controlled.

Impact

A single malicious or compromised storage node can return a syntactically valid response containing a sliver index the client never requested, paired with an empty symbol list. That unrequested key is admitted into the progress map with zero symbols. Because the completion check (slivers_needs_more_symbols) scans the entire map, it then treats recovery as perpetually incomplete and returns NotEnoughSymbolsToDecodeSliver — even after honest nodes have already supplied enough symbols for every sliver actually requested. The read fails.

This affects every SDK read path that enters the recovery fallback — byte-range reads, streaming reads, and quilt reads — and therefore the public aggregator that serves reads through the same code. An empty symbol list is the simplest trigger, but any unrequested index with fewer than the required symbols poisons the check identically.

Fix

Track the requested indices in a shared HashSet<SliverIndex> and skip (with a warning) any returned index not present in it, so unrequested keys can no longer be inserted into the progress map or stall the completion check.

Testing

  • Added recovery_ignores_symbols_for_unrequested_sliver_index, a regression test asserting that an unrequested index in a node response is neither inserted into the progress map nor reported by the completion check.
  • cargo nextest run -p walrus-sdk --features "test-utils walrus-sui/test-utils" recovery_ignores_symbols_for_unrequested_sliver_index — passes.
  • cargo clippy -p walrus-sdk --features "test-utils walrus-sui/test-utils" --tests — clean.
  • cargo fmt — clean.

The sliver recovery fallback in `recover_slivers` seeds its progress map
(`symbols_by_sliver`) with exactly the requested sliver indices, but
`process_decoding_symbols_result` then merged every index a node returned
via `entry().or_insert_with()`, without checking it against the request.

A single malicious or compromised storage node could return a
syntactically valid response containing a sliver index the client never
requested, paired with an empty symbol list. That unrequested key was
admitted into the progress map with zero symbols, and because the
completion check (`slivers_needs_more_symbols`) scans the entire map, it
would then report recovery as perpetually incomplete. Recovery returned
`NotEnoughSymbolsToDecodeSliver` even after honest nodes supplied enough
symbols for every sliver actually requested, failing the read.

This affects every SDK read path that enters the recovery fallback
(byte-range reads, streaming reads, and quilt reads), and therefore the
public aggregator that serves reads through the same code.

Fix: track the requested indices in a shared set and skip (with a
warning) any returned index not present in it, so unrequested keys can no
longer poison the completion check.
@github-actions

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open 14 days with no activity. It will be closed in 7 days unless you remove the stale label, add the do-not-close label, or comment on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant