Skip to content

Fix: origin-rewriting wrappers bypass high-security restrictions#610

Open
n13 wants to merge 3 commits into
mainfrom
high_security_fix
Open

Fix: origin-rewriting wrappers bypass high-security restrictions#610
n13 wants to merge 3 commits into
mainfrom
high_security_fix

Conversation

@n13

@n13 n13 commented Jun 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

High-security (HS) accounts are meant to be limited to whitelisted calls (e.g. reversible schedule_transfer/cancel). They could be bypassed: Recovery::as_recovered and Utility::as_derivative synthesize a fresh Signed origin after top-level transaction validation, so a non-HS outer signer could dispatch a non-whitelisted call as an HS account — including when nested under batch/batch_all/force_batch/if_else.

This PR enforces the HS whitelist at the effective dispatched origin, entirely at the runtime/extension layer:

  • HighSecurityConfig::call_allowed_for recursively resolves origin-rewriting wrappers (as_recovered/as_derivative) and origin-preserving combinators (batch*/if_else), re-checking the whitelist at each effective signer. Root-only dispatch_as/with_weight and the scheduler are intentionally not traversed.
  • Shared via a new HighSecurityInspector::is_call_allowed provided method, so ReversibleTransactionExtension and pallet-multisig use one implementation (no duplicated logic).
  • Recursion is bounded and fail-closed (MAX_CALL_DEPTH = 16, well below MAX_EXTRINSIC_DEPTH = 256): over-nested calls are rejected rather than allowed to escape the whitelist, so validation work can't be exhausted.
  • The extension weight() scales with the number of nodes traversed (one is_high_security read per node).
  • pallet-recovery and pallet-utility are left unchanged from upstream Substrate.

Test plan

  • cargo test -p quantus-runtime --lib transaction_extensions::tests — origin-rewriter bypass blocked, whitelisted inner call allowed, nested-in-batch bypass blocked, non-HS origin unaffected, over-nested call rejected fail-closed
  • cargo test -p pallet-multisig
  • cargo test -p pallet-reversible-transfers
  • Runtime integration tests (recovery / reversible)

n13 added 2 commits June 29, 2026 14:50
for all origin-altering calls
Bound `HighSecurityConfig::call_allowed_for` to a fixed nesting depth
(MAX_CALL_DEPTH = 16) and fail closed beyond it, so deeply nested
origin-rewriting/combinator calls cannot exhaust transaction-validation
work. Adds a regression test.

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: Approve — no remaining blockers from this review.

Update: the only blocker from my earlier review (the failing Format check) is resolved. Commit e196524a ("format") makes the Fast Checks (Format) job pass, and git show -w confirms it is whitespace-only (rustfmt line-reflow in runtime/src/transaction_extensions.rsweight(), validate_with(), and two test constructors), so the logic and tests validated earlier are unaffected. (Build & Test / Clippy & Doc were still running at review time — merge once they go green.)

Effective-origin high-security enforcement (reconfirmed):

  • The recursive resolver re-checks the whitelist at the effective dispatched origin, covering the origin-rewriting wrappers (Recovery::as_recovered, Utility::as_derivative) and the origin-preserving combinators (batch / batch_all / force_batch / if_else).
  • The remaining pallet-utility origin-altering calls — dispatch_as, with_weight, dispatch_as_fallible — are all ensure_root, so they are not an unprivileged bypass and correctly are not traversed. The scheduler's extrinsics are disabled.
  • ReversibleTransactionExtension and pallet-multisig share the single HighSecurityInspector::is_call_allowed implementation (no duplicated logic); multisig re-checks at execute time against the multisig address.
  • Recursion is bounded (MAX_CALL_DEPTH = 16) and fails closed, well under the MAX_EXTRINSIC_DEPTH = 256 decode limit; weight() scales with the number of traversed nodes.
  • Recovery address handling is safe: this runtime sets type Lookup = AccountIdLookup, which only resolves MultiAddress::Id, so the non-Id arm cannot dispatch and is not an alternate-encoding bypass.

No security or logic blocker found.

Local tests (still valid — the format commit is whitespace-only):

  • cargo test -p quantus-runtime --lib transaction_extensions::tests passed
  • cargo test -p pallet-multisig passed
  • cargo test -p pallet-reversible-transfers passed

@n13 n13 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated verdict after the format commit: previous blocker cleared.

The new format commit fixed the issue I flagged; GitHub's Fast Checks (Format) job is now passing. My code-review verdict is approve/merge-ready from the effective-origin high-security enforcement perspective.

Remaining note: the build/test matrix and clippy/doc jobs are still running on the latest commit, so I would wait for those checks to finish green before merging.

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.

1 participant