feat(walletapi): mark and scrub unverified sender attribution for rings > 2#21
feat(walletapi): mark and scrub unverified sender attribution for rings > 2#21DHEBP wants to merge 5 commits into
Conversation
…anic The receiver decode paths used sender_idx <= RingSize against a 0-indexed Publickeylist of length RingSize, so a crafted byte equal to RingSize indexed one slot past the slice and panicked the receiving wallet during history processing. Tighten to < in both the CBOR and CBOR_V2 paths so valid indices are 0..RingSize-1.
Add SenderVerified and RingSize to rpc.Entry, populated in both receiver decode paths. SenderVerified is true only at ring size 2, where the receiver override pins attribution to the counterparty; for larger rings the sender chose the unauthenticated attribution byte, so entry.Sender must not be presented as trusted. RingSize is carried so consumers can render attribution confidence per entry.
…ry fields For ring sizes greater than 2 the receiver-decode attribution byte is sender-chosen and unauthenticated, yet entry.Sender was set from it and the raw byte was shipped in the exported entry.Data. Because the ring Publickeylist is public, a consumer could re-derive the claimed sender from entry.Data[0] even when entry.Sender was trusted, letting a sender name any ring member as the apparent sender of a payment they never made. Blank entry.Sender and zero the leading slot byte of the entry.Data copy when !SenderVerified, so an unverified attribution cannot be re-derived from any exported field. The verified (ring==2) path and the self/send decode sites are unchanged. Enforces the SenderVerified flag added in 1a0e0ba, which until now had no consumer. A source-level guard locks the scrub at both receiver decode paths so a regression fails loud. This is a decode-time scrub and does not retroactively clean entries persisted before the change.
DHEBP
changed the title
PR #21 — Six-Perspective ReviewMulti-perspective review (Product / Developer / QE / Security / DevOps / UI-UX). Every factual claim below was independently verification-checked and is tagged CONFIRMED, PLAUSIBLE, or REFUTED. Bottom lineRequest changes (small, well-scoped). The central question — when a sender attribution can't be authenticated, present a forgeable guess or refuse? — is answered correctly and at the right layer (the decode/export boundary, before any field leaves the wallet). The crypto reasoning holds: no exported field ( 🔴 Fix before merge1. Bounds fix checks the wrong collection — residual OOB panic. Security · medium · PLAUSIBLE — 2. Self/send transfers export 3. The only test executes no decode path; scrub behavior is untested. QE · high · CONFIRMED — 🟠 Address now via documentation (the PR's deferred items)4. Pre-upgrade persisted history is never re-scrubbed. DevOps · high · CONFIRMED — 5. Two operational corollaries of #4 DevOps · medium · CONFIRMED ×2:
6. 🟡 Track / minor (verified)
✅ Cleared (phantom concerns ruled out — don't re-flag)
Per-role summary
The design deserves to merge; it needs the |
…l scrub test Addresses the DEROFDN#21 six-perspective review: - Self-send sites set SenderVerified=true + RingSize (both decode arms): a wallet-authored tx has a certain sender at any ring size, so a contract- following consumer no longer distrusts the user's own sends. (review DEROFDN#2) - Add Test_Attribution_Scrub_Behavior: drives the real receiver decode on a sim chain and asserts the scrub effect (ring2 keeps Sender+Data[0]; ring>2 blanks Sender + zeroes Data[0]). Rebuilds until the receiver lands in a non-zero ring slot so the Data[0]==0 assertion has teeth. Mutation-checked. The source-grep guard is kept as a tripwire. (review DEROFDN#3) - Document the (RingSize, SenderVerified, Sender) renderer truth table on the field; drop the redundant exported_payload[:] slice. (review DEROFDN#6, DEROFDN#9) The <=->< bounds guard is sufficient: rpc.NewAddress panics on an undecompressable ring key before the Publickeylist rebuild, so len(Publickeylist)==RingSize holds and the index cannot go out of range. (review #1)
…anon Addresses the DEROFDN#22 review: - curatedRingCandidates canonicalizes the sender, recipient, and every preferred decoy to its base address before the distinctness/dedup checks, and the ring carries the base form. A ring member is identified by its pubkey, not its address string, so an integrated/payment-id encoding of an account already in the ring (sender, recipient, or another decoy) no longer slips the string-keyed checks and lands a duplicate pubkey that consensus rejects AFTER signing. Test_CuratedRing_AltEncoding_NoDuplicate_A2 covers recipient / sender / decoy-vs-decoy / cross-network, each mutation-proven. (review #1) - Anonymous attribution now fails closed at ring size < 4 (no decoy slot) instead of silently falling through to honest attribution. (review DEROFDN#8) - Port the unverified-attribution export scrub + self-send flag into the decode arms (this stack predates it); behavioral scrub test + an honest-byte guard that asserts honest mode writes the receiver slot (closes the grep guard's reassignment blind spot). (review DEROFDN#3, DEROFDN#9) The bounds guard is sufficient for the same reason as DEROFDN#21: bad ring keys panic at NewAddress before the Publickeylist rebuild, so len==RingSize. (review DEROFDN#4)
Add an advanced-only AttributionSelf mode that points the receiver-readable attribution byte at the sender's own ring slot (witness_index[0]) — the attribution field's original purpose, offered explicitly rather than by default. The receiver-pointed default stays untouched and un-overridable; self-pointing is reachable ONLY via the named AttributionSelf value. Engine (walletapi/transaction_build.go): - AttributionMode gains AttributionSelf; one build branch writes witness_index[0]. Works at any ring size (slot 0 always exists), so it is intentionally not subject to the anonymous ring<4 fail-closed guard. - Guardrail comment reframed: honest/default can never self-point; the sender slot is reachable only via the explicit value. CLI (cmd/dero-wallet-cli): - session attribution flag becomes a 3-way mode; menu option 2 cycles Default -> Anonymous -> Self -> Default. Landing on Self triggers a mandatory, permanent-self-doxx warning; declining leaves the mode unchanged. - Self bypasses the anonymous ring-size downgrade and is reported verbatim. The result line is ring-2-aware: at ring 2 the sender is already structurally provable (decode overrides sender_idx; SenderVerified=true for honest too), so the written byte adds nothing over honest; at ring>2 it is a permanent on-chain record recoverable by a non-blanking/future reader. - --anonymous seeds Anonymous only; Self is never reachable from a flag. Tests: - Test_AttributionSelf_WritesSenderSlot: build-path proof the byte points at the sender slot (mutation-proven: flip to [1] -> RED). - Test_AttributionSelf_EndToEnd_ScrubHolds: build->broadcast->mined->decode proof the DEROFDN#21 scrub still blanks the byte for a current wallet at ring>2. - 3-way cycle, downgrade-bypass, result-line, and label-suffix unit tests. The DEROFDN#21 scrub blanks the byte for current recipient wallets at ring>2 but does not make self "safe" — the raw on-chain byte is permanent. That permanence is why the warning is mandatory.
Add an advanced-only AttributionSelf mode that points the receiver-readable attribution byte at the sender's own ring slot (witness_index[0]) — the attribution field's original purpose, offered explicitly rather than by default. The receiver-pointed default stays untouched and un-overridable; self-pointing is reachable ONLY via the named AttributionSelf value. Engine (walletapi/transaction_build.go): - AttributionMode gains AttributionSelf; one build branch writes witness_index[0]. Works at any ring size (slot 0 always exists), so it is intentionally not subject to the anonymous ring<4 fail-closed guard. - Guardrail comment reframed: honest/default can never self-point; the sender slot is reachable only via the explicit value. CLI (cmd/dero-wallet-cli): - session attribution flag becomes a 3-way mode; menu option 2 cycles Default -> Anonymous -> Self -> Default. Landing on Self triggers a mandatory, permanent-self-doxx warning; declining leaves the mode unchanged. - Self bypasses the anonymous ring-size downgrade and is reported verbatim. The result line is ring-2-aware: at ring 2 the sender is already structurally provable (decode overrides sender_idx; SenderVerified=true for honest too), so the written byte adds nothing over honest; at ring>2 it is a permanent on-chain record recoverable by a non-blanking/future reader. - --anonymous seeds Anonymous only; Self is never reachable from a flag. Tests: - Test_AttributionSelf_WritesSenderSlot: build-path proof the byte points at the sender slot (mutation-proven: flip to [1] -> RED). - Test_AttributionSelf_EndToEnd_ScrubHolds: build->broadcast->mined->decode proof the DEROFDN#21 scrub still blanks the byte for a current wallet at ring>2. - 3-way cycle, downgrade-bypass, result-line, and label-suffix unit tests. The DEROFDN#21 scrub blanks the byte for current recipient wallets at ring>2 but does not make self "safe" — the raw on-chain byte is permanent. That permanence is why the warning is mandatory.
…etroactive scrub (review #1/DEROFDN#4/DEROFDN#5/DEROFDN#6) DEROFDN#6 — Entry.String() printed "Sender: %s" unconditionally: a scrubbed ring>2 transfer rendered a bare "Sender: " line that reads as a decode bug — the exact "looks broken" outcome the design meant to avoid, contradicting the truth table documented on SenderVerified in the same file. The Sender line now follows the table: verified -> the address; scrubbed (no payload error) -> "unknown (unverifiable, ring size N)"; decode failure -> a distinct "unknown (payload decode failed)". Blast radius: two CLI debug dumps (%+v in show_transfers error paths); JSON marshalling untouched. Pinned by rpc/wallet_rpc_test.go (offline, sub-second): all three arms, plus forbids the bare line and scrub/decode-failure conflation. DEROFDN#4/DEROFDN#5 — the non-retroactive caveat lived only in commit 4e9de56's message. Now in the docs consumers read: SenderVerified's doc comment states the scrub is decode-time only (pre-upgrade entries keep their guessed Sender and unscrubbed Data[0], served as-is by Get_Transfers; the guarantee holds only for blocks decoded post-upgrade), the upgrade is one-way for the privacy property, and pre-upgrade entries deserialize SenderVerified=false, RingSize=0 — indistinguishable from a scrub without re-decoding, so a migration MUST re-derive RingSize from chain data (a read-path scrub keyed on persisted fields would wrongly blank the wallet's own verified sends — why the rescan is a follow-up, not this commit). RingSize's doc gains the "0 = pre-upgrade/unknown, not a real ring" note; the GetTransfers handler doc points RPC consumers at the contract. #1 hardening — both decode arms bounded sender_idx against Statement.RingSize while indexing Statement.Publickeylist. Safe today (len(Publickeylist)==RingSize holds at that point), but the guard now bounds against the slice actually indexed, so it survives any future break of that invariant. Existing pins green: the scrub-behavior sim test and the source-grep guard (its regexes key on the scrub lines, untouched here).
|
Thanks — all tiers addressed: #2/#3 landed earlier in #2 (self-send distrust): fixed in #3 (grep-only test): fixed in #1 (bounds vs wrong collection): took the hardening — both decode arms now bound #4/#5 (non-retroactive caveat): in the docs consumers read now. #6 (bare Heads-up: #22/#23 fork from |
2 participants
The decrypted sender-index byte is sender-chosen and unauthenticated, so for any ring > 2 a sender can name the wrong member as the sender. This surfaces
SenderVerified(true only for the structural ring-2 case) andRingSizeonrpc.Entryso receivers can tell a structural attribution from a guess, and then acts on that flag: when!SenderVerifiedit blanksentry.Senderand zeroes the leading slot byte ofentry.Data, so an unverified attribution can't be re-derived from any exported field via the publicPublickeylist. The verified (ring-2) path and the self/send decode sites are unchanged; a source-level guard locks the scrub at both receiver paths. Pairs with #20 — together they close the receiver-side attribution finding.