Skip to content

fix(restore): bound blob processing + negative-cache permanent decrypt failures (GH #501) - #529

Closed
harrymove-ctrl wants to merge 2 commits into
devfrom
fix/bounded-restore-blob-processing
Closed

fix(restore): bound blob processing + negative-cache permanent decrypt failures (GH #501)#529
harrymove-ctrl wants to merge 2 commits into
devfrom
fix/bounded-restore-blob-processing

Conversation

@harrymove-ctrl

Copy link
Copy Markdown
Collaborator

Summary

restore() discovers Walrus Blob objects by current on-chain ownership only, never by who created/uploaded them. A blob transferred into a victim's wallet by an attacker (carrying memwal_* metadata) gets downloaded and SEAL-decrypt-attempted on every future restore() call, forever — a failed decrypt/validation was never recorded anywhere, so the same foreign blob keeps getting rediscovered and reprocessed indefinitely.

Per Henry's explicit direction on this thread (an uploader/relayer allowlist would break the self-host-then-migrate-to-managed-relayer path and is expensive/fragile to implement), this fixes it with bounded processing + rate limiting instead:

  • Negative cache: new restore_failed_blobs table. A blob that fails SEAL decrypt (classified via the existing DecryptOutcome::permanent_from_error) or UTF-8 validation for a given (owner, namespace, blob_id) is excluded from future restore() calls. Transient failures (timeouts, 429/503) are never cached, so an infra blip can't permanently blacklist a legitimate blob.
  • Clamped limit: RestoreRequest.limit is now capped at 100, matching /api/ask's existing convention. It was previously unbounded on the Rust side (the only real ceiling was an incidental sidecar constant unrelated to this parameter).
  • Owner-scoped rate limit: new check_restore_call_rate_limit (default 10/min/owner), reusing the existing Redis sliding-window primitive in rate_limit.rs, on top of the generic account limiter — bounds how often an attacker can force a fresh first-time-discovery cost by repeatedly transferring junk blob_ids into a victim's wallet.
  • Fixed a pre-existing bug found during review: permanent_from_error's timeout check matched the sidecar's timeoutMs= config echo (present on every composed error), so it misclassified essentially all real decrypt failures — including the exact NoAccessError this fix depends on — as transient. Tightened the timeout signal and added NoAccessError to the permanent list, with tests pinning the actual wire format.

None of this inspects, records, or restricts who uploaded/created/relayed a blob — it's keyed purely by (owner, blob_id) outcome and owner-level call frequency. The self-host-then-migrate-to-managed-relayer path is untouched.

Fixes WALM-299 / GH #501.

Test plan

  • cargo build — clean
  • cargo test --bin memwal-server — 412 passed, 5 failed (pre-existing, unrelated jobs::tests::*, fail only due to no local Postgres in this sandbox — PoolTimedOut), 31 ignored
  • New unit tests for the negative-cache round-trip, the NoAccessError/timeout classifier fix, and the limit clamp all pass
  • DB-gated and Redis-gated integration tests could not be exercised locally (no live Postgres/Redis in this environment) — need a run against a real dev DB/Redis before merge
  • Manual PoC replay against the original WALM-299 repro (attacker blob transferred to victim, victim restores twice) not yet performed

hien-p added 2 commits August 4, 2026 12:22
…ailures (GH #501)

restore() discovered blob_ids purely by current on-chain ownership, so a
Walrus Blob transferred into a victim's wallet by an attacker (carrying
memwal_* metadata) got downloaded and SEAL-decrypt-attempted on every
single future restore() call, forever, since a failed decrypt/validation
was never recorded anywhere. Per Henry's explicit rejection of an
uploader/relayer allowlist fix for this issue, this instead:

- Negative-caches permanently-failed (owner, namespace, blob_id) attempts
  in a new restore_failed_blobs table, so a foreign blob that fails SEAL
  decrypt (classified via the existing DecryptOutcome::permanent_from_error)
  or UTF-8 validation is excluded from future restore() calls. Transient
  failures (SEAL timeouts, 429/503) are never cached, so an infra blip
  can't permanently blacklist a legitimate blob.
- Clamps RestoreRequest.limit to 100, matching /api/ask's existing cap.
- Adds an owner-scoped restore call-frequency guard (default 10/min) on
  top of the generic account rate limiter, to bound how often an attacker
  can force a fresh first-time-discovery cost by repeatedly transferring
  junk blob_ids into a victim's wallet.

None of this inspects who uploaded, created, or relayed a blob -- it's
keyed purely by (owner, blob_id) outcome and owner-level call frequency,
so the self-host-then-migrate-to-managed-relayer path is untouched.
…ure as transient

sendSealFailure (sidecar) echoes the configured request timeout as
'(traceId=..., timeoutMs=...)' on every composed error, permanent or
not. The classifier's bare lower.contains("timeout") check matched
that config echo unconditionally, so it returned transient for every
real error from /seal/decrypt before the permanent-error branch was
ever reached -- including NoAccessError, the exact failure GH #501's
bounded-restore fix depends on catching to negative-cache a foreign,
undecryptable blob.

Match the actual timeout signal (TimeoutError / "aborted due to
timeout") instead of the bare substring, and add NoAccessError to the
permanent list. Both existing classifier tests used simplified
messages that never exercised the real wire format, so they passed
despite the bug; added tests pin the actual composed shape for both
the no-access and genuine-timeout cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants