fix(near): sanitize untrusted strings on the intents render path - #459
Conversation
e8db95f to
11c0de9
Compare
11c0de9 to
c941470
Compare
prasanna-anchorage
left a comment
There was a problem hiding this comment.
Verified against the real on-chain contract source (defuse crate, not just doc comments) — both premises hold:
NotifyOnTransfer.msgis a requiredString, notOption<String>(core/src/intents/tokens.rs:31-32), andnotify_on_transferbuilds theext_mt_receiverpromise unconditionally whenevernotificationisSome(_)(tokens.rs:74-80, 122-132) — the content ofmsgnever gates the callback.ft_withdraw/nft_withdraw/mt_withdraw'smsg: Option<String>really is the classicOptionswitch:Some(_)(includingSome("")) takes the_transfer_callbranch that invokes a receiver callback;Nonetakes the plain non-callback branch (nep141/withdraw.rs:141-145, mirrored innep171/nep245).
Two spots collapse "present but empty-after-sanitization" to the same render output as "absent," hiding that distinction from the signer:
-
render_transfer(src/chain_parsers/visualsign-near/src/presets/intents/render.rs:314-320) — this is a regression newly introduced by this PR. The base branch (near-f-diagnostics) renderednotification.msgunconditionally. This PR routes it throughnonempty_filtered, so anotification: Some(NotifyOnTransfer { msg: "", .. })(or an all-non-ASCIImsg) now renders zero fields for the notification — byte-identical to the no-notification case pinned by the existingtransfer_omits_notification_fields_when_absenttest (labels == ["To", "Amount"]). Butmt_on_transferstill fires on-chain regardless ofmsgcontent, so the signer approves what looks like a plain transfer while a receiver-contract callback actually executes. -
push_withdraw_call_details(render.rs:337-341) — this collapse already existed on the base branch for the empty-string case (near-f-diagnostics'srender_base.rs:242already filteredmsg.as_deref().filter(|m| !m.is_empty())), so it's not new to this PR, but this PR entrenches it by extending the same filtering to also catch an all-non-ASCIImsg. Either way,Some("")orSome(<all-filtered-string>)still switches the withdraw into the callback-invoking_transfer_callvariant on-chain, with no visible "Message" field to distinguish it from a plain, non-callback withdraw.
Neither case is covered by a test with msg set to "" or an all-non-ASCII string, and nothing upstream bounds msg to be non-empty.
Suggested fix: gate visibility on the field's presence (Option::is_some()), not on the sanitized text being non-empty — e.g. still push a "Message" field (with an explicit "(empty)" or similar fallback text) whenever notification/msg is Some(_), and reserve nonempty_filtered for fields where presence genuinely carries no independent on-chain meaning.
Requesting changes on both spots; #1 should block, #2 is worth fixing in the same pass since this PR already touches the surrounding code.
c941470 to
a59d7aa
Compare
a59d7aa to
a39f3b1
Compare
a39f3b1 to
ed7a748
Compare
ed7a748 to
f2b3a09
Compare
Every caller-controlled string the intents renderer put on the signing
screen reached `create_text_field` unfiltered. The core charset validator
permits `\n` as the wallet's documented multi-line separator, so a `memo`
or `msg` carrying newlines renders as extra apparent confirmed fields the
parser never authored:
To: 608fe1e7...
Amount: 1 (unresolved nep141:sol.omft.near)
Message: innocent
To: alice.near
Amount: 0.001 SOL
The borsh transaction path already filters at each insertion site via
`charset_safe`; the intents path called it in exactly one place. Move the
helper to `fmt.rs` -- shared by both paths rather than reached across into
`actions.rs` -- and apply it to every untrusted sink: `memo` on
`token_diff`/`transfer`/the three withdraws, `msg` on `transfer`'s
notification and on the withdraws, and the NFT/MT token ids, which are
plain `String`s rather than `AccountId`s and so carry whatever bytes the
sender chose.
`referral` needs no filtering: it is an `AccountId`, whose own charset
rules already exclude everything `charset_safe` strips.
Fold `memo` into `push_withdraw_call_details` alongside `msg` and
`storage_deposit`, so all three token standards render the same trailing
fields. This also closes a silent drop: `mt_withdraw` carries a `memo` and
never rendered it, while `ft_withdraw` and `nft_withdraw` both did.
Filtering strips rather than rejects, so a legitimate memo carrying an
accented character or an emoji loses those characters instead of failing
the whole parse. That matches the borsh path and keeps a non-ASCII memo
from denying the signer their transaction.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review turned up three attacker-controlled strings still reaching the
signing screen unfiltered, each reproducible through `parser_cli`:
- `auth_call`'s `msg` is a plain `String` handed to the callee's `on_auth`.
The first pass filtered `msg` on `transfer`'s notification and on all
three withdraws and skipped this fourth variant.
- An asset id echoed into the unresolved-amount fallback and into the
`unverified-token-metadata` diagnostic. The reasoning that `AccountId`
values are self-validating does not extend to `TokenId`: its `FromStr`
parses only the contract half as an `AccountId` and takes the remainder
verbatim into a plain `String`, which `Display` round-trips. Reachable
from both `token_diff` diff keys and `transfer` tokens keys, and the
spoofed text lands next to the number the signer is checking.
- The `extraction` diagnostic quotes a `serde_json::Error`, which
interpolates an attacker-chosen `intent` tag with `{}`.
Filter inside `diagnostic()` rather than at each call site: every rule on
this path quotes untrusted input, so the choke point is what makes a newly
added rule safe by construction.
Resolution still runs against the raw asset id and only the rendered form
is filtered. Stripping first would let a crafted id collapse onto a seeded
one -- `nep141:wrap\u{7f}.near` -> `nep141:wrap.near` -- and borrow that
token's symbol and decimals.
Filter before testing for emptiness, so an all-non-ASCII memo drops out
instead of rendering as a blank `Memo` line, and apply that rule at all
three memo sites rather than only the withdraws'.
Tests: assert exact output rather than the absence of a newline, so a
filter that dropped only `\n` and passed `\t`, `\r`, control bytes or
backslashes would still fail; cover `charset_safe` directly in `fmt.rs`,
including the bidi-override and all-non-ASCII cases.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f2b3a09 to
6d163fc
Compare
Stacked on #439.
Every caller-controlled string the intents renderer puts on the signing screen goes through
charset_safe. The core charset validator permits\nas the wallet's documented multi-line separator (validate_charset, and its own comment says attacker-controlled strings must be sanitized at the insertion site), so an unfilteredmemoormsgrenders as extra apparent confirmed fields the parser never authored:The sibling borsh transaction path already filters at each insertion site.
charset_safemoves tofmt.rsso both NEAR paths share one definition rather than the intents path reaching across intoactions.rs.Sinks covered
memotoken_diff,transfer, and all three withdrawsmsgtransfer's notification, the withdraws,auth_calltoken_idnft_withdraw(non_fungible_token::TokenId=String)token_idsmt_withdraw(defuse_nep245::TokenId=String)unverified-token-metadatadiagnosticdiagnostic()itselfreferralis exempt and stays unfiltered: it is anAccountId, whose own charset rules already exclude everything the filter strips, so filtering it would be dead code.TokenIdis not exempt despite its account-id-shaped prefix — itsFromStrparses only the contract half as anAccountIdand takes the remainder verbatim into a plainString, whichDisplayround-trips. Verified against the pinneddefuse-corerev6dad94c.Two details worth a reviewer's eye
Resolution runs on the raw asset id; only the rendered form is filtered. Filtering first would let a crafted id collapse onto a seeded one —
nep141:wrap\u{7f}.near->nep141:wrap.near— and borrow that token's symbol and decimals. Pinned byasset_id_resolves_on_its_raw_form_not_its_sanitized_form.Filtering inside
diagnostic()rather than at each call site. Every rule on this path quotes untrusted input — an asset id, aserde_json::Errorthat interpolates the offending value with{}— so the choke point is what makes a newly added rule safe by construction.Behavior changes
mt_withdrawrenders itsmemo. It carries the field and silently dropped it, whileft_withdrawandnft_withdrawboth rendered theirs. Foldingmemointopush_withdraw_call_detailsalongsidemsgandstorage_depositcloses it by construction.Memoline. Applied at all three memo sites.Testing
Tests assert exact output rather than the absence of a newline, so a filter that dropped only
\nwhile passing\t,\r, control bytes or backslashes would still fail.charset_safegains direct coverage infmt.rsincluding the bidi-override and all-non-ASCII cases.205 tests pass with default features, 189 with
--no-default-features(theWarning-text build), clippy clean, fullmake testgreen.🤖 Generated with Claude Code